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:

quoted

On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing [off-list ref] wrote:

...

quoted

quoted

+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.

quoted

quoted

+free_pgnames:
+       devm_kfree(dev, pgnames);

Ditto.

...

quoted

quoted

+out:

Useless label.

Hmm.. my compiler disagrees.

The comment implies that you return directly instead of using `goto out;`.

quoted

quoted

+       return ret;

...

quoted

quoted

+               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.

...

quoted

quoted

+               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?

...

quoted

quoted

+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!

quoted

quoted

+};

...

quoted

quoted

+               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?

...

quoted

quoted

+               if (value <= 6)
+                       writel(value, sfp->padctl + IO_PADSHARE_SEL);
+               else

quoted

+                       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?

-- 
With Best Regards,
Andy Shevchenko