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

[J, KEERTHY]

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

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