于 2017年5月15日 GMT+08:00 上午4:16:41, Maxime Ripard <[email protected]> 写到: >Hi, > >On Wed, May 10, 2017 at 12:30:26AM +0800, [email protected] wrote: >> Hi everyone, >> I think we still need to discuss on a thermal sensor driver on some >> Allwinner SoCs (specially, H3/A64/H5/A83T/R40). >> >> These SoCs seem to have already a stable design of thermal sensor, >> which can easily add or remove several thermal channels (H3 has >> only one thermal channel, H5 2, other SoCs 3). Note: older SoCs' >> GPADC or thermal sensor has never more than 1 thermal channel. >> >> Maxime has already said to reject a dedicated driver, however >> I still think it more appropriate to have a dedicated driver, >> because: >> 1. Current driver is an IIO-based driver that registers a thermal >zone, >> which is a mixture of two different driver architectures. They are >> acceptable for a real general purpose ADC that contains thermal >part, >> but not acceptable for a real thermal-only sensor. > >You keep saying that IIO is a bad idea, but you never developped >why. So, could you say what makes you think that?
Thermal framework is already enough for a thermal driver. Add a IIO part will only make it more complex. > >> See [1]. For a thermal zone-based thermal driver, a thermal zone >for >> it is necessary; however, here Maxime considered it a IIO driver >and >> assumes that "The thermal zone itself shouldn't even be in this >> patch", but without this thermal zone defined the thermal zone >part >> will directly refuse to probe. > >Surely that's something we can easily fix. > >> 2. The newer generation SoCs really have already stable design, that >> has not changed in already 5 SoC designs. (And the still unknown >> SUN8IW10 design (B288?) also uses this version of thermal sensor, >> so this number should be at least 6) > >I'm not sure what your point here is. > >The current driver supports 7 of them (A10, A10s, A13, A20, A31, A23 >and A33), and expanding it to support the new SoCs will make it >support 13 of them. That also looks like a worthwile investment. But for the old 7 there's 2 groups of different designs: A10 to A31 is one design, and A23 and A33 is in fact a changed design. > >> 3. The designs have changed a lot so that the initialization code >could >> be hardly shared. See [2], which has more than a half of >> initialization code rewritten. > >That's a bold claim. The only thing you're doing in that patch is: > 1) Adding support for more clocks > 2) Adding two configurable registers offset > 3) Adding four register writes, 3 of them that can be easily shared > with the previous design using regfields. But just for a thermal driver there's already too many changes. > >It barely qualifies as "changing a lot", and isn't a compelling >argument to duplicate a 700+ LoC driver. 700 LoC is the GPADC part, not the thermal part. The dedicated thermal driver from Ondrej only weighes 200+ lines. > >> 4. Newer designs have the capability to have multiple sensor >channels, >> which could not be dealed with current drivers now, and will be a >> rewrite disaster if we keep the IIO & thermal two framework >design, >> which will make the code fragemented and difficult to maintain >> (it will give people a feeling that we forced two drivers with >some >> similar functionalities to be a driver). > >Whether it's difficult to maintain or not is not your responsibility, >nor your choice to make. Chen-Yu and I will end up maintaining it. > >And again, whatever change is needed will be easier to maintain than >*three* drivers that share the same functionalities. We have two, we >want to kill one, it's not to have two all over again. The current driver have no consideration of multiple thermal channels, as it's only available in newer style thermal sensors. The two remaining drivers doesn't share *the same* functionalities. The thermal driver doesn't do GPADC, and lots of code in sun8i-gpadc-iio is for GPADC. > >> 5. In the situation of R40, a old-style GPADC(RTP?) and a new-style >> thermal sensor co-exists. That could indicate that they're >different >> designs and the new-style thermal sensor is better than the >> integrated thermal sensor in the GPADC. > >[citation needed] Check the R40 user manual. (already available on R40 wiki page) In user manual v1.0, 3.14. RTP is the old style RTP with thermal sensor stripped. 3.15. Thermal Sensor is the new style THS. > >And we can support both using the same driver. > >> So I think a dedicated driver is still better, which will be placed >at >> drivers/thermal and be shared among new-generation thermal sensor, >> even if the thermal sensor seems to be a modified version of old >design. >> It's changed enough to have a dedicated driver. >> >> That will largely reduce the maintaince difficulty of both drivers. >> We are not sharing code for sharing code, we are sharing code for >easier >> maintaince. The share of code between two kinds of THS is much >smaller >> than the burden brought by the code sharing, and with a dedicated >driver, >> the MFD and IIO maintainers can see less code that should originally >> go to thermal framework. >> >> P.S. Maybe the A33 thermal sensor also shouldn't go into IIO >framework, >> for the first reason mentioned above. > >I still haven't seen any compelling argument, and until I see one, I >will oppose any thing like that, for the reasons above. Totally you are thinking reusing the full GPADC driver on H3+. However, the only reusable code is in fact very few. > >Maxime -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
