Date:   Wed, 11 Nov 2020 09:51:52 +0100
From:   Lars Povlsen <lars.povlsen@...rochip.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Lars Povlsen <lars.povlsen@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-arm Mailing List" <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [PATCH v8 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO


Andy Shevchenko writes:

> On Tue, Nov 10, 2020 at 5:51 PM Lars Povlsen <lars.povlsen@...rochip.com> wrote:
>> > On Mon, Nov 9, 2020 at 3:27 PM Lars Povlsen <lars.povlsen@...rochip.com> 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.

Ok, will do.

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

As it appears there are different opinions on this I'll let the pinctrl
maintainer decide.

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

I digress. I'll remove it.

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

Thank you for your comments. I'll send the new version shortly.

---Lars

--
Lars Povlsen,
Microchip

Powered by blists - more mailing lists