linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Johan Hovold <johan@kernel.org>
Cc: linux-usb@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	kernel-team@android.com
Subject: Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes
Date: Mon, 07 Dec 2020 15:34:23 +0000	[thread overview]
Message-ID: <d5fa2065009d5854b4c719003ebcb255@kernel.org> (raw)
In-Reply-To: <X85FVc07Hc7LQQU8@localhost>

On 2020-12-07 15:08, Johan Hovold wrote:
> On Mon, Dec 07, 2020 at 02:41:03PM +0000, Marc Zyngier wrote:
>> On 2020-12-07 14:01, Johan Hovold wrote:
>> > On Fri, Dec 04, 2020 at 04:47:35PM +0000, Marc Zyngier wrote:
>> >> Having recently tried to use the CBUS GPIOs that come thanks to the
>> >> ftdio_sio driver, it occurred to me that the driver has a couple of
>> >> usability issues:
>> >>
>> >> - it advertises potential GPIOs that are reserved to other uses (LED
>> >>   control, or something else)
>> >
>> > Consider the alternative, that the gpio offsets (for CBUS0, CBUS1,
>> > CBUS2
>> > or CBUS4) varies depending on how the pins have been muxed. Hardly very
>> > user friendly.
>> 
>> That's not what I suggest. If you want fixed GPIO offsets, fine by me.
>> But telling the user "these are GPIOs you can use", and then
>> "on second though, you can't" is not exactly consistent.
> 
> It's really no different from any other gpio chip which registers all
> its lines, including those which may have been muxed for other 
> purposes.

If they claim that their lines are available, and then refuse to
let the user play with it, that's just a bug willing to be fixed.

>> >> - it returns an odd error (-ENODEV), instead of the expected -EINVAL
>> >>   when a line is unavailable, leading to a difficult diagnostic
>> >
>> > Hmm, maybe. Several gpio driver return -ENODEV when trying to request
>> > reserved pins. Even gpiolib returns -ENODEV when a pins is not yet
>> > available due to probe deferal.
>> 
>> -ENODEV really means "no GPIOchip" in this context. The fact that
>> other drivers return -ENODEV for reserved pins looks like a bug to me.
> 
> No, the chip is there. The -ENODEV is what you get when requesting the
> line, because the line isn't available.

I still believe that ENODEV is the wrong error. The device is there,
but the request is invalid because the line is used by something else.
EINVAL, EBUSY, ENXIO would all be (sort of) OK.

> 
>> > -EBUSY could also be an alternative, but that's used to indicate that a
>> > line is already in use as a gpio.
>> 
>> Or something else. Which is exactly the case, as it's been allocated
>> to another function.
> 
> Right, there are invalid requests (e.g. requesting line five of a four
> line chip), lines that are already in use, and lines not available due
> to muxing.
> 
> And then there's the question of whether to use the same or distinct
> errnos for these. I believe using distinct errnos provides more
> feedback, but we can certainly pick another errno for this if it's
> really that confusing.

Fundamentally, I don't think the backend driver should be in charge
of the error reporting. That should be the char device's job. Leaving it
to the individual drivers is a sure way to have an inconsistent API.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

  reply	other threads:[~2020-12-07 15:35 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-04 16:47 Marc Zyngier
2020-12-04 16:47 ` [PATCH 1/4] gpiolib: cdev: Flag invalid GPIOs as used Marc Zyngier
2020-12-07 14:16   ` Johan Hovold
2020-12-07 14:59     ` Marc Zyngier
2020-12-09  9:25   ` Linus Walleij
2020-12-04 16:47 ` [PATCH 2/4] USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib Marc Zyngier
2020-12-09  9:28   ` Linus Walleij
2020-12-04 16:47 ` [PATCH 3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity Marc Zyngier
2020-12-07 14:29   ` Johan Hovold
2020-12-07 15:00     ` Marc Zyngier
2020-12-07 15:19       ` Johan Hovold
2020-12-09  9:35         ` Linus Walleij
2020-12-09 17:05           ` Johan Hovold
2020-12-09 17:39             ` Johan Hovold
2020-12-04 16:47 ` [PATCH 4/4] USB: serial: ftdi_sio: Drop GPIO line checking dead code Marc Zyngier
2020-12-07  9:55 ` [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes Andy Shevchenko
2020-12-07 14:01 ` Johan Hovold
2020-12-07 14:41   ` Marc Zyngier
2020-12-07 15:08     ` Johan Hovold
2020-12-07 15:34       ` Marc Zyngier [this message]
2020-12-07 15:49         ` Johan Hovold
2020-12-09  9:20           ` Linus Walleij
2020-12-09 15:42             ` Johan Hovold

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d5fa2065009d5854b4c719003ebcb255@kernel.org \
    --to=maz@kernel.org \
    --cc=bgolaszewski@baylibre.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=johan@kernel.org \
    --cc=kernel-team@android.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --subject='Re: [PATCH 0/4] USB: ftdio_sio: GPIO validity fixes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).