| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Dec 2011 05:54:35 +0000
From: Dong Aisheng-B29396 <B29396@...escale.com>
To: Sascha Hauer <s.hauer@...gutronix.de>,
Linus Walleij <linus.walleij@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linus.walleij@...ricsson.com" <linus.walleij@...ricsson.com>,
Guo Shawn-R65073 <r65073@...escale.com>,
"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: RE: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@...gutronix.de]
> Sent: Tuesday, December 06, 2011 5:19 AM
> To: Linus Walleij
> Cc: Dong Aisheng-B29396; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; linus.walleij@...ricsson.com; Guo Shawn-
> R65073; 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.
>
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
--
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