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 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.

>> This reminds me that there was a patch-series to allow battery extension drivers
>> like this one to actually use the power-supply core code for show()/store()
>> Thomas IIRC that series was done by you ?  What is the status of that ?
>>
>> Also looking at the userspace API parts of this again I wonder
>> if mapping  BAT_PRI_AC_MODE_TOKEN -> "Trickle" is the right thing do
>> maybe "Long Life" would be a better match ?  That depends on what the option
>> actually does under the hood I guess. Is this known ?
>>
> 
> I originally thought to use Long Life rather than Trickle. We discussed
> it here:
> 
> https://lore.kernel.org/linux-pm/5cfe4c42-a003-4668-8c3a-f18fb6b7fba6@gmx.de/
> 
> Based on the existing documentation and the fact that the wilco driver
> already mapped it, it was decided to stick with the existing precedent
> of using Trickle.

Ok, I was just wondering if this was discussed already, since it was
lets stick with "Trickle".

> That said, Armin at first suggested creating a new "Primarily AC" entry.
> That's personally my favorite option, though I understand if we don't
> have to have 50 CHARGE_TYPE entries that just slightly different
> variations. :)

Right.

Regards,

Hans