Re: [PATCH 6/6] gpio: macsmc: Add IRQ support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@xxxxxxxxxxxxxxx> wrote:
>
> From: Hector Martin <marcan@xxxxxxxxx>
>
> Add IRQ support to the macsmc driver. This patch has updates from Joey
> Gouly and Russell King.

...

> +       u16 type = event >> 16;
> +       u8 offset = (event >> 8) & 0xff;

The ' & 0xff' part is redundant.

...

> +       return (ret == 0) ? NOTIFY_OK : NOTIFY_DONE;

Parentheses and ' == 0' parts are redundant. You may swap ternary operands.

...

> +static void macsmc_gpio_irq_enable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
> +
> +       gpiochip_enable_irq(gc, irqd_to_hwirq(d));
> +       set_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow);

You may use temporary variable for hwirq

  irq_hw_number_t hwirq = irdq_to_hwirq(d);

> +}
> +
> +static void macsmc_gpio_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
> +
> +       clear_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow);
> +       gpiochip_disable_irq(gc, irqd_to_hwirq(d));

Ditto.

> +}
> +
> +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);

> +       int offset = irqd_to_hwirq(d);

As above.


> +       u32 mode;

> +       if (!test_bit(offset, smcgp->irq_supported))
> +               return -EINVAL;

We have a valid mask for IRQs. Can it be used here instead?

> +       switch (type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               mode = IRQ_MODE_HIGH;
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               mode = IRQ_MODE_LOW;
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               mode = IRQ_MODE_RISING;
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               mode = IRQ_MODE_FALLING;
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               mode = IRQ_MODE_BOTH;
> +               break;
> +       default:
> +               return -EINVAL;
>         }
>
> +       smcgp->irq_mode_shadow[offset] = mode;

Usually we want to have handle_bad_irq() handler by default and in
->set_type() we lock a handler depending on the flags. Why is this not
the case in this driver?

>         return 0;
>  }

...

> +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct macsmc_gpio *smcgp = gpiochip_get_data(gc);

> +       smc_key key = macsmc_gpio_key(irqd_to_hwirq(d));
> +       int offset = irqd_to_hwirq(d);

As above.

> +       bool val;
> +
> +       if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) {
> +               u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset];
> +               if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0)
> +                       dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd);
> +               else
> +                       smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset];
> +       }
> +
> +       val = test_bit(offset, smcgp->irq_enable_shadow);
> +       if (test_bit(offset, smcgp->irq_enable) != val) {
> +               if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ENABLE | val) < 0)
> +                       dev_err(smcgp->dev, "GPIO IRQ en/disable failed for %p4ch\n", &key);
> +               else
> +                       change_bit(offset, smcgp->irq_enable);
> +       }
> +
> +       mutex_unlock(&smcgp->irq_mutex);
> +}

--
With Best Regards,
Andy Shevchenko