LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/5] HDQ Driver for OMAP2430/3430
@ 2008-10-08  7:16 Gadiyar, Anand
  2008-10-08 19:59 ` Evgeniy Polyakov
  2008-10-10 20:38 ` Andrew Morton
  0 siblings, 2 replies; 12+ messages in thread
From: Gadiyar, Anand @ 2008-10-08  7:16 UTC (permalink / raw)
  To: johnpol; +Cc: linux-kernel, linux-omap, Chikkature Rajashekar, Madhusudhan

From: Madhusudhan Chikkature <madhu.cr@ti.com>

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 <madhu.cr@ti.com>
Acked-by: Felipe Balbi <felipe.balbi@nokia.com>
Acked-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
---
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.000000000 +0530
+++ linux-2.6/drivers/w1/masters/Kconfig        2008-09-26 14:39:26.000000000 +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.000000000 +0530
+++ linux-2.6/drivers/w1/masters/Makefile       2008-09-26 14:40:25.000000000 +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.000000000 +0000
+++ linux-2.6/drivers/w1/masters/omap_hdq.c     2008-09-26 14:28:36.000000000 +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>
+
+#include "../w1.h"
+#include "../w1_int.h"
+
+#define        MOD_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_STATUS                    0x10
+#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_CLEAR                    0
+#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 __init omap_hdq_probe(struct platform_device *pdev);
+static int omap_hdq_remove(struct platform_device *pdev);
+
+static struct platform_driver omap_hdq_driver = {
+       .probe =        omap_hdq_probe,
+       .remove =       omap_hdq_remove,
+       .driver =       {
+               .name = "omap_hdq",
+       },
+};
+
+static u8 omap_w1_read_byte(void *_hdq);
+static void omap_w1_write_byte(void *_hdq, u8 byte);
+static u8 omap_w1_reset_bus(void *_hdq);
+static void omap_w1_search_bus(void *_hdq, u8 search_type,
+       w1_slave_found_callback slave_found);
+
+
+static struct w1_bus_master omap_w1_master = {
+       .read_byte      = omap_w1_read_byte,
+       .write_byte     = omap_w1_write_byte,
+       .reset_bus      = omap_w1_reset_bus,
+       .search         = omap_w1_search_bus,
+};
+
+/* HDQ register I/O routines */
+static inline u8 hdq_reg_in(struct hdq_data *hdq_data, u32 offset)
+{
+       return __raw_readb(hdq_data->hdq_base + offset);
+}
+
+static inline void hdq_reg_out(struct hdq_data *hdq_data, u32 offset, u8 val)
+{
+       __raw_writeb(val, hdq_data->hdq_base + offset);
+}
+
+static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
+                       u8 val, u8 mask)
+{
+       u8 new_val = (__raw_readb(hdq_data->hdq_base + offset) & ~mask)
+                       | (val & mask);
+       __raw_writeb(new_val, hdq_data->hdq_base + offset);
+
+       return new_val;
+}
+
+/*
+ * Wait for one or more bits in flag change.
+ * HDQ_FLAG_SET: wait until any bit in the flag is set.
+ * HDQ_FLAG_CLEAR: wait until all bits in the flag are cleared.
+ * return 0 on success and -ETIMEDOUT in the case of timeout.
+ */
+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);
+               }
+               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);
+               }
+               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;
+       }
+
+       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
+       *status = hdq_data->hdq_irqstatus;
+       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
+       /* 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);
+               return ret;
+       }
+
+       return ret;
+}
+
+/* HDQ Interrupt service routine */
+static irqreturn_t hdq_isr(int irq, void *_hdq)
+{
+       struct hdq_data *hdq_data = _hdq;
+       unsigned long irqflags;
+
+       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
+       hdq_data->hdq_irqstatus = hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
+       dev_dbg(hdq_data->dev, "hdq_isr: %x", hdq_data->hdq_irqstatus);
+
+       if (hdq_data->hdq_irqstatus &
+               (OMAP_HDQ_INT_STATUS_TXCOMPLETE | OMAP_HDQ_INT_STATUS_RXCOMPLETE
+               | OMAP_HDQ_INT_STATUS_TIMEOUT)) {
+               /* wake up sleeping process */
+               wake_up_interruptible(&hdq_wait_queue);
+       }
+
+       return IRQ_HANDLED;
+}
+
+/* HDQ Mode: always return success */
+static u8 omap_w1_reset_bus(void *_hdq)
+{
+       return 0;
+}
+
+/* W1 search callback function */
+static void omap_w1_search_bus(void *_hdq, u8 search_type,
+       w1_slave_found_callback slave_found)
+{
+       u64 module_id, rn_le, cs, id;
+
+       if (w1_id)
+               module_id = w1_id;
+       else
+               module_id = 0x1;
+
+       rn_le = cpu_to_le64(module_id);
+       /*
+        * HDQ might not obey truly the 1-wire spec.
+        * So calculate CRC based on module parameter.
+        */
+       cs = w1_calc_crc8((u8 *)&rn_le, 7);
+       id = (cs << 56) | module_id;
+
+       slave_found(_hdq, id);
+}
+
+static int _omap_hdq_reset(struct hdq_data *hdq_data)
+{
+       int ret;
+       u8 tmp_status;
+
+       hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG, OMAP_HDQ_SYSCONFIG_SOFTRESET);
+       /*
+        * Select HDQ mode & enable clocks.
+        * It is observed that INT flags can't be cleared via a read and GO/INIT
+        * won't return to zero if interrupt is disabled. So we always enable
+        * interrupt.
+        */
+       hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
+               OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
+               OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+
+       /* wait for reset to complete */
+       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
+               OMAP_HDQ_SYSSTATUS_RESETDONE, OMAP_HDQ_FLAG_SET, &tmp_status);
+       if (ret)
+               dev_dbg(hdq_data->dev, "timeout waiting HDQ reset, %x",
+                               tmp_status);
+       else {
+               hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
+                       OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
+                       OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+               hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
+                       OMAP_HDQ_SYSCONFIG_AUTOIDLE);
+       }
+
+       return ret;
+}
+
+/* Issue break pulse to the device */
+static int omap_hdq_break(struct hdq_data *hdq_data)
+{
+       int ret;
+       u8 tmp_status;
+       unsigned long irqflags;
+
+       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+       if (ret < 0)
+               return -EINTR;
+
+       if (!hdq_data->hdq_usecount) {
+               mutex_unlock(&hdq_data->hdq_mutex);
+               return -EINVAL;
+       }
+
+       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);
+
+       /* set the INIT and GO bit */
+       hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
+               OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
+               OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
+               OMAP_HDQ_CTRL_STATUS_GO);
+
+       /* wait for the TIMEOUT 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");
+               mutex_unlock(&hdq_data->hdq_mutex);
+               return -EINTR;
+       }
+
+       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
+       tmp_status = hdq_data->hdq_irqstatus;
+       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
+       /* check irqstatus */
+       if (!(tmp_status & OMAP_HDQ_INT_STATUS_TIMEOUT)) {
+               dev_dbg(hdq_data->dev, "timeout waiting for TIMEOUT, %x",
+                               tmp_status);
+               mutex_unlock(&hdq_data->hdq_mutex);
+               return -ETIMEDOUT;
+       }
+       /*
+        * wait for both INIT and GO bits rerurn to zero.
+        * zero wait time expected for interrupt mode.
+        */
+       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
+                       OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
+                       OMAP_HDQ_CTRL_STATUS_GO, OMAP_HDQ_FLAG_CLEAR,
+                       &tmp_status);
+       if (ret)
+               dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
+                       "return to zero, %x", tmp_status);
+
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       return ret;
+}
+
+static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
+{
+       int ret;
+       u8 status;
+       unsigned long irqflags;
+       unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
+
+       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+       if (ret < 0)
+               return -EINTR;
+
+       if (!hdq_data->hdq_usecount) {
+               mutex_unlock(&hdq_data->hdq_mutex);
+               return -EINVAL;
+       }
+
+       if (!(hdq_data->hdq_irqstatus & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
+               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
+                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
+                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
+               /*
+                * The RX comes immediately after TX. It
+                * triggers another interrupt before we
+                * sleep. So we have to wait for RXCOMPLETE bit.
+                */
+               while (!(hdq_data->hdq_irqstatus
+                       & OMAP_HDQ_INT_STATUS_RXCOMPLETE)
+                       && time_before(jiffies, timeout)) {
+                       set_current_state(TASK_UNINTERRUPTIBLE);
+                       schedule_timeout(1);
+               }
+               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
+                       OMAP_HDQ_CTRL_STATUS_DIR);
+               spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
+               status = hdq_data->hdq_irqstatus;
+               spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
+               /* check irqstatus */
+               if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
+                       dev_dbg(hdq_data->dev, "timeout waiting for"
+                               "RXCOMPLETE, %x", status);
+                       mutex_unlock(&hdq_data->hdq_mutex);
+                       return -ETIMEDOUT;
+               }
+       }
+       /* the data is ready. Read it in! */
+       *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       return 0;
+
+}
+
+/* Enable clocks and set the controller to HDQ mode */
+static int omap_hdq_get(struct hdq_data *hdq_data)
+{
+       int ret = 0;
+
+       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+       if (ret < 0)
+               return -EINTR;
+
+       if (OMAP_HDQ_MAX_USER == hdq_data->hdq_usecount) {
+               dev_dbg(hdq_data->dev, "attempt to exceed the max use count");
+               mutex_unlock(&hdq_data->hdq_mutex);
+               ret = -EINVAL;
+       } else {
+               hdq_data->hdq_usecount++;
+               try_module_get(THIS_MODULE);
+               if (1 == hdq_data->hdq_usecount) {
+                       if (clk_enable(hdq_data->hdq_ick)) {
+                               dev_dbg(hdq_data->dev, "Can not enable ick\n");
+                               clk_put(hdq_data->hdq_ick);
+                               clk_put(hdq_data->hdq_fck);
+                               mutex_unlock(&hdq_data->hdq_mutex);
+                               return -ENODEV;
+                       }
+                       if (clk_enable(hdq_data->hdq_fck)) {
+                               dev_dbg(hdq_data->dev, "Can not enable fck\n");
+                               clk_put(hdq_data->hdq_ick);
+                               clk_put(hdq_data->hdq_fck);
+                               mutex_unlock(&hdq_data->hdq_mutex);
+                               return -ENODEV;
+                       }
+
+                       /* make sure HDQ is out of reset */
+                       if (!(hdq_reg_in(hdq_data, OMAP_HDQ_SYSSTATUS) &
+                               OMAP_HDQ_SYSSTATUS_RESETDONE)) {
+                               ret = _omap_hdq_reset(hdq_data);
+                               if (ret)
+                                       /* back up the count */
+                                       hdq_data->hdq_usecount--;
+                       } else {
+                               /* select HDQ mode & enable clocks */
+                               hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
+                                       OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
+                                       OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+                               hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
+                                       OMAP_HDQ_SYSCONFIG_AUTOIDLE);
+                               hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
+                       }
+               }
+       }
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       return ret;
+}
+
+/* Disable clocks to the module */
+static int omap_hdq_put(struct hdq_data *hdq_data)
+{
+       int ret = 0;
+
+       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+       if (ret < 0)
+               return -EINTR;
+
+       if (0 == hdq_data->hdq_usecount) {
+               dev_dbg(hdq_data->dev, "attempt to decrement use count"
+                       "when it is zero");
+               ret = -EINVAL;
+       } else {
+               hdq_data->hdq_usecount--;
+               module_put(THIS_MODULE);
+               if (0 == hdq_data->hdq_usecount) {
+                       clk_disable(hdq_data->hdq_ick);
+                       clk_disable(hdq_data->hdq_fck);
+               }
+       }
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       return ret;
+}
+
+/* Read a byte of data from the device */
+static u8 omap_w1_read_byte(void *_hdq)
+{
+       struct hdq_data *hdq_data = _hdq;
+       u8 val;
+       int ret;
+
+       ret = hdq_read_byte(hdq_data, &val);
+       if (ret) {
+               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+               if (ret < 0) {
+                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
+                       return -EINTR;
+               }
+               hdq_data->init_trans = 0;
+               mutex_unlock(&hdq_data->hdq_mutex);
+               omap_hdq_put(hdq_data);
+               return -1;
+       }
+
+       /* Write followed by a read, release the module */
+       if (hdq_data->init_trans) {
+               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+               if (ret < 0) {
+                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
+                       return -EINTR;
+               }
+               hdq_data->init_trans = 0;
+               mutex_unlock(&hdq_data->hdq_mutex);
+               omap_hdq_put(hdq_data);
+       }
+
+       return val;
+}
+
+/* Write a byte of data to the device */
+static void omap_w1_write_byte(void *_hdq, u8 byte)
+{
+       struct hdq_data *hdq_data = _hdq;
+       int ret;
+       u8 status;
+
+       /* First write to initialize the transfer */
+       if (hdq_data->init_trans == 0)
+               omap_hdq_get(hdq_data);
+
+       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+       if (ret < 0) {
+               dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
+               return;
+       }
+       hdq_data->init_trans++;
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       hdq_write_byte(hdq_data, byte, &status);
+       dev_dbg(hdq_data->dev, "Ctrl status %x\n", status);
+
+       /* Second write, data transfered. Release the module */
+       if (hdq_data->init_trans > 1) {
+               omap_hdq_put(hdq_data);
+               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
+               if (ret < 0) {
+                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
+                       return;
+               }
+               hdq_data->init_trans = 0;
+               mutex_unlock(&hdq_data->hdq_mutex);
+       }
+
+       return;
+}
+
+static int __init omap_hdq_probe(struct platform_device *pdev)
+{
+       struct hdq_data *hdq_data;
+       struct resource *res;
+       int ret, irq;
+       u8 rev;
+
+       if (!pdev)
+               return -ENODEV;
+
+       hdq_data = kmalloc(sizeof(*hdq_data), GFP_KERNEL);
+       if (!hdq_data) {
+               dev_dbg(&pdev->dev, "unable to allocate memory\n");
+               ret = -ENODEV;
+               goto err_kmalloc;
+       }
+
+       hdq_data->dev = &pdev->dev;
+       platform_set_drvdata(pdev, hdq_data);
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!res) {
+               dev_dbg(&pdev->dev, "unable to get resource\n");
+               ret = ENXIO;
+               goto err_resource;
+       }
+
+       hdq_data->hdq_base = ioremap(res->start, SZ_4K);
+       if (!hdq_data->hdq_base) {
+               dev_dbg(&pdev->dev, "ioremap failed\n");
+               ret = -EINVAL;
+               goto err_ioremap;
+       }
+
+       /* get interface & functional clock objects */
+       hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
+       hdq_data->hdq_fck = clk_get(&pdev->dev, "hdq_fck");
+
+       if (IS_ERR(hdq_data->hdq_ick) || IS_ERR(hdq_data->hdq_fck)) {
+               dev_dbg(&pdev->dev, "Can't get HDQ clock objects\n");
+               if (IS_ERR(hdq_data->hdq_ick)) {
+                       ret = PTR_ERR(hdq_data->hdq_ick);
+                       goto err_clk;
+               }
+               if (IS_ERR(hdq_data->hdq_fck)) {
+                       ret = PTR_ERR(hdq_data->hdq_fck);
+                       clk_put(hdq_data->hdq_ick);
+                       goto err_clk;
+               }
+       }
+
+       hdq_data->hdq_usecount = 0;
+       mutex_init(&hdq_data->hdq_mutex);
+
+       if (clk_enable(hdq_data->hdq_ick)) {
+               dev_dbg(&pdev->dev, "Can not enable ick\n");
+               ret = -ENODEV;
+               goto err_ick;
+       }
+
+       if (clk_enable(hdq_data->hdq_fck)) {
+               dev_dbg(&pdev->dev, "Can not enable fck\n");
+               ret = -ENODEV;
+               goto err_fck;
+       }
+
+       rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
+       dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
+               (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
+
+       spin_lock_init(&hdq_data->hdq_spinlock);
+       omap_hdq_break(hdq_data);
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               ret = -ENXIO;
+               goto err_irq;
+       }
+
+       ret = request_irq(irq, hdq_isr, IRQF_DISABLED, "omap_hdq", hdq_data);
+       if (ret < 0) {
+               dev_dbg(&pdev->dev, "could not request irq\n");
+               goto err_irq;
+       }
+
+       /* don't clock the HDQ until it is needed */
+       clk_disable(hdq_data->hdq_ick);
+       clk_disable(hdq_data->hdq_fck);
+
+       omap_w1_master.data = hdq_data;
+
+       ret = w1_add_master_device(&omap_w1_master);
+       if (ret) {
+               dev_dbg(&pdev->dev, "Failure in registering w1 master\n");
+               goto err_w1;
+       }
+
+       return 0;
+
+err_w1:
+err_irq:
+       clk_disable(hdq_data->hdq_fck);
+
+err_fck:
+       clk_disable(hdq_data->hdq_ick);
+
+err_ick:
+       clk_put(hdq_data->hdq_ick);
+       clk_put(hdq_data->hdq_fck);
+
+err_clk:
+       iounmap(hdq_data->hdq_base);
+
+err_ioremap:
+err_resource:
+       platform_set_drvdata(pdev, NULL);
+       kfree(hdq_data);
+
+err_kmalloc:
+       return ret;
+
+}
+
+static int omap_hdq_remove(struct platform_device *pdev)
+{
+       struct hdq_data *hdq_data = platform_get_drvdata(pdev);
+
+       mutex_lock(&hdq_data->hdq_mutex);
+
+       if (0 != hdq_data->hdq_usecount) {
+               dev_dbg(&pdev->dev, "removed when use count is not zero\n");
+               return -EBUSY;
+       }
+
+       mutex_unlock(&hdq_data->hdq_mutex);
+
+       /* remove module dependency */
+       clk_put(hdq_data->hdq_ick);
+       clk_put(hdq_data->hdq_fck);
+       free_irq(INT_24XX_HDQ_IRQ, hdq_data);
+       platform_set_drvdata(pdev, NULL);
+       iounmap(hdq_data->hdq_base);
+       kfree(hdq_data);
+
+       return 0;
+}
+
+static int __init
+omap_hdq_init(void)
+{
+       return platform_driver_register(&omap_hdq_driver);
+}
+module_init(omap_hdq_init);
+
+static void __exit
+omap_hdq_exit(void)
+{
+       platform_driver_unregister(&omap_hdq_driver);
+}
+module_exit(omap_hdq_exit);
+
+module_param(w1_id, int, S_IRUSR);
+MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection");
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("HDQ driver Library");
+MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-08  7:16 [PATCH 1/5] HDQ Driver for OMAP2430/3430 Gadiyar, Anand
@ 2008-10-08 19:59 ` Evgeniy Polyakov
  2008-10-10 20:38 ` Andrew Morton
  1 sibling, 0 replies; 12+ messages in thread
