Re: [PATCH] pinctrl: ocelot: Fix interrupt controller

[Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>]

The 09/02/2022 17:51, Andy Shevchenko wrote:

Hi Andy,

> 
> > +       /*
> > +        * It is enough to read only one action because the trigger level is the
> > +        * same for all of them.
> > +        */
> 
> Hmm... this is interesting. How is the hardware supposed to work if
> the user asks for two contradictory levels for two different IRQs?

The HW can detect the changes in line for each pin on which the IRQ is.
And each pin will have a different irq_desc with different actions.
Or maybe I missunderstood the question?

Also maybe a better way to get trigger level is to use
irqd_get_trigger_type.

...

> > +               struct ocelot_irq_work *work = kmalloc(sizeof(*work), GFP_ATOMIC);
> 
> It's more visible what's going on if you split this to definition and
> assignment and move assignment closer to its first user.
> 
> > +               if (!work)
> > +                       return;

So you would like something like this:
---
struct ocelot_irq_work *work;

work = kmalloc(sizeof(*work), GFP_ATOMIC);
if (!work)
    return;
...
---

> 
> ...
> 
> >         type &= IRQ_TYPE_SENSE_MASK;
> 
> This seems redundant, see below.
> 
> 
> > -       if (!(type & (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH)))
> > +       if (type == IRQ_TYPE_NONE)
> >                 return -EINVAL;
> 
> Is it ever possible? IIRC the IRQ chip code, the set->type won't be
> called at all in such a case. Also type is already limited to the
> sense mask, no?

It is not possible. From what I have seen on the callstack, the type is
already anded with IRQ_TYPE_SENSE_MASK, and it would not call
ocelot_irq_set_type for IRQ_TYPE_NONE.
Therefor I will remove these.

> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
/Horatiu