Re: [PATCH v2 4/6] regulator: Add support for QCOM PMIC VBUS booster
On 6/15/2020 5:00 AM, Mark Brown wrote:
> On Fri, Jun 12, 2020 at 04:19:16PM -0700, Wesley Cheng wrote:
>
>> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
>> @@ -0,0 +1,147 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + */
>
> Please make the entire comment a C++ one so things look more
> intentional.
>
Hi Mark,
Sure, will do.
>> +static int qcom_usb_vbus_enable(struct regulator_dev *rdev)
>> +{
>
>> +static int qcom_usb_vbus_disable(struct regulator_dev *rdev)
>> +{
>
>> +static int qcom_usb_vbus_is_enabled(struct regulator_dev *rdev)
>> +{
>
> These operations can all be replaced by regulator_is_enabled_regmap()
> and friends.
>
Got it. This simplifies the driver a lot. Thanks for the tip.
>> +init_data.constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS;
>
> No, this is broken - regulators should not override the constraints the
> machine sets.
>
Understood. I decided to go with of_get_regulator_init_data() to
initialize the init_data parameter. This should take care of the
constraint settings.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v2 4/6] regulator: Add support for QCOM PMIC VBUS booster
On 6/12/2020 8:28 PM, Randy Dunlap wrote: > On 6/12/20 4:19 PM, Wesley Cheng wrote: >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index 074a2ef55943..f9165f9f9051 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -797,6 +797,16 @@ config REGULATOR_QCOM_SPMI >>Qualcomm SPMI PMICs as a module. The module will be named >>"qcom_spmi-regulator". >> >> +config REGULATOR_QCOM_USB_VBUS >> +tristate "Qualcomm USB Vbus regulator driver" >> +depends on SPMI || COMPILE_TEST >> +help >> + If you say yes to this option, support will be included for the >> + regulator used to enable the VBUS output. >> + >> + Say M here if you want to include support for enabling the VBUS output >> + as a module. The module will be named "qcom_usb-regulator". > > Hi, > Shouldn't that module name match what is in the Makefile? > > Thanks, Randy. Missed this as I was going back and forth on the file name. Thanks for the catch. >> + >> config REGULATOR_RC5T583 >> tristate "RICOH RC5T583 Power regulators" >> depends on MFD_RC5T583 >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >> index c0d6b96ebd78..cbab28aa7b56 100644 >> --- a/drivers/regulator/Makefile >> +++ b/drivers/regulator/Makefile >> @@ -89,6 +89,7 @@ obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o >> obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o >> obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o >> obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o >> +obj-$(CONFIG_REGULATOR_QCOM_USB_VBUS) += qcom_usb_vbus-regulator.o >> obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o >> obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o >> obj-$(CONFIG_REGULATOR_PV88060) += pv88060-regulator.o > > > thanks. > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 4/6] regulator: Add support for QCOM PMIC VBUS booster
On Fri, Jun 12, 2020 at 04:19:16PM -0700, Wesley Cheng wrote:
> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
Please make the entire comment a C++ one so things look more
intentional.
> +static int qcom_usb_vbus_enable(struct regulator_dev *rdev)
> +{
> +static int qcom_usb_vbus_disable(struct regulator_dev *rdev)
> +{
> +static int qcom_usb_vbus_is_enabled(struct regulator_dev *rdev)
> +{
These operations can all be replaced by regulator_is_enabled_regmap()
and friends.
> + init_data.constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS;
No, this is broken - regulators should not override the constraints the
machine sets.
signature.asc
Description: PGP signature
Re: [PATCH v2 4/6] regulator: Add support for QCOM PMIC VBUS booster
On 6/12/20 4:19 PM, Wesley Cheng wrote: > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index 074a2ef55943..f9165f9f9051 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -797,6 +797,16 @@ config REGULATOR_QCOM_SPMI > Qualcomm SPMI PMICs as a module. The module will be named > "qcom_spmi-regulator". > > +config REGULATOR_QCOM_USB_VBUS > + tristate "Qualcomm USB Vbus regulator driver" > + depends on SPMI || COMPILE_TEST > + help > + If you say yes to this option, support will be included for the > + regulator used to enable the VBUS output. > + > + Say M here if you want to include support for enabling the VBUS output > + as a module. The module will be named "qcom_usb-regulator". Hi, Shouldn't that module name match what is in the Makefile? > + > config REGULATOR_RC5T583 > tristate "RICOH RC5T583 Power regulators" > depends on MFD_RC5T583 > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index c0d6b96ebd78..cbab28aa7b56 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -89,6 +89,7 @@ obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o > obj-$(CONFIG_REGULATOR_QCOM_SPMI) += qcom_spmi-regulator.o > +obj-$(CONFIG_REGULATOR_QCOM_USB_VBUS) += qcom_usb_vbus-regulator.o > obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o > obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o > obj-$(CONFIG_REGULATOR_PV88060) += pv88060-regulator.o thanks. -- ~Randy
