Hi,
On 8/27/24 8:24 PM, Andres Salomon wrote:
> On Mon, 26 Aug 2024 16:44:35 +0200
> Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx/
>>
>> 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@xxxxxx/
>
> 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
