Re: [PATCH] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY_NOW

[Nícolas F. R. A. Prado]

On Fri, Mar 08, 2024 at 10:26:29AM +0100, AngeloGioacchino Del Regno wrote:
> Il 07/03/24 23:05, Nícolas F. R. A. Prado ha scritto:
> > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS
> > specification as required, it seems that not all batteries implement it.
> > On platforms with such batteries, reading the property will cause an
> > error to be printed:
> > 
> > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5
> > 
> > This not only pollutes the log, distracting from real problems on the
> > device, but also prevents the uevent file from being read since it
> > contains all properties, including the faulty one.
> > 
> > The following table summarizes the findings for a handful of platforms:
> > 
> > Platform                                Status  Manufacturer    Model
> > ------------------------------------------------------------------------
> > mt8186-corsola-steelix-sku131072        OK      BYD             L22B3PG0
> > mt8195-cherry-tomato-r2                 NOT OK  PANASON         AP16L5J
> > mt8192-asurada-spherion-r0              NOT OK  PANASON         AP15O5L
> > mt8183-kukui-jacuzzi-juniper-sku16      NOT OK  LGC KT0         AP16L8J
> > mt8173-elm-hana                         OK      Sunwoda         L18D3PG1
> > sc7180-trogdor-lazor-limozeen-nots-r5   NOT OK  Murata          AP18C4K
> > sc7180-trogdor-kingoftown               NOT OK  333-AC-0D-A     GG02047XL
> > rk3399-gru-kevin                        OK      SDI             4352D51
> > 
> > Identify during probe, based on the battery model, if this is one of the
> > quirky batteries, and if so, don't register the
> > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW property.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > ---
> >   drivers/power/supply/sbs-battery.c | 45 ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 45 insertions(+)
> > 
> > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> > index a6c204c08232..85ff331cf87a 100644
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -1112,6 +1112,49 @@ static const struct power_supply_desc sbs_default_desc = {
> >   	.external_power_changed = sbs_external_power_changed,
> >   };
> > +static const char * const unsupported_time_to_empty_now_models[] = {
> > +	"AP16L5J", "AP15O5L", "AP16L8J", "AP18C4K", "GG02047XL"
> > +};
> 
> I think that you must make sure that this is seen as a quirk, rather than
> "something normal" because - as you said - the SBS specification says that
> the TIME_TO_EMPTY_NOW is *required*, so, this is a *deviation* from the spec
> (so, the SBS implementation in those devices is *out of spec*!).
> 
> static const char * const quirk_remove_time_to_empty_now_models
> or                        quirk_unsupported_time_to_empty_now_models
> 
> ...the former, if you want to avoid having a variable name that is 5000 characters
> long (:-P); the latter, if you don't care about that (there's no rule anyway).

(and I just noticed I forgot the usual sbs_ prefix here, so it'll get a few more
characters ;P)

> 
> 
> Besides that, since I didn't like what I just saw, I looked for different
> alternatives; the only other one is to de-constify the sbs_properties[] array,
> which is something that I also dislike anyway.
> 
> I'm not sure if deconstifying that could be acceptable, but if it would, you
> would be able to remove-reorder without copies of this and that.

Personally I don't see an issue with creating a new struct and copying things
over - this will only happen during probe and for the quirky batteries anyway -
and it's nice to keep things const for the common case.

> 
> Anyway - the only thing I really want here is to make sure that this is seen
> as a quirk and a clear deviation from the specification - everything else is
> a plus, and not really a blocker for me.

Yep, and you're right on that. I'll make sure to slap the "quirk" sticker on the
variable and function for next version.

Thanks,
Nícolas