mbox series

[0/4] USB: ftdio_sio: GPIO validity fixes

Message ID 20201204164739.781812-1-maz@kernel.org
Headers show
Series USB: ftdio_sio: GPIO validity fixes | expand

Message

Marc Zyngier Dec. 4, 2020, 4:47 p.m. UTC
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)

- it returns an odd error (-ENODEV), instead of the expected -EINVAL
  when a line is unavailable, leading to a difficult diagnostic

We address the issues in a number of ways:

- Stop reporting invalid GPIO lines as valid to userspace. It
  definitely seems odd to do so. Instead, report the line as being
  used, making the userspace interface a bit more consistent.

- Implement the init_valid_mask() callback in the ftdi_sio driver,
  allowing it to report which lines are actually valid.

- As suggested by Linus, give an indication to the user of why some of
  the GPIO lines are unavailable, and point them to a useful tool
  (once per boot). It is a bit sad that there next to no documentation
  on how to use these CBUS pins.

- Drop the error reporting code, which has become useless at this
  point.

Tested with a couple of FTDI devices (FT230X and FT231X) and various
CBUS configurations.

Marc Zyngier (4):
  gpiolib: cdev: Flag invalid GPIOs as used
  USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib
  USB: serial: ftdi_sio: Log the CBUS GPIO validity
  USB: serial: ftdi_sio: Drop GPIO line checking dead code

 drivers/gpio/gpiolib-cdev.c   |  1 +
 drivers/usb/serial/ftdi_sio.c | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Dec. 7, 2020, 9:55 a.m. UTC | #1
On Fri, Dec 4, 2020 at 6:49 PM Marc Zyngier <maz@kernel.org> 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)

>

> - it returns an odd error (-ENODEV), instead of the expected -EINVAL

>   when a line is unavailable, leading to a difficult diagnostic

>

> We address the issues in a number of ways:

>

> - Stop reporting invalid GPIO lines as valid to userspace. It

>   definitely seems odd to do so. Instead, report the line as being

>   used, making the userspace interface a bit more consistent.

>

> - Implement the init_valid_mask() callback in the ftdi_sio driver,

>   allowing it to report which lines are actually valid.

>

> - As suggested by Linus, give an indication to the user of why some of

>   the GPIO lines are unavailable, and point them to a useful tool

>   (once per boot). It is a bit sad that there next to no documentation

>   on how to use these CBUS pins.

>

> - Drop the error reporting code, which has become useless at this

>   point.

>

> Tested with a couple of FTDI devices (FT230X and FT231X) and various

> CBUS configurations.


Series looks pretty good to me, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Marc Zyngier (4):

>   gpiolib: cdev: Flag invalid GPIOs as used

>   USB: serial: ftdi_sio: Report the valid GPIO lines to gpiolib

>   USB: serial: ftdi_sio: Log the CBUS GPIO validity

>   USB: serial: ftdi_sio: Drop GPIO line checking dead code

>

>  drivers/gpio/gpiolib-cdev.c   |  1 +

>  drivers/usb/serial/ftdi_sio.c | 26 +++++++++++++++++++++++---

>  2 files changed, 24 insertions(+), 3 deletions(-)

>

> --

> 2.28.0

>



-- 
With Best Regards,
Andy Shevchenko
Johan Hovold Dec. 7, 2020, 2:01 p.m. UTC | #2
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.

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

-EBUSY could also be an alternative, but that's used to indicate that a
line is already in use as a gpio.

> We address the issues in a number of ways:

> 

> - Stop reporting invalid GPIO lines as valid to userspace. It

>   definitely seems odd to do so. Instead, report the line as being

>   used, making the userspace interface a bit more consistent.

> 

> - Implement the init_valid_mask() callback in the ftdi_sio driver,

>   allowing it to report which lines are actually valid.

> 

> - As suggested by Linus, give an indication to the user of why some of

>   the GPIO lines are unavailable, and point them to a useful tool

>   (once per boot). It is a bit sad that there next to no documentation

>   on how to use these CBUS pins.


Don't be sad, Marc; write some documentation. ;)

Johan
Johan Hovold Dec. 7, 2020, 2:29 p.m. UTC | #3
On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
> The validity of the ftdi CBUS GPIO is pretty hidden so far,

> and finding out *why* some GPIOs don't work is sometimes

> hard to identify. So let's help the user by displaying the

> map of the CBUS pins that are valid for a GPIO.

> 

> Also, tell the user about the magic ftx-prog tool that can

> make GPIOs appear: https://github.com/richardeoin/ftx-prog

> 

> Suggested-by: Linus Walleij <linus.walleij@linaro.org>

> Signed-off-by: Marc Zyngier <maz@kernel.org>

