lkml.org 
[lkml]   [2019]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs
Date
From
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...

\
 
 \ /
  Last update: 2019-12-06 14:14    [W:0.072 / U:0.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site