Message-ID: <20200225112303.GT32540@localhost>
Date:   Tue, 25 Feb 2020 12:23:03 +0100
From:   Johan Hovold <johan@...nel.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc:     johan@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        peter_hong@...tek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V3 6/6] USB: serial: f81232: Add generator for F81534A

On Thu, Jan 30, 2020 at 01:47:52PM +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@...il.com>
> ---
> Changelog:
> v3:
> 	1. Modify some define with prefix F81534A_CTRL_.
> 	2. Use kmemdup() in f81534a_ctrl_set_register().
> 	3. Not accpet with short transfers in f81534a_ctrl_set_register().
> 	4. Add comment in f81534a_ctrl_enable_all_ports() to describe magic
> 	   constants.
> 	5. Remove non-need usb_get_dev()/usb_put_dev().
> 	6. Add F81534A_CTRL_ID in MODULE_DEVICE_TABLE().
> 
> v2:
> 	1: Simplify the generator behavior.
> 	2: Change multiply MODULE_DEVICE_TABLE() to 1 only.
> 
>  drivers/usb/serial/f81232.c | 134 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index 21410a7f3a8b..0303f94b2521 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 f81232_id_table[] = {
>  	F81232_ID,
>  	{ }					/* Terminating entry */
> @@ -46,9 +49,15 @@ 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 combined_id_table[] = {
>  	F81232_ID,
>  	F81534A_SERIES_ID,
> +	F81534A_CTRL_ID,
>  	{ }					/* Terminating entry */
>  };
>  MODULE_DEVICE_TABLE(usb, combined_id_table);
> @@ -61,6 +70,7 @@ MODULE_DEVICE_TABLE(usb, combined_id_table);
>  #define F81232_REGISTER_REQUEST		0xa0
>  #define F81232_GET_REGISTER		0xc0
>  #define F81232_SET_REGISTER		0x40
> +#define F81534A_ACCESS_REG_RETRY	2
>  
>  #define SERIAL_BASE_ADDRESS		0x0120
>  #define RECEIVE_BUFFER_REGISTER		(0x00 + SERIAL_BASE_ADDRESS)
> @@ -92,6 +102,8 @@ MODULE_DEVICE_TABLE(usb, combined_id_table);
>  #define F81534A_TRIGGER_MULTIPLE_4X	BIT(3)
>  #define F81534A_FIFO_128BYTE		(BIT(1) | BIT(0))
>  
> +#define F81534A_MAX_PORT		12
> +

This define isn't used anymore.

>  /* Serial port self GPIO control, 2bytes [control&output data][input data] */
>  #define F81534A_GPIO_REG		0x10e
>  #define F81534A_GPIO_MODE2_DIR		BIT(6) /* 1: input, 0: output */
> @@ -101,6 +113,9 @@ MODULE_DEVICE_TABLE(usb, combined_id_table);
>  #define F81534A_GPIO_MODE1_OUTPUT	BIT(1)
>  #define F81534A_GPIO_MODE0_OUTPUT	BIT(0)
>  
> +#define F81534A_CTRL_CMD_ENABLE_PORT	0x116
> +
> +
>  struct f81232_private {
>  	struct mutex lock;
>  	u8 modem_control;
> @@ -848,6 +863,89 @@ 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 = 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 = usb_translate_errors(status);

Please don't use usb_translate_errors() for non-errors (short
transfers). Handle it explicitly instead so that the intent is clear
here (e.g. to retry on short transfers).

> +			if (status == -EIO)
> +				continue;
> +
> +			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, 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.
> +	 */
> +	enable[0] = 0xff;
> +	enable[1] = 0x8f;
> +
> +	status = f81534a_ctrl_set_register(dev, F81534A_CTRL_CMD_ENABLE_PORT,
> +			sizeof(enable), enable);
> +	if (status)
> +		dev_warn(&dev->dev, "set ENABLE_PORT failed: %d\n", status);

Use dev_err() here, and "failed to enable ports" is probably a better
wording.

> +
> +	return status;
> +}
> +
> +static int f81534a_ctrl_probe(struct usb_interface *intf,
> +				const struct usb_device_id *id)
> +{
> +
> +	return f81534a_ctrl_enable_all_ports(intf);
> +}
> +
> +static void f81534a_ctrl_disconnect(struct usb_interface *intf)
> +{
> +}

Shouldn't you disable the ports when unbinding the control driver?

I've applied all the patches in the series up until this one so no need
to resend those.

Johan

Powered by blists - more mailing lists