Message-ID: <20240901150857.7f130363@jic23-huawei>
Date: Sun, 1 Sep 2024 15:08:57 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Abhash jha <abhashkumarjha123@...il.com>
Cc: linux-iio@...r.kernel.org, songqiang1304521@...il.com, lars@...afoo.de,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: proximity: vl53l0x-i2c: Added continuous mode
 support

On Sat, 31 Aug 2024 22:12:27 +0530
Abhash jha <abhashkumarjha123@...il.com> wrote:

> > Also, consider if other triggers could be used as if not you need to
> > both document why and add the validation callbacks to stop other triggers
> > being assigned (once you've added one that can be!)
> >
> > Feel free to ask if you have more questions, but your first reference
> > should be other drivers (and I hope we don't have any that do it this way).
> >  
> I used this driver as a reference
> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/vf610_adc.c#L556

ouch.  Whilst that's an ancient driver, its unfortunately handling the buffer
completely wrongly :(  It's old enough that I suspect there will be no interest
in improving that one.

> 
> >
> > ouch, not a write 1 to clear register?  I can't find docs, but this is really
> > nasty bit of interface design if you have to toggle the bit.
> >  
> Actually ST has not provided a register map or any application note
> for the sensor.
> So there's no way to cross reference. Hence I kept the original code.
> But I will try to write 1 to clear register with my sensor.
> 
> > > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > > +                                     indio_dev,
> > > +                                     &iio_pollfunc_store_time,  
> >
> > This is odd.  You don't seem to have a function to be called to actually store
> > the data.  Note you also need to consider if other triggers might be used.
> >
> > I'm not sure what reason we have to do that here though as this is a very
> > conventional one interrupt per 'scan' of data device.
> >
> > So you should be registering a trigger, and a buffer then letting the
> > trigger drive the buffer.  
> 
> Why do I need to register a trigger? Would it be fine to let the irq
> fill the buffer
> with data as  it continuously reads it in the poll function?

That is a possibility but it's not nearly as flexible as providing
a trigger so we generally only do that if there is a fifo in the way
so no 'per scan' signal is available.

The model of IIO is that you may associate different triggers
with a device and each trigger can drive capture from multiple devices
allowing reasonable data synchronisation.  There are lots of
devices that don't have a sequencer / 'continuous' mode and we
want the interface to looks the same for those as it does for those
that do have such support.

> 
> So according to my understanding,
> I need to move all the data reading and pushing to the poll function
> and not do it in the irq handler.
> Then register that poll function here during iio_triggered_buffer_setup.
> Is there something that I am missing?

You have it right.

The only other thing is to work out if it is possible to use other
triggers with the device (I can't see why you wouldn't be able to
use this trigger to drive other device).  

Sometimes using other triggers requires a slightly more complex
sequence so you can check if using a devices own trigger or not
and modify what goes on if it is a different trigger.

If multiple trigger support is complex, it is fine to start out
with providing a trigger validation function iio_info->validate_trigger()
and there is an iio_validate_own_trigger() that you can use to
avoid having to code anything. That relies on the iio_trigger
and the iio_device having the same parent (which they should
do here).

Jonathan

> 
> Regards,
> Abhash

Powered by blists - more mailing lists