Re: [RFT PATCH v3 16/27] irqchip/apple-aic: Add support for the Apple Interrupt Controller

[Hector Martin]

On 06/03/2021 00.05, Andy Shevchenko wrote:
> > +#define pr_fmt(fmt) "%s: " fmt, __func__
>
> This is not needed, really, if you have unique / distinguishable
> messages in the first place.
> Rather people include module names, which may be useful.

Makes sense, I'll switch to KBUILD_MODNAME.

> > +#define MASK_BIT(x)            BIT((x) & 0x1f)
>
> GENMASK(4,0)

It's not really a register bitmask, but rather extracting the low bits 
of an index... but sure, GENMASK also expresses that. Changed.

> > +static atomic_t aic_vipi_flag[AIC_MAX_CPUS];
> > +static atomic_t aic_vipi_enable[AIC_MAX_CPUS];
>
> Isn't it easier to handle these when they are full width, i.e. 32
> items per the array?

I don't think so, it doesn't really buy us anything. It's just a maximum 
beyond which the driver doesn't work in its current state anyway (if the 
number were much larger it'd make sense to dynamically allocate these, 
but not at this point).

> > +static int aic_irq_set_affinity(struct irq_data *d,
> > +                               const struct cpumask *mask_val, bool force)
> > +{
> > +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
> > +       struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
> > +       int cpu;
> > +
> > +       if (hwirq > ic->nr_hw)
>
> >= ?

Good catch, but this is actually obsolete. Higher IRQs go into the FIQ 
irqchip, so this should never happen (it's a leftover from when they 
were a single one). I'll remove it.

Ack on the other comments, thanks!

--
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub