| Message ID | 20160525093702.64d7309c@endymion |
|---|---|
| State | Accepted |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); > > }
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); > } > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
--- 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); }
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(-)