Messages in this thread | | | Date | Tue, 6 Dec 2011 14:25:01 +0800 | From | Shawn Guo <> | Subject | Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support |
| |
On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote: > On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote: > > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote: > > > > > +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: > > > > > +/* 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. > > */ > > > > > +static const struct pinctrl_pin_desc mx53_pads[] = { > > > + IMX_PINCTRL_PIN(MX53_GPIO_19), > > > + IMX_PINCTRL_PIN(MX53_KEY_COL0), > > (...) > > > > > +/* 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. > > > > > +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. > > > > > +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. > > > > > +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> > But that's also the reason why we have so many lengthy iomux-mx*.h on imx. Taking iomux-mx53.h for example, it's a 109K header with 1219 LOC, but probably only 10% of the definitions will actually be used.
> which means that you don't have to look into the datasheet to get the > different options for a pin
Looking at the datasheet when we write code is a pretty natural thing to me.
-- Regards, Shawn
> (and don't have a chance to get it wrong). > I don't really want to lose this. >
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |