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;
Subject: Re: [PATCH V1 3/6] USB: serial: f81232: Add generator for F81534A
To:     Johan Hovold <johan@kernel.org>
Cc:     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>
References: <1559789656-15847-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1559789656-15847-4-git-send-email-hpeter+linux_kernel@gmail.com>
 <20190828150252.GJ13017@localhost>
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Message-ID: <b194f7e6-963d-df3b-5295-4323dae0846d@gmail.com>
Date:   Mon, 2 Sep 2019 10:59:17 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190828150252.GJ13017@localhost>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 ?

Thanks
-- 
With Best Regards,
Peter Hong