Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1262114 > unrolled thread

Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver Peter Hung <hpeter@gmail.com> - 2015-11-04 09:20 +0100
    Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver Oliver Neukum <oneukum@suse.com> - 2015-11-04 09:50 +0100
      Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver Peter Hung <hpeter@gmail.com> - 2015-11-05 03:50 +0100

#1262114 — Re: [PATCH V6 1/1] usb:serial: Add Fintek F81532/534 driver

Hi

Oliver Neukum 於 2015/11/3 下午 06:03 寫道:
> On Tue, 2015-11-03 at 11:51 +0800, Peter Hung wrote:
>> +static int f81534_attach(struct usb_serial *serial)
>> +{
>> +	struct f81534_serial_private *serial_priv = NULL;
>> +	int status;
>> +	int i;
>> +	int offset;
>> +	uintptr_t setting_idx = (uintptr_t) usb_get_serial_data(serial);
>> +
>> +	serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL);
>> +	if (!serial_priv)
>> +		return -ENOMEM;
>> +
>> +	usb_set_serial_data(serial, serial_priv);
>> +	serial_priv->setting_idx = setting_idx;
>> +
>> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
>> +		/* Disable all interrupt before submit URB */
>> +		status = f81534_setregister(serial->dev, i,
>> +					INTERRUPT_ENABLE_REGISTER, 0x00);
>> +		if (status) {
>> +			dev_err(&serial->dev->dev, "%s: IER disable failed\n",
>> +					__func__);
>> +			goto failed;
>> +		}
>> +	}
>> +
>> +	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 ?


>> +static int f81534_port_remove(struct usb_serial_port *port)
>> +{
>> +	struct f81534_port_private *port_priv;
>> +
>> +	f81534_release_gpio(port);
>> +	port_priv = usb_get_serial_port_data(port);
>> +	kfree(port_priv);
>> +	return 0;
>> +}
>> +
>> +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.

>> +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.

>> +static int f81534_prepare_write_buffer(struct usb_serial_port *port,
>> +					void *dest, size_t size)
>> +{
>> +	unsigned char *ptr = (unsigned char *) dest;
>> +	struct f81534_port_private *port_priv = usb_get_serial_port_data(port);
>> +	int port_num = port_priv->phy;
>> +	struct usb_serial *serial = port->serial;
>> +
>> +	WARN_ON(size != serial->port[0]->bulk_out_size);
>> +
>> +	if (size != F81534_WRITE_BUFFER_SIZE) {
>> +		WARN_ON(size != F81534_WRITE_BUFFER_SIZE);
>
> What is the sense of this?
>

I'll remove the double-check section with next version patch.


>> +	ptr[F81534_RECEIVE_BLOCK_SIZE * 0] = 0;
>> +	ptr[F81534_RECEIVE_BLOCK_SIZE * 1] = 1;
>> +	ptr[F81534_RECEIVE_BLOCK_SIZE * 2] = 2;
>> +	ptr[F81534_RECEIVE_BLOCK_SIZE * 3] = 3;
>
> Either these ...
>
>> +	ptr[F81534_RECEIVE_BLOCK_SIZE * port_num + 0] = port_num;
>
> .. or that is redundant
>

I'll remove it too.

Thanks for your advice.
-- 
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/

[toc] | [next] | [standalone]

#1262139

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:
> >> +static int f81534_attach(struct usb_serial *serial)
> >> +{
> >> +	struct f81534_serial_private *serial_priv = NULL;
> >> +	int status;
> >> +	int i;
> >> +	int offset;
> >> +	uintptr_t setting_idx = (uintptr_t) usb_get_serial_data(serial);
> >> +
> >> +	serial_priv = kzalloc(sizeof(*serial_priv), GFP_KERNEL);
> >> +	if (!serial_priv)
> >> +		return -ENOMEM;
> >> +
> >> +	usb_set_serial_data(serial, serial_priv);
> >> +	serial_priv->setting_idx = setting_idx;
> >> +
> >> +	for (i = 0; i < F81534_NUM_PORT; ++i) {
> >> +		/* Disable all interrupt before submit URB */
> >> +		status = f81534_setregister(serial->dev, i,
> >> +					INTERRUPT_ENABLE_REGISTER, 0x00);
> >> +		if (status) {
> >> +			dev_err(&serial->dev->dev, "%s: IER disable failed\n",
> >> +					__func__);
> >> +			goto failed;
> >> +		}
> >> +	}
> >> +
> >> +	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.

> >> +static int f81534_port_remove(struct usb_serial_port *port)
> >> +{
> >> +	struct f81534_port_private *port_priv;
> >> +
> >> +	f81534_release_gpio(port);
> >> +	port_priv = usb_get_serial_port_data(port);
> >> +	kfree(port_priv);
> >> +	return 0;
> >> +}
> >> +
> >> +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?

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

	Regards
		Oliver


--
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/

[toc] | [prev] | [next] | [standalone]

#1262847

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web