[PATCH v1 1/5] i2c-designware-baytrail: describe magic numbers

[PATCH v1 1/5] i2c-designware-baytrail: describe magic numbers

[Andy Shevchenko]

The patch converts hardcoded numerical constants to a named ones.

While here, align the variable name in get_sem() and reset_semaphore().

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 5f1ff4c..e9cb355 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -22,22 +22,24 @@
 
 #define SEMAPHORE_TIMEOUT	100
 #define PUNIT_SEMAPHORE		0x7
+#define PUNIT_SEMAPHORE_BIT	BIT(0)
+#define PUNIT_SEMAPHORE_ACQUIRE	BIT(1)
 
 static unsigned long acquired;
 
 static int get_sem(struct device *dev, u32 *sem)
 {
-	u32 reg_val;
+	u32 data;
 	int ret;
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ, PUNIT_SEMAPHORE,
-			    &reg_val);
+				&data);
 	if (ret) {
 		dev_err(dev, "iosf failed to read punit semaphore\n");
 		return ret;
 	}
 
-	*sem = reg_val & 0x1;
+	*sem = data & PUNIT_SEMAPHORE_BIT;
 
 	return 0;
 }
@@ -52,9 +54,9 @@ static void reset_semaphore(struct device *dev)
 		return;
 	}
 
-	data = data & 0xfffffffe;
+	data &= ~PUNIT_SEMAPHORE_BIT;
 	if (iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
-				 PUNIT_SEMAPHORE, data))
+				PUNIT_SEMAPHORE, data))
 		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
 }
 
@@ -70,9 +72,9 @@ int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return 0;
 
-	/* host driver writes 0x2 to side band semaphore register */
+	/* host driver writes to side band semaphore register */
 	ret = iosf_mbi_write(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_WRITE,
-				 PUNIT_SEMAPHORE, 0x2);
+				PUNIT_SEMAPHORE, PUNIT_SEMAPHORE_ACQUIRE);
 	if (ret) {
 		dev_err(dev->dev, "iosf punit semaphore request failed\n");
 		return ret;
-- 
2.1.4

[PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Andy Shevchenko]

It seems we have same message for different return values in get_sem() and
baytrail_i2c_acquire(). I suspect this is just a typo, so this patch fixes it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index e9cb355..9b67655 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -99,8 +99,8 @@ int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	reset_semaphore(dev->dev);
 
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
-		PUNIT_SEMAPHORE, &sem);
-	if (!ret)
+				PUNIT_SEMAPHORE, &sem);
+	if (ret)
 		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
 	else
 		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
-- 
2.1.4

[PATCH v1 3/5] i2c-designware-baytrail: fix sparse warnings

[Andy Shevchenko]

There is no need to export functions that are used as the callbacks in the
struct dw_i2c_dev. Otherwise we get the following warnings:

drivers/i2c/busses/i2c-designware-baytrail.c:63:5: warning: symbol 'baytrail_i2c_acquire' was not declared. Should it be static?
drivers/i2c/busses/i2c-designware-baytrail.c:114:6: warning: symbol 'baytrail_i2c_release' was not declared. Should it be static?

While here, do few indentation fixes, remove i2c_dw_eval_lock_support() from
functions exported to the modules and redundant assignment of local sem
variable.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 9b67655..d334744 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -17,7 +17,9 @@
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+
 #include <asm/iosf_mbi.h>
+
 #include "i2c-designware-core.h"
 
 #define SEMAPHORE_TIMEOUT	100
@@ -60,9 +62,9 @@ static void reset_semaphore(struct device *dev)
 		dev_err(dev, "iosf failed to reset punit semaphore during write\n");
 }
 
-int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
+static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 {
-	u32 sem = 0;
+	u32 sem;
 	int ret;
 	unsigned long start, end;
 
@@ -109,9 +111,8 @@ int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 
 	return -ETIMEDOUT;
 }
-EXPORT_SYMBOL(baytrail_i2c_acquire);
 
-void baytrail_i2c_release(struct dw_i2c_dev *dev)
+static void baytrail_i2c_release(struct dw_i2c_dev *dev)
 {
 	if (!dev || !dev->dev)
 		return;
@@ -123,7 +124,6 @@ void baytrail_i2c_release(struct dw_i2c_dev *dev)
 	dev_dbg(dev->dev, "punit semaphore held for %ums\n",
 		jiffies_to_msecs(jiffies - acquired));
 }
-EXPORT_SYMBOL(baytrail_i2c_release);
 
 int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 {
@@ -139,7 +139,6 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 		return 0;
 
 	status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
-
 	if (ACPI_FAILURE(status))
 		return 0;
 
@@ -155,7 +154,6 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
 
 	return 0;
 }
-EXPORT_SYMBOL(i2c_dw_eval_lock_support);
 
 MODULE_AUTHOR("David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>");
 MODULE_DESCRIPTION("Baytrail I2C Semaphore driver");
-- 
2.1.4

[PATCH v1 4/5] i2c-designware-baytrail: cross-check lock functions

[Andy Shevchenko]

It seems the idea behind the cross-check is to prevent acquire semaphore when
there is no release callback and vice versa. Thus, patch fixes a typo.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index d334744..036d9bdc0 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -71,7 +71,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev || !dev->dev)
 		return -ENODEV;
 
-	if (!dev->acquire_lock)
+	if (!dev->release_lock)
 		return 0;
 
 	/* host driver writes to side band semaphore register */
-- 
2.1.4

[PATCH v1 5/5] i2c-designware-baytrail: baytrail_i2c_acquire() might sleep

[Andy Shevchenko]

This patch marks baytrail_i2c_acquire() that it might sleep. Also it chages
while-loop to do-while and, though it is matter of taste, gives a chance to
check one more time before report a timeout.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 036d9bdc0..7d7ae97 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -68,6 +68,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	int ret;
 	unsigned long start, end;
 
+	might_sleep();
+
 	if (!dev || !dev->dev)
 		return -ENODEV;
 
@@ -85,7 +87,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	/* host driver waits for bit 0 to be set in semaphore register */
 	start = jiffies;
 	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
-	while (!time_after(jiffies, end)) {
+	do {
 		ret = get_sem(dev->dev, &sem);
 		if (!ret && sem) {
 			acquired = jiffies;
@@ -95,7 +97,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 		}
 
 		usleep_range(1000, 2000);
-	}
+	} while (time_before(jiffies, end));
 
 	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
 	reset_semaphore(dev->dev);
-- 
2.1.4

Re: [PATCH v1 1/5] i2c-designware-baytrail: describe magic numbers

[David E. Box]

On Tue, Feb 10, 2015 at 07:06:06PM +0200, Andy Shevchenko wrote:
> The patch converts hardcoded numerical constants to a named ones.
> 
> While here, align the variable name in get_sem() and reset_semaphore().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[David E. Box]

On Tue, Feb 10, 2015 at 07:06:07PM +0200, Andy Shevchenko wrote:
> It seems we have same message for different return values in get_sem() and
> baytrail_i2c_acquire(). I suspect this is just a typo, so this patch fixes it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Thanks for spotting this. Timeouts are infrequent and I didn't notice the
error here.

Dave

> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index e9cb355..9b67655 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -99,8 +99,8 @@ int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  	reset_semaphore(dev->dev);
>  
>  	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, BT_MBI_BUNIT_READ,
> -		PUNIT_SEMAPHORE, &sem);
> -	if (!ret)
> +				PUNIT_SEMAPHORE, &sem);
> +	if (ret)
>  		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
>  	else
>  		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> -- 
> 2.1.4
> 

Re: [PATCH v1 3/5] i2c-designware-baytrail: fix sparse warnings

[David E. Box]

On Tue, Feb 10, 2015 at 07:06:08PM +0200, Andy Shevchenko wrote:
> There is no need to export functions that are used as the callbacks in the
> struct dw_i2c_dev. Otherwise we get the following warnings:
> 
> drivers/i2c/busses/i2c-designware-baytrail.c:63:5: warning: symbol 'baytrail_i2c_acquire' was not declared. Should it be static?
> drivers/i2c/busses/i2c-designware-baytrail.c:114:6: warning: symbol 'baytrail_i2c_release' was not declared. Should it be static?
> 
> While here, do few indentation fixes, remove i2c_dw_eval_lock_support() from
> functions exported to the modules and redundant assignment of local sem
> variable.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Yep. These remained from original versions of the patch that did use them as
callbacks in struct dw_i2c_dev. Thanks.

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]


> +				PUNIT_SEMAPHORE, &sem);
> +	if (ret)
>  		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
>  	else
>  		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);

Shouldn't the latter be a dev_dbg?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 4/5] i2c-designware-baytrail: cross-check lock functions

[David E. Box]

On Tue, Feb 10, 2015 at 07:06:09PM +0200, Andy Shevchenko wrote:
> It seems the idea behind the cross-check is to prevent acquire semaphore when
> there is no release callback and vice versa. Thus, patch fixes a typo.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index d334744..036d9bdc0 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -71,7 +71,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  	if (!dev || !dev->dev)
>  		return -ENODEV;
>  
> -	if (!dev->acquire_lock)
> +	if (!dev->release_lock)
>  		return 0;
>  
>  	/* host driver writes to side band semaphore register */
> -- 
> 2.1.4
> 

