RE: [PATCH] thermal: qoriq: add multiple sensors support

[Andy Tang <andy.tang@nxp.com>]

Thanks Daniel,

Please see my reply inline.

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@linaro.org>
> Sent: 2018年10月14日 4:43
> To: Andy Tang <andy.tang@nxp.com>; rui.zhang@intel.com
> Cc: edubezval@gmail.com; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] thermal: qoriq: add multiple sensors support
> 
> 
> Hi Yuantian,
> 
> 
> On 27/09/2018 04:42, andy.tang@nxp.com wrote:
> > From: Yuantian Tang <andy.tang@nxp.com>
> >
> > There is only one sensor supported in current driver.
> > Multiple sensors are existing on Layscape socs. To support them,
> > covert this driver to support multiple sensors.
> 
> s/covert/convert/
> 
> What about the following changelog ?
> 
> "
> The QorIQ Layerscape SoC has several thermal sensors but the current
> driver only supports one.
> 
> Massage the code to be sensor oriented and allow the support for
> multiple sensors.
> "
[Andy]  Thanks, will update

> 
> > Signed-off-by: Tang Yuantian <andy.tang@nxp.com>
> > ---
> >  drivers/thermal/qoriq_thermal.c |  117
> > +++++++++++++++++++++++----------------
> >  1 files changed, 70 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c
> > b/drivers/thermal/qoriq_thermal.c index c866cc1..7c1e88a 100644
> > --- a/drivers/thermal/qoriq_thermal.c
> > +++ b/drivers/thermal/qoriq_thermal.c
> > @@ -69,14 +69,21 @@ struct qoriq_tmu_regs {
> >  	u32 ttr3cr;		/* Temperature Range 3 Control Register */
> >  };
> >
> > +struct qoriq_tmu_data;
> > +
> >  /*
> >   * Thermal zone data
> >   */
> > +struct qoriq_sensor {
> > +	struct thermal_zone_device	*tzd;
> > +	struct qoriq_tmu_data		*qdata;
> > +	int				id;
> > +};
> 
> Can you move the qoriq_tmu_site_regs structure content inside the
> qoriq_sensor structure and kill the 'sites' field in the qoriq_tmu_regs
> structure ? Otherwise we end up with a SITES_MAX array in the
> qoriq_tmu_data structure and another one in the qoriq_tmu_regs
> structure.
[Andy] I am afraid I can't.
qoriq_tmu_site_regs structure is to define the registers. After iomap, TMU can be accessed.
qoriq_sensor structure is used for each sensor. It DONOT include the register defines.
qoriq_tmu_data structure is used for global TMU date.
So there is no any duplicated or redundant data here.

