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