From: Evgeniy Polyakov @ 2008-10-08 19:59 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, linux-omap, Chikkature Rajashekar, Madhusudhan,
	Gadiyar, Anand

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 (gadiyar@ti.com) wrote:
> From: Madhusudhan Chikkature <madhu.cr@ti.com>
> 
> 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 <madhu.cr@ti.com>
> Acked-by: Felipe Balbi <felipe.balbi@nokia.com>
> Acked-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> ---
> 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.000000000 +0530
> +++ linux-2.6/drivers/w1/masters/Kconfig        2008-09-26 14:39:26.000000000 +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.000000000 +0530
> +++ linux-2.6/drivers/w1/masters/Makefile       2008-09-26 14:40:25.000000000 +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.000000000 +0000
> +++ linux-2.6/drivers/w1/masters/omap_hdq.c     2008-09-26 14:28:36.000000000 +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>
> +
> +#include "../w1.h"
> +#include "../w1_int.h"
> +
> +#define        MOD_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_STATUS                    0x10
> +#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_CLEAR                    0
> +#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 __init omap_hdq_probe(struct platform_device *pdev);
> +static int omap_hdq_remove(struct platform_device *pdev);
> +
> +static struct platform_driver omap_hdq_driver = {
> +       .probe =        omap_hdq_probe,
> +       .remove =       omap_hdq_remove,
> +       .driver =       {
> +               .name = "omap_hdq",
> +       },
> +};
> +
> +static u8 omap_w1_read_byte(void *_hdq);
> +static void omap_w1_write_byte(void *_hdq, u8 byte);
> +static u8 omap_w1_reset_bus(void *_hdq);
> +static void omap_w1_search_bus(void *_hdq, u8 search_type,
> +       w1_slave_found_callback slave_found);
> +
> +
> +static struct w1_bus_master omap_w1_master = {
> +       .read_byte      = omap_w1_read_byte,
> +       .write_byte     = omap_w1_write_byte,
> +       .reset_bus      = omap_w1_reset_bus,
> +       .search         = omap_w1_search_bus,
> +};
> +
> +/* HDQ register I/O routines */
> +static inline u8 hdq_reg_in(struct hdq_data *hdq_data, u32 offset)
> +{
> +       return __raw_readb(hdq_data->hdq_base + offset);
> +}
> +
> +static inline void hdq_reg_out(struct hdq_data *hdq_data, u32 offset, u8 val)
> +{
> +       __raw_writeb(val, hdq_data->hdq_base + offset);
> +}
> +
> +static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
> +                       u8 val, u8 mask)
> +{
> +       u8 new_val = (__raw_readb(hdq_data->hdq_base + offset) & ~mask)
> +                       | (val & mask);
> +       __raw_writeb(new_val, hdq_data->hdq_base + offset);
> +
> +       return new_val;
> +}
> +
> +/*
> + * Wait for one or more bits in flag change.
> + * HDQ_FLAG_SET: wait until any bit in the flag is set.
> + * HDQ_FLAG_CLEAR: wait until all bits in the flag are cleared.
> + * return 0 on success and -ETIMEDOUT in the case of timeout.
> + */
> +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);
> +               }
> +               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);
> +               }
> +               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;
> +       }
> +
> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +       *status = hdq_data->hdq_irqstatus;
> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +       /* 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);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +/* HDQ Interrupt service routine */
> +static irqreturn_t hdq_isr(int irq, void *_hdq)
> +{
> +       struct hdq_data *hdq_data = _hdq;
> +       unsigned long irqflags;
> +
> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +       hdq_data->hdq_irqstatus = hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +       dev_dbg(hdq_data->dev, "hdq_isr: %x", hdq_data->hdq_irqstatus);
> +
> +       if (hdq_data->hdq_irqstatus &
> +               (OMAP_HDQ_INT_STATUS_TXCOMPLETE | OMAP_HDQ_INT_STATUS_RXCOMPLETE
> +               | OMAP_HDQ_INT_STATUS_TIMEOUT)) {
> +               /* wake up sleeping process */
> +               wake_up_interruptible(&hdq_wait_queue);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +/* HDQ Mode: always return success */
> +static u8 omap_w1_reset_bus(void *_hdq)
> +{
> +       return 0;
> +}
> +
> +/* W1 search callback function */
> +static void omap_w1_search_bus(void *_hdq, u8 search_type,
> +       w1_slave_found_callback slave_found)
> +{
> +       u64 module_id, rn_le, cs, id;
> +
> +       if (w1_id)
> +               module_id = w1_id;
> +       else
> +               module_id = 0x1;
> +
> +       rn_le = cpu_to_le64(module_id);
> +       /*
> +        * HDQ might not obey truly the 1-wire spec.
> +        * So calculate CRC based on module parameter.
> +        */
> +       cs = w1_calc_crc8((u8 *)&rn_le, 7);
> +       id = (cs << 56) | module_id;
> +
> +       slave_found(_hdq, id);
> +}
> +
> +static int _omap_hdq_reset(struct hdq_data *hdq_data)
> +{
> +       int ret;
> +       u8 tmp_status;
> +
> +       hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG, OMAP_HDQ_SYSCONFIG_SOFTRESET);
> +       /*
> +        * Select HDQ mode & enable clocks.
> +        * It is observed that INT flags can't be cleared via a read and GO/INIT
> +        * won't return to zero if interrupt is disabled. So we always enable
> +        * interrupt.
> +        */
> +       hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +               OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> +               OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> +
> +       /* wait for reset to complete */
> +       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
> +               OMAP_HDQ_SYSSTATUS_RESETDONE, OMAP_HDQ_FLAG_SET, &tmp_status);
> +       if (ret)
> +               dev_dbg(hdq_data->dev, "timeout waiting HDQ reset, %x",
> +                               tmp_status);
> +       else {
> +               hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                       OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> +                       OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> +               hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> +                       OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> +       }
> +
> +       return ret;
> +}
> +
> +/* Issue break pulse to the device */
> +static int omap_hdq_break(struct hdq_data *hdq_data)
> +{
> +       int ret;
> +       u8 tmp_status;
> +       unsigned long irqflags;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (!hdq_data->hdq_usecount) {
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINVAL;
> +       }
> +
> +       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);
> +
> +       /* set the INIT and GO bit */
> +       hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +               OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
> +               OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
> +               OMAP_HDQ_CTRL_STATUS_GO);
> +
> +       /* wait for the TIMEOUT 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");
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINTR;
> +       }
> +
> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +       tmp_status = hdq_data->hdq_irqstatus;
> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +       /* check irqstatus */
> +       if (!(tmp_status & OMAP_HDQ_INT_STATUS_TIMEOUT)) {
> +               dev_dbg(hdq_data->dev, "timeout waiting for TIMEOUT, %x",
> +                               tmp_status);
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -ETIMEDOUT;
> +       }
> +       /*
> +        * wait for both INIT and GO bits rerurn to zero.
> +        * zero wait time expected for interrupt mode.
> +        */
> +       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                       OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
> +                       OMAP_HDQ_CTRL_STATUS_GO, OMAP_HDQ_FLAG_CLEAR,
> +                       &tmp_status);
> +       if (ret)
> +               dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
> +                       "return to zero, %x", tmp_status);
> +
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return ret;
> +}
> +
> +static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
> +{
> +       int ret;
> +       u8 status;
> +       unsigned long irqflags;
> +       unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (!hdq_data->hdq_usecount) {
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINVAL;
> +       }
> +
> +       if (!(hdq_data->hdq_irqstatus & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
> +               /*
> +                * The RX comes immediately after TX. It
> +                * triggers another interrupt before we
> +                * sleep. So we have to wait for RXCOMPLETE bit.
> +                */
> +               while (!(hdq_data->hdq_irqstatus
> +                       & OMAP_HDQ_INT_STATUS_RXCOMPLETE)
> +                       && time_before(jiffies, timeout)) {
> +                       set_current_state(TASK_UNINTERRUPTIBLE);
> +                       schedule_timeout(1);
> +               }
> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
> +                       OMAP_HDQ_CTRL_STATUS_DIR);
> +               spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +               status = hdq_data->hdq_irqstatus;
> +               spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +               /* check irqstatus */
> +               if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
> +                       dev_dbg(hdq_data->dev, "timeout waiting for"
> +                               "RXCOMPLETE, %x", status);
> +                       mutex_unlock(&hdq_data->hdq_mutex);
> +                       return -ETIMEDOUT;
> +               }
> +       }
> +       /* the data is ready. Read it in! */
> +       *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return 0;
> +
> +}
> +
> +/* Enable clocks and set the controller to HDQ mode */
> +static int omap_hdq_get(struct hdq_data *hdq_data)
> +{
> +       int ret = 0;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (OMAP_HDQ_MAX_USER == hdq_data->hdq_usecount) {
> +               dev_dbg(hdq_data->dev, "attempt to exceed the max use count");
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               ret = -EINVAL;
> +       } else {
> +               hdq_data->hdq_usecount++;
> +               try_module_get(THIS_MODULE);
> +               if (1 == hdq_data->hdq_usecount) {
> +                       if (clk_enable(hdq_data->hdq_ick)) {
> +                               dev_dbg(hdq_data->dev, "Can not enable ick\n");
> +                               clk_put(hdq_data->hdq_ick);
> +                               clk_put(hdq_data->hdq_fck);
> +                               mutex_unlock(&hdq_data->hdq_mutex);
> +                               return -ENODEV;
> +                       }
> +                       if (clk_enable(hdq_data->hdq_fck)) {
> +                               dev_dbg(hdq_data->dev, "Can not enable fck\n");
> +                               clk_put(hdq_data->hdq_ick);
> +                               clk_put(hdq_data->hdq_fck);
> +                               mutex_unlock(&hdq_data->hdq_mutex);
> +                               return -ENODEV;
> +                       }
> +
> +                       /* make sure HDQ is out of reset */
> +                       if (!(hdq_reg_in(hdq_data, OMAP_HDQ_SYSSTATUS) &
> +                               OMAP_HDQ_SYSSTATUS_RESETDONE)) {
> +                               ret = _omap_hdq_reset(hdq_data);
> +                               if (ret)
> +                                       /* back up the count */
> +                                       hdq_data->hdq_usecount--;
> +                       } else {
> +                               /* select HDQ mode & enable clocks */
> +                               hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                                       OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
> +                                       OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
> +                               hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
> +                                       OMAP_HDQ_SYSCONFIG_AUTOIDLE);
> +                               hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
> +                       }
> +               }
> +       }
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return ret;
> +}
> +
> +/* Disable clocks to the module */
> +static int omap_hdq_put(struct hdq_data *hdq_data)
> +{
> +       int ret = 0;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (0 == hdq_data->hdq_usecount) {
> +               dev_dbg(hdq_data->dev, "attempt to decrement use count"
> +                       "when it is zero");
> +               ret = -EINVAL;
> +       } else {
> +               hdq_data->hdq_usecount--;
> +               module_put(THIS_MODULE);
> +               if (0 == hdq_data->hdq_usecount) {
> +                       clk_disable(hdq_data->hdq_ick);
> +                       clk_disable(hdq_data->hdq_fck);
> +               }
> +       }
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return ret;
> +}
> +
> +/* Read a byte of data from the device */
> +static u8 omap_w1_read_byte(void *_hdq)
> +{
> +       struct hdq_data *hdq_data = _hdq;
> +       u8 val;
> +       int ret;
> +
> +       ret = hdq_read_byte(hdq_data, &val);
> +       if (ret) {
> +               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +               if (ret < 0) {
> +                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> +                       return -EINTR;
> +               }
> +               hdq_data->init_trans = 0;
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               omap_hdq_put(hdq_data);
> +               return -1;
> +       }
> +
> +       /* Write followed by a read, release the module */
> +       if (hdq_data->init_trans) {
> +               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +               if (ret < 0) {
> +                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> +                       return -EINTR;
> +               }
> +               hdq_data->init_trans = 0;
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               omap_hdq_put(hdq_data);
> +       }
> +
> +       return val;
> +}
> +
> +/* Write a byte of data to the device */
> +static void omap_w1_write_byte(void *_hdq, u8 byte)
> +{
> +       struct hdq_data *hdq_data = _hdq;
> +       int ret;
> +       u8 status;
> +
> +       /* First write to initialize the transfer */
> +       if (hdq_data->init_trans == 0)
> +               omap_hdq_get(hdq_data);
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0) {
> +               dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> +               return;
> +       }
> +       hdq_data->init_trans++;
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       hdq_write_byte(hdq_data, byte, &status);
> +       dev_dbg(hdq_data->dev, "Ctrl status %x\n", status);
> +
> +       /* Second write, data transfered. Release the module */
> +       if (hdq_data->init_trans > 1) {
> +               omap_hdq_put(hdq_data);
> +               ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +               if (ret < 0) {
> +                       dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
> +                       return;
> +               }
> +               hdq_data->init_trans = 0;
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +       }
> +
> +       return;
> +}
> +
> +static int __init omap_hdq_probe(struct platform_device *pdev)
> +{
> +       struct hdq_data *hdq_data;
> +       struct resource *res;
> +       int ret, irq;
> +       u8 rev;
> +
> +       if (!pdev)
> +               return -ENODEV;
> +
> +       hdq_data = kmalloc(sizeof(*hdq_data), GFP_KERNEL);
> +       if (!hdq_data) {
> +               dev_dbg(&pdev->dev, "unable to allocate memory\n");
> +               ret = -ENODEV;
> +               goto err_kmalloc;
> +       }
> +
> +       hdq_data->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, hdq_data);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_dbg(&pdev->dev, "unable to get resource\n");
> +               ret = ENXIO;
> +               goto err_resource;
> +       }
> +
> +       hdq_data->hdq_base = ioremap(res->start, SZ_4K);
> +       if (!hdq_data->hdq_base) {
> +               dev_dbg(&pdev->dev, "ioremap failed\n");
> +               ret = -EINVAL;
> +               goto err_ioremap;
> +       }
> +
> +       /* get interface & functional clock objects */
> +       hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
> +       hdq_data->hdq_fck = clk_get(&pdev->dev, "hdq_fck");
> +
> +       if (IS_ERR(hdq_data->hdq_ick) || IS_ERR(hdq_data->hdq_fck)) {
> +               dev_dbg(&pdev->dev, "Can't get HDQ clock objects\n");
> +               if (IS_ERR(hdq_data->hdq_ick)) {
> +                       ret = PTR_ERR(hdq_data->hdq_ick);
> +                       goto err_clk;
> +               }
> +               if (IS_ERR(hdq_data->hdq_fck)) {
> +                       ret = PTR_ERR(hdq_data->hdq_fck);
> +                       clk_put(hdq_data->hdq_ick);
> +                       goto err_clk;
> +               }
> +       }
> +
> +       hdq_data->hdq_usecount = 0;
> +       mutex_init(&hdq_data->hdq_mutex);
> +
> +       if (clk_enable(hdq_data->hdq_ick)) {
> +               dev_dbg(&pdev->dev, "Can not enable ick\n");
> +               ret = -ENODEV;
> +               goto err_ick;
> +       }
> +
> +       if (clk_enable(hdq_data->hdq_fck)) {
> +               dev_dbg(&pdev->dev, "Can not enable fck\n");
> +               ret = -ENODEV;
> +               goto err_fck;
> +       }
> +
> +       rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
> +       dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
> +               (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
> +
> +       spin_lock_init(&hdq_data->hdq_spinlock);
> +       omap_hdq_break(hdq_data);
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               ret = -ENXIO;
> +               goto err_irq;
> +       }
> +
> +       ret = request_irq(irq, hdq_isr, IRQF_DISABLED, "omap_hdq", hdq_data);
> +       if (ret < 0) {
> +               dev_dbg(&pdev->dev, "could not request irq\n");
> +               goto err_irq;
> +       }
> +
> +       /* don't clock the HDQ until it is needed */
> +       clk_disable(hdq_data->hdq_ick);
> +       clk_disable(hdq_data->hdq_fck);
> +
> +       omap_w1_master.data = hdq_data;
> +
> +       ret = w1_add_master_device(&omap_w1_master);
> +       if (ret) {
> +               dev_dbg(&pdev->dev, "Failure in registering w1 master\n");
> +               goto err_w1;
> +       }
> +
> +       return 0;
> +
> +err_w1:
> +err_irq:
> +       clk_disable(hdq_data->hdq_fck);
> +
> +err_fck:
> +       clk_disable(hdq_data->hdq_ick);
> +
> +err_ick:
> +       clk_put(hdq_data->hdq_ick);
> +       clk_put(hdq_data->hdq_fck);
> +
> +err_clk:
> +       iounmap(hdq_data->hdq_base);
> +
> +err_ioremap:
> +err_resource:
> +       platform_set_drvdata(pdev, NULL);
> +       kfree(hdq_data);
> +
> +err_kmalloc:
> +       return ret;
> +
> +}
> +
> +static int omap_hdq_remove(struct platform_device *pdev)
> +{
> +       struct hdq_data *hdq_data = platform_get_drvdata(pdev);
> +
> +       mutex_lock(&hdq_data->hdq_mutex);
> +
> +       if (0 != hdq_data->hdq_usecount) {
> +               dev_dbg(&pdev->dev, "removed when use count is not zero\n");
> +               return -EBUSY;
> +       }
> +
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       /* remove module dependency */
> +       clk_put(hdq_data->hdq_ick);
> +       clk_put(hdq_data->hdq_fck);
> +       free_irq(INT_24XX_HDQ_IRQ, hdq_data);
> +       platform_set_drvdata(pdev, NULL);
> +       iounmap(hdq_data->hdq_base);
> +       kfree(hdq_data);
> +
> +       return 0;
> +}
> +
> +static int __init
> +omap_hdq_init(void)
> +{
> +       return platform_driver_register(&omap_hdq_driver);
> +}
> +module_init(omap_hdq_init);
> +
> +static void __exit
> +omap_hdq_exit(void)
> +{
> +       platform_driver_unregister(&omap_hdq_driver);
> +}
> +module_exit(omap_hdq_exit);
> +
> +module_param(w1_id, int, S_IRUSR);
> +MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection");
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_DESCRIPTION("HDQ driver Library");
> +MODULE_LICENSE("GPL");

