Messages in this thread Patch in this message | | | Date | Sat, 08 Jan 2022 11:06:42 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 4/4] irqchip/meson-gpio: Add support for meson s4 SoCs |
| |
On Sat, 08 Jan 2022 08:42:18 +0000, Qianggui Song <qianggui.song@amlogic.com> wrote: > > The meson s4 SoCs support 12 gpio irq lines compared with previous > serial chips and have something different, details are as below. > > IRQ Number: > - 80:68 13 pins on bank Z > - 67:48 20 pins on bank X > - 47:36 12 pins on bank H > - 35:24 12 pins on bank D > - 23:22 2 pins on bank E > - 21:14 8 pins on bank C > - 13:0 13 pins on bank B > > - PADCTRL_GPIO_IRQ_CTRL0 > bit[31]: enable/disable the whole irq lines
s/the whole/all the/
> bit[12-23]: single edge trigger > bit[0-11]: poll trigger > > - PADCTRL_GPIO_IRQ_CTRL[X] > - bit[0-16]: 7 bits to chooge gpio source for irq line 2*[X] - 2
choose?
> - bit[16-22]:7 bits to chooge gpio source for irq line 2*[X] - 1 > where X = 1-6 > > - PADCTRL_GPIO_IRQ_CTRL[7] > bit[0-11]: both edge trigger
This information would fit better in the code than in the commit message.
> > Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> > --- > drivers/irqchip/irq-meson-gpio.c | 51 ++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c > index 98419428fcbd..c5d20a866c37 100644 > --- a/drivers/irqchip/irq-meson-gpio.c > +++ b/drivers/irqchip/irq-meson-gpio.c > @@ -42,6 +42,11 @@ > #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) > #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) > > +/* use for s4 chips */
s/use/Used/
> +#define REG_EDGE_POL_S4 0x1c > +#define REG_EDGE_POL_MASK_S4(x) \ > + ({typeof(x) _x = (x); BIT(_x) | BIT(12 + (_x)); })
Why on Earth should this macro handle multiple types?
> + > struct meson_gpio_irq_controller; > static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, > unsigned int channel, unsigned long hwirq); > @@ -50,6 +55,9 @@ static void meson_a1_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, > unsigned int channel, > unsigned long hwirq); > static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl); > +static unsigned int > +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl, > + unsigned int idx, u32 val); > > struct irq_ctl_ops { > void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, > @@ -96,6 +104,17 @@ struct meson_gpio_irq_params { > .pin_sel_mask = 0x7f, \ > .channel_num = 8, \ > > +#define INIT_MESON_S4_COMMON_DATA(irqs) \ > + INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init, \ > + meson_a1_gpio_irq_sel_pin, \ > + meson_s4_gpio_irq_sel_type) \ > + .support_edge_both = true, \ > + .edge_both_offset = 0, \ > + .edge_single_offset = 12, \ > + .pol_low_offset = 0, \ > + .pin_sel_mask = 0xff, \ > + .channel_num = 12, \ > + > static const struct meson_gpio_irq_params meson8_params = { > INIT_MESON8_COMMON_DATA(134) > }; > @@ -126,6 +145,10 @@ static const struct meson_gpio_irq_params a1_params = { > INIT_MESON_A1_COMMON_DATA(62) > }; > > +static const struct meson_gpio_irq_params s4_params = { > + INIT_MESON_S4_COMMON_DATA(82) > +}; > + > static const struct of_device_id meson_irq_gpio_matches[] = { > { .compatible = "amlogic,meson8-gpio-intc", .data = &meson8_params }, > { .compatible = "amlogic,meson8b-gpio-intc", .data = &meson8b_params }, > @@ -135,6 +158,7 @@ static const struct of_device_id meson_irq_gpio_matches[] = { > { .compatible = "amlogic,meson-g12a-gpio-intc", .data = &axg_params }, > { .compatible = "amlogic,meson-sm1-gpio-intc", .data = &sm1_params }, > { .compatible = "amlogic,meson-a1-gpio-intc", .data = &a1_params }, > + { .compatible = "amlogic,meson-s4-gpio-intc", .data = &s4_params }, > { } > }; > > @@ -202,6 +226,33 @@ static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl) > meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, BIT(31), BIT(31)); > } > > +static unsigned int > +meson_s4_gpio_irq_sel_type(struct meson_gpio_irq_controller *ctl, > + unsigned int idx, unsigned int type)
An 'unsigned int' return type, directly returned by a caller that has 'int' as its return type. What could possibly go wrong?
> +{ > + unsigned int val = 0; > + > + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4, BIT(0 + (idx)), 0);
Drop the 0 + as well as the useless bracketing all over this function.
> + > + if (type == IRQ_TYPE_EDGE_BOTH) { > + val |= BIT(ctl->params->edge_both_offset + (idx)); > + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL_S4, > + BIT(ctl->params->edge_both_offset + (idx)), val); > + return 0; > + } > + > + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) > + val |= BIT(ctl->params->pol_low_offset + (idx)); > + > + if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) > + val |= BIT(ctl->params->edge_single_offset + (idx)); > + > + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, > + REG_EDGE_POL_MASK_S4(idx), val); > + > + return 0; > +}; > + > static int > meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl, > unsigned long hwirq,
M.
-- Without deviation from the norm, progress is not possible.
| |