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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 15/04/24 21: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
> 
> Detect if this is one of the quirky batteries during presence update, so
> that hot-plugging works as expected, and if so report -ENODATA for
> POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW, which removes it from uevent and
> prevents throwing errors.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> Changes in v2:
> - Reworked patch to lay down and use a proper quirk infrastructure, and
>    update the quirks on the presence update callback so it works properly
>    even when hot-plugging different batteries
> - Link to v1: https://lore.kernel.org/r/20240307-sbs-time-empty-now-error-v1-1-18d0f8702330@collabora.com
> ---
>   drivers/power/supply/sbs-battery.c | 55 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index a6c204c08232..92acbda9e78e 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -214,6 +214,7 @@ struct sbs_info {
>   	struct delayed_work		work;
>   	struct mutex			mode_lock;
>   	u32				flags;
> +	u32				quirks;
>   	int				technology;
>   	char				strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1];
>   };
> @@ -263,6 +264,54 @@ static void sbs_disable_charger_broadcasts(struct sbs_info *chip)
>   		dev_dbg(&chip->client->dev, "%s\n", __func__);
>   }
>   
> +/* 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},
> +	{"Murata", "AP18C4K", SBS_QUIRK_BROKEN_TTE_NOW},
> +	{"333-AC-0D-A", "GG02047XL", SBS_QUIRK_BROKEN_TTE_NOW},
> +};
> +
> +static const char *sbs_get_constant_string(struct sbs_info *chip,
> +					   enum power_supply_property psp);
> +
> +static void sbs_update_quirks(struct sbs_info *chip)
> +{
> +	const char *model;
> +	const char *manufacturer;
> +	unsigned int i = 0;

Please reorder:

	const char *manufacturer;
	const char *model;
	unsigned int i;

...and please remove (like shown, of course) the double initialization of the
`i` variable, as you're initializing it again later in your for loop.

> +
> +	/* reset quirks from battery before the hot-plug event */
> +	chip->quirks = 0;
> +
> +	manufacturer = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MANUFACTURER);
> +	model = sbs_get_constant_string(chip, POWER_SUPPLY_PROP_MODEL_NAME);
> +	if (IS_ERR(manufacturer) || IS_ERR(model)) {
> +		dev_warn(&chip->client->dev, "Couldn't read manufacturer and model to set quirks\n");
> +		return;
> +	}
> +
> +	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(&chip->client->dev, "Added quirk disabling TIME_TO_EMPTY_NOW\n");

I don't really expect many quirks, but having a dev_info() print for every
quirk that gets set would make this driver too chatty, IMO.

Please, either turn that into a dev_dbg() or print a mask of the quirks .. or both.

Cheers,
Angelo

> +}
> +
>   static int sbs_update_presence(struct sbs_info *chip, bool is_present)
>   {
>   	struct i2c_client *client = chip->client;
> @@ -323,6 +372,8 @@ static int sbs_update_presence(struct sbs_info *chip, bool is_present)
>   	dev_dbg(&client->dev, "PEC: %s\n", (client->flags & I2C_CLIENT_PEC) ?
>   		"enabled" : "disabled");
>   
> +	sbs_update_quirks(chip);
> +
>   	if (!chip->is_present && is_present && !chip->charger_broadcasts)
>   		sbs_disable_charger_broadcasts(chip);
>   
> @@ -614,6 +665,10 @@ static int sbs_get_battery_property(struct i2c_client *client,
>   	struct sbs_info *chip = i2c_get_clientdata(client);
>   	s32 ret;
>   
> +	if (psp == POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW &&
> +	    chip->quirks & SBS_QUIRK_BROKEN_TTE_NOW)
> +		return -ENODATA;
> +
>   	ret = sbs_read_word_data(client, sbs_data[reg_offset].addr);
>   	if (ret < 0)
>   		return ret;
> 
> ---
> base-commit: 11afac187274a6177a7ac82997f8691c0f469e41
> change-id: 20240307-sbs-time-empty-now-error-322bc074d3f2
> 
> Best regards,