Re: [PATCH] USB: serial: ftdi_sio: Add support for CBUS GPIO
Thanks Andy,
On 1 August 2018 at 18:08, Andy Shevchenko wrote:
> On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain wrote:
>> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
>> pins, allowing host to control them via simple USB control transfers.
>> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
>> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
>> USB to Serial function.
>>
>> In this implementation, a GPIO controller is registered on FTDI probe
>> if at least one CBUS pin is configured for I/O mode. For now, only
>> FTX devices are supported.
>>
>
>> This patch is based on previous Stefan Agner implementation tentative
>> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).
>
> The best approach to refer to a mail is to put a message-id, something like
>
> (... [1].)
>
> [1]: Message-Id: <...message-id...>
>
>> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
>> + void *val, size_t bytes)
>> +{
>
>> + if (bytes % 2) /* 16-bit eeprom */
>> + return -EINVAL;
>
> Why is it fatal?
> You may read one byte less (and provide an error code like -EIO, or
> whatever fits better) or more (and provide exact amount asked).
Yes, indeed.
>
>> + return 0;
>> +}
>
>> + rv = ftdi_read_pins(fgc->port, );
>> + if (rv)
>> + dev_err(>dev, "Unable to read CBUS GPIO pins, %d\n",
>> rv);
>
> Why not to return an error?
>
> (Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.
Will do.
>
>> + rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask,
>> FTDI_SIO_BITMODE_CBUS);
>> + if (rv)
>> + dev_err(>dev, "Unable set CBUS Bit-Bang mode, %d\n",
>> rv);
>
> Ditto WRT message.
yes
>
>> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> + struct usb_device *udev = port->serial->dev;
>> + struct ftdi_gpiochip *fgc;
>> + int rv;
>> +
>
>> + fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
>> + if (!fgc)
>> + return -ENOMEM;
>
> devm_ ?
>
>> + cbus_mux = kmalloc(4, GFP_KERNEL);
>> + if (!cbus_mux)
>> + return -ENOMEM;
>
> Is it mandatory to alloc so small amount on heap in this case?
Yes, because it is used as USB transfer buffer (DMA...) which can not
be on the stack.
>
>> + /* CBUS pins are individually configurable via 8-bit MUX
>> control
>> +* value, living at 0x1a for CBUS0. cf application note
>> AN_201.
>> +*/
>
> Is it a comment style used in this file?
>
>> + rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
>
> 0x1a is a magic.
>
>> + rv = gpiochip_add_data(>gc, fgc);
>> + if (rv) {
>> + dev_err(>dev, "Unable to add gpiochip\n");
>> + kfree(fgc);
>> + return rv;
>> + }
>
> devm_ ?
>
>> + return 0;
>> +}
>> +
>> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
>> +{
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> + struct ftdi_gpiochip *fgc = priv->fgc;
>> +
>
>> + if (fgc) {
>
> How you can be here when fgc == NULL?!
>
>> + gpiochip_remove(>gc);
>> + kfree(fgc);
>> + }
>> +}
>
> I guess complete function will go away when you switch to devm.
>
>
> --
> With Best Regards,
> Andy Shevchenko
Regards,
Loic
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: serial: ftdi_sio: Add support for CBUS GPIO
On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain wrote:
> Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
> pins, allowing host to control them via simple USB control transfers.
> To make use of a CBUS pin in Bit Bang mode, the pin must be configured
> to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
> USB to Serial function.
>
> In this implementation, a GPIO controller is registered on FTDI probe
> if at least one CBUS pin is configured for I/O mode. For now, only
> FTX devices are supported.
>
> This patch is based on previous Stefan Agner implementation tentative
> on LKML ([PATCH 0/2] FTDI CBUS GPIO support).
The best approach to refer to a mail is to put a message-id, something like
(... [1].)
[1]: Message-Id: <...message-id...>
> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
> + void *val, size_t bytes)
> +{
> + if (bytes % 2) /* 16-bit eeprom */
> + return -EINVAL;
Why is it fatal?
You may read one byte less (and provide an error code like -EIO, or
whatever fits better) or more (and provide exact amount asked).
> + return 0;
> +}
> + rv = ftdi_read_pins(fgc->port, );
> + if (rv)
> + dev_err(>dev, "Unable to read CBUS GPIO pins, %d\n",
> rv);
Why not to return an error?
(Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.
> + rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask,
> FTDI_SIO_BITMODE_CBUS);
> + if (rv)
> + dev_err(>dev, "Unable set CBUS Bit-Bang mode, %d\n",
> rv);
Ditto WRT message.
> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_device *udev = port->serial->dev;
> + struct ftdi_gpiochip *fgc;
> + int rv;
> +
> + fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
> + if (!fgc)
> + return -ENOMEM;
devm_ ?
> + cbus_mux = kmalloc(4, GFP_KERNEL);
> + if (!cbus_mux)
> + return -ENOMEM;
Is it mandatory to alloc so small amount on heap in this case?
> + /* CBUS pins are individually configurable via 8-bit MUX
> control
> +* value, living at 0x1a for CBUS0. cf application note
> AN_201.
> +*/
Is it a comment style used in this file?
> + rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
0x1a is a magic.
> + rv = gpiochip_add_data(>gc, fgc);
> + if (rv) {
> + dev_err(>dev, "Unable to add gpiochip\n");
> + kfree(fgc);
> + return rv;
> + }
devm_ ?
> + return 0;
> +}
> +
> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct ftdi_gpiochip *fgc = priv->fgc;
> +
> + if (fgc) {
How you can be here when fgc == NULL?!
> + gpiochip_remove(>gc);
> + kfree(fgc);
> + }
> +}
I guess complete function will go away when you switch to devm.
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: serial: ftdi_sio: Add support for CBUS GPIO
Some FTDI devices like FTX or FT232R support CBUS Bit Bang mode on CBUS
pins, allowing host to control them via simple USB control transfers.
To make use of a CBUS pin in Bit Bang mode, the pin must be configured
to I/O mode in the FTDI EEPROM. This mode perfectly coexists with regular
USB to Serial function.
In this implementation, a GPIO controller is registered on FTDI probe
if at least one CBUS pin is configured for I/O mode. For now, only
FTX devices are supported.
This patch is based on previous Stefan Agner implementation tentative
on LKML ([PATCH 0/2] FTDI CBUS GPIO support).
Signed-off-by: Loic Poulain
---
drivers/usb/serial/Kconfig| 9 ++
drivers/usb/serial/ftdi_sio.c | 222 ++
drivers/usb/serial/ftdi_sio.h | 83
3 files changed, 314 insertions(+)
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 533f127..64c9f2e 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -181,6 +181,15 @@ config USB_SERIAL_FTDI_SIO
To compile this driver as a module, choose M here: the
module will be called ftdi_sio.
+config USB_SERIAL_FTDI_SIO_CBUS_GPIO
+ bool "USB FDTI CBUS GPIO support"
+ depends on USB_SERIAL_FTDI_SIO
+ depends on GPIOLIB
+ help
+ Say yes here to add support for the CBUS bit-bang mode, allowing CBUS
+ pins to act as GPIOs. Note that pins must first be configured for GPIO
+ in the device's EEPROM. The FT232R and FT-X series support this mode.
+
config USB_SERIAL_VISOR
tristate "USB Handspring Visor / Palm m50x / Sony Clie Driver"
help
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b5cef32..3cfb5fd 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -40,12 +41,19 @@
#include
#include
#include
+
#include "ftdi_sio.h"
#include "ftdi_sio_ids.h"
#define DRIVER_AUTHOR "Greg Kroah-Hartman , Bill Ryder
, Kuba Ober , Andreas Mohr, Johan Hovold
"
#define DRIVER_DESC "USB FTDI Serial Converters Driver"
+struct ftdi_gpiochip {
+ struct gpio_chip gc;
+ unsigned int cbus_map[4];
+ unsigned long cbus_mask;
+ struct usb_serial_port *port;
+};
struct ftdi_private {
enum ftdi_chip_type chip_type;
@@ -72,6 +80,8 @@ struct ftdi_private {
unsigned int latency; /* latency setting in use */
unsigned short max_packet_size;
struct mutex cfg_lock; /* Avoid mess by parallel calls of config
ioctl() and change_speed() */
+
+ struct ftdi_gpiochip *fgc;
};
/* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1528,6 +1538,211 @@ static int get_lsr_info(struct usb_serial_port *port,
return 0;
}
+#ifdef CONFIG_USB_SERIAL_FTDI_SIO_CBUS_GPIO
+
+static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
+ void *val, size_t bytes)
+{
+ if (bytes % 2) /* 16-bit eeprom */
+ return -EINVAL;
+
+ while (bytes) {
+ int rv;
+
+ rv = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+FTDI_SIO_READ_EEPROM_REQUEST,
+FTDI_SIO_READ_EEPROM_REQUEST_TYPE,
+0, off / 2, val, 2, WDR_TIMEOUT);
+ if (rv < 0)
+ return rv;
+
+ off += 2;
+ val += 2;
+ bytes -= 2;
+ }
+
+ return 0;
+}
+
+static int ftdi_set_bitmode(struct usb_serial_port *port, u8 bitmask,
+ u8 bitmode)
+{
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ __u16 urb_value = 0;
+
+ urb_value = bitmode << 8 | bitmask;
+
+ if (usb_control_msg(port->serial->dev,
+usb_sndctrlpipe(port->serial->dev, 0),
+FTDI_SIO_SET_BITMODE_REQUEST,
+FTDI_SIO_SET_BITMODE_REQUEST_TYPE,
+urb_value, priv->interface,
+NULL, 0, WDR_SHORT_TIMEOUT) < 0) {
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int ftdi_read_pins(struct usb_serial_port *port, u8 *val)
+{
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ unsigned char *buf;
+
+ buf = kmalloc(1, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ if (usb_control_msg(port->serial->dev,
+usb_rcvctrlpipe(port->serial->dev, 0),
+FTDI_SIO_READ_PINS_REQUEST,
+FTDI_SIO_READ_PINS_REQUEST_TYPE,
+0, priv->interface, buf, 1, WDR_TIMEOUT) < 0) {
+ kfree(buf);
+ return -EIO;
+ }
+
+
