Re: [PATCH 1/7] ark3116: (3rd try) Setup some basic infrastructure for new ark3116 driver.

[Bart Hartgers]

2009/10/27 Greg KH <greg@xxxxxxxxx>:
> On Sun, Oct 25, 2009 at 06:50:58PM +0100, bart.hartgers@xxxxxxxxx wrote:
>> Signed-off-by: Bart Hartgers <bart.hartgers@xxxxxxxxx>
>> ---
>> Index: linux-2.6.32-rc4/drivers/usb/serial/ark3116.c
>> ===================================================================
>> --- linux-2.6.32-rc4.orig/drivers/usb/serial/ark3116.c    Â2009-10-18 14:25:02.000000000 +0200
>> +++ linux-2.6.32-rc4/drivers/usb/serial/ark3116.c   2009-10-18 14:25:14.000000000 +0200
>> @@ -1,4 +1,6 @@
>> Â/*
>> + * Copyright (C) 2009 by Bart Hartgers (bart.hartgers+ark3116@xxxxxxxxx)
>> + * Original version:
>> Â * Copyright (C) 2006
>> Â * Â Simon Schulz (ark3116_driver <at> auctionant.de)
>> Â *
>> @@ -6,10 +8,13 @@
>> Â * - implements a driver for the arkmicro ark3116 chipset (vendor=0x6547,
>> Â * Â productid=0x0232) (used in a datacable called KQ-U8A)
>> Â *
>> - * - based on code by krisfx -> thanks !!
>> - * Â (see http://www.linuxquestions.org/questions/showthread.php?p=2184457#post2184457)
>> + * Supports full modem status lines, break, hardware flow control. Does not
>> + * support software flow control, since I do not know how to enable it in hw.
>> Â *
>> - * Â- based on logs created by usbsnoopy
>> + * This driver is a essentially new implementation. I initially dug
>> + * into the old ark3116.c driver and suddenly realized the ark3116 is
>> + * a 16450 with a USB interface glued to it. See comments at the
>> + * bottom of this file.
>> Â *
>> Â * This program is free software; you can redistribute it and/or modify it
>> Â * under the terms of the GNU General Public License as published by the
>> @@ -19,15 +24,31 @@
>>
>> Â#include <linux/kernel.h>
>> Â#include <linux/init.h>
>> +#include <asm/atomic.h>
>> +#include <linux/ioctl.h>
>> Â#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> Â#include <linux/module.h>
>> Â#include <linux/usb.h>
>> Â#include <linux/usb/serial.h>
>> Â#include <linux/serial.h>
>> +#include <linux/serial_reg.h>
>> Â#include <linux/uaccess.h>
>> -
>> +#include <linux/mutex.h>
>>
>> Âstatic int debug;
>> +/*
>> + * Version information
>> + */
>> +
>> +#define DRIVER_VERSION "v0.3"
>> +#define DRIVER_AUTHOR "Bart Hartgers <bart.hartgers+ark3116@xxxxxxxxx>"
>> +#define DRIVER_DESC "USB ARK3116 serial/IrDA driver"
>> +#define DRIVER_DEV_DESC "ARK3116 RS232/IrDA"
>> +#define DRIVER_NAME "ark3116"
>> +
>> +/* usb timeout of 1 second */
>> +#define ARK_TIMEOUT (1*HZ)
>>
>> Âstatic struct usb_device_id id_table [] = {
>> Â Â Â { USB_DEVICE(0x6547, 0x0232) },
>> @@ -45,6 +66,52 @@ static int is_irda(struct usb_serial *se
>> Â Â Â return 0;
>> Â}
>>
>> +struct ark3116_private {
>> +   wait_queue_head_t    delta_msr_wait;
>> +   struct async_icount   icount;
>> +   int           irda;  /* 1 for irda device */
>> +
>> + Â Â /* protects hw register updates */
>> +   struct mutex      Âlock;
>> +
>> +   int           quot;  /* baudrate divisor */
>> + Â Â __u8 Â Â Â Â Â Â Â Â Â Âlcr; Â Â/* line control register value */
>> + Â Â __u8 Â Â Â Â Â Â Â Â Â Âhcr; Â Â/* handshake control register (0x8)
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* value
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â*/
>> + Â Â /* register values - updated asynchronously */
>> +   atomic_t        Âmcr;
>> +   atomic_t        Âmsr;
>> +   atomic_t        Âlsr;
>
> These don't need to be atomic, please don't use them if they are not
> needed. ÂJust use the lock to protect updating them if needed.
>
At least lsr and msr are (or will be in patch 6) updated by the
interrupt_callback. I am happy to add another lock (for
interrupt-context it would need a spinlock rather than a mutex,
right?). I am just surprised that is considered cleaner than using
atomic_t.

For mcr I am not sure whether a race between tiocmget and tiocmset is
possible. I didn't see any obvious locking in usb-serial.c or
tty_io.c, that's why I used an atomic_t. Am I missing something?

> Care to respin this series based on the review so far?
Will do somewhere tomorrow. Mike and I are working out how to combine
our patches.

Groeten,
Bart

>
> thanks,
>
> greg k-h
>



-- 
Bart Hartgers - New e-mail: bart.hartgers@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/