[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: Tuesday, December 06, 2011 5:19 AM
To: Linus Walleij
Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
R65073; kernel at pengutronix.de
Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:

quoted

On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng [off-list ref]

wrote:

quoted

quoted

+enum imx_mx53_pads {
+ ? ? ? MX53_GPIO_19 = 0,
+ ? ? ? MX53_KEY_COL0 = 1,

(...)

First I thought it looked a bit strange since you needed enums for all
pads but then I realized that your macros use the same enumerator name
to name the pad and then it looks sort of clever.

But maybe put in a comment about that here:

quoted

+/* Pad names for the pinmux subsystem */

Like this:

/*
 * Pad names for the pinmux subsystem.
 * These pad names are constructed from the pin enumerator names
 * in the IMX_PINCTRL_PIN() macro.
 */

quoted

+static const struct pinctrl_pin_desc mx53_pads[] = {
+ ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
+ ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),

(...)

quoted

+/* mx53 pin groups and mux mode */
+static const unsigned mx53_fec_pins[] = {
+ ? ? ? MX53_FEC_MDC,
+ ? ? ? MX53_FEC_MDIO,
+ ? ? ? MX53_FEC_REF_CLK,
+ ? ? ? MX53_FEC_RX_ER,
+ ? ? ? MX53_FEC_CRS_DV,
+ ? ? ? MX53_FEC_RXD1,
+ ? ? ? MX53_FEC_RXD0,
+ ? ? ? MX53_FEC_TX_EN,
+ ? ? ? MX53_FEC_TXD1,
+ ? ? ? MX53_FEC_TXD0,
+};

I understand this.

quoted

+static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0,
+0 };

But what is this? Just zeroes? Why?
Especially with a const so they really cannot be anything else. The
same pin (0) can only be enumerated once.

quoted

+static const unsigned mx53_sd1_pins[] = {
+ ? ? ? MX53_SD1_CMD,
+ ? ? ? MX53_SD1_CLK,
+ ? ? ? MX53_SD1_DATA0,
+ ? ? ? MX53_SD1_DATA1,
+ ? ? ? MX53_SD1_DATA2,
+ ? ? ? MX53_SD1_DATA3,
+
+};
+static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };

And here again.

quoted

+static const unsigned mx53_sd3_pins[] = {
+ ? ? ? MX53_PATA_DATA8,
+ ? ? ? MX53_PATA_DATA9,
+ ? ? ? MX53_PATA_DATA10,
+ ? ? ? MX53_PATA_DATA11,
+ ? ? ? MX53_PATA_DATA0,
+ ? ? ? MX53_PATA_DATA1,
+ ? ? ? MX53_PATA_DATA2,
+ ? ? ? MX53_PATA_DATA3,
+ ? ? ? MX53_PATA_IORDY,
+ ? ? ? MX53_PATA_RESET_B,
+
+};
+static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2,
+2 };

This also looks strange. Can you explain what these muxes are?

Freescale has named the pins after their primary function which is quite
confusing.

The above means:

MX53_PATA_DATA8 -> mux mode 4
MX53_PATA_DATA9 -> mux mode 4
...

This brings me to the point that currently we have the pins described as

#define MX53_PAD_<name>__<function>

which means that you don't have to look into the datasheet to get the
different options for a pin (and don't have a chance to get it wrong).
I don't really want to lose this.

Obviously current used pin defines in that way is pretty good.
And I also don't want to lose this.

Actually I also have tried to see if we can reuse the current iomux-v3 code.

For current pinmux driver, one approach I can see is that define mux
in enum for each pin like:

enum MX53_PAD_GPIO_19_MUX {
        MX53_PAD_GPIO_19__KPP_COL_5,
        MX53_PAD_GPIO_19__GPIO4_5,
        MX53_PAD_GPIO_19__CCM_CLKO,
        MX53_PAD_GPIO_19__SPDIF_OUT1,
        MX53_PAD_GPIO_19__RTC_CE_RTC_EXT_TRIG2,
        MX53_PAD_GPIO_19__ECSPI1_RDY,
        MX53_PAD_GPIO_19__FEC_TDATA_3,
        MX53_PAD_GPIO_19__SRC_INT_BOOT,
};
Then put them in a common file for each mx53 based board to use.

Take uart1 as an example, it could be:
static const unsigned mx53_uart1_pins[] = {
        MX53_CSI0_DAT10,
        MX53_CSI0_DAT11,
};

static const unsigned mx53_uart1_mux[] = {
        MX53_CSI0_DAT10__UART1_TXD_MUX,
        MX53_CSI0_DAT11__UART1_RXD_MUX
};

static const struct imx_pin_group mx53_pin_groups[] = {
        IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
};

The benefit is that it's very clear and good maintainable.
The defect is it will increase the code size a lot by defining all
pin's mux enum and each pin's mux array in board file.

Do you think if it's ok or you have any better idea?

Regards
Dong Aisheng