[PATCH 1/5] HDQ Driver for OMAP2430/3430
From: Madhusudhan Chikkature <[EMAIL PROTECTED]> The HDQ/1-Wire module of TI OMAP2430/3430 platforms implement the hardware protocol of the master functions of the Benchmark HDQ and the Dallas Semiconductor 1-Wire protocols. These protocols use a single wire for communication between the master (HDQ/1-Wire controller) and the slave (HDQ/1-Wire external compliant device). This patch provides the HDQ driver to suppport TI OMAP2430/3430 platforms. Signed-off-by: Madhusudhan Chikkature <[EMAIL PROTECTED]> Acked-by: Felipe Balbi <[EMAIL PROTECTED]> Acked-by: Evgeniy Polyakov <[EMAIL PROTECTED]> --- Sending this series on behalf of Madhusudhan. drivers/w1/masters/Kconfig|7 drivers/w1/masters/Makefile |1 drivers/w1/masters/omap_hdq.c | 730 ++ 3 files changed, 738 insertions(+) Index: linux-2.6/drivers/w1/masters/Kconfig === --- linux-2.6.orig/drivers/w1/masters/Kconfig 2008-09-19 13:39:41.0 +0530 +++ linux-2.6/drivers/w1/masters/Kconfig2008-09-26 14:39:26.0 +0530 @@ -52,5 +52,12 @@ config W1_MASTER_GPIO This support is also available as a module. If so, the module will be called w1-gpio.ko. +config HDQ_MASTER_OMAP + tristate "OMAP HDQ driver" + depends on ARCH_OMAP2430 || ARCH_OMAP34XX + help + Say Y here if you want support for the 1-wire or HDQ Interface + on an OMAP processor. + endmenu Index: linux-2.6/drivers/w1/masters/Makefile === --- linux-2.6.orig/drivers/w1/masters/Makefile 2008-09-19 13:39:41.0 +0530 +++ linux-2.6/drivers/w1/masters/Makefile 2008-09-26 14:40:25.0 +0530 @@ -7,3 +7,4 @@ obj-$(CONFIG_W1_MASTER_DS2490) += ds249 obj-$(CONFIG_W1_MASTER_DS2482) += ds2482.o obj-$(CONFIG_W1_MASTER_DS1WM) += ds1wm.o obj-$(CONFIG_W1_MASTER_GPIO) += w1-gpio.o +obj-$(CONFIG_HDQ_MASTER_OMAP) += omap_hdq.o Index: linux-2.6/drivers/w1/masters/omap_hdq.c === --- /dev/null 1970-01-01 00:00:00.0 + +++ linux-2.6/drivers/w1/masters/omap_hdq.c 2008-09-26 14:28:36.0 +0530 @@ -0,0 +1,730 @@ +/* + * drivers/w1/masters/omap_hdq.c + * + * Copyright (C) 2007 Texas Instruments, Inc. + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + * + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../w1.h" +#include "../w1_int.h" + +#defineMOD_NAME"OMAP_HDQ:" + +#define OMAP_HDQ_REVISION 0x00 +#define OMAP_HDQ_TX_DATA 0x04 +#define OMAP_HDQ_RX_DATA 0x08 +#define OMAP_HDQ_CTRL_STATUS 0x0c +#define OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK (1<<6) +#define OMAP_HDQ_CTRL_STATUS_CLOCKENABLE (1<<5) +#define OMAP_HDQ_CTRL_STATUS_GO(1<<4) +#define OMAP_HDQ_CTRL_STATUS_INITIALIZATION(1<<2) +#define OMAP_HDQ_CTRL_STATUS_DIR (1<<1) +#define OMAP_HDQ_CTRL_STATUS_MODE (1<<0) +#define OMAP_HDQ_INT_STATUS0x10 +#define OMAP_HDQ_INT_STATUS_TXCOMPLETE (1<<2) +#define OMAP_HDQ_INT_STATUS_RXCOMPLETE (1<<1) +#define OMAP_HDQ_INT_STATUS_TIMEOUT(1<<0) +#define OMAP_HDQ_SYSCONFIG 0x14 +#define OMAP_HDQ_SYSCONFIG_SOFTRESET (1<<1) +#define OMAP_HDQ_SYSCONFIG_AUTOIDLE(1<<0) +#define OMAP_HDQ_SYSSTATUS 0x18 +#define OMAP_HDQ_SYSSTATUS_RESETDONE (1<<0) + +#define OMAP_HDQ_FLAG_CLEAR0 +#define OMAP_HDQ_FLAG_SET 1 +#define OMAP_HDQ_TIMEOUT (HZ/5) + +#define OMAP_HDQ_MAX_USER 4 + +static DECLARE_WAIT_QUEUE_HEAD(hdq_wait_queue); +static int w1_id; + +struct hdq_data { + struct device *dev; + void __iomem*hdq_base; + /* lock status update */ + struct mutex hdq_mutex; + int hdq_usecount; + struct clk *hdq_ick; + struct clk *hdq_fck; + u8 hdq_irqstatus; + /* device lock */ + spinlock_t hdq_spinlock; + /* +* Used to control the call to omap_hdq_get and omap_hdq_put. +* HDQ Protocol: Write the CMD|REG_address first, followed by +* the data wrire or read. +*/ + int init_trans; +}; + +static int omap_hdq_get(struct hdq_data *hdq_data); +static int omap_hdq_put(struct hdq_data *hdq_data); +static int omap_hdq_break(struct hdq_data *hdq_data); + +static int
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
Hi all. Andrew, please apply the whole serie to the upcoming tree when merge window is opened. Thank you. On Wed, Oct 08, 2008 at 12:46:25PM +0530, Gadiyar, Anand ([EMAIL PROTECTED]) wrote: > From: Madhusudhan Chikkature <[EMAIL PROTECTED]> > > The HDQ/1-Wire module of TI OMAP2430/3430 platforms implement the hardware > protocol of the master functions of the Benchmark HDQ and the Dallas > Semiconductor 1-Wire protocols. These protocols use a single wire for > communication between the master (HDQ/1-Wire controller) and the slave > (HDQ/1-Wire external compliant device). > > This patch provides the HDQ driver to suppport TI OMAP2430/3430 platforms. > > Signed-off-by: Madhusudhan Chikkature <[EMAIL PROTECTED]> > Acked-by: Felipe Balbi <[EMAIL PROTECTED]> > Acked-by: Evgeniy Polyakov <[EMAIL PROTECTED]> > --- > Sending this series on behalf of Madhusudhan. > > drivers/w1/masters/Kconfig|7 > drivers/w1/masters/Makefile |1 > drivers/w1/masters/omap_hdq.c | 730 > ++ > 3 files changed, 738 insertions(+) > > Index: linux-2.6/drivers/w1/masters/Kconfig > === > --- linux-2.6.orig/drivers/w1/masters/Kconfig 2008-09-19 13:39:41.0 > +0530 > +++ linux-2.6/drivers/w1/masters/Kconfig2008-09-26 14:39:26.0 > +0530 > @@ -52,5 +52,12 @@ config W1_MASTER_GPIO > This support is also available as a module. If so, the module > will be called w1-gpio.ko. > > +config HDQ_MASTER_OMAP > + tristate "OMAP HDQ driver" > + depends on ARCH_OMAP2430 || ARCH_OMAP34XX > + help > + Say Y here if you want support for the 1-wire or HDQ Interface > + on an OMAP processor. > + > endmenu > > Index: linux-2.6/drivers/w1/masters/Makefile > === > --- linux-2.6.orig/drivers/w1/masters/Makefile 2008-09-19 13:39:41.0 > +0530 > +++ linux-2.6/drivers/w1/masters/Makefile 2008-09-26 14:40:25.0 > +0530 > @@ -7,3 +7,4 @@ obj-$(CONFIG_W1_MASTER_DS2490) += ds249 > obj-$(CONFIG_W1_MASTER_DS2482) += ds2482.o > obj-$(CONFIG_W1_MASTER_DS1WM) += ds1wm.o > obj-$(CONFIG_W1_MASTER_GPIO) += w1-gpio.o > +obj-$(CONFIG_HDQ_MASTER_OMAP) += omap_hdq.o > Index: linux-2.6/drivers/w1/masters/omap_hdq.c > === > --- /dev/null 1970-01-01 00:00:00.0 + > +++ linux-2.6/drivers/w1/masters/omap_hdq.c 2008-09-26 14:28:36.0 > +0530 > @@ -0,0 +1,730 @@ > +/* > + * drivers/w1/masters/omap_hdq.c > + * > + * Copyright (C) 2007 Texas Instruments, Inc. > + * > + * This file is licensed under the terms of the GNU General Public License > + * version 2. This program is licensed "as is" without any warranty of any > + * kind, whether express or implied. > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "../w1.h" > +#include "../w1_int.h" > + > +#defineMOD_NAME"OMAP_HDQ:" > + > +#define OMAP_HDQ_REVISION 0x00 > +#define OMAP_HDQ_TX_DATA 0x04 > +#define OMAP_HDQ_RX_DATA 0x08 > +#define OMAP_HDQ_CTRL_STATUS 0x0c > +#define OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK (1<<6) > +#define OMAP_HDQ_CTRL_STATUS_CLOCKENABLE (1<<5) > +#define OMAP_HDQ_CTRL_STATUS_GO(1<<4) > +#define OMAP_HDQ_CTRL_STATUS_INITIALIZATION(1<<2) > +#define OMAP_HDQ_CTRL_STATUS_DIR (1<<1) > +#define OMAP_HDQ_CTRL_STATUS_MODE (1<<0) > +#define OMAP_HDQ_INT_STATUS0x10 > +#define OMAP_HDQ_INT_STATUS_TXCOMPLETE (1<<2) > +#define OMAP_HDQ_INT_STATUS_RXCOMPLETE (1<<1) > +#define OMAP_HDQ_INT_STATUS_TIMEOUT(1<<0) > +#define OMAP_HDQ_SYSCONFIG 0x14 > +#define OMAP_HDQ_SYSCONFIG_SOFTRESET (1<<1) > +#define OMAP_HDQ_SYSCONFIG_AUTOIDLE(1<<0) > +#define OMAP_HDQ_SYSSTATUS 0x18 > +#define OMAP_HDQ_SYSSTATUS_RESETDONE (1<<0) > + > +#define OMAP_HDQ_FLAG_CLEAR0 > +#define OMAP_HDQ_FLAG_SET 1 > +#define OMAP_HDQ_TIMEOUT (HZ/5) > + > +#define OMAP_HDQ_MAX_USER 4 > + > +static DECLARE_WAIT_QUEUE_HEAD(hdq_wait_queue); > +static int w1_id; > + > +struct hdq_data { > + struct device *dev; > + void __iomem*hdq_base; > + /* lock status update */ > + struct mutex hdq_mutex; > + int hdq_usecount; > + struct clk *hdq_ick; > + struct clk *hdq_fck; > + u8 hdq_irqstatus; > + /* device lock */ > + spinlock_t
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
On Wed, 8 Oct 2008 12:46:25 +0530 "Gadiyar, Anand" <[EMAIL PROTECTED]> wrote: > From: Madhusudhan Chikkature <[EMAIL PROTECTED]> > > The HDQ/1-Wire module of TI OMAP2430/3430 platforms implement the hardware > protocol of the master functions of the Benchmark HDQ and the Dallas > Semiconductor 1-Wire protocols. These protocols use a single wire for > communication between the master (HDQ/1-Wire controller) and the slave > (HDQ/1-Wire external compliant device). > > This patch provides the HDQ driver to suppport TI OMAP2430/3430 platforms. Every tab character in all five patches was converted to eight-spaces by your email client. Please fix the mailer and resend everything. > +++ linux-2.6/drivers/w1/masters/omap_hdq.c 2008-09-26 14:28:36.0 > +0530 > @@ -0,0 +1,730 @@ > +/* > + * drivers/w1/masters/omap_hdq.c > + * > + * Copyright (C) 2007 Texas Instruments, Inc. > + * > + * This file is licensed under the terms of the GNU General Public License > + * version 2. This program is licensed "as is" without any warranty of any > + * kind, whether express or implied. > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include We conventionally put a blank line between the linux/ includes and the asm/ includes. > +static int omap_hdq_get(struct hdq_data *hdq_data); > +static int omap_hdq_put(struct hdq_data *hdq_data); > +static int omap_hdq_break(struct hdq_data *hdq_data); These three aren't strictly needed, because these functions are defined before first use. I think it's best to not declare them. > +static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset, > + u8 flag, u8 flag_set, u8 *status) > +{ > + int ret = 0; > + unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT; > + > + if (flag_set == OMAP_HDQ_FLAG_CLEAR) { > + /* wait for the flag clear */ > + while (((*status = hdq_reg_in(hdq_data, offset)) & flag) > + && time_before(jiffies, timeout)) { > + set_current_state(TASK_UNINTERRUPTIBLE); > + schedule_timeout(1); Use schedule_timeout_uninterruptible(1) > + } > + if (*status & flag) > + ret = -ETIMEDOUT; > + } else if (flag_set == OMAP_HDQ_FLAG_SET) { > + /* wait for the flag set */ > + while (!((*status = hdq_reg_in(hdq_data, offset)) & flag) > + && time_before(jiffies, timeout)) { > + set_current_state(TASK_UNINTERRUPTIBLE); > + schedule_timeout(1); elsewhere.. > + } > + if (!(*status & flag)) > + ret = -ETIMEDOUT; > + } else > + return -EINVAL; > + > + return ret; > +} > + > +/* write out a byte and fill *status with HDQ_INT_STATUS */ > +static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status) > +{ > + int ret; > + u8 tmp_status; > + unsigned long irqflags; > + > + *status = 0; > + > + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags); > + /* clear interrupt flags via a dummy read */ > + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS); > + /* ISR loads it with new INT_STATUS */ > + hdq_data->hdq_irqstatus = 0; > + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags); > + > + hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val); > + > + /* set the GO bit */ > + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, OMAP_HDQ_CTRL_STATUS_GO, > + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); > + /* wait for the TXCOMPLETE bit */ > + ret = wait_event_interruptible_timeout(hdq_wait_queue, > + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT); > + if (ret < 0) { > + dev_dbg(hdq_data->dev, "wait interrupted"); > + return -EINTR; > + } Is this desirable? The user hits ^C and the driver bails out? I assume so, but was this tested? > + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags); > + *status = hdq_data->hdq_irqstatus; > + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags); It's unusual to put a lock around a single atomic move instruction. > + /* check irqstatus */ > + if (!(*status & OMAP_HDQ_INT_STATUS_TXCOMPLETE)) { > + dev_dbg(hdq_data->dev, "timeout waiting for" > + "TXCOMPLETE/RXCOMPLETE, %x", *status); > + return -ETIMEDOUT; > + } > + > + /* wait for the GO bit return to zero */ > + ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS, > + OMAP_HDQ_CTRL_STATUS_GO, > + OMAP_HDQ_FLAG_CLEAR, &tmp_status); > + if (ret) { > + dev_dbg(hdq_data->dev, "timeout waiting GO bit" > + "return to zero, %x", tm
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
- Original Message - From: "Andrew Morton" <[EMAIL PROTECTED]> To: "Gadiyar, Anand" <[EMAIL PROTECTED]> Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; ; <[EMAIL PROTECTED]> Sent: Saturday, October 11, 2008 2:08 AM Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 > On Wed, 8 Oct 2008 12:46:25 +0530 > "Gadiyar, Anand" <[EMAIL PROTECTED]> wrote: > >> From: Madhusudhan Chikkature <[EMAIL PROTECTED]> >> >> The HDQ/1-Wire module of TI OMAP2430/3430 platforms implement the hardware >> protocol of the master functions of the Benchmark HDQ and the Dallas >> Semiconductor 1-Wire protocols. These protocols use a single wire for >> communication between the master (HDQ/1-Wire controller) and the slave >> (HDQ/1-Wire external compliant device). >> >> This patch provides the HDQ driver to suppport TI OMAP2430/3430 platforms. > > Every tab character in all five patches was converted to eight-spaces by > your email client. Please fix the mailer and resend everything. > >> +++ linux-2.6/drivers/w1/masters/omap_hdq.c 2008-09-26 >> 14:28:36.0 +0530 >> @@ -0,0 +1,730 @@ >> +/* >> + * drivers/w1/masters/omap_hdq.c >> + * >> + * Copyright (C) 2007 Texas Instruments, Inc. >> + * >> + * This file is licensed under the terms of the GNU General Public License >> + * version 2. This program is licensed "as is" without any warranty of any >> + * kind, whether express or implied. >> + * >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > We conventionally put a blank line between the linux/ includes and the > asm/ includes. > >> +static int omap_hdq_get(struct hdq_data *hdq_data); >> +static int omap_hdq_put(struct hdq_data *hdq_data); >> +static int omap_hdq_break(struct hdq_data *hdq_data); > > These three aren't strictly needed, because these functions are defined > before first use. I think it's best to not declare them. > >> +static int hdq_wait_for_flag(struct hdq_data *hdq_data, u32 offset, >> + u8 flag, u8 flag_set, u8 *status) >> +{ >> + int ret = 0; >> + unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT; >> + >> + if (flag_set == OMAP_HDQ_FLAG_CLEAR) { >> + /* wait for the flag clear */ >> + while (((*status = hdq_reg_in(hdq_data, offset)) & flag) >> + && time_before(jiffies, timeout)) { >> + set_current_state(TASK_UNINTERRUPTIBLE); >> + schedule_timeout(1); > > Use schedule_timeout_uninterruptible(1) > >> + } >> + if (*status & flag) >> + ret = -ETIMEDOUT; >> + } else if (flag_set == OMAP_HDQ_FLAG_SET) { >> + /* wait for the flag set */ >> + while (!((*status = hdq_reg_in(hdq_data, offset)) & flag) >> + && time_before(jiffies, timeout)) { >> + set_current_state(TASK_UNINTERRUPTIBLE); >> + schedule_timeout(1); > > elsewhere.. > >> + } >> + if (!(*status & flag)) >> + ret = -ETIMEDOUT; >> + } else >> + return -EINVAL; >> + >> + return ret; >> +} >> + >> +/* write out a byte and fill *status with HDQ_INT_STATUS */ >> +static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status) >> +{ >> + int ret; >> + u8 tmp_status; >> + unsigned long irqflags; >> + >> + *status = 0; >> + >> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags); >> + /* clear interrupt flags via a dummy read */ >> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS); >> + /* ISR loads it with new INT_STATUS */ >> + hdq_data->hdq_irqstatus = 0; >> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags); >> + >> + hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val); >> + >> + /* set the GO bit */ >> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, >> OMAP_HDQ_CTRL_STATUS_GO, >> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); >> + /* wait for the TXCOMPLETE bit */ >> + ret = wait_event_interruptible_timeout(hdq_wait_queue, >> +
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
Hi. On Mon, Oct 13, 2008 at 06:55:43PM +0530, Madhusudhan Chikkature ([EMAIL PROTECTED]) wrote: > >> +static int hdq_write_byte(struct hdq_data *hdq_data, u8 val, u8 *status) > >> +{ > >> + int ret; > >> + u8 tmp_status; > >> + unsigned long irqflags; > >> + > >> + *status = 0; > >> + > >> + spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags); > >> + /* clear interrupt flags via a dummy read */ > >> + hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS); > >> + /* ISR loads it with new INT_STATUS */ > >> + hdq_data->hdq_irqstatus = 0; > >> + spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags); > >> + > >> + hdq_reg_out(hdq_data, OMAP_HDQ_TX_DATA, val); > >> + > >> + /* set the GO bit */ > >> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, > >> OMAP_HDQ_CTRL_STATUS_GO, > >> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); > >> + /* wait for the TXCOMPLETE bit */ > >> + ret = wait_event_interruptible_timeout(hdq_wait_queue, > >> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT); > >> + if (ret < 0) { > >> + dev_dbg(hdq_data->dev, "wait interrupted"); > >> + return -EINTR; > >> + } > > > > Is this desirable? The user hits ^C and the driver bails out? > > > > I assume so, but was this tested? > > Andrew, What is the test scenario you mean here? A user hitting ^C when the > driver is waiting for the TXCOMPLETE bit? Just plain return looks suspicious, is there some kind of a race between calling code (which for example frees hdq_data) and the path, which sets the bit and wakes up this thread? -- Evgeniy Polyakov -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
> On Mon, 13 Oct 2008 18:55:43 +0530 "Madhusudhan Chikkature" <[EMAIL > PROTECTED]> wrote: > > - Original Message - > From: "Andrew Morton" <[EMAIL PROTECTED]> > To: "Gadiyar, Anand" <[EMAIL PROTECTED]> > Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; ; > <[EMAIL PROTECTED]> > Sent: Saturday, October 11, 2008 2:08 AM > Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 > > > >> + /* set the GO bit */ > >> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, > >> OMAP_HDQ_CTRL_STATUS_GO, > >> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); > >> + /* wait for the TXCOMPLETE bit */ > >> + ret = wait_event_interruptible_timeout(hdq_wait_queue, > >> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT); > >> + if (ret < 0) { > >> + dev_dbg(hdq_data->dev, "wait interrupted"); > >> + return -EINTR; > >> + } > > > > Is this desirable? The user hits ^C and the driver bails out? > > > > I assume so, but was this tested? > > Andrew, What is the test scenario you mean here? A user hitting ^C when the > driver is waiting for the TXCOMPLETE bit? Yes. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
On Tue, Oct 14, 2008 at 07:30:58AM -0700, Andrew Morton ([EMAIL PROTECTED]) wrote: > > Why not just skipping the waiting and returning error pretending user > > really sent a signal? > > Better than nothing, but because signal_pending() isn't actually true, > upper layers wil behave differently. If they check... For example omap_hdq_break() can be interrupted and omap_hdq_probe() does not check its return value. -- Evgeniy Polyakov -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
> On Tue, 14 Oct 2008 14:21:50 +0530 "Madhusudhan Chikkature" <[EMAIL > PROTECTED]> wrote: > > - Original Message - > From: "Andrew Morton" <[EMAIL PROTECTED]> > To: "Madhusudhan Chikkature" <[EMAIL PROTECTED]> > Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; > > Sent: Monday, October 13, 2008 9:23 PM > Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 > > > >> On Mon, 13 Oct 2008 18:55:43 +0530 "Madhusudhan Chikkature" <[EMAIL > >> PROTECTED]> wrote: > >> > >> - Original Message - > >> From: "Andrew Morton" <[EMAIL PROTECTED]> > >> To: "Gadiyar, Anand" <[EMAIL PROTECTED]> > >> Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; > >> ; <[EMAIL PROTECTED]> > >> Sent: Saturday, October 11, 2008 2:08 AM > >> Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 > >> > >> > >> >> + /* set the GO bit */ > >> >> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, > >> >> OMAP_HDQ_CTRL_STATUS_GO, > >> >> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); > >> >> + /* wait for the TXCOMPLETE bit */ > >> >> + ret = wait_event_interruptible_timeout(hdq_wait_queue, > >> >> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT); > >> >> + if (ret < 0) { > >> >> + dev_dbg(hdq_data->dev, "wait interrupted"); > >> >> + return -EINTR; > >> >> + } > >> > > >> > Is this desirable? The user hits ^C and the driver bails out? > >> > > >> > I assume so, but was this tested? > >> > >> Andrew, What is the test scenario you mean here? A user hitting ^C when > >> the driver is waiting for the TXCOMPLETE bit? > > > > Yes. > > > > Yes. It is desired to return an error if the condition in the wait is not > met. I need to change the check for return value to check for zero and neg > value. > > I spent some time to test this perticular scenario.I could not really see any > impact of hitting ^C when an application is > transfering data in the background. When the h/w is programmed to transfer > data and the driver issues a wait, I see that > TXCOMPLETE interrupt comes immediately and wakes up as expected. > > So guess I am unable to hit ^C exactly when the driver is waiting in > wait_event_interruptible_timeout(before the condition > is met) for it to catch the signal. Is it generally suggested to use > wait_event_timeout so that ^C signal is not caught? > I think it's reasonable to permit the driver's operations to be interrupted in this manner. It's done in quite a few other places. But the problem is actually *testing* it. I guess one could do a whitebox-style test. Add new code like: { static int x; if (!(x++ % 1000)) { printk("hit ^c now\n"); msleep(2000); } } in the right place. Tricky. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
> On Tue, 14 Oct 2008 17:42:49 +0400 Evgeniy Polyakov <[EMAIL PROTECTED]> wrote: > Hi. > > On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton ([EMAIL PROTECTED]) > wrote: > > I think it's reasonable to permit the driver's operations to be interrupted > > in this manner. It's done in quite a few other places. But the problem is > > actually *testing* it. > > Why not just skipping the waiting and returning error pretending user > really sent a signal? Better than nothing, but because signal_pending() isn't actually true, upper layers wil behave differently. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
Hi. On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton ([EMAIL PROTECTED]) wrote: > I think it's reasonable to permit the driver's operations to be interrupted > in this manner. It's done in quite a few other places. But the problem is > actually *testing* it. Why not just skipping the waiting and returning error pretending user really sent a signal? -- Evgeniy Polyakov -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
- Original Message - From: "Andrew Morton" <[EMAIL PROTECTED]> To: "Madhusudhan Chikkature" <[EMAIL PROTECTED]> Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; Sent: Monday, October 13, 2008 9:23 PM Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 >> On Mon, 13 Oct 2008 18:55:43 +0530 "Madhusudhan Chikkature" <[EMAIL >> PROTECTED]> wrote: >> >> - Original Message - >> From: "Andrew Morton" <[EMAIL PROTECTED]> >> To: "Gadiyar, Anand" <[EMAIL PROTECTED]> >> Cc: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>; ; >> <[EMAIL PROTECTED]> >> Sent: Saturday, October 11, 2008 2:08 AM >> Subject: Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430 >> >> >> >> + /* set the GO bit */ >> >> + hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, >> >> OMAP_HDQ_CTRL_STATUS_GO, >> >> + OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO); >> >> + /* wait for the TXCOMPLETE bit */ >> >> + ret = wait_event_interruptible_timeout(hdq_wait_queue, >> >> + hdq_data->hdq_irqstatus, OMAP_HDQ_TIMEOUT); >> >> + if (ret < 0) { >> >> + dev_dbg(hdq_data->dev, "wait interrupted"); >> >> + return -EINTR; >> >> + } >> > >> > Is this desirable? The user hits ^C and the driver bails out? >> > >> > I assume so, but was this tested? >> >> Andrew, What is the test scenario you mean here? A user hitting ^C when the >> driver is waiting for the TXCOMPLETE bit? > > Yes. > Yes. It is desired to return an error if the condition in the wait is not met. I need to change the check for return value to check for zero and neg value. I spent some time to test this perticular scenario.I could not really see any impact of hitting ^C when an application is transfering data in the background. When the h/w is programmed to transfer data and the driver issues a wait, I see that TXCOMPLETE interrupt comes immediately and wakes up as expected. So guess I am unable to hit ^C exactly when the driver is waiting in wait_event_interruptible_timeout(before the condition is met) for it to catch the signal. Is it generally suggested to use wait_event_timeout so that ^C signal is not caught? Regards, Madhu -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
> On Tue, Oct 14, 2008 at 07:30:58AM -0700, Andrew Morton > ([EMAIL PROTECTED]) wrote: >> > Why not just skipping the waiting and returning error pretending user >> > really sent a signal? >> >> Better than nothing, but because signal_pending() isn't actually true, >> upper layers wil behave differently. > > If they check... > > For example omap_hdq_break() can be interrupted and omap_hdq_probe() > does not check its return value. > > -- > Evgeniy Polyakov > > Yes. I got the point. But the omap_hdq_break is not significant for HDQ. It just resets the slave and in HDQ mode no response is expected. So the driver will still work even if omap_hdq_break fails. On the TX path it is important to check for the failure which was missing. I will add that check. On the other note, let me correct myself that catching the signal on that very small wait in TX path may not be desired by the driver. The ISR wakes it up as expected. I intend to use "wait_event_timeout" instead and exit in the error path if timeout elapsed. I will repost the whole series after fixing the comments provided by Andrew. Thanks, Madhu -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html