Hi Johan, On 4 September 2018 at 14:47, Johan Hovold <johan@xxxxxxxxxx> wrote: > On Sat, Aug 04, 2018 at 12:17:15PM +0200, 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 [1]. >> >> [1] Message-Id: 1434838377-8042-1-git-send-email-stefan@xxxxxxxx >> >> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx> >> --- >> v2: Use message-id for LKML reference >> Rework read_eeprom according to Andy's comment and return read count >> Remove noisy messages >> Comment style alignment >> Add defines for magic values >> Cannot use devm, because gpiochip is linked to the port not to the udev >> v3: >> Fix typo in CBUS mask description comment > > First, thanks for looking into this; seems like we're finally about to > get support for the CBUS gpios. > > But now I get not one, but two, competing implementations for this > posted in one month: > > https://lkml.kernel.org/r/20180825204744.2307-1-pados@xxxxxxxx > > And it looks like you both have been considering at least some of the > earlier attempts, which is great. > > I've reviewed both patches and it seems to me that Karoly's version is > closer to what I'd like to see as the end-result. Specifically this > means having: > > - fixed offsets for the physical pins rather than having the offsets > depend on eeprom configuration > > - better type abstraction (we want to add support for ft232r and ft232h > eventually as well) > > The other patch is also more complete in that it: > > - considers the initial pin state > - implements a request() callback > - implements a get_direction() callback > > In contrast, with this implementation as it stands, we could end up with > a driven pin being reported as an input (corner case, but still). > > Implementation-wise, the other patch is also closer to what I'd like to > see (e.g. not using atomic bit ops, and getting the error handling right > from the start). > > There are some issues that needs to be addressed in the other patch as > well, and in doing so it would be wise to look at your patch for > inspiration (e.g. naming issues and adding an eeprom helper to only read > the couple of configuration words needed). > > In the end, going with the other patch means less work for me, even if > you both (by unfortunate timing) have forced me to do more than twice > the amount of review work already. Thanks for review, I'm fine with that and I'll review the other patch. Karoly can use chunks of my patch if useful. Regards, Loic
