Messages in this thread | | | Date | Tue, 6 Dec 2011 11:39:51 +0800 | Subject | Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support | From | Dong Aisheng <> |
| |
Hi Linus,
First thanks for your review.
2011/12/6 Linus Walleij <linus.walleij@linaro.org>: > 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. > */ > Indeed. Thanks for your info.
>> +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. > It's the mux mode for each pins in that group for a specific function(here it's fec). I add the comments in pinmux-imx-core.h when define this group structure. Maybe i still need to add some comments here to avoid confusing.
>> +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? > >> +static const unsigned mx53_uart1_pins[] = { >> + MX53_CSI0_DAT10, >> + MX53_CSI0_DAT11, >> +}; >> +static const unsigned mx53_uart1_mux[] = { 2, 2 }; > > And here again? > >> +static const struct imx_pin_group mx53_pin_groups[] = { >> + IMX_PIN_GROUP("fecgrp", mx53_fec_pins, mx53_fec_mux), >> + IMX_PIN_GROUP("sd1grp", mx53_sd1_pins, mx53_sd1_mux), >> + IMX_PIN_GROUP("sd3grp", mx53_sd3_pins, mx53_sd3_mux), >> + IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux), >> +}; > > So I understand the first and second argument to IMX_PIN_GROUP() > but not the third. > >> +/* mx53 funcs and groups */ >> +static const char * const fecgrps[] = { "fecgrp" }; >> +static const char * const sd1grps[] = { "sd1grp" }; >> +static const char * const sd3grps[] = { "sd3grp" }; >> +static const char * const uart1grps[] = { "uart1grp" }; >> + >> +static const struct imx_pmx_func mx53_pmx_functions[] = { >> + IMX_PMX_FUNC("fec", fecgrps), >> + IMX_PMX_FUNC("sd1", sd1grps), >> + IMX_PMX_FUNC("sd3", sd3grps), >> + IMX_PMX_FUNC("uart1", uart1grps), >> +}; > > This looks good. >
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/
| |