[U-Boot] [PATCH 05/11] ARM: tegra: pinctrl: remove duplication

[Stephen Warren]

On 03/20/2014 07:25 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On 20 March 2014 12:57, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>
>> On 03/14/2014 01:37 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On 13 March 2014 11:42, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>
>>>> Much of arch/arm/cpu/tegra*-common/pinmux.c is identical. Remove the
>>>> duplication by creating pinmux-common.c for all the identical code.
>>>>
>>>> This leaves:
>>>> * arch/arm/include/asm/arch-tegra*/pinmux.h defining only the names of
>>>>   the various pins/pin groups, drive groups, and mux functions.
>>>> * arch/arm/cpu/tegra*-common/pinmux.c containing only the lookup table
>>>>   stating which pin groups support which mux functions.
>>>>
>>>> The code in pinmux-common.c is semantically identical to that in the
>>>> various original pinmux.c, but had some consistency and cleanup fixes
>>>> applied during migration.

>>>> diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c
>>
>>>> +/* return 1 if a pin_pupd_is in range */
>>>> +#define pmux_pin_pupd_isvalid(pupd) \
>>>> +       (((pupd) >= PMUX_PULL_NORMAL) && ((pupd) <= PMUX_PULL_UP))
>>>> +
>>>> +/* return 1 if a pin_tristate_is in range */
>>>> +#define pmux_pin_tristate_isvalid(tristate) \
>>>> +       (((tristate) >= PMUX_TRI_NORMAL) && ((tristate) <= PMUX_TRI_TRISTATE))
>>>> +
>>>> +#ifdef TEGRA_PMX_HAS_PIN_IO_BIT_ETC
>>>
>>> Do we need this #Ifdef?
>>>
>>>> +/* return 1 if a pin_io_is in range */
>>>> +#define pmux_pin_io_isvalid(io) \
>>>> +       (((io) >= PMUX_PIN_OUTPUT) && ((io) <= PMUX_PIN_INPUT))
>>
>> We certainly need not to compile this code, since e.g. PMUX_PIN_INPUT
>> doesn't exist on Tegra20 due to equivalent #ifdefs in pinmux.h.
>>
>> I do explicitly want to keep the ifdefs in pinmux.h, so that APIs are
>> not prototyped, and values not defined, for features that don't exist on
>> the SoC that U-Boot is being built for. This validates at compile time
>> that code isn't using invalid APIs. While pinmux.h could be split up
>> into a few separate header files to avoid the ifdefs, I think that would
>> make the header situation far more complex than it needs to be.
> 
> Arguably you have created this problem by having #ifdefs in the C file
> - if there was a separate file for each SoC then it would be much
> harder to mess this up.

Well, then you get a link error rather than a compiler error for an
unprototyped function or undefined enum/#define. I think the compile
error is a bit more obvious myself, but granted either would work.

...
>> So in summary, I'd like to keep the ifdefs. I think they're pretty
>> simple and not a maintenance burden. Do you object?
> 
> No. I can't possibly object given that you have completed such a great clean-up.

Great, thanks. I'll post V2 tomorrow.