Hi,

Arnd Bergmann writes:

On Thursday 30 June 2011 16:58:38 Lothar Wa?mann wrote:

quoted

quoted

When adding new infrastructure, always keep in mind how you want it to look
after the device tree conversion. The partitions and min/max_* are easily covered
with that, but the init/exit function pointers are somewhat problematic.

Fortunately, you don't really require these functions for this driver. The _exit
function is completely unused, so just get rid of it.

The init function is used only to set up iomux, so the logical replacement is
a pointer to the iomux data, and calling mxs_iomux_setup_multiple_pads
directly from the driver.

NO! I strongly object that. mxs_iomux_setup_multiple_pads() is a
platform specific function and has no business inside a device driver
that should be platform agnostic.
Consider the same controller being part of a different SoC that
requires you to call mxs_iomux_v2_setup_multiple_pads() instead. You
would then have to check for the SoC type inside the driver to find
the right function to call which is quite obscene.

Won't this problem just go away as soon as we get the mxs converted to the
generic pinmux subsystem that Linus Walleij is doing?

Obviously, you would not have to check the soc type really, you would have
to instead check for different versions of the gpmi, which you most likely
have to anyway because reusing the same hardware block in a new chip usually
comes with other changes as well. 

The pin muxing is SoC specific, not GPMI specific. So the SoC version
should be checked, not something else that may be loosely linked to
it.

Note how the driver already does this with code like

+	if (GPMI_IS_MX23(this) || GPMI_IS_MX28(this))
+		nfc = &gpmi_nfc_hal_mxs;

If you really want to call out obsceneties, how about the fact that this
driver comes with an 805 line patch to add a HAL for a single chip!

Such abstractions should not be introduced as long as there is only
a single instance of the hardware.

I had already commented on this in the first round of patches from
Huang in March this year.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________