The original submission of the charge_behaviour property did not
implement proper formatting in the default formatting handler in
power_supply_sysfs.c.
At that time it was not a problem because the only provider of the UAPI,
thinkpad_acpi, did its own formatting.
Now there is an in-tree driver, mm8013, and out-of-tree driver which use
the normal power-supply properties and are affected by the wrong
formatting.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Thomas Weißschuh (4):
power: supply: core: fix charge_behaviour formatting
power: supply: test-power: implement charge_behaviour property
power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
power: supply: core: drop workaround for missing POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
drivers/power/supply/mm8013.c | 5 +++++
drivers/power/supply/power_supply_sysfs.c | 28 ++++++++++++++++++++++++++++
drivers/power/supply/test_power.c | 10 ++++++++++
include/linux/power_supply.h | 1 +
4 files changed, 44 insertions(+)
---
base-commit: 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478
change-id: 20230929-power_supply-charge_behaviour_prop-10ccfd96a666
Best regards,
--
Thomas Weißschuh <[email protected]>
This property is documented to have a special format which exposes all
available behaviours and the currently active one at the same time.
For this special format some helpers are provided.
However the default property logic in power_supply_sysfs.c is not using
the helper and the default logic only prints the currently active
behaviour.
Adjust power_supply_sysfs.c to follow the documented format.
There are currently two in-tree drivers exposing charge behaviours:
thinkpad_acpi and mm8013.
thinkpad_acpi is not affected by the change, as it directly uses the
helpers and does not use the power_supply_sysfs.c logic.
As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
the new logic will preserve the simple output format in this case.
Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
include/linux/power_supply.h | 1 +
2 files changed, 33 insertions(+)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 977611e16373..3680cfc2e908 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
return count;
}
+static ssize_t power_supply_show_charge_behaviour(struct device *dev,
+ struct power_supply *psy,
+ struct power_supply_attr *ps_attr,
+ union power_supply_propval *value,
+ char *buf)
+{
+ union power_supply_propval available;
+ int ret;
+
+ ret = power_supply_get_property(psy,
+ POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+ value);
+ if (ret < 0)
+ return ret;
+
+ ret = power_supply_get_property(psy,
+ POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
+ &available);
+ if (ret == -EINVAL)
+ return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
+ else if (ret < 0)
+ return ret;
+
+ return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
+}
+
static ssize_t power_supply_show_property(struct device *dev,
struct device_attribute *attr,
char *buf) {
@@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
if (psp == POWER_SUPPLY_PROP_TYPE) {
value.intval = psy->desc->type;
+ } else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
+ value.intval = -1;
} else {
ret = power_supply_get_property(psy, psp, &value);
@@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
ret = power_supply_show_usb_type(dev, psy->desc,
&value, buf);
break;
+ case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+ ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
+ &value, buf);
+ break;
case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = sysfs_emit(buf, "%s\n", value.strval);
break;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c0992a77feea..9a6e6b488164 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -135,6 +135,7 @@ enum power_supply_property {
POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+ POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
--
2.43.0
As the mm8013 driver was extended to also report that property the
workaround is not needed anymore.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/power/supply/power_supply_sysfs.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3680cfc2e908..3804a3bbed24 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -273,7 +273,6 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
static ssize_t power_supply_show_charge_behaviour(struct device *dev,
struct power_supply *psy,
- struct power_supply_attr *ps_attr,
union power_supply_propval *value,
char *buf)
{
@@ -289,9 +288,7 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev,
ret = power_supply_get_property(psy,
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
&available);
- if (ret == -EINVAL)
- return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
- else if (ret < 0)
+ if (ret < 0)
return ret;
return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
@@ -337,8 +334,7 @@ static ssize_t power_supply_show_property(struct device *dev,
&value, buf);
break;
case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
- ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
- &value, buf);
+ ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
break;
case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
ret = sysfs_emit(buf, "%s\n", value.strval);
--
2.43.0
Hi Thomas,
On 2/4/24 18:26, Thomas Weißschuh wrote:
> As the mm8013 driver was extended to also report that property the
> workaround is not needed anymore.
>
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/power/supply/power_supply_sysfs.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 3680cfc2e908..3804a3bbed24 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -273,7 +273,6 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>
> static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> struct power_supply *psy,
> - struct power_supply_attr *ps_attr,
> union power_supply_propval *value,
> char *buf)
> {
> @@ -289,9 +288,7 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> ret = power_supply_get_property(psy,
> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> &available);
> - if (ret == -EINVAL)
> - return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
More of a remark on patch 1 really, note that if you use
POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] here in patch 1, e.g. :
return sysfs_emit(buf, "%s\n",
POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[value->intval]);
then you can omit the ps_attr argument in patch 1 already, which in turn
will cause this patch to be simpler / smaller.
Regards,
Hans
> - else if (ret < 0)
> + if (ret < 0)
> return ret;
>
> return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> @@ -337,8 +334,7 @@ static ssize_t power_supply_show_property(struct device *dev,
> &value, buf);
> break;
> case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> - ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> - &value, buf);
> + ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
> break;
> case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
> ret = sysfs_emit(buf, "%s\n", value.strval);
>
Hi Thomas,
Thank you for your patches for this.
On 2/4/24 18:26, Thomas Weißschuh wrote:
> This property is documented to have a special format which exposes all
> available behaviours and the currently active one at the same time.
> For this special format some helpers are provided.
>
> However the default property logic in power_supply_sysfs.c is not using
> the helper and the default logic only prints the currently active
> behaviour.
>
> Adjust power_supply_sysfs.c to follow the documented format.
>
> There are currently two in-tree drivers exposing charge behaviours:
> thinkpad_acpi and mm8013.
> thinkpad_acpi is not affected by the change, as it directly uses the
> helpers and does not use the power_supply_sysfs.c logic.
>
> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
> the new logic will preserve the simple output format in this case.
>
> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> Signed-off-by: Thomas Weißschuh <[email protected]>
> ---
> drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
> include/linux/power_supply.h | 1 +
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 977611e16373..3680cfc2e908 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
> return count;
> }
>
> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> + struct power_supply *psy,
> + struct power_supply_attr *ps_attr,
> + union power_supply_propval *value,
> + char *buf)
> +{
> + union power_supply_propval available;
> + int ret;
> +
> + ret = power_supply_get_property(psy,
> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> + value);
> + if (ret < 0)
> + return ret;
> +
> + ret = power_supply_get_property(psy,
> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> + &available);
> + if (ret == -EINVAL)
> + return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
> + else if (ret < 0)
No need for "else if" here since the if above does a return. So you can just do:
if (ret < 0)
return ret;
> + return ret;
> +
> + return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> +}
> +
> static ssize_t power_supply_show_property(struct device *dev,
> struct device_attribute *attr,
> char *buf) {
> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>
> if (psp == POWER_SUPPLY_PROP_TYPE) {
> value.intval = psy->desc->type;
> + } else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
> + value.intval = -1;
> } else {
> ret = power_supply_get_property(psy, psp, &value);
>
I'm not a fan of this, I guess that you are doing this because you do not
want to enter this if:
if (ps_attr->text_values_len > 0 &&
value.intval < ps_attr->text_values_len && value.intval >= 0) {
return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
}
But by doing this you add both the special case of setting value.intval = -1
here and you now need to do the power_supply_get_property() which is in the else
manually in power_supply_show_charge_behaviour() and the error handling / logging
of power_supply_get_property() in power_supply_show_charge_behaviour() is different.
What I think you can (and should) do here instead is move:
if (ps_attr->text_values_len > 0 &&
value.intval < ps_attr->text_values_len && value.intval >= 0) {
return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
}
into the default case of the switch (psp) {} below it in a preparation patch.
This if is never entered for the 2 non default cases in that switch, so it
is safe to move it into the default case, e.g. something like this:
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);
I think that also actually makes things a bit cleaner then the current
early-exit for printing enum values.
And then in the next patch you can just add:
case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
&value, buf);
break;
Without needing to set intval = -1 and without needing to get the value
inside power_supply_show_charge_behaviour() since that will already
be done for you then.
Regards,
Hans
> @@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
> ret = power_supply_show_usb_type(dev, psy->desc,
> &value, buf);
> break;
> + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> + ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> + &value, buf);
> + break;
> case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
> ret = sysfs_emit(buf, "%s\n", value.strval);
> break;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index c0992a77feea..9a6e6b488164 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -135,6 +135,7 @@ enum power_supply_property {
> POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
> POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
> POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
>
Hi,
On 2/5/24 10:52, Hans de Goede wrote:
> Hi Thomas,
>
> Thank you for your patches for this.
>
> On 2/4/24 18:26, Thomas Weißschuh wrote:
>> This property is documented to have a special format which exposes all
>> available behaviours and the currently active one at the same time.
>> For this special format some helpers are provided.
>>
>> However the default property logic in power_supply_sysfs.c is not using
>> the helper and the default logic only prints the currently active
>> behaviour.
>>
>> Adjust power_supply_sysfs.c to follow the documented format.
>>
>> There are currently two in-tree drivers exposing charge behaviours:
>> thinkpad_acpi and mm8013.
>> thinkpad_acpi is not affected by the change, as it directly uses the
>> helpers and does not use the power_supply_sysfs.c logic.
>>
>> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
>> the new logic will preserve the simple output format in this case.
>>
>> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
>> Signed-off-by: Thomas Weißschuh <[email protected]>
>> ---
>> drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
>> include/linux/power_supply.h | 1 +
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index 977611e16373..3680cfc2e908 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>> return count;
>> }
>>
>> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
>> + struct power_supply *psy,
>> + struct power_supply_attr *ps_attr,
>> + union power_supply_propval *value,
>> + char *buf)
>> +{
>> + union power_supply_propval available;
>> + int ret;
>> +
>> + ret = power_supply_get_property(psy,
>> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
>> + value);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = power_supply_get_property(psy,
>> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>> + &available);
>> + if (ret == -EINVAL)
>> + return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
>> + else if (ret < 0)
>
> No need for "else if" here since the if above does a return. So you can just do:
>
> if (ret < 0)
> return ret;
>
>
>> + return ret;
>> +
>> + return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
>> +}
>> +
>> static ssize_t power_supply_show_property(struct device *dev,
>> struct device_attribute *attr,
>> char *buf) {
>> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>
>> if (psp == POWER_SUPPLY_PROP_TYPE) {
>> value.intval = psy->desc->type;
>> + } else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
>> + value.intval = -1;
>> } else {
>> ret = power_supply_get_property(psy, psp, &value);
>>
>
> I'm not a fan of this, I guess that you are doing this because you do not
> want to enter this if:
>
> if (ps_attr->text_values_len > 0 &&
> value.intval < ps_attr->text_values_len && value.intval >= 0) {
> return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> }
>
> But by doing this you add both the special case of setting value.intval = -1
> here and you now need to do the power_supply_get_property() which is in the else
> manually in power_supply_show_charge_behaviour() and the error handling / logging
> of power_supply_get_property() in power_supply_show_charge_behaviour() is different.
>
> What I think you can (and should) do here instead is move:
>
> if (ps_attr->text_values_len > 0 &&
> value.intval < ps_attr->text_values_len && value.intval >= 0) {
> return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> }
>
> into the default case of the switch (psp) {} below it in a preparation patch.
>
> This if is never entered for the 2 non default cases in that switch, so it
> is safe to move it into the default case, e.g. something like this:
>
> 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);
>
> I think that also actually makes things a bit cleaner then the current
> early-exit for printing enum values.
>
> And then in the next patch you can just add:
>
> case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> &value, buf);
> break;
>
> Without needing to set intval = -1 and without needing to get the value
> inside power_supply_show_charge_behaviour() since that will already
> be done for you then.
One more note on this. With the suggested preparation patch to move
the if checking for ps_attr->text_values into the default case,
you can then also properly turn POWER_SUPPLY_PROP_USB_TYPE into an
enum too:
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 977611e16373..768df64330f4 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -209,7 +209,7 @@ static struct power_supply_attr power_supply_attrs[] = {
POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
POWER_SUPPLY_ENUM_ATTR(TYPE),
- POWER_SUPPLY_ATTR(USB_TYPE),
+ POWER_SUPPLY_ENUM_ATTR(USB_TYPE),
POWER_SUPPLY_ENUM_ATTR(SCOPE),
POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
IOW you can now set ps_attr->text_values* for POWER_SUPPLY_PROP_USB_TYPE
too, without needing to worry this causing power_supply_show_usb_type()
no longer to get called.
The reason I'm mentioning this is because power_supply_show_usb_type()
and power_supply_charge_behaviour_show() show a lot of code-duplication.
And I think that we can have one generic function replacing both
by using ps_attr->text_values* instead of hardcoding POWER_SUPPLY_USB_TYPE_TEXT
resp. power_supply_charge_behaviour_show (and yes this contradicts
my earlier comment on patch 4/4).
Although at a closer look I see now that the usb-types code uses
a list of supported type enum values in the power_supply_desc,
where as the charge_behavior code uses a bitmask which you retrieve
through a new property.
Thinking more about this, I think that adding a fake
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE property as you do here
might actually not be the best idea. All the other properties
are visible in sysfs, so this one is a bit weird. I think it might
be better to add this info to power_supply_desc like how this is
done for the usb-types.
And:
const enum power_supply_usb_type *usb_types;
size_t num_usb_types;
Should probably be converted into a bitmask too. I checked
and there are not that many users of this.
Once we have that as a bitmask too, we can refactor
power_supply_show_usb_type() and power_supply_charge_behaviour_show()
into a single new helper.
But I believe that refactoring:
const enum power_supply_usb_type *usb_types;
size_t num_usb_types;
into a bitmask is out of scope for this series. So I guess
that for now just add the bitmask of available charge behaviors
to power_supply_desc rather then making it a fake property
and then in the future we can do the refactor of usb-type
to a bitmask and remove the code duplication between
power_supply_show_usb_type() and power_supply_charge_behaviour_show()
Sebastian, any comments ?
Regards,
Hans
>> @@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
>> ret = power_supply_show_usb_type(dev, psy->desc,
>> &value, buf);
>> break;
>> + case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>> + ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
>> + &value, buf);
>> + break;
>> case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>> ret = sysfs_emit(buf, "%s\n", value.strval);
>> break;
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index c0992a77feea..9a6e6b488164 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -135,6 +135,7 @@ enum power_supply_property {
>> POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
>> POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
>> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
>> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>> POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>> POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
>>
>
Hi,
On Mon, Feb 05, 2024 at 12:43:54PM +0100, Hans de Goede wrote:
> On 2/5/24 10:52, Hans de Goede wrote:
> > Thank you for your patches for this.
Indeed, thanks for cleaning up this mess.
> > On 2/4/24 18:26, Thomas Wei?schuh wrote:
> >> This property is documented to have a special format which exposes all
> >> available behaviours and the currently active one at the same time.
> >> For this special format some helpers are provided.
> >>
> >> However the default property logic in power_supply_sysfs.c is not using
> >> the helper and the default logic only prints the currently active
> >> behaviour.
> >>
> >> Adjust power_supply_sysfs.c to follow the documented format.
> >>
> >> There are currently two in-tree drivers exposing charge behaviours:
> >> thinkpad_acpi and mm8013.
> >> thinkpad_acpi is not affected by the change, as it directly uses the
> >> helpers and does not use the power_supply_sysfs.c logic.
> >>
> >> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
> >> the new logic will preserve the simple output format in this case.
> >>
> >> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> >> Signed-off-by: Thomas Wei?schuh <[email protected]>
> >> ---
> >> drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
> >> include/linux/power_supply.h | 1 +
> >> 2 files changed, 33 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> >> index 977611e16373..3680cfc2e908 100644
> >> --- a/drivers/power/supply/power_supply_sysfs.c
> >> +++ b/drivers/power/supply/power_supply_sysfs.c
> >> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
> >> return count;
> >> }
> >>
> >> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> >> + struct power_supply *psy,
> >> + struct power_supply_attr *ps_attr,
> >> + union power_supply_propval *value,
> >> + char *buf)
> >> +{
> >> + union power_supply_propval available;
> >> + int ret;
> >> +
> >> + ret = power_supply_get_property(psy,
> >> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> >> + value);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + ret = power_supply_get_property(psy,
> >> + POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> >> + &available);
> >> + if (ret == -EINVAL)
> >> + return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
> >> + else if (ret < 0)
> >
> > No need for "else if" here since the if above does a return. So you can just do:
> >
> > if (ret < 0)
> > return ret;
> >
> >
> >> + return ret;
> >> +
> >> + return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> >> +}
> >> +
> >> static ssize_t power_supply_show_property(struct device *dev,
> >> struct device_attribute *attr,
> >> char *buf) {
> >> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
> >>
> >> if (psp == POWER_SUPPLY_PROP_TYPE) {
> >> value.intval = psy->desc->type;
> >> + } else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
> >> + value.intval = -1;
> >> } else {
> >> ret = power_supply_get_property(psy, psp, &value);
> >>
> >
> > I'm not a fan of this, I guess that you are doing this because you do not
> > want to enter this if:
> >
> > if (ps_attr->text_values_len > 0 &&
> > value.intval < ps_attr->text_values_len && value.intval >= 0) {
> > return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> > }
> >
> > But by doing this you add both the special case of setting value.intval = -1
> > here and you now need to do the power_supply_get_property() which is in the else
> > manually in power_supply_show_charge_behaviour() and the error handling / logging
> > of power_supply_get_property() in power_supply_show_charge_behaviour() is different.
> >
> > What I think you can (and should) do here instead is move:
> >
> > if (ps_attr->text_values_len > 0 &&
> > value.intval < ps_attr->text_values_len && value.intval >= 0) {
> > return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> > }
> >
> > into the default case of the switch (psp) {} below it in a preparation patch.
> >
> > This if is never entered for the 2 non default cases in that switch, so it
> > is safe to move it into the default case, e.g. something like this:
> >
> > 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);
> >
> > I think that also actually makes things a bit cleaner then the current
> > early-exit for printing enum values.
> >
> > And then in the next patch you can just add:
> >
> > case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> > ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> > &value, buf);
> > break;
> >
> > Without needing to set intval = -1 and without needing to get the value
> > inside power_supply_show_charge_behaviour() since that will already
> > be done for you then.
>
> One more note on this. With the suggested preparation patch to move
> the if checking for ps_attr->text_values into the default case,
> you can then also properly turn POWER_SUPPLY_PROP_USB_TYPE into an
> enum too:
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 977611e16373..768df64330f4 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -209,7 +209,7 @@ static struct power_supply_attr power_supply_attrs[] = {
> POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
> POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
> POWER_SUPPLY_ENUM_ATTR(TYPE),
> - POWER_SUPPLY_ATTR(USB_TYPE),
> + POWER_SUPPLY_ENUM_ATTR(USB_TYPE),
> POWER_SUPPLY_ENUM_ATTR(SCOPE),
> POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
> POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
>
> IOW you can now set ps_attr->text_values* for POWER_SUPPLY_PROP_USB_TYPE
> too, without needing to worry this causing power_supply_show_usb_type()
> no longer to get called.
>
> The reason I'm mentioning this is because power_supply_show_usb_type()
> and power_supply_charge_behaviour_show() show a lot of code-duplication.
>
> And I think that we can have one generic function replacing both
> by using ps_attr->text_values* instead of hardcoding POWER_SUPPLY_USB_TYPE_TEXT
> resp. power_supply_charge_behaviour_show (and yes this contradicts
> my earlier comment on patch 4/4).
>
> Although at a closer look I see now that the usb-types code uses
> a list of supported type enum values in the power_supply_desc,
> where as the charge_behavior code uses a bitmask which you retrieve
> through a new property.
>
> Thinking more about this, I think that adding a fake
> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE property as you do here
> might actually not be the best idea. All the other properties
> are visible in sysfs, so this one is a bit weird. I think it might
> be better to add this info to power_supply_desc like how this is
> done for the usb-types.
>
> And:
> const enum power_supply_usb_type *usb_types;
> size_t num_usb_types;
>
> Should probably be converted into a bitmask too. I checked
> and there are not that many users of this.
>
> Once we have that as a bitmask too, we can refactor
> power_supply_show_usb_type() and power_supply_charge_behaviour_show()
> into a single new helper.
>
> But I believe that refactoring:
>
> const enum power_supply_usb_type *usb_types;
> size_t num_usb_types;
>
> into a bitmask is out of scope for this series. So I guess
> that for now just add the bitmask of available charge behaviors
> to power_supply_desc rather then making it a fake property
> and then in the future we can do the refactor of usb-type
> to a bitmask and remove the code duplication between
> power_supply_show_usb_type() and power_supply_charge_behaviour_show()
>
> Sebastian, any comments ?
Sorry for the delay coming to this series; I fully agree with this.
Thanks for the thorough review.
-- Sebastian