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

[Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

Hi Baruch,
 
 On mer., déc. 13 2017, Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquel,
>
> On Wed, Dec 13, 2017 at 09:55:01AM +0100, Miquel RAYNAL wrote:
>> > > > 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.
>
> I don't think so. The code of these functions in the vendor kernel overheat 
> support implementation is the same, duplicated. The variations are only in 
> registers/bits offsets. A single routine with one or two added 
> armada_thermal_data fields would be much easier to comprehend and maintain.
>
>> 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?
>
> The Marvell code does not "share" the code. Separate functions means 
> duplicated code that obscures the hardware details, making maintenance harder 
> on the long run.

Well, Miquel speak about writting new code, so I don't see why you refer
the Marvell LSP code. Also, I don't see how having common function will
duplicate the code.

Gregory

>
> https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
>
> baruch
>
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html