lkml.org 
[lkml]   [2011]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
    Date
    > -----Original Message-----
    > From: Guo Shawn-R65073
    > Sent: Tuesday, December 06, 2011 2:58 PM
    > To: Dong Aisheng-B29396
    > Cc: Sascha Hauer; Linus Walleij; linux-kernel@vger.kernel.org; linux-arm-
    > kernel@lists.infradead.org; linus.walleij@stericsson.com; Guo Shawn-
    > R65073; kernel@pengutronix.de
    > Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
    >
    > On Tue, Dec 06, 2011 at 01:54:35PM +0800, Dong Aisheng-B29396 wrote:
    > > > -----Original Message-----
    > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
    > > > Sent: Tuesday, December 06, 2011 5:19 AM
    > > > To: Linus Walleij
    > > > Cc: Dong Aisheng-B29396; linux-kernel@vger.kernel.org; linux-arm-
    > > > kernel@lists.infradead.org; linus.walleij@stericsson.com; Guo Shawn-
    > > > R65073; kernel@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:
    > > > > 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>
    > > >
    > > > 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, };
    >
    > I would say, no, do not do that, because it simply does not worth.
    > Most of the definitions will probably never be used.
    >
    > IMO, we can just focus on the support for device tree case (imx6) for now.
    > With proper DT binding for pinctrl settled, all these data can go into DT.
    > For those non-DT cases, we may want to leave them as they are for now.
    >

    Can current pinctrl framework support DT well?
    Linus,
    Can you help answer it or if you have a plan on DT support if it's still not ready?

    I was ever thought it might not support DT that why I changed to run the
    driver for imx53 first.

    >
    > > 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-12-06 08:23    [W:3.084 / U:1.272 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site