Re: [PATCH v2] USB: serial: cp210x: Fix GPIO in autosuspend
>> On Di, 2019-03-19 at 11:36 +0100, Johan Hovold wrote: >> On Mon, Feb 18, 2019 at 10:17:12AM +0100, Oliver Neukum wrote: >>> On So, 2019-02-17 at 18:59 +0100, Karoly Pados wrote: >>>> Current GPIO code in cp210x fails to take USB autosuspend into account, >>>> making it practically impossible to use GPIOs with autosuspend enabled >>>> without user configuration. Fix this like for ftdi_sio in a previous patch. >>>> Tested on a CP2102N. >>> your patch is looking good to me, but I am afraid there are issues. >>> How do the GPIO lines on the device interact with USB reset and system >>> suspend? >> >> What was your concern here, Oliver? >> >> If you have a device resetting or losing power (for reset_resume) the >> GPIO lines will revert to the default. But that change is not reported >> to user space, is it? > > This driver doesn't support reset_resume() so that shouldn't be an > issue, right? We'd disconnect (deregister the gpiochip) and re-probe > instead. > >> So the original patch is correct, but there are more situations rather >> than suspend which could trigger the problem. > > This patch just added the missing auto-resume handling, but, yeah, > there may be devices out there for which things may get out of sync if > they lose state over suspend. I think Karoly confirmed this wasn't the > case with cp210x. > The way I tested this is very simple: I just toggled an output pin. If the device would lose its state during suspend, then the pin state would return to its factory default as soon the it went into suspend. Since the LED stayed stable over suspend (I connected an LED), it seems these devices don't lose state. I did not, however, test all device state in detail to see if every setting remained. I cannot vouch for whether *all* state is retained.
[PATCH v2] USB: serial: cp210x: Fix GPIO in autosuspend
Current GPIO code in cp210x fails to take USB autosuspend into account,
making it practically impossible to use GPIOs with autosuspend enabled
without user configuration. Fix this like for ftdi_sio in a previous patch.
Tested on a CP2102N.
Signed-off-by: Karoly Pados
---
Changelog:
- v2: Restrict new autopm calls to GPIO paths.
Always check result of usb_autopm_get.
Rebased to current usb_serial upstream.
drivers/usb/serial/cp210x.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index fac7a4547523..f7aaecad6e21 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1370,8 +1370,13 @@ static int cp210x_gpio_get(struct gpio_chip *gc,
unsigned int gpio)
if (priv->partnum == CP210X_PARTNUM_CP2105)
req_type = REQTYPE_INTERFACE_TO_HOST;
+ result = usb_autopm_get_interface(serial->interface);
+ if (result)
+ return result;
+
result = cp210x_read_vendor_block(serial, req_type,
CP210X_READ_LATCH, &buf, sizeof(buf));
+ usb_autopm_put_interface(serial->interface);
if (result < 0)
return result;
@@ -1392,6 +1397,10 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
buf.mask = BIT(gpio);
+ result = usb_autopm_get_interface(serial->interface);
+ if (result)
+ goto out;
+
if (priv->partnum == CP210X_PARTNUM_CP2105) {
result = cp210x_write_vendor_block(serial,
REQTYPE_HOST_TO_INTERFACE,
@@ -1399,7 +1408,6 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
sizeof(buf));
} else {
u16 wIndex = buf.state << 8 | buf.mask;
-
result = usb_control_msg(serial->dev,
usb_sndctrlpipe(serial->dev, 0),
CP210X_VENDOR_SPECIFIC,
@@ -1409,6 +1417,9 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
NULL, 0, USB_CTRL_SET_TIMEOUT);
}
+ usb_autopm_put_interface(serial->interface);
+
+out:
if (result < 0) {
dev_err(&serial->interface->dev, "failed to set GPIO value:
%d\n",
result);
--
2.20.1
Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend
Uhm, yes, sorry, it kind of went out of my head. I am doing a lot of travelling
lately (in the
past 48 hours I've been on 3 airplanes), and I had travels on earlier weeks
too, mixed with
some project releases and family celebrations. So my head is just somewhere
else.
Realistically, I'll submit something by the weekend when I'm home again.
Karoly
February 4, 2019 5:09 PM, "Johan Hovold" wrote:
> On Tue, Jan 15, 2019 at 11:29:42AM +0100, Johan Hovold wrote:
>
>> On Tue, Jan 15, 2019 at 10:26:07AM +0100, Johan Hovold wrote:
>> On Tue, Jan 15, 2019 at 09:17:58AM +, Karoly Pados wrote:
>>>> I think it's better to add the autopm call to gpio210x_gpio_get/set
>>>> only. This will allow for a simpler patch, and keeps the autopm handling
>>>> confined to the gpio paths.
>>>
>>> I'll submit a v2.
>>>
>>>>> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
>>>>> unsigned int gpio, int
>>>>> value)
>>>>> } else {
>>>>> u16 wIndex = buf.state << 8 | buf.mask;
>>>>>
>>>>> + usb_autopm_get_interface(serial->interface);
>>>>
>>>> Also make sure to always check for errors from autopm_get().
>>>
>>> I checked everywhere else, the reason I didn't check here is on
>>> purpose based on your previous feedback. The caller function here
>>> doesn't have a return value, so the only way to return errors is to
>>> log, but in my last patch to ftdi_sio you made clear that errors from
>>> autopm_get shouldn't get logged. Trying to call usb_control_msg() even
>>> though the device could not wake does not cause issues, and the return
>>> value from usb_control_msg() clearly identifies the reason for failure
>>> (failure due to autosuspend), so error information is not lost either.
>>> So I thought not checking here has no real disadvantage and I still
>>> stay conformant to your previous guidance.
>>
>> Ok, I understand your reasoning, but please do check for errors and bail
>> out early if autopm_get() fails. No need to log errors.
>>
>> Actually, we should probably add the missing error handling to the
>> callers and have gpio_set() propagate errors too. If you want to take a
>> stab at that, that could be a follow-on patch.
>
> Karoly, did you plan on sending a v2 of this one?
>
> Thanks,
> Johan
Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend
> I think it's better to add the autopm call to gpio210x_gpio_get/set
> only. This will allow for a simpler patch, and keeps the autopm handling
> confined to the gpio paths.
I'll submit a v2.
>> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
>> unsigned int gpio, int
>> value)
>> } else {
>> u16 wIndex = buf.state << 8 | buf.mask;
>>
>> + usb_autopm_get_interface(serial->interface);
>
> Also make sure to always check for errors from autopm_get().
I checked everywhere else, the reason I didn't check here is on purpose based
on your previous feedback. The caller function here doesn't have a return
value, so the only way to return errors is to log, but in my last patch to
ftdi_sio you made clear that errors from autopm_get shouldn't get logged.
Trying to call usb_control_msg() even though the device could not wake does not
cause issues, and the return value from usb_control_msg() clearly identifies
the reason for failure (failure due to autosuspend), so error information is
not lost either. So I thought not checking here has no real disadvantage and I
still stay conformant to your previous guidance.
[PATCH] USB: serial: cp210x: Fix GPIO in autosuspend
Current GPIO code in cp210x fails to take USB autosuspend into account,
making it practically impossible to use GPIOs with autosuspend enabled
without user configuration. Fix this like for ftdi_sio in a previous patch.
Tested on a CP2102N.
Signed-off-by: Karoly Pados
---
drivers/usb/serial/cp210x.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index c0777a374a88..8f974eabce63 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -598,9 +598,15 @@ static int cp210x_read_vendor_block(struct usb_serial
*serial, u8 type, u16 val,
void *dmabuf;
int result;
+ result = usb_autopm_get_interface(serial->interface);
+ if (result)
+ return result;
+
dmabuf = kmalloc(bufsize, GFP_KERNEL);
- if (!dmabuf)
+ if (!dmabuf) {
+ usb_autopm_put_interface(serial->interface);
return -ENOMEM;
+ }
result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
CP210X_VENDOR_SPECIFIC, type, val,
@@ -618,6 +624,7 @@ static int cp210x_read_vendor_block(struct usb_serial
*serial, u8 type, u16 val,
}
kfree(dmabuf);
+ usb_autopm_put_interface(serial->interface);
return result;
}
@@ -702,9 +709,15 @@ static int cp210x_write_vendor_block(struct usb_serial
*serial, u8 type,
void *dmabuf;
int result;
+ result = usb_autopm_get_interface(serial->interface);
+ if (result)
+ return result;
+
dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
- if (!dmabuf)
+ if (!dmabuf) {
+ usb_autopm_put_interface(serial->interface);
return -ENOMEM;
+ }
result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
CP210X_VENDOR_SPECIFIC, type, val,
@@ -712,6 +725,7 @@ static int cp210x_write_vendor_block(struct usb_serial
*serial, u8 type,
USB_CTRL_SET_TIMEOUT);
kfree(dmabuf);
+ usb_autopm_put_interface(serial->interface);
if (result == bufsize) {
result = 0;
@@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
} else {
u16 wIndex = buf.state << 8 | buf.mask;
+ usb_autopm_get_interface(serial->interface);
result = usb_control_msg(serial->dev,
usb_sndctrlpipe(serial->dev, 0),
CP210X_VENDOR_SPECIFIC,
@@ -1390,6 +1405,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
CP210X_WRITE_LATCH,
wIndex,
NULL, 0, USB_CTRL_SET_TIMEOUT);
+ usb_autopm_put_interface(serial->interface);
}
if (result < 0) {
--
2.20.1
[PATCH] USB: serial: ftdi_sio: Fix GPIO not working in autosuspend
There is a bug in the current GPIO code for ftdi_sio: it failed to take USB
autosuspend into account. If the device is in autosuspend, calls to
usb_control_msg() fail with -EHOSTUNREACH. Because the standard value for
autosuspend timeout is usually 2-5 seconds, this made it almost impossible
to use the GPIOs on machines that have USB autosuspend enabled. This patch
fixes the issue by acquiring a PM lock on the device for the duration of
the USB transfers. Tested on an FT231X device.
Signed-off-by: Karoly Pados
---
Please consider backporting to 4.20.x, otherwise the GPIO driver is not
really usable for anybody with USB autosuspend enabled (eg. many laptops),
at least not without manual configuration.
drivers/usb/serial/ftdi_sio.c | 21 -
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 1ab2a6191013..01813dce37f2 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1783,6 +1783,13 @@ static int ftdi_set_bitmode(struct usb_serial_port
*port, u8 mode)
int result;
u16 val;
+ result = usb_autopm_get_interface(port->serial->interface);
+ if (result) {
+ dev_err(&port->serial->interface->dev,
+ "Failed to wake device from autosuspend.\n");
+ return result;
+ }
+
val = (mode << 8) | (priv->gpio_output << 4) | priv->gpio_value;
result = usb_control_msg(serial->dev,
usb_sndctrlpipe(serial->dev, 0),
@@ -1795,6 +1802,8 @@ static int ftdi_set_bitmode(struct usb_serial_port *port,
u8 mode)
val, result);
}
+ usb_autopm_put_interface(port->serial->interface);
+
return result;
}
@@ -1846,9 +1855,18 @@ static int ftdi_read_cbus_pins(struct usb_serial_port
*port)
unsigned char *buf;
int result;
+ result = usb_autopm_get_interface(port->serial->interface);
+ if (result) {
+ dev_err(&port->serial->interface->dev,
+ "Failed to wake device from autosuspend.\n");
+ return result;
+ }
+
buf = kmalloc(1, GFP_KERNEL);
- if (!buf)
+ if (!buf) {
+ usb_autopm_put_interface(port->serial->interface);
return -ENOMEM;
+ }
result = usb_control_msg(serial->dev,
usb_rcvctrlpipe(serial->dev, 0),
@@ -1863,6 +1881,7 @@ static int ftdi_read_cbus_pins(struct usb_serial_port
*port)
}
kfree(buf);
+ usb_autopm_put_interface(port->serial->interface);
return result;
}
--
2.20.1
Re: [PATCH v5] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
Oops, I sent v4 again, so it had no changes at all. Can't I just resend v5 instead of calling it v6? September 24, 2018 10:48 AM, "Johan Hovold" wrote: > On Sun, Sep 23, 2018 at 06:03:30PM +0200, Karoly Pados wrote: > >> This patch allows using the CBUS pins of FT-X devices as GPIO in CBUS >> bitbanging mode. There is no conflict between the GPIO and VCP >> functionality in this mode. Tested on FT230X and FT231X. >> >> As there is no way to request the current CBUS register configuration >> from the device, all CBUS pins are set to a known state when the first >> GPIO is requested. This allows using libftdi to set the GPIO pins >> before loading this module for UART functionality, a behavior that >> existing applications might be relying upon (though no specific case >> is known to the authors of this patch). >> >> Signed-off-by: Karoly Pados >> --- >> Changelog: >> - v2: Fix compile error when CONFIG_GPIOLIB is not defined. >> - v3: Incorporate review feedback. >> - v4: Include linux/gpio/driver.h unconditionally. >> Replace and invert gpio_input with gpio_output. >> Make ftdi_gpio_direction_get return 0/1. >> Change dev_err msg in ftdi_set_bitmode_req. >> Change formatting of error checking in ftdi_gpio_get. >> Drop dev_err in ftdi_gpio_set. >> Remove some line breaks and empty lines. >> Change error handling in ftdi_read_eeprom (and adjust caller). >> Replace SIO->FTX in FTDI_SIO_CBUS_MUX_GPIO macro name. >> - v5: Read only 4 bytes from eeprom in ftx_gpioconf_init. >> Compare ftdi_read_eeprom result with 0 instead of eq. cehck. >> Reserve 4 GPIOs even for FT234X. >> Release CBUS after gpiochip deregister to avoid possible race. >> Adjust comment on FTDI_SIO_SET_BITMODE macro. >> Protect GPIO value/dir setting with mutex. > > This patch doesn't add any locking so I'm assuming you posted the wrong > version of the patch. > >> Add support for gpiochip.get_multiple and set_multiple. >> Add names to GPIO lines. > > I'll wait for v6. > > Thanks, > Johan
Re: [PATCH v4] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
>> + goto out_free;
>> +
>> + /* Chip-type guessing logic based on libftdi. */
>> + priv->gc.ngpio = 4; /* FT230X, FT231X */
>> + if (le16_to_cpu(serial->dev->descriptor.bcdDevice) != 0x1000)
>> + priv->gc.ngpio = 1; /* FT234XD */
>
> As I mentioned in my last mail: I've asked FTDI about this, but I fear
> that FTX234XD has bcdDevice 0x1000 and we may need to just always
> register all four pins after all.
>
To avoid missing 4.20, what is the latest time I should wait for FTDI's answer?
Or should I just submit v5 as it is now and you'll incorporate FTDI's feedback
when you receive it?
>> +static void ftdi_gpio_remove(struct usb_serial_port *port)
>> +{
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +
>> + if (priv->gpio_used) {
>> + /* Remark: Exiting CBUS-mode does not reset pin states too */
>> + ftdi_exit_cbus_mode(port);
>> + priv->gpio_used = false;
>> + }
>
> This should go after deregistration or we have a tiny race window here.
Can you elaborate on that to make sure I get it right?
By "deregistration" do you mean deregistering the GPIO chip below in the same
method?
Does that mean something can call into our module while this method is running?
If not, I'm clueless about the possible race here.
Re: [PATCH v2] USB: serial: cp210x: Implement GPIO support for CP2102N
Thank you very much for squeezing this into your schedule. I've also become a lot more experienced now in contributing to the kernel, concerning both planning and code. Next time things should work out even smoother. Wish you a great and rewarding vacation! Karoly July 20, 2018 12:16 PM, "Johan Hovold" wrote: > On Wed, Jul 18, 2018 at 09:38:12PM +, Karoly Pados wrote: > >> Hello Johan, >> >> I hope maybe after you come back from your vacation you can still >> merge this into 4.19, I guess the merge window will still be open. > > You shouldn't make assumptions about peoples vacation plans. ;) > >> Given that there are >> about another 5 weeks until 4.19rc1 merge window closes, >> I didn't think I'd be late with this patch, I am sorry. > > New code must be ready and reviewed well in time before the merge window > *opens*. So generally you cannot expect anything you post this week to > be included in 4.19. > >> However, as already explained to you, it'd be very important >> for me to get this patch at least into 4.19, as otherwise the >> time I loose is much greater than a single release cycle (due >> to distro cycles), and I have actually a crowdfunding product >> depending on this. > > I know, and I already cut some corners for you with the baud-rate > handling changes even though this is strictly not a valid reason to > change the normal procedures. > > That being said, I took a look at your gpio patch today and as it looks > like you've addressed the major concerns with v1, and done so in a clean > way, I'll make an exception. > > I'll post some review comments, but I've already fixed up the patch > myself. It's mostly style issues, but I did also find two bugs. > > I do need you to respin the commit message, though; a commit message > should be self-contained and not rely on external discussions to make > sense (you can of course still refer/link to it). Patch-revision info > should generally go below the cut-off line, unless it is useful to have > it recorded. > > Can you post a reworked commit message today so I can include this in my > 4.19 updates? > > Thanks, > Johan
[PATCH v4] USB: serial: cp210x: Implement GPIO support for CP2102N
This patch adds GPIO support for CP2102N devices.
It introduces new generic code to support emulating separate
input and outputs directions even though these devices
only know output modes (open-drain and pushpull). Existing
GPIO support for CP2105 has been migrated over to the new
code structure.
Only limitation is that for the QFN28 variant, only 4 out of
7 GPIOs are supported. This is because the config array
locations of the last 3 pins are not documented, and reverse
engineering revealed offsets that conflicted with other
documented functions. Hence we'll play it safe instead
until somebody clears this up further.
Signed-off-by: Karoly Pados
---
Patch changelog:
v1: Initial implementation.
v2: Incorporate Johan's feedback, most importantly generalize
new code and unify with CP2105.
v3 (by Johan): Apply stylistic and minor bug fixes.
v4: Corrected commit message. No code changes.
drivers/usb/serial/cp210x.c | 245 ++--
1 file changed, 209 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 4a118eb13590..b9bc80700be7 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -224,9 +224,10 @@ MODULE_DEVICE_TABLE(usb, id_table);
struct cp210x_serial_private {
#ifdef CONFIG_GPIOLIB
struct gpio_chipgc;
- u8 config;
- u8 gpio_mode;
boolgpio_registered;
+ u8 gpio_pushpull;
+ u8 gpio_altfunc;
+ u8 gpio_input;
#endif
u8 partnum;
speed_t max_speed;
@@ -343,6 +344,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
#define CONTROL_WRITE_RTS 0x0200
/* CP210X_VENDOR_SPECIFIC values */
+#define CP210X_READ_2NCONFIG 0x000E
#define CP210X_READ_LATCH 0x00C2
#define CP210X_GET_PARTNUM 0x370B
#define CP210X_GET_PORTCONFIG 0x370C
@@ -452,6 +454,12 @@ struct cp210x_config {
#define CP2105_GPIO1_RXLED_MODEBIT(1)
#define CP2105_GPIO1_RS485_MODEBIT(2)
+/* CP2102N configuration array indices */
+#define CP210X_2NCONFIG_CONFIG_VERSION_IDX 2
+#define CP210X_2NCONFIG_GPIO_MODE_IDX 581
+#define CP210X_2NCONFIG_GPIO_RSTLATCH_IDX 587
+#define CP210X_2NCONFIG_GPIO_CONTROL_IDX 600
+
/* CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x2 bytes. */
struct cp210x_gpio_write {
u8 mask;
@@ -1313,17 +1321,8 @@ static int cp210x_gpio_request(struct gpio_chip *gc,
unsigned int offset)
struct usb_serial *serial = gpiochip_get_data(gc);
struct cp210x_serial_private *priv = usb_get_serial_data(serial);
- switch (offset) {
- case 0:
- if (priv->config & CP2105_GPIO0_TXLED_MODE)
- return -ENODEV;
- break;
- case 1:
- if (priv->config & (CP2105_GPIO1_RXLED_MODE |
- CP2105_GPIO1_RS485_MODE))
- return -ENODEV;
- break;
- }
+ if (priv->gpio_altfunc & BIT(offset))
+ return -ENODEV;
return 0;
}
@@ -1331,10 +1330,15 @@ static int cp210x_gpio_request(struct gpio_chip *gc,
unsigned int offset)
static int cp210x_gpio_get(struct gpio_chip *gc, unsigned int gpio)
{
struct usb_serial *serial = gpiochip_get_data(gc);
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
+ u8 req_type = REQTYPE_DEVICE_TO_HOST;
int result;
u8 buf;
- result = cp210x_read_vendor_block(serial, REQTYPE_INTERFACE_TO_HOST,
+ if (priv->partnum == CP210X_PARTNUM_CP2105)
+ req_type = REQTYPE_INTERFACE_TO_HOST;
+
+ result = cp210x_read_vendor_block(serial, req_type,
CP210X_READ_LATCH, &buf, sizeof(buf));
if (result < 0)
return result;
@@ -1345,7 +1349,9 @@ static int cp210x_gpio_get(struct gpio_chip *gc, unsigned
int gpio)
static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
{
struct usb_serial *serial = gpiochip_get_data(gc);
+ struct cp210x_serial_private *priv = usb_get_serial_data(serial);
struct cp210x_gpio_write buf;
+ int result;
if (value == 1)
buf.state = BIT(gpio);
@@ -1354,25 +1360,68 @@ static void cp210x_gpio_set(struct gpio_chip *gc,
unsigned int gpio, int value)
buf.mask = BIT(gpio);
- cp210x_write_vendor_block(serial, REQTYPE_HOST_TO_INTERFACE,
- CP210X_WRITE_LATCH, &buf, sizeof(buf));
+ if (priv->partnum == CP210X_PARTNUM_CP2105) {
+ result = cp210x_write_vendor_block(serial,
+