-- 
	Evgeniy Polyakov

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-08  7:16 [PATCH 1/5] HDQ Driver for OMAP2430/3430 Gadiyar, Anand
  2008-10-08 19:59 ` Evgeniy Polyakov
@ 2008-10-10 20:38 ` Andrew Morton
  2008-10-13 13:25   ` Madhusudhan Chikkature
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2008-10-10 20:38 UTC (permalink / raw)
  To: Gadiyar, Anand; +Cc: johnpol, linux-kernel, linux-omap, madhu.cr

On Wed, 8 Oct 2008 12:46:25 +0530
"Gadiyar, Anand" <gadiyar@ti.com> wrote:

> From: Madhusudhan Chikkature <madhu.cr@ti.com>
> 
> 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.000000000 +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);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
>
> ...
>
> +/* Issue break pulse to the device */
> +static int omap_hdq_break(struct hdq_data *hdq_data)
> +{
> +       int ret;
> +       u8 tmp_status;
> +       unsigned long irqflags;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (!hdq_data->hdq_usecount) {
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINVAL;
> +       }
> +
> +       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);
> +
> +       /* set the INIT and GO bit */
> +       hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +               OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
> +               OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
> +               OMAP_HDQ_CTRL_STATUS_GO);
> +
> +       /* wait for the TIMEOUT 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");
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINTR;

Multiple-return-statements-per-function are a common source of
maintenance problems: locking errors, resource leaks.  This is why
kernel code usually does the `goto out' way of avoiding this.

