Re: [PATCH 1/7] iio: adc: sun4i-gpadc: rework for support multiple thermal sensor

From: Maxime Ripard <maxime.ripard@bootlin.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Yangtao Li <tiny.windzz@gmail.com>,
	lee.jones@linaro.org, robh+dt@kernel.org, mark.rutland@arm.com,
	wens@csie.org, knaack.h@gmx.de, lars@metafoo.de,
	pmeerw@pmeerw.net, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH 1/7] iio: adc: sun4i-gpadc: rework for support multiple thermal sensor
Date: Mon, 6 May 2019 14:28:07 +0200	[thread overview]
Message-ID: <20190506122807.4u323iys74jddcet@flea> (raw)
In-Reply-To: <20190505162215.3594f77d@archlinux>

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

Hi,

On Sun, May 05, 2019 at 04:22:15PM +0100, Jonathan Cameron wrote:
> On Fri,  3 May 2019 03:28:07 -0400
> Yangtao Li <tiny.windzz@gmail.com> wrote:
>
> > For some SOCs, there are more than one thermal sensor, and there are
> > currently four sensors on the A80. So we need to do some work in order
> > to support multiple thermal sensors:
> >
> >   1) add sensor_count in gpadc_data.
> >   2) introduce sun4i_sensor_tzd in sun4i_gpadc_iio, to support multiple
> >      thermal_zone_device and distinguish between different sensors.
> >   3) modify read temperature and initialization function.
>
> This comment doesn't mention the devm change. If it had it would have
> raised immediate alarm bells.
>
> I'm also not keen on the web of pointers that this driver is steadily
> evolving.  I can't immediately see how to reduce that complexity however.

So I might be responsible for that, and looking back, this has been a
mistake.

This driver was initally put together to support a controller found in
older (A10 up to A31) Allwinner SoCs. This controller had an ADC
driver that could be operated as a touchscreen controller, and was
providing a CPU temperature sensor and a general purpose ADC.

However, we already had a driver for that controller in drivers/input
to report the CPU temperature, and the one in IIO was introduced to
support the general purpose ADC (and the CPU temperature). The long
term goal was to add the touchscreen feature as well eventually so
that we could remove the one in drivers/input. That didn't happen.

At the same time, the Allwinner hardware slowly evolved to remove the
touchscreen and ADC features, and only keep the CPU temperature
readout. It then evolved further on to support multiple temperatures
(for different clusters, the GPU, and so on).

So, today, we're in a situation where I was pushing everything into
that IIO drivers since there was similiraties between all the
generations, but the fact that we have to support so many odd cases
(DT bindings compatibility, controllers with and without ADC, etc)
that it becomes a real mess.

And that mess isn't really used by anybody, since we want to have the
touchscreen.

There's only one SoC that is supported only by that driver, which is
the A33 that only had a CPU temperature readout, and is still pretty
similar to the latest SoC from Allwinner (that is supported by this
series).

I guess, for everyone's sanity and in order to not stall this further,
it would just be better to create an hwmon driver for the A33 (and
onwards, including the H6) for the SoC that just have the temperature
readout feature. And for the older SoC, we just keep the older driver
under input/. Once the A33 is supported, we'll remove the driver in
IIO (and the related bits in drivers/mfd).

Armbian already has a driver for that they never upstreamed iirc, so
it might be a good starting point, and we would add the support for
the H6. How does that sound?

Sorry for wasting everybody's time on this.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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