| Message ID | 20180927024204.17314-1-andy.tang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
| Series | thermal: qoriq: add multiple sensors support | expand |
Hi Rui, Appreciate if you can give it a review. BR, Andy > -----Original Message----- > From: andy.tang@nxp.com <andy.tang@nxp.com> > Sent: 2018年9月27日 10:42 > To: rui.zhang@intel.com > Cc: edubezval@gmail.com; daniel.lezcano@linaro.org; > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Andy Tang > <andy.tang@nxp.com> > Subject: [PATCH] thermal: qoriq: add multiple sensors support > > 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. > > 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; > +}; > + > struct qoriq_tmu_data { > - struct thermal_zone_device *tz; > struct qoriq_tmu_regs __iomem *regs; > - int sensor_id; > bool little_endian; > + struct qoriq_sensor *sensor[SITES_MAX]; > }; > > static void tmu_write(struct qoriq_tmu_data *p, u32 val, void __iomem > *addr) @@ -97,48 +104,83 @@ static u32 tmu_read(struct > qoriq_tmu_data *p, void __iomem *addr) > > static int tmu_get_temp(void *p, int *temp) { > + struct qoriq_sensor *qsensor = p; > + struct qoriq_tmu_data *qdata = qsensor->qdata; > u32 val; > - struct qoriq_tmu_data *data = p; > > - val = tmu_read(data, &data->regs->site[data->sensor_id].tritsr); > + val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr); > *temp = (val & 0xff) * 1000; > > return 0; > } > > -static int qoriq_tmu_get_sensor_id(void) > +static const struct thermal_zone_of_device_ops tmu_tz_ops = { > + .get_temp = tmu_get_temp, > +}; > + > +static int qoriq_tmu_register_tmu_zone(struct platform_device *pdev) > { > - int ret, id; > + struct qoriq_tmu_data *qdata = platform_get_drvdata(pdev); > struct of_phandle_args sensor_specs; > struct device_node *np, *sensor_np; > + int ret, id, sites = 0; > > np = of_find_node_by_name(NULL, "thermal-zones"); > if (!np) > return -ENODEV; > > - sensor_np = of_get_next_child(np, NULL); > - ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", > - "#thermal-sensor-cells", > - 0, &sensor_specs); > - if (ret) { > + for_each_available_child_of_node(np, sensor_np) { > + ret = of_parse_phandle_with_args(sensor_np, > "thermal-sensors", > + "#thermal-sensor-cells", > + 0, &sensor_specs); > + if (ret) { > + of_node_put(np); > + of_node_put(sensor_np); > + return ret; > + } > + > + 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; > + } > + > of_node_put(np); > of_node_put(sensor_np); > - return ret; > - } > > - 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); > } > > - 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); > > -- > 1.7.1
On 11/10/2018 10:07, Andy Tang wrote: > Hi Rui, > > Appreciate if you can give it a review. Thanks for the heads up. I'll take care of that. -- Daniel >> -----Original Message----- >> From: andy.tang@nxp.com <andy.tang@nxp.com> >> Sent: 2018年9月27日 10:42 >> To: rui.zhang@intel.com >> Cc: edubezval@gmail.com; daniel.lezcano@linaro.org; >> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Andy Tang >> <andy.tang@nxp.com> >> Subject: [PATCH] thermal: qoriq: add multiple sensors support >> >> 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. >> >> 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; >> +}; >> + >> struct qoriq_tmu_data { >> - struct thermal_zone_device *tz; >> struct qoriq_tmu_regs __iomem *regs; >> - int sensor_id; >> bool little_endian; >> + struct qoriq_sensor *sensor[SITES_MAX]; >> }; >> >> static void tmu_write(struct qoriq_tmu_data *p, u32 val, void __iomem >> *addr) @@ -97,48 +104,83 @@ static u32 tmu_read(struct >> qoriq_tmu_data *p, void __iomem *addr) >> >> static int tmu_get_temp(void *p, int *temp) { >> + struct qoriq_sensor *qsensor = p; >> + struct qoriq_tmu_data *qdata = qsensor->qdata; >> u32 val; >> - struct qoriq_tmu_data *data = p; >> >> - val = tmu_read(data, &data->regs->site[data->sensor_id].tritsr); >> + val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr); >> *temp = (val & 0xff) * 1000; >> >> return 0; >> } >> >> -static int qoriq_tmu_get_sensor_id(void) >> +static const struct thermal_zone_of_device_ops tmu_tz_ops = { >> + .get_temp = tmu_get_temp, >> +}; >> + >> +static int qoriq_tmu_register_tmu_zone(struct platform_device *pdev) >> { >> - int ret, id; >> + struct qoriq_tmu_data *qdata = platform_get_drvdata(pdev); >> struct of_phandle_args sensor_specs; >> struct device_node *np, *sensor_np; >> + int ret, id, sites = 0; >> >> np = of_find_node_by_name(NULL, "thermal-zones"); >> if (!np) >> return -ENODEV; >> >> - sensor_np = of_get_next_child(np, NULL); >> - ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", >> - "#thermal-sensor-cells", >> - 0, &sensor_specs); >> - if (ret) { >> + for_each_available_child_of_node(np, sensor_np) { >> + ret = of_parse_phandle_with_args(sensor_np, >> "thermal-sensors", >> + "#thermal-sensor-cells", >> + 0, &sensor_specs); >> + if (ret) { >> + of_node_put(np); >> + of_node_put(sensor_np); >> + return ret; >> + } >> + >> + 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; >> + } >> + >> of_node_put(np); >> of_node_put(sensor_np); >> - return ret; >> - } >> >> - 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); >> } >> >> - 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); >> >> -- >> 1.7.1 >
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. " > 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. > struct qoriq_tmu_data { > - struct thermal_zone_device *tz; > struct qoriq_tmu_regs __iomem *regs; > - int sensor_id; > bool little_endian; > + struct qoriq_sensor *sensor[SITES_MAX]; > }; > > static void tmu_write(struct qoriq_tmu_data *p, u32 val, void __iomem *addr) > @@ -97,48 +104,83 @@ static u32 tmu_read(struct qoriq_tmu_data *p, void __iomem *addr) > > static int tmu_get_temp(void *p, int *temp) > { > + struct qoriq_sensor *qsensor = p; > + struct qoriq_tmu_data *qdata = qsensor->qdata; > u32 val; > - struct qoriq_tmu_data *data = p; > > - val = tmu_read(data, &data->regs->site[data->sensor_id].tritsr); > + val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr); > *temp = (val & 0xff) * 1000; > > return 0; > } > > -static int qoriq_tmu_get_sensor_id(void) > +static const struct thermal_zone_of_device_ops tmu_tz_ops = { > + .get_temp = tmu_get_temp, > +}; > + > +static int qoriq_tmu_register_tmu_zone(struct platform_device *pdev) > { > - int ret, id; > + struct qoriq_tmu_data *qdata = platform_get_drvdata(pdev); > struct of_phandle_args sensor_specs; > struct device_node *np, *sensor_np; > + int ret, id, sites = 0; > > np = of_find_node_by_name(NULL, "thermal-zones"); > if (!np) > return -ENODEV; > > - sensor_np = of_get_next_child(np, NULL); > - ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", > - "#thermal-sensor-cells", > - 0, &sensor_specs); > - if (ret) { > + for_each_available_child_of_node(np, sensor_np) { > + ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", > + "#thermal-sensor-cells", > + 0, &sensor_specs); > + if (ret) { > + of_node_put(np); > + of_node_put(sensor_np); > + return ret; > + } > + > + 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; > + } > + > of_node_put(np); > of_node_put(sensor_np); > - return ret; > - } > > - 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 ? > } > > - 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); > >
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&data=02%7C01%7Candy.tang%40nxp.com%7C80 > b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b4c6fa92cd99c5 > c301635%7C0%7C0%7C636750601624930581&sdata=wbhRsdAYdai > 5RqgW1AIPAn2Wls9s782E1%2B%2BJScuX3VM%3D&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&data=02%7C01%7Candy.tan > g%40nxp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3 > bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636750601624930581& > ;sdata=eqY8T%2FCWExUWYjRx%2Fum8tTYcm8nUiSMUtIqfMW4KcFM%3D > &reserved=0> Facebook | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ft > witter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40n > xp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b4c > 6fa92cd99c5c301635%7C0%7C0%7C636750601624930581&sdata= > Vij4EBAgMPtV4KBDr5hT1fMazxiSs9naSgH4oAGUFBc%3D&reserved= > 0> Twitter | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F > www.linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40 > nxp.com%7C80b2371c721b4d0d334908d6314c6b4d%7C686ea1d3bc2b > 4c6fa92cd99c5c301635%7C0%7C0%7C636750601624930581&sdat > a=Uouq%2BRYyMq5E6MgfBwbiQ3YrUYCvMb4PVYHa0Fv6u08%3D&re > served=0> Blog
On 15/10/2018 03:41, Andy Tang wrote: > 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. It is not about duplicate but just code reorg. This patch changes the structure as: struct qoriq_tmu_data { - struct thermal_zone_device *tz; struct qoriq_tmu_regs __iomem *regs; - int sensor_id; bool little_endian; + struct qoriq_sensor *sensor[SITES_MAX]; }; So we have: struct qoriq_tmu_data => struct qoriq_sensor[SITES_MAX] => struct qoriq_tmu_regs => struct qoriq_tmu_site_regs[SITES_MAX] I'm proposing to move struct qoriq_tmu_site_regs inside the struct qoriq_sensor. We end up with: struct qoriq_sensor { struct thermal_zone_device *tzd; struct struct qoriq_tmu_site_regs *regs; struct qoriq_tmu_data *qdata; int id; }; >>> - 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); What I meant is about code separation between the driver itself and the of-thermal code. The code above re-inspect the DT to find out the sensor ids in order to enable them and somehow this is not wrong but breaks the self encapsulation of the driver. I was suggesting if it isn't possible to enable all the sensors without taking care of digging into the DT.
Hi Daniel, Please see my reply inline. > -----Original Message----- > From: Daniel Lezcano <daniel.lezcano@linaro.org> > Sent: 2018年10月15日 16:56 > 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 > > > > >> > >>> 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. > > It is not about duplicate but just code reorg. > > This patch changes the structure as: > > struct qoriq_tmu_data { > - struct thermal_zone_device *tz; > struct qoriq_tmu_regs __iomem *regs; > - int sensor_id; > bool little_endian; > + struct qoriq_sensor *sensor[SITES_MAX]; > }; > > > So we have: > > struct qoriq_tmu_data > => struct qoriq_sensor[SITES_MAX] > => struct qoriq_tmu_regs > => struct qoriq_tmu_site_regs[SITES_MAX] > > I'm proposing to move struct qoriq_tmu_site_regs inside the struct > qoriq_sensor. > > > We end up with: > > struct qoriq_sensor { > struct thermal_zone_device *tzd; > struct struct qoriq_tmu_site_regs *regs; > struct qoriq_tmu_data *qdata; > int id; > }; [Andy] I see your point. If I add a *regs member to qoriq_sensor struct, then I can speed up the access to the register. But I don't think it is better. Currently I can access sensor register by: qdata->regs->site[qsensor->id]. The whole point here is we can't MOVE struct qoriq_tmu_site_regs to the struct qoriq_sensor. We can only COPY it to struct qoriq_sensor. This is the TMU module memory map: struct qoriq_tmu_regs { ...... u32 tscfgr; /* Sensor Configuration Register */ u8 res4[0x78]; struct qoriq_tmu_site_regs site[SITES_MAX]; u8 res5[0x9f8]; u32 ipbrr0; /* IP Block Revision Register 0 */ u32 ipbrr1; /* IP Block Revision Register 1 */ .......} struct qoriq_tmu_site_regs site[SITES_MAX] is part of the memory map. It can't be removed or we have to define a similar struct to fill it up. > > > >>> - 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); > > What I meant is about code separation between the driver itself and the > of-thermal code. > > The code above re-inspect the DT to find out the sensor ids in order to > enable them and somehow this is not wrong but breaks the self > encapsulation of the driver. I was suggesting if it isn't possible to enable all > the sensors without taking care of digging into the DT. [Andy] I don't want to re-parse the DT here too. But I have to. This driver will be used by all our SOCs with different sensor IDs and number. For example: there are 2 sensors on ls1088a platform with ID 0 and 1. While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5. If we don't scan the DT we would not know how many sensors it is and what are the sensor's IDs, unless we hardcode it in driver. BR, Andy > > > > > -- > > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F > www.linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7C38 > 9338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4c6fa92cd99c5c > 301635%7C0%7C0%7C636751905429537336&sdata=IKzO0HNquNm > BPrvxikDiIBFchvdtg8HpqJiuYkqo1%2F4%3D&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&data=02%7C01%7Candy.tan > g%40nxp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C636751905429537336&s > data=vdR7lC1Q%2FbHUgJXlwlLaoZUdfawrACOQUM94yhPfWVA%3D& > reserved=0> Facebook | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ft > witter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40n > xp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4c > 6fa92cd99c5c301635%7C0%7C0%7C636751905429547345&sdata= > k2QUt15gv8NI4fyqPYFoFVN%2FiM7XKOzh45eax%2F2e2Hk%3D&rese > rved=0> Twitter | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F > www.linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40 > nxp.com%7C389338eb5bc44d6c268308d6327bfc1c%7C686ea1d3bc2b4 > c6fa92cd99c5c301635%7C0%7C0%7C636751905429547345&sdata > =cbTIEuaYP4l%2FLZR45M253QRfqgXbZPw87%2Bo5EfwTzys%3D&res > erved=0> Blog
On 16/10/2018 05:01, Andy Tang wrote: > Hi Daniel, > > Please see my reply inline. > >> -----Original Message----- [ ... ] >> I'm proposing to move struct qoriq_tmu_site_regs inside the struct >> qoriq_sensor. >> >> >> We end up with: >> >> struct qoriq_sensor { >> struct thermal_zone_device *tzd; >> struct struct qoriq_tmu_site_regs *regs; >> struct qoriq_tmu_data *qdata; >> int id; >> }; > [Andy] I see your point. If I add a *regs member to qoriq_sensor struct, then I can speed up the access to the register. > But I don't think it is better. Currently I can access sensor register by: qdata->regs->site[qsensor->id]. > The whole point here is we can't MOVE struct qoriq_tmu_site_regs to the struct qoriq_sensor. We can only COPY > it to struct qoriq_sensor. > > This is the TMU module memory map: > struct qoriq_tmu_regs { > ...... > u32 tscfgr; /* Sensor Configuration Register */ > u8 res4[0x78]; > struct qoriq_tmu_site_regs site[SITES_MAX]; > u8 res5[0x9f8]; > u32 ipbrr0; /* IP Block Revision Register 0 */ > u32 ipbrr1; /* IP Block Revision Register 1 */ > .......} > struct qoriq_tmu_site_regs site[SITES_MAX] is part of the memory > map. > It can't be removed or we have to define a similar struct to fill it up. Mmh, I see. Thanks for the clarification. A consolidation around this would have been nice but it means a DT change (reduce the size of the map). So I guess we can leave it as it is. >>>>> - 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); >> >> What I meant is about code separation between the driver itself and the >> of-thermal code. >> >> The code above re-inspect the DT to find out the sensor ids in order to >> enable them and somehow this is not wrong but breaks the self >> encapsulation of the driver. I was suggesting if it isn't possible to enable all >> the sensors without taking care of digging into the DT. > > [Andy] I don't want to re-parse the DT here too. But I have to. > This driver will be used by all our SOCs with different sensor IDs and number. > For example: there are 2 sensors on ls1088a platform with ID 0 and 1. > While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5. > If we don't scan the DT we would not know how many sensors it is and what are the sensor's IDs, > unless we hardcode it in driver. Yes, you are not the only one in this situation IMO and the drivers supporting multiple sensors are increasing, so this will repeat again and again. That could be hardcoded in the driver by using the compatible string but it will be nicer if we can fix that in the DT. [Cc'ing Rob] What is missing is a description of the sensors id in the temperature device node. We have the <thermal-sensor-cells> with 0 or 1 telling if there is one or several sensors but we can't specify which sensor ids we have. The only alternative is to parse the thermal zones to found out which sensors are in use and use them to initialize the driver, an approach which breaks the self-encapsulation: the of-thermal framework is the one in charge of doing the link between the thermal zone and a sensor id. Is it acceptable to add the list of the sensors id in the temp device node, so the driver can initialize these sensors without parsing the thermal zone in the DT ?
Hi Daniel, > -----Original Message----- > From: Daniel Lezcano <daniel.lezcano@linaro.org> > Sent: 2018年10月16日 19:21 > 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; Rob Herring <robh+dt@kernel.org> > Subject: Re: [PATCH] thermal: qoriq: add multiple sensors support > > >>>> 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); > >> > >> What I meant is about code separation between the driver itself and > >> the of-thermal code. > >> > >> The code above re-inspect the DT to find out the sensor ids in order > >> to enable them and somehow this is not wrong but breaks the self > >> encapsulation of the driver. I was suggesting if it isn't possible to > >> enable all the sensors without taking care of digging into the DT. > > > > [Andy] I don't want to re-parse the DT here too. But I have to. > > This driver will be used by all our SOCs with different sensor IDs and > number. > > For example: there are 2 sensors on ls1088a platform with ID 0 and 1. > > While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5. > > If we don't scan the DT we would not know how many sensors it is and > > what are the sensor's IDs, unless we hardcode it in driver. > > Yes, you are not the only one in this situation IMO and the drivers > supporting multiple sensors are increasing, so this will repeat again and > again. > > That could be hardcoded in the driver by using the compatible string but it > will be nicer if we can fix that in the DT. > > [Cc'ing Rob] > > What is missing is a description of the sensors id in the temperature > device node. We have the <thermal-sensor-cells> with 0 or 1 telling if > there is one or several sensors but we can't specify which sensor ids we > have. The only alternative is to parse the thermal zones to found out which > sensors are in use and use them to initialize the driver, an approach which > breaks the self-encapsulation: the of-thermal framework is the one in > charge of doing the link between the thermal zone and a sensor id. > > Is it acceptable to add the list of the sensors id in the temp device node, so > the driver can initialize these sensors without parsing the thermal zone in > the DT ? > Have you got any conclusion yet? When can I send the next version of this patch? BR, Andy > > > > -- > > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F > www.linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7Cf6 > 343bd28e0643833fa108d6335973ca%7C686ea1d3bc2b4c6fa92cd99c5c > 301635%7C0%7C0%7C636752856644006568&sdata=OKIXYjHb4Pvc > 8lmevq%2FwJe9sosvNRFxhvPAb8TT3FWQ%3D&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&data=02%7C01%7Candy.tan > g%40nxp.com%7Cf6343bd28e0643833fa108d6335973ca%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C636752856644006568&s > data=LQ3T5kpXFrqRrQR1Jt9OjY547TWsfxaIU1z8tK%2F6x8Y%3D&res > erved=0> Facebook | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ft > witter.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40n > xp.com%7Cf6343bd28e0643833fa108d6335973ca%7C686ea1d3bc2b4c6 > fa92cd99c5c301635%7C0%7C0%7C636752856644006568&sdata=h > MOS1WxQBcS%2Bvxyjwz68nkZFaB%2FeZ21E33U50GdF9pY%3D&res > erved=0> Twitter | > <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2F > www.linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40 > nxp.com%7Cf6343bd28e0643833fa108d6335973ca%7C686ea1d3bc2b4c > 6fa92cd99c5c301635%7C0%7C0%7C636752856644162836&sdata= > 2j0bJLJgzXsmlrG8hR6aKSYvmg3RriaH%2Bzt1x4OF8kU%3D&reserved= > 0> Blog
On 24/10/2018 04:48, Andy Tang wrote: > Hi Daniel, > >> -----Original Message----- >> From: Daniel Lezcano <daniel.lezcano@linaro.org> >> Sent: 2018年10月16日 19:21 >> 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; Rob Herring <robh+dt@kernel.org> >> Subject: Re: [PATCH] thermal: qoriq: add multiple sensors support >> >>>>>> 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); >>>> >>>> What I meant is about code separation between the driver itself and >>>> the of-thermal code. >>>> >>>> The code above re-inspect the DT to find out the sensor ids in order >>>> to enable them and somehow this is not wrong but breaks the self >>>> encapsulation of the driver. I was suggesting if it isn't possible to >>>> enable all the sensors without taking care of digging into the DT. >>> >>> [Andy] I don't want to re-parse the DT here too. But I have to. >>> This driver will be used by all our SOCs with different sensor IDs and >> number. >>> For example: there are 2 sensors on ls1088a platform with ID 0 and 1. >>> While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5. >>> If we don't scan the DT we would not know how many sensors it is and >>> what are the sensor's IDs, unless we hardcode it in driver. >> >> Yes, you are not the only one in this situation IMO and the drivers >> supporting multiple sensors are increasing, so this will repeat again and >> again. >> >> That could be hardcoded in the driver by using the compatible string but it >> will be nicer if we can fix that in the DT. >> >> [Cc'ing Rob] >> >> What is missing is a description of the sensors id in the temperature >> device node. We have the <thermal-sensor-cells> with 0 or 1 telling if >> there is one or several sensors but we can't specify which sensor ids we >> have. The only alternative is to parse the thermal zones to found out which >> sensors are in use and use them to initialize the driver, an approach which >> breaks the self-encapsulation: the of-thermal framework is the one in >> charge of doing the link between the thermal zone and a sensor id. >> >> Is it acceptable to add the list of the sensors id in the temp device node, so >> the driver can initialize these sensors without parsing the thermal zone in >> the DT ? >> > Have you got any conclusion yet? > When can I send the next version of this patch? Let's give an opportunity to Rob to answer.
On Tue, Oct 16, 2018 at 6:21 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 16/10/2018 05:01, Andy Tang wrote: > > Hi Daniel, > > > > Please see my reply inline. > > > >> -----Original Message----- > > [ ... ] > > >> I'm proposing to move struct qoriq_tmu_site_regs inside the struct > >> qoriq_sensor. > >> > >> > >> We end up with: > >> > >> struct qoriq_sensor { > >> struct thermal_zone_device *tzd; > >> struct struct qoriq_tmu_site_regs *regs; > >> struct qoriq_tmu_data *qdata; > >> int id; > >> }; > > [Andy] I see your point. If I add a *regs member to qoriq_sensor struct, then I can speed up the access to the register. > > But I don't think it is better. Currently I can access sensor register by: qdata->regs->site[qsensor->id]. > > The whole point here is we can't MOVE struct qoriq_tmu_site_regs to the struct qoriq_sensor. We can only COPY > > it to struct qoriq_sensor. > > > > This is the TMU module memory map: > > struct qoriq_tmu_regs { > > ...... > > u32 tscfgr; /* Sensor Configuration Register */ > > u8 res4[0x78]; > > struct qoriq_tmu_site_regs site[SITES_MAX]; > > u8 res5[0x9f8]; > > u32 ipbrr0; /* IP Block Revision Register 0 */ > > u32 ipbrr1; /* IP Block Revision Register 1 */ > > .......} > > struct qoriq_tmu_site_regs site[SITES_MAX] is part of the memory > > map. > > It can't be removed or we have to define a similar struct to fill it up. > > Mmh, I see. Thanks for the clarification. > > A consolidation around this would have been nice but it means a DT > change (reduce the size of the map). So I guess we can leave it as it is. > > > >>>>> - 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); > >> > >> What I meant is about code separation between the driver itself and the > >> of-thermal code. > >> > >> The code above re-inspect the DT to find out the sensor ids in order to > >> enable them and somehow this is not wrong but breaks the self > >> encapsulation of the driver. I was suggesting if it isn't possible to enable all > >> the sensors without taking care of digging into the DT. > > > > [Andy] I don't want to re-parse the DT here too. But I have to. > > This driver will be used by all our SOCs with different sensor IDs and number. > > For example: there are 2 sensors on ls1088a platform with ID 0 and 1. > > While on ls1043a there are 6 sensors with ID 0, 1, 2, 3, 4, 5. > > If we don't scan the DT we would not know how many sensors it is and what are the sensor's IDs, > > unless we hardcode it in driver. > > Yes, you are not the only one in this situation IMO and the drivers > supporting multiple sensors are increasing, so this will repeat again > and again. > > That could be hardcoded in the driver by using the compatible string but > it will be nicer if we can fix that in the DT. > > [Cc'ing Rob] > > What is missing is a description of the sensors id in the temperature > device node. We have the <thermal-sensor-cells> with 0 or 1 telling if > there is one or several sensors but we can't specify which sensor ids we > have. The only alternative is to parse the thermal zones to found out > which sensors are in use and use them to initialize the driver, an > approach which breaks the self-encapsulation: the of-thermal framework > is the one in charge of doing the link between the thermal zone and a > sensor id. > > Is it acceptable to add the list of the sensors id in the temp device > node, so the driver can initialize these sensors without parsing the > thermal zone in the DT ? In other cases like this, we've usually avoided having a property as the data is already there in DT. Granted, it's not in the most convenient form. We already have an iterator to iterate on all occurrences of a property in the tree (or subtree). Sure, it's not really that efficient, but does it matter? The question that comes up is do you have sensors not defined in DT? Rob
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; +}; + struct qoriq_tmu_data { - struct thermal_zone_device *tz; struct qoriq_tmu_regs __iomem *regs; - int sensor_id; bool little_endian; + struct qoriq_sensor *sensor[SITES_MAX]; }; static void tmu_write(struct qoriq_tmu_data *p, u32 val, void __iomem *addr) @@ -97,48 +104,83 @@ static u32 tmu_read(struct qoriq_tmu_data *p, void __iomem *addr) static int tmu_get_temp(void *p, int *temp) { + struct qoriq_sensor *qsensor = p; + struct qoriq_tmu_data *qdata = qsensor->qdata; u32 val; - struct qoriq_tmu_data *data = p; - val = tmu_read(data, &data->regs->site[data->sensor_id].tritsr); + val = tmu_read(qdata, &qdata->regs->site[qsensor->id].tritsr); *temp = (val & 0xff) * 1000; return 0; } -static int qoriq_tmu_get_sensor_id(void) +static const struct thermal_zone_of_device_ops tmu_tz_ops = { + .get_temp = tmu_get_temp, +}; + +static int qoriq_tmu_register_tmu_zone(struct platform_device *pdev) { - int ret, id; + struct qoriq_tmu_data *qdata = platform_get_drvdata(pdev); struct of_phandle_args sensor_specs; struct device_node *np, *sensor_np; + int ret, id, sites = 0; np = of_find_node_by_name(NULL, "thermal-zones"); if (!np) return -ENODEV; - sensor_np = of_get_next_child(np, NULL); - ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", - "#thermal-sensor-cells", - 0, &sensor_specs); - if (ret) { + for_each_available_child_of_node(np, sensor_np) { + ret = of_parse_phandle_with_args(sensor_np, "thermal-sensors", + "#thermal-sensor-cells", + 0, &sensor_specs); + if (ret) { + of_node_put(np); + of_node_put(sensor_np); + return ret; + } + + 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; + } + of_node_put(np); of_node_put(sensor_np); - return ret; - } - 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); } - 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);