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

[Karoly Pados]

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: ft230x
Description: Binary data

Attachment: ft231x
Description: Binary data