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.vaitti...@fi.rohmeurope.com> 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.vaitti...@fi.rohmeurope.com>
>
> 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
