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?
> 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.
> 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.
It barely qualifies as "changing a lot", and isn't a compelling
argument to duplicate a 700+ LoC driver.
> 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.
> 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]
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.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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.
signature.asc
Description: PGP signature
