Re: [PATCH 10/10] pinctrl: starfive: Switch to dynamic chip name output

[Emil Renner Berthing <kernel@esmil.dk>]

On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <maz@kernel.org> wrote:
> On Thu, 10 Feb 2022 12:59:59 +0000,
> Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <maz@kernel.org> wrote:
> > > On Wed, 09 Feb 2022 23:30:55 +0000,
> > > Emil Renner Berthing <kernel@esmil.dk> wrote:
> > > >
> > > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <maz@kernel.org> wrote:
> > > > >
> > > > > Instead of overloading the name field, use the relevant callback to
> > > > > output the device name.
> > > > >
> > > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > > ---
> > > > >  drivers/pinctrl/pinctrl-starfive.c | 11 +++++++++--
> > > > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c
> > > > > index 5be9866c2b3c..f29d9ccf858b 100644
> > > > > --- a/drivers/pinctrl/pinctrl-starfive.c
> > > > > +++ b/drivers/pinctrl/pinctrl-starfive.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include <linux/of.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/reset.h>
> > > > > +#include <linux/seq_file.h>
> > > > >  #include <linux/spinlock.h>
> > > > >
> > > > >  #include <linux/pinctrl/pinctrl.h>
> > > > > @@ -1163,12 +1164,20 @@ static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static void starfive_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > > > > +{
> > > > > +       struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > > > > +
> > > > > +       seq_printf(p, sfp->gc.label);
> > > > > +}
> > > > > +
> > > > >  static struct irq_chip starfive_irq_chip = {
> > > > >         .irq_ack = starfive_irq_ack,
> > > > >         .irq_mask = starfive_irq_mask,
> > > > >         .irq_mask_ack = starfive_irq_mask_ack,
> > > > >         .irq_unmask = starfive_irq_unmask,
> > > > >         .irq_set_type = starfive_irq_set_type,
> > > > > +       .irq_print_chip = starfive_irq_print_chip,
> > > > >         .flags = IRQCHIP_SET_TYPE_MASKED,
> > > > >  };
> > > >
> > > > The parent interrupt doesn't show up in /proc/interrupts anyway, so if
> > > > setting the name is considered abuse we can just drop the addition
> > > > above and just delete the two lines below.
> > >
> > > Are you sure this never appears? Is there a another irqchip stacked on
> > > top of this one? Could you please dump /sys/kernel/debug/irq/irqs/XX,
> > > where XX is an interrupt number using one of these GPIO pins? Please
> > > run it without this patch, as I just noticed that debugfs blindly
> > > uses the name.
> >
> > Yes, the old gpio driver this derives from used to set
> >     sfp->gc.irq.parent_handler = NULL
> > and then register its own irq handler, then the parent would show up
> > in /proc/interrupts. But after switching to letting the gpio framework
> > register the handler it stopped showing up.
>
> But this patch does not deal with the parent interrupt. It deals with
> the irqchip that is used for the 'children interrupt'. Output
> interrupts for a chained handler are never shown, as they don't really
> make much sense on their own (you'd only see the sum of the input
> interrupts).

I see. Sorry for the confusion.

> >
> > root@visionfive~# cat /proc/interrupts
> >            CPU0       CPU1
> >   5:       5035       4907  RISC-V INTC   5 Edge      riscv-timer
> >   6:        960          0  SiFive PLIC   4 Edge      dw-mci
> >   7:       4384          0  SiFive PLIC   5 Edge      dw-mci
> >   8:        562          0  SiFive PLIC   6 Edge      eth0
> >  10:          1          0  SiFive PLIC   7 Edge      eth0
> >  11:          0          0  SiFive PLIC   2 Edge      dw_axi_dmac_platform
> >  15:       2690          0  SiFive PLIC  44 Edge      xhci-hcd:usb1
> >  17:          0          0  SiFive PLIC  43 Edge      104c0000.usb
> >  18:          0          0  SiFive PLIC   1 Edge      dw_axi_dmac_platform
> >  20:        234          0  SiFive PLIC  96 Edge      118b0000.i2c
> >  21:          0          0  SiFive PLIC  97 Edge      118c0000.i2c
> >  22:          7          0  SiFive PLIC  98 Edge      118d0000.trng
> >  28:          0          0  SiFive PLIC 101 Edge      sf_lcdc
> >  29:          0          0  SiFive PLIC 103 Edge      sf_vpp1
> >  30:          0          0  SiFive PLIC  70 Edge      12410000.spi
> >  31:        205          0  SiFive PLIC  73 Edge      ttyS0
> >  32:          0          0  SiFive PLIC  74 Edge      12450000.i2c
> >  33:          0          0  SiFive PLIC  80 Edge      12480000.watchdog
> >  34:         28          0  SiFive PLIC 122 Edge      124a0000.tmon
> >  37:          0          0  11910000.pinctrl  35 Edge      gpiomon
>                               ^^^^^^^^^^^^^^^^
> This is what this patch deals with. Going with your suggestion of
> dropping this output (or to hardcode it to something else) would be a
> userspace visible change, and we can't do that.

Gotcha. The SoC has been out in very few numbers for less than a year
and the driver only entered mainline in 5.17-rc1, so I doubt anyone
has had time to write scripts that check for this, but I'll let it be
up to you.

Reviewed-by: Emil Renner Berthing <kernel@esmil.dk>
Tested-by: Emil Renner Berthing <kernel@esmil.dk>

Thanks.
/Emil