Re: [PATCH V1 3/4] irqchip/loongson-liointc: Fix IRQ trigger polarity

[Jianmin Lv <lvjianmin@xxxxxxxxxxx>]

On 2023/5/21 下午6:46, WANG Xuerui wrote:
> On 2023/5/20 14:38, Jianmin Lv wrote:
> > For IRQ controller INT_POLARITY regitser of Loongson-2K CPU
>
> "For the INT_POLARITY register of Loongson-2K series IRQ controller"?
>
> > series, '0' indicates high level or rising edge triggered IRQ,
> > '1' indicates low level or falling edge triggered IRQ.
>
> Remove the two "IRQ"s; the topic is "polarity", not "IRQs".
>
> Also please mention the source of this information; I've checked the 
> Loongson 2K1000LA User Manual v1.0 and it seems a similar description is 
> found in Table 9-2, Section 9.3 (中断寄存器描述 / Description of the 
> Interrupt Registers). It mentioned "Intpol_0" and "Intpol_1" but the 
> description is consistent with the wording here.
>
> > For Loongson-3A CPU series, setting INT_POLARITY register is not
> > supported and writting it has no effect.
>
> Only 3A and not the whole Loongson-3 series?
>
> Also typo: "writing".

Ok, I'll adjust the commit as your suggestion above, thanks.

> > So trigger polarity setting shouled be fixed for Loongson-2K CPU
> > series.
>
> The changes seem to be just inversion of the polarity flags. It should 
> be correct given your description, and not affect Loongson-3 series 
> because it's supposed to behave as noops; it may be better to move the 
> explanation regarding Loongson-3 behavior to code comment (e.g. 
> somewhere near the definition of LIOINTC_REG_INTC_POL) so it's 
> immediately visible to drive-by readers not familiar with LoongArch 
> internals, without them having to dig through commit history to see this.
Good suggestion, I'll add the information near the definition of 
LIOINTC_REG_INTC_POL.

> > Fixes: 17343d0b4039 ("irqchip/loongson-liointc: Support to set IRQ 
> > type for ACPI path")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Chong Qiao <qiaochong@xxxxxxxxxxx>
> > Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
>
> Again, who's the proper author for this patch? Given the tags it seems 
> the author should be Chong Qiao, but I didn't see an Author: line at the 
> beginning.

Again, I'll adjust them as following:
Co-developed-by: Chong Qiao <qiaochong@xxxxxxxxxxx>
Signed-off-by: Chong Qiao <qiaochong@xxxxxxxxxxx>
Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>

Thanks.

> > ---
> >   drivers/irqchip/irq-loongson-liointc.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-loongson-liointc.c 
> > b/drivers/irqchip/irq-loongson-liointc.c
> > index 8d00a9ad5b00..9a9c2bf048a3 100644
> > --- a/drivers/irqchip/irq-loongson-liointc.c
> > +++ b/drivers/irqchip/irq-loongson-liointc.c
> > @@ -116,19 +116,19 @@ static int liointc_set_type(struct irq_data 
> > *data, unsigned int type)
> >       switch (type) {
> >       case IRQ_TYPE_LEVEL_HIGH:
> >           liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, false);
> > -        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, true);
> > +        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, false);
> >           break;
> >       case IRQ_TYPE_LEVEL_LOW:
> >           liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, false);
> > -        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, false);
> > +        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, true);
> >           break;
> >       case IRQ_TYPE_EDGE_RISING:
> >           liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, true);
> > -        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, true);
> > +        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, false);
> >           break;
> >       case IRQ_TYPE_EDGE_FALLING:
> >           liointc_set_bit(gc, LIOINTC_REG_INTC_EDGE, mask, true);
> > -        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, false);
> > +        liointc_set_bit(gc, LIOINTC_REG_INTC_POL, mask, true);
> >           break;
> >       default:
> >           irq_gc_unlock_irqrestore(gc, flags);