[PATCH] i2c: i801: Drop needless bit-wise OR

[PATCH] i2c: i801: Drop needless bit-wise OR

[Jean Delvare]

The interrupt handling code makes it look like several status values
may be merged together before being processed, while this will never
happen. Change from bit-wise OR to simple assignment to make it more
obvious and avoid misunderstanding.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Daniel Kurtz <djkurtz@chromium.org>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
Daniel, was there any reason for this bit-wise OR, which I may be
missing?

 drivers/i2c/busses/i2c-i801.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-4.5.orig/drivers/i2c/busses/i2c-i801.c	2016-05-24 11:04:33.169026906 +0200
+++ linux-4.5/drivers/i2c/busses/i2c-i801.c	2016-05-24 11:05:40.564642488 +0200
@@ -548,7 +548,7 @@ static irqreturn_t i801_isr(int irq, voi
 	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
 	if (status) {
 		outb_p(status, SMBHSTSTS(priv));
-		priv->status |= status;
+		priv->status = status;
 		wake_up(&priv->waitq);
 	}
 


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Mika Westerberg]

On Wed, May 25, 2016 at 09:37:02AM +0200, Jean Delvare wrote:
> The interrupt handling code makes it look like several status values
> may be merged together before being processed, while this will never
> happen. Change from bit-wise OR to simple assignment to make it more
> obvious and avoid misunderstanding.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Daniel Kurtz]

Hi Jean,

On Wed, May 25, 2016 at 3:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> The interrupt handling code makes it look like several status values
> may be merged together before being processed, while this will never
> happen. Change from bit-wise OR to simple assignment to make it more
> obvious and avoid misunderstanding.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
> Daniel, was there any reason for this bit-wise OR, which I may be
> missing?

The only thing I can think of is that I didn't want to assume that we
would always clear priv->status before another interrupt arrived.

>
>  drivers/i2c/busses/i2c-i801.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-4.5.orig/drivers/i2c/busses/i2c-i801.c        2016-05-24 11:04:33.169026906 +0200
> +++ linux-4.5/drivers/i2c/busses/i2c-i801.c     2016-05-24 11:05:40.564642488 +0200
> @@ -548,7 +548,7 @@ static irqreturn_t i801_isr(int irq, voi
>         status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>         if (status) {
>                 outb_p(status, SMBHSTSTS(priv));
> -               priv->status |= status;
> +               priv->status = status;
>                 wake_up(&priv->waitq);
>         }
>
>
>
> --
> Jean Delvare
> SUSE L3 Support

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Jean Delvare]

Hi Daniel,

On Mon, 30 May 2016 22:07:55 +0800, Daniel Kurtz wrote:
> On Wed, May 25, 2016 at 3:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > The interrupt handling code makes it look like several status values
> > may be merged together before being processed, while this will never
> > happen. Change from bit-wise OR to simple assignment to make it more
> > obvious and avoid misunderstanding.
> >
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Daniel Kurtz <djkurtz@chromium.org>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > ---
> > Daniel, was there any reason for this bit-wise OR, which I may be
> > missing?
> 
> The only thing I can think of is that I didn't want to assume that we
> would always clear priv->status before another interrupt arrived.

Well my question is quite clear: can this actually happen? I can't see
how.

> >  drivers/i2c/busses/i2c-i801.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- linux-4.5.orig/drivers/i2c/busses/i2c-i801.c        2016-05-24 11:04:33.169026906 +0200
> > +++ linux-4.5/drivers/i2c/busses/i2c-i801.c     2016-05-24 11:05:40.564642488 +0200
> > @@ -548,7 +548,7 @@ static irqreturn_t i801_isr(int irq, voi
> >         status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
> >         if (status) {
> >                 outb_p(status, SMBHSTSTS(priv));
> > -               priv->status |= status;
> > +               priv->status = status;
> >                 wake_up(&priv->waitq);
> >         }

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Daniel Kurtz]

On Wed, Jun 1, 2016 at 5:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
>
> Hi Daniel,
>
> On Mon, 30 May 2016 22:07:55 +0800, Daniel Kurtz wrote:
> > On Wed, May 25, 2016 at 3:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > The interrupt handling code makes it look like several status values
> > > may be merged together before being processed, while this will never
> > > happen. Change from bit-wise OR to simple assignment to make it more
> > > obvious and avoid misunderstanding.
> > >
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Daniel Kurtz <djkurtz@chromium.org>
> > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > ---
> > > Daniel, was there any reason for this bit-wise OR, which I may be
> > > missing?
> >
> > The only thing I can think of is that I didn't want to assume that we
> > would always clear priv->status before another interrupt arrived.
>
> Well my question is quite clear: can this actually happen? I can't see
> how.

I have no idea.  You'd have to ask Intel, I guess.

