diff mbox series

[5/7] power: supply: bq25890: Factor out regulator registration code

Message ID 20221010210310.165461-5-marex@denx.de (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [1/7] power: supply: bq25890: Document POWER_SUPPLY_PROP_CURRENT_NOW | expand

Commit Message

Marek Vasut Oct. 10, 2022, 9:03 p.m. UTC
Pull the regulator registration code into separate function, so it can
be extended to register more regulators later. Currently this is only
moving ifdeffery into one place and other preparatory changes. The
dev_err_probe() output string is changed to explicitly list vbus
regulator failure, so that once more regulators are registered, it
would be clear which one failed.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
To: linux-pm@vger.kernel.org
---
 drivers/power/supply/bq25890_charger.c | 51 ++++++++++++++++++--------
 1 file changed, 35 insertions(+), 16 deletions(-)

Comments

Hans de Goede Oct. 11, 2022, 8:20 a.m. UTC | #1
Hi,

On 10/10/22 23:03, Marek Vasut wrote:
> Pull the regulator registration code into separate function, so it can
> be extended to register more regulators later. Currently this is only
> moving ifdeffery into one place and other preparatory changes. The
> dev_err_probe() output string is changed to explicitly list vbus
> regulator failure, so that once more regulators are registered, it
> would be clear which one failed.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

First of all thank you for your work on this series. Based purely
on reading the commit messages patches 1-4 sound good to me. I will
do a more detailed review tomorrow.

As for patch 5-7 thinking some more about adding a Vsys regulator
just to report the Vsys reading feels wrong to me.

A regulator device's voltage in sysfs is about the value the regulator
is supposed to try and regulate its outputted voltage to, while here
we are talking about an ADC reading of the actual outputted voltage.

This really should *not* be modeled as a regulator if anything the
hwmon interface would be applicable for this ADC reading and
the power_supply core has support for exporting some of
the psy info through hwmon now.

So what should happen for Vsys IMHO is make it a new
POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
for this new property to the power-supply core, also make the core's
hwmon glue code export this in the registered hwmon device so that
e.g. a sensors applet on the desktop can easily show it (*).

Sorry for the confusion with my ack in the other thread which
only meant to agree with a part of the alinea/sentence I put
the ack under.

Regards,

Hans


*) As part of this the hwmon glue should probably also add
labels (resulting in in#_label sysfs files) to the psy
properties mirrored there so that it is clear which in#_input
value in sysfs represents what. You can simply check this
works as it should by running the "sensors" utility
which will print the labels if labels have been provided.



> ---
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> To: linux-pm@vger.kernel.org
> ---
>  drivers/power/supply/bq25890_charger.c | 51 ++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 6cc3c23cd8853..7ab27a9dce14a 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1114,6 +1114,36 @@ static const struct regulator_desc bq25890_vbus_desc = {
>  	.fixed_uV = 5000000,
>  	.n_voltages = 1,
>  };
> +
> +static int bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
> +	struct regulator_config cfg = {
> +		.dev = bq->dev,
> +		.driver_data = bq,
> +	};
> +	struct regulator_dev *reg;
> +
> +	if (!IS_ERR_OR_NULL(bq->usb_phy))
> +		return 0;
> +
> +	if (pdata)
> +		cfg.init_data = pdata->regulator_init_data;
> +
> +	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
> +	if (IS_ERR(reg)) {
> +		return dev_err_probe(bq->dev, PTR_ERR(reg),
> +				     "registering vbus regulator");
> +	}
> +
> +	return 0;
> +}
> +#else
> +static inline int
> +bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	return 0
> +}
>  #endif
>  
>  static int bq25890_get_chip_version(struct bq25890_device *bq)
> @@ -1309,27 +1339,16 @@ static int bq25890_probe(struct i2c_client *client,
>  
>  	/* OTG reporting */
>  	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +
> +	ret = bq25890_register_regulator(bq);
> +	if (ret)
> +		return ret;
> +
>  	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
>  		INIT_WORK(&bq->usb_work, bq25890_usb_work);
>  		bq->usb_nb.notifier_call = bq25890_usb_notifier;
>  		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
>  	}
> -#ifdef CONFIG_REGULATOR
> -	else {
> -		struct bq25890_platform_data *pdata = dev_get_platdata(dev);
> -		struct regulator_config cfg = { };
> -		struct regulator_dev *reg;
> -
> -		cfg.dev = dev;
> -		cfg.driver_data = bq;
> -		if (pdata)
> -			cfg.init_data = pdata->regulator_init_data;
> -
> -		reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
> -		if (IS_ERR(reg))
> -			return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
> -	}
> -#endif
>  
>  	ret = bq25890_power_supply_init(bq);
>  	if (ret < 0) {
Marek Vasut Oct. 11, 2022, 4:39 p.m. UTC | #2
On 10/11/22 10:20, Hans de Goede wrote:
> Hi,

Hi,

> On 10/10/22 23:03, Marek Vasut wrote:
>> Pull the regulator registration code into separate function, so it can
>> be extended to register more regulators later. Currently this is only
>> moving ifdeffery into one place and other preparatory changes. The
>> dev_err_probe() output string is changed to explicitly list vbus
>> regulator failure, so that once more regulators are registered, it
>> would be clear which one failed.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> First of all thank you for your work on this series. Based purely
> on reading the commit messages patches 1-4 sound good to me. I will
> do a more detailed review tomorrow.
> 
> As for patch 5-7 thinking some more about adding a Vsys regulator
> just to report the Vsys reading feels wrong to me.
> 
> A regulator device's voltage in sysfs is about the value the regulator
> is supposed to try and regulate its outputted voltage to, while here
> we are talking about an ADC reading of the actual outputted voltage.
> 
> This really should *not* be modeled as a regulator if anything the
> hwmon interface would be applicable for this ADC reading and
> the power_supply core has support for exporting some of
> the psy info through hwmon now.
> 
> So what should happen for Vsys IMHO is make it a new
> POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
> for this new property to the power-supply core, also make the core's
> hwmon glue code export this in the registered hwmon device so that
> e.g. a sensors applet on the desktop can easily show it (*).
> 
> Sorry for the confusion with my ack in the other thread which
> only meant to agree with a part of the alinea/sentence I put
> the ack under.

I'm not sure that's all there is to the Vsys regulator, it would let us 
model the connection between the charger chip and PMIC, where the 
charger would be the supply and the PMIC the regulator consumer. If the 
PMIC can determine its input voltage, it might be able to configure 
itself to some more optimal mode of operation. With the Vsys regulator, 
the PMIC can determine its voltage. So I think the Vsys regulator would 
be useful in that scenario (that's how it is wired on my board btw.).
Hans de Goede Oct. 12, 2022, 10:10 a.m. UTC | #3
Hi Marek,

On 10/11/22 18:39, Marek Vasut wrote:
> On 10/11/22 10:20, Hans de Goede wrote:
>> Hi,
> 
> Hi,
> 
>> On 10/10/22 23:03, Marek Vasut wrote:
>>> Pull the regulator registration code into separate function, so it can
>>> be extended to register more regulators later. Currently this is only
>>> moving ifdeffery into one place and other preparatory changes. The
>>> dev_err_probe() output string is changed to explicitly list vbus
>>> regulator failure, so that once more regulators are registered, it
>>> would be clear which one failed.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>
>> First of all thank you for your work on this series. Based purely
>> on reading the commit messages patches 1-4 sound good to me. I will
>> do a more detailed review tomorrow.
>>
>> As for patch 5-7 thinking some more about adding a Vsys regulator
>> just to report the Vsys reading feels wrong to me.
>>
>> A regulator device's voltage in sysfs is about the value the regulator
>> is supposed to try and regulate its outputted voltage to, while here
>> we are talking about an ADC reading of the actual outputted voltage.
>>
>> This really should *not* be modeled as a regulator if anything the
>> hwmon interface would be applicable for this ADC reading and
>> the power_supply core has support for exporting some of
>> the psy info through hwmon now.
>>
>> So what should happen for Vsys IMHO is make it a new
>> POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
>> for this new property to the power-supply core, also make the core's
>> hwmon glue code export this in the registered hwmon device so that
>> e.g. a sensors applet on the desktop can easily show it (*).
>>
>> Sorry for the confusion with my ack in the other thread which
>> only meant to agree with a part of the alinea/sentence I put
>> the ack under.
> 
> I'm not sure that's all there is to the Vsys regulator, it would let us model the connection between the charger chip and PMIC, where the charger would be the supply and the PMIC the regulator consumer. If the PMIC can determine its input voltage, it might be able to configure itself to some more optimal mode of operation. With the Vsys regulator, the PMIC can determine its voltage. So I think the Vsys regulator would be useful in that scenario (that's how it is wired on my board btw.).

You are right that theoretically there is nothing wrong with
the model of having the charger-IC's Vsys output being
a perent regulator for the PMIC.

As for this being useful to actually have I have my doubts
though. All the PMICs I know automatically select the
optimal mode/parameters for their buck convertors based
on the detected input voltage. So they basically always
work optimally as long as the input voltage is within
their supported range. So having Vsys moduled as
a regulator is only theoretically useful which is not
a good argument for adding this to the kernel in this
way IMHO. 

I believe it is important to go back to the original
problem / question which we are trying to solve / answer
here, which is:

"what is the best way to make the readings from
the Vsys ADC available to userspace?"

Looking at standard Linux userspace (Debian/Fedora/etc.)
all the userspace tools capable of reporting voltages
of various voltage rails inside the system to the user
use the hwmon interface for this. This is also why
the power-supply class core recently got support for
proxying some psy properties to a hwmon class device.

So the way I see it is that if we want to report Vsys to
userspace, that we then clearly need to report it through
the hwmon interface.

And my previous proposal:

make it a new POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while
adding support for this new property to the power-supply core,
also make the core's hwmon glue code export this in
the registered hwmon device.

Still seems the best solution to me here.

Regards,

Hans
Hans de Goede Oct. 12, 2022, 10:13 a.m. UTC | #4
Hi again ...

On 10/12/22 12:10, Hans de Goede wrote:
> Hi Marek,
> 
> On 10/11/22 18:39, Marek Vasut wrote:
>> On 10/11/22 10:20, Hans de Goede wrote:
>>> Hi,
>>
>> Hi,
>>
>>> On 10/10/22 23:03, Marek Vasut wrote:
>>>> Pull the regulator registration code into separate function, so it can
>>>> be extended to register more regulators later. Currently this is only
>>>> moving ifdeffery into one place and other preparatory changes. The
>>>> dev_err_probe() output string is changed to explicitly list vbus
>>>> regulator failure, so that once more regulators are registered, it
>>>> would be clear which one failed.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> First of all thank you for your work on this series. Based purely
>>> on reading the commit messages patches 1-4 sound good to me. I will
>>> do a more detailed review tomorrow.
>>>
>>> As for patch 5-7 thinking some more about adding a Vsys regulator
>>> just to report the Vsys reading feels wrong to me.
>>>
>>> A regulator device's voltage in sysfs is about the value the regulator
>>> is supposed to try and regulate its outputted voltage to, while here
>>> we are talking about an ADC reading of the actual outputted voltage.
>>>
>>> This really should *not* be modeled as a regulator if anything the
>>> hwmon interface would be applicable for this ADC reading and
>>> the power_supply core has support for exporting some of
>>> the psy info through hwmon now.
>>>
>>> So what should happen for Vsys IMHO is make it a new
>>> POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
>>> for this new property to the power-supply core, also make the core's
>>> hwmon glue code export this in the registered hwmon device so that
>>> e.g. a sensors applet on the desktop can easily show it (*).
>>>
>>> Sorry for the confusion with my ack in the other thread which
>>> only meant to agree with a part of the alinea/sentence I put
>>> the ack under.
>>
>> I'm not sure that's all there is to the Vsys regulator, it would let us model the connection between the charger chip and PMIC, where the charger would be the supply and the PMIC the regulator consumer. If the PMIC can determine its input voltage, it might be able to configure itself to some more optimal mode of operation. With the Vsys regulator, the PMIC can determine its voltage. So I think the Vsys regulator would be useful in that scenario (that's how it is wired on my board btw.).
> 
> You are right that theoretically there is nothing wrong with
> the model of having the charger-IC's Vsys output being
> a perent regulator for the PMIC.
> 
> As for this being useful to actually have I have my doubts
> though. All the PMICs I know automatically select the
> optimal mode/parameters for their buck convertors based
> on the detected input voltage. So they basically always
> work optimally as long as the input voltage is within
> their supported range. So having Vsys moduled as
> a regulator is only theoretically useful which is not
> a good argument for adding this to the kernel in this
> way IMHO. 
> 
> I believe it is important to go back to the original
> problem / question which we are trying to solve / answer
> here, which is:
> 
> "what is the best way to make the readings from
> the Vsys ADC available to userspace?"
> 
> Looking at standard Linux userspace (Debian/Fedora/etc.)
> all the userspace tools capable of reporting voltages
> of various voltage rails inside the system to the user
> use the hwmon interface for this. This is also why
> the power-supply class core recently got support for
> proxying some psy properties to a hwmon class device.
> 
> So the way I see it is that if we want to report Vsys to
> userspace, that we then clearly need to report it through
> the hwmon interface.
> 
> And my previous proposal:
> 
> make it a new POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while
> adding support for this new property to the power-supply core,
> also make the core's hwmon glue code export this in
> the registered hwmon device.
> 
> Still seems the best solution to me here.

p.s.

I guess we could also add both the new property and register
Vsys as a regulator, but registering a regulator without any
actual consumers seems superfluous.

Regards,

Hans
Hans de Goede Oct. 12, 2022, 7:59 p.m. UTC | #5
Hi,

On 10/10/22 23:03, Marek Vasut wrote:
> Pull the regulator registration code into separate function, so it can
> be extended to register more regulators later. Currently this is only
> moving ifdeffery into one place and other preparatory changes. The
> dev_err_probe() output string is changed to explicitly list vbus
> regulator failure, so that once more regulators are registered, it
> would be clear which one failed.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

As discussed in the other thread I don't really see the
added value of exporting Vsys as a regulator (also see my
latest reply in the other thread).

So I don't plan to review patches 5-7 atm.

Regards,

Hans



> ---
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> To: linux-pm@vger.kernel.org
> ---
>  drivers/power/supply/bq25890_charger.c | 51 ++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 6cc3c23cd8853..7ab27a9dce14a 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1114,6 +1114,36 @@ static const struct regulator_desc bq25890_vbus_desc = {
>  	.fixed_uV = 5000000,
>  	.n_voltages = 1,
>  };
> +
> +static int bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
> +	struct regulator_config cfg = {
> +		.dev = bq->dev,
> +		.driver_data = bq,
> +	};
> +	struct regulator_dev *reg;
> +
> +	if (!IS_ERR_OR_NULL(bq->usb_phy))
> +		return 0;
> +
> +	if (pdata)
> +		cfg.init_data = pdata->regulator_init_data;
> +
> +	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
> +	if (IS_ERR(reg)) {
> +		return dev_err_probe(bq->dev, PTR_ERR(reg),
> +				     "registering vbus regulator");
> +	}
> +
> +	return 0;
> +}
> +#else
> +static inline int
> +bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	return 0
> +}
>  #endif
>  
>  static int bq25890_get_chip_version(struct bq25890_device *bq)
> @@ -1309,27 +1339,16 @@ static int bq25890_probe(struct i2c_client *client,
>  
>  	/* OTG reporting */
>  	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +
> +	ret = bq25890_register_regulator(bq);
> +	if (ret)
> +		return ret;
> +
>  	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
>  		INIT_WORK(&bq->usb_work, bq25890_usb_work);
>  		bq->usb_nb.notifier_call = bq25890_usb_notifier;
>  		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
>  	}
> -#ifdef CONFIG_REGULATOR
> -	else {
> -		struct bq25890_platform_data *pdata = dev_get_platdata(dev);
> -		struct regulator_config cfg = { };
> -		struct regulator_dev *reg;
> -
> -		cfg.dev = dev;
> -		cfg.driver_data = bq;
> -		if (pdata)
> -			cfg.init_data = pdata->regulator_init_data;
> -
> -		reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
> -		if (IS_ERR(reg))
> -			return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
> -	}
> -#endif
>  
>  	ret = bq25890_power_supply_init(bq);
>  	if (ret < 0) {
Marek Vasut Oct. 12, 2022, 10:02 p.m. UTC | #6
On 10/12/22 12:10, Hans de Goede wrote:
> Hi Marek,

Hello Hans,

>>> On 10/10/22 23:03, Marek Vasut wrote:
>>>> Pull the regulator registration code into separate function, so it can
>>>> be extended to register more regulators later. Currently this is only
>>>> moving ifdeffery into one place and other preparatory changes. The
>>>> dev_err_probe() output string is changed to explicitly list vbus
>>>> regulator failure, so that once more regulators are registered, it
>>>> would be clear which one failed.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>
>>> First of all thank you for your work on this series. Based purely
>>> on reading the commit messages patches 1-4 sound good to me. I will
>>> do a more detailed review tomorrow.
>>>
>>> As for patch 5-7 thinking some more about adding a Vsys regulator
>>> just to report the Vsys reading feels wrong to me.
>>>
>>> A regulator device's voltage in sysfs is about the value the regulator
>>> is supposed to try and regulate its outputted voltage to, while here
>>> we are talking about an ADC reading of the actual outputted voltage.
>>>
>>> This really should *not* be modeled as a regulator if anything the
>>> hwmon interface would be applicable for this ADC reading and
>>> the power_supply core has support for exporting some of
>>> the psy info through hwmon now.
>>>
>>> So what should happen for Vsys IMHO is make it a new
>>> POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
>>> for this new property to the power-supply core, also make the core's
>>> hwmon glue code export this in the registered hwmon device so that
>>> e.g. a sensors applet on the desktop can easily show it (*).
>>>
>>> Sorry for the confusion with my ack in the other thread which
>>> only meant to agree with a part of the alinea/sentence I put
>>> the ack under.
>>
>> I'm not sure that's all there is to the Vsys regulator, it would let us model the connection between the charger chip and PMIC, where the charger would be the supply and the PMIC the regulator consumer. If the PMIC can determine its input voltage, it might be able to configure itself to some more optimal mode of operation. With the Vsys regulator, the PMIC can determine its voltage. So I think the Vsys regulator would be useful in that scenario (that's how it is wired on my board btw.).
> 
> You are right that theoretically there is nothing wrong with
> the model of having the charger-IC's Vsys output being
> a perent regulator for the PMIC.
> 
> As for this being useful to actually have I have my doubts
> though. All the PMICs I know automatically select the
> optimal mode/parameters for their buck convertors based
> on the detected input voltage. So they basically always
> work optimally as long as the input voltage is within
> their supported range. So having Vsys moduled as
> a regulator is only theoretically useful which is not
> a good argument for adding this to the kernel in this
> way IMHO.

I would be careful with "All the PMICs" , not all of them are fully 
automatic, some are just broken or partly automatic. i.MX28 POWER unit 
and one of the Dialog 9062 I think come to mind.

I also think if we have a supplier (bq25890) -> consumer (pmic input) 
relationship in hardware between two chips, we should model it using 
regulators. This is the common approach on embedded systems, so I don't 
see why we shouldn't do it here the same way?

> I believe it is important to go back to the original
> problem / question which we are trying to solve / answer
> here, which is:
> 
> "what is the best way to make the readings from
> the Vsys ADC available to userspace?"

Actually, I don't particularly care about exposing Vsys to user space. 
We are already changing the ABI with these first four patches, and the 
Vsys is no longer reported with 1..4 applied, so shall we change the 
question to:
"
Do we care about making Vsys reading available to userspace at all ?
"

> Looking at standard Linux userspace (Debian/Fedora/etc.)
> all the userspace tools capable of reporting voltages
> of various voltage rails inside the system to the user
> use the hwmon interface for this. This is also why
> the power-supply class core recently got support for
> proxying some psy properties to a hwmon class device.
> 
> So the way I see it is that if we want to report Vsys to
> userspace, that we then clearly need to report it through
> the hwmon interface.

For regulators, you can read their values via /sys/class/regulators/* , 
if the user is interesting in Vsys .

Maybe that is where we disagree -- I'm not particularly interested in 
exposing Vsys to user space, but since it was exposed before, I tried to 
retain that exposure, although via different channel. And the regulator 
also makes the Vsys useful, since it can be used as a supply on the 
kernel level.

> And my previous proposal:
> 
> make it a new POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while
> adding support for this new property to the power-supply core,
> also make the core's hwmon glue code export this in
> the registered hwmon device.
> 
> Still seems the best solution to me here.

[...]
Michał Mirosław Oct. 12, 2022, 10:22 p.m. UTC | #7
On Thu, Oct 13, 2022 at 12:02:52AM +0200, Marek Vasut wrote:
> On 10/12/22 12:10, Hans de Goede wrote:
[...]
> > Looking at standard Linux userspace (Debian/Fedora/etc.)
> > all the userspace tools capable of reporting voltages
> > of various voltage rails inside the system to the user
> > use the hwmon interface for this. This is also why
> > the power-supply class core recently got support for
> > proxying some psy properties to a hwmon class device.
> > 
> > So the way I see it is that if we want to report Vsys to
> > userspace, that we then clearly need to report it through
> > the hwmon interface.
> 
> For regulators, you can read their values via /sys/class/regulators/* , if
> the user is interesting in Vsys .
> 
> Maybe that is where we disagree -- I'm not particularly interested in
> exposing Vsys to user space, but since it was exposed before, I tried to
> retain that exposure, although via different channel. And the regulator also
> makes the Vsys useful, since it can be used as a supply on the kernel level.

I find knowing Vsys useful at least to have a way to quickly check if
the voltage requirements of other parts of the system are met. I don't
mind regulator vs pure hwmon interface for reading it. Though, a regulator
for Vsys could also include controlling VSYSMIN and BATFET_DIS if anyone
cared to change those at runtime.

Best Regards
Michał Mirosław
Hans de Goede Oct. 13, 2022, 9:12 a.m. UTC | #8
Hi,

On 10/13/22 00:02, Marek Vasut wrote:
> On 10/12/22 12:10, Hans de Goede wrote:
>> Hi Marek,
> 
> Hello Hans,
> 
>>>> On 10/10/22 23:03, Marek Vasut wrote:
>>>>> Pull the regulator registration code into separate function, so it can
>>>>> be extended to register more regulators later. Currently this is only
>>>>> moving ifdeffery into one place and other preparatory changes. The
>>>>> dev_err_probe() output string is changed to explicitly list vbus
>>>>> regulator failure, so that once more regulators are registered, it
>>>>> would be clear which one failed.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>
>>>> First of all thank you for your work on this series. Based purely
>>>> on reading the commit messages patches 1-4 sound good to me. I will
>>>> do a more detailed review tomorrow.
>>>>
>>>> As for patch 5-7 thinking some more about adding a Vsys regulator
>>>> just to report the Vsys reading feels wrong to me.
>>>>
>>>> A regulator device's voltage in sysfs is about the value the regulator
>>>> is supposed to try and regulate its outputted voltage to, while here
>>>> we are talking about an ADC reading of the actual outputted voltage.
>>>>
>>>> This really should *not* be modeled as a regulator if anything the
>>>> hwmon interface would be applicable for this ADC reading and
>>>> the power_supply core has support for exporting some of
>>>> the psy info through hwmon now.
>>>>
>>>> So what should happen for Vsys IMHO is make it a new
>>>> POWER_SUPPLY_PROP_SYSTEM_VOLTAGE property and while adding support
>>>> for this new property to the power-supply core, also make the core's
>>>> hwmon glue code export this in the registered hwmon device so that
>>>> e.g. a sensors applet on the desktop can easily show it (*).
>>>>
>>>> Sorry for the confusion with my ack in the other thread which
>>>> only meant to agree with a part of the alinea/sentence I put
>>>> the ack under.
>>>
>>> I'm not sure that's all there is to the Vsys regulator, it would let us model the connection between the charger chip and PMIC, where the charger would be the supply and the PMIC the regulator consumer. If the PMIC can determine its input voltage, it might be able to configure itself to some more optimal mode of operation. With the Vsys regulator, the PMIC can determine its voltage. So I think the Vsys regulator would be useful in that scenario (that's how it is wired on my board btw.).
>>
>> You are right that theoretically there is nothing wrong with
>> the model of having the charger-IC's Vsys output being
>> a perent regulator for the PMIC.
>>
>> As for this being useful to actually have I have my doubts
>> though. All the PMICs I know automatically select the
>> optimal mode/parameters for their buck convertors based
>> on the detected input voltage. So they basically always
>> work optimally as long as the input voltage is within
>> their supported range. So having Vsys moduled as
>> a regulator is only theoretically useful which is not
>> a good argument for adding this to the kernel in this
>> way IMHO.
> 
> I would be careful with "All the PMICs" , not all of them are fully automatic, some are just broken or partly automatic. i.MX28 POWER unit and one of the Dialog 9062 I think come to mind.
> 
> I also think if we have a supplier (bq25890) -> consumer (pmic input) relationship in hardware between two chips, we should model it using regulators. This is the common approach on embedded systems, so I don't see why we shouldn't do it here the same way?
> 
>> I believe it is important to go back to the original
>> problem / question which we are trying to solve / answer
>> here, which is:
>>
>> "what is the best way to make the readings from
>> the Vsys ADC available to userspace?"
> 
> Actually, I don't particularly care about exposing Vsys to user space. We are already changing the ABI with these first four patches, and the Vsys is no longer reported with 1..4 applied, so shall we change the question to:
> "
> Do we care about making Vsys reading available to userspace at all ?
> "
> 
>> Looking at standard Linux userspace (Debian/Fedora/etc.)
>> all the userspace tools capable of reporting voltages
>> of various voltage rails inside the system to the user
>> use the hwmon interface for this. This is also why
>> the power-supply class core recently got support for
>> proxying some psy properties to a hwmon class device.
>>
>> So the way I see it is that if we want to report Vsys to
>> userspace, that we then clearly need to report it through
>> the hwmon interface.
> 
> For regulators, you can read their values via /sys/class/regulators/* , if the user is interesting in Vsys .
> 
> Maybe that is where we disagree -- I'm not particularly interested in exposing Vsys to user space, but since it was exposed before, I tried to retain that exposure, although via different channel. And the regulator also makes the Vsys useful, since it can be used as a supply on the kernel level.

I don't particularly care a lot about exposing Vsys to user space either,
but like you I do believe that we should at least keep the functionality
around while fixing the wrong property use.

I'm still not 100% convinced a regular for Vsys is not a bit
overkill but I don't want to block going this route either.

So I'll go and review the last 3 patches and then lets wait
and see what Sebastian (SRE) has to say.

Regards,

Hans
Hans de Goede Oct. 13, 2022, 9:24 a.m. UTC | #9
Hi,

On 10/10/22 23:03, Marek Vasut wrote:
> Pull the regulator registration code into separate function, so it can
> be extended to register more regulators later. Currently this is only
> moving ifdeffery into one place and other preparatory changes. The
> dev_err_probe() output string is changed to explicitly list vbus
> regulator failure, so that once more regulators are registered, it
> would be clear which one failed.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> To: linux-pm@vger.kernel.org

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/bq25890_charger.c | 51 ++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 6cc3c23cd8853..7ab27a9dce14a 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1114,6 +1114,36 @@ static const struct regulator_desc bq25890_vbus_desc = {
>  	.fixed_uV = 5000000,
>  	.n_voltages = 1,
>  };
> +
> +static int bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
> +	struct regulator_config cfg = {
> +		.dev = bq->dev,
> +		.driver_data = bq,
> +	};
> +	struct regulator_dev *reg;
> +
> +	if (!IS_ERR_OR_NULL(bq->usb_phy))
> +		return 0;
> +
> +	if (pdata)
> +		cfg.init_data = pdata->regulator_init_data;
> +
> +	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
> +	if (IS_ERR(reg)) {
> +		return dev_err_probe(bq->dev, PTR_ERR(reg),
> +				     "registering vbus regulator");
> +	}
> +
> +	return 0;
> +}
> +#else
> +static inline int
> +bq25890_register_regulator(struct bq25890_device *bq)
> +{
> +	return 0
> +}
>  #endif
>  
>  static int bq25890_get_chip_version(struct bq25890_device *bq)
> @@ -1309,27 +1339,16 @@ static int bq25890_probe(struct i2c_client *client,
>  
>  	/* OTG reporting */
>  	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> +
> +	ret = bq25890_register_regulator(bq);
> +	if (ret)
> +		return ret;
> +
>  	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
>  		INIT_WORK(&bq->usb_work, bq25890_usb_work);
>  		bq->usb_nb.notifier_call = bq25890_usb_notifier;
>  		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
>  	}
> -#ifdef CONFIG_REGULATOR
> -	else {
> -		struct bq25890_platform_data *pdata = dev_get_platdata(dev);
> -		struct regulator_config cfg = { };
> -		struct regulator_dev *reg;
> -
> -		cfg.dev = dev;
> -		cfg.driver_data = bq;
> -		if (pdata)
> -			cfg.init_data = pdata->regulator_init_data;
> -
> -		reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
> -		if (IS_ERR(reg))
> -			return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
> -	}
> -#endif
>  
>  	ret = bq25890_power_supply_init(bq);
>  	if (ret < 0) {
Marek Vasut Oct. 14, 2022, 4:46 p.m. UTC | #10
On 10/13/22 11:12, Hans de Goede wrote:
> Hi,

Hi,

[...]

>>> Looking at standard Linux userspace (Debian/Fedora/etc.)
>>> all the userspace tools capable of reporting voltages
>>> of various voltage rails inside the system to the user
>>> use the hwmon interface for this. This is also why
>>> the power-supply class core recently got support for
>>> proxying some psy properties to a hwmon class device.
>>>
>>> So the way I see it is that if we want to report Vsys to
>>> userspace, that we then clearly need to report it through
>>> the hwmon interface.
>>
>> For regulators, you can read their values via /sys/class/regulators/* , if the user is interesting in Vsys .
>>
>> Maybe that is where we disagree -- I'm not particularly interested in exposing Vsys to user space, but since it was exposed before, I tried to retain that exposure, although via different channel. And the regulator also makes the Vsys useful, since it can be used as a supply on the kernel level.
> 
> I don't particularly care a lot about exposing Vsys to user space either,
> but like you I do believe that we should at least keep the functionality
> around while fixing the wrong property use.

I agree.

> I'm still not 100% convinced a regular for Vsys is not a bit
> overkill but I don't want to block going this route either.

Why would a regulator be an overkill compared to hwmon ?
What am I missing here ?

> So I'll go and review the last 3 patches and then lets wait
> and see what Sebastian (SRE) has to say.

Thank you

[...]
Hans de Goede Oct. 15, 2022, 2:16 p.m. UTC | #11
Hi,

On 10/14/22 18:46, Marek Vasut wrote:
> On 10/13/22 11:12, Hans de Goede wrote:
>> Hi,
> 
> Hi,
> 
> [...]
> 
>>>> Looking at standard Linux userspace (Debian/Fedora/etc.)
>>>> all the userspace tools capable of reporting voltages
>>>> of various voltage rails inside the system to the user
>>>> use the hwmon interface for this. This is also why
>>>> the power-supply class core recently got support for
>>>> proxying some psy properties to a hwmon class device.
>>>>
>>>> So the way I see it is that if we want to report Vsys to
>>>> userspace, that we then clearly need to report it through
>>>> the hwmon interface.
>>>
>>> For regulators, you can read their values via /sys/class/regulators/* , if the user is interesting in Vsys .
>>>
>>> Maybe that is where we disagree -- I'm not particularly interested in exposing Vsys to user space, but since it was exposed before, I tried to retain that exposure, although via different channel. And the regulator also makes the Vsys useful, since it can be used as a supply on the kernel level.
>>
>> I don't particularly care a lot about exposing Vsys to user space either,
>> but like you I do believe that we should at least keep the functionality
>> around while fixing the wrong property use.
> 
> I agree.
> 
>> I'm still not 100% convinced a regular for Vsys is not a bit
>> overkill but I don't want to block going this route either.
> 
> Why would a regulator be an overkill compared to hwmon ?
> What am I missing here ?

A regulator is another struct device, and also more code at the
driver level then just adding a new property (there already is
a hwmon device related to the psy device, so a new property would
just use that.

Anyways you have expressed a clear preference for the regulator
approach and I'm fine with that, lets see what sre has to say.

Regards,

Hans
Marek Vasut Oct. 15, 2022, 5:52 p.m. UTC | #12
On 10/15/22 16:16, Hans de Goede wrote:

Hi,

>>>>> Looking at standard Linux userspace (Debian/Fedora/etc.)
>>>>> all the userspace tools capable of reporting voltages
>>>>> of various voltage rails inside the system to the user
>>>>> use the hwmon interface for this. This is also why
>>>>> the power-supply class core recently got support for
>>>>> proxying some psy properties to a hwmon class device.
>>>>>
>>>>> So the way I see it is that if we want to report Vsys to
>>>>> userspace, that we then clearly need to report it through
>>>>> the hwmon interface.
>>>>
>>>> For regulators, you can read their values via /sys/class/regulators/* , if the user is interesting in Vsys .
>>>>
>>>> Maybe that is where we disagree -- I'm not particularly interested in exposing Vsys to user space, but since it was exposed before, I tried to retain that exposure, although via different channel. And the regulator also makes the Vsys useful, since it can be used as a supply on the kernel level.
>>>
>>> I don't particularly care a lot about exposing Vsys to user space either,
>>> but like you I do believe that we should at least keep the functionality
>>> around while fixing the wrong property use.
>>
>> I agree.
>>
>>> I'm still not 100% convinced a regular for Vsys is not a bit
>>> overkill but I don't want to block going this route either.
>>
>> Why would a regulator be an overkill compared to hwmon ?
>> What am I missing here ?
> 
> A regulator is another struct device, and also more code at the
> driver level then just adding a new property (there already is
> a hwmon device related to the psy device, so a new property would
> just use that.
> 
> Anyways you have expressed a clear preference for the regulator
> approach and I'm fine with that, lets see what sre has to say.

Let's do that indeed.

Thanks for clarifying!
diff mbox series

Patch

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 6cc3c23cd8853..7ab27a9dce14a 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1114,6 +1114,36 @@  static const struct regulator_desc bq25890_vbus_desc = {
 	.fixed_uV = 5000000,
 	.n_voltages = 1,
 };
+
+static int bq25890_register_regulator(struct bq25890_device *bq)
+{
+	struct bq25890_platform_data *pdata = dev_get_platdata(bq->dev);
+	struct regulator_config cfg = {
+		.dev = bq->dev,
+		.driver_data = bq,
+	};
+	struct regulator_dev *reg;
+
+	if (!IS_ERR_OR_NULL(bq->usb_phy))
+		return 0;
+
+	if (pdata)
+		cfg.init_data = pdata->regulator_init_data;
+
+	reg = devm_regulator_register(bq->dev, &bq25890_vbus_desc, &cfg);
+	if (IS_ERR(reg)) {
+		return dev_err_probe(bq->dev, PTR_ERR(reg),
+				     "registering vbus regulator");
+	}
+
+	return 0;
+}
+#else
+static inline int
+bq25890_register_regulator(struct bq25890_device *bq)
+{
+	return 0
+}
 #endif
 
 static int bq25890_get_chip_version(struct bq25890_device *bq)
@@ -1309,27 +1339,16 @@  static int bq25890_probe(struct i2c_client *client,
 
 	/* OTG reporting */
 	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+
+	ret = bq25890_register_regulator(bq);
+	if (ret)
+		return ret;
+
 	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
 		INIT_WORK(&bq->usb_work, bq25890_usb_work);
 		bq->usb_nb.notifier_call = bq25890_usb_notifier;
 		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
 	}
-#ifdef CONFIG_REGULATOR
-	else {
-		struct bq25890_platform_data *pdata = dev_get_platdata(dev);
-		struct regulator_config cfg = { };
-		struct regulator_dev *reg;
-
-		cfg.dev = dev;
-		cfg.driver_data = bq;
-		if (pdata)
-			cfg.init_data = pdata->regulator_init_data;
-
-		reg = devm_regulator_register(dev, &bq25890_vbus_desc, &cfg);
-		if (IS_ERR(reg))
-			return dev_err_probe(dev, PTR_ERR(reg), "registering regulator");
-	}
-#endif
 
 	ret = bq25890_power_supply_init(bq);
 	if (ret < 0) {