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 <[email protected]>
---
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"
+};
+
+static void sbs_remove_unsupported_properties(struct power_supply_config *psy_cfg,
+ struct power_supply_desc *sbs_desc)
+{
+ enum power_supply_property *new_properties;
+ struct sbs_info *chip = psy_cfg->drv_data;
+ bool missing_time_to_empty_now = false;
+ const char *model_name;
+ unsigned int new_num_properties;
+ unsigned int i = 0, j = 0;
+
+ model_name = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
+ if (IS_ERR(model_name))
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(unsupported_time_to_empty_now_models); i++) {
+ if (!strcmp(model_name, unsupported_time_to_empty_now_models[i])) {
+ missing_time_to_empty_now = true;
+ break;
+ }
+ }
+
+ if (!missing_time_to_empty_now)
+ return;
+
+ new_num_properties = ARRAY_SIZE(sbs_properties) - 1;
+ new_properties = devm_kzalloc(&chip->client->dev, new_num_properties * sizeof(sbs_properties[0]), GFP_KERNEL);
+
+ for (i = 0; i < sbs_desc->num_properties; i++) {
+ if (sbs_desc->properties[i] == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW)
+ continue;
+
+ new_properties[j] = sbs_desc->properties[i];
+ j++;
+ }
+
+ sbs_desc->properties = new_properties;
+ sbs_desc->num_properties = new_num_properties;
+};
+
static int sbs_probe(struct i2c_client *client)
{
struct sbs_info *chip;
@@ -1210,6 +1253,8 @@ static int sbs_probe(struct i2c_client *client)
if (rc)
return rc;
+ sbs_remove_unsupported_properties(&psy_cfg, sbs_desc);
+
chip->power_supply = devm_power_supply_register(&client->dev, sbs_desc,
&psy_cfg);
if (IS_ERR(chip->power_supply))
---
base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2
Best regards,
--
Nícolas F. R. A. Prado <[email protected]>
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 <[email protected]>
> ---
> 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).
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.
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.
> +
> +static void sbs_remove_unsupported_properties(struct power_supply_config *psy_cfg,
> + struct power_supply_desc *sbs_desc)
P.S.: sbs_quirk_remove_unsupported_properties :-)
Cheers,
Angelo
> +{
> + enum power_supply_property *new_properties;
> + struct sbs_info *chip = psy_cfg->drv_data;
> + bool missing_time_to_empty_now = false;
> + const char *model_name;
> + unsigned int new_num_properties;
> + unsigned int i = 0, j = 0;
> +
> + model_name = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> + if (IS_ERR(model_name))
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(unsupported_time_to_empty_now_models); i++) {
> + if (!strcmp(model_name, unsupported_time_to_empty_now_models[i])) {
> + missing_time_to_empty_now = true;
> + break;
> + }
> + }
> +
> + if (!missing_time_to_empty_now)
> + return;
> +
> + new_num_properties = ARRAY_SIZE(sbs_properties) - 1;
> + new_properties = devm_kzalloc(&chip->client->dev, new_num_properties * sizeof(sbs_properties[0]), GFP_KERNEL);
> +
> + for (i = 0; i < sbs_desc->num_properties; i++) {
> + if (sbs_desc->properties[i] == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW)
> + continue;
> +
> + new_properties[j] = sbs_desc->properties[i];
> + j++;
> + }
> +
> + sbs_desc->properties = new_properties;
> + sbs_desc->num_properties = new_num_properties;
> +};
> +
> static int sbs_probe(struct i2c_client *client)
> {
> struct sbs_info *chip;
> @@ -1210,6 +1253,8 @@ static int sbs_probe(struct i2c_client *client)
> if (rc)
> return rc;
>
> + sbs_remove_unsupported_properties(&psy_cfg, sbs_desc);
> +
> chip->power_supply = devm_power_supply_register(&client->dev, sbs_desc,
> &psy_cfg);
> if (IS_ERR(chip->power_supply))
>
> ---
> base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
> change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2
>
> Best regards,
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 <[email protected]>
> > ---
> > 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
Hi N?colas,
On Thu, Mar 07, 2024 at 05:05:10PM -0500, N?colas F. R. A. Prado wrote:
> 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 <[email protected]>
> ---
> 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"
> +};
> +
> +static void sbs_remove_unsupported_properties(struct power_supply_config *psy_cfg,
> + struct power_supply_desc *sbs_desc)
> +{
> + enum power_supply_property *new_properties;
> + struct sbs_info *chip = psy_cfg->drv_data;
> + bool missing_time_to_empty_now = false;
> + const char *model_name;
> + unsigned int new_num_properties;
> + unsigned int i = 0, j = 0;
> +
> + model_name = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> + if (IS_ERR(model_name))
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(unsupported_time_to_empty_now_models); i++) {
> + if (!strcmp(model_name, unsupported_time_to_empty_now_models[i])) {
> + missing_time_to_empty_now = true;
> + break;
> + }
> + }
> +
> + if (!missing_time_to_empty_now)
> + return;
> +
> + new_num_properties = ARRAY_SIZE(sbs_properties) - 1;
> + new_properties = devm_kzalloc(&chip->client->dev, new_num_properties * sizeof(sbs_properties[0]), GFP_KERNEL);
> +
> + for (i = 0; i < sbs_desc->num_properties; i++) {
> + if (sbs_desc->properties[i] == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW)
> + continue;
> +
> + new_properties[j] = sbs_desc->properties[i];
> + j++;
> + }
> +
> + sbs_desc->properties = new_properties;
> + sbs_desc->num_properties = new_num_properties;
> +};
> +
> static int sbs_probe(struct i2c_client *client)
> {
> struct sbs_info *chip;
> @@ -1210,6 +1253,8 @@ static int sbs_probe(struct i2c_client *client)
> if (rc)
> return rc;
>
> + sbs_remove_unsupported_properties(&psy_cfg, sbs_desc);
> +
> chip->power_supply = devm_power_supply_register(&client->dev, sbs_desc,
> &psy_cfg);
> if (IS_ERR(chip->power_supply))
>
> ---
I think it makes sense to use a proper quirk infrastructure.
Something like this:
/* required by the spec, but missing in some implementations */
#define SBS_QUIRK_BROKEN_TTE_NOW BIT(0)
struct sbs_quirk_entry {
const char *manufacturer;
const char *model;
u32 flags;
};
static const struct sbs_quirk_entry sbs_quirks[] = {
{"PANASON", "AP16L5J", SBS_QUIRK_BROKEN_TTE_NOW},
{"PANASON", "AP15O5L", SBS_QUIRK_BROKEN_TTE_NOW},
{"LGC KT0", "AP16L8J", SBS_QUIRK_BROKEN_TTE_NOW},
...
};
static void sbs_update_quirks(...) {
...
manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER);
model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
/* reset quirks from battery before the hot-plug event */
chip->quirks = 0;
for (i = 0; i < ARRAY_SIZE(sbs_quirks); i++) {
if (strcmp(manufacturer, sbs_quirks[i].manufacturer))
continue;
if (strcmp(model, sbs_quirks[i].model))
continue;
chip->quirks |= sbs_quirks[i].flags;
}
if (chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
dev_info(dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n");
}
Also I think instead of removing the property, it's better to return
-ENODATA for TIME_TO_EMPTY. That will remove it from the uevent file,
but still expose the sysfs property file. So it's slightly worse from
that aspect. But it means the quirk can be handled in sbs_update_presence()
and it can be added/removed dynamically when different battery models
are used with hot-plugging.
For that it should be enough to call the above sbs_update_quirks() in
sbs_update_presence() and add this at the beginning of
sbs_get_battery_property():
if (psp == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW &&
chip->quirks == SBS_QUIRK_BROKEN_TTE_NOW)
return -ENODATA;
As a side-effect it fixes your struggling with keeping the
properties constant. Can you check, if that works?
Thanks,
-- Sebastian