Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]

On Thu, Mar 02, 2017 at 12:02:13AM +0800, Icenowy Zheng wrote:
> 
> 2017年3月1日 23:56于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >
> > On Wed, Mar 01, 2017 at 06:20:51PM +0800, Icenowy Zheng wrote: 
> > > 
> > > 2017年3月1日 18:14于 Maxime Ripard <maxime.ripard@free-electrons.com>写道： 
> > > > 
> > > > On Tue, Feb 28, 2017 at 03:18:13PM +0800, Icenowy Zheng wrote: 
> > > > > 
> > > > > 2017年2月28日 14:44于 Maxime Ripard <maxime.ripard@free-electrons.com>写道： 
> > > > > > 
> > > > > > On Tue, Feb 28, 2017 at 03:40:53AM +0800, Icenowy Zheng wrote: 
> > > > > > > From: Ondrej Jirman <megous@megous.com> 
> > > > > > > 
> > > > > > > Allwinner SoCs from H3 (including H5, A64, etc) have a new version of 
> > > > > > > thermal sensor, and needs a new driver for it. 
> > > > > > > 
> > > > > > > Add such a driver. 
> > > > > > > 
> > > > > > > Currently only H3 is supported, but other SoCs are easily to be 
> > > > > > > supported by adding new formula and set the sensor number. 
> > > > > > > 
> > > > > > > Signed-off-by: Ondřej Jirman <megous@megous.com> 
> > > > > > > [Icenowy: extend to support further multiple-sensor SoCs, change commit 
> > > > > > >  message] 
> > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> 
> > > > > > 
> > > > > > There's no need to create a new driver for that. This can be handled 
> > > > > > by the GPADC driver we already have. 
> > > > > 
> > > > > sun8i-ths is not GPADC at all. 
> > > > > 
> > > > > The latest SoC I know that use GPADC as thermal sensor is A33. 
> > > > 
> > > > It's not called the same way, but it definitely is an evolution of the 
> > > > same controller. There's no need for a new driver, only reworking what 
> > > > is already there. 
> > > 
> > > I don't think so -- here's some evidence: 
> 
> But the H3 THS have many new IRQs, functions, different sampling
> rate set method and a quite different register layout.
> 
> Doing this in GPADC driver is possible but meaningless.
> 
> > > 
> > > 1. The old GPADC do not have module clock. 
> >
> > The A33 could use a PLL. 
> 
> But it's a dedicated mod clk on new generation THS.

And all of this really are evolutions. The block is still driven in
the exact same way. And this is where there is value in having the
same driver: you share the logic, which is mostly common, instead of
duplicating it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3991 bytes --]

On Thu, Mar 09, 2017 at 08:36:24AM +0800, Icenowy Zheng wrote:
> 
> 
> 02.03.2017, 22:11, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Thu, Mar 02, 2017 at 12:02:13AM +0800, Icenowy Zheng wrote:
> >>  2017年3月1日 23:56于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >>  >
> >>  > On Wed, Mar 01, 2017 at 06:20:51PM +0800, Icenowy Zheng wrote:
> >>  > >
> >>  > > 2017年3月1日 18:14于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >>  > > >
> >>  > > > On Tue, Feb 28, 2017 at 03:18:13PM +0800, Icenowy Zheng wrote:
> >>  > > > >
> >>  > > > > 2017年2月28日 14:44于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >>  > > > > >
> >>  > > > > > On Tue, Feb 28, 2017 at 03:40:53AM +0800, Icenowy Zheng wrote:
> >>  > > > > > > From: Ondrej Jirman <megous@megous.com>
> >>  > > > > > >
> >>  > > > > > > Allwinner SoCs from H3 (including H5, A64, etc) have a new version of
> >>  > > > > > > thermal sensor, and needs a new driver for it.
> >>  > > > > > >
> >>  > > > > > > Add such a driver.
> >>  > > > > > >
> >>  > > > > > > Currently only H3 is supported, but other SoCs are easily to be
> >>  > > > > > > supported by adding new formula and set the sensor number.
> >>  > > > > > >
> >>  > > > > > > Signed-off-by: Ondřej Jirman <megous@megous.com>
> >>  > > > > > > [Icenowy: extend to support further multiple-sensor SoCs, change commit
> >>  > > > > > >  message]
> >>  > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >>  > > > > >
> >>  > > > > > There's no need to create a new driver for that. This can be handled
> >>  > > > > > by the GPADC driver we already have.
> >>  > > > >
> >>  > > > > sun8i-ths is not GPADC at all.
> >>  > > > >
> >>  > > > > The latest SoC I know that use GPADC as thermal sensor is A33.
> >>  > > >
> >>  > > > It's not called the same way, but it definitely is an evolution of the
> >>  > > > same controller. There's no need for a new driver, only reworking what
> >>  > > > is already there.
> >>  > >
> >>  > > I don't think so -- here's some evidence:
> >>
> >>  But the H3 THS have many new IRQs, functions, different sampling
> >>  rate set method and a quite different register layout.
> >>
> >>  Doing this in GPADC driver is possible but meaningless.
> >>
> >>  > >
> >>  > > 1. The old GPADC do not have module clock.
> >>  >
> >>  > The A33 could use a PLL.
> >>
> >>  But it's a dedicated mod clk on new generation THS.
> >
> > And all of this really are evolutions. The block is still driven in
> > the exact same way. And this is where there is value in having the
> > same driver: you share the logic, which is mostly common, instead of
> > duplicating it.
> 
> After some thinking, I can accept a common driver for A23/A33 thermal
> sensor and H3/A64/H5 ones, but I cannot accept use iio-sun4i-gpadc
> driver for it, as now H3/A64/H5 thermal sensors are not just an ADC --
> they features hardware alarm levels, and in some SoC it become a
> multiple channel thermal sensor.

