[PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[Johan Hovold]

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(-)

-- 
2.19.0

[PATCH 1/2] USB: serial: ftdi_sio: fix gpio name collisions

[Johan Hovold]

Drop the gpio line names, which cause gpiolib to complain loudly
whenever a second ftdi gpiochip is registered:

	gpio gpiochip5: Detected name collision for GPIO name 'CBUS0'
	gpio gpiochip5: Detected name collision for GPIO name 'CBUS1'
	gpio gpiochip5: Detected name collision for GPIO name 'CBUS2'
	gpio gpiochip5: Detected name collision for GPIO name 'CBUS3'

and also prevents the legacy sysfs interface from being used (as the
line names are used as device names whenever they are set):

	sysfs: cannot create duplicate filename '/class/gpio/CBUS0'

Until non-unique names are supported by gpiolib (without warnings and
stack dumps), let's leave the gpio lines unnamed.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 6b727ada20cf..be50b2a200aa 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1778,10 +1778,6 @@ static void remove_sysfs_attrs(struct usb_serial_port *port)
 
 #ifdef CONFIG_GPIOLIB
 
-static const char * const ftdi_ftx_gpio_names[] = {
-	"CBUS0", "CBUS1", "CBUS2", "CBUS3"
-};
-
 static int ftdi_set_bitmode(struct usb_serial_port *port, u8 mode)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
@@ -2032,7 +2028,6 @@ static int ftx_gpioconf_init(struct usb_serial_port *port)
 
 	/* FIXME: FT234XD alone has 1 GPIO, but how to recognize this IC? */
 	priv->gc.ngpio = 4;
-	priv->gc.names = ftdi_ftx_gpio_names;
 
 	/* Determine which pins are configured for CBUS bitbanging */
 	priv->gpio_altfunc = 0xff;
-- 
2.19.0

[PATCH 2/2] USB: serial: ftdi_sio: add support for FT232R CBUS gpios

[Johan Hovold]

Enable support for cbus gpios on FT232R. The cbus configuration is
stored in one word in the EEPROM at offset 0x0a (byte-offset 0x14) with
the mux config for CBUS0, CBUS1, CBUS2 and CBUS3 in bits 0..3, 4..7,
8..11 and 12..15, respectively.

Tested using FT232RL by configuring one cbus pin at a time.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 40 +++++++++++++++++++++++++++++++++--
 drivers/usb/serial/ftdi_sio.h |  3 ++-
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index be50b2a200aa..f1eb20acb3bb 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2007,7 +2007,40 @@ static int ftdi_read_eeprom(struct usb_serial *serial, void *dst, u16 addr,
 	return 0;
 }
 
-static int ftx_gpioconf_init(struct usb_serial_port *port)
+static int ftdi_gpio_init_ft232r(struct usb_serial_port *port)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	u16 cbus_config;
+	u8 *buf;
+	int ret;
+	int i;
+
+	buf = kmalloc(2, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = ftdi_read_eeprom(port->serial, buf, 0x14, 2);
+	if (ret < 0)
+		goto out_free;
+
+	cbus_config = le16_to_cpup((__le16 *)buf);
+	dev_dbg(&port->dev, "cbus_config = 0x%04x\n", cbus_config);
+
+	priv->gc.ngpio = 4;
+
+	priv->gpio_altfunc = 0xff;
+	for (i = 0; i < priv->gc.ngpio; ++i) {
+		if ((cbus_config & 0xf) == FTDI_FT232R_CBUS_MUX_GPIO)
+			priv->gpio_altfunc &= ~BIT(i);
+		cbus_config >>= 4;
+	}
+out_free:
+	kfree(buf);
+
+	return ret;
+}
+
+static int ftdi_gpio_init_ftx(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct usb_serial *serial = port->serial;
@@ -2049,8 +2082,11 @@ static int ftdi_gpio_init(struct usb_serial_port *port)
 	int result;
 
 	switch (priv->chip_type) {
+	case FT232RL:
+		result = ftdi_gpio_init_ft232r(port);
+		break;
 	case FTX:
-		result = ftx_gpioconf_init(port);
+		result = ftdi_gpio_init_ftx(port);
 		break;
 	default:
 		return 0;
diff --git a/drivers/usb/serial/ftdi_sio.h b/drivers/usb/serial/ftdi_sio.h
index 6cfe682f8348..a79a1325b4d9 100644
--- a/drivers/usb/serial/ftdi_sio.h
+++ b/drivers/usb/serial/ftdi_sio.h
@@ -457,7 +457,8 @@ enum ftdi_sio_baudrate {
 #define FTDI_SIO_READ_EEPROM_REQUEST_TYPE 0xc0
 #define FTDI_SIO_READ_EEPROM_REQUEST FTDI_SIO_READ_EEPROM
 
-#define FTDI_FTX_CBUS_MUX_GPIO		8
+#define FTDI_FTX_CBUS_MUX_GPIO		0x8
+#define FTDI_FT232R_CBUS_MUX_GPIO	0xa
 
 
 /* Descriptors returned by the device
-- 
2.19.0

Re: [PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[Linus Walleij]

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

Re: [PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[Johan Hovold]

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

Re: [PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[Linus Walleij]

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

Re: [PATCH 0/2] USB: serial: gpio line-name fix and FT232R CBUS gpio support

[Johan Hovold]

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