Hi Daniel Lezcano,
Thanks for the review.
> Subject: Re: [PATCH v2 2/2] thermal/drivers: Add TSU driver for RZ/G2L
>
> On 20/11/2021 20:47, Biju Das wrote:
> > Add Thermal Sensor Unit(TSU) driver for RZ/G2L SoC.
>
> Please give a technical description of the sensor as it is the first
> submission.
Agreed. Will do.
>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Removed devm_add_action_or_reset from probe.
> > ---
> > drivers/thermal/Kconfig | 9 ++
> > drivers/thermal/Makefile | 1 +
> > drivers/thermal/rzg2l_thermal.c | 235
> > ++++++++++++++++++++++++++++++++
> > 3 files changed, 245 insertions(+)
> > create mode 100644 drivers/thermal/rzg2l_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > d7f44deab5b1..e37691e0bf20 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -354,6 +354,15 @@ config RCAR_GEN3_THERMAL
> > Enable this to plug the R-Car Gen3 or RZ/G2 thermal sensor driver
> into
> > the Linux thermal framework.
> >
> > +config RZG2L_THERMAL
> > + tristate "Renesas RZ/G2L thermal driver"
> > + depends on ARCH_RENESAS || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + depends on OF
> > + help
> > + Enable this to plug the RZ/G2L thermal sensor driver into the
> Linux
> > + thermal framework.
> > +
> > config KIRKWOOD_THERMAL
> > tristate "Temperature sensor on Marvell Kirkwood SoCs"
> > depends on MACH_KIRKWOOD || COMPILE_TEST diff --git
> > a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > 82fc3e616e54..f0c36a1530d5 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_SUN8I_THERMAL) += sun8i_thermal.o
> > obj-$(CONFIG_ROCKCHIP_THERMAL) += rockchip_thermal.o
> > obj-$(CONFIG_RCAR_THERMAL) += rcar_thermal.o
> > obj-$(CONFIG_RCAR_GEN3_THERMAL) += rcar_gen3_thermal.o
> > +obj-$(CONFIG_RZG2L_THERMAL) += rzg2l_thermal.o
> > obj-$(CONFIG_KIRKWOOD_THERMAL) += kirkwood_thermal.o
> > obj-y += samsung/
> > obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o
> > diff --git a/drivers/thermal/rzg2l_thermal.c
> > b/drivers/thermal/rzg2l_thermal.c new file mode 100644 index
> > 000000000000..ea828c15dd1e
> > --- /dev/null
> > +++ b/drivers/thermal/rzg2l_thermal.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L TSU Thermal Sensor Driver
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corporation */ #include
> > +<linux/delay.h> #include <linux/err.h> #include <linux/io.h> #include
> > +<linux/iopoll.h> #include <linux/module.h> #include
> > +<linux/of_device.h> #include <linux/platform_device.h> #include
> > +<linux/pm_runtime.h> #include <linux/reset.h> #include
> > +<linux/thermal.h>
> > +
> > +#include "thermal_hwmon.h"
> > +
> > +#define CTEMP_MASK 0xFFF
> > +
> > +/* default calibration values, if FUSE values are missing */
> > +#define SW_CALIB0_VAL 3148
> > +#define SW_CALIB1_VAL 503
> > +
> > +/* Register offsets */
> > +#define TSU_SM 0x00
> > +#define TSU_ST 0x04
> > +#define TSU_SAD 0x0C
> > +#define TSU_SS 0x10
> > +
> > +#define OTPTSUTRIM_REG(n) (0x18 + ((n) * 0x4))
> > +
> > +/* Sensor Mode Register(TSU_SM) */
> > +#define TSU_SM_EN_TS BIT(0)
> > +#define TSU_SM_ADC_EN_TS BIT(1)
> > +#define TSU_SM_NORMAL_MODE (TSU_SM_EN_TS | TSU_SM_ADC_EN_TS)
> > +
> > +/* TSU_ST bits */
> > +#define TSU_ST_START BIT(0)
> > +
> > +#define TSU_SS_CONV_RUNNING BIT(0)
> > +
> > +#define TS_CODE_AVE_SCALE(x) ((x) * 1000000)
> > +#define MCELSIUS(temp) ((temp) * 1000)
>
> cf. include/linux/units.h
The value we get is Celsius, which we are converting it to milli Celsius,
So that we can update with 0.5 degree(500 milli Celsius) granularity.
Ok will replace "1000" with "MILLIDEGREE_PER_DEGREE"
Defined in include/linux/units.h
>
> > +#define TS_CODE_CAP_TIMES 8 /* Capture times */
> > +#define RZG2L_THERMAL_GRAN 500 /* milli Celsius */
> > +
> > +#define RZG2L_TSU_SS_TIMEOUT_US 1000
> > +
> > +struct rzg2l_thermal_priv {
> > + struct device *dev;
> > + void __iomem *base;
> > + struct thermal_zone_device *zone;
> > + struct reset_control *rstc;
> > + u32 calib0, calib1;
> > +};
> > +
> > +static inline u32 rzg2l_thermal_read(struct rzg2l_thermal_priv *priv,
> > +u32 reg) {
> > + return ioread32(priv->base + reg);
> > +}
> > +
> > +static inline void rzg2l_thermal_write(struct rzg2l_thermal_priv *priv,
> u32 reg,
> > + u32 data)
> > +{
> > + iowrite32(data, priv->base + reg);
> > +}
> > +
> > +static int rzg2l_thermal_round(int temp) {
> > + int result, round_offs;
> > +
> > + round_offs = temp >= 0 ? RZG2L_THERMAL_GRAN / 2 : -
> RZG2L_THERMAL_GRAN / 2;
> > + result = (temp + round_offs) / RZG2L_THERMAL_GRAN;
> > +
> > + return result * RZG2L_THERMAL_GRAN;
> > +}
>
> roundup() | rounddown() ? (cf math.h)
OK will use roundup operation.
>
> > +
> > +static int rzg2l_thermal_get_temp(void *devdata, int *temp) {
> > + struct rzg2l_thermal_priv *priv = devdata;
> > + u32 result, dsensor, ts_code_ave;
> > + int val, i;
> > +
> > + result = 0;
>
> Init in the declaration
OK.
> What unit is the value returned ?
Value from the sensor is positive.
Actual sensor value is the value after applying curvature correction.
Then we apply the calibration constants to the above value to compute Tj, that value
can be negative.
Basically,
Tj Degree Celsius = curvature_correction(val) * caliberation constants - 40
>
> > + /* Read the ADC value 8 times with an interval of 20 microsecs */
>
> Why do you have to do that ? + comment format
TSU repeats measurement at 20 micro seconds intervals and automatically updates
the results of measurement.
As per HW manual procedure for measuring the temperature, it recommends to
Read 8 values consecutively every 20 micro seconds and take the average.
>
> /*
> *
> */
>
>
> > + for (i = 0; i < TS_CODE_CAP_TIMES ; i++) {
> > + result += rzg2l_thermal_read(priv, TSU_SAD) & CTEMP_MASK;
> > + usleep_range(20, 30);
> > + }
> > +
> > + /* Calculate the average value */
>
> With a proper comment in the loop, no need to add this one
OK.
>
> > + ts_code_ave = result / TS_CODE_CAP_TIMES;
>
> > + /* Curvature correction */
>
> What is it? + comment format
Sensor value may have drifts, to compensate that apply curvature correction
Based on this formula.
>
> > + dsensor = TS_CODE_AVE_SCALE(ts_code_ave) /
> > + (TS_CODE_AVE_SCALE(1) + (ts_code_ave * 13));
> > +
>
> No litteral, s/13/a_macro/
OK
>
> > + /* Temperature calculation */
> > + val = ((dsensor - priv->calib1) * (MCELSIUS(165) /
> > + (priv->calib0 - priv->calib1))) - MCELSIUS(40);
>
> Elaborate please + comment format
OK will do.
Basically,
Tj Degree Celsius = curvature_correction(val) * caliberation constants - 40
>
> > +
> > + /* Round value to device granularity setting */
>
> Comment format
OK. Will update.
>
> > + *temp = rzg2l_thermal_round(val);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops rzg2l_tz_of_ops = {
> > + .get_temp = rzg2l_thermal_get_temp,
> > +};
> > +
> > +static int rzg2l_thermal_init(struct rzg2l_thermal_priv *priv) {
> > + u32 reg_val;
> > +
> > + rzg2l_thermal_write(priv, TSU_SM, TSU_SM_NORMAL_MODE);
> > + rzg2l_thermal_write(priv, TSU_ST, 0);
> > +
> > + /* Before setting START bit, Wait for 60 µs */
>
> Why + comment format. The comment should not describe what is doing the
> code but *why*. It is obvious usleep(60) waits 60us
OK, it will take 60 micro seconds to place sensor in thermal mode.
After sensor in normal mode only we should set the start bit.
Will update comment with suggested format.
>
> > + usleep_range(60, 80);
> > +
> > + reg_val = rzg2l_thermal_read(priv, TSU_ST);
> > + reg_val |= TSU_ST_START;
> > + rzg2l_thermal_write(priv, TSU_ST, reg_val);
> > +
> > + return readl_poll_timeout(priv->base + TSU_SS, reg_val,
> > + reg_val == TSU_SS_CONV_RUNNING, 50,
> > + RZG2L_TSU_SS_TIMEOUT_US);
> > +}
> > +
> > +static void rzg2l_thermal_reset_assert_pm_disable_put(struct
> > +platform_device *pdev) {
> > + struct rzg2l_thermal_priv *priv = dev_get_drvdata(&pdev->dev);
> > +
> > + pm_runtime_put(&pdev->dev);
> > + pm_runtime_disable(&pdev->dev);
> > + reset_control_assert(priv->rstc);
> > +}
> > +
> > +static int rzg2l_thermal_remove(struct platform_device *pdev) {
> > + struct rzg2l_thermal_priv *priv = dev_get_drvdata(&pdev->dev);
> > +
> > + thermal_remove_hwmon_sysfs(priv->zone);
> > + rzg2l_thermal_reset_assert_pm_disable_put(pdev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzg2l_thermal_probe(struct platform_device *pdev) {
> > + struct thermal_zone_device *zone;
> > + struct rzg2l_thermal_priv *priv;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base))
> > + return PTR_ERR(priv->base);
> > +
> > + priv->dev = dev;
> > + priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(priv->rstc))
> > + return dev_err_probe(dev, PTR_ERR(priv->rstc),
> > + "failed to get cpg reset");
> > +
> > + reset_control_deassert(priv->rstc);
> > +
> > + pm_runtime_enable(dev);
> > + pm_runtime_get_sync(dev);
> > +
> > + priv->calib0 = rzg2l_thermal_read(priv, OTPTSUTRIM_REG(0));
> > + if (!priv->calib0)
> > + priv->calib0 = SW_CALIB0_VAL;
> > +
> > + priv->calib1 = rzg2l_thermal_read(priv, OTPTSUTRIM_REG(1));
> > + if (!priv->calib1)
> > + priv->calib1 = SW_CALIB1_VAL;
> > +
> > + platform_set_drvdata(pdev, priv);
> > + ret = rzg2l_thermal_init(priv);
> > + if (ret) {
> > + dev_err(dev, "Failed to start TSU");
> > + goto err;
> > + }
> > +
> > + zone = devm_thermal_zone_of_sensor_register(dev, 0, priv,
> > + &rzg2l_tz_of_ops);
> > + if (IS_ERR(zone)) {
> > + dev_err(dev, "Can't register thermal zone");
> > + ret = PTR_ERR(zone);
>
> dev_err(dev, "Can't register thermal zone: %d", ret);
>
> > + goto err;
> > + }
> > +
> > + priv->zone = zone;
> > + priv->zone->tzp->no_hwmon = false;
> > + ret = thermal_add_hwmon_sysfs(priv->zone);
> > + if (ret)
> > + goto err;
> > +
> > + dev_info(dev, "TSU probed with %s caliberation values",
> > + rzg2l_thermal_read(priv, OTPTSUTRIM_REG(0)) ? "hw" : "sw");
>
> s/dev_info/dev_debug/
OK.
Regards,
Biju
>
> > + return 0;
> > +
> > +err:
> > + rzg2l_thermal_reset_assert_pm_disable_put(pdev);
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id rzg2l_thermal_dt_ids[] = {
> > + { .compatible = "renesas,rzg2l-tsu", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_thermal_dt_ids);
> > +
> > +static struct platform_driver rzg2l_thermal_driver = {
> > + .driver = {
> > + .name = "rzg2l_thermal",
> > + .of_match_table = rzg2l_thermal_dt_ids,
> > + },
> > + .probe = rzg2l_thermal_probe,
> > + .remove = rzg2l_thermal_remove,
> > +};
> > +module_platform_driver(rzg2l_thermal_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/G2L TSU Thermal Sensor Driver");
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
