| Message ID | 20230309225041.477440-1-sre@kernel.org |
|---|---|
| Headers | show |
| Series | Add DT support for generic ADC battery | expand |
Hi Sebastian, thanks for your patch! On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > + /* > + * Set if constant battery information from firmware should be > + * exposed automatically. No driver specific code is required > + * in that case. If the driver also handles a property provided > + * by constant firmware data, the driver's handler is preferred. > + */ > + bool expose_battery_info; Playing it safe with opt-in I see! But I would probably invert it and add a hide_battery_info for those that don't wanna expose it. It seems pretty useful to just expose this in general. However I have no insight in what happens on laptops etc for this so I guess you have your reasons, either way: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp); > +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info, > + enum power_supply_property psp, > + union power_supply_propval *val); I think the build robots complain because you need to add some stubs for the not enabled case. Yours, Linus Walleij
On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > power-supply properties are reported in µV, µA and µW. > The IIO API provides mV, mA, mW, so the values need to > be multiplied by 1000. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Fixes: tag? Cc: stable@vger.kernel.org Reviewed-by: Linus Walleij <linus.walleij@linaro.org> This code can not have seen much testing. Yours, Linus Walleij
On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > Drop CHARGE_NOW support, which requires a platform specific > calculation method. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> I agree. If we want to support this, we should use the generic methods with interpolation tables defined in DT as well, and it also ideally requires load compensated resistance calculation to figure out Ri so this can bring any kind of reasonable precision. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Sebastian, All To me this does look like a great simplification :) On 3/10/23 00:50, Sebastian Reichel wrote: > Convert driver to use managed resources to simplify driver code. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
On 3/10/23 00:50, Sebastian Reichel wrote: > Error printing happens automatically for memory allocation problems. > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > drivers/power/supply/generic-adc-battery.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c > index d07eeb7d46d3..771e5cfc49c3 100644 > --- a/drivers/power/supply/generic-adc-battery.c > +++ b/drivers/power/supply/generic-adc-battery.c > @@ -243,10 +243,8 @@ static int gab_probe(struct platform_device *pdev) > bool any = false; > > adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL); > - if (!adc_bat) { > - dev_err(&pdev->dev, "failed to allocate memory\n"); > + if (!adc_bat) > return -ENOMEM; > - } > > psy_cfg.drv_data = adc_bat; > psy_desc = &adc_bat->psy_desc;
On Mon, Mar 13, 2023 at 8:49 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/10/23 10:29, Linus Walleij wrote: > > On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > > > >> Drop CHARGE_NOW support, which requires a platform specific > >> calculation method. > >> > >> Signed-off-by: Sebastian Reichel <sre@kernel.org> > > > > I agree. If we want to support this, we should use the generic > > methods with interpolation tables defined in DT as well, and it also > > ideally requires load compensated resistance calculation to figure > > out Ri so this can bring any kind of reasonable precision. > > I guess you have your reasons, besides you have far better insight to > things than I do - hence I am not really objecting this - just asking a > question ;) > > Do we have generic facilities of computing this based on the DT tables / > Ri in place(?) Not yet, for the Samsung batteries I used a static look-up table derived from the compatible string for calculating Ri from VBAT and from that calculate the capacity from estimated open circuit voltage, see drivers/power/supply/samsung-sdi-battery.c > I guess that we do need/see platform specific > implementations as long as there is no generic "de-facto" way of doing > this available... The method I used with Samsung batteries is fine as long as all you need to know to know everything about a battery is the compatible string. Pretty much any Lion battery with a clearly defined product name can be done this way. The only reason to put the interpolation tables into the device tree would be to support any random battery, such as one that you do not know the model or this can change. I am however mildly sceptic about adding that: if you know the VBAT-to-Ri and OCV-to-capacity tables, you must have a datasheet, and then you know the name of the battery product and hence you know the right compatible string... I think the right way to handle any capacity curves for any battery would be to create static data like I did for the Samsung batteries. Yours, Linus Walleij
Hi, On Fri, Mar 10, 2023 at 09:23:06AM +0100, Linus Walleij wrote: > On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@kernel.org> wrote: > > > power-supply properties are reported in µV, µA and µW. > > The IIO API provides mV, mA, mW, so the values need to > > be multiplied by 1000. > > > > Signed-off-by: Sebastian Reichel <sre@kernel.org> > > Fixes: tag? > Cc: stable@vger.kernel.org > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > This code can not have seen much testing. There is no mainline board using this driver and I think there never was one. I did add a Fixes tag now, but its probably not worth any backporting trouble considering it has no users. -- Sebastian
On Tue, Mar 14, 2023 at 12:17 AM Sebastian Reichel <sre@kernel.org> wrote: > There is no mainline board using this driver and I think there > never was one. I did add a Fixes tag now, but its probably not worth > any backporting trouble considering it has no users. Good point. If a tree falls in the wood an no-one is there to hear it, it doesn't make a sound. Yours, Linus Walleij