Messages in this thread | | | Subject | Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs | From | Qianggui Song <> | Date | Thu, 12 Dec 2019 19:13:16 +0800 |
| |
On 2019/12/12 1:26, Marc Zyngier wrote: > 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), > }; > Thanks, will try it in later patch > static const struct of_device_id meson_irq_gpio_matches[] = { > > > Thanks, > > M. >
| |