So.. please consider.  In this case

	ret = -EINTR;
	goto out;

would fit nicely.

> +       }
> +
> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +       tmp_status = hdq_data->hdq_irqstatus;
> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +       /* check irqstatus */
> +       if (!(tmp_status & OMAP_HDQ_INT_STATUS_TIMEOUT)) {
> +               dev_dbg(hdq_data->dev, "timeout waiting for TIMEOUT, %x",
> +                               tmp_status);
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -ETIMEDOUT;

Here too.

> +       }
> +       /*
> +        * wait for both INIT and GO bits rerurn to zero.
> +        * zero wait time expected for interrupt mode.
> +        */
> +       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                       OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
> +                       OMAP_HDQ_CTRL_STATUS_GO, OMAP_HDQ_FLAG_CLEAR,
> +                       &tmp_status);
> +       if (ret)
> +               dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
> +                       "return to zero, %x", tmp_status);
> +
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return ret;
> +}
> +
> +static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
> +{
> +       int ret;
> +       u8 status;
> +       unsigned long irqflags;
> +       unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
> +
> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
> +       if (ret < 0)
> +               return -EINTR;
> +
> +       if (!hdq_data->hdq_usecount) {
> +               mutex_unlock(&hdq_data->hdq_mutex);
> +               return -EINVAL;
> +       }
> +
> +       if (!(hdq_data->hdq_irqstatus & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
> +               /*
> +                * The RX comes immediately after TX. It
> +                * triggers another interrupt before we
> +                * sleep. So we have to wait for RXCOMPLETE bit.
> +                */
> +               while (!(hdq_data->hdq_irqstatus
> +                       & OMAP_HDQ_INT_STATUS_RXCOMPLETE)
> +                       && time_before(jiffies, timeout)) {
> +                       set_current_state(TASK_UNINTERRUPTIBLE);
> +                       schedule_timeout(1);

schedule_timeout_uninterruptible(1)

If we were to implement the presently-missing
wait_event_uninterruptible_timeout() (like
wait_event_interruptible_timeout), could we use it here?

> +               }
> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
> +                       OMAP_HDQ_CTRL_STATUS_DIR);
> +               spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
> +               status = hdq_data->hdq_irqstatus;
> +               spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
> +               /* check irqstatus */
> +               if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
> +                       dev_dbg(hdq_data->dev, "timeout waiting for"
> +                               "RXCOMPLETE, %x", status);
> +                       mutex_unlock(&hdq_data->hdq_mutex);
> +                       return -ETIMEDOUT;
> +               }
> +       }
> +       /* the data is ready. Read it in! */
> +       *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       return 0;
> +
> +}
> +
>
> ...
>
> +static int __init omap_hdq_probe(struct platform_device *pdev)
> +{
> +       struct hdq_data *hdq_data;
> +       struct resource *res;
> +       int ret, irq;
> +       u8 rev;
> +
> +       if (!pdev)
> +               return -ENODEV;

Can this happen?

> +       hdq_data = kmalloc(sizeof(*hdq_data), GFP_KERNEL);
> +       if (!hdq_data) {
> +               dev_dbg(&pdev->dev, "unable to allocate memory\n");
> +               ret = -ENODEV;

-ENOMEM

> +               goto err_kmalloc;
> +       }
> +
> +       hdq_data->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, hdq_data);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_dbg(&pdev->dev, "unable to get resource\n");
> +               ret = ENXIO;

