* [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers @ 2018-07-11 8:32 Jorge Ramirez-Ortiz 2018-07-11 12:29 ` Lorenzo Bianconi 0 siblings, 1 reply; 10+ messages in thread From: Jorge Ramirez-Ortiz @ 2018-07-11 8:32 UTC (permalink / raw) To: jramirez, jic23, lorenzo.bianconi Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel Currently IRQ_NONE is returned only when there is no data on the fifo. When there is no data on the fifo the driver can not push to the buffers and therefore user space readers polling for data available will not be awoken and continue to wait. This commit just extends the same semantics to fifo read errors. Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com> --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c index 4994f92..4959923 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c @@ -472,7 +472,7 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) count = st_lsm6dsx_read_fifo(hw); mutex_unlock(&hw->fifo_lock); - return !count ? IRQ_NONE : IRQ_HANDLED; + return (!count || count < 0) ? IRQ_NONE : IRQ_HANDLED; } static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev) -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 8:32 [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers Jorge Ramirez-Ortiz @ 2018-07-11 12:29 ` Lorenzo Bianconi 2018-07-11 13:24 ` Jorge Ramirez-Ortiz 0 siblings, 1 reply; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-11 12:29 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio > Currently IRQ_NONE is returned only when there is no data on the fifo. > > When there is no data on the fifo the driver can not push to the > buffers and therefore user space readers polling for data available > will not be awoken and continue to wait. > > This commit just extends the same semantics to fifo read errors. Hi Jorge, IRQ_NONE is used to indicate this interrupt is not intended for this driver (this could happen if the irq line is in open-drain). If the interrupt is for st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. Regards, Lorenzo > > Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com> > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > index 4994f92..4959923 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > @@ -472,7 +472,7 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > count = st_lsm6dsx_read_fifo(hw); > mutex_unlock(&hw->fifo_lock); > > - return !count ? IRQ_NONE : IRQ_HANDLED; > + return (!count || count < 0) ? IRQ_NONE : IRQ_HANDLED; > } > > static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev) > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 12:29 ` Lorenzo Bianconi @ 2018-07-11 13:24 ` Jorge Ramirez-Ortiz 2018-07-11 14:17 ` Lorenzo Bianconi 2018-07-11 15:26 ` Lorenzo Bianconi 0 siblings, 2 replies; 10+ messages in thread From: Jorge Ramirez-Ortiz @ 2018-07-11 13:24 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: >> Currently IRQ_NONE is returned only when there is no data on the fifo. >> >> When there is no data on the fifo the driver can not push to the >> buffers and therefore user space readers polling for data available >> will not be awoken and continue to wait. >> >> This commit just extends the same semantics to fifo read errors. > Hi Jorge, > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > (this could happen if the irq line is in open-drain). If the interrupt is for > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. yes I understand. This was just a trivial attempt (I guess a really bad idea) to get some debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) fails when processing the data ready irq. do you think it would make sense to add a dev_err to st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail silently thanks for coming back to me despite the bad patch > > Regards, > Lorenzo > >> Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com> >> --- >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c >> index 4994f92..4959923 100644 >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c >> @@ -472,7 +472,7 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) >> count = st_lsm6dsx_read_fifo(hw); >> mutex_unlock(&hw->fifo_lock); >> >> - return !count ? IRQ_NONE : IRQ_HANDLED; >> + return (!count || count < 0) ? IRQ_NONE : IRQ_HANDLED; >> } >> >> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev) >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 13:24 ` Jorge Ramirez-Ortiz @ 2018-07-11 14:17 ` Lorenzo Bianconi 2018-07-11 15:26 ` Lorenzo Bianconi 1 sibling, 0 replies; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-11 14:17 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio > On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: > > > Currently IRQ_NONE is returned only when there is no data on the fifo. > > > > > > When there is no data on the fifo the driver can not push to the > > > buffers and therefore user space readers polling for data available > > > will not be awoken and continue to wait. > > > > > > This commit just extends the same semantics to fifo read errors. > > Hi Jorge, > > > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > > (this could happen if the irq line is in open-drain). If the interrupt is for > > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. > > yes I understand. > > This was just a trivial attempt (I guess a really bad idea) to get some > debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) > fails when processing the data ready irq. No worries, but it is not a good idea to add that info in irq/spurious > do you think it would make sense to add a dev_err to > st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail > silently Could be usefull, afaiu you need to know if fifo read fails for any reason, correct? > > thanks for coming back to me despite the bad patch No worries :) Regards, Lorenzo > > > > > Regards, > > Lorenzo > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com> > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > index 4994f92..4959923 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > @@ -472,7 +472,7 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > > > count = st_lsm6dsx_read_fifo(hw); > > > mutex_unlock(&hw->fifo_lock); > > > - return !count ? IRQ_NONE : IRQ_HANDLED; > > > + return (!count || count < 0) ? IRQ_NONE : IRQ_HANDLED; > > > } > > > static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev) > > > -- > > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 13:24 ` Jorge Ramirez-Ortiz 2018-07-11 14:17 ` Lorenzo Bianconi @ 2018-07-11 15:26 ` Lorenzo Bianconi 2018-07-11 16:00 ` Jorge Ramirez-Ortiz 1 sibling, 1 reply; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-11 15:26 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio On Jul 11, Jorge Ramirez-Ortiz wrote: > On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: > > > Currently IRQ_NONE is returned only when there is no data on the fifo. > > > > > > When there is no data on the fifo the driver can not push to the > > > buffers and therefore user space readers polling for data available > > > will not be awoken and continue to wait. > > > > > > This commit just extends the same semantics to fifo read errors. > > Hi Jorge, > > > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > > (this could happen if the irq line is in open-drain). If the interrupt is for > > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. > > yes I understand. > > This was just a trivial attempt (I guess a really bad idea) to get some > debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) > fails when processing the data ready irq. > do you think it would make sense to add a dev_err to > st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail > silently do you mean something like (just compiled, not tested): --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) err = regmap_bulk_read(hw->regmap, hw->settings->fifo_ops.fifo_diff.addr, &fifo_status, sizeof(fifo_status)); - if (err < 0) + if (err < 0) { + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", + err); return err; + } if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) return 0; @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); - if (err < 0) + if (err < 0) { + dev_err(hw->dev, + "failed to read pattern from fifo (err=%d)\n", + err); return err; + } /* * Data are written to the FIFO with a specific pattern @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) if (unlikely(reset_ts)) { err = st_lsm6dsx_reset_hw_ts(hw); - if (err < 0) + if (err < 0) { + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", + err); return err; + } } return read_len; } Regards, Lorenzo > > thanks for coming back to me despite the bad patch > > > > > Regards, > > Lorenzo > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com> > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > index 4994f92..4959923 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > @@ -472,7 +472,7 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private) > > > count = st_lsm6dsx_read_fifo(hw); > > > mutex_unlock(&hw->fifo_lock); > > > - return !count ? IRQ_NONE : IRQ_HANDLED; > > > + return (!count || count < 0) ? IRQ_NONE : IRQ_HANDLED; > > > } > > > static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev) > > > -- > > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 15:26 ` Lorenzo Bianconi @ 2018-07-11 16:00 ` Jorge Ramirez-Ortiz 2018-07-11 16:34 ` Lorenzo Bianconi 0 siblings, 1 reply; 10+ messages in thread From: Jorge Ramirez-Ortiz @ 2018-07-11 16:00 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio On 07/11/2018 05:26 PM, Lorenzo Bianconi wrote: > On Jul 11, Jorge Ramirez-Ortiz wrote: >> On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: >>>> Currently IRQ_NONE is returned only when there is no data on the fifo. >>>> >>>> When there is no data on the fifo the driver can not push to the >>>> buffers and therefore user space readers polling for data available >>>> will not be awoken and continue to wait. >>>> >>>> This commit just extends the same semantics to fifo read errors. >>> Hi Jorge, >>> >>> IRQ_NONE is used to indicate this interrupt is not intended for this driver >>> (this could happen if the irq line is in open-drain). If the interrupt is for >>> st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. >> yes I understand. >> >> This was just a trivial attempt (I guess a really bad idea) to get some >> debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) >> fails when processing the data ready irq. >> do you think it would make sense to add a dev_err to >> st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail >> silently > do you mean something like (just compiled, not tested): > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > err = regmap_bulk_read(hw->regmap, > hw->settings->fifo_ops.fifo_diff.addr, > &fifo_status, sizeof(fifo_status)); > - if (err < 0) > + if (err < 0) { > + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", > + err); > return err; > + } > > if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) > return 0; > @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); > - if (err < 0) > + if (err < 0) { > + dev_err(hw->dev, > + "failed to read pattern from fifo (err=%d)\n", > + err); > return err; > + } > > /* > * Data are written to the FIFO with a specific pattern > @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > if (unlikely(reset_ts)) { > err = st_lsm6dsx_reset_hw_ts(hw); > - if (err < 0) > + if (err < 0) { > + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", > + err); > return err; > + } > } > return read_len; > } > > Regards, > Lorenzo yes, you beat me to it but yes, that is what I was thinking about. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 16:00 ` Jorge Ramirez-Ortiz @ 2018-07-11 16:34 ` Lorenzo Bianconi 2018-07-11 16:59 ` Jorge Ramirez-Ortiz 0 siblings, 1 reply; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-11 16:34 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio > On 07/11/2018 05:26 PM, Lorenzo Bianconi wrote: > > On Jul 11, Jorge Ramirez-Ortiz wrote: > > > On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: > > > > > Currently IRQ_NONE is returned only when there is no data on the fifo. > > > > > > > > > > When there is no data on the fifo the driver can not push to the > > > > > buffers and therefore user space readers polling for data available > > > > > will not be awoken and continue to wait. > > > > > > > > > > This commit just extends the same semantics to fifo read errors. > > > > Hi Jorge, > > > > > > > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > > > > (this could happen if the irq line is in open-drain). If the interrupt is for > > > > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. > > > yes I understand. > > > > > > This was just a trivial attempt (I guess a really bad idea) to get some > > > debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) > > > fails when processing the data ready irq. > > > do you think it would make sense to add a dev_err to > > > st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail > > > silently > > do you mean something like (just compiled, not tested): > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > err = regmap_bulk_read(hw->regmap, > > hw->settings->fifo_ops.fifo_diff.addr, > > &fifo_status, sizeof(fifo_status)); > > - if (err < 0) > > + if (err < 0) { > > + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", > > + err); > > return err; > > + } > > if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) > > return 0; > > @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); > > - if (err < 0) > > + if (err < 0) { > > + dev_err(hw->dev, > > + "failed to read pattern from fifo (err=%d)\n", > > + err); > > return err; > > + } > > /* > > * Data are written to the FIFO with a specific pattern > > @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > if (unlikely(reset_ts)) { > > err = st_lsm6dsx_reset_hw_ts(hw); > > - if (err < 0) > > + if (err < 0) { > > + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", > > + err); > > return err; > > + } > > } > > return read_len; > > } > > > > Regards, > > Lorenzo > > > yes, you beat me to it but yes, that is what I was thinking about. > Ops, it was not intended as a race :). Feel free to send your patch since you proposed the idea. Regards, Lorenzo > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 16:34 ` Lorenzo Bianconi @ 2018-07-11 16:59 ` Jorge Ramirez-Ortiz 2018-07-11 17:07 ` Lorenzo Bianconi 2018-07-12 7:59 ` Lorenzo Bianconi 0 siblings, 2 replies; 10+ messages in thread From: Jorge Ramirez-Ortiz @ 2018-07-11 16:59 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio On 07/11/2018 06:34 PM, Lorenzo Bianconi wrote: >> On 07/11/2018 05:26 PM, Lorenzo Bianconi wrote: >>> On Jul 11, Jorge Ramirez-Ortiz wrote: >>>> On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: >>>>>> Currently IRQ_NONE is returned only when there is no data on the fifo. >>>>>> >>>>>> When there is no data on the fifo the driver can not push to the >>>>>> buffers and therefore user space readers polling for data available >>>>>> will not be awoken and continue to wait. >>>>>> >>>>>> This commit just extends the same semantics to fifo read errors. >>>>> Hi Jorge, >>>>> >>>>> IRQ_NONE is used to indicate this interrupt is not intended for this driver >>>>> (this could happen if the irq line is in open-drain). If the interrupt is for >>>>> st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. >>>> yes I understand. >>>> >>>> This was just a trivial attempt (I guess a really bad idea) to get some >>>> debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) >>>> fails when processing the data ready irq. >>>> do you think it would make sense to add a dev_err to >>>> st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail >>>> silently >>> do you mean something like (just compiled, not tested): >>> >>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c >>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c >>> @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) >>> err = regmap_bulk_read(hw->regmap, >>> hw->settings->fifo_ops.fifo_diff.addr, >>> &fifo_status, sizeof(fifo_status)); >>> - if (err < 0) >>> + if (err < 0) { >>> + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", >>> + err); >>> return err; >>> + } >>> if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) >>> return 0; >>> @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) >>> for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { >>> err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); >>> - if (err < 0) >>> + if (err < 0) { >>> + dev_err(hw->dev, >>> + "failed to read pattern from fifo (err=%d)\n", >>> + err); >>> return err; >>> + } >>> /* >>> * Data are written to the FIFO with a specific pattern >>> @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) >>> if (unlikely(reset_ts)) { >>> err = st_lsm6dsx_reset_hw_ts(hw); >>> - if (err < 0) >>> + if (err < 0) { >>> + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", >>> + err); >>> return err; >>> + } >>> } >>> return read_len; >>> } >>> >>> Regards, >>> Lorenzo >> >> yes, you beat me to it but yes, that is what I was thinking about. >> > Ops, it was not intended as a race :). Feel free to send your patch since you > proposed the idea. hey of course not :) please go ahead with your change! got to start packing for my summer break anyways :) I was working this morning validating the driver on the LSM6DS33TR with libiio and it is all good btw. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 16:59 ` Jorge Ramirez-Ortiz @ 2018-07-11 17:07 ` Lorenzo Bianconi 2018-07-12 7:59 ` Lorenzo Bianconi 1 sibling, 0 replies; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-11 17:07 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio On Jul 11, Jorge Ramirez-Ortiz wrote: > On 07/11/2018 06:34 PM, Lorenzo Bianconi wrote: > > > On 07/11/2018 05:26 PM, Lorenzo Bianconi wrote: > > > > On Jul 11, Jorge Ramirez-Ortiz wrote: > > > > > On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: > > > > > > > Currently IRQ_NONE is returned only when there is no data on the fifo. > > > > > > > > > > > > > > When there is no data on the fifo the driver can not push to the > > > > > > > buffers and therefore user space readers polling for data available > > > > > > > will not be awoken and continue to wait. > > > > > > > > > > > > > > This commit just extends the same semantics to fifo read errors. > > > > > > Hi Jorge, > > > > > > > > > > > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > > > > > > (this could happen if the irq line is in open-drain). If the interrupt is for > > > > > > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. > > > > > yes I understand. > > > > > > > > > > This was just a trivial attempt (I guess a really bad idea) to get some > > > > > debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) > > > > > fails when processing the data ready irq. > > > > > do you think it would make sense to add a dev_err to > > > > > st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail > > > > > silently > > > > do you mean something like (just compiled, not tested): > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > err = regmap_bulk_read(hw->regmap, > > > > hw->settings->fifo_ops.fifo_diff.addr, > > > > &fifo_status, sizeof(fifo_status)); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) > > > > return 0; > > > > @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > > > err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, > > > > + "failed to read pattern from fifo (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > /* > > > > * Data are written to the FIFO with a specific pattern > > > > @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > if (unlikely(reset_ts)) { > > > > err = st_lsm6dsx_reset_hw_ts(hw); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > } > > > > return read_len; > > > > } > > > > > > > > Regards, > > > > Lorenzo > > > > > > yes, you beat me to it but yes, that is what I was thinking about. > > > > > Ops, it was not intended as a race :). Feel free to send your patch since you > > proposed the idea. > > hey of course not :) please go ahead with your change! got to start packing > for my summer break anyways :) Ok, will post a patch adding that stuff. Have a nice vacation ;) > > I was working this morning validating the driver on the LSM6DS33TR with > libiio and it is all good btw. > Cool! > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers 2018-07-11 16:59 ` Jorge Ramirez-Ortiz 2018-07-11 17:07 ` Lorenzo Bianconi @ 2018-07-12 7:59 ` Lorenzo Bianconi 1 sibling, 0 replies; 10+ messages in thread From: Lorenzo Bianconi @ 2018-07-12 7:59 UTC (permalink / raw) To: Jorge Ramirez-Ortiz; +Cc: jic23, knaack.h, lars, pmeerw, linux-iio > On 07/11/2018 06:34 PM, Lorenzo Bianconi wrote: > > > On 07/11/2018 05:26 PM, Lorenzo Bianconi wrote: > > > > On Jul 11, Jorge Ramirez-Ortiz wrote: > > > > > On 07/11/2018 02:29 PM, Lorenzo Bianconi wrote: > > > > > > > Currently IRQ_NONE is returned only when there is no data on the fifo. > > > > > > > > > > > > > > When there is no data on the fifo the driver can not push to the > > > > > > > buffers and therefore user space readers polling for data available > > > > > > > will not be awoken and continue to wait. > > > > > > > > > > > > > > This commit just extends the same semantics to fifo read errors. > > > > > > Hi Jorge, > > > > > > > > > > > > IRQ_NONE is used to indicate this interrupt is not intended for this driver > > > > > > (this could happen if the irq line is in open-drain). If the interrupt is for > > > > > > st_lsm6dsx I would prefer to return IRQ_HANDLED even in case of error. > > > > > yes I understand. > > > > > > > > > > This was just a trivial attempt (I guess a really bad idea) to get some > > > > > debug info (via /proc/irq/.../spurious) any time the driver read (spi/i2c) > > > > > fails when processing the data ready irq. > > > > > do you think it would make sense to add a dev_err to > > > > > st_lsm6dsx_i2c_read/st_lsm6dsx_spi_read? at the moment the driver would fail > > > > > silently > > > > do you mean something like (just compiled, not tested): > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > @@ -298,8 +298,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > err = regmap_bulk_read(hw->regmap, > > > > hw->settings->fifo_ops.fifo_diff.addr, > > > > &fifo_status, sizeof(fifo_status)); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, "failed to read fifo status reg (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK)) > > > > return 0; > > > > @@ -313,8 +316,12 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > > > err = st_lsm6dsx_read_block(hw, hw->buff, pattern_len); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, > > > > + "failed to read pattern from fifo (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > /* > > > > * Data are written to the FIFO with a specific pattern > > > > @@ -385,8 +392,11 @@ static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > if (unlikely(reset_ts)) { > > > > err = st_lsm6dsx_reset_hw_ts(hw); > > > > - if (err < 0) > > > > + if (err < 0) { > > > > + dev_err(hw->dev, "failed to reset hw ts (err=%d)\n", > > > > + err); > > > > return err; > > > > + } > > > > } > > > > return read_len; > > > > } > > > > > > > > Regards, > > > > Lorenzo > > > > > > yes, you beat me to it but yes, that is what I was thinking about. > > > > > Ops, it was not intended as a race :). Feel free to send your patch since you > > proposed the idea. > > hey of course not :) please go ahead with your change! got to start packing > for my summer break anyways :) > > I was working this morning validating the driver on the LSM6DS33TR with > libiio and it is all good btw. According to the wai, LSM6DS33TR should be compatible with LSM6DS3/LSM6DS3H (I just looked at wai val, not all ds). If so I guess you can add explicit support for it (after your vacation :)) Regards, Lorenzo > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-07-12 7:59 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-11 8:32 [PATCH] iio: imu: st_lsm6dsx: irq not handled unless data pushed to buffers Jorge Ramirez-Ortiz 2018-07-11 12:29 ` Lorenzo Bianconi 2018-07-11 13:24 ` Jorge Ramirez-Ortiz 2018-07-11 14:17 ` Lorenzo Bianconi 2018-07-11 15:26 ` Lorenzo Bianconi 2018-07-11 16:00 ` Jorge Ramirez-Ortiz 2018-07-11 16:34 ` Lorenzo Bianconi 2018-07-11 16:59 ` Jorge Ramirez-Ortiz 2018-07-11 17:07 ` Lorenzo Bianconi 2018-07-12 7:59 ` Lorenzo Bianconi
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).