[PATCH] thermal/drivers/of: Add a get_temp_id callback function

[PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Daniel Lezcano]

Currently when we register a sensor, we specify the sensor id and a data
pointer to be passed when the get_temp function is called. However the
sensor_id is not passed to the get_temp callback forcing the driver to
do extra allocation and adding back pointer to find out from the sensor
information the driver data and then back to the sensor id.

Add a new callback get_temp_id() which will be called if set. It will
call the get_temp_id() with the sensor id.

That will be more consistent with the registering function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/thermal/of-thermal.c | 19 +++++++++++++------
 include/linux/thermal.h      |  1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 2df059cc07e2..787d1cbe13f3 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -78,6 +78,8 @@ struct __thermal_zone {
 
 	/* sensor interface */
 	void *sensor_data;
+	int sensor_id;
+
 	const struct thermal_zone_of_device_ops *ops;
 };
 
@@ -88,10 +90,14 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
 {
 	struct __thermal_zone *data = tz->devdata;
 
-	if (!data->ops->get_temp)
-		return -EINVAL;
+	if (data->ops->get_temp)
+		return data->ops->get_temp(data->sensor_data, temp);
 
-	return data->ops->get_temp(data->sensor_data, temp);
+	if (data->ops->get_temp_id)
+		return data->ops->get_temp_id(data->sensor_id,
+					      data->sensor_data, temp);
+
+	return -EINVAL;
 }
 
 static int of_thermal_set_trips(struct thermal_zone_device *tz,
@@ -407,8 +413,8 @@ static struct thermal_zone_device_ops of_thermal_ops = {
 /***   sensor API   ***/
 
 static struct thermal_zone_device *
-thermal_zone_of_add_sensor(struct device_node *zone,
-			   struct device_node *sensor, void *data,
+thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor,
+			   int sensor_id, void *data,
 			   const struct thermal_zone_of_device_ops *ops)
 {
 	struct thermal_zone_device *tzd;
@@ -426,6 +432,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
 	mutex_lock(&tzd->lock);
 	tz->ops = ops;
 	tz->sensor_data = data;
+	tz->sensor_id = sensor_id;
 
 	tzd->ops->get_temp = of_thermal_get_temp;
 	tzd->ops->get_trend = of_thermal_get_trend;
@@ -516,7 +523,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
 		}
 
 		if (sensor_specs.np == sensor_np && id == sensor_id) {
-			tzd = thermal_zone_of_add_sensor(child, sensor_np,
+			tzd = thermal_zone_of_add_sensor(child, sensor_np, sensor_id,
 							 data, ops);
 			if (!IS_ERR(tzd))
 				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 5f4705f46c2f..2762d7e6dd86 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -351,6 +351,7 @@ struct thermal_genl_event {
  *		   hardware.
  */
 struct thermal_zone_of_device_ops {
+	int (*get_temp_id)(int, void *, int *);
 	int (*get_temp)(void *, int *);
 	int (*get_trend)(void *, int, enum thermal_trend *);
 	int (*set_trips)(void *, int, int);
-- 
2.17.1

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Eduardo Valentin]

Hello,

On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
> Currently when we register a sensor, we specify the sensor id and a data
> pointer to be passed when the get_temp function is called. However the
> sensor_id is not passed to the get_temp callback forcing the driver to
> do extra allocation and adding back pointer to find out from the sensor
> information the driver data and then back to the sensor id.
> 
> Add a new callback get_temp_id() which will be called if set. It will
> call the get_temp_id() with the sensor id.
> 
> That will be more consistent with the registering function.

I still do not understand why we need to have a get_id callback.
The use cases I have seen so far, which I have been intentionally rejecting, are
mainly solvable by creating other compatible entries. And really, if you
have, say a bandgap, chip that supports multiple sensors, but on
SoC version A it has 5 sensors, and on SoC version B it has only 4,
or on SoC version C, it has 5 but they are either logially located
in different places (gpu vs iva regions), these are all cases in which
you want a different compatible!

Do you mind sharing why you need a get sensor id callback?

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/thermal/of-thermal.c | 19 +++++++++++++------
>  include/linux/thermal.h      |  1 +
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 2df059cc07e2..787d1cbe13f3 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -78,6 +78,8 @@ struct __thermal_zone {
>  
>  	/* sensor interface */
>  	void *sensor_data;
> +	int sensor_id;
> +
>  	const struct thermal_zone_of_device_ops *ops;
>  };
>  
> @@ -88,10 +90,14 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>  {
>  	struct __thermal_zone *data = tz->devdata;
>  
> -	if (!data->ops->get_temp)
> -		return -EINVAL;
> +	if (data->ops->get_temp)
> +		return data->ops->get_temp(data->sensor_data, temp);
>  
> -	return data->ops->get_temp(data->sensor_data, temp);
> +	if (data->ops->get_temp_id)
> +		return data->ops->get_temp_id(data->sensor_id,
> +					      data->sensor_data, temp);
> +
> +	return -EINVAL;
>  }
>  
>  static int of_thermal_set_trips(struct thermal_zone_device *tz,
> @@ -407,8 +413,8 @@ static struct thermal_zone_device_ops of_thermal_ops = {
>  /***   sensor API   ***/
>  
>  static struct thermal_zone_device *
> -thermal_zone_of_add_sensor(struct device_node *zone,
> -			   struct device_node *sensor, void *data,
> +thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor,
> +			   int sensor_id, void *data,
>  			   const struct thermal_zone_of_device_ops *ops)
>  {
>  	struct thermal_zone_device *tzd;
> @@ -426,6 +432,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>  	mutex_lock(&tzd->lock);
>  	tz->ops = ops;
>  	tz->sensor_data = data;
> +	tz->sensor_id = sensor_id;
>  
>  	tzd->ops->get_temp = of_thermal_get_temp;
>  	tzd->ops->get_trend = of_thermal_get_trend;
> @@ -516,7 +523,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
>  		}
>  
>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
> -			tzd = thermal_zone_of_add_sensor(child, sensor_np,
> +			tzd = thermal_zone_of_add_sensor(child, sensor_np, sensor_id,
>  							 data, ops);
>  			if (!IS_ERR(tzd))
>  				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 5f4705f46c2f..2762d7e6dd86 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -351,6 +351,7 @@ struct thermal_genl_event {
>   *		   hardware.
>   */
>  struct thermal_zone_of_device_ops {
> +	int (*get_temp_id)(int, void *, int *);
>  	int (*get_temp)(void *, int *);
>  	int (*get_trend)(void *, int, enum thermal_trend *);
>  	int (*set_trips)(void *, int, int);
> -- 
> 2.17.1
> 

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Daniel Lezcano]

On 23/04/2019 17:44, Eduardo Valentin wrote:
> Hello,
> 
> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
>> Currently when we register a sensor, we specify the sensor id and a data
>> pointer to be passed when the get_temp function is called. However the
>> sensor_id is not passed to the get_temp callback forcing the driver to
>> do extra allocation and adding back pointer to find out from the sensor
>> information the driver data and then back to the sensor id.
>>
>> Add a new callback get_temp_id() which will be called if set. It will
>> call the get_temp_id() with the sensor id.
>>
>> That will be more consistent with the registering function.
> 
> I still do not understand why we need to have a get_id callback.
> The use cases I have seen so far, which I have been intentionally rejecting, are
> mainly solvable by creating other compatible entries. And really, if you
> have, say a bandgap, chip that supports multiple sensors, but on
> SoC version A it has 5 sensors, and on SoC version B it has only 4,
> or on SoC version C, it has 5 but they are either logially located
> in different places (gpu vs iva regions), these are all cases in which
> you want a different compatible!
> 
> Do you mind sharing why you need a get sensor id callback?

It is not a get sensor id callback, it is a get_temp callback which pass
the sensor id.

See in the different drivers, it is a common pattern there is a
structure for the driver, then a structure for the sensor. When the
get_temp is called, the callback needs info from the sensor structure
and from the driver structure, so a back pointer to the driver structure
is added in the sensor structure.

Example:

struct hisi_thermal_sensor {
        struct hisi_thermal_data *data;   <-- back pointer
        struct thermal_zone_device *tzd;
        const char *irq_name;
        uint32_t id; 		<-- note this field
        uint32_t thres_temp;
};

struct hisi_thermal_data {
        const struct hisi_thermal_ops *ops;
        struct hisi_thermal_sensor *sensor;
        struct platform_device *pdev;
        struct clk *clk;
        void __iomem *regs;
        int nr_sensors;
};


In the get_temp callback:

static int hisi_thermal_get_temp(void *__data, int *temp)
{
        struct hisi_thermal_sensor *sensor = __data;
        struct hisi_thermal_data *data = sensor->data;

        *temp = data->ops->get_temp(sensor);

        dev_dbg(&data->pdev->dev, "tzd=%p, id=%d, temp=%d, thres=%d\n",
                sensor->tzd, sensor->id, *temp, sensor->thres_temp);

        return 0;
}


Another example:

struct qoriq_sensor {
        struct thermal_zone_device      *tzd;
        struct qoriq_tmu_data           *qdata; <-- back pointer
        int                             id;  	<-- note this field
};

struct qoriq_tmu_data {
        struct qoriq_tmu_regs __iomem *regs;
        bool little_endian;
        struct qoriq_sensor     *sensor[SITES_MAX];
};

static int tmu_get_temp(void *p, int *temp)
{
        struct qoriq_sensor *qsensor = p;
        struct qoriq_tmu_data *qdata = qsensor->qdata;
        u32 val;

        val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr);
        *temp = (val & 0xff) * 1000;

        return 0;
}

Another example:


struct rockchip_thermal_sensor {
        struct rockchip_thermal_data *thermal;	<-- back pointer
        struct thermal_zone_device *tzd;
        int id;					<-- note this field
};

struct rockchip_thermal_data {
        const struct rockchip_tsadc_chip *chip;
        struct platform_device *pdev;
        struct reset_control *reset;

        struct rockchip_thermal_sensor sensors[SOC_MAX_SENSORS];

	[ ... ]
};


static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
{
        struct rockchip_thermal_sensor *sensor = _sensor;
        struct rockchip_thermal_data *thermal = sensor->thermal;
        const struct rockchip_tsadc_chip *tsadc = sensor->thermal->chip;
        int retval;

        retval = tsadc->get_temp(&tsadc->table,
                                 sensor->id, thermal->regs, out_temp);
        dev_dbg(&thermal->pdev->dev, "sensor %d - temp: %d, retval: %d\n",
                sensor->id, *out_temp, retval);

        return retval;
}

This patch adds an alternate get_temp_id() along with the get_temp()
ops. If the ops field for get_temp_id is filled, it will be invoked and
will pass the sensor id used when registering. It results the driver
structure can be passed and the sensor id gives the index in the sensor
 table in this structure. The back pointer is no longer needed, the id
field neither and I suspect, by domino effect, more structures will
disappear or will be simplified (eg. the above rockchip sensor structure
disappear and the thermal_data structure will contain an array of
thermal zone devices).

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  drivers/thermal/of-thermal.c | 19 +++++++++++++------
>>  include/linux/thermal.h      |  1 +
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index 2df059cc07e2..787d1cbe13f3 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -78,6 +78,8 @@ struct __thermal_zone {
>>  
>>  	/* sensor interface */
>>  	void *sensor_data;
>> +	int sensor_id;
>> +
>>  	const struct thermal_zone_of_device_ops *ops;
>>  };
>>  
>> @@ -88,10 +90,14 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>  {
>>  	struct __thermal_zone *data = tz->devdata;
>>  
>> -	if (!data->ops->get_temp)
>> -		return -EINVAL;
>> +	if (data->ops->get_temp)
>> +		return data->ops->get_temp(data->sensor_data, temp);
>>  
>> -	return data->ops->get_temp(data->sensor_data, temp);
>> +	if (data->ops->get_temp_id)
>> +		return data->ops->get_temp_id(data->sensor_id,
>> +					      data->sensor_data, temp);
>> +
>> +	return -EINVAL;
>>  }
>>  
>>  static int of_thermal_set_trips(struct thermal_zone_device *tz,
>> @@ -407,8 +413,8 @@ static struct thermal_zone_device_ops of_thermal_ops = {
>>  /***   sensor API   ***/
>>  
>>  static struct thermal_zone_device *
>> -thermal_zone_of_add_sensor(struct device_node *zone,
>> -			   struct device_node *sensor, void *data,
>> +thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor,
>> +			   int sensor_id, void *data,
>>  			   const struct thermal_zone_of_device_ops *ops)
>>  {
>>  	struct thermal_zone_device *tzd;
>> @@ -426,6 +432,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>  	mutex_lock(&tzd->lock);
>>  	tz->ops = ops;
>>  	tz->sensor_data = data;
>> +	tz->sensor_id = sensor_id;
>>  
>>  	tzd->ops->get_temp = of_thermal_get_temp;
>>  	tzd->ops->get_trend = of_thermal_get_trend;
>> @@ -516,7 +523,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
>>  		}
>>  
>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>> -			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>> +			tzd = thermal_zone_of_add_sensor(child, sensor_np, sensor_id,
>>  							 data, ops);
>>  			if (!IS_ERR(tzd))
>>  				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 5f4705f46c2f..2762d7e6dd86 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -351,6 +351,7 @@ struct thermal_genl_event {
>>   *		   hardware.
>>   */
>>  struct thermal_zone_of_device_ops {
>> +	int (*get_temp_id)(int, void *, int *);
>>  	int (*get_temp)(void *, int *);
>>  	int (*get_trend)(void *, int, enum thermal_trend *);
>>  	int (*set_trips)(void *, int, int);
>> -- 
>> 2.17.1
>>


-- 
 <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

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Daniel Lezcano]

On 24/04/2019 01:08, Daniel Lezcano wrote:
> On 23/04/2019 17:44, Eduardo Valentin wrote:
>> Hello,
>>
>> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
>>> Currently when we register a sensor, we specify the sensor id and a data
>>> pointer to be passed when the get_temp function is called. However the
>>> sensor_id is not passed to the get_temp callback forcing the driver to
>>> do extra allocation and adding back pointer to find out from the sensor
>>> information the driver data and then back to the sensor id.
>>>
>>> Add a new callback get_temp_id() which will be called if set. It will
>>> call the get_temp_id() with the sensor id.
>>>
>>> That will be more consistent with the registering function.
>>
>> I still do not understand why we need to have a get_id callback.
>> The use cases I have seen so far, which I have been intentionally rejecting, are
>> mainly solvable by creating other compatible entries. And really, if you
>> have, say a bandgap, chip that supports multiple sensors, but on
>> SoC version A it has 5 sensors, and on SoC version B it has only 4,
>> or on SoC version C, it has 5 but they are either logially located
>> in different places (gpu vs iva regions), these are all cases in which
>> you want a different compatible!
>>
>> Do you mind sharing why you need a get sensor id callback?
> 
> It is not a get sensor id callback, it is a get_temp callback which pass
> the sensor id.
> 
> See in the different drivers, it is a common pattern there is a
> structure for the driver, then a structure for the sensor. When the
> get_temp is called, the callback needs info from the sensor structure
> and from the driver structure, so a back pointer to the driver structure
> is added in the sensor structure.

Hi Eduardo,

does the explanation clarifies the purpose of this change?



> Example:
> 
> struct hisi_thermal_sensor {
>         struct hisi_thermal_data *data;   <-- back pointer
>         struct thermal_zone_device *tzd;
>         const char *irq_name;
>         uint32_t id; 		<-- note this field
>         uint32_t thres_temp;
> };
> 
> struct hisi_thermal_data {
>         const struct hisi_thermal_ops *ops;
>         struct hisi_thermal_sensor *sensor;
>         struct platform_device *pdev;
>         struct clk *clk;
>         void __iomem *regs;
>         int nr_sensors;
> };
> 
> 
> In the get_temp callback:
> 
> static int hisi_thermal_get_temp(void *__data, int *temp)
> {
>         struct hisi_thermal_sensor *sensor = __data;
>         struct hisi_thermal_data *data = sensor->data;
> 
>         *temp = data->ops->get_temp(sensor);
> 
>         dev_dbg(&data->pdev->dev, "tzd=%p, id=%d, temp=%d, thres=%d\n",
>                 sensor->tzd, sensor->id, *temp, sensor->thres_temp);
> 
>         return 0;
> }
> 
> 
> Another example:
> 
> struct qoriq_sensor {
>         struct thermal_zone_device      *tzd;
>         struct qoriq_tmu_data           *qdata; <-- back pointer
>         int                             id;  	<-- note this field
> };
> 
> struct qoriq_tmu_data {
>         struct qoriq_tmu_regs __iomem *regs;
>         bool little_endian;
>         struct qoriq_sensor     *sensor[SITES_MAX];
> };
> 
> static int tmu_get_temp(void *p, int *temp)
> {
>         struct qoriq_sensor *qsensor = p;
>         struct qoriq_tmu_data *qdata = qsensor->qdata;
>         u32 val;
> 
>         val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr);
>         *temp = (val & 0xff) * 1000;
> 
>         return 0;
> }
> 
> Another example:
> 
> 
> struct rockchip_thermal_sensor {
>         struct rockchip_thermal_data *thermal;	<-- back pointer
>         struct thermal_zone_device *tzd;
>         int id;					<-- note this field
> };
> 
> struct rockchip_thermal_data {
>         const struct rockchip_tsadc_chip *chip;
>         struct platform_device *pdev;
>         struct reset_control *reset;
> 
>         struct rockchip_thermal_sensor sensors[SOC_MAX_SENSORS];
> 
> 	[ ... ]
> };
> 
> 
> static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
> {
>         struct rockchip_thermal_sensor *sensor = _sensor;
>         struct rockchip_thermal_data *thermal = sensor->thermal;
>         const struct rockchip_tsadc_chip *tsadc = sensor->thermal->chip;
>         int retval;
> 
>         retval = tsadc->get_temp(&tsadc->table,
>                                  sensor->id, thermal->regs, out_temp);
>         dev_dbg(&thermal->pdev->dev, "sensor %d - temp: %d, retval: %d\n",
>                 sensor->id, *out_temp, retval);
> 
>         return retval;
> }
> 
> This patch adds an alternate get_temp_id() along with the get_temp()
> ops. If the ops field for get_temp_id is filled, it will be invoked and
> will pass the sensor id used when registering. It results the driver
> structure can be passed and the sensor id gives the index in the sensor
>  table in this structure. The back pointer is no longer needed, the id
> field neither and I suspect, by domino effect, more structures will
> disappear or will be simplified (eg. the above rockchip sensor structure
> disappear and the thermal_data structure will contain an array of
> thermal zone devices).
> 
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> ---
>>>  drivers/thermal/of-thermal.c | 19 +++++++++++++------
>>>  include/linux/thermal.h      |  1 +
>>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>> index 2df059cc07e2..787d1cbe13f3 100644
>>> --- a/drivers/thermal/of-thermal.c
>>> +++ b/drivers/thermal/of-thermal.c
>>> @@ -78,6 +78,8 @@ struct __thermal_zone {
>>>  
>>>  	/* sensor interface */
>>>  	void *sensor_data;
>>> +	int sensor_id;
>>> +
>>>  	const struct thermal_zone_of_device_ops *ops;
>>>  };
>>>  
>>> @@ -88,10 +90,14 @@ static int of_thermal_get_temp(struct thermal_zone_device *tz,
>>>  {
>>>  	struct __thermal_zone *data = tz->devdata;
>>>  
>>> -	if (!data->ops->get_temp)
>>> -		return -EINVAL;
>>> +	if (data->ops->get_temp)
>>> +		return data->ops->get_temp(data->sensor_data, temp);
>>>  
>>> -	return data->ops->get_temp(data->sensor_data, temp);
>>> +	if (data->ops->get_temp_id)
>>> +		return data->ops->get_temp_id(data->sensor_id,
>>> +					      data->sensor_data, temp);
>>> +
>>> +	return -EINVAL;
>>>  }
>>>  
>>>  static int of_thermal_set_trips(struct thermal_zone_device *tz,
>>> @@ -407,8 +413,8 @@ static struct thermal_zone_device_ops of_thermal_ops = {
>>>  /***   sensor API   ***/
>>>  
>>>  static struct thermal_zone_device *
>>> -thermal_zone_of_add_sensor(struct device_node *zone,
>>> -			   struct device_node *sensor, void *data,
>>> +thermal_zone_of_add_sensor(struct device_node *zone, struct device_node *sensor,
>>> +			   int sensor_id, void *data,
>>>  			   const struct thermal_zone_of_device_ops *ops)
>>>  {
>>>  	struct thermal_zone_device *tzd;
>>> @@ -426,6 +432,7 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>>>  	mutex_lock(&tzd->lock);
>>>  	tz->ops = ops;
>>>  	tz->sensor_data = data;
>>> +	tz->sensor_id = sensor_id;
>>>  
>>>  	tzd->ops->get_temp = of_thermal_get_temp;
>>>  	tzd->ops->get_trend = of_thermal_get_trend;
>>> @@ -516,7 +523,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
>>>  		}
>>>  
>>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>> -			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>> +			tzd = thermal_zone_of_add_sensor(child, sensor_np, sensor_id,
>>>  							 data, ops);
>>>  			if (!IS_ERR(tzd))
>>>  				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>> index 5f4705f46c2f..2762d7e6dd86 100644
>>> --- a/include/linux/thermal.h
>>> +++ b/include/linux/thermal.h
>>> @@ -351,6 +351,7 @@ struct thermal_genl_event {
>>>   *		   hardware.
>>>   */
>>>  struct thermal_zone_of_device_ops {
>>> +	int (*get_temp_id)(int, void *, int *);
>>>  	int (*get_temp)(void *, int *);
>>>  	int (*get_trend)(void *, int, enum thermal_trend *);
>>>  	int (*set_trips)(void *, int, int);
>>> -- 
>>> 2.17.1
>>>
> 
> 


-- 
 <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

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Andrey Smirnov]

On Mon, Apr 29, 2019 at 9:51 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 24/04/2019 01:08, Daniel Lezcano wrote:
> > On 23/04/2019 17:44, Eduardo Valentin wrote:
> >> Hello,
> >>
> >> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
> >>> Currently when we register a sensor, we specify the sensor id and a data
> >>> pointer to be passed when the get_temp function is called. However the
> >>> sensor_id is not passed to the get_temp callback forcing the driver to
> >>> do extra allocation and adding back pointer to find out from the sensor
> >>> information the driver data and then back to the sensor id.
> >>>
> >>> Add a new callback get_temp_id() which will be called if set. It will
> >>> call the get_temp_id() with the sensor id.
> >>>
> >>> That will be more consistent with the registering function.
> >>
> >> I still do not understand why we need to have a get_id callback.
> >> The use cases I have seen so far, which I have been intentionally rejecting, are
> >> mainly solvable by creating other compatible entries. And really, if you
> >> have, say a bandgap, chip that supports multiple sensors, but on
> >> SoC version A it has 5 sensors, and on SoC version B it has only 4,
> >> or on SoC version C, it has 5 but they are either logially located
> >> in different places (gpu vs iva regions), these are all cases in which
> >> you want a different compatible!
> >>
> >> Do you mind sharing why you need a get sensor id callback?
> >
> > It is not a get sensor id callback, it is a get_temp callback which pass
> > the sensor id.
> >
> > See in the different drivers, it is a common pattern there is a
> > structure for the driver, then a structure for the sensor. When the
> > get_temp is called, the callback needs info from the sensor structure
> > and from the driver structure, so a back pointer to the driver structure
> > is added in the sensor structure.
>
> Hi Eduardo,
>
> does the explanation clarifies the purpose of this change?
>

Eduardo, did you ever have a chance to revisit this thread? I would
really like to make some progress on this one to unblock my i.MX8MQ
hwmon series.

Thanks,
Andrey Smirnov

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Eduardo Valentin]

On Thu, May 23, 2019 at 07:48:56PM -0700, Andrey Smirnov wrote:
> On Mon, Apr 29, 2019 at 9:51 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 24/04/2019 01:08, Daniel Lezcano wrote:
> > > On 23/04/2019 17:44, Eduardo Valentin wrote:
> > >> Hello,
> > >>
> > >> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
> > >>> Currently when we register a sensor, we specify the sensor id and a data
> > >>> pointer to be passed when the get_temp function is called. However the
> > >>> sensor_id is not passed to the get_temp callback forcing the driver to
> > >>> do extra allocation and adding back pointer to find out from the sensor
> > >>> information the driver data and then back to the sensor id.
> > >>>
> > >>> Add a new callback get_temp_id() which will be called if set. It will
> > >>> call the get_temp_id() with the sensor id.
> > >>>
> > >>> That will be more consistent with the registering function.
> > >>
> > >> I still do not understand why we need to have a get_id callback.
> > >> The use cases I have seen so far, which I have been intentionally rejecting, are
> > >> mainly solvable by creating other compatible entries. And really, if you
> > >> have, say a bandgap, chip that supports multiple sensors, but on
> > >> SoC version A it has 5 sensors, and on SoC version B it has only 4,
> > >> or on SoC version C, it has 5 but they are either logially located
> > >> in different places (gpu vs iva regions), these are all cases in which
> > >> you want a different compatible!
> > >>
> > >> Do you mind sharing why you need a get sensor id callback?
> > >
> > > It is not a get sensor id callback, it is a get_temp callback which pass
> > > the sensor id.
> > >
> > > See in the different drivers, it is a common pattern there is a
> > > structure for the driver, then a structure for the sensor. When the
> > > get_temp is called, the callback needs info from the sensor structure
> > > and from the driver structure, so a back pointer to the driver structure
> > > is added in the sensor structure.
> >

Do you mind sending a patch showing how one could convert an existing
driver to use this new API?

> > Hi Eduardo,
> >
> > does the explanation clarifies the purpose of this change?
> >
> 
> Eduardo, did you ever have a chance to revisit this thread? I would
> really like to make some progress on this one to unblock my i.MX8MQ
> hwmon series.

The problem I have with this patch is that it is an API which resides
only in of-thermal. Growing APIs on DT only diverges of-thermal from
thermal core and platform drivers.

Besides, this patch needs to document the API in Documention/

> 
> Thanks,
> Andrey Smirnov

Re: [PATCH] thermal/drivers/of: Add a get_temp_id callback function

[Andrey Smirnov]

On Tue, May 28, 2019 at 8:05 PM Eduardo Valentin <edubezval@gmail.com> wrote:
>
> On Thu, May 23, 2019 at 07:48:56PM -0700, Andrey Smirnov wrote:
> > On Mon, Apr 29, 2019 at 9:51 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> > >
> > > On 24/04/2019 01:08, Daniel Lezcano wrote:
> > > > On 23/04/2019 17:44, Eduardo Valentin wrote:
> > > >> Hello,
> > > >>
> > > >> On Tue, Apr 16, 2019 at 07:22:03PM +0200, Daniel Lezcano wrote:
> > > >>> Currently when we register a sensor, we specify the sensor id and a data
> > > >>> pointer to be passed when the get_temp function is called. However the
> > > >>> sensor_id is not passed to the get_temp callback forcing the driver to
> > > >>> do extra allocation and adding back pointer to find out from the sensor
> > > >>> information the driver data and then back to the sensor id.
> > > >>>
> > > >>> Add a new callback get_temp_id() which will be called if set. It will
> > > >>> call the get_temp_id() with the sensor id.
> > > >>>
> > > >>> That will be more consistent with the registering function.
> > > >>
> > > >> I still do not understand why we need to have a get_id callback.
> > > >> The use cases I have seen so far, which I have been intentionally rejecting, are
> > > >> mainly solvable by creating other compatible entries. And really, if you
> > > >> have, say a bandgap, chip that supports multiple sensors, but on
> > > >> SoC version A it has 5 sensors, and on SoC version B it has only 4,
> > > >> or on SoC version C, it has 5 but they are either logially located
> > > >> in different places (gpu vs iva regions), these are all cases in which
> > > >> you want a different compatible!
> > > >>
> > > >> Do you mind sharing why you need a get sensor id callback?
> > > >
> > > > It is not a get sensor id callback, it is a get_temp callback which pass
> > > > the sensor id.
> > > >
> > > > See in the different drivers, it is a common pattern there is a
> > > > structure for the driver, then a structure for the sensor. When the
> > > > get_temp is called, the callback needs info from the sensor structure
> > > > and from the driver structure, so a back pointer to the driver structure
> > > > is added in the sensor structure.
> > >
>
> Do you mind sending a patch showing how one could convert an existing
> driver to use this new API?
>

There's already a patch doing that for qoriq driver:
https://lore.kernel.org/lkml/20190404080647.8173-2-daniel.lezcano@linaro.org/

Here's what can be done with Armada sensor driver:
https://gist.github.com/ndreys/d733228756a17e1dcb31ec8f9a0e9115
(please note that I don't have Armada HW, so I haven't tested those
changes, just made them as an example)

> > > Hi Eduardo,
> > >
> > > does the explanation clarifies the purpose of this change?
> > >
> >
> > Eduardo, did you ever have a chance to revisit this thread? I would
> > really like to make some progress on this one to unblock my i.MX8MQ
> > hwmon series.
>
> The problem I have with this patch is that it is an API which resides
> only in of-thermal. Growing APIs on DT only diverges of-thermal from
> thermal core and platform drivers.
>

I don't have a horse in this race and would be fine if this API isn't
accepted, so this is up for you to decide. My main goal is to get my
i.MX8MQ hwmon patches moving and accepted.

> Besides, this patch needs to document the API in Documention/

I'd like to get more of a commitment on this before I'd start
investing time into improving this patch any further. If this patch is
a go, then I'd be happy to add that documentation.

Thanks,
Andrey Smirnov