diff mbox

[1/5] drivers: w1: omap_hdq: cleanup and bug fixes.

Message ID 1397651532-31456-2-git-send-email-sourav.poddar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Poddar, Sourav April 16, 2014, 12:32 p.m. UTC
The patch adds the following to the omap hdq driver.
1. HDQ Device reset call in probe.
2. Enabling '1 wire mode' and checking for presence pulse bit.
3. Proper disabling and enabling of interrupts during read path.
4. Add re-initialization code during SKIP ROM command execution.
5. Miscellaneous cleanup(formatting, return error checks).

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/w1/masters/omap_hdq.c |   85 ++++++++++++++++++++++++++++++++---------
 1 file changed, 68 insertions(+), 17 deletions(-)

Comments

Paul Walmsley April 28, 2014, 7:19 p.m. UTC | #1
On Wed, 16 Apr 2014, Sourav Poddar wrote:

> The patch adds the following to the omap hdq driver.
> 1. HDQ Device reset call in probe.
> 2. Enabling '1 wire mode' and checking for presence pulse bit.
> 3. Proper disabling and enabling of interrupts during read path.
> 4. Add re-initialization code during SKIP ROM command execution.
> 5. Miscellaneous cleanup(formatting, return error checks).
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>  drivers/w1/masters/omap_hdq.c |   85 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
> index 9900e8e..0a7bf7f 100644
> --- a/drivers/w1/masters/omap_hdq.c
> +++ b/drivers/w1/masters/omap_hdq.c

...

> @@ -115,6 +116,15 @@ static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
>  	return new_val;
>  }
>  
> +static void hdq_disable_interrupt(struct hdq_data *hdq_data, u32 offset,
> +				  u8 mask)
> +{
> +	u32 ie;
> +
> +	ie = readl(hdq_data->hdq_base + offset);
> +	writel(ie & mask, hdq_data->hdq_base + offset);
> +}
> +

Does this function really need to take offset and mask arguments?  Won't 
they always be constant, and therefore, no need to pass them?


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Poddar, Sourav April 29, 2014, 9:15 a.m. UTC | #2
Hi Paul,

On Tuesday 29 April 2014 12:49 AM, Paul Walmsley wrote:
> On Wed, 16 Apr 2014, Sourav Poddar wrote:
>
>> The patch adds the following to the omap hdq driver.
>> 1. HDQ Device reset call in probe.
>> 2. Enabling '1 wire mode' and checking for presence pulse bit.
>> 3. Proper disabling and enabling of interrupts during read path.
>> 4. Add re-initialization code during SKIP ROM command execution.
>> 5. Miscellaneous cleanup(formatting, return error checks).
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> ---
>>   drivers/w1/masters/omap_hdq.c |   85 ++++++++++++++++++++++++++++++++---------
>>   1 file changed, 68 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
>> index 9900e8e..0a7bf7f 100644
>> --- a/drivers/w1/masters/omap_hdq.c
>> +++ b/drivers/w1/masters/omap_hdq.c
> ...
>
>> @@ -115,6 +116,15 @@ static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
>>   	return new_val;
>>   }
>>
>> +static void hdq_disable_interrupt(struct hdq_data *hdq_data, u32 offset,
>> +				  u8 mask)
>> +{
>> +	u32 ie;
>> +
>> +	ie = readl(hdq_data->hdq_base + offset);
>> +	writel(ie&  mask, hdq_data->hdq_base + offset);
>> +}
>> +
> Does this function really need to take offset and mask arguments?  Won't
> they always be constant, and therefore, no need to pass them?

Yes, thats correct, they are always constant and can be used directly 
inside the
api. I will fix this in my next version.

>
> - Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c
index 9900e8e..0a7bf7f 100644
--- a/drivers/w1/masters/omap_hdq.c
+++ b/drivers/w1/masters/omap_hdq.c
@@ -27,21 +27,22 @@ 
 #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_CTRL_STATUS_INTERRUPTMASK	(1 << 6)
+#define OMAP_HDQ_CTRL_STATUS_CLOCKENABLE	(1 << 5)
+#define OMAP_HDQ_CTRL_STATUS_PRESENCE		(1 << 3)
+#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_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_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_SYSSTATUS_RESETDONE		(1 << 0)
 
 #define OMAP_HDQ_FLAG_CLEAR			0
 #define OMAP_HDQ_FLAG_SET			1
@@ -115,6 +116,15 @@  static inline u8 hdq_reg_merge(struct hdq_data *hdq_data, u32 offset,
 	return new_val;
 }
 
