Re: [RESEND PATCH v4 2/4] thermal: k3: Add support for bandgap sensors

[Daniel Lezcano]

On 19/03/2020 13:52, Keerthy wrote:
> 
> 
> On 19/03/20 6:08 pm, Daniel Lezcano wrote:
>> On 18/03/2020 09:30, Keerthy wrote:
>>> 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@xxxxxx>
>>> ---

[ ... ]

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

[ ... ]

>>> 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.

[ ... ]

>>> +
>>> +ÂÂÂ /* 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.

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

And remove reg_cnt which is unused.

Thanks

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog