linux-kernel - Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA+hA=Sr0rSAQ54G_L-hxEutadAi3SzWSJ59jK+wzmXw+iBs7w@mail.gmail.com>
Date:	Tue, 6 Dec 2011 11:39:51 +0800
From:	Dong Aisheng <dongas86@...il.com>
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,
	s.hauer@...gutronix.de, shawn.guo@...escale.com,
	kernel@...gutronix.de
Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

Hi Linus,

First  thanks for your review.

2011/12/6 Linus Walleij <linus.walleij@...aro.org>:
> 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.
>  */
>
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/