Re: [PATCH v3 1/1] gpio: mpfs: add polarfire soc gpio support

[Lewis.Hanly]

On Sat, 2022-07-16 at 11:33 +0100, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Sat, 16 Jul 2022 08:11:13 +0100,
> <lewis.hanly@xxxxxxxxxxxxx> wrote:
> > From: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>
> > 
> > Add a driver to support the Polarfire SoC gpio controller.
> > 
> > Signed-off-by: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>
> 
> [...]
> 
> > +static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> > +                                        unsigned int child,
> > +                                        unsigned int child_type,
> > +                                        unsigned int *parent,
> > +                                        unsigned int *parent_type)
> > +{
> > +     struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +     struct irq_data *d = irq_get_irq_data(mpfs_gpio-
> > >irq_number[child]);
> 
> This looks totally wrong. It means that you have already instantiated
> part of the hierarchy, and it is likely that you will get multiple
> hierarchy sharing some levels, which isn't intended.
Thanks Marc, as you have pointed out the hierarchical interrupt flow is
not fitting our hw. I am in the process of reworking the driver to
implement chained interrupt flow as you pointed out before.
> 
> > +     *parent_type = IRQ_TYPE_NONE;
> > +     *parent = irqd_to_hwirq(d);
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpfs_gpio_probe(struct platform_device *pdev)
> > +{
> > +     struct clk *clk;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *node = pdev->dev.of_node;
> > +     struct device_node *irq_parent;
> > +     struct gpio_irq_chip *girq;
> > +     struct irq_domain *parent;
> > +     struct mpfs_gpio_chip *mpfs_gpio;
> > +     int i, ret, ngpio;
> > +
> > +     mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio),
> > GFP_KERNEL);
> > +     if (!mpfs_gpio)
> > +             return -ENOMEM;
> > +
> > +     mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(mpfs_gpio->base))
> > +             return dev_err_probe(dev, PTR_ERR(mpfs_gpio->clk),
> > "input clock not found.\n");
> > +
> > +     clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(clk))
> > +             return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get
> > failed\n");
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to enable
> > clock\n");
> > +
> > +     mpfs_gpio->clk = clk;
> > +
> > +     ngpio = of_irq_count(node);
> > +     if (ngpio > NUM_GPIO) {
> > +             ret = -ENXIO;
> > +             goto cleanup_clock;
> > +     }
> > +
> > +     irq_parent = of_irq_find_parent(node);
> > +     if (!irq_parent) {
> > +             ret = -ENODEV;
> > +             goto cleanup_clock;
> > +     }
> > +     parent = irq_find_host(irq_parent);
> > +     if (!parent) {
> > +             ret = -ENODEV;
> > +             goto cleanup_clock;
> > +     }
> > +
> > +     /* Get the interrupt numbers. */
> > +     /* Clear/Disable All interrupts before enabling parent
> > interrupts. */
> > +     for (i = 0; i < ngpio; i++) {
> > +             mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i);
> 
> Bingo. You are allocating the interrupt for the level below. You
> really shouldn't do that.
> 
> If you need to retrieve the *hwirq* for the level below, you need to
> parse the DT without triggering an IRQ allocation (of_irq_parse_one()
> and co).
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.