Re: [PATCH] power: supply: register HWMON devices with valid names

[Guenter Roeck]

On Thu, Aug 22, 2019 at 05:09:19PM +0200, Romain Izard wrote:
> With the introduction of the HWMON compatibility layer to the power
> supply framework in Linux 5.3, all power supply devices' names can be
> used directly to create HWMON devices with the same names.
> 
> But HWMON has rules on allowable names that are different from the power
> supply framework. The dash character is forbidden, as it is used by the
> libsensors library in userspace as a separator, whereas this character
> is used in the device names in more than half of the existing power
> supply drivers. This last case is consistent with the typical naming
> usage with MFD and Device Tree.
> 
> This leads to warnings in the kernel log, with the format:
> 
> power_supply gpio-charger: hwmon: \
> 	'gpio-charger' is not a valid name attribute, please fix
> 
> Add a protection to power_supply_add_hwmon_sysfs() that replaces any
> dash in the device name with an underscore when registering with the
> HWMON framework. Other forbidden characters (star, slash, space, tab,
> newline) are not replaced, as they are not in common use.
> 
> Fixes: e67d4dfc9ff1 ("power: supply: Add HWMON compatibility layer")
> Signed-off-by: Romain Izard <romain.izard.pro@xxxxxxxxx>
> ---
>  drivers/power/supply/power_supply_hwmon.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index 51fe60440d12..ebe964bd512c 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -284,6 +284,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>  	struct device *dev = &psy->dev;
>  	struct device *hwmon;
>  	int ret, i;
> +	const char *name;
>  
>  	if (!devres_open_group(dev, power_supply_add_hwmon_sysfs,
>  			       GFP_KERNEL))
> @@ -334,7 +335,19 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>  		}
>  	}
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name,
> +	name = psy->desc->name;
> +	if (strchr(name, '-')) {
> +		char *new_name;
> +
> +		new_name = devm_kstrdup(dev, name, GFP_KERNEL);
> +		if (!new_name) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +		strreplace(new_name, '-', '_');
> +		name = (const char *) new_name;

Is that typecast necessary ?

Other than that,

Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

> +	}
> +	hwmon = devm_hwmon_device_register_with_info(dev, name,
>  						psyhw,
>  						&power_supply_hwmon_chip_info,
>  						NULL);
> -- 
> 2.17.1
>