Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 25 Feb 2020 12:23:03 +0100
From:   Johan Hovold <johan@kernel.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Cc:     johan@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        peter_hong@fintek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V3 6/6] USB: serial: f81232: Add generator for F81534A
Message-ID: <20200225112303.GT32540@localhost>
References: <20200130054752.9368-1-hpeter+linux_kernel@gmail.com>
 <20200130054752.9368-7-hpeter+linux_kernel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200130054752.9368-7-hpeter+linux_kernel@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@gmail.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