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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d8bff098-f852-4c55-0afc-d7fd043dd10a@linaro.org>
Date:   Thu, 19 Mar 2020 14:36:48 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Keerthy <j-keerthy@...com>, rui.zhang@...el.com, robh+dt@...nel.org
Cc:     amit.kucheria@...durent.com, t-kristo@...com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
        mark.rutland@....com
Subject: Re: [RESEND PATCH v4 2/4] thermal: k3: Add support for bandgap
 sensors

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

[ ... ]

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