On Mon, May 06, 2019 at 07:55:25PM +0200, Ondřej Jirman wrote: > On Tue, May 07, 2019 at 01:08:39AM +0800, Frank Lee wrote: > > On Tue, May 7, 2019 at 12:52 AM Icenowy Zheng <icenowy@xxxxxxx> wrote: > > > > > > 在 2019-05-06一的 14:28 +0200，Maxime Ripard写道： > > > > 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@xxxxxxxxx> 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). > > > > a hwmon driver or a thermal driver？ > > > > > > > > I think a thermal driver is better. > > > > This is what I hope to see a few months ago. > > > > > > > > Other SoCs' thermal sensor drivers are all thermal drivers. > > > > > > > > > > > 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? > > > > > > I think the developer abandoned to upstream it because of the previous > > > problem ;-) > > > > > > Maybe it can be taken and add A33&H6 support. > > > > If OK, I am going to start some thermal driver work this weekend. : ) > > There are plenty of thermal drivers flying around, with varying levels > of support for various SoCs: > > - H3/H5: https://megous.com/git/linux/commit/?h=ths-5.1&id=b8e20c5da7a00b3a3fa1b274fc8d5bea95872b0a > - A83T: https://megous.com/git/linux/commit/?h=ths-5.1&id=796dff9a946fd475cc1e4bb948a723ea841c640c > - H6: https://megous.com/git/linux/commit/?h=opi3-5.1&id=aeab762c19b4aa228a295258c9d6b2e1f143bf86 > > For H3/H5 Icenowy also tried to upstream some variant of my THS driver, with > better SID/calibration data reading support. > > I'd suggest starting with the H6 driver above (as that implements the > calibration data readout correctly), and make it so that it can support multiple > SoCs. Yeah, that seems like a good plan Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
