Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs | Date | Wed, 11 Dec 2019 17:26:18 +0000 | From | Marc Zyngier <> |
| |
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@amlogic.com> >>> --- >>> 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...
| |