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 | From | Qianggui Song <> | Date | Tue, 10 Dec 2019 10:08:04 +0800 |
| |
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. };
About support_edge_both support_edge_both & edge_both_offset are used for sm1 or later chip, but the value are different with A1 which this patch set support.For SM1 is 8, but A1 is 16, so I am not one hundred percent sure that later chip design will change to edge_both_offet to zero to set both edge trigger. Let' s see edge_single_offset, it is set to 0 in A1 from 16 which is previous chip use.I think support_edge_bot should be kept.
>> >> static const struct of_device_id meson_irq_gpio_matches[] = { >> @@ -100,9 +130,18 @@ static void meson_gpio_irq_update_bits(struct >> meson_gpio_irq_controller *ctl, >> writel_relaxed(tmp, ctl->base + reg); >> } >> >> -static unsigned int meson_gpio_irq_channel_to_reg(unsigned int >> channel) >> +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller >> *ctl, >> + unsigned int channel, unsigned long hwirq) >> { >> - return (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; >> + unsigned int reg_offset; >> + unsigned int bit_offset; >> + >> + reg_offset = (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; >> + bit_offset = REG_PIN_SEL_SHIFT(channel); >> + >> + meson_gpio_irq_update_bits(ctl, reg_offset, >> + ctl->params->pin_sel_mask << bit_offset, >> + hwirq << bit_offset); >> } >> >> static int >> @@ -110,7 +149,7 @@ meson_gpio_irq_request_channel(struct >> meson_gpio_irq_controller *ctl, >> unsigned long hwirq, >> u32 **channel_hwirq) >> { >> - unsigned int reg, idx; >> + unsigned int idx; >> >> spin_lock(&ctl->lock); >> >> @@ -129,10 +168,7 @@ meson_gpio_irq_request_channel(struct >> meson_gpio_irq_controller *ctl, >> * Setup the mux of the channel to route the signal of the pad >> * to the appropriate input of the GIC >> */ >> - reg = meson_gpio_irq_channel_to_reg(idx); >> - meson_gpio_irq_update_bits(ctl, reg, >> - 0xff << REG_PIN_SEL_SHIFT(idx), >> - hwirq << REG_PIN_SEL_SHIFT(idx)); >> + ctl->params->ops.gpio_irq_sel_pin(ctl, idx, hwirq); >> >> /* >> * Get the hwirq number assigned to this channel through >> @@ -173,7 +209,9 @@ static int meson_gpio_irq_type_setup(struct >> meson_gpio_irq_controller *ctl, >> { >> u32 val = 0; >> unsigned int idx; >> + const struct meson_gpio_irq_params *params; >> >> + params = ctl->params; >> idx = meson_gpio_irq_get_channel_idx(ctl, channel_hwirq); >> >> /* >> @@ -190,22 +228,22 @@ static int meson_gpio_irq_type_setup(struct >> meson_gpio_irq_controller *ctl, >> * precedence over the other edge/polarity settings >> */ >> if (type == IRQ_TYPE_EDGE_BOTH) { >> - if (!ctl->params->support_edge_both) >> + if (!params->support_edge_both) >> return -EINVAL; >> >> - val |= REG_BOTH_EDGE(idx); >> + val |= REG_BOTH_EDGE(params, idx); >> } else { >> if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) >> - val |= REG_EDGE_POL_EDGE(idx); >> + val |= REG_EDGE_POL_EDGE(params, idx); >> >> if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) >> - val |= REG_EDGE_POL_LOW(idx); >> + val |= REG_EDGE_POL_LOW(params, idx); >> } >> >> spin_lock(&ctl->lock); >> >> meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, >> - REG_EDGE_POL_MASK(idx), val); >> + REG_EDGE_POL_MASK(params, idx), val); >> >> spin_unlock(&ctl->lock); >> >> @@ -371,6 +409,9 @@ static int __init meson_gpio_irq_parse_dt(struct >> device_node *node, >> return ret; >> } >> >> + if (ctl->params->ops.gpio_irq_init) >> + ctl->params->ops.gpio_irq_init(ctl); > > It would make sense to provide a dummy init() method, since > you have all the infrastructure already. > Okay >> + >> return 0; >> } > > Thanks, > > M. >
| |