[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20111205211838.GG27267@pengutronix.de>
Date: Mon, 5 Dec 2011 22:18:38 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Dong Aisheng <b29396@...escale.com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linus.walleij@...ricsson.com,
shawn.guo@...escale.com, kernel@...gutronix.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:
> On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@...escale.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>
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.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists