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

quoted

On Wed, 13 Oct 2021 at 18:59, Andy Shevchenko [off-list ref] 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 [off-list ref] wrote:
On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing [off-list ref] 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@beagleboard.org/ (local)
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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help