Re: [PATCH 1/2] thermal: mediatek: add support for MT7986 and MT7981

[Henry Yen (顏修溫)]

On Tue, 2022-11-29 at 12:11 +0000, Daniel Golle wrote:
> On Tue, Nov 29, 2022 at 11:53:58AM +0000, Henry Yen (顏修溫) wrote:
> > On Mon, 2022-10-31 at 23:07 +0000, Daniel Golle wrote:
> > > diff --git a/drivers/thermal/mtk_thermal.c
> > > b/drivers/thermal/mtk_thermal.c
> > > 
> > > +static int raw_to_mcelsius_v3(struct mtk_thermal *mt, int
> > > sensno,
> > > s32 raw)
> > > +{
> > > +	s32 tmp;
> > > +
> > > +	if (raw == 0)
> > > +		return 0;
> > > +
> > > +	raw &= 0xfff;
> > > +	tmp = 100000 * 15 / 16 * 10000;
> > > +	tmp /= 4096 - 512 + mt->adc_ge;
> > > +	tmp /= 1490;
> > > +	tmp *= raw - mt->vts[sensno] - 2900 - mt->adc_oe + 512;
> > 
> > Hi Daniel,
> > 
> > Regarding the conversion formula, I would suggest following the
> > original one, i.e., discarding "adc_oe" parameter as shown in [1].
> 
> Ok, I see. According to the commit description it looked to me more
> like adc_oe has been dropped by accident, it doesn't sound like it
> happened on purpose:
> "Refactor MT7986 thermal temperature calculation formula to
> prevent loss of floating-point accuracy."
> 
> Hence it made sense to be to keep the parameter and really only
> use the updated formula to not loose precision. Maybe you can inquire
> with the original author if dropping adc_oe was intentional, despite
> being unmentioned in the commit message.

Actually I'm just the original author who developes MT7986/MT7981
thermal driver and refactors this conversion formula. Getting rid of
"adc_oe" is indeed an intentional change, because back then our
hardware members indicated that this parameter shouldn't have been
used.

Thank you for your carefulness :)

> 
> > This equation is derived based on hardware-specific theory, so any
> > arbitrary change could possibly lead to incorrect temperature
> > output.
> > Thanks.
> 
> On my BPi-R3 board I found the value 512 burned into the efuse, so in
> practise the resulting calculated temperature is exactly the same on
> this board.
> 
> If other MT7986 or MT7981 boards will have arbitrary values stored in
> adc_oe field in the efuse because this value isn't even used during
> the manufacturer's calibration process, then of course, there is no
> choice other than dropping it here as well.
> 

The value of "adc_oe" is not necessarily 512. It could possibly vary
from board to board.

- Henry

> > 
> > [1]
> > 
> > 
https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/baf36c7eef477aae1f8f2653b6c29e2caf48475b%5E%21/#F0
> > 
> > 
> > Henry
> > 
> > > +
> > > +	return mt->degc_cali * 500 - tmp;
> > > +}
> > > +
> > >  /**
> > >   * mtk_thermal_get_bank - get bank
> > >   * @bank:	The bank