Thanks Andy,
On 1 August 2018 at 18:08, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 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).
Yes, indeed.
>
>> + 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.
Will do.
>
>> + 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.
yes
>
>> +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?
Yes, because it is used as USB transfer buffer (DMA...) which can not
be on the stack.
>
>> + /* 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
Regards,
Loic
--
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