+static void hdq_disable_interrupt(struct hdq_data *hdq_data, u32 offset,
+				  u8 mask)
+{
+	u32 ie;
+
+	ie = readl(hdq_data->hdq_base + offset);
+	writel(ie & mask, hdq_data->hdq_base + offset);
+}
+
 /*
  * Wait for one or more bits in flag change.
  * HDQ_FLAG_SET: wait until any bit in the flag is set.
@@ -263,8 +273,7 @@  static int _omap_hdq_reset(struct hdq_data *hdq_data)
 	 * interrupt.
 	 */
 	hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
-		OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-		OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+		OMAP_HDQ_CTRL_STATUS_CLOCKENABLE);
 
 	/* wait for reset to complete */
 	ret = hdq_wait_for_flag(hdq_data, OMAP_HDQ_SYSSTATUS,
@@ -275,7 +284,8 @@  static int _omap_hdq_reset(struct hdq_data *hdq_data)
 	else {
 		hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
 			OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-			OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+			OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
+			OMAP_HDQ_CTRL_STATUS_MODE);
 		hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
 			OMAP_HDQ_SYSCONFIG_AUTOIDLE);
 	}
@@ -327,6 +337,18 @@  static int omap_hdq_break(struct hdq_data *hdq_data)
 		ret = -ETIMEDOUT;
 		goto out;
 	}
+
+	/*
+	 * check for the presence detect bit to get
+	 * set to show that the slave is responding
+	 */
+	if (hdq_reg_in(hdq_data, OMAP_HDQ_CTRL_STATUS) &
+			OMAP_HDQ_CTRL_STATUS_PRESENCE) {
+		dev_dbg(hdq_data->dev, "Presence bit not set\n");
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
 	/*
 	 * wait for both INIT and GO bits rerurn to zero.
 	 * zero wait time expected for interrupt mode.
@@ -361,6 +383,8 @@  static int hdq_read_byte(struct hdq_data *hdq_data, u8 *val)
 		goto out;
 	}
 
+	hdq_data->hdq_irqstatus = 0;
+
 	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,
@@ -426,7 +450,8 @@  static int omap_hdq_get(struct hdq_data *hdq_data)
 				/* select HDQ mode & enable clocks */
 				hdq_reg_out(hdq_data, OMAP_HDQ_CTRL_STATUS,
 					OMAP_HDQ_CTRL_STATUS_CLOCKENABLE |
-					OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+					OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK |
+					OMAP_HDQ_CTRL_STATUS_MODE);
 				hdq_reg_out(hdq_data, OMAP_HDQ_SYSCONFIG,
 					OMAP_HDQ_SYSCONFIG_AUTOIDLE);
 				hdq_reg_in(hdq_data, OMAP_HDQ_INT_STATUS);
@@ -471,6 +496,10 @@  static u8 omap_w1_read_byte(void *_hdq)
 	u8 val = 0;
 	int ret;
 
+	/* First write to initialize the transfer */
+	if (hdq_data->init_trans == 0)
+		omap_hdq_get(hdq_data);
+
 	ret = hdq_read_byte(hdq_data, &val);
 	if (ret) {
 		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
@@ -484,6 +513,10 @@  static u8 omap_w1_read_byte(void *_hdq)
 		return -1;
 	}
 
+	hdq_disable_interrupt(hdq_data, OMAP_HDQ_CTRL_STATUS,
+			      ~OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK);
+	hdq_data->hdq_usecount = 0;
+
 	/* Write followed by a read, release the module */
 	if (hdq_data->init_trans) {
 		ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
@@ -510,6 +543,14 @@  static void omap_w1_write_byte(void *_hdq, u8 byte)
 	if (hdq_data->init_trans == 0)
 		omap_hdq_get(hdq_data);
 
+	/*
+	 * We need to reset the slave before
+	 * issuing the SKIP ROM command, else
+	 * the slave will not work.
+	 */
+	if (byte == W1_SKIP_ROM)
+		omap_hdq_break(hdq_data);
+
 	ret = mutex_lock_interruptible(&hdq_data->hdq_mutex);
 	if (ret < 0) {
 		dev_dbg(hdq_data->dev, "Could not acquire mutex\n");
@@ -563,7 +604,17 @@  static int omap_hdq_probe(struct platform_device *pdev)
 	mutex_init(&hdq_data->hdq_mutex);
 
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_dbg(&pdev->dev, "pm_runtime_get_sync failed\n");
+		goto err_w1;
+	}
+
+	ret = _omap_hdq_reset(hdq_data);
+	if (ret) {
+		dev_dbg(&pdev->dev, "reset failed\n");
+		return -EINVAL;
+	}
 
 	rev = hdq_reg_in(hdq_data, OMAP_HDQ_REVISION);
 	dev_info(&pdev->dev, "OMAP HDQ Hardware Rev %c.%c. Driver in %s mode\n",