On 3/19/2020 7:06 PM, Daniel Lezcano wrote:

On 19/03/2020 13:52, Keerthy wrote:

quoted

On 19/03/20 6:08 pm, Daniel Lezcano wrote:

quoted

On 18/03/2020 09:30, Keerthy wrote:

quoted

The bandgap provides current and voltage reference for its internal
circuits and other analog IP blocks. The analog-to-digital
converter (ADC) produces an output value that is proportional
to the silicon temperature.

Currently reading temperatures and creating work to periodically
read temperatures from the zones are supported.
There are no active/passive cooling agent supported.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

[ ... ]

quoted

quoted

quoted

+static const int k3_adc_to_temp[] = {
+    -40000, -40000, -40000, -40000, -39800, -39400, -39000, -38600,

[ ... ]

quoted

quoted

quoted

123000,
+    123400, 123800, 124200, 124600, 124900, 125000,
+};

Can be this array replaced by an initialization array with a formula?

Why do we have most of the time a step of 400 then suddenly 500 and 400
again? eg. 30600, 31000, 31400, 31900, 32500, 33000, 33400

This has come from a polynomial equation which i do not want to
calculate every time we read the temperature. Hence prefer Look up table.

Agree, it makes sense to not calculate every time the temperature is read.

I was suggesting to fill the array at init time with this polynomial
formula instead of hardcoding it.

As in case of OMAP this is a standard polynomial equation that does not
change so i would prefer not calculating in the driver and having the 
look up table from TRM.

[ ... ]

quoted

quoted

quoted

+
+    /* Get the sensor count in the VTM */
+    val = readl(bgp->base + K3_VTM_DEVINFO_PWR0_OFFSET);
+    cnt = val & K3_VTM_DEVINFO_PWR0_TEMPSENS_CT_MASK;
+    cnt >>= __ffs(K3_VTM_DEVINFO_PWR0_TEMPSENS_CT_MASK);
+
+    data = devm_kcalloc(dev, cnt, sizeof(*data), GFP_KERNEL);
+    if (!data) {
+        ret = -ENOMEM;
+        goto err_alloc;
+    }
+
+    /* Register the thermal sensors */
+    for (id = 0; id < cnt; id++) {
+        data[id].sensor_id = id;
+        data[id].bgp = bgp;
+        data[id].ctrl_offset = K3_VTM_TMPSENS0_CTRL_OFFSET +
+                    id * K3_VTM_REGS_PER_TS;
+        data[id].stat_offset = data[id].ctrl_offset + 0x8;
+        INIT_WORK(&data[id].thermal_wq, k3_thermal_work);

         What is supposed to do ?

Periodically poll temperature. I know there is no passive cooling agent
like cpufreq at the moment but i do have a critical trip do you
recommend to remove that?

Actually I was referring to the workq which is initialized, the callback
set but it is never called. It can be removed.

Okay got it.

Please take also the opportunity to wrap the calls to the register with
an explicit helper function name.

IIUC the comment asks me to define a separate function that takes care 
of the body of the for loop.

And remove reg_cnt which is unused.

Sure

Thanks,
Keerthy

Thanks

   -- Daniel