bzzt, that should have been -ENXIO.

> +               goto err_resource;
> +       }
> +
> +       hdq_data->hdq_base = ioremap(res->start, SZ_4K);
> +       if (!hdq_data->hdq_base) {
> +               dev_dbg(&pdev->dev, "ioremap failed\n");
> +               ret = -EINVAL;
> +               goto err_ioremap;
> +       }
> +
> +       /* get interface & functional clock objects */
> +       hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
> +       hdq_data->hdq_fck = clk_get(&pdev->dev, "hdq_fck");
> +
> +       if (IS_ERR(hdq_data->hdq_ick) || IS_ERR(hdq_data->hdq_fck)) {
> +               dev_dbg(&pdev->dev, "Can't get HDQ clock objects\n");
> +               if (IS_ERR(hdq_data->hdq_ick)) {
> +                       ret = PTR_ERR(hdq_data->hdq_ick);
> +                       goto err_clk;
> +               }
> +               if (IS_ERR(hdq_data->hdq_fck)) {
> +                       ret = PTR_ERR(hdq_data->hdq_fck);
> +                       clk_put(hdq_data->hdq_ick);
> +                       goto err_clk;
> +               }
> +       }
> +
> +       hdq_data->hdq_usecount = 0;
> +       mutex_init(&hdq_data->hdq_mutex);
> +
> +       if (clk_enable(hdq_data->hdq_ick)) {
> +               dev_dbg(&pdev->dev, "Can not enable ick\n");
> +               ret = -ENODEV;
> +               goto err_ick;
> +       }
> +
> +       if (clk_enable(hdq_data->hdq_fck)) {
> +               dev_dbg(&pdev->dev, "Can not enable fck\n");
> +               ret = -ENODEV;
> +               goto err_fck;
> +       }

