| Message ID | 20201204164739.781812-4-maz@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | USB: ftdio_sio: GPIO validity fixes | expand |
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
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.
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
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
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
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 --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; }
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(+)