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

From: Emil Renner Berthing
Date: Thu Feb 10 2022 - 09:14:37 EST

On Thu, 10 Feb 2022 at 14:50, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> On Thu, 10 Feb 2022 13:44:12 +0000,
> Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> >
> > On Thu, 10 Feb 2022 at 14:32, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > 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).
> >
> > 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