Messages in this thread | | | Date | Fri, 14 Sep 2018 16:54:34 +0000 | From | "Karoly Pados" <> | Subject | Re: [PATCH v3] USB: serial: ftdi_sio: implement GPIO support for FT-X devices |
| |
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 [unhandled content-type:application/octet-stream][unhandled content-type:application/octet-stream] | |