ooh, I like err_ick and err_fck a lot.  They sound like akpm review
comments at the end of a long day.

> +       rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
> +       dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
> +               (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
> +
> +       spin_lock_init(&hdq_data->hdq_spinlock);
> +       omap_hdq_break(hdq_data);
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               ret = -ENXIO;
> +               goto err_irq;
> +       }
> +
> +       ret = request_irq(irq, hdq_isr, IRQF_DISABLED, "omap_hdq", hdq_data);
> +       if (ret < 0) {
> +               dev_dbg(&pdev->dev, "could not request irq\n");
> +               goto err_irq;
> +       }
> +
> +       /* don't clock the HDQ until it is needed */
> +       clk_disable(hdq_data->hdq_ick);
> +       clk_disable(hdq_data->hdq_fck);
> +
> +       omap_w1_master.data = hdq_data;
> +
> +       ret = w1_add_master_device(&omap_w1_master);
> +       if (ret) {
> +               dev_dbg(&pdev->dev, "Failure in registering w1 master\n");
> +               goto err_w1;
> +       }
> +
> +       return 0;
> +
> +err_w1:
> +err_irq:
> +       clk_disable(hdq_data->hdq_fck);
> +
> +err_fck:
> +       clk_disable(hdq_data->hdq_ick);
> +
> +err_ick:
> +       clk_put(hdq_data->hdq_ick);
> +       clk_put(hdq_data->hdq_fck);
> +
> +err_clk:
> +       iounmap(hdq_data->hdq_base);
> +
> +err_ioremap:
> +err_resource:
> +       platform_set_drvdata(pdev, NULL);
> +       kfree(hdq_data);
> +
> +err_kmalloc:
> +       return ret;
> +
> +}
> +
> +static int omap_hdq_remove(struct platform_device *pdev)
> +{
> +       struct hdq_data *hdq_data = platform_get_drvdata(pdev);
> +
> +       mutex_lock(&hdq_data->hdq_mutex);
> +
> +       if (0 != hdq_data->hdq_usecount) {

Well.  Lots of people dislike that trick.  It's used to catch
accidental use of = where == was intended, but the compiler will warn
anyway.  There's less point in using it for !=.

> +               dev_dbg(&pdev->dev, "removed when use count is not zero\n");
> +               return -EBUSY;
> +       }
> +
> +       mutex_unlock(&hdq_data->hdq_mutex);
> +
> +       /* remove module dependency */
> +       clk_put(hdq_data->hdq_ick);
> +       clk_put(hdq_data->hdq_fck);
> +       free_irq(INT_24XX_HDQ_IRQ, hdq_data);
> +       platform_set_drvdata(pdev, NULL);
> +       iounmap(hdq_data->hdq_base);
> +       kfree(hdq_data);
> +
> +       return 0;
> +}
> +
> +static int __init
> +omap_hdq_init(void)
> +{
> +       return platform_driver_register(&omap_hdq_driver);
> +}
> +module_init(omap_hdq_init);
> +
> +static void __exit
> +omap_hdq_exit(void)
> +{
> +       platform_driver_unregister(&omap_hdq_driver);
> +}
> +module_exit(omap_hdq_exit);
> +
> +module_param(w1_id, int, S_IRUSR);
> +MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection");
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_DESCRIPTION("HDQ driver Library");
> +MODULE_LICENSE("GPL");

The code looks pretty good.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-10 20:38 ` Andrew Morton
@ 2008-10-13 13:25   ` Madhusudhan Chikkature
  2008-10-13 13:42     ` Evgeniy Polyakov
  2008-10-13 15:53     ` Andrew Morton
  0 siblings, 2 replies; 12+ messages in thread
From: Madhusudhan Chikkature @ 2008-10-13 13:25 UTC (permalink / raw)
  To: Andrew Morton, Gadiyar, Anand; +Cc: johnpol, linux-kernel, linux-omap


