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]; linux-omap@vger.kernel.org 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]; linux-omap@vger.kernel.org; [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]; linux-omap@vger.kernel.org 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]; linux-omap@vger.kernel.org; [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
- Original Message - From: Andrew Morton [EMAIL PROTECTED] To: Gadiyar, Anand [EMAIL PROTECTED] Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-omap@vger.kernel.org; [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 linux/kernel.h +#include linux/module.h +#include linux/platform_device.h +#include linux/interrupt.h +#include linux/err.h +#include linux/clk.h +#include linux/io.h +#include asm/irq.h +#include mach/hardware.h 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? Andrew, What is the test scenario you mean here? A user hitting ^C when the driver is waiting for the TXCOMPLETE bit? + 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
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]; linux-omap@vger.kernel.org; [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 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 linux/kernel.h +#include linux/module.h +#include linux/platform_device.h +#include linux/interrupt.h +#include linux/err.h +#include linux/clk.h +#include linux/io.h +#include asm/irq.h +#include mach/hardware.h 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, tmp_status); +