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

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/21/2016 08:11 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 20 April 2016 at 16:01, Stephen Warren <swarren@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@wwwdotorg.org> wrote:
>>>>
>>>> From: Stephen Warren <swarren@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.

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

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