Re: [PATCH v4 2/3] thermal: hisilicon: add thermal sensor driver for Hi3660

[Wangtao (Kevin, Kirin)]

å 2017/9/4 23:06, Leo Yan åé:
> On Mon, Sep 04, 2017 at 01:06:39PM +0200, Daniel Lezcano wrote:
> > Hi Kevin,
> >
> >
> > On 04/09/2017 09:56, Wangtao (Kevin, Kirin) wrote:
> > > å 2017/9/1 5:17, Daniel Lezcano åé:
> > > > Hi Kevin,
> > > >
> > > >
> > > > On 29/08/2017 10:17, Tao Wang wrote:
> > > > > From: Tao Wang <kevin.wangtao@xxxxxxxxxx>
> > > > >
> > > > > This patch adds the support for thermal sensor of Hi3660 SoC.
> > > >
> > > > for the Hi3660 SoC thermal sensor.
> > > >
> > > > > this will register sensors for thermal framework and use device
> > > > > tree to bind cooling device.
> > > >
> > > > Is it possible to give a pointer to some documentation or to describe
> > > > the hardware?
> > > Yes, there used to be on patch V3, I removed it on V4.
> > > > An explanation of the adc min max coef[] range[] conversion would be
> > > > nice.
> > > OK
> > > > In addition, having the rational behind the average and the max would be
> > > > nice. Do we really need both avg and max as virtual sensor?
> > > We only need max currently.
> > > > > Signed-off-by: Tao Wang <kevin.wangtao@xxxxxxxxxx>
> > > > > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > > > > ---
> > > > >    drivers/thermal/Kconfig        |  13 +++
> > > > >    drivers/thermal/Makefile       |   1 +
> > > > >    drivers/thermal/hisi_tsensor.c | 209
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > >
> > > >
> > > > IMO, we don't need a new file, but merge this code with the current
> > > > hisi_thermal.c driver. BTW, the hi6220 has also a tsensor which is
> > > > different from this one.
> > > >
> > > > I suggest to base the hi3660 thermal driver on top of the cleanup I sent
> > > > for the hi6220.
> > > The tsensor of hi3660 is a different one, merging the code with hi6220
> > > will need a lot of change.
> >
> > Have a look at the hisi_thermal.c at:
> >
> > https://git.linaro.org/people/daniel.lezcano/linux.git/tree/drivers/thermal/hisi_thermal.c?h=thermal/hikey-4.14
> >
> > after the cleanup I recently sent.
> >
> > I'm pretty sure, with a little effort, we can merge both.
> >
> > Especially if the virtual things is separated.
> >
> > At the end, what do we do ? Read a register.
>
> Just more input at here. I agree currently Hi3660 thermal driver
> is quite similiar with Hi6220, before we wrote a dedicated Hi3660
> thermal driver due we used mailbox method rather than shared
> memory modeThere are no shared memory mode in thermal driver, I think you mix it up with clock driver.
>
> If we merge two thermal drivers, this means Hi3660 register layout
> should be adjusted as same with Hi6220; I am not sure if this is
> feasible and need Kevin to confirm for this.
Hi3660's register layout is different from Hi6220, and Hi3660's tsensors are configed by MCU, Kernel driver only need to read the temperature.
> And does this mean we need provide interrupt mode for Hi3660? Or
> we can extend the driver to only support pollig mode?

> [...]
>
> Thanks,
> Leo Yan
>
> .