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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jonathan,

comments inline.

> On Fri, 27 Oct 2017 15:26:09 +0200
> Lorenzo Bianconi <lorenzo.bianconi83@xxxxxxxxx> wrote:
>
>> > On Wed, 25 Oct 2017 20:16:08 +0200
>> > Lorenzo Bianconi <lorenzo.bianconi83@xxxxxxxxx> 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?

I guess we could have a potential race doing in that way right?

>
>> 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.

I guess the less ugly approach is to mask the irq line for one-shot reading.
Right, in this case hw is doing something unhelpful for sw guys :)

>
> 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.
>

No worries, I knew you were traveling :)
Btw I have to send the v2 with a different Signed-off-by, my email
address is no more valid since I moved to a different company

Regards,
Lorenzo

>>
>> Reagrds,
>> Lorenzo
>>
> <snip>
>>
>> >> +
>> >> +     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.
>
>>
>> >> +
>> >> +     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.

The output register will keep the right value since the ODR is just
1Hz, so I guess it does not matter

>>
>> >> +
>> >> +     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).
>> >
>> >> +             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;
>> >> +     }



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux