lkml.org 
[lkml]   [2011]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
    From
    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/

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