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@xxxxxxxxxx>
>>> Tested-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
>>> ---
>>>  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