On Sun, Jan 22, 2017 at 12:32:16AM +0300, Maksim Salau wrote:
> The adaptor can be found on development boards for 78k, RL78 and V850
> microcontrollers produced by Renesas Electronics Corporation.
>
> This is not a full-featured USB to serial converter, however it allows
> basic communication and simple control which is enough for programming of
> on-board flash and debugging through a debug monitor.
>
> uPD78F0730 is a USB-enabled microcontroller with USB-to-UART conversion
> implemented in firmware.
>
> This chip is also present in some debugging adaptors which use it for
> USB-to-SPI conversion as well. The present driver doesn't cover SPI,
> only USB-to-UART conversion is supported.
>
> Signed-off-by: Maksim Salau <maksim.salau@xxxxxxxxx>
> ---
> Changes in v5:
> Fixed a typo in assignment of opcode of the SET_DTR_RTS request
>
> Changes in v4:
> Addressed comments from Johan
You made some further changes than what I suggested but forgot to
document those. Often better to explicitly list the changes made rather
than refer to review comments this way.
This already looks really good; just a few minor things noted below.
> drivers/usb/serial/Kconfig | 9 +
> drivers/usb/serial/Makefile | 1 +
> drivers/usb/serial/upd78f0730.c | 458 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 468 insertions(+)
> create mode 100644 drivers/usb/serial/upd78f0730.c
> +static int upd78f0730_send_ctl(struct usb_serial_port *port,
> + void *data, int size)
> +{
> + struct device *dev = &port->dev;
> + struct usb_device *usbdev = port->serial->dev;
> + void *buf;
> + int res;
> +
> + if (!size || !data)
> + return -EINVAL;
> +
> + buf = kmemdup(data, size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + res = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0), 0x00,
> + USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> + 0x0000, 0x0000, buf, size, USB_CTRL_SET_TIMEOUT);
> +
> + kfree(buf);
> +
> + if (res < 0)
> + return res;
> +
> + if (res != size) {
> + dev_err(dev, "%s - send failed: opcode=%02x, size=%d, res=%d\n",
> + __func__, *(u8 *)data, size, res);
You still want an error message in case res < 0 (as in v3), but you can
return that errno instead of -EIO then.
You can drop the __func__ prefixes you added to error messages in v5
throughout, better to spell out what went wrong (e.g. "failed to send
control request %02x: %d\n", *(u8 *)data, res).
> + /* The maximum expected length of a transfer is 6 bytes */
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int upd78f0730_attach(struct usb_serial *serial)
> +{
> + const char num_ports = serial->num_ports;
> +
> + if (serial->num_bulk_in < num_ports ||
> + serial->num_bulk_out < num_ports) {
> + dev_err(&serial->interface->dev, "%s - missing endpoints\n",
> + __func__);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
This is new in v5, and should have been mentioned in the changelog above.
Note that this is not strictly necessary, though. Some drivers
dereferenced pointers without the appropriate sanity checks, but this
driver and the generic callbacks it uses, does not suffer from such
problems.
I suggest you just drop this check (i.e. the attach callback).
> +static int upd78f0730_tiocmget(struct tty_struct *tty)
> +{
> + struct device *dev = tty->dev;
> + struct upd78f0730_port_private *private;
> + struct usb_serial_port *port = tty->driver_data;
> + int signals;
> + int res;
> +
> + private = usb_get_serial_port_data(port);
> +
> + mutex_lock(&private->lock);
> + signals = private->line_signals;
> + mutex_unlock(&private->lock);
> +
> + res = ((signals & UPD78F0730_DTR) ? TIOCM_DTR : 0) |
> + ((signals & UPD78F0730_RTS) ? TIOCM_RTS : 0);
> +
> + dev_dbg(dev, "%s - res = %x\n", __func__, res);
Note that __func__ is fine for concise debug messages like this one
(that users generally won't see).
But please drop it from all ERR and WARN messages.
> +
> + return res;
> +}
Thanks,
Johan
--
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
