Re: [PATCH v5 2/2] thermal: uniphier: add UniPhier thermal driver

[Masahiro Yamada]

2017-07-21 20:21 GMT+09:00 Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>:

> +static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
> +{
> +       struct regmap *map = tdev->regmap;
> +       u32 val;
> +       int ret;
> +
> +       /* stop PVT */
> +       regmap_write_bits(map, tdev->data->block_base + PVTCTLEN,
> +                         PVTCTLEN_EN, 0);
> +
> +       /*
> +        * set default value if missing calibrated value
> +        *
> +        * Since SoC has a calibrated value that was set in advance,
> +        * TMODCOEF shows non-zero and PVT refers the value internally.
> +        *
> +        * However, some boards don't have the calibrated value.
> +        * In that case, TMODCOEF shows zero and the driver has to set
> +        * default value manually.
> +        */
> +       ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
> +       if (ret)
> +               return ret;
> +       if (!val)
> +               regmap_write(map, tdev->data->tmod_setup_addr,
> +                           TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
> +                           TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));


This code is strange.

What if TMODCOEF has no calibrated value and
"socionext,tmod-calibration" is not set either?


->tmod_setupaddr will be set to zero
and the sensor would not work, right?




> +       /* get tmod-calibration values */
> +       calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
> +                               NULL);
> +       if (calib) {
> +               tdev->tmod_calib0 = of_read_number(calib, 1);
> +               tdev->tmod_calib1 = of_read_number(calib + 1, 1);
> +       }


>From your DT change (https://patchwork.kernel.org/patch/9826391/),
this property seems a pair of u32 values, like follows:

socionext,tmod-calibration = <0x0f22 0x68ee>;


Why do you need to use of_read_number() to retrieve each u32 value?

See the comment and return value (u64):

------------------>8-----------------
/* Helper to read a big number; size is in cells (not bytes) */
static inline u64 of_read_number(const __be32 *cell, int size)
------------------8<-----------------


Also, you are not checking the length of the property.
of_read_number() may over-run the property.



of_property_read_u32_array() will be a better choice.


I'd propose the code like follows:





diff --git a/drivers/thermal/uniphier_thermal.c
b/drivers/thermal/uniphier_thermal.c
index 1a77c5bf6930..eea8a3053584 100644
--- a/drivers/thermal/uniphier_thermal.c
+++ b/drivers/thermal/uniphier_thermal.c
@@ -94,16 +94,16 @@ struct uniphier_tm_soc_data {
 struct uniphier_tm_dev {
        struct regmap *regmap;
        bool alert_en[ALERT_CH_NUM];
-       u32 tmod_calib0;
-       u32 tmod_calib1;
        struct thermal_zone_device *tz_dev;
        const struct uniphier_tm_soc_data *data;
 };

-static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev)
+static int uniphier_tm_initialize_sensor(struct uniphier_tm_dev *tdev,
+                                        struct device *dev)
 {
        struct regmap *map = tdev->regmap;
        u32 val;
+       u32 tmod_val[2];
        int ret;

        /* stop PVT */
@@ -123,10 +123,22 @@ static int uniphier_tm_initialize_sensor(struct
uniphier_tm_dev *tdev)
        ret = regmap_read(map, tdev->data->map_base + TMODCOEF, &val);
        if (ret)
                return ret;
-       if (!val)
+       if (!val) {
+               /*
+                * No preset value found in the register.
+                * Look for the fall-back values in DT.
+                */
+               ret = of_property_read_u32_array(dev->of_node,
+                                                "socionext,tmod-calibration",
+                                                tmod_val,
ARRAY_SIZE(tmod_val));
+               if (ret) {
+                       dev_err(dev, "neither register nor DT has
calibrated values.\n");
+                       return ret;
+               }
                regmap_write(map, tdev->data->tmod_setup_addr,
-                           TMODSETUP0_EN | TMODSETUP0_VAL(tdev->tmod_calib0) |
-                           TMODSETUP1_EN | TMODSETUP1_VAL(tdev->tmod_calib1));
+                            TMODSETUP0_EN | TMODSETUP0_VAL(tmod_val[0]) |
+                            TMODSETUP1_EN | TMODSETUP1_VAL(tmod_val[1]));
+       }

        /* select temperature mode */
        regmap_write_bits(map, tdev->data->block_base + PVTCTLMODE,
@@ -254,7 +266,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
        struct device_node *parent;
        struct uniphier_tm_dev *tdev;
        const struct thermal_trip *trips;
-       const __be32 *calib;
        int i, ret, irq, ntrips, crit_temp = INT_MAX;

        tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
@@ -269,14 +280,6 @@ static int uniphier_tm_probe(struct platform_device *pdev)
        if (irq < 0)
                return irq;

-       /* get tmod-calibration values */
-       calib = of_get_property(dev->of_node, "socionext,tmod-calibration",
-                               NULL);
-       if (calib) {
-               tdev->tmod_calib0 = of_read_number(calib, 1);
-               tdev->tmod_calib1 = of_read_number(calib + 1, 1);
-       }
-
        /* get regmap from syscon node */
        parent = of_get_parent(dev->of_node); /* parent should be syscon node */
        regmap = syscon_node_to_regmap(parent);
@@ -288,7 +291,7 @@ static int uniphier_tm_probe(struct platform_device *pdev)
        }
        tdev->regmap = regmap;

-       ret = uniphier_tm_initialize_sensor(tdev);
+       ret = uniphier_tm_initialize_sensor(tdev, dev);
        if (ret) {
                dev_err(dev, "failed to initialize sensor\n");
                return ret;




-- 
Best Regards
Masahiro Yamada