The A23 and A33 is trivial to support on the already existing GPADC
driver. If you have it already working on the A33, then the amount of
work for your driver to support the A33, or for the GPADC to support
the newer SoC is strictly the same.

Except in one case, we have a common, debugged, already reviewed and
already merged driver. In the other case, we have no such things.

> I will insist on doing a dedicated driver for it, or if you like, I can add
> A23/A33 support to this driver. (Considering no one have already posted
> any patches for A23/A33 thermal sensor, except my old ones, so my
> work at least won't conflict with anything merged)

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474821.html

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On Wed, Mar 01, 2017 at 06:20:51PM +0800, Icenowy Zheng wrote:
> 
> 2017年3月1日 18:14于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >
> > On Tue, Feb 28, 2017 at 03:18:13PM +0800, Icenowy Zheng wrote: 
> > > 
> > > 2017年2月28日 14:44于 Maxime Ripard <maxime.ripard@free-electrons.com>写道： 
> > > > 
> > > > On Tue, Feb 28, 2017 at 03:40:53AM +0800, Icenowy Zheng wrote: 
> > > > > From: Ondrej Jirman <megous@megous.com> 
> > > > > 
> > > > > Allwinner SoCs from H3 (including H5, A64, etc) have a new version of 
> > > > > thermal sensor, and needs a new driver for it. 
> > > > > 
> > > > > Add such a driver. 
> > > > > 
> > > > > Currently only H3 is supported, but other SoCs are easily to be 
> > > > > supported by adding new formula and set the sensor number. 
> > > > > 
> > > > > Signed-off-by: Ondřej Jirman <megous@megous.com> 
> > > > > [Icenowy: extend to support further multiple-sensor SoCs, change commit 
> > > > >  message] 
> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> 
> > > > 
> > > > There's no need to create a new driver for that. This can be handled 
> > > > by the GPADC driver we already have. 
> > > 
> > > sun8i-ths is not GPADC at all. 
> > > 
> > > The latest SoC I know that use GPADC as thermal sensor is A33. 
> >
> > It's not called the same way, but it definitely is an evolution of the 
> > same controller. There's no need for a new driver, only reworking what 
> > is already there. 
> 
> I don't think so -- here's some evidence:
> 
> 1. The old GPADC do not have module clock.

The A33 could use a PLL.

> 2. The old GPADC do not have calibration stored in eFUSE.

The A33 had calibration data stored in the efuses.

> 3. R40 SoC have both RTP (it should be the old GPADC) and
> new-generation THS.

And this is probably just to be compatible with the A20.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Tue, Feb 28, 2017 at 03:18:13PM +0800, Icenowy Zheng wrote:
> 
> 2017年2月28日 14:44于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >
> > On Tue, Feb 28, 2017 at 03:40:53AM +0800, Icenowy Zheng wrote: 
> > > From: Ondrej Jirman <megous@megous.com> 
> > > 
> > > Allwinner SoCs from H3 (including H5, A64, etc) have a new version of 
> > > thermal sensor, and needs a new driver for it. 
> > > 
> > > Add such a driver. 
> > > 
> > > Currently only H3 is supported, but other SoCs are easily to be 
> > > supported by adding new formula and set the sensor number. 
> > > 
> > > Signed-off-by: Ondřej Jirman <megous@megous.com> 
> > > [Icenowy: extend to support further multiple-sensor SoCs, change commit 
> > >  message] 
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz> 
> >
> > There's no need to create a new driver for that. This can be handled 
> > by the GPADC driver we already have. 
> 
> sun8i-ths is not GPADC at all.
> 
> The latest SoC I know that use GPADC as thermal sensor is A33.

It's not called the same way, but it definitely is an evolution of the
same controller. There's no need for a new driver, only reworking what
is already there.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] thermal: add support for the thermal sensor on Allwinner new SoCs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Tue, Feb 28, 2017 at 03:40:53AM +0800, Icenowy Zheng wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> Allwinner SoCs from H3 (including H5, A64, etc) have a new version of
> thermal sensor, and needs a new driver for it.
> 
> Add such a driver.
> 
> Currently only H3 is supported, but other SoCs are easily to be
> supported by adding new formula and set the sensor number.
> 
> Signed-off-by: Ondřej Jirman <megous@megous.com>
> [Icenowy: extend to support further multiple-sensor SoCs, change commit
>  message]
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>

There's no need to create a new driver for that. This can be handled
by the GPADC driver we already have.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]