On Tue, Nov 10, 2020 at 5:51 PM Lars Povlsen <[email protected]> wrote:
> > On Mon, Nov 9, 2020 at 3:27 PM Lars Povlsen <[email protected]>
> > wrote:
> >> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
> >> (SGPIO) device used in various SoC's.
> >
> > Please, elaborate what you said previously, because now it has no
> > justification to be a pin control driver.
>
> As previously stated, the individual pins have possible other functions
> than GPIO. When these functions are added, the driver will need pinctrl
> functinality. This was accepted by Linux Walleij.
Yes, I understand that. What I meant is to update the commit message
to tell this to the reviewers / readers / anthropologists.
...
> >> + return -EOPNOTSUPP;
> >
> > Are you sure? IIRC internally we are using ENOTSUPP.
> >
> > Couple of drivers seem to be wrongly using the other one.
>
> Checkpatch complains about ENOTSUPP:
>
> # ENOTSUPP is not a standard error code and should be avoided in new patches.
> # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type
> ENOTSUPP.
checkpatch is wrong if this is internal code and to me sounds like
it's not going out of the kernel.
...
> >> + err = -EOPNOTSUPP;
> >
> > Ditto.
>
> Ditto.
Ditto.
...
> >> + dev_err(pctldev->dev, "Pin %d direction as %s is not
> >> possible\n",
> >> + pin, input ? "input" : "output");
> >
> > Do we need this noise? Isn't user space getting a proper error code as
> > per doc and can handle this?
> >
>
> This need not go to user space, as one use-case is using the pin as a
> i2c mux. In this case no signs of the error condition is recorded, it
> just doesn't work. So I concur it is not noise, it is sign of an
> erroneous situation which should be fixed, IMHO.
>
> The message makes it easy to locate the issue, if any. The message will
> not occur on a properly configured system.
It's noise. As we discussed with Alexandre (and I guess came to the
same page) that its consumer's business how to treat the error.
> Lets have the maintainer make the call.
...
> >> +static int microchip_sgpio_get_ports(struct sgpio_priv *priv)
> >> +{
> >> +}
> >
> > As per previous version comment, i.e. perhaps find an existing API for
> > this kind of parser or introduce a generic one.
>
> I fixed the use of OF api's - that was surely an oversight.
>
> I have searched for a suitable API without finding one. The closest
> thing was the parsing of "gpio-reserved-ranges" in gpiolib-of.c, but
> that was coded directly. So I think this might not be of general use.
>
> If it is, lets do that after the driver is merged.
I guess it will be a lot of benefit to have such API earlier than later.
--
With Best Regards,
Andy Shevchenko
