Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote:
> On Wed, 11 Dec 2024, Hans de Goede wrote:
> 
>> Make battery_modes a map between tokens and enum power_supply_charge_type
>> values instead of between tokens and strings and use the new
>> power_supply_charge_types_show/_parse() helpers for show()/store()
>> to ensure that things are handled in the same way as in other drivers.
>>
>> This also changes battery_supported_modes to be a bitmap of charge-types
>> (enum power_supply_charge_type values) rather then a bitmap of indices
>> into battery_modes[].
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++-------------
>>  1 file changed, 25 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
>> index 5671bd0deee7..9a4cfcb8bbe0 100644
>> --- a/drivers/platform/x86/dell/dell-laptop.c
>> +++ b/drivers/platform/x86/dell/dell-laptop.c
>> @@ -103,15 +103,15 @@ static bool mute_led_registered;
>>  
>>  struct battery_mode_info {
>>  	int token;
>> -	const char *label;
>> +	enum power_supply_charge_type charge_type;
>>  };
>>  
>>  static const struct battery_mode_info battery_modes[] = {
>> -	{ BAT_PRI_AC_MODE_TOKEN,   "Trickle" },
>> -	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
>> -	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
>> -	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
>> -	{ BAT_CUSTOM_MODE_TOKEN,   "Custom" },
>> +	{ BAT_PRI_AC_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_TRICKLE },
>> +	{ BAT_EXPRESS_MODE_TOKEN,  POWER_SUPPLY_CHARGE_TYPE_FAST },
>> +	{ BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD },
>> +	{ BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE },
>> +	{ BAT_CUSTOM_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_CUSTOM },
>>  };
>>  static u32 battery_supported_modes;
>>  
>> @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>> -	ssize_t count = 0;
>> +	enum power_supply_charge_type charge_type;
>>  	int i;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>> -		bool active;
>> +		charge_type = battery_modes[i].charge_type;
>>  
>> -		if (!(battery_supported_modes & BIT(i)))
>> +		if (!(battery_supported_modes & BIT(charge_type)))
>>  			continue;
>>  
>> -		active = dell_battery_mode_is_active(battery_modes[i].token);
>> -		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
>> -				battery_modes[i].label);
>> +		if (!dell_battery_mode_is_active(battery_modes[i].token))
>> +			continue;
>> +
>> +		return power_supply_charge_types_show(dev, battery_supported_modes,
>> +						      charge_type, buf);
>>  	}
>>  
>> -	/* convert the last space to a newline */
>> -	if (count > 0)
>> -		count--;
>> -	count += sysfs_emit_at(buf, count, "\n");
>> -
>> -	return count;
>> +	/* No active mode found */
>> +	return -EIO;
>>  }
>>  
>>  static ssize_t charge_types_store(struct device *dev,
>>  		struct device_attribute *attr,
>>  		const char *buf, size_t size)
>>  {
>> -	bool matched = false;
>> -	int err, i;
>> +	int charge_type, err, i;
>> +
>> +	charge_type = power_supply_charge_types_parse(battery_supported_modes, buf);
>> +	if (charge_type < 0)
>> +		return charge_type;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>> -		if (!(battery_supported_modes & BIT(i)))
>> -			continue;
>> -
>> -		if (sysfs_streq(battery_modes[i].label, buf)) {
>> -			matched = true;
>> +		if (battery_modes[i].charge_type == charge_type)
>>  			break;
>> -		}
>>  	}
>> -	if (!matched)
>> -		return -EINVAL;
>> +	if (i == ARRAY_SIZE(battery_modes))
>> +		return -EIO;
> 
> Hi Hans,
> 
> Is this errno change helpful/correct?

power_supply_charge_types_parse() already checks that the user-input
is one of the values advertised in the passed in battery_supported_modes,
so when we loop to translate the enum power_supply_charge_type value
returned by power_supply_charge_types_parse() then we should find
a matching entry in battery_modes[] if not something is wrong at
the driver level. So not -EINVAL, since this is a driver issue not
a user input issue.

> There is zero I/O done before 
> reaching this point, just input validation, so why does it return errno 
> that is "I/O error"? If you want to differentiate from -EINVAL, I suggest 
> using -ENOENT (but I personally think -EINVAL would be fine as well 
> because it's still an invalid argument even if it passed one stage of 
> the input checks).

-ENOENT instead of -EIO works for me.

Shall I send out a new version with that changed?

Note that merging this requires the earlier patches from this
series which have been merged into:

https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next

so this either requires an immutable tag from Sebastian for you to merge,
or this should be merged through Sebastian's tree.

Regards,

Hans