Re: [PATCH v2] regmap: irq: handle HW using separate rising/falling edge interrupts

[Bartosz Golaszewski]

pt., 7 gru 2018 o 14:04 Bartosz Golaszewski <brgl@xxxxxxxx> napisaÅ(a):
>
> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>
> Some interrupt controllers use separate bits for controlling rising
> and falling edge interrupts in the mask register i.e. they have one
> interrupt for rising edge and one for falling.
>
> We already handle the case where we have a single interrupt in the
> mask register and a separate type configuration register.
>
> Add a new switch to regmap_irq_chip which tells the framework to use
> the mask_base address for configuring the edge of the interrupts that
> define type_falling/rising_mask values.
>
> For such interrupts we never update the type_base bits. For interrupts
> that don't define type masks or their regmap irq chip doesn't set the
> type_in_mask to true everything stays the same.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> ---
> This is a second try at supporting HW which uses separate interrupts
> for rising/falling edge events.
>
> This time we're adding an explicit flag for such chips.
>
> Below is an example of the regmap irq chip configuration that makes
> use of this new feature:
>
> static const struct regmap_irq foobar_irqs[] = {
>         /*
>          * Make two interrupts into a single virtual irq with
>          * configurable edge.
>          */
>         [FOOBAR_INT_EDGE] = {
>                 .reg_offset             = FOOBAR_INT_GLBL_OFFSET,
>                 /* mask is two bits here but regmap handles it nicely */
>                 .mask                   = FOOBAR_INT_GPIO_MSK,
>                 .type_falling_mask      = FOOBAR_INT_GPIO_F_MSK,
>                 .type_rising_mask       = FOOBAR_INT_GPIO_R_MSK,
>         },
>         /* Regular interrupt, nothing changes. */
>         [FOOBAR_INT_NOEDGE] = {
>                 .reg_offset             = FOOBAR_INT_GLBL_OFFSET,
>                 .mask                   = FOOBAR_INT_FOO_MSK,
>         },
>
> static struct regmap_irq_chip foobar_irq_chip = {
>         .name                   = "foobar-irq",
>         .irqs                   = foobar_irqs,
>         .num_irqs               = ARRAY_SIZE(foobar_irqs),
>         .num_regs               = 2,
>         .status_base            = FOOBAR_REG_INT_GLBL,
>         .mask_base              = FOOBAR_REG_INTM_GLBL,
>         .type_in_mask           = true,
>         /* NOTE: we're not defining type_base here. */
> };
>

Hi Mark,

would you be willing to pick this one up for v4.21 or do you have some
objections? If it would go in in the next merge window then it would
make it easier to merge my driver afterwards since I could already
make use of it.

Bart

>  drivers/base/regmap/regmap-irq.c | 64 ++++++++++++++++++++++----------
>  include/linux/regmap.h           |  4 ++
>  2 files changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 429ca8ed7e51..603b1554f81c 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -157,20 +157,23 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
>                 }
>         }
>
> -       for (i = 0; i < d->chip->num_type_reg; i++) {
> -               if (!d->type_buf_def[i])
> -                       continue;
> -               reg = d->chip->type_base +
> -                       (i * map->reg_stride * d->type_reg_stride);
> -               if (d->chip->type_invert)
> -                       ret = regmap_irq_update_bits(d, reg,
> -                               d->type_buf_def[i], ~d->type_buf[i]);
> -               else
> -                       ret = regmap_irq_update_bits(d, reg,
> -                               d->type_buf_def[i], d->type_buf[i]);
> -               if (ret != 0)
> -                       dev_err(d->map->dev, "Failed to sync type in %x\n",
> -                               reg);
> +       /* Don't update the type bits if we're using mask bits for irq type. */
> +       if (!d->chip->type_in_mask) {
> +               for (i = 0; i < d->chip->num_type_reg; i++) {
> +                       if (!d->type_buf_def[i])
> +                               continue;
> +                       reg = d->chip->type_base +
> +                               (i * map->reg_stride * d->type_reg_stride);
> +                       if (d->chip->type_invert)
> +                               ret = regmap_irq_update_bits(d, reg,
> +                                       d->type_buf_def[i], ~d->type_buf[i]);
> +                       else
> +                               ret = regmap_irq_update_bits(d, reg,
> +                                       d->type_buf_def[i], d->type_buf[i]);
> +                       if (ret != 0)
> +                               dev_err(d->map->dev, "Failed to sync type in %x\n",
> +                                       reg);
> +               }
>         }
>
>         if (d->chip->runtime_pm)
> @@ -194,8 +197,27 @@ static void regmap_irq_enable(struct irq_data *data)
>         struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
>         struct regmap *map = d->map;
>         const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
> +       unsigned int mask, type;
> +
> +       type = irq_data->type_falling_mask | irq_data->type_rising_mask;
> +
> +       /*
> +        * The type_in_mask flag means that the underlying hardware uses
> +        * separate mask bits for rising and falling edge interrupts, but
> +        * we want to make them into a single virtual interrupt with
> +        * configurable edge.
> +        *
> +        * If the interrupt we're enabling defines the falling or rising
> +        * masks then instead of using the regular mask bits for this
> +        * interrupt, use the value previously written to the type buffer
> +        * at the corresponding offset in regmap_irq_set_type().
> +        */
> +       if (d->chip->type_in_mask && type)
> +               mask = d->type_buf[irq_data->reg_offset / map->reg_stride];
> +       else
> +               mask = irq_data->mask;
>
> -       d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~irq_data->mask;
> +       d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~mask;
>  }
>
>  static void regmap_irq_disable(struct irq_data *data)
> @@ -430,6 +452,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
>         struct regmap_irq_chip_data *d;
>         int i;
>         int ret = -ENOMEM;
> +       int num_type_reg;
>         u32 reg;
>         u32 unmask_offset;
>
> @@ -479,13 +502,14 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
>                         goto err_alloc;
>         }
>
> -       if (chip->num_type_reg) {
> -               d->type_buf_def = kcalloc(chip->num_type_reg,
> -                                       sizeof(unsigned int), GFP_KERNEL);
> +       num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg;
> +       if (num_type_reg) {
> +               d->type_buf_def = kcalloc(num_type_reg,
> +                                         sizeof(unsigned int), GFP_KERNEL);
>                 if (!d->type_buf_def)
>                         goto err_alloc;
>
> -               d->type_buf = kcalloc(chip->num_type_reg, sizeof(unsigned int),
> +               d->type_buf = kcalloc(num_type_reg, sizeof(unsigned int),
>                                       GFP_KERNEL);
>                 if (!d->type_buf)
>                         goto err_alloc;
> @@ -600,7 +624,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
>                 }
>         }
>
> -       if (chip->num_type_reg) {
> +       if (chip->num_type_reg && !chip->type_in_mask) {
>                 for (i = 0; i < chip->num_irqs; i++) {
>                         reg = chip->irqs[i].type_reg_offset / map->reg_stride;
>                         d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index a367d59c301d..b7aa50cfb306 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1131,6 +1131,9 @@ struct regmap_irq {
>   * @ack_invert:  Inverted ack register: cleared bits for ack.
>   * @wake_invert: Inverted wake register: cleared bits are wake enabled.
>   * @type_invert: Invert the type flags.
> + * @type_in_mask: Use the mask registers for controlling irq type. For
> + *                interrupts defining type_rising/falling_mask use mask_base
> + *                for edge configuration and never update bits in type_base.
>   * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
>   *
>   * @num_regs:    Number of registers in each control bank.
> @@ -1169,6 +1172,7 @@ struct regmap_irq_chip {
>         bool wake_invert:1;
>         bool runtime_pm:1;
>         bool type_invert:1;
> +       bool type_in_mask:1;
>
>         int num_regs;
>
> --
> 2.19.1
>