Hi Chris,
On Mon, Jul 30, 2018 at 2:33 PM Chris Brandt <[email protected]> wrote:
> On Monday, July 30, 2018, Geert Uytterhoeven wrote:
> > > if (sci_port->irqs[0] < 0)
> > > return -ENXIO;
> > >
> > > - if (sci_port->irqs[1] < 0) {
> > > - sci_port->irqs[1] = sci_port->irqs[0];
> > > - sci_port->irqs[2] = sci_port->irqs[0];
> > > - sci_port->irqs[3] = sci_port->irqs[0];
> > > - }
> > > + if (sci_port->irqs[1] < 0)
> > > + for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
> >
> > Shouldn't the "- 1" be dropped?
>
> In reality, the last entry of the array is 'SCIx_NR_IRQS', so it's not
> really used anywhere.
>
> The original array was:
> enum {
> SCIx_ERI_IRQ, (the only IRQ specified in DT)
=0
> SCIx_RXI_IRQ, << copy from irqs[0]
= 1
> SCIx_TXI_IRQ, << copy from irqs[0]
= 2
> SCIx_BRI_IRQ, << copy from irqs[0]
= 3
> SCIx_NR_IRQS, (didn’t' touch)
= 4
>
> SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */
> };
That's not the array, but the enum.
The array is in struct sci_port:
int irqs[SCIx_NR_IRQS];
It has four entries, at indices 0..3.
> So the new for loop used "- "1 in order to create the same outcome.
>
> But as far as I can tell irqs[SCIx_NR_IRQS] is never used anywhere, it
> doesn't really matter.
irqs[SCIx_NR_IRQS] does not exist!
sci_irq_desc[SCIx_NR_IRQS] aka sci_irq_desc[SCIx_MUX_IRQ] does exit,
but that's a different array.
> > With the above fixed:
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
>
> I have no problem taking the "- 1" out.
>
> But...here's the funny part: It was you that suggested the "- 1" ;)
>
> On Thursday, July 26, 2018, Geert Uytterhoeven wrote:
> > > @@ -2809,6 +2845,8 @@ static int sci_init_single(struct platform_device
> > *dev,
> > > sci_port->irqs[1] = sci_port->irqs[0];
> > > sci_port->irqs[2] = sci_port->irqs[0];
> > > sci_port->irqs[3] = sci_port->irqs[0];
> > > + sci_port->irqs[4] = sci_port->irqs[0];
> > > + sci_port->irqs[5] = sci_port->irqs[0];
> >
> > You may want to start using a loop from 1 to ARRAY_SIZE(sci_port->irqs) - 1
> > instead.
Your loop is:
for (i = 1; i < ARRAY_SIZE(sci_port->irqs) - 1; i++)
It loops over 1..ARRAY_SIZE(sci_port->irqs) - 2.
Note the "<" and the "- 1".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
