Hi Baruch, On mer., déc. 13 2017, Baruch Siach <baruch@xxxxxxxxxx> 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@xxxxxxxxxx - tel: +972.2.679.5364, http://www.tkos.co.il - > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
