Re: [PATCH v6 4/6] power: supply: core: Add some helpers to use the battery OCV capacity table

[Quentin Schulz]

Hi Baolin,

On Thu, Nov 01, 2018 at 03:22:18PM +0800, Baolin Wang wrote:
> Hi Quentin,
> 
> On 29 October 2018 at 22:48, Quentin Schulz <quentin.schulz@xxxxxxxxxxx> wrote:
[...]
> >
> >> +             return len;
> >> +     } else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
> >> +             dev_err(&psy->dev, "Too many temperature values\n");
> >> +             return -EINVAL;
> >> +     } else if (len > 0) {
> >> +             of_property_read_u32_array(battery_np, "ocv-capacity-celsius",
> >> +                                        info->ocv_temp, len);
> >> +     }
> >> +
> >> +     for (index = 0; index < len; index++) {
> >
> > This won't work as intended as we can reach this part of the code with
> > len = -EINVAL;
> 
> No, the condition (index < len) is false if len = -EINVAL, maybe one
> reason keep index as 'int' type.
> 

Ugh. Next time I'll make sure my brain is wired before reviewing a
patch, sorry :)

[...]
> >> +             if (!info->ocv_table[index]) {
> >> +                     power_supply_put_battery_info(psy, info);
> >> +                     return -ENOMEM;
> >> +             }
> >> +
> >> +             for (i = 0; i < tab_len; i++) {
> >> +                     table[i].ocv = be32_to_cpu(*list++);
> >> +                     table[i].capacity = be32_to_cpu(*list++);
> >
> > Please check these are valid values.
> 
> Um, It is hard to validate the values for OCV and capacity, because
> now we do not know each battery's parameters. Any good suggestion?
> 

You know the capacity is between 0 and 100 (since it's an unsigned
value, checking against 100 is enough), that's a good start.

For the OCV, I'd say it's basically between the minimum and maximum
voltage a battery can hold. I don't know enough about batteries to
give the correct bounds unfortunately :(

[...]
> > What is the use case for letting a user call this function? I can
> > understand you want to delete the arrays if they're invalid in the
> > get_function, which could be done thanks to a goto statement or with
> > this function if you really want (if it does not mess with refs) but I
> > don't see why you want to export this function.
> 
> Cause some drivers will copy the OCV tables into their own structures,
> one helper function to help to free memory of battery information. In
> future, this can be expanded to clean up more resources of battery
> information.
> 

Couldn't they only use a pointer to the appropriate table? Or do you
plan on the drivers directly modifying the table but wanting to keep a
clean copy on the core side?

I find it weird to free the tables. I'd suppose that a driver wants to
keep all tables available to be able to chose the appropriate one
depending on the current temperature of the battery (which is what
power_supply_batinfo_ocv2cap is for if I understood correctly) and not
only at definite time (probe function for the driver you attached to the
patch series IIRC). If you need to keep all tables, why would you want
to copy them to the driver and not keep them in the core and use a
pointer to the table in current use?

I'm sure I'm missing something, let me know what it is. Thanks.

[...]
> > I would like to give my specific use case of the OCV on X-Powers PMICs
> > so that hopefully we can get things sorted out before it's too late to
> > make modifications that might be needed.
> >
> > I'm adding a few people on Cc that work on the X-Power PMICs so that
> > hopefully we can have all the correct pieces of information and go
> > towards the right solution.
> >
> > In the AXP818/AXP288 datasheet[1] (p.57), we have access to OCV values
> > and battery RDC register.
> >
> > The hardware learns from the battery or from the given OCV and RDC
> > values and then updates the returned battery capacity accordingly (in
> > another register).
> > I've 32 values (see the issue with a max of 20 values?) to be written in
> 
> I think you misunderstood the 20, it is means we can have max 20 OCV tables now.
> 

Indeed, misread the patch, thanks for clarifying.

> > different registers that represent the battery capacity at a given
> > voltage. I do not have to do any computation on the kernel side, I just
> > have to set the registers with the correct values and the battery
> > capacity will be auto-updated by the PMIC. With this patch series,
> > should I just call power_supply_get_battery_info, get the OCV tables and
> 
> I think so.
> 
> > do my thing in the driver? Could we have a more generic way to do it
> > (does it make sense to have a generic one?)?
> 
> Sorry, maybe I did not follow you here. You said your hardware will
> help to get the capacity automatically if you set the register, so
> what generic way you want to introduce? Could you elaborate on?
> 

I think I got my thoughts tangled-up, I can't honestly find a generic
way right now.

> >
> > We also definitely need a sysfs entry so that the user can enter the new
> > values of the RDC/OCV since it changes during the life cycle of the
> > battery IIRC.
> 
> Why it changed? Due to different temperatures or other factors? If the
> factor is temperature, I think we have supplied one generic way:
> https://lore.kernel.org/patchwork/patch/1002350/
> 

Apparently age is a factor in the OCV curve. I don't know if it's
substantial enough to care about though.

Anyway, I have a very strong bias towards not having to modify the
Device Tree or recompile the kernel when a piece of hardware can be
replaced easily by something different. I see the battery as such a
piece of hardware. I understand the need to define the battery that
comes with a product but I also like to let the users switch the battery
(for a bigger one for example) without getting their hands dirty with
getting the kernel sources, recompiling, etc.

What if the provided OCV curves are not the best ones and the user has
found better ones?

For that, I like to let the user configure parameters that impact the
battery after we've optionaly set the default one.

I'd be mad to have to recompile the Device Tree or kernel when switching
devices on a USB bus for example.

Does that make sense?

Thanks,
Quentin

Attachment: signature.asc
Description: PGP signature