> ---

>  drivers/usb/serial/ftdi_sio.c | 9 +++++++++

>  1 file changed, 9 insertions(+)

> 

> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c

> index 13e575f16bcd..b9d3b33891fc 100644

> --- a/drivers/usb/serial/ftdi_sio.c

> +++ b/drivers/usb/serial/ftdi_sio.c

> @@ -2012,6 +2012,15 @@ static int ftdi_gpio_init_valid_mask(struct gpio_chip *gc,

>  

>  	bitmap_complement(valid_mask, &map, ngpios);

>  

> +	if (bitmap_empty(valid_mask, ngpios))

> +		dev_warn(&port->dev, "No usable GPIO\n");


This isn't an error of any kind, and certainly not something that
deserves a warning in the system log on every probe. Not everyone cares
about the GPIO interface.

> +	else

> +		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",

> +			 ngpios, valid_mask);


And while printing this mask has some worth I'm still reluctant to be
spamming the logs with it. Just like gpolib has a dev_dbg() for
registering chips, this should probably be demoted to KERN_DEBUG as
well.

> +

> +	if (!bitmap_full(valid_mask, ngpios))

> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog to enable GPIOs if required\n");

> +


And again, this is not something that belongs in the logs of just about
every system with an attached ftdi device.

While not possible to combine with the valid_mask approach, this is
something which we could otherwise add to the request() callback for the
first request that fails due to the mux configuration.

>  	return 0;

>  }


Johan
Marc Zyngier Dec. 7, 2020, 2:41 p.m. UTC | #4
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 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.

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

>> We address the issues in a number of ways:

>> 

>> - Stop reporting invalid GPIO lines as valid to userspace. It

>>   definitely seems odd to do so. Instead, report the line as being

>>   used, making the userspace interface a bit more consistent.

>> 

>> - Implement the init_valid_mask() callback in the ftdi_sio driver,

>>   allowing it to report which lines are actually valid.

>> 

>> - As suggested by Linus, give an indication to the user of why some of

>>   the GPIO lines are unavailable, and point them to a useful tool

>>   (once per boot). It is a bit sad that there next to no documentation

>>   on how to use these CBUS pins.

> 

> Don't be sad, Marc; write some documentation. ;)


I sure will, right after I have fixed the rest of the kernel bugs
I have introduced. With a bit of luck, that's right after I finally
kick the bucket.

         M.
-- 
Jazz is not dead. It just smells funny...
Marc Zyngier Dec. 7, 2020, 3 p.m. UTC | #5
On 2020-12-07 14:29, Johan Hovold wrote:
> On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:

>> The validity of the ftdi CBUS GPIO is pretty hidden so far,

>> and finding out *why* some GPIOs don't work is sometimes

>> hard to identify. So let's help the user by displaying the

>> map of the CBUS pins that are valid for a GPIO.

>> 

>> Also, tell the user about the magic ftx-prog tool that can

>> make GPIOs appear: https://github.com/richardeoin/ftx-prog

>> 

>> Suggested-by: Linus Walleij <linus.walleij@linaro.org>

>> Signed-off-by: Marc Zyngier <maz@kernel.org>

>> ---

>>  drivers/usb/serial/ftdi_sio.c | 9 +++++++++

>>  1 file changed, 9 insertions(+)

>> 

>> diff --git a/drivers/usb/serial/ftdi_sio.c 

>> b/drivers/usb/serial/ftdi_sio.c

>> index 13e575f16bcd..b9d3b33891fc 100644

>> --- a/drivers/usb/serial/ftdi_sio.c

>> +++ b/drivers/usb/serial/ftdi_sio.c

>> @@ -2012,6 +2012,15 @@ static int ftdi_gpio_init_valid_mask(struct 

>> gpio_chip *gc,

>> 

>>  	bitmap_complement(valid_mask, &map, ngpios);

>> 

>> +	if (bitmap_empty(valid_mask, ngpios))

>> +		dev_warn(&port->dev, "No usable GPIO\n");

> 

> This isn't an error of any kind, and certainly not something that

> deserves a warning in the system log on every probe. Not everyone cares

> about the GPIO interface.

> 

>> +	else

>> +		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",

>> +			 ngpios, valid_mask);

> 

> And while printing this mask has some worth I'm still reluctant to be

> spamming the logs with it. Just like gpolib has a dev_dbg() for

> registering chips, this should probably be demoted to KERN_DEBUG as

> well.

> 

>> +

>> +	if (!bitmap_full(valid_mask, ngpios))

>> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog 

>> to enable GPIOs if required\n");

>> +

> 

> And again, this is not something that belongs in the logs of just about

> every system with an attached ftdi device.


