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

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

On Thu, Sep 01, 2022 at 09:03:49PM +0300, Andy Shevchenko wrote:
> 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.

It's probably also more logical to call this "hwirq".

> > +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);
...
> > +       if (!test_bit(offset, smcgp->irq_supported))
> > +               return -EINVAL;
> 
> We have a valid mask for IRQs. Can it be used here instead?

Looks like we can, thanks for the suggestion.

> > +       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?

"lock a handler" ? I guess you mean select a handler.

I don't see a reason why we couldn't switch between handle_bad_irq()
and handle_simple_irq(). I would guess (I don't know the implementation
details of the Apple platform) that the SMC forwards a message when the
IRQ happens, but I'm guessing that this is well tested on the platform
with the simple flow handler. Changing it to something else would need
discussion with the Asahi Linux folk.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!