----- Original Message ----- 
From: "Andrew Morton" <akpm@linux-foundation.org>
To: "Gadiyar, Anand" <gadiyar@ti.com>
Cc: <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; <madhu.cr@ti.com>
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" <gadiyar@ti.com> wrote:
> 
>> From: Madhusudhan Chikkature <madhu.cr@ti.com>
>> 
>> 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.000000000 +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, %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);
>> +               return ret;
>> +       }
>> +
>> +       return ret;
>> +}
>>
>> ...
>>
>> +/* Issue break pulse to the device */
>> +static int omap_hdq_break(struct hdq_data *hdq_data)
>> +{
>> +       int ret;
>> +       u8 tmp_status;
>> +       unsigned long irqflags;
>> +
>> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
>> +       if (ret < 0)
>> +               return -EINTR;
>> +
>> +       if (!hdq_data->hdq_usecount) {
>> +               mutex_unlock(&hdq_data->hdq_mutex);
>> +               return -EINVAL;
>> +       }
>> +
>> +       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);
>> +
>> +       /* set the INIT and GO bit */
>> +       hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> +               OMAP_HDQ_CTRL_STATUS_INITIALIZATION | OMAP_HDQ_CTRL_STATUS_GO,
>> +               OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
>> +               OMAP_HDQ_CTRL_STATUS_GO);
>> +
>> +       /* wait for the TIMEOUT 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");
>> +               mutex_unlock(&hdq_data->hdq_mutex);
>> +               return -EINTR;
> 
> Multiple-return-statements-per-function are a common source of
> maintenance problems: locking errors, resource leaks.  This is why
> kernel code usually does the `goto out' way of avoiding this.
> 
> So.. please consider.  In this case
> 
> ret = -EINTR;
> goto out;
> 
> would fit nicely.
> 
>> +       }
>> +
>> +       spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> +       tmp_status = hdq_data->hdq_irqstatus;
>> +       spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> +       /* check irqstatus */
>> +       if (!(tmp_status & OMAP_HDQ_INT_STATUS_TIMEOUT)) {
>> +               dev_dbg(hdq_data->dev, "timeout waiting for TIMEOUT, %x",
>> +                               tmp_status);
>> +               mutex_unlock(&hdq_data->hdq_mutex);
>> +               return -ETIMEDOUT;
> 
> Here too.
> 
>> +       }
>> +       /*
>> +        * wait for both INIT and GO bits rerurn to zero.
>> +        * zero wait time expected for interrupt mode.
>> +        */
>> +       ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> +                       OMAP_HDQ_CTRL_STATUS_INITIALIZATION |
>> +                       OMAP_HDQ_CTRL_STATUS_GO, OMAP_HDQ_FLAG_CLEAR,
>> +                       &tmp_status);
>> +       if (ret)
>> +               dev_dbg(hdq_data->dev, "timeout waiting INIT&GO bits"
>> +                       "return to zero, %x", tmp_status);
>> +
>> +       mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> +       return ret;
>> +}
>> +
>> +static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
>> +{
>> +       int ret;
>> +       u8 status;
>> +       unsigned long irqflags;
>> +       unsigned long timeout = jiffies + OMAP_HDQ_TIMEOUT;
>> +
>> +       ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
>> +       if (ret < 0)
>> +               return -EINTR;
>> +
>> +       if (!hdq_data->hdq_usecount) {
>> +               mutex_unlock(&hdq_data->hdq_mutex);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!(hdq_data->hdq_irqstatus & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
>> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS,
>> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO,
>> +                       OMAP_HDQ_CTRL_STATUS_DIR | OMAP_HDQ_CTRL_STATUS_GO);
>> +               /*
>> +                * The RX comes immediately after TX. It
>> +                * triggers another interrupt before we
>> +                * sleep. So we have to wait for RXCOMPLETE bit.
>> +                */
>> +               while (!(hdq_data->hdq_irqstatus
>> +                       & OMAP_HDQ_INT_STATUS_RXCOMPLETE)
>> +                       && time_before(jiffies, timeout)) {
>> +                       set_current_state(TASK_UNINTERRUPTIBLE);
>> +                       schedule_timeout(1);
> 
> schedule_timeout_uninterruptible(1)
> 
> If we were to implement the presently-missing
> wait_event_uninterruptible_timeout() (like
> wait_event_interruptible_timeout), could we use it here?
> 
>> +               }
>> +               hdq_reg_merge(hdq_data, OMAP_HDQ_CTRL_STATUS, 0,
>> +                       OMAP_HDQ_CTRL_STATUS_DIR);
>> +               spin_lock_irqsave(&hdq_data->hdq_spinlock, irqflags);
>> +               status = hdq_data->hdq_irqstatus;
>> +               spin_unlock_irqrestore(&hdq_data->hdq_spinlock, irqflags);
>> +               /* check irqstatus */
>> +               if (!(status & OMAP_HDQ_INT_STATUS_RXCOMPLETE)) {
>> +                       dev_dbg(hdq_data->dev, "timeout waiting for"
>> +                               "RXCOMPLETE, %x", status);
>> +                       mutex_unlock(&hdq_data->hdq_mutex);
>> +                       return -ETIMEDOUT;
>> +               }
>> +       }
>> +       /* the data is ready. Read it in! */
>> +       *val = hdq_reg_in(hdq_data, OMAP_HDQ_RX_DATA);
>> +       mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> +       return 0;
>> +
>> +}
>> +
>>
>> ...
>>
>> +static int __init omap_hdq_probe(struct platform_device *pdev)
>> +{
>> +       struct hdq_data *hdq_data;
>> +       struct resource *res;
>> +       int ret, irq;
>> +       u8 rev;
>> +
>> +       if (!pdev)
>> +               return -ENODEV;
> 
> Can this happen?
> 
>> +       hdq_data = kmalloc(sizeof(*hdq_data), GFP_KERNEL);
>> +       if (!hdq_data) {
>> +               dev_dbg(&pdev->dev, "unable to allocate memory\n");
>> +               ret = -ENODEV;
> 
> -ENOMEM
> 
>> +               goto err_kmalloc;
>> +       }
>> +
>> +       hdq_data->dev = &pdev->dev;
>> +       platform_set_drvdata(pdev, hdq_data);
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res) {
>> +               dev_dbg(&pdev->dev, "unable to get resource\n");
>> +               ret = ENXIO;
> 
> bzzt, that should have been -ENXIO.
> 
>> +               goto err_resource;
>> +       }
>> +
>> +       hdq_data->hdq_base = ioremap(res->start, SZ_4K);
>> +       if (!hdq_data->hdq_base) {
>> +               dev_dbg(&pdev->dev, "ioremap failed\n");
>> +               ret = -EINVAL;
>> +               goto err_ioremap;
>> +       }
>> +
>> +       /* get interface & functional clock objects */
>> +       hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
>> +       hdq_data->hdq_fck = clk_get(&pdev->dev, "hdq_fck");
>> +
>> +       if (IS_ERR(hdq_data->hdq_ick) || IS_ERR(hdq_data->hdq_fck)) {
>> +               dev_dbg(&pdev->dev, "Can't get HDQ clock objects\n");
>> +               if (IS_ERR(hdq_data->hdq_ick)) {
>> +                       ret = PTR_ERR(hdq_data->hdq_ick);
>> +                       goto err_clk;
>> +               }
>> +               if (IS_ERR(hdq_data->hdq_fck)) {
>> +                       ret = PTR_ERR(hdq_data->hdq_fck);
>> +                       clk_put(hdq_data->hdq_ick);
>> +                       goto err_clk;
>> +               }
>> +       }
>> +
>> +       hdq_data->hdq_usecount = 0;
>> +       mutex_init(&hdq_data->hdq_mutex);
>> +
>> +       if (clk_enable(hdq_data->hdq_ick)) {
>> +               dev_dbg(&pdev->dev, "Can not enable ick\n");
>> +               ret = -ENODEV;
>> +               goto err_ick;
>> +       }
>> +
>> +       if (clk_enable(hdq_data->hdq_fck)) {
>> +               dev_dbg(&pdev->dev, "Can not enable fck\n");
>> +               ret = -ENODEV;
>> +               goto err_fck;
>> +       }
> 
> ooh, I like err_ick and err_fck a lot.  They sound like akpm review
> comments at the end of a long day.
> 
>> +       rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
>> +       dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",
>> +               (rev >> 4) + '0', (rev & 0x0f) + '0', "Interrupt");
>> +
>> +       spin_lock_init(&hdq_data->hdq_spinlock);
>> +       omap_hdq_break(hdq_data);
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq < 0) {
>> +               ret = -ENXIO;
>> +               goto err_irq;
>> +       }
>> +
>> +       ret = request_irq(irq, hdq_isr, IRQF_DISABLED, "omap_hdq", hdq_data);
>> +       if (ret < 0) {
>> +               dev_dbg(&pdev->dev, "could not request irq\n");
>> +               goto err_irq;
>> +       }
>> +
>> +       /* don't clock the HDQ until it is needed */
>> +       clk_disable(hdq_data->hdq_ick);
>> +       clk_disable(hdq_data->hdq_fck);
>> +
>> +       omap_w1_master.data = hdq_data;
>> +
>> +       ret = w1_add_master_device(&omap_w1_master);
>> +       if (ret) {
>> +               dev_dbg(&pdev->dev, "Failure in registering w1 master\n");
>> +               goto err_w1;
>> +       }
>> +
>> +       return 0;
>> +
>> +err_w1:
>> +err_irq:
>> +       clk_disable(hdq_data->hdq_fck);
>> +
>> +err_fck:
>> +       clk_disable(hdq_data->hdq_ick);
>> +
>> +err_ick:
>> +       clk_put(hdq_data->hdq_ick);
>> +       clk_put(hdq_data->hdq_fck);
>> +
>> +err_clk:
>> +       iounmap(hdq_data->hdq_base);
>> +
>> +err_ioremap:
>> +err_resource:
>> +       platform_set_drvdata(pdev, NULL);
>> +       kfree(hdq_data);
>> +
>> +err_kmalloc:
>> +       return ret;
>> +
>> +}
>> +
>> +static int omap_hdq_remove(struct platform_device *pdev)
>> +{
>> +       struct hdq_data *hdq_data = platform_get_drvdata(pdev);
>> +
>> +       mutex_lock(&hdq_data->hdq_mutex);
>> +
>> +       if (0 != hdq_data->hdq_usecount) {
> 
> Well.  Lots of people dislike that trick.  It's used to catch
> accidental use of = where == was intended, but the compiler will warn
> anyway.  There's less point in using it for !=.
> 
>> +               dev_dbg(&pdev->dev, "removed when use count is not zero\n");
>> +               return -EBUSY;
>> +       }
>> +
>> +       mutex_unlock(&hdq_data->hdq_mutex);
>> +
>> +       /* remove module dependency */
>> +       clk_put(hdq_data->hdq_ick);
>> +       clk_put(hdq_data->hdq_fck);
>> +       free_irq(INT_24XX_HDQ_IRQ, hdq_data);
>> +       platform_set_drvdata(pdev, NULL);
>> +       iounmap(hdq_data->hdq_base);
>> +       kfree(hdq_data);
>> +
>> +       return 0;
>> +}
>> +
>> +static int __init
>> +omap_hdq_init(void)
>> +{
>> +       return platform_driver_register(&omap_hdq_driver);
>> +}
>> +module_init(omap_hdq_init);
>> +
>> +static void __exit
>> +omap_hdq_exit(void)
>> +{
>> +       platform_driver_unregister(&omap_hdq_driver);
>> +}
>> +module_exit(omap_hdq_exit);
>> +
>> +module_param(w1_id, int, S_IRUSR);
>> +MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection");
>> +
>> +MODULE_AUTHOR("Texas Instruments");
>> +MODULE_DESCRIPTION("HDQ driver Library");
>> +MODULE_LICENSE("GPL");
> 
> The code looks pretty good.
> 
> 
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-13 13:25   ` Madhusudhan Chikkature
@ 2008-10-13 13:42     ` Evgeniy Polyakov
  2008-10-13 15:53     ` Andrew Morton
  1 sibling, 0 replies; 12+ messages in thread
From: Evgeniy Polyakov @ 2008-10-13 13:42 UTC (permalink / raw)
  To: Madhusudhan Chikkature
  Cc: Andrew Morton, Gadiyar, Anand, linux-kernel, linux-omap

Hi.

On Mon, Oct 13, 2008 at 06:55:43PM +0530, Madhusudhan Chikkature (madhu.cr@ti.com) 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-13 13:25   ` Madhusudhan Chikkature
  2008-10-13 13:42     ` Evgeniy Polyakov
@ 2008-10-13 15:53     ` Andrew Morton
  2008-10-14  8:51       ` Madhusudhan Chikkature
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2008-10-13 15:53 UTC (permalink / raw)
  To: Madhusudhan Chikkature; +Cc: gadiyar, johnpol, linux-kernel, linux-omap

> On Mon, 13 Oct 2008 18:55:43 +0530 "Madhusudhan Chikkature" <madhu.cr@ti.com> wrote:
> 
> ----- Original Message ----- 
> From: "Andrew Morton" <akpm@linux-foundation.org>
> To: "Gadiyar, Anand" <gadiyar@ti.com>
> Cc: <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; <madhu.cr@ti.com>
> 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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-13 15:53     ` Andrew Morton
@ 2008-10-14  8:51       ` Madhusudhan Chikkature
  2008-10-14 12:50         ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: Madhusudhan Chikkature @ 2008-10-14  8:51 UTC (permalink / raw)
  To: Andrew Morton; +Cc: gadiyar, johnpol, linux-kernel, linux-omap


----- Original Message ----- 
From: "Andrew Morton" <akpm@linux-foundation.org>
To: "Madhusudhan Chikkature" <madhu.cr@ti.com>
Cc: <gadiyar@ti.com>; <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <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" <madhu.cr@ti.com> wrote:
>> 
>> ----- Original Message ----- 
>> From: "Andrew Morton" <akpm@linux-foundation.org>
>> To: "Gadiyar, Anand" <gadiyar@ti.com>
>> Cc: <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; <madhu.cr@ti.com>
>> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-14  8:51       ` Madhusudhan Chikkature
@ 2008-10-14 12:50         ` Andrew Morton
  2008-10-14 13:42           ` Evgeniy Polyakov
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2008-10-14 12:50 UTC (permalink / raw)
  To: Madhusudhan Chikkature; +Cc: gadiyar, johnpol, linux-kernel, linux-omap

