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:50, Marc Zyngier <maz@kernel.org> wrote:
> On Thu, 10 Feb 2022 13:44:12 +0000,
> Emil Renner Berthing <kernel@esmil.dk> wrote:
> >
> > 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.
>
> Ah, I should have checked that. In which case, would you be OK if I
> simply pushed the removal of this label as a fix for 5.17, and just
> have it to say "Star5 GPIO", for example, without any indication of
> the device (which appears in debugfs anyway as part of the irqdomain)?

I'm fine with it although I'd prefer "StarFive GPIO". I haven't seen
star5 used anywhere.
But shouldn't changes like this normally go through Linus Walleij's tree?

/Emil