Re: [PATCH V4 1/1] USB: serial: f81232: Add generator for F81534A

[Johan Hovold <johan@kernel.org>]

On Wed, Mar 04, 2020 at 11:37:51AM +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
> write 0x8fff to GPIO device register F81534A_CTRL_CMD_ENABLE_PORT(116h)
> to enable all available serial ports.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> Changelog:
> v4:
> 	1. Remove unused define.
> 	2. Remove usb_translate_errors() in f81534a_ctrl_set_register()
> 	   with short transfer.
> 	3. Replace dev_warn() with dev_err() in f81534a_ctrl_enable_all_ports()
> 	4. Disable & remove all usb serial port device when disconnect().

> +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 = kmemdup(val, size, GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	while (retry--) {
> +		status = usb_control_msg(dev,
> +					usb_sndctrlpipe(dev, 0),
> +					F81232_REGISTER_REQUEST,
> +					F81232_SET_REGISTER,
> +					reg,
> +					0,
> +					tmp,
> +					size,
> +					USB_CTRL_SET_TIMEOUT);
> +		if (status != size) {
> +			status = -EIO;

You shouldn't discard any error code you get here; only set it to -EIO
if the transfer is short.

Your previous patch didn't retry on -ENOMEM and -ENODEV (e.g. if the
device was disconnected) which seems reasonable.

What errors are you seeing here when you really do need to resend?
Perhaps only retry on those specifically (and short transfers)?

> +			continue;
> +		}
> +
> +		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, bool en)
> +{
> +	struct usb_device *dev = interface_to_usbdev(intf);
> +	unsigned char enable[2] = {0};
> +	int status;
> +
> +	/*
> +	 * Enable all available serial ports, define as following:
> +	 * bit 15	: Reset behavior (when HUB got soft reset)
> +	 *			0: maintain all serial port enabled state.
> +	 *			1: disable all serial port.
> +	 * bit 0~11	: Serial port enable bit.
> +	 */
> +	if (en) {
> +		enable[0] = 0xff;
> +		enable[1] = 0x8f;
> +	}
> +
> +	status = f81534a_ctrl_set_register(dev, F81534A_CTRL_CMD_ENABLE_PORT,
> +			sizeof(enable), enable);
> +	if (status)
> +		dev_err(&dev->dev, "failed to enable ports: %d\n", status);

Please use &intf->dev here and for the dev_err() in
f81534a_ctrl_set_register() as that will include the driver name in the
prefix.

> +
> +	return status;
> +}

Johan