> On Tue, 14 Oct 2008 14:21:50 +0530 "Madhusudhan Chikkature" <madhu.cr@ti.com> wrote:
> 
> ----- Original Message ----- 
> From: "Andrew Morton" <akpm@linux-foundation.org>
> To: "Madhusudhan Chikkature" <madhu.cr@ti.com>
> Cc: <gadiyar@ti.com>; <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <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" <madhu.cr@ti.com> wrote:
> >> 
> >> ----- Original Message ----- 
> >> From: "Andrew Morton" <akpm@linux-foundation.org>
> >> To: "Gadiyar, Anand" <gadiyar@ti.com>
> >> Cc: <johnpol@2ka.mipt.ru>; <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; <madhu.cr@ti.com>
> >> 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-14 12:50         ` Andrew Morton
@ 2008-10-14 13:42           ` Evgeniy Polyakov
  2008-10-14 14:30             ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: Evgeniy Polyakov @ 2008-10-14 13:42 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Madhusudhan Chikkature, gadiyar, linux-kernel, linux-omap

Hi.

On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton (akpm@linux-foundation.org) 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-14 13:42           ` Evgeniy Polyakov
@ 2008-10-14 14:30             ` Andrew Morton
  2008-10-14 14:55               ` Evgeniy Polyakov
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2008-10-14 14:30 UTC (permalink / raw)
  To: Evgeniy Polyakov; +Cc: madhu.cr, gadiyar, linux-kernel, linux-omap

> On Tue, 14 Oct 2008 17:42:49 +0400 Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> Hi.
> 
> On Tue, Oct 14, 2008 at 05:50:02AM -0700, Andrew Morton (akpm@linux-foundation.org) 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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-14 14:30             ` Andrew Morton
@ 2008-10-14 14:55               ` Evgeniy Polyakov
  2008-10-16  7:05                 ` Madhusudhan Chikkature
  0 siblings, 1 reply; 12+ messages in thread
From: Evgeniy Polyakov @ 2008-10-14 14:55 UTC (permalink / raw)
  To: Andrew Morton; +Cc: madhu.cr, gadiyar, linux-kernel, linux-omap

On Tue, Oct 14, 2008 at 07:30:58AM -0700, Andrew Morton (akpm@linux-foundation.org) 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] HDQ Driver for OMAP2430/3430
  2008-10-14 14:55               ` Evgeniy Polyakov
@ 2008-10-16  7:05                 ` Madhusudhan Chikkature
  0 siblings, 0 replies; 12+ messages in thread
From: Madhusudhan Chikkature @ 2008-10-16  7:05 UTC (permalink / raw)
  To: Evgeniy Polyakov; +Cc: Andrew Morton, gadiyar, linux-kernel, linux-omap

> On Tue, Oct 14, 2008 at 07:30:58AM -0700, Andrew Morton
> (akpm@linux-foundation.org) 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2008-10-16  7:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-08  7:16 [PATCH 1/5] HDQ Driver for OMAP2430/3430 Gadiyar, Anand
2008-10-08 19:59 ` Evgeniy Polyakov
2008-10-10 20:38 ` Andrew Morton
2008-10-13 13:25   ` Madhusudhan Chikkature
2008-10-13 13:42     ` Evgeniy Polyakov
2008-10-13 15:53     ` Andrew Morton
2008-10-14  8:51       ` Madhusudhan Chikkature
2008-10-14 12:50         ` Andrew Morton
2008-10-14 13:42           ` Evgeniy Polyakov
2008-10-14 14:30             ` Andrew Morton
2008-10-14 14:55               ` Evgeniy Polyakov
2008-10-16  7:05                 ` Madhusudhan Chikkature

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).