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

[Simon Glass]

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

> 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.

Why are you worried about code duplication in a header file?

Regards,
Simon