[PATCH 1/7] pinctrl: enable pxa3xx pinmux

[Haojian Zhuang]

>-----Original Message-----
>From: Linus Walleij [mailto:linus.walleij at linaro.org]
>Sent: 2011年11月28日 5:08 PM
>To: Haojian Zhuang
>Cc: linux-arm-kernel at lists.infradead.org; swarren at nvidia.com;
>linux at arm.linux.org.uk; eric.y.miao at gmail.com; grant.likely at secretlab.ca;
>arnd at arndb.de
>Subject: Re: [PATCH 1/7] pinctrl: enable pxa3xx pinmux
>
>On Sat, Nov 26, 2011 at 12:08 AM, Haojian Zhuang
><haojian.zhuang at marvell.com> wrote:
>
>> Support PXA300/PXA310/PXA320/PXA910 pinmux.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang at marvell.com>
>
>Cool!
>
>> +#ifdef CONFIG_ARCH_PXA
>> +static struct pinctrl_pin_desc pxa300_pads[] = {
>(...)
>
>Ick, per-arch #ifdefs in the driver/ directory is not good. Can you
>arrange
>for this to be #ifdef:ed up in your board file and passed in as platform
>data instead of being defined here?
>
>I know that the U300 driver defines all it's pins in the driver file,
>but it only supports one single board on one single chipset and
>is thus pretty fixed, nominally this kind of stuff should come from
>the board file(s) or the device tree.
>
>> +static int pxa3xx_get_gpio_func(unsigned int cpuid, unsigned int gpio)
>> +{
>> +       int ret = 0;
>> +
>> +       switch (cpuid) {
>> +#ifdef CONFIG_ARCH_PXA
>> +       case PINMUX_PXA300:
>> +       case PINMUX_PXA310:
>> +               if (gpio == 50)
>> +                       ret = 2;
>> +               else if (gpio == 49 || gpio == 51 || gpio == 53)
>> +                       ret = 3;
>> +               break;
>> +       case PINMUX_PXA320:
>> +               if (gpio == 56 || (gpio > 58 && gpio < 63))
>> +                       goto out;
>> +               break;
>> +#endif
>> +#ifdef CONFIG_ARCH_MMP
>> +       case PINMUX_PXA910:
>> +               if ((gpio > 116 && gpio < 121) || gpio == 122 || gpio
>== 123 ||
>> +                       gpio == 125 || gpio == 99 || gpio == 58 ||
>gpio == 59)
>> +                       ret = 1;
>> +               break;
>> +#endif
>
>Do you need these #ifdef:s?

I can remove this #ifdef.

>
>They need to go, one way or another.
>
>Switching on cpuid is excellent, provided it's passed in from
>the outside, the definitons of these cases should be available
>anyway should they not.
>
>#ifdef:in out for footprint or unused code paths on some platforms
>should *not* be done, it creates more trouble than it solves, see
>Documentation/SubmittingPatches section 2) #ifdefs are ugly.
>
>> +static int pxa3xx_get_mfpr(unsigned int cpuid, unsigned int pin)
>> +{
>> +       int ret;
>> +       switch (cpuid) {
>> +#ifdef CONFIG_ARCH_PXA
>
>Dito. Etc.
I can remove this too.

>
>> +#ifdef CONFIG_ARCH_MMP
>> +static struct pinctrl_desc pxa910_pctrl_desc = {
>> +       .name           = "pxa910-pinctrl",
>> +       .pins           = pxa910_pads,
>> +       .npins          = ARRAY_SIZE(pxa910_pads),
>> +       .maxpin         = 260,
>> +       .pctlops        = &pxa3xx_pctrl_ops,
>> +       .pmxops         = &pxa3xx_pmx_ops,
>> +       .owner          = THIS_MODULE,
>> +};
>
>This is not good either. Get rid of all #ifdef
I can remove this too.
>
>> +#ifdef CONFIG_ARCH_PXA
>> +       if (cpu_is_pxa300()) {
>> +               info->cpuid = PINMUX_PXA300;
>> +               info->grp = pxa300_pin_groups;
>> +               info->func = pxa300_pmx_functions;
>> +               info->num_grps = ARRAY_SIZE(pxa300_pin_groups);
>> +               info->num_funcs = ARRAY_SIZE(pxa300_pmx_functions);
>> +               info->pctl = pinctrl_register(&pxa300_pctrl_desc,
>&pdev->dev,
>> +                               info);
>> +               range = pxa300_gpio_ranges;
>> +               range_num = ARRAY_SIZE(pxa300_gpio_ranges);
>> +       } else if (cpu_is_pxa310()) {
>> +               info->cpuid = PINMUX_PXA310;
>> +               info->grp = pxa310_pin_groups;
>> +               info->func = pxa310_pmx_functions;
>> +               info->num_grps = ARRAY_SIZE(pxa310_pin_groups);
>> +               info->num_funcs = ARRAY_SIZE(pxa310_pmx_functions);
>> +               info->pctl = pinctrl_register(&pxa310_pctrl_desc,
>&pdev->dev,
>> +                               info);
>> +               range = pxa310_gpio_ranges;
>> +               range_num = ARRAY_SIZE(pxa310_gpio_ranges);
>> +       } else if (cpu_is_pxa320()) {
>> +               info->cpuid = PINMUX_PXA320;
>> +               info->grp = pxa320_pin_groups;
>> +               info->func = pxa320_pmx_functions;
>> +               info->num_grps = ARRAY_SIZE(pxa320_pin_groups);
>> +               info->num_funcs = ARRAY_SIZE(pxa320_pmx_functions);
>> +               info->pctl = pinctrl_register(&pxa320_pctrl_desc,
>&pdev->dev,
>> +                               info);
>> +               range = pxa320_gpio_ranges;
>> +               range_num = ARRAY_SIZE(pxa320_gpio_ranges);
>> +       }
>> +#endif
>> +#ifdef CONFIG_ARCH_MMP
>> +       if (cpu_is_pxa910()) {
>> +               info->cpuid = PINMUX_PXA910;
>> +               info->grp = pxa910_pin_groups;
>> +               info->func = pxa910_pmx_functions;
>> +               info->num_grps = ARRAY_SIZE(pxa910_pin_groups);
>> +               info->num_funcs = ARRAY_SIZE(pxa910_pmx_functions);
>> +               info->pctl = pinctrl_register(&pxa910_pctrl_desc,
>&pdev->dev,
>> +                               info);
>> +               range = pxa910_gpio_ranges;
>> +               range_num = ARRAY_SIZE(pxa910_gpio_ranges);
>> +       }
>> +#endif
>
>Same here.
>
>But *apart* from the #ifdef stuff it looks very good!
>So if you can get rid of that, I'll be very happy!
The only block issue at here is that cpu_is_pxa3xx() is defined in arch-pxa and cpu_is_pxa910() is defined in arch-mmp. If I only enable arch-pxa, I would meet build error for undefined cpu_is_pxa910().

>
>Yours,
>Linus Walleij