Re: [PATCH v3] USB: serial: ftdi_sio: implement GPIO support for FT-X devices

["Karoly Pados" <pados@pados.hu>]

[-- Attachment #1: Type: text/plain, Size: 3009 bytes --]

Hi,

Thanks again for the review.

>> #include <linux/usb/serial.h>
>> +#if defined(CONFIG_GPIOLIB)
>> +#include <linux/gpio/driver.h>
>> +#endif
> 
> Hmm. I already commented on this in v1.

Yeah, and I changed it too, but I now realized I misunderstood your intentions.
You want me to remove the conditional compilation completely, while I thought
you just prefer the #if defined() style instead of #ifdef. A misunderstanding
on my part.

> 
>> #include "ftdi_sio.h"
>> #include "ftdi_sio_ids.h"
>> 
>> @@ -72,6 +75,14 @@ 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() */
>> +#if defined(CONFIG_GPIOLIB)
>> + struct gpio_chip gc;
>> + bool gpio_registered; /* is the gpiochip in kernel registered */
>> + bool gpio_used; /* true if the user requested a gpio */
>> + u8 gpio_altfunc; /* which pins are in gpio mode */
>> + u8 gpio_input; /* pin directions cache */
> 
> And I asked you to invert this one (i.e. replace with gpio_output).

Later when we discussed and you replied to my comments, I interpreted your response
it can stay this way. Another misunderstanding, sorry.

>> +/* Returns the number of bytes read */
>> +static int ftdi_read_eeprom(struct usb_serial *serial,
>> + void *dst, /* must be kmalloc'd using GFP_KERNEL*/
> 
> Whether GFP_KERNEL was used is not really relevant, but highlighting
> that the buffer needs to be DMA-able is good.
> 
>> + u16 addr, /* must be aligned to 16 bits */
>> + u16 nbytes) /* must be a multiple of 16 bits */
> 
> I think checkpatch gets confused by you odd argument comments here. Use
> a kernel doc comment, if you want to be this specific instead.

I'll just remove the comments on the parameters of this function. The 
sb_control_msg is a dead giveaway the buffer must be DMA-able, and the
error checks at the start make the requirements on the other parameters also
obvious.

>> +
>> + /* 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 */
> 
> No known way to identify FT234XD here?
> 
> After taking a quick peek at libftdi, it seems we really have no clue
> how to detect these device types and 0x1000 could be for all FTX
> devices. Heck, the current kernel driver just assumes anything we don't
> recognise to be an FTX, something which would now hit this code path...
> 
> What devices did you and Loic have? Could you post the lsusb -v output
> for these? Perhaps someone with an FT234XD can chime in as well.
> 

No clue about this one. I only own FT230X and FT231X devices, but it looks
like they cannot be told apart, except for eeprom strings which are reconfigurable
by the user. I wouldn't rely on such things. Anyway, lsusb -v outputs attached.

Karoly

[-- Attachment #2: ft230x --]
[-- Type: application/octet-stream, Size: 2227 bytes --]

Bus 001 Device 012: ID 0403:6015 Future Technology Devices International, Ltd Bridge(I2C/SPI/UART/FIFO)
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x0403 Future Technology Devices International, Ltd
  idProduct          0x6015 Bridge(I2C/SPI/UART/FIFO)
  bcdDevice           10.00
  iManufacturer           1 FTDI
  iProduct                2 FT230X Basic UART
  iSerial                 3 DAZ0W15A
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0020
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass    255 Vendor Specific Subclass
      bInterfaceProtocol    255 Vendor Specific Protocol
      iInterface              2 FT230X Basic UART
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
can't get device qualifier: Resource temporarily unavailable
can't get debug descriptor: Resource temporarily unavailable
Device Status:     0x0000
  (Bus Powered)

[-- Attachment #3: ft231x --]
[-- Type: application/octet-stream, Size: 2205 bytes --]

Bus 001 Device 006: ID 0403:6015 Future Technology Devices International, Ltd Bridge(I2C/SPI/UART/FIFO)
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0         8
  idVendor           0x0403 Future Technology Devices International, Ltd
  idProduct          0x6015 Bridge(I2C/SPI/UART/FIFO)
  bcdDevice           10.00
  iManufacturer           1 FTDI
  iProduct                2 LC231X
  iSerial                 3 FT377065
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0020
    bNumInterfaces          1
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0x80
      (Bus Powered)
    MaxPower               90mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass       255 Vendor Specific Class
      bInterfaceSubClass    255 Vendor Specific Subclass
      bInterfaceProtocol    255 Vendor Specific Protocol
      iInterface              2 LC231X
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               0
can't get device qualifier: Resource temporarily unavailable
can't get debug descriptor: Resource temporarily unavailable
Device Status:     0x0000
  (Bus Powered)