Re: [PATCH v4 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings

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

Hi,

On 8/27/24 9:04 PM, Hans de Goede wrote:
> Hi,
> 
> On 8/27/24 8:24 PM, Andres Salomon wrote:
>> On Mon, 26 Aug 2024 16:44:35 +0200
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> Hi Andres,
>>>
>>> On 8/20/24 9:30 AM, Andres Salomon wrote:
>> [...]
>>>> +
>>>> +static ssize_t charge_type_show(struct device *dev,
>>>> +		struct device_attribute *attr,
>>>> +		char *buf)
>>>> +{
>>>> +	ssize_t count = 0;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>>>> +		bool active;
>>>> +
>>>> +		if (!(battery_supported_modes & BIT(i)))
>>>> +			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 you look at the way how charge_type is shown by the power_supply_sysfs.c
>>> file which is used for power-supply drivers which directly register
>>> a power-supply themselves rather then extending an existing driver, this
>>> is not the correct format.
>>>
>>> drivers/power/supply/power_supply_sysfs.c
>>>
>>> lists charge_type as:
>>>
>>>         POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE),
>>>
>>> and ENUM type properties use the following for show() :
>>>
>>> 	default:
>>> 		if (ps_attr->text_values_len > 0 &&
>>> 				value.intval < ps_attr->text_values_len && value.intval >= 0) {
>>> 			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>>> 		} else {
>>> 			ret = sysfs_emit(buf, "%d\n", value.intval);
>>> 		}
>>> 	}
>>>
>>> with in this case text_values pointing to:
>>>
>>> static const char * const POWER_SUPPLY_CHARGE_TYPE_TEXT[] = {
>>> 	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_FAST]		= "Fast",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_STANDARD]	= "Standard",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE]	= "Adaptive",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_LONGLIFE]	= "Long Life",
>>> 	[POWER_SUPPLY_CHARGE_TYPE_BYPASS]	= "Bypass",
>>> };
>>>
>>> So value.intval will be within the expected range hitting:
>>>
>>> 			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>>>
>>> IOW instead of outputting something like this:
>>>
>>> Fast [Standard] Long Life
>>>
>>> which is what your show() function does it outputs only
>>> the active value as a string, e.g.:
>>>
>>> Standard
>>>
>>> Yes not being able to see the supported values is annoying I actually
>>> wrote an email about that earlier today:
>>>
>>> https://lore.kernel.org/linux-pm/49993a42-aa91-46bf-acef-4a089db4c2db@redhat.com/
>>>
>>> but we need to make sure that the output is consistent between drivers otherwise
>>> userspace can never know how to use the API, so for charge_type the dell
>>> driver should only output the active type, not all the options.
>>
>> So should I just wait to make any changes until you hear back in that
>> thread?
> 
> Yes that might be best.
> 
>> I'm not overly excited about changing it to use the current
>> charge_type API, given that the only way to get a list of modes that the
>> hardware supports is to try setting them all and seeing what fails.
>>
>> I suppose another option is to rename it to charge_types in the dell
>> driver under the assumption that your proposed charge_types API (or
>> something like it) will be added..
> 
> Right, if we get a favorable reaction to my charge_types suggestion
> then we can go ahead with the dell-laptop changes using charge_types
> instead of charge_type. I was already thinking along those lines
> myself too.
> 
> So if my RFC gets a favorable response lets do that.
> 
> In that case you don't even need to send a new version just
> renaming charge_type to charge_types is something which I can do
> while merging this.

Sebastian has acked the charge_types support. So I've done
s/charge_type/charge_types/ support and merged this.

Note that once charge_types get added to the power-supply
core (I hope to post patches for this soon-ish), then there
will be a power_supply_charge_types_show() helper which
will replace most of the body of charge_types_show() to make
sure that the output does not change when switching to this helper
I have changed the order of battery_modes:

--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -107,9 +107,9 @@ struct battery_mode_info {
 };
 
 static const struct battery_mode_info battery_modes[] = {
-	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
-	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
 	{ 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" },
 };

Now it matches the order of the POWER_SUPPLY_CHARGE_TYPE_* values
in include/linux/power_supply.h and the future
power_supply_charge_types_show() helper will show the (available)
strings in that order.

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


Regards,

Hans