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

[Marc Zyngier <maz@xxxxxxxxxx>]

On Thu, 10 Feb 2022 12:59:59 +0000,
Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> 
> On Thu, 10 Feb 2022 at 10:06, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > On Wed, 09 Feb 2022 23:30:55 +0000,
> > Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> > >
> > > On Wed, 9 Feb 2022 at 17:49, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > >
> > > > Instead of overloading the name field, use the relevant callback to
> > > > output the device name.
> > > >
> > > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > > ---
> > > >  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).

> 
> 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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.