Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs
From: Marc Zyngier
Date: Wed Dec 11 2019 - 12:26:24 EST
On 2019-12-10 02:08, Qianggui Song wrote:
Hi, Marc
Thank you for your review
On 2019/12/6 21:13, Marc Zyngier wrote:
On 2019-12-06 12:17, Qianggui Song wrote:
Since Meson-A1 Socs register layout of gpio interrupt controller
have
difference with previous chips, registers to decide irq line and
offset
of trigger method are all changed, the current driver should be
modified.
Signed-off-by: Qianggui Song <qianggui.song@xxxxxxxxxxx>
---
drivers/irqchip/irq-meson-gpio.c | 79
++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 19 deletions(-)
diff --git a/drivers/irqchip/irq-meson-gpio.c
b/drivers/irqchip/irq-meson-gpio.c
index 829084b568fa..1824ffc30de2 100644
--- a/drivers/irqchip/irq-meson-gpio.c
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -30,44 +30,74 @@
* stuck at 0. Bits 8 to 15 are responsive and have the expected
* effect.
*/
-#define REG_EDGE_POL_EDGE(x) BIT(x)
-#define REG_EDGE_POL_LOW(x) BIT(16 + (x))
-#define REG_BOTH_EDGE(x) BIT(8 + (x))
-#define REG_EDGE_POL_MASK(x) ( \
- REG_EDGE_POL_EDGE(x) | \
- REG_EDGE_POL_LOW(x) | \
- REG_BOTH_EDGE(x))
+#define REG_EDGE_POL_EDGE(params,
x) BIT((params)->edge_single_offset + (x))
+#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset +
(x))
+#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset +
(x))
+#define REG_EDGE_POL_MASK(params, x) ( \
+ REG_EDGE_POL_EDGE(params, x) | \
+ REG_EDGE_POL_LOW(params, x) | \
+ REG_BOTH_EDGE(params, x))
#define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8)
#define REG_FILTER_SEL_SHIFT(x) ((x) * 4)
+#define INIT_MESON8_COMMON_DATA \
+ .edge_single_offset = 0, \
+ .pol_low_offset = 16, \
+ .pin_sel_mask = 0xff, \
+ .ops = { \
+ .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \
+ },
Please place the #defines that operate on the various data
structures
*after* the definition of the structures. It would greatly help
reading the changes.
OK, will place it below the definition of struct
meson_gpio_irq_params
in the next patch.
+
+struct meson_gpio_irq_controller;
+static void meson8_gpio_irq_sel_pin(struct
meson_gpio_irq_controller
*ctl,
+ unsigned int channel, unsigned long hwirq);
+struct irq_ctl_ops {
+ void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl,
+ unsigned int channel,
+ unsigned long hwirq);
+ void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl);
+};
+
struct meson_gpio_irq_params {
unsigned int nr_hwirq;
bool support_edge_both;
+ unsigned int edge_both_offset;
+ unsigned int edge_single_offset;
+ unsigned int pol_low_offset;
+ unsigned int pin_sel_mask;
+ struct irq_ctl_ops ops;
};
static const struct meson_gpio_irq_params meson8_params = {
.nr_hwirq = 134,
+ INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params meson8b_params = {
.nr_hwirq = 119,
+ INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params gxbb_params = {
.nr_hwirq = 133,
+ INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params gxl_params = {
.nr_hwirq = 110,
+ INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params axg_params = {
.nr_hwirq = 100,
+ INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params sm1_params = {
.nr_hwirq = 100,
.support_edge_both = true,
+ .edge_both_offset = 8,
+ INIT_MESON8_COMMON_DATA
};
OK, this isn't great. The least you could do is to make
your initializer parametric, so that it takes the nr_hwirq as
a parameter.
Then, any additional member that overrides common behaviour
should come after the main initializer.
Also, do you need 'support_edge_both'? Isn't a non-zero
'edge_both_offset' enough to detect the feature?
Sorry, but I am not very clear that "make your initializer
parametric,
so that it takes the nr_hwirq as a parameter". Is that
initializer(initial function in .ops ? ) as a parameter of struct
meson_gpio_irq_params ? If nr_hwirq as a parameter of init function
of
.ops then will make lot of init function for each platform.
How about move .ops from macro like below:
#define INIT_MESON8_COMMON_DATA \
.edge_single_offset = 0, \
.pol_low_offset = 16, \
.pin_sel_mask = 0xff,
static const struct meson_gpio_irq_params sm1_params = {
.nr_hwirq = 100,//main initializer
.ops = {
.gpio_irq_sel_pin = meson8_gpio_irq_sel_pin,
/*in below to assign support_edge_both
* edge_both_offset
* call after main initializer to additional
* member
*/
.gpio_irq_init = meson_sm1_irq_init,
},
INIT_MESON8_COMMON_DATA// m8 to sm1 are the same.
};
No, what I'm suggesting is something like this:
diff --git a/drivers/irqchip/irq-meson-gpio.c
b/drivers/irqchip/irq-meson-gpio.c
index 8478100706a6..27a3207a944d 100644
--- a/drivers/irqchip/irq-meson-gpio.c
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -43,24 +43,27 @@
/* Below is used for Meson-A1 series like chips*/
#define REG_PIN_A1_SEL 0x04
-#define INIT_MESON8_COMMON_DATA \
- .edge_single_offset = 0, \
- .pol_low_offset = 16, \
- .pin_sel_mask = 0xff, \
- .ops = { \
- .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \
- },
-
-#define INIT_MESON_A1_COMMON_DATA \
- .support_edge_both = true, \
- .edge_both_offset = 16, \
- .edge_single_offset = 8, \
- .pol_low_offset = 0, \
- .pin_sel_mask = 0x7f, \
- .ops = { \
- .gpio_irq_sel_pin = meson_a1_gpio_irq_sel_pin, \
- .gpio_irq_init = meson_a1_gpio_irq_init, \
- },
+#define INIT_MESON_COMMON(irqs, init, sel) \
+ .nr_hwirq = irqs, \
+ .ops = { \
+ .gpio_irq_sel_pin = sel, \
+ .gpio_irq_init = init, \
+ }
+
+#define INIT_MESON8_COMMON_DATA(irqs) \
+ INIT_MESON_COMMON(irqs, NULL, \
+ meson8_gpio_irq_sel_pin), \
+ .pol_low_offset = 16, \
+ .pin_sel_mask = 0xff,
+
+#define INIT_MESON_A1_COMMON_DATA(irqs) \
+ INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init, \
+ meson_a1_gpio_irq_sel_pin), \
+ .support_edge_both = true, \
+ .edge_both_offset = 16, \
+ .edge_single_offset = 8, \
+ .pol_low_offset = 0, \
+ .pin_sel_mask = 0x7f,
struct meson_gpio_irq_controller;
static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller
*ctl,
@@ -89,40 +92,33 @@ struct meson_gpio_irq_params {
};
static const struct meson_gpio_irq_params meson8_params = {
- .nr_hwirq = 134,
- INIT_MESON8_COMMON_DATA
+ INIT_MESON8_COMMON_DATA(134),
};
static const struct meson_gpio_irq_params meson8b_params = {
- .nr_hwirq = 119,
- INIT_MESON8_COMMON_DATA
+ INIT_MESON8_COMMON_DATA(119),
};
static const struct meson_gpio_irq_params gxbb_params = {
- .nr_hwirq = 133,
- INIT_MESON8_COMMON_DATA
+ INIT_MESON8_COMMON_DATA(133),
};
static const struct meson_gpio_irq_params gxl_params = {
- .nr_hwirq = 110,
- INIT_MESON8_COMMON_DATA
+ INIT_MESON8_COMMON_DATA(110),
};
static const struct meson_gpio_irq_params axg_params = {
- .nr_hwirq = 100,
- INIT_MESON8_COMMON_DATA
+ INIT_MESON8_COMMON_DATA(100),
};
static const struct meson_gpio_irq_params sm1_params = {
- .nr_hwirq = 100,
+ INIT_MESON8_COMMON_DATA(100),
.support_edge_both = true,
.edge_both_offset = 8,
- INIT_MESON8_COMMON_DATA
};
static const struct meson_gpio_irq_params a1_params = {
- .nr_hwirq = 62,
- INIT_MESON_A1_COMMON_DATA
+ INIT_MESON_A1_COMMON_DATA(62),
};
static const struct of_device_id meson_irq_gpio_matches[] = {
Thanks,
M.
--
Jazz is not dead. It just smells funny...