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

From: Andy Tang
Date: Sun Oct 14 2018 - 21:41:31 EST

Thanks Daniel,

Please see my reply inline.

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Sent: 2018å10æ14æ 4:43
> To: Andy Tang <andy.tang@xxxxxxx>; rui.zhang@xxxxxxxxx
> Cc: edubezval@xxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] thermal: qoriq: add multiple sensors support
>
>
> Hi Yuantian,
>
>
> On 27/09/2018 04:42, andy.tang@xxxxxxx wrote:
> > From: Yuantian Tang <andy.tang@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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