Re: [PATCH v3 01/23] thermal: armada: add a function that sanitizes the thermal zone name

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 16/07/2018 16:41, Miquel Raynal wrote:
> Thermal zone names must follow certain rules imposed by the framework.
> They are limited in length and shall not have any hyphen '-'.
> 
> This is done in a separate function for future use in another location.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Why do you have to provide a function to test that?

Logically, the one who did the change to add a thermal name, should
check its code works. Without a proper name that won't work.

So this function is testing something which should be already tested, no?



> ---
>  drivers/thermal/armada_thermal.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index 4c275ec10ac5..077e8e562306 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -70,6 +70,7 @@ struct armada_thermal_priv {
>  	void __iomem *status;
>  	void __iomem *control0;
>  	void __iomem *control1;
> +	char zone_name[THERMAL_NAME_LENGTH];
>  	struct armada_thermal_data *data;
>  };
>  
> @@ -353,6 +354,36 @@ static const struct of_device_id armada_thermal_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(of, armada_thermal_id_table);
>  
> +static void armada_set_sane_name(struct platform_device *pdev,
> +				 struct armada_thermal_priv *priv)
> +{
> +	const char *name = dev_name(&pdev->dev);
> +	char *insane_char;
> +
> +	if (strlen(name) > THERMAL_NAME_LENGTH) {
> +		/*
> +		 * When inside a system controller, the device name has the
> +		 * form: f06f8000.system-controller:ap-thermal so stripping
> +		 * after the ':' should give us a shorter but meaningful name.
> +		 */
> +		name = strrchr(name, ':');
> +		if (!name)
> +			name = "armada_thermal";
> +		else
> +			name++;
> +	}
> +
> +	/* Save the name locally */
> +	strncpy(priv->zone_name, name, THERMAL_NAME_LENGTH);
> +
> +	/* Then check there are no '-' or hwmon core will complain */
> +	do {
> +		insane_char = strpbrk(priv->zone_name, "-");
> +		if (insane_char)
> +			*insane_char = '_';
> +	} while (insane_char);
> +}
> +
>  static int armada_thermal_probe(struct platform_device *pdev)
>  {
>  	void __iomem *control = NULL;
> @@ -381,6 +412,9 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  
>  	priv->data = (struct armada_thermal_data *)match->data;
>  
> +	/* Ensure device name is correct for the thermal core */
> +	armada_set_sane_name(pdev, priv);
> +
>  	/*
>  	 * Legacy DT bindings only described "control1" register (also referred
>  	 * as "control MSB" on old documentation). New bindings cover
> @@ -402,7 +436,7 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  
>  	priv->data->init_sensor(pdev, priv);
>  
> -	thermal = thermal_zone_device_register(dev_name(&pdev->dev), 0, 0, priv,
> +	thermal = thermal_zone_device_register(priv->zone_name, 0, 0, priv,
>  					       &ops, NULL, 0, 0);
>  	if (IS_ERR(thermal)) {
>  		dev_err(&pdev->dev,
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel