Re: [PATCH V2 6/7] USB: serial: f81232: Add generator for F81534A

[Johan Hovold <johan@kernel.org>]

On Mon, Sep 23, 2019 at 10:24:48AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The Fintek F81534A series is contains 1 HUB / 1 GPIO device / n UARTs,
> but the UART is default disable and need enabled by GPIO device(2c42/16F8).
> 
> When F81534A plug to host, we can only see 1 HUB & 1 GPIO device and we
> need write 0x8fff to GPIO device register F81534A_CMD_ENABLE_PORT(116h)
> to enable all available serial ports.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 135 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 01cb5a5ea1d2..82cc1e6cff62 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -36,6 +36,9 @@
>  	{ USB_DEVICE(0x2c42, 0x1635) },	/* 8 port UART device */ \
>  	{ USB_DEVICE(0x2c42, 0x1636) }	/* 12 port UART device */
>  
> +#define F81534A_CTRL_ID		\
> +	{ USB_DEVICE(0x2c42, 0x16f8) }	/* Global control device */
> +
>  static const struct usb_device_id id_table[] = {
>  	F81232_ID,
>  	{ }					/* Terminating entry */
> @@ -46,6 +49,11 @@ static const struct usb_device_id f81534a_id_table[] = {
>  	{ }					/* Terminating entry */
>  };
>  
> +static const struct usb_device_id f81534a_ctrl_id_table[] = {
> +	F81534A_CTRL_ID,
> +	{ }					/* Terminating entry */
> +};
> +
>  static const struct usb_device_id all_serial_id_table[] = {
>  	F81232_ID,
>  	F81534A_SERIES_ID,
> @@ -105,6 +113,8 @@ MODULE_DEVICE_TABLE(usb, all_serial_id_table);
>  #define F81534A_GPIO_MODE1_OUTPUT	BIT(1)
>  #define F81534A_GPIO_MODE0_OUTPUT	BIT(0)
>  
> +#define F81534A_CMD_ENABLE_PORT		0x116

Maybe use a distinct prefix for requests to the control device (e.g.
F81534A_CTRL_).

> +
>  struct f81232_private {
>  	struct mutex lock;
>  	u8 modem_control;
> @@ -853,6 +863,95 @@ static void f81232_lsr_worker(struct work_struct *work)
>  		dev_warn(&port->dev, "read LSR failed: %d\n", status);
>  }
>  
> +static int f81534a_ctrl_set_register(struct usb_device *dev, u16 reg, u16 size,
> +					void *val)
> +{
> +	int retry = F81534A_ACCESS_REG_RETRY;
> +	int status;
> +	u8 *tmp;
> +
> +	tmp = kmalloc(size, GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	memcpy(tmp, val, size);

Use kmemdup().

> +
> +	while (retry--) {
> +		status = usb_control_msg(dev,
> +					usb_sndctrlpipe(dev, 0),
> +					F81534A_REGISTER_REQUEST,
> +					F81534A_SET_REGISTER,
> +					reg,
> +					0,
> +					tmp,
> +					size,
> +					USB_CTRL_SET_TIMEOUT);
> +		if (status != size) {
> +			status = usb_translate_errors(status);
> +			if (status == -EIO)
> +				continue;

This will retry on short transfers, is that what you intended?

> +
> +			status = -EIO;
> +		} else {
> +			status = 0;
> +		}
> +
> +		break;
> +	}
> +
> +	if (status) {
> +		dev_err(&dev->dev, "set ctrl reg: %x, failed status: %d\n", reg,
> +				status);
> +	}
> +
> +	kfree(tmp);
> +	return status;
> +}
> +
> +static int f81534a_ctrl_enable_all_ports(struct usb_interface *intf)
> +{
> +	struct usb_device *dev = interface_to_usbdev(intf);
> +	unsigned char enable[2];
> +	int status;
> +
> +	/* enable all available serial ports */
> +	enable[0] = 0xff;
> +	enable[1] = 0x8f;

What are these magic constants?

> +
> +	status = f81534a_ctrl_set_register(dev, F81534A_CMD_ENABLE_PORT,
> +			sizeof(enable), enable);
> +	if (status)
> +		dev_warn(&dev->dev, "set CMD_ENABLE_PORT failed: %d\n", status);
> +
> +	return status;
> +}
> +
> +static int f81534a_ctrl_probe(struct usb_interface *intf,
> +				const struct usb_device_id *id)
> +{
> +	struct usb_device *dev = interface_to_usbdev(intf);
> +	int status;
> +
> +	status = f81534a_ctrl_enable_all_ports(intf);
> +	if (status)
> +		return status;
> +
> +	dev = usb_get_dev(dev);

This isn't strictly needed.

> +	return 0;
> +}

Johan