> > -	if (sensor_specs.args_count >= 1) {
> > -		id = sensor_specs.args[0];
> > -		WARN(sensor_specs.args_count > 1,
> > -				"%s: too many cells in sensor specifier %d\n",
> > -				sensor_specs.np->name, sensor_specs.args_count);
> > -	} else {
> > -		id = 0;
> > +		if (id > SITES_MAX)
> > +			return -EINVAL;
> > +
> > +		qdata->sensor[id] = devm_kzalloc(&pdev->dev,
> > +				sizeof(struct qoriq_sensor), GFP_KERNEL);
> > +		if (!qdata->sensor[id])
> > +			return -ENOMEM;
> > +
> > +		qdata->sensor[id]->id = id;
> > +		qdata->sensor[id]->qdata = qdata;
> > +
> > +		qdata->sensor[id]->tzd =
> devm_thermal_zone_of_sensor_register(
> > +				&pdev->dev, id, qdata->sensor[id], &tmu_tz_ops);
> > +
> > +		if (IS_ERR(qdata->sensor[id]->tzd)) {
> > +			ret = PTR_ERR(qdata->sensor[id]->tzd);
> > +			dev_err(&pdev->dev,
> > +				"Failed to register thermal zone device.\n");
> > +			return -ENODEV;
> > +		}
> > +
> > +		sites |= 0x1 << (15 - id);
> 
> The current code is reading the DT in order to get the sensor id and
> initialize it. IOW, the DT gives the sensors to use.
> 
> IMO, it would be more self contained if the driver initializes all the sensors
> without taking care of the DT and let the of- code to do the binding when
> the thermal zone, no ?
[Andy] could you please explain more about this way? I am not sure how to implement it.
But one thing is for sure: we must get the sensor IDs explicitly so that we can enable them by
the following command:  tmu_write(qdata, sites | TMR_ME | TMR_ALPF, &qdata->regs->tmr);

BR,
Andy  

> 
> >  	}
> >
> > -	of_node_put(np);
> > -	of_node_put(sensor_np);
> > +	/* Enable monitoring */
> > +	if (sites != 0)
> > +		tmu_write(qdata, sites | TMR_ME | TMR_ALPF,
> &qdata->regs->tmr);
> >
> > -	return id;
> > +	return 0;
> >  }
> >
> >  static int qoriq_tmu_calibration(struct platform_device *pdev) @@
> > -188,16 +230,11 @@ static void qoriq_tmu_init_device(struct
> qoriq_tmu_data *data)
> >  	tmu_write(data, TMR_DISABLE, &data->regs->tmr);  }
> >
> > -static const struct thermal_zone_of_device_ops tmu_tz_ops = {
> > -	.get_temp = tmu_get_temp,
> > -};
> > -
> >  static int qoriq_tmu_probe(struct platform_device *pdev)  {
> >  	int ret;
> >  	struct qoriq_tmu_data *data;
> >  	struct device_node *np = pdev->dev.of_node;
> > -	u32 site = 0;
> >
> >  	if (!np) {
> >  		dev_err(&pdev->dev, "Device OF-Node is NULL"); @@ -213,13
> +250,6 @@
> > static int qoriq_tmu_probe(struct platform_device *pdev)
> >
> >  	data->little_endian = of_property_read_bool(np, "little-endian");
> >
> > -	data->sensor_id = qoriq_tmu_get_sensor_id();
> > -	if (data->sensor_id < 0) {
> > -		dev_err(&pdev->dev, "Failed to get sensor id\n");
> > -		ret = -ENODEV;
> > -		goto err_iomap;
> > -	}
> > -
> >  	data->regs = of_iomap(np, 0);
> >  	if (!data->regs) {
> >  		dev_err(&pdev->dev, "Failed to get memory region\n"); @@
> -233,18
> > +263,13 @@ static int qoriq_tmu_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto err_tmu;
> >
> > -	data->tz = thermal_zone_of_sensor_register(&pdev->dev,
> data->sensor_id,
> > -				data, &tmu_tz_ops);
> > -	if (IS_ERR(data->tz)) {
> > -		ret = PTR_ERR(data->tz);
> > -		dev_err(&pdev->dev,
> > -			"Failed to register thermal zone device %d\n", ret);
> > -		goto err_tmu;
> > +	ret = qoriq_tmu_register_tmu_zone(pdev);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to register sensors\n");
> > +		ret = -ENODEV;
> > +		goto err_iomap;
> >  	}
> >
> > -	/* Enable monitoring */
> > -	site |= 0x1 << (15 - data->sensor_id);
> > -	tmu_write(data, site | TMR_ME | TMR_ALPF, &data->regs->tmr);
> >
> >  	return 0;
> >
> > @@ -261,8 +286,6 @@ static int qoriq_tmu_remove(struct
> platform_device
> > *pdev)  {
> >  	struct qoriq_tmu_data *data = platform_get_drvdata(pdev);
> >
> > -	thermal_zone_of_sensor_unregister(&pdev->dev, data->tz);
> > -
> >  	/* Disable monitoring */
> >  	tmu_write(data, TMR_DISABLE, &data->regs->tmr);
> >
> >
> 
> 
> --
> 
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.linaro.org%2F&amp;data=02%7C01%7Candy.tang%40nxp.com%7C80
> b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636750601624930581&amp;sdata=wbhRsdAYdai
> 5RqgW1AIPAn2Wls9s782E1%2B%2BJScuX3VM%3D&amp;reserved=0>
> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Candy.tan
> g%40nxp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636750601624930581&amp
> ;sdata=eqY8T%2FCWExUWYjRx%2Fum8tTYcm8nUiSMUtIqfMW4KcFM%3D
> &amp;reserved=0> Facebook |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ft
> witter.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Candy.tang%40n
> xp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C0%7C636750601624930581&amp;sdata=
> Vij4EBAgMPtV4KBDr5hT1fMazxiSs9naSgH4oAGUFBc%3D&amp;reserved=
> 0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> www.linaro.org%2Flinaro-blog%2F&amp;data=02%7C01%7Candy.tang%40
> nxp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C636750601624930581&amp;sdat
> a=Uouq%2BRYyMq5E6MgfBwbiQ3YrUYCvMb4PVYHa0Fv6u08%3D&amp;re
> served=0> Blog