>
>
> > >  drivers/i2c/busses/i2c-i801.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > --- linux-4.5.orig/drivers/i2c/busses/i2c-i801.c        2016-05-24 11:04:33.169026906 +0200
> > > +++ linux-4.5/drivers/i2c/busses/i2c-i801.c     2016-05-24 11:05:40.564642488 +0200
> > > @@ -548,7 +548,7 @@ static irqreturn_t i801_isr(int irq, voi
> > >         status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
> > >         if (status) {
> > >                 outb_p(status, SMBHSTSTS(priv));
> > > -               priv->status |= status;
> > > +               priv->status = status;
> > >                 wake_up(&priv->waitq);
> > >         }
>
> --
> Jean Delvare
> SUSE L3 Support

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Jean Delvare]

On Wed, 1 Jun 2016 17:38:27 +0800, Daniel Kurtz wrote:
> On Wed, Jun 1, 2016 at 5:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> >
> > Hi Daniel,
> >
> > On Mon, 30 May 2016 22:07:55 +0800, Daniel Kurtz wrote:
> > > On Wed, May 25, 2016 at 3:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > > The interrupt handling code makes it look like several status values
> > > > may be merged together before being processed, while this will never
> > > > happen. Change from bit-wise OR to simple assignment to make it more
> > > > obvious and avoid misunderstanding.
> > > >
> > > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > > Cc: Daniel Kurtz <djkurtz@chromium.org>
> > > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > > ---
> > > > Daniel, was there any reason for this bit-wise OR, which I may be
> > > > missing?
> > >
> > > The only thing I can think of is that I didn't want to assume that we
> > > would always clear priv->status before another interrupt arrived.
> >
> > Well my question is quite clear: can this actually happen? I can't see
> > how.
> 
> I have no idea.  You'd have to ask Intel, I guess.

You wrote the code based on public documentation, I thought you would
know. But if you can't be bothered, never mind, I'll trust my
understanding of the code.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Daniel Kurtz]

On Thu, Jun 2, 2016 at 7:45 PM, Jean Delvare <jdelvare@suse.de> wrote:
>
> On Wed, 1 Jun 2016 17:38:27 +0800, Daniel Kurtz wrote:
> > On Wed, Jun 1, 2016 at 5:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Mon, 30 May 2016 22:07:55 +0800, Daniel Kurtz wrote:
> > > > On Wed, May 25, 2016 at 3:37 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > > > The interrupt handling code makes it look like several status values
> > > > > may be merged together before being processed, while this will never
> > > > > happen. Change from bit-wise OR to simple assignment to make it more
> > > > > obvious and avoid misunderstanding.
> > > > >
> > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > > > Cc: Daniel Kurtz <djkurtz@chromium.org>
> > > > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > > > ---
> > > > > Daniel, was there any reason for this bit-wise OR, which I may be
> > > > > missing?
> > > >
> > > > The only thing I can think of is that I didn't want to assume that we
> > > > would always clear priv->status before another interrupt arrived.
> > >
> > > Well my question is quite clear: can this actually happen? I can't see
> > > how.
> >
> > I have no idea.  You'd have to ask Intel, I guess.
>
> You wrote the code based on public documentation, I thought you would
> know. But if you can't be bothered, never mind, I'll trust my
> understanding of the code.

Here is the documentation:
http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-6-datasheet.pdf
Page 570

I thought maybe there were situations where you could get INTR and an
error condition, but I don't see anything like that in the
documentation, so I think you are right and only one bit will be set
at a time.

Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>


> --
> Jean Delvare
> SUSE L3 Support

Re: i2c: i801: Drop needless bit-wise OR

[Benjamin Tissoires]

On May 25 2016 or thereabouts, Jean Delvare wrote:
> The interrupt handling code makes it look like several status values
> may be merged together before being processed, while this will never
> happen. Change from bit-wise OR to simple assignment to make it more
> obvious and avoid misunderstanding.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
> ---

Looks good to me:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> Daniel, was there any reason for this bit-wise OR, which I may be
> missing?
> 
>  drivers/i2c/busses/i2c-i801.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-4.5.orig/drivers/i2c/busses/i2c-i801.c	2016-05-24 11:04:33.169026906 +0200
> +++ linux-4.5/drivers/i2c/busses/i2c-i801.c	2016-05-24 11:05:40.564642488 +0200
> @@ -548,7 +548,7 @@ static irqreturn_t i801_isr(int irq, voi
>  	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
>  	if (status) {
>  		outb_p(status, SMBHSTSTS(priv));
> -		priv->status |= status;
> +		priv->status = status;
>  		wake_up(&priv->waitq);
>  	}
>  

Re: [PATCH] i2c: i801: Drop needless bit-wise OR

[Wolfram Sang]

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

On Wed, May 25, 2016 at 09:37:02AM +0200, Jean Delvare wrote:
> The interrupt handling code makes it look like several status values
> may be merged together before being processed, while this will never
> happen. Change from bit-wise OR to simple assignment to make it more
> obvious and avoid misunderstanding.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>

Applied to for-next, thanks!


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