Fine by me, this patch can be dropped without issue. After all,
I now know how to deal with these chips.

> While not possible to combine with the valid_mask approach, this is

> something which we could otherwise add to the request() callback for 

> the

> first request that fails due to the mux configuration.


That was Linus' initial suggestion. But I think a consistent user
API is more important than free advise in the kernel log.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
Johan Hovold Dec. 7, 2020, 3:08 p.m. UTC | #6
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.

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

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

> >> We address the issues in a number of ways:

> >> 

> >> - Stop reporting invalid GPIO lines as valid to userspace. It

> >>   definitely seems odd to do so. Instead, report the line as being

> >>   used, making the userspace interface a bit more consistent.

> >> 

> >> - Implement the init_valid_mask() callback in the ftdi_sio driver,

> >>   allowing it to report which lines are actually valid.

> >> 

> >> - As suggested by Linus, give an indication to the user of why some of

> >>   the GPIO lines are unavailable, and point them to a useful tool

> >>   (once per boot). It is a bit sad that there next to no documentation

> >>   on how to use these CBUS pins.

> > 

> > Don't be sad, Marc; write some documentation. ;)

> 

> I sure will, right after I have fixed the rest of the kernel bugs

> I have introduced. With a bit of luck, that's right after I finally

> kick the bucket.


Hear, hear.

Johan
Johan Hovold Dec. 7, 2020, 3:19 p.m. UTC | #7
On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:
> On 2020-12-07 14:29, Johan Hovold wrote:
> > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:

> >> +	if (!bitmap_full(valid_mask, ngpios))
> >> +		dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog 
> >> to enable GPIOs if required\n");
> >> +
> > 
> > And again, this is not something that belongs in the logs of just about
> > every system with an attached ftdi device.
> 
> Fine by me, this patch can be dropped without issue. After all,
> I now know how to deal with these chips.
> 
> > While not possible to combine with the valid_mask approach, this is
> > something which we could otherwise add to the request() callback for 
> > the
> > first request that fails due to the mux configuration.
> 
> That was Linus' initial suggestion. But I think a consistent user
> API is more important than free advise in the kernel log.

I tend to agree. So since your valid-mask approach clearly has some
merit in that it marks the lines in use when using the new cdev
interface, perhaps we should stick with that.

Johan
Marc Zyngier Dec. 7, 2020, 3:34 p.m. UTC | #8
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...
Johan Hovold Dec. 7, 2020, 3:49 p.m. UTC | #9
On Mon, Dec 07, 2020 at 03:34:23PM +0000, Marc Zyngier wrote:
> 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.


My point was that this is how *all* gpio drivers work, and that muxing
is somewhat orthogonal to the gpio controller implementation.

Not sure how you would even "fix" that since muxing can often be changed
at runtime while the number of lines is typically a hardware feature
(which we report to the user). The resource is still there but it may
not be available for use.

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


Fair enough.

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


I agree, and your valid-mask approach takes care of the static mux-
configuration case nicely.

Johan
Linus Walleij Dec. 9, 2020, 9:28 a.m. UTC | #10
On Fri, Dec 4, 2020 at 5:47 PM Marc Zyngier <maz@kernel.org> wrote:

> Since it is pretty common for only some of the CBUS lines to be

> valid as GPIO lines, let's report such validity to the rest of

> the kernel.

>

> Signed-off-by: Marc Zyngier <maz@kernel.org>


Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


Yours,
Linus Walleij
Linus Walleij Dec. 9, 2020, 9:35 a.m. UTC | #11
On Mon, Dec 7, 2020 at 4:19 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:

> > On 2020-12-07 14:29, Johan Hovold wrote:

> > > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:

>

> > >> +  if (!bitmap_full(valid_mask, ngpios))

> > >> +          dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog

> > >> to enable GPIOs if required\n");

> > >> +

> > >

> > > And again, this is not something that belongs in the logs of just about

> > > every system with an attached ftdi device.

> >

> > Fine by me, this patch can be dropped without issue. After all,

> > I now know how to deal with these chips.

> >

> > > While not possible to combine with the valid_mask approach, this is

> > > something which we could otherwise add to the request() callback for

> > > the

> > > first request that fails due to the mux configuration.

> >

> > That was Linus' initial suggestion. But I think a consistent user

> > API is more important than free advise in the kernel log.

>

> I tend to agree. So since your valid-mask approach clearly has some

> merit in that it marks the lines in use when using the new cdev

> interface, perhaps we should stick with that.


It sounds like we agree that this patch sans prints is acceptable.

It makes things better so let's go with that.

The problem for the user is that the line looks to be
"used by the kernel" (true in some sense) but they have no
idea what to do about it and that the ftx-prog will solve
their hacking problem.

