Received-SPF: pass (google.com: domain of linux-kernel+bounces-97041-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 8 Mar 2024 08:11:53 -0500
From: =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado <nfraprado@collabora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Sebastian Reichel <sre@kernel.org>, kernel@collabora.com,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] power: supply: sbs-battery: Handle unsupported
 PROP_TIME_TO_EMPTY_NOW
Message-ID: <58fbe746-528f-4761-99ae-a42fe2c41c38@notapiano>
References: <20240307-sbs-time-empty-now-error-v1-1-18d0f8702330@collabora.com>
 <3a0910c4-87f4-4c0f-b6b6-3f7bf553a5f0@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3a0910c4-87f4-4c0f-b6b6-3f7bf553a5f0@collabora.com>

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@collabora.com>
> > ---
> >   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