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