It's a matter of taste admittedly, I have noticed that some
subsystem maintainers are "dmesg minimalists" and want
as little as possible in dmesg while some are "dmesg maximalists"
and want as much messages and help as possible for
users in dmesg. I tend toward the latter but it's not like
I don't see the beauty and feeling of control that comes
with a clean dmesg.

My usual argument is that different loglevels exist for a
reason and those who don't want advice can just filter out
anything but errors or worse. But it seems they don't really wanna
hear that because on their pet systems KERN_INFO it is on
by default so it bothers them.

Yours,
Linus Walleij
Johan Hovold Dec. 9, 2020, 3:42 p.m. UTC | #12
On Wed, Dec 09, 2020 at 10:20:38AM +0100, Linus Walleij wrote:
> On Mon, Dec 7, 2020 at 4:48 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Dec 07, 2020 at 03:34:23PM +0000, Marc Zyngier wrote:
> 
> > > 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.
> >
> > My point was that this is how *all* gpio drivers work, and that muxing
> > is somewhat orthogonal to the gpio controller implementation.
> 
> This is true. It's because it is orthogonal that the separate subsystem
> for pin control including pin muxing exists.
> 
> Should I be really overly picky, the drivers that can mux lines like
> this should be implementing the pin control mux driver side as
> well just to make Linux aware of this. But if the muxing cannot
> be changed by the kernel (albeit with special tools) then it would
> be pretty overengineered for this case. Things would be much
> easier if this wasn't some flashing configuration but more of a
> runtime thing (which is kind of the implicit assumption in pin
> control land).

We'd still have problem of how to configure these hot-pluggable devices
at runtime, so it's not necessarily easier.

If I remember correctly the xr_serial driver under review is doing
something like muxing at runtime, but by simply having whichever
interface (tty or gpio) that claims the resource first implicitly set
the mux configuration. I have to revisit that.

> We don't really have many drivers that are "muxable by
> (intrusive) flashing" as opposed to "muxable by setting some
> bits" so in that way these FTDI drivers and siblings are special.

Yeah, but the gpio-reserved-range (valid-mask) feature which Marc used
comes close here.

Johan
Johan Hovold Dec. 9, 2020, 5:05 p.m. UTC | #13
On Wed, Dec 09, 2020 at 10:35:53AM +0100, Linus Walleij wrote:
> On Mon, Dec 7, 2020 at 4:19 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Dec 07, 2020 at 03:00:37PM +0000, Marc Zyngier wrote:
> > > On 2020-12-07 14:29, Johan Hovold wrote:
> > > > On Fri, Dec 04, 2020 at 04:47:38PM +0000, Marc Zyngier wrote:
> >
> > > >> +  if (!bitmap_full(valid_mask, ngpios))
> > > >> +          dev_warn_once(&port->dev, "Consider using a tool such as ftx-prog
> > > >> to enable GPIOs if required\n");
> > > >> +
> > > >
> > > > And again, this is not something that belongs in the logs of just about
> > > > every system with an attached ftdi device.
> > >
> > > Fine by me, this patch can be dropped without issue. After all,
> > > I now know how to deal with these chips.
> > >
> > > > While not possible to combine with the valid_mask approach, this is
> > > > something which we could otherwise add to the request() callback for
> > > > the
> > > > first request that fails due to the mux configuration.
> > >
> > > That was Linus' initial suggestion. But I think a consistent user
> > > API is more important than free advise in the kernel log.
> >
> > I tend to agree. So since your valid-mask approach clearly has some
> > merit in that it marks the lines in use when using the new cdev
> > interface, perhaps we should stick with that.
> 
> It sounds like we agree that this patch sans prints is acceptable.
> 
> It makes things better so let's go with that.

Sounds good.

I'm about to apply patches 2, 3 and 4 with some smaller changes like
demoting the printk messages to KERN_DEBUG and dropping the ftx-progs
warning.

> The problem for the user is that the line looks to be
> "used by the kernel" (true in some sense) but they have no
> idea what to do about it and that the ftx-prog will solve
> their hacking problem.

Right, it's not ideal, but the datasheets for these devices clearly
states that the configuration of the CBUS pins is done in EEPROM and the
vendor provides some tool to do that. Then there's a bunch of open
source implementations for the same including ftx-progs (which can only
be used for a subset of these devices).

I'd be fine with a dev_err() on the first request that fails saying that
the CBUS pin is not configured for GPIO use (perhaps even on every
request if its not something that a non-root user can trigger). But we
cannot have both that and have the line marked in-use through the
chardev interface currently.

I'm admittedly a bit torn on which is preferable.

Johan