From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Thu, 21 Apr 2016 10:40:34 -0600 Subject: [U-Boot] [PATCH 15/60] gpio: tegra: header file split In-Reply-To: References: <1461099580-3866-1-git-send-email-swarren@wwwdotorg.org> <1461099580-3866-16-git-send-email-swarren@wwwdotorg.org> <5717FC24.8070707@wwwdotorg.org> Message-ID: <57190282.5020501@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/21/2016 08:11 AM, Simon Glass wrote: > Hi Stephen, > > On 20 April 2016 at 16:01, Stephen Warren wrote: >> On 04/20/2016 01:26 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 19 April 2016 at 14:58, Stephen Warren wrote: >>>> >>>> From: Stephen Warren >>>> >>>> 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 . >> >> >>>> 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?