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

[Emil Renner Berthing <kernel@xxxxxxxx>]

On Wed, 13 Oct 2021 at 18:59, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Wed, Oct 13, 2021 at 06:38:14PM +0200, Emil Renner Berthing wrote:
> > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
>
> ...
>
> > > > +free_pinmux:
> > > > +       devm_kfree(dev, pinmux);
> > > > +free_pins:
> > > > +       devm_kfree(dev, pins);
> > > > +free_grpname:
> > > > +       devm_kfree(dev, grpname);
> > >
> > > What the heck?!
> >
> > Just to be clear. You mean we don't need to explicitly free them
> > because they're tied to the device right? I don't think the device
> > will go away just because a single device tree entry can't be parsed,
> > so on such errors this garbage would be left behind. You can still
> > argue we shouldn't optimize for broken device trees, I just want to
> > make it at conscious decision.
>
> If you are using devm_kfree() it is quite likely shows either of the following
> issues:
>
> * you mustn't use devm_*() in the first place due to object lifetime;
> * you shouldn't use devm_kfree() since this is the whole point of devm.

Hmm.. it seems like other drivers that dynamically builds the groups
and functions either also uses devm_kcalloc/devm_kfree like fx.
pinctrl-single or implements their own code to clean up groups and
functions when unloaded. There are no group destroy or function
destroy callbacks. I like devm_kcalloc/devm_kfree version better since
it's less code to write.

> > > > +free_pgnames:
> > > > +       devm_kfree(dev, pgnames);
> > >
> > > Ditto.
>
> ...
>
> > > > +out:
> > >
> > > Useless label.
> >
> > Hmm.. my compiler disagrees.
>
> The comment implies that you return directly instead of using `goto out;`.
>
> > > > +       return ret;
>
> ...
>
> > > > +               v = pinmux[i];
> > > > +               dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU);
> > > > +               doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU);
> > > > +               din  = (v >> 8) & 0xffU;
> > >
> > > What is this voodoo for?
> >
> > In order to do pinmux we need the following pieces of information from
> > the device tree for each pin ("GPIO" they call it):
> >
> > output signal: 0-133 + 1bit reverse flag
> > output enable signal: 0-133 + 1bit reverse flag
> > optional input signal: 0-74 + special "none" value, right now 0xff
> > gpio number: 0-63
> >
> > As the code is now all that info is packed into a u32 for each pin
> > using the GPIOMUX macro defined in the dt-binding header added in
> > patch 10. There is also a diagram for how this packing is done. The
> > above voodoo is for unpacking that.
> >
> > I'd very much like to hear if you have a better solution for how to
> > convey that information from the device tree to here.
>
> At very least this code should have something like above in the comment.

Will add!

> ...
>
> > > > +               if (din != 0xff)
> > > > +                       reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din;
> > > > +               else
> > > > +                       reg_din = NULL;
> > >
> > > This looks like you maybe use gpio-regmap instead?
> >
> > This was discussed at length when Drew sent in the GPIO part of this code:
> > https://lore.kernel.org/linux-riscv/20210701002037.912625-1-drew@xxxxxxxxxxxxxxx/
> > The conclusion was that because pinmux and controlling the pins from
> > software in GPIO mode uses the same registers it is better to do a
> > combined driver like this that can share the lock among other things.
>
> And what does prevent exactly to base the GPIO part on gpio-regmap?

Other reasons are that gpio-regmap doesn't implement the .set_config
and .add_pin_ranges callbacks. add_pin_ranges is needed because the 64
GPIOs map to different pin numbers depending on the chosen "signal
group".

> ...
>
> > > > +static const unsigned char starfive_drive_strength[] = {
> > > > +       14, 21, 28, 35, 42, 49, 56, 63,
> > >
> > > Why table? Can you simply use the formula?!
> >
> > Heh, yeah. So these are rounded values from a table and I never
> > noticed that after rounding they follow a nice arithmetic progression.
> > It'll probably still be nice to have an explanation in the comments
> > about the formula then.
>
> Yup!
>
> > > > +};
>
> ...
>
> > > > +               irq_set_handler_locked(d, handle_bad_irq);
> > >
> > > Why is this here? Move it to ->probe().
> >
> > My thinking was that if something tries to set a an unsupported irq
> > type, we should make sure the caller doesn't get spurious interrupts
> > because we left the handler at its old value.
>
> You already assigned to this handler in the ->probe(), what's this then?

But userspace could fx. first request IRQ_TYPE_EDGE_BOTH through the
gpio api and later load a driver that might request an unsupported irq
type right? Or am I missing something.

> ...
>
> > > > +               if (value <= 6)
> > > > +                       writel(value, sfp->padctl + IO_PADSHARE_SEL);
> > > > +               else
> > >
> > > > +                       dev_err(dev, "invalid signal group %u\n", value);
> > >
> > > Why _err if you not bail out here?
> >
> > My thinking was that if the device tree specifies an invalid signal
> > group we should just leave the setting alone and not break booting,
> > but still be loud about it. Maybe that's too lenient and it's better
> > to crash and burn immediately if someone does that.
>
> Here is inconsistency between level of the message and following action.
> There are (rare!) cases when it's justified, but I believe it's not the
> case here. You have two choices or justify why you have to use error
> level without stopping process.
>
> ...
>
> All uncommented stuff you agreed on, correct?

Yes, thank you! (.. or at least I'll get back to you if something comes up ;)

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv