Received-SPF: pass (google.com: domain of linux-kernel+bounces-52352-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <53082075-852f-4698-b354-ed30e7fd2683@redhat.com>
Date: Mon, 5 Feb 2024 10:52:17 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] power: supply: core: fix charge_behaviour formatting
To: =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>,
 Sebastian Reichel <sre@kernel.org>, Konrad Dybcio <konradybcio@kernel.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
 Sebastian Reichel <sebastian.reichel@collabora.com>
References: <20240204-power_supply-charge_behaviour_prop-v1-0-06a20c958f96@weissschuh.net>
 <20240204-power_supply-charge_behaviour_prop-v1-1-06a20c958f96@weissschuh.net>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240204-power_supply-charge_behaviour_prop-v1-1-06a20c958f96@weissschuh.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 <linux@weissschuh.net>
> ---
>  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,
>