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

[Ji-Ze Hong (Peter Hong)]

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@xxxxxxxxx>
>
> 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 ?

Thanks
--
With Best Regards,
Peter Hong