Hi Stephen, On 21 April 2016 at 10:40, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 04/21/2016 08:11 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 20 April 2016 at 16:01, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 04/20/2016 01:26 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 19 April 2016 at 14:58, Stephen Warren <swar...@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> From: Stephen Warren <swar...@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 <s...@chromium.org> Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot