| Message ID | 20180930122703.7115-1-johan@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: serial: gpio line-name fix and FT232R CBUS gpio support | expand |
On Sun, Sep 30, 2018 at 2:29 PM Johan Hovold <johan@kernel.org> wrote: > Turns out gpiolib still doesn't like having non-unique line names, so > drop the line names from the recently added FTX cbus gpio > implementation before adding support also for FT232R. Oh. > Linus, we finally got around to adding gpio support for FTDI devices; > see commit > > ba93cc7da896 ("USB: serial: ftdi_sio: implement GPIO support for FT-X devices") > > in my usb-next branch (and linux-next). This is good news, I think it's a pretty neat way for people to get a few inexpensive GPIOs from their serial adapters. > The gpiolib warnings and inability to use the legacy sysfs interface > prevents us from setting the line names however as someone is bound to > plugin more than one of these devices at some point. I think we > discussed this issue with the name space and hotpluggable devices a few > years ago, but looks like this topic may need to be revisited. Hm I guess the right long-term fix is to allow per-gpiochip unique names rather than enforcing globally unique names. The idea is to make it possible for userspace to look up a GPIO on a chip by name, so if the gpiochip has a unique name, and the line name is unique on that chip it should be good enough. Yours, Linus Walleij
On Mon, Oct 01, 2018 at 11:43:55AM +0200, Linus Walleij wrote: > On Sun, Sep 30, 2018 at 2:29 PM Johan Hovold <johan@kernel.org> wrote: > > Linus, we finally got around to adding gpio support for FTDI devices; > > see commit > > > > ba93cc7da896 ("USB: serial: ftdi_sio: implement GPIO support for FT-X devices") > > > > in my usb-next branch (and linux-next). > > This is good news, I think it's a pretty neat way for people to get > a few inexpensive GPIOs from their serial adapters. > > > The gpiolib warnings and inability to use the legacy sysfs interface > > prevents us from setting the line names however as someone is bound to > > plugin more than one of these devices at some point. I think we > > discussed this issue with the name space and hotpluggable devices a few > > years ago, but looks like this topic may need to be revisited. > > Hm I guess the right long-term fix is to allow per-gpiochip unique > names rather than enforcing globally unique names. Indeed. > The idea is to make it possible for userspace to look up a GPIO > on a chip by name, so if the gpiochip has a unique name, > and the line name is unique on that chip it should be good > enough. I haven't really had time do dig into this again, but is this also an issue with the chardev interface? I thought this was one of the things you wanted to get right with the new interface, so hopefully that's already taken care of. If the flat name space is only an issue with the legacy interface we might get away with simply not using the line names in sysfs when a new chip flag is set (unless we can resue .can_sleep, but there seems to be some i2c devices already using line names). Thanks, Johan
On Fri, Oct 5, 2018 at 3:40 PM Johan Hovold <johan@kernel.org> wrote: > On Mon, Oct 01, 2018 at 11:43:55AM +0200, Linus Walleij wrote: > > The idea is to make it possible for userspace to look up a GPIO > > on a chip by name, so if the gpiochip has a unique name, > > and the line name is unique on that chip it should be good > > enough. > > I haven't really had time do dig into this again, but is this also an > issue with the chardev interface? > > I thought this was one of the things > you wanted to get right with the new interface, so hopefully that's > already taken care of. It is always possible to use /dev/gpiochipN and offet M on that gpiochip to pick a unique line. That is a unique offset on a unique chip. The gpiochip also has a "label" which may be something user-readable such as part numer, but the unique string is its name such as "gpiochip0". For symbolic look-up though, like a file has to have a unique path, the name of the line should be unique. It is allowed to have unnamed lines though, so it is only a hint. While I would like all drivers to uniquely name their lines in DT or ACPI, or using the .names array in the gpio_chip struct, it cannot be enforced because of legacy support, so many old systems had no names specified, and the DT and ACPI properties to name lines were introduced after the chardev actually. All I enforce is that if names are added, they should be unique. What we *could* do is conjure a unique name per line if the hardware description doesn't provice one... like "line0", "line1", "line2"... "lineN" on the chip. Should we add a patch like that? The only side effect I can see if that maybe the footprint increase is not so nice. I had it in mind but it slipped of my radar. It would make all lines always have unique names. > If the flat name space is only an issue with the legacy interface we > might get away with simply not using the line names in sysfs when a new > chip flag is set (unless we can resue .can_sleep, but there seems to be > some i2c devices already using line names). Hm. So you mean it is bad that the exporting GPIOs in the old sysfs brings out the line name in sysfs if the line is named. I just want the sysfs to die, but yeah what you say makes sense. I don't know if it's such a big issue, my focus has been on making the chardev more appetizing and the sysfs uncomfortably oldschool amongst users. This would make it even more uncomfortable. But I don't know if the old sysfs users actually use the line names much? Yours, Linus Walleij
Hi Linus, Sorry about the late reply, this one got buried in the mailbox during conference travels. On Wed, Oct 10, 2018 at 11:36:28AM +0200, Linus Walleij wrote: > On Fri, Oct 5, 2018 at 3:40 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Oct 01, 2018 at 11:43:55AM +0200, Linus Walleij wrote: > > > > The idea is to make it possible for userspace to look up a GPIO > > > on a chip by name, so if the gpiochip has a unique name, > > > and the line name is unique on that chip it should be good > > > enough. > > > > I haven't really had time do dig into this again, but is this also an > > issue with the chardev interface? > > > > I thought this was one of the things > > you wanted to get right with the new interface, so hopefully that's > > already taken care of. > > It is always possible to use /dev/gpiochipN and offet M on > that gpiochip to pick a unique line. That is a unique offset > on a unique chip. The gpiochip also has a "label" which may > be something user-readable such as part numer, but the unique > string is its name such as "gpiochip0". Right. > For symbolic look-up though, like a file has to have a unique > path, the name of the line should be unique. Sure, but there's no way to guarantee that (consider pluggable devices) unless you encode the topology in the name. > It is allowed to have unnamed lines though, so it is only a > hint. While I would like all drivers to uniquely name their > lines in DT or ACPI, or using the .names array in the gpio_chip > struct, it cannot be enforced because of legacy support, > so many old systems had no names specified, and > the DT and ACPI properties to name lines were introduced > after the chardev actually. Oh, that's right. > All I enforce is that if names are added, they should be > unique. Which effectively means you cannot have a driver provide default line names, as that would cause conflicts if there are ever more than one such device in a system (be it i2c or pluggable USB). > What we *could* do is conjure a unique name per line if > the hardware description doesn't provice one... like > "line0", "line1", "line2"... "lineN" on the chip. > > Should we add a patch like that? The only side effect > I can see if that maybe the footprint increase is not so > nice. > > I had it in mind but it slipped of my radar. It would make > all lines always have unique names. No, I don't think that's needed or desirable. But take the concrete use case at hand; a gpio controller connected over USB. These FTDI devices exposes four gpios on a set of pins that differ from model to model. On some devices these lines are available on a set of pins named CBUS0..CBUS3, on others a different set of pins such as ACBUS5, ACBUS6, ACBUS8 and ACBUS9 are used. Exporting which line is exposed on which pin obviously has some worth, but this is currently not possible due to the requirement that line names must be unique. Having an interface that allows you to look up say ACBUS6 given a particular gpiochip would also be useful to have (user space knows which USB device it is interested in so finding the gpiochip is straight forward). Perhaps the chardev interface could return a set of matched gpiochips if a particular line name is found on several chips for user space to iterate over. Essentially what I'm going for is to have the line names only be required to be unique per chip, and let user space deal with any collisions. The flat line name space really only works for something like DT, and then only if you actually have access to the DTS (I'm assuming there's no uniqueness requirement in the binding docs?). But you could still run into trouble if an i2c driver provides default names, or if you use anything pluggable. > > If the flat name space is only an issue with the legacy interface we > > might get away with simply not using the line names in sysfs when a new > > chip flag is set (unless we can resue .can_sleep, but there seems to be > > some i2c devices already using line names). > > Hm. So you mean it is bad that the exporting GPIOs in the old > sysfs brings out the line name in sysfs if the line is named. Right, this specifically prevents using line names with pluggable devices. > I just want the sysfs to die, but yeah what you say makes > sense. I don't know if it's such a big issue, my focus has been > on making the chardev more appetizing and the sysfs > uncomfortably oldschool amongst users. This would make it > even more uncomfortable. > > But I don't know if the old sysfs users actually use the line > names much? The problem is that the line names are used in sysfs as device names (file names) instead of "gpioN" whenever a line name has been specified. So on systems with line names defined, we would definitely risk regressions if we simply stopped exporting gpios using those names. Doing so for a subset of devices (e.g. i2c, or anything connected over a slow bus) may still be considered though. That would also deal with the USB case. But again, I'm not sure if we'd run into other problems with the chardev interface (e.g. what happens if I lookup "CBUS3" and there are more than one match?). Thanks, Johan
Turns out gpiolib still doesn't like having non-unique line names, so drop the line names from the recently added FTX cbus gpio implementation before adding support also for FT232R. Linus, we finally got around to adding gpio support for FTDI devices; see commit ba93cc7da896 ("USB: serial: ftdi_sio: implement GPIO support for FT-X devices") in my usb-next branch (and linux-next). The gpiolib warnings and inability to use the legacy sysfs interface prevents us from setting the line names however as someone is bound to plugin more than one of these devices at some point. I think we discussed this issue with the name space and hotpluggable devices a few years ago, but looks like this topic may need to be revisited. Thanks, Johan Johan Hovold (2): USB: serial: ftdi_sio: fix gpio name collisions USB: serial: ftdi_sio: add support for FT232R CBUS gpios drivers/usb/serial/ftdi_sio.c | 45 +++++++++++++++++++++++++++++------ drivers/usb/serial/ftdi_sio.h | 3 ++- 2 files changed, 40 insertions(+), 8 deletions(-)