Re: [PATCH v5 2/4] power: supply: core: implement extension API

[Sebastian Reichel]

Hi,

On Thu, Dec 05, 2024 at 09:46:36PM +0100, Thomas Weißschuh wrote:
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
> 
> While the created sysfs attributes look similar to the attributes
> provided by the powersupply core, there are various deficiencies:
> 
> * They don't show up in uevent payload.
> * They can't be queried with the standard in-kernel APIs.
> * They don't work with triggers.
> * The extending driver has to reimplement all of the parsing,
> formatting and sysfs display logic.
> * Writing a extension driver is completely different from writing a
> normal power supply driver.
> 
> This extension API avoids all of these issues.
> An extension is just a "struct power_supply_ext" with the same kind of
> callbacks as in a normal "struct power_supply_desc".
> 
> The API is meant to be used via battery_hook_register(), the same way as
> the current extensions.
> 
> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> ---

[...]

> @@ -1241,7 +1297,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
>  int power_supply_property_is_writeable(struct power_supply *psy,
>  					enum power_supply_property psp)
>  {
> -	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
> +	struct power_supply_ext_registration *reg;
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->property_is_writeable)
> +				return reg->ext->property_is_writeable(psy, reg->ext,
> +								       reg->data, psp);
> +			else
> +				return -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
> +		return -ENODEV;
> +
> +	return psy->desc->property_is_writeable(psy, psp);
>  }

I think the two 'return -ENODEV' should just be 'return 0'. This
function basically returns a bool. Otherwise the patch looks good to
me. Thanks!

-- Sebastian

Attachment: signature.asc
Description: PGP signature