Messages in this thread | | | Subject | Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs | Date | Fri, 06 Dec 2019 13:13:50 +0000 | From | Marc Zyngier <> |
| |
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.
> + > +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?
> > 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.
> + > return 0; > }
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |