Re: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
>
> On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:

...

> > > +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> > > +{
> > > +       return sfp->gc.parent;
> > > +}
> > > +
> >
> > This seems useless helper. You may do what it's doing just in place.
> > It will save 5 LOCs.
>
> I don't mind removing it, I just think it's easier to read when we're
> explicit that all we want is a dev pointer, and we don't suddenly need
> to know the parent of the gpio chip in all the pinmux/pinconf
> callbacks.

I don't really see the gain of it.

...

> > > +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > > +{
> > > +       struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> > > +       void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> > > +
> > > +       /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> > > +       return readl_relaxed(doen) != GPO_ENABLE;
> >
> > I believe the idea was to return the predefined values for the direction.
>
> You mean this?
>   return readl_relaxed(doen) == GPO_ENABLE ? GPIO_LINE_DIRECTION_OUT :
> GPIO_LINE_DIRECTION_IN;

For example, or with if (...) return _OUT; return _IN;'

> > > +}

...

> > > +       if (trigger & IRQ_TYPE_EDGE_BOTH)
> > > +               irq_set_handler_locked(d, handle_edge_irq);
> > > +       else if (trigger & IRQ_TYPE_LEVEL_MASK)
> > > +               irq_set_handler_locked(d, handle_level_irq);
> >
> > Usually we don't assign this twice, so it should be after the switch.
> >
> > > +       switch (trigger) {

> > > +       default:
> >
> > > +               irq_set_handler_locked(d, handle_bad_irq);
> >
> > Why? You have it already in ->probe(), what's the point?
>
> So last time you asked about this, I explained a situation where
> userspace first grabs a GPIO, set the interrupt to edge triggered, and
> then later loads a driver that requests an unsupported IRQ type.

I didn't get this scenario. Is it real?

> Then
> I'd like to set the handler back to handle_bad_irq so we don't get
> weird interrupts, but maybe now you know a reason why that doesn't
> matter or can't happen?

In ->probe() you set _default_ handler to bad(), what do you mean by
'set the handler back to bad()'? How is it otherwise if you free an
interrupt?

So, please elaborate with call traces what the scenario / use case you
are talking about. If it's true what you are saying, we have a
situation (plenty of GPIO drivers don't do what you are suggesting
here).

> > > +               return -EINVAL;
> > > +       }

...

> > > +       ret = reset_control_deassert(rst);
> > > +       if (ret)
> > > +               return dev_err_probe(dev, ret, "could not deassert resetd\n");
> >
> > > +       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > > +       if (ret)
> >
> > I don't see who will assert reset here.
>
> No, so originally this driver would first assert and then deassert
> reset. I decided against that because in all likelyhood earlier boot
> stages would have set pinmux up for a serial port, and we don't want
> to interrupt the serial debug output. The only reason I make sure the
> reset line is deasserted is in case someone makes a really minimal
> bootloader that just does the absolute minimal to load a Linux kernel
> and doesn't even log any anything.
>
> By the same token we also don't want to assert reset on error in case
> it resets pin muxing for the the serial line that was supposed to log
> the error.

Perhaps comment in the code explaining this?

-- 
With Best Regards,
Andy Shevchenko