Re: [RFC PATCH v2 1/6] power: supply: add cap2ocv batinfo helper

[Vaittinen, Matti]

Hi deee Ho Linus,

Thanks (again) for taking a look at this! Highly appreciated :]

On Tue, 2020-12-08 at 09:54 +0100, Linus Walleij wrote:
> On Fri, Dec 4, 2020 at 1:41 PM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> 
> > The power-supply core supports concept of OCV (Open Circuit
> > Voltage) =>
> > SOC (State Of Charge) conversion tables. Usually these tables are
> > used
> > to estimate SOC based on OCV. Some systems use so called "Zero
> > Adjust"
> > where at the near end-of-battery condition the SOC from coulomb
> > counter
> > is used to retrieve the OCV - and OCV and VSYS difference is used
> > to
> > re-estimate the battery capacity.
> > 
> > Add helper to do look-up the other-way around and also get the OCV
> > based on SOC
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> 
> Overall a good idea!
> 
> > +/**
> > + * power_supply_cap2ocv_simple() - find the battery OCV by
> > capacity
> > + * @table: Pointer to battery OCV/CAP lookup table
> > + * @table_len: OCV/CAP table length
> > + * @cap: Current cap value
> > + *
> > + * This helper function is used to look up battery OCV according
> > to
> > + * current capacity value from one OCV table, and the OCV table
> > must be ordered
> > + * descending.
> > + *
> > + * Return: the battery OCV.
> > + */
> 
> Spell out the abbreviations in the kerneldoc and not just the commit.
> These will be read much more than the commit message so I would
> move all the excellent info in the commit message into the kerneldoc
> and
> diet the commit message instead.

Hm. I think you have a point here.

> 
> > +int power_supply_cap2ocv_simple(struct
> > power_supply_battery_ocv_table *table,
> > +                               int table_len, int cap)
> > +{
> > +       int i, ocv, tmp;
> > +
> > +       for (i = 0; i < table_len; i++)
> > +               if (cap > table[i].capacity)
> > +                       break;
> > +
> > +       if (i > 0 && i < table_len) {
> > +               tmp = (table[i - 1].ocv - table[i].ocv) *
> > +                     (cap - table[i].capacity);
> > +
> > +               tmp /= table[i - 1].capacity - table[i].capacity;
> > +               ocv = tmp + table[i].ocv;
> 
> This is some linear interpolation right? It's not immediately evident
> so insert
> some comment about what is going on.

Yes. Code interpolates the OCV using two closest known values from
table. This is pretty much identical loop to the existing ocv2cap
calculation - it would have been better to include it in the diff. OTOH
- I did not expect seeing any proper careful reviewing - this RFC was
sent to collect opinion on whether this would be worth further work.
Anyways - If this function is added it should be changed to take more
accurate SOC - perhaps 0.1%(?) - I'm afraid rounding the current
capacity to nearest 1% will kill the accuracy and render this somewhat
useless.

This makes me wonder if the SOC/OCV table in DT should also support
providing values using 0.1% as unit? (I don't think this is a must but
it might be useful).

> 
> >  /**
> >   * power_supply_ocv2cap_simple() - find the battery capacity
> >   * @table: Pointer to battery OCV lookup table
> > @@ -847,6 +884,20 @@ power_supply_find_ocv2cap_table(struct
> > power_supply_battery_info *info,
> 
> I suspect this kerneldoc could be improved in the process as well.
> 

I agree. And also for few others. But that could be a separate patch no
matter if this RFC proceeds or not :)

> Yours,
> Linus Walleij