Re: [PATCH v1 5/5] i2c-designware-baytrail: baytrail_i2c_acquire() might sleep

[David E. Box]

On Tue, Feb 10, 2015 at 07:06:10PM +0200, Andy Shevchenko wrote:
> This patch marks baytrail_i2c_acquire() that it might sleep. Also it chages
> while-loop to do-while and, though it is matter of taste, gives a chance to
> check one more time before report a timeout.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Acked-by: David E. Box <david.e.box-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 036d9bdc0..7d7ae97 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -68,6 +68,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  	int ret;
>  	unsigned long start, end;
>  
> +	might_sleep();
> +
>  	if (!dev || !dev->dev)
>  		return -ENODEV;
>  
> @@ -85,7 +87,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  	/* host driver waits for bit 0 to be set in semaphore register */
>  	start = jiffies;
>  	end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
> -	while (!time_after(jiffies, end)) {
> +	do {
>  		ret = get_sem(dev->dev, &sem);
>  		if (!ret && sem) {
>  			acquired = jiffies;
> @@ -95,7 +97,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
>  		}
>  
>  		usleep_range(1000, 2000);
> -	}
> +	} while (time_before(jiffies, end));
>  
>  	dev_err(dev->dev, "punit semaphore timed out, resetting\n");
>  	reset_semaphore(dev->dev);
> -- 
> 2.1.4
> 

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Andy Shevchenko]

On Wed, 2015-02-11 at 17:46 +0100, Wolfram Sang wrote:
> > +				PUNIT_SEMAPHORE, &sem);
> > +	if (ret)
> >  		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
> >  	else
> >  		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> 
> Shouldn't the latter be a dev_dbg?

For me it seems not. Here is error patch and we have already in error
recovery, so, intention to see if the semaphore becomes alive after
reset. Am I right, David?


-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[David E. Box]

On Wed, Feb 11, 2015 at 06:59:51PM +0200, Andy Shevchenko wrote:
> On Wed, 2015-02-11 at 17:46 +0100, Wolfram Sang wrote:
> > > +				PUNIT_SEMAPHORE, &sem);
> > > +	if (ret)
> > >  		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
> > >  	else
> > >  		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> > 
> > Shouldn't the latter be a dev_dbg?
> 
> For me it seems not. Here is error patch and we have already in error
> recovery, so, intention to see if the semaphore becomes alive after
> reset. Am I right, David?

Yes. We've timed out by this section of code so we want to verify the
semaphore was reset. Failure to read the semaphore though is a separate
error as well.

Dave

> 
> 
> -- 
> Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Intel Finland Oy
> 

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Andy Shevchenko]

On Wed, 2015-02-11 at 17:46 +0100, Wolfram Sang wrote:
> > +				PUNIT_SEMAPHORE, &sem);
> > +	if (ret)
> >  		dev_err(dev->dev, "iosf failed to read punit semaphore\n");
> >  	else
> >  		dev_err(dev->dev, "PUNIT SEM: %d\n", sem);
> 
> Shouldn't the latter be a dev_dbg?

Wolfram, do we agree on this as states in the patch? Maybe you have more
comments, questions?

Otherwise, should I rebase this on top of 4.0-rc1 or you will be okay
with current version?

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]


> > Shouldn't the latter be a dev_dbg?
> 
> Wolfram, do we agree on this as states in the patch? Maybe you have more
> comments, questions?

Yes, it is fine this way.

> Otherwise, should I rebase this on top of 4.0-rc1 or you will be okay
> with current version?

Well, if it needs rebasing, please do. If not, please say so, too :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1 2/5] i2c-designware-baytrail: fix typo in error path

[Andy Shevchenko]

On Mon, 2015-02-23 at 19:42 +0100, Wolfram Sang wrote:
> > > Shouldn't the latter be a dev_dbg?
> > 
> > Wolfram, do we agree on this as states in the patch? Maybe you have more
> > comments, questions?
> 
> Yes, it is fine this way.
> 
> > Otherwise, should I rebase this on top of 4.0-rc1 or you will be okay
> > with current version?
> 
> Well, if it needs rebasing, please do. If not, please say so, too :)

It applies clearly on top of current linux-next without any changes from
my side. So, no rebase is needed.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

Re: [PATCH v1 1/5] i2c-designware-baytrail: describe magic numbers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Tue, Feb 10, 2015 at 07:06:06PM +0200, Andy Shevchenko wrote:
> The patch converts hardcoded numerical constants to a named ones.
> 
> While here, align the variable name in get_sem() and reset_semaphore().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Applied the whole series to for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]