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

From: "J, KEERTHY" <j-keerthy@ti.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>, <rui.zhang@intel.com>,
	<robh+dt@kernel.org>
Cc: <amit.kucheria@verdurent.com>, <t-kristo@ti.com>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-pm@vger.kernel.org>, <mark.rutland@arm.com>
Subject: Re: [RESEND PATCH v4 2/4] thermal: k3: Add support for bandgap sensors
Date: Mon, 23 Mar 2020 09:22:52 +0530	[thread overview]
Message-ID: <b73f1fdc-16d6-7320-ab63-e51bb388fb5d@ti.com> (raw)
In-Reply-To: <d8bff098-f852-4c55-0afc-d7fd043dd10a@linaro.org>

On 3/19/2020 7:06 PM, Daniel Lezcano wrote:
> 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@ti.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.

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.

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