Re: [PATCH v2 3/4] thermal: armada: add support for CP110

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hello Baruch,


> > > How would a separate init_sensor routine improve things?
> > 
> > So yes please do it, thanks to this you won't have to add the
> > control_msb_offset member and can use a clean function. Moreover if
> > in the future we see some usefulness for this LSB register then we
> > could use the new compatible for the Armada 38x.
> 
> There are two separate issues here:
> 
>   1. DT binding
> 
>   2. init_sensor callback implementation
> 
> We both agree on #1. The A38x and CP110 need separate compatible
> strings. In case we want to access the LSB control register on Armada
> 38x, we will need yet another compatible string
> (marvell,armada380-v2-thermal maybe?).
> 
> As for #2, I'm all for sharing as much code as possible. I find the
> vendor kernel approach of duplicating the init routines[1] unhelpful
> as it violates the DRY principle. The differences between
> armada380_init_sensor() and cp110_init_sensor() are minor. In my
> opinion, these differences should be expressed explicitly in the
> armada_thermal_data, in a similar way to my suggested
> control_msb_offset field. The vendor code hides these differences in
> slight variations of duplicated code.
> 
> What is the advantage of a separate init routine?

The advantage is that is the very near future I plan to add the
overheat interrupt only on CP110 (not on 38x) and this needs some
initialization. So if we don't make different routines now, I will
have to do it right after.

What would be fine is to have the shared code in a separate function,
like it is done in Marvell kernel. What do you think about that?

Thanks,
Miquèl