diff mbox series

[3/4] USB: serial: ftdi_sio: Log the CBUS GPIO validity

Message ID 20201204164739.781812-4-maz@kernel.org
State New
Headers show
Series [1/4] gpiolib: cdev: Flag invalid GPIOs as used | expand

Commit Message

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

Comments

Johan Hovold Dec. 7, 2020, 2:29 p.m. UTC | #1
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, 3 p.m. UTC | #2
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.
Johan Hovold Dec. 7, 2020, 3:19 p.m. UTC | #3
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
Linus Walleij Dec. 9, 2020, 9:35 a.m. UTC | #4
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, 5:05 p.m. UTC | #5
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
Johan Hovold Dec. 9, 2020, 5:39 p.m. UTC | #6
On Wed, Dec 09, 2020 at 06:05:58PM +0100, Johan Hovold wrote:
> On Wed, Dec 09, 2020 at 10:35:53AM +0100, Linus Walleij wrote:


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


I've applied the patches now. Having this reported through the chardev
interface must be better than having to match up a failed request with
something in the system log.

Johan
diff mbox series

Patch

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");
+	else
+		dev_info(&port->dev, "Enabling CBUS%*pbl for GPIO\n",
+			 ngpios, valid_mask);
+
+	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");
+
 	return 0;
 }