On Wed, Aug 1, 2018 at 6:46 PM, Loic Poulain <loic.poulain@xxxxxxxxxx> 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 ([PATCH 0/2] FTDI CBUS GPIO support).
The best approach to refer to a mail is to put a message-id, something like
(... [1].)
[1]: Message-Id: <...message-id...>
> +static int ftdi_read_eeprom(struct usb_device *udev, unsigned int off,
> + void *val, size_t bytes)
> +{
> + if (bytes % 2) /* 16-bit eeprom */
> + return -EINVAL;
Why is it fatal?
You may read one byte less (and provide an error code like -EIO, or
whatever fits better) or more (and provide exact amount asked).
> + return 0;
> +}
> + rv = ftdi_read_pins(fgc->port, &val);
> + if (rv)
> + dev_err(&udev->dev, "Unable to read CBUS GPIO pins, %d\n", rv);
Why not to return an error?
(Message itself sounds like a noise. Perhaps, dev_dbg() or throw away.
> + rv = ftdi_set_bitmode(fgc->port, fgc->cbus_mask, FTDI_SIO_BITMODE_CBUS);
> + if (rv)
> + dev_err(&udev->dev, "Unable set CBUS Bit-Bang mode, %d\n", rv);
Ditto WRT message.
> +static int ftdi_register_cbus_gpiochip(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_device *udev = port->serial->dev;
> + struct ftdi_gpiochip *fgc;
> + int rv;
> +
> + fgc = kzalloc(sizeof(*fgc), GFP_KERNEL);
> + if (!fgc)
> + return -ENOMEM;
devm_ ?
> + cbus_mux = kmalloc(4, GFP_KERNEL);
> + if (!cbus_mux)
> + return -ENOMEM;
Is it mandatory to alloc so small amount on heap in this case?
> + /* CBUS pins are individually configurable via 8-bit MUX control
> + * value, living at 0x1a for CBUS0. cf application note AN_201.
> + */
Is it a comment style used in this file?
> + rv = ftdi_read_eeprom(udev, 0x1a, cbus_mux, 4);
0x1a is a magic.
> + rv = gpiochip_add_data(&fgc->gc, fgc);
> + if (rv) {
> + dev_err(&udev->dev, "Unable to add gpiochip\n");
> + kfree(fgc);
> + return rv;
> + }
devm_ ?
> + return 0;
> +}
> +
> +static void ftdi_unregister_cbus_gpiochip(struct usb_serial_port *port)
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct ftdi_gpiochip *fgc = priv->fgc;
> +
> + if (fgc) {
How you can be here when fgc == NULL?!
> + gpiochip_remove(&fgc->gc);
> + kfree(fgc);
> + }
> +}
I guess complete function will go away when you switch to devm.
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
