Subject: Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver
To: Oliver Neukum <oneukum@suse.com>
References: <1446522669-18987-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1446545007.27681.12.camel@suse.com> <5639BF9B.2040005@gmail.com>
 <1446626302.8807.2.camel@suse.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw,
        peter_hong@fintek.com.tw, Peter Hung <hpeter+linux_kernel@gmail.com>
From: Peter Hung <hpeter@gmail.com>
Message-ID: <563AC363.20106@gmail.com>
Date: Thu, 5 Nov 2015 10:48:03 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <1446626302.8807.2.camel@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2964
Lines: 87

Hi,

Oliver Neukum 於 2015/11/4 下午 04:38 寫道:
> On Wed, 2015-11-04 at 16:19 +0800, Peter Hung wrote:
>> Hi
>>
>> Oliver Neukum 於 2015/11/3 下午 06:03 寫道:
>>> On Tue, 2015-11-03 at 11:51 +0800, Peter Hung wrote:
>>>> +	for (i = 0; i < F81534_NUM_PORT; ++i)
>>>> +		atomic_set(&serial_priv->port_active[i], 0);
>>>
>>> Should be ATOMIC_INIT()
>>>
>>
>> ATOMIC_INIT() seems to be used only for variable initializer, It cant be
>> used for dynamic allocation. Should I change it to a normal boolean
>> flag protecting with spin_lock ?
>
> No, if it doesn't work, use the current code.

OK, I'll use current code.

>>>> +static void f81534_compare_msr(struct usb_serial_port *port, u8 *msr,
>>>
>>> Is the point of passing a pointer to msr locking?
>>>
>>>> +				bool is_port_open)
>>
>> This function is used only with URB callback function. The *msr is
>> reported by H/W with newest MSR. The USB-Serial generic system will
>> re-submit read URB when callback complete. So this function should
>> run once on the same time.
>
> Yes, so why don't you pass an u8 as opposed to a pointer to an u8?

I'll re-write it from u8* to u8.

>>>> +static int f81534_tiocmget(struct tty_struct *tty)
>>>> +{
>>>> +	struct usb_serial_port *port = tty->driver_data;
>>>> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
>>>> +	unsigned long flags;
>>>> +	int r;
>>>> +	u8 msr, mcr;
>>>> +
>>>> +	/*
>>>> +	 * We'll avoid to direct read MSR register. The IC will read the MSR
>>>> +	 * changed and report it f81534_process_per_serial_block() by
>>>> +	 * F81534_TOKEN_MSR_CHANGE.
>>>> +	 *
>>>> +	 * When this device in heavy loading (e.g., BurnInTest Loopback Test)
>>>> +	 * The report of MSR register will delay received a bit. It's due to
>>>> +	 * MSR interrupt is lowest priority in 16550A. So we decide to sleep
>>>> +	 * a little time to pass the test.
>>>> +	 */
>>>> +	if (schedule_timeout_interruptible(
>>>> +			msecs_to_jiffies(F81534_DELAY_READ_MSR))) {
>>>> +		dev_info(&port->dev, "%s: breaked !!\n", __func__);
>>>> +	}
>>>
>>> Is the delay necessary or isn't it?
>>> If it is necessary you should do something about the signal.
>>>
>>
>> We add this delay due to stress test (Loop-back & 921600bps with
>> BurnInTest). It'll receive MSR with some delay when connecting with
>> DTR-DSR & RTS/CTS, but the delay smaller than 10ms. So we decided to
>> delay some time to pass the test.
>
> OK, but how do you guarantee the delay you need if you get a signal,
> which would abort the delay?
>

Hmm, you are right. It seems to replace *_interruptible to
*_killable and return -EINTR to guarantee not abort by normal
signal.


Thanks for your advices
-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/