[U-Boot] [PATCH 15/60] gpio: tegra: header file split

[Simon Glass]

Hi Stephen,

On 21 April 2016 at 10:40, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/21/2016 08:11 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 20 April 2016 at 16:01, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 04/20/2016 01:26 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 19 April 2016 at 14:58, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>
>>>>> Tegra's gpio.h contains a mix of private definitions for use inside the
>>>>> GPIO driver and custom machine-specific APIs. Move the private
>>>>> definitions
>>>>> out of the global include directory since nothing should need to access
>>>>> them. Move the public definitions to the machine-specific include
>>>>> directory <mach/>.
>>>
>>>
>>>
>>>>> diff --git a/drivers/gpio/tegra_gpio_priv.h
>>>>> b/drivers/gpio/tegra_gpio_priv.h
>>>
>>>
>>>
>>>>> +/*
>>>>> + * GPIO registers are split into two chunks; low and high.
>>>>> + * On Tegra20, all low chunks appear first, then all high chunks.
>>>>> + * In later SoCs, the low and high chunks are interleaved together.
>>>>> + */
>>>>> +#define GPIO_CTLR_BANK_HIGH_REGS \
>>>>> +       uint gpio_masked_config[TEGRA_GPIO_PORTS]; \
>>>>> +       uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \
>>>>> +       uint gpio_masked_out[TEGRA_GPIO_PORTS]; \
>>>>> +       uint reserved0[TEGRA_GPIO_PORTS]; \
>>>>> +       uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \
>>>>> +       uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \
>>>>> +       uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \
>>>>> +       uint reserved1[TEGRA_GPIO_PORTS];
>>>>> +
>>>>> +/* GPIO Controller registers for a single bank */
>>>>> +struct gpio_ctlr_bank {
>>>>> +       uint gpio_config[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_dir_out[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_out[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_in[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_int_status[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_int_enable[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_int_level[TEGRA_GPIO_PORTS];
>>>>> +       uint gpio_int_clear[TEGRA_GPIO_PORTS];
>>>>> +#ifndef CONFIG_TEGRA20
>>>>> +       GPIO_CTLR_BANK_HIGH_REGS
>>>>> +#endif
>>>>> +};
>>>>> +
>>>>> +#ifdef CONFIG_TEGRA20
>>>>> +struct gpio_ctlr_bank_high {
>>>>> +       GPIO_CTLR_BANK_HIGH_REGS
>>>>
>>>>
>>>>
>>>> This seems a bit ugly. Perhaps you could havestruct
>>>> gpio_ctlr_high_regs and include that here? It adds a level of
>>>> indirection but that doesn't seem very important.
>>>
>>>
>>>
>>> In newer Tegras, there's no differentiation between the two register sets
>>> that were "low" and "high" in Tegra20. I'd rather not saddle the
>>> non-Tegra20
>>> struct layouts with some odd naming/nesting just because the Tegra20
>>> layout
>>> was odd. I don't see any problem with using a #define for this; it
>>> doesn't
>>> seem to make the code complex.
>>
>>
>> OK, well then how about just duplicating the two structs, and dropping
>> the #define?
>>
>> #ifdfef CONFIG_TEGRA20
>> struct gpio_ctlr_bank {
>>
>> };
>> #else
>> struct gpio_ctlr_bank {
>> };
>> #endif
>
>
> Given that the driver doesn't use any registers in the high bank, and indeed
> can't; the driver's reliance on structs rather than register defines means
> that the high bank registers would have to be accessed differently between
> Tegra20 and other SoCs, I propose simply not representing those registers.
> Instead, how about:
>
> struct gpio_ctlr_bank {
>         uint gpio_config[TEGRA_GPIO_PORTS];
>         uint gpio_dir_out[TEGRA_GPIO_PORTS];
>         uint gpio_out[TEGRA_GPIO_PORTS];
>         uint gpio_in[TEGRA_GPIO_PORTS];
>         uint gpio_int_status[TEGRA_GPIO_PORTS];
>         uint gpio_int_enable[TEGRA_GPIO_PORTS];
>         uint gpio_int_level[TEGRA_GPIO_PORTS];
>         uint gpio_int_clear[TEGRA_GPIO_PORTS];
> #ifndef CONFIG_TEGRA20
>         uint unused[TEGRA_GPIO_PORTS * 8];
> #endif
> };
>
> struct gpio_ctlr {
>         struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS];
> };
>
> That removes all the duplication, without any macros etc.
>
> Does that look reasonable? If I fixup the patch like that, I'll add a brief
> comment describing what the unused registers are, similar to the one in
> patch v1.

SGTM

>
>>> I wonder if we should just convert away from structs for registers
>>> entirely.
>>> Everything in the HW docs is just numbers, matching those to the structs
>>> is
>>> always painful, and if we used #defines instead of structs, representing
>>> this HW difference would end up being much cleaner and avoid using a
>>> macro
>>> to "cut/paste" a register list 2 times; see the way the Linux kernel
>>> driver
>>> handles this.
>>
>>
>> Structs are definitely easier to read and particularly in this case
>> where each struct element is an array.
>
>
> I'm not really sure there's much objective difference between the
> readability of the two. Arrays can easily be abstracted via a macro or
> inline function so that the call site looks essentially identical; () vs [].

IMO the code is much harder to follow when you need to look up macros,
etc. C already supports arrays :-)

>
>> Why are you worried about code duplication in a header file?
>
>
> I'm not sure why I would special case my concerns and ignore duplication in
> certain locations yet still care about duplication in general or elsewhere?

We commonly put ugliness in header files. So long as the resulting
syntax (in C files) is pretty obvious and non-surprising, this makes
sense. Most of the time these header files are ignored by humans when
reading the code since it is pretty obvious from the C code what is
going on.

Examples include static inline to drop functions, hardware register
definitions, bitfield definitions, #ifdef setup (see image.h), etc.

Perhaps by that argument your original #define scheme is fine. I don't
like things that make it hard to grep the code for stuff, but this is
minor. So I'm going to withdraw my objection (sorry).

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon