| 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 |
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) {
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.).
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
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
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) {
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. [...]
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
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
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) {
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 [...]
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
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 --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) {
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(-)