Re: [PATCH 1/2] iio: light: add support to UVIS25 sensor

On Fri, 27 Oct 2017 15:26:09 +0200
Lorenzo Bianconi [off-list ref] wrote:

quoted

On Wed, 25 Oct 2017 20:16:08 +0200
Lorenzo Bianconi [off-list ref] wrote:

Really minor English comment to start.
add support 'for' UVIS25 sensor
 

Hi Jonathan,

I added regmap support, thanks for the hint, the code is much better :)
Just wondering how to manage drdy issue in more elegant way.
As you outlined, the problem is due to the fact the trigger interrupt
is currently masked
but st_uvis25_set_enable() in st_uvis25_read_oneshot() actually
enables interrupt generation
so during the msleep() we get tons of interrupts if we configured the
line as IRQ_TYPE_LEVEL_HIGH.

Can you clear it with a single read at the start of the read_oneshot?

One possible solution could be to drop trigger support and push data
to iio buffer directly in main irq thread handler (but in this way we
are force to use just this kind of trigger).
What do you think?

Not ideal... It's another case of hardware doing something unhelpful
for software.  We'll have to paper over it as usual ;)

Go with which ever route you think works out nicest.

Sorry for the delay on this reply - I missed your reply on my first
pass through my emails in amongst the other parts of the thread.

Reagrds,
Lorenzo

<snip>

quoted

quoted

+
+     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
+     if (err < 0) {
+             dev_err(hw->dev, "failed to read %02x register\n", addr);
+             goto unlock;
+     }
+
+     data = (data & ~mask) | ((val << __ffs(mask)) & mask);
+
+     err = hw->tf->write(hw->dev, addr, sizeof(data), &data);
+     if (err < 0)
+             dev_err(hw->dev, "failed to write %02x register\n", addr);
+
+unlock:
+     mutex_unlock(&hw->lock);
+
+     return err < 0 ? err : 0;
+}
+
+int st_uvis25_set_enable(struct st_uvis25_hw *hw, bool enable)
+{
+     int err;
+
+     err = st_uvis25_write_with_mask(hw, ST_UVIS25_REG_CTRL1_ADDR,
+                                     ST_UVIS25_REG_ODR_MASK, enable);
+     if (err < 0)
+             return err;
+
+     hw->enabled = enable;
+
+     return 0;
+}
+
+static int st_uvis25_read_oneshot(struct st_uvis25_hw *hw, u8 addr, int *val)
+{
+     u8 data;
+     int err;
+
+     err = st_uvis25_set_enable(hw, true);
+     if (err < 0)
+             return err;
+
+     msleep(1500);  

Could drive this off the interrupt rather than disabling the interrupt?
Would that be a little neater (simple completion here).
 

What do you mean? I did not get you.

If the interrupt 'stuck' case wasn't occuring you could basically
let the buffered path run once in order to get the oneshot read.
Thus you could use an interrupt instead of a sleep.
Doesn't really matter though.

quoted

quoted

+
+     err = hw->tf->read(hw->dev, addr, sizeof(data), &data);
+     if (err < 0)
+             return err;  

Is there a potential race here if for some reason we managed to
got to sleep for another conversion?  I think to be completely
safe you need force an additional read after the disable (or
will that fail to clear the data ready?
 

We can move the read access after the st_uvis25_set_enable(hw, false)
in order to avoid an unnecessary read, do you agree?

You could perhaps get a later reading if you did that?
I don't suppose it really matters.

quoted

quoted

+
+     st_uvis25_set_enable(hw, false);
+
+     *val = data;
+
+     return IIO_VAL_INT;
+}
+
+static int st_uvis25_read_raw(struct iio_dev *iio_dev,
+                           struct iio_chan_spec const *ch,
+                           int *val, int *val2, long mask)
+{
+     int ret;
+
+     ret = iio_device_claim_direct_mode(iio_dev);
+     if (ret)
+             return ret;
+
+     switch (mask) {
+     case IIO_CHAN_INFO_PROCESSED: {
+             struct st_uvis25_hw *hw = iio_priv(iio_dev);
+
+             /*
+              * mask irq line during oneshot read since the sensor
+              * does not export the capability to disable data-ready line
+              * in the register map and it is enabled by default.
+              * If the line is unmasked during read_raw() it will be set
+              * active and never reset since the trigger is disabled
+              */  

Nasty but well documented...

I wonder if there is a nicer way to handle this... If we leave it on
the issues is that we end up with the status being checked by the interrupt
handler for the trigger (harmless if a waste of time) then the trigger
being fired (with nothing associated with it).  No consumers are attached
so we call notify done for all of them and finally that will result in
a try reenable. You could supply one of those that results in a read
to the device.  It think that would always deal with your case of
the data ready getting stuck high..

(Not totally sure though as it's been a while since I dealt with a
sensor with this particular issue).
 

quoted

+             if (hw->irq > 0)
+                     disable_irq(hw->irq);
+             ret = st_uvis25_read_oneshot(hw, ch->address, val);
+             if (hw->irq > 0)
+                     enable_irq(hw->irq);
+             break;
+     }
+     default:
+             ret = -EINVAL;
+             break;
+     }