Re: [PATCH v3 08/23] thermal: armada: use the resource managed registration helper alternative

[Eduardo Valentin <edubezval@gmail.com>]

Hey,

On Mon, Jul 16, 2018 at 04:41:51PM +0200, Miquel Raynal wrote:
> Current use of thermal_zone_device_register() triggers a warning at boot
> and should be replaced by devm_thermal_zone_of_sensor_register(). This
> allows better handling of multiple thermal zones for later multi-sensors
> support.
> 
> Also change the driver data to embed a new structure to make the
> difference between legacy data (which needs to be cleaned) and
> syscon-related data.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/thermal/armada_thermal.c | 123 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 101 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index be346c6afde2..dd894312dedf 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -64,6 +64,7 @@ struct armada_thermal_data;
>  
>  /* Marvell EBU Thermal Sensor Dev Structure */
>  struct armada_thermal_priv {
> +	struct device *dev;
>  	struct regmap *syscon;
>  	char zone_name[THERMAL_NAME_LENGTH];
>  	struct armada_thermal_data *data;
> @@ -95,6 +96,26 @@ struct armada_thermal_data {
>  	unsigned int syscon_status_off;
>  };
>  
> +struct armada_drvdata {
> +	enum drvtype {
> +		LEGACY,
> +		SYSCON
> +	} type;
> +	union {
> +		struct armada_thermal_priv *priv;
> +		struct thermal_zone_device *tz;
> +	} data;
> +};
> +
> +/*
> + * struct armada_thermal_sensor - hold the information of one thermal sensor
> + * @thermal: pointer to the local private structure
> + * @tzd: pointer to the thermal zone device
> + */
> +struct armada_thermal_sensor {
> +	struct armada_thermal_priv *priv;
> +};
> +
>  static void armadaxp_init(struct platform_device *pdev,
>  			  struct armada_thermal_priv *priv)
>  {
> @@ -243,16 +264,14 @@ static bool armada_is_valid(struct armada_thermal_priv *priv)
>  	return reg & priv->data->is_valid_bit;
>  }
>  
> -static int armada_get_temp(struct thermal_zone_device *thermal,
> -			   int *temp)
> +static int armada_read_sensor(struct armada_thermal_priv *priv, int *temp)
>  {
> -	struct armada_thermal_priv *priv = thermal->devdata;
>  	u32 reg, div;
>  	s64 sample, b, m;
>  
>  	/* Valid check */
>  	if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> -		dev_err(&thermal->device,
> +		dev_err(priv->dev,
>  			"Temperature sensor reading not valid\n");
>  		return -EIO;
>  	}
> @@ -278,7 +297,33 @@ static int armada_get_temp(struct thermal_zone_device *thermal,
>  	return 0;
>  }
>  
> -static struct thermal_zone_device_ops ops = {
> +static int armada_get_temp_legacy(struct thermal_zone_device *thermal,
> +				  int *temp)
> +{
> +	struct armada_thermal_priv *priv = thermal->devdata;
> +	int ret;
> +
> +	/* Do the actual reading */
> +	ret = armada_read_sensor(priv, temp);
> +
> +	return ret;
> +}
> +
> +static struct thermal_zone_device_ops legacy_ops = {
> +	.get_temp = armada_get_temp_legacy,
> +};
> +
> +static int armada_get_temp(void *_sensor, int *temp)
> +{
> +	struct armada_thermal_sensor *sensor = _sensor;
> +	struct armada_thermal_priv *priv = sensor->priv;
> +	int ret;


The above variable:
drivers/thermal/armada_thermal.c:320:6: warning: unused variable ‘ret’ [-Wunused-variable]
  int ret;
      ^~~
cc1: some warnings being treated as errors
scripts/Makefile.build:323: recipe for target 'drivers/thermal/armada_thermal.o' failed


> +
> +	/* Do the actual reading */
> +	return armada_read_sensor(priv, &temp);

I think you meant 

+	return armada_read_sensor(priv, temp);

here right?


I obviously get:

  CC [M]  drivers/thermal/armada_thermal.o
drivers/thermal/armada_thermal.c: In function ‘armada_get_temp’:
drivers/thermal/armada_thermal.c:323:34: error: passing argument 2 of ‘armada_read_sensor’ from incompatible pointer type [-Werror=incompatible-pointer-types]
  return armada_read_sensor(priv, &temp);
                                  ^
drivers/thermal/armada_thermal.c:267:12: note: expected ‘int *’ but argument is of type ‘int **’
 static int armada_read_sensor(struct armada_thermal_priv *priv, int *temp)
            ^~~~~~~~~~~~~~~~~~


> +}
> +
> +static struct thermal_zone_of_device_ops of_ops = {
>  	.get_temp = armada_get_temp,
>  };
>  
> @@ -480,7 +525,9 @@ static void armada_set_sane_name(struct platform_device *pdev,
>  
>  static int armada_thermal_probe(struct platform_device *pdev)
>  {
> -	struct thermal_zone_device *thermal;
> +	struct thermal_zone_device *tz;
> +	struct armada_thermal_sensor *sensors;
> +	struct armada_drvdata *drvdata;
>  	const struct of_device_id *match;
>  	struct armada_thermal_priv *priv;
>  	int ret;
> @@ -493,11 +540,13 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
>  	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). Then, bindings moved to cover
> @@ -510,35 +559,65 @@ static int armada_thermal_probe(struct platform_device *pdev)
>  	 * is to define an overall system controller and put the thermal node
>  	 * into it, which requires the use of regmaps across all the driver.
>  	 */
> -	if (IS_ERR(syscon_node_to_regmap(pdev->dev.parent->of_node)))
> +	if (IS_ERR(syscon_node_to_regmap(pdev->dev.parent->of_node))) {
> +		/* Ensure device name is correct for the thermal core */
> +		armada_set_sane_name(pdev, priv);
> +
>  		ret = armada_thermal_probe_legacy(pdev, priv);
> -	else
> -		ret = armada_thermal_probe_syscon(pdev, priv);
> +		if (ret)
> +			return ret;
>  
> +		priv->data->init(pdev, priv);
> +
> +		tz = thermal_zone_device_register(priv->zone_name, 0, 0, priv,
> +						  &legacy_ops, NULL, 0, 0);
> +		if (IS_ERR(tz)) {
> +			dev_err(&pdev->dev,
> +				"Failed to register thermal zone device\n");
> +			return PTR_ERR(tz);
> +		}
> +
> +		drvdata->type = LEGACY;
> +		drvdata->data.tz = tz;
> +		platform_set_drvdata(pdev, drvdata);
> +
> +		return 0;
> +	}
> +
> +	ret = armada_thermal_probe_syscon(pdev, priv);
>  	if (ret)
>  		return ret;
>  
>  	priv->data->init(pdev, priv);
> +	drvdata->type = SYSCON;
> +	drvdata->data.priv = priv;
> +	platform_set_drvdata(pdev, drvdata);
>  
> -	thermal = thermal_zone_device_register(priv->zone_name, 0, 0, priv,
> -					       &ops, NULL, 0, 0);
> -	if (IS_ERR(thermal)) {
> +	sensors = devm_kzalloc(&pdev->dev, sizeof(struct armada_thermal_sensor),
> +			       GFP_KERNEL);
> +	if (!sensors)
> +		return -ENOMEM;
> +
> +	sensors->priv = priv;
> +
> +	tz = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, sensors,
> +						  &of_ops);
> +	if (IS_ERR(tz)) {
>  		dev_err(&pdev->dev,
> -			"Failed to register thermal zone device\n");
> -		return PTR_ERR(thermal);
> +			"Failed to register thermal sensor (err: %ld)\n",
> +			PTR_ERR(tz));
> +		return PTR_ERR(tz);
>  	}
>  
> -	platform_set_drvdata(pdev, thermal);
> -
>  	return 0;
>  }
>  
>  static int armada_thermal_exit(struct platform_device *pdev)
>  {
> -	struct thermal_zone_device *armada_thermal =
> -		platform_get_drvdata(pdev);
> +	struct armada_drvdata *drvdata = platform_get_drvdata(pdev);
>  
> -	thermal_zone_device_unregister(armada_thermal);
> +	if (drvdata->type == LEGACY)
> +		thermal_zone_device_unregister(drvdata->data.tz);
>  
>  	return 0;
>  }
> -- 
> 2.14.1
> 

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