Re: [PATCH V1 3/6] USB: serial: f81232: Add generator for F81534A

[Johan Hovold <johan@kernel.org>]

On Mon, Sep 02, 2019 at 10:59:17AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> Hi Johan,
> 
> Johan Hovold 於 2019/8/28 下午 11:02 寫道:
> > On Thu, Jun 06, 2019 at 10:54:13AM +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, add
> >> GPIO device USB interface to device_list and trigger generate worker,
> >> f81534a_generate_worker to run f81534a_ctrl_generate_ports().
> >>
> >> The operation in f81534a_ctrl_generate_ports() as following:
> >> 	1: Write 0x8fff to F81534A_CMD_ENABLE_PORT register for enable all
> >> 	   UART device.
> >>
> >> 	2: Read port existence & current status from F81534A_CMD_PORT_STATUS
> >> 	   register. the higher 16bit will indicate the UART existence. If the
> >> 	   UART is existence, we'll check it GPIO mode as long as not default
> >> 	   value (default is all input mode).
> >>
> >> 	3: 1 GPIO device will check with max 15s and check next GPIO device when
> >> 	   timeout. (F81534A_CTRL_RETRY * F81534A_CTRL_TIMER)
> >>
> >> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> > 
> > This is all looks crazy... Please better describe how the device works,
> > and you want to implement support.
> 
> I'll try to refactor more simply for first add into kernel.
> 
> >> ---
> >>   drivers/usb/serial/f81232.c | 356 +++++++++++++++++++++++++++++++++++++++++++-
> >>   1 file changed, 355 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> >> index 75dfc0b9ef30..e9470fb0d691 100644
> >> --- a/drivers/usb/serial/f81232.c
> >> +++ b/drivers/usb/serial/f81232.c
> >> @@ -41,6 +41,12 @@ static const struct usb_device_id id_table[] = {
> >>   };
> >>   MODULE_DEVICE_TABLE(usb, id_table);
> >>   
> >> +static const struct usb_device_id f81534a_ctrl_id_table[] = {
> >> +	{ USB_DEVICE(0x2c42, 0x16f8) },		/* Global control device */
> >> +	{ }					/* Terminating entry */
> >> +};
> >> +MODULE_DEVICE_TABLE(usb, f81534a_ctrl_id_table);
> > 
> > You can only have one MODULE_DEVICE_TABLE()...
> 
> I had a question about this. In this file, we'll need support 3 sets of
> id f81232(1)/f81534a(9)/f81534a_ctrl(1). So I will refactor the code
> about id section to the below due to the id table will use more than
> once:
> 
> =======================================================================
> #define F81232_ID		\
> 	{ USB_DEVICE(0x1934, 0x0706) }	/* 1 port UART device */
> 
> #define F81534A_SERIES_ID	\
> 	{ USB_DEVICE(0x2c42, 0x1602) },	/* In-Box 2 port UART device */	\
> 	{ USB_DEVICE(0x2c42, 0x1604) },	/* In-Box 4 port UART device */	\
> 	{ USB_DEVICE(0x2c42, 0x1605) },	/* In-Box 8 port UART device */	\
> 	{ USB_DEVICE(0x2c42, 0x1606) },	/* In-Box 12 port UART device */ \
> 	{ USB_DEVICE(0x2c42, 0x1608) },	/* Non-Flash type */ \
> 	{ USB_DEVICE(0x2c42, 0x1632) },	/* 2 port UART device */ \
> 	{ USB_DEVICE(0x2c42, 0x1634) },	/* 4 port UART device */ \
> 	{ 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 */
> };
> 
> static const struct usb_device_id f81534a_id_table[] = {
> 	F81534A_SERIES_ID,
> 	{ }					/* 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,
> 	{ }					/* Terminating entry */
> };
> MODULE_DEVICE_TABLE(usb, all_serial_id_table);
> =======================================================================
> 
> but the checkpatch.pl give me the warning below:
> ERROR: Macros with complex values should be enclosed in parentheses
> #42: FILE: f81232.c:28:
> +#define F81534A_SERIES_ID      \
> +       { USB_DEVICE(0x2c42, 0x1602) }, /* In-Box 2 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1604) }, /* In-Box 4 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1605) }, /* In-Box 8 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1606) }, /* In-Box 12 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1608) }, /* Non-Flash type */ \
> +       { USB_DEVICE(0x2c42, 0x1632) }, /* 2 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1634) }, /* 4 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1635) }, /* 8 port UART device */ \
> +       { USB_DEVICE(0x2c42, 0x1636) }  /* 12 port UART device */
> 
> Is any suggestion ?

Just ignore checkpatch.pl, that's often the right answer. We already
have something similar to the above in usb-serial-simple.c.

Unless you can come up with some better way to deal with this.

Johan