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.
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?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
