linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andy Tang <andy.tang@nxp.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>,
	"rui.zhang@intel.com" <rui.zhang@intel.com>
Cc: "edubezval@gmail.com" <edubezval@gmail.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] thermal: qoriq: add multiple sensors support
Date: Mon, 15 Oct 2018 01:41:15 +0000	[thread overview]
Message-ID: <DB5PR0401MB2213D4C1874291B87F231547F3FD0@DB5PR0401MB2213.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <3d1f8304-9005-23f3-2e0e-ef9c962c9f6e@linaro.org>

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


  reply	other threads:[~2018-10-15  1:41 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-27  2:42 andy.tang
2018-10-11  8:07 ` Andy Tang
2018-10-11  8:56   ` Daniel Lezcano
2018-10-13 20:42 ` Daniel Lezcano
2018-10-15  1:41   ` Andy Tang [this message]
2018-10-15  8:55     ` Daniel Lezcano
2018-10-16  3:01       ` Andy Tang
2018-10-16 11:20         ` Daniel Lezcano
2018-10-24  2:48           ` Andy Tang
2018-10-24  8:29             ` Daniel Lezcano
2018-10-24 12:52           ` Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB5PR0401MB2213D4C1874291B87F231547F3FD0@DB5PR0401MB2213.eurprd04.prod.outlook.com \
    --to=andy.tang@nxp.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=edubezval@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rui.zhang@intel.com \
    --subject='RE: [PATCH] thermal: qoriq: add multiple sensors support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).