On Tue, Nov 10, 2020 at 5:51 PM Lars Povlsen [off-list ref] wrote:

quoted

On Mon, Nov 9, 2020 at 3:27 PM Lars Povlsen [off-list ref] wrote:

quoted

quoted

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.

...

quoted

quoted

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

...

quoted

quoted

+                       err = -EOPNOTSUPP;

Ditto.

Ditto.

Ditto.

...

quoted

quoted

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

...

quoted

quoted

+static int microchip_sgpio_get_ports(struct sgpio_priv *priv)
+{

quoted

quoted

+}

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