Re: [PATCH] staging: iio: ad5933: replaced kfifo by triggered_buffer

[Jonathan Cameron]

On Sun, 2 Dec 2018 16:10:45 -0200
Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:

> On 11/25, Jonathan Cameron wrote:
> > On Thu, 22 Nov 2018 10:53:47 -0200
> > Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
> >   
> > > Previously, there was an implicit creation of a kfifo which was replaced
> > > by a call to triggered_buffer_setup, which is already implemented in iio
> > > infrastructure.
> > > 
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>  
> > 
> > I'm a little surprised that this would work without screaming a lot as 
> > it will register an interrupt with no handlers. Do you have this
> > device to test?  It's rapidly heading in the direction of too complex
> > a driver to fix without test hardware.  Also, semantically this change
> > is not sensible as it implies an operating mode which the driver
> > is not using.
> >   
> 
> Thanks for reviewing the patch. I'm not expert at electronics so I'm
> still studying the datasheet to understand how the ad5933 works.
> 
> > There are fundamental questions about how we handle an autotriggered
> > sweep that need answering before this driver can move forwards.
> > It needs some concept of a higher level trigger rather than a per
> > sample one like we typically use in IIO.
> >   
> 
> From what I've read so far it seems that we would need to have two
> operation modes: one for the frequency sweep (that need to be
> discussed yet) and another for the receive stage (in which ad5933 would
> be continuously requested for data through I2C). So my first approach
> would be to build up an abstract trigger that would allow switching
> between these two operation modes. What do you think about that?

Hmm.  I haven't looked at it in much depth yet but based largely on the intro
to the datasheet and what you say here...

Mode 1 : Sweep.
* Controls for peak-to-peak, start, resolution, number of points.

Two ways we could treat this.
* A single buffer with no meta data and rely on start and stop signals
  and careful sync.  This is similar to what we have previously talked
  about doing for impact sensors.

* We could present the current frequency as a channel.  At which point
  this is a simple triple of drive frequency, real and imaginary responses.
  It would need a start control though to get a sweep going. Also potentially
  a control to force a waiting period for the start frequency to sweep
  transition.  There is also the repeat option to deal with.

Mode 2 : Read without changing anything?  This is just hitting the repeat
constantly I guess as I can't immediately see another control for it.
This could also be implemented within the above as a one step sweep with
an indefinite repeat count (magic number such as -1).

Anyhow, just some initial thoughts. I'll think more on this one as
the best answer isn't obvious!

> 
> > The main focus in the short term should be around defining that ABI
> > as it may fundamentally change the structure of the driver.
> > If you want to take this on (and it'll be a big job I think!) then
> > it may be possible to source some hardware to support that effort.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> As a member of the FLOSS group at Universidade de São Paulo I have the
> hardware to test the driver though I didn't figure out all the steps to
> do it yet. I intend to continue development on this driver so I'm really
> thankful for all advise given.

Great!

Jonathan

> 
> Thanks,
> 
> Marcelo
> 
> > > ---
> > >  .../staging/iio/impedance-analyzer/Kconfig    |  2 +-
> > >  .../staging/iio/impedance-analyzer/ad5933.c   | 25 ++++---------------
> > >  2 files changed, 6 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/impedance-analyzer/Kconfig b/drivers/staging/iio/impedance-analyzer/Kconfig
> > > index dd97b6bb3fd0..d0af5aa55dc0 100644
> > > --- a/drivers/staging/iio/impedance-analyzer/Kconfig
> > > +++ b/drivers/staging/iio/impedance-analyzer/Kconfig
> > > @@ -7,7 +7,7 @@ config AD5933
> > >  	tristate "Analog Devices AD5933, AD5934 driver"
> > >  	depends on I2C
> > >  	select IIO_BUFFER
> > > -	select IIO_KFIFO_BUF
> > > +	select IIO_TRIGGERED_BUFFER
> > >  	help
> > >  	  Say yes here to build support for Analog Devices Impedance Converter,
> > >  	  Network Analyzer, AD5933/4, provides direct access via sysfs.
> > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > > index f9bcb8310e21..edb8b540bbf1 100644
> > > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > > @@ -20,7 +20,7 @@
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > >  #include <linux/iio/buffer.h>
> > > -#include <linux/iio/kfifo_buf.h>
> > > +#include <linux/iio/triggered_buffer.h>
> > >  
> > >  /* AD5933/AD5934 Registers */
> > >  #define AD5933_REG_CONTROL_HB		0x80	/* R/W, 1 byte */
> > > @@ -615,22 +615,6 @@ static const struct iio_buffer_setup_ops ad5933_ring_setup_ops = {
> > >  	.postdisable = ad5933_ring_postdisable,
> > >  };
> > >  
> > > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > > -{
> > > -	struct iio_buffer *buffer;
> > > -
> > > -	buffer = iio_kfifo_allocate();
> > > -	if (!buffer)
> > > -		return -ENOMEM;
> > > -
> > > -	iio_device_attach_buffer(indio_dev, buffer);
> > > -
> > > -	/* Ring buffer functions - here trigger setup related */
> > > -	indio_dev->setup_ops = &ad5933_ring_setup_ops;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  static void ad5933_work(struct work_struct *work)
> > >  {
> > >  	struct ad5933_state *st = container_of(work,
> > > @@ -744,7 +728,8 @@ static int ad5933_probe(struct i2c_client *client,
> > >  	indio_dev->channels = ad5933_channels;
> > >  	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> > >  
> > > -	ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > > +	ret = iio_triggered_buffer_setup(indio_dev, NULL, NULL,
> > > +					 &ad5933_ring_setup_ops);  
> > 
> > The absence of either of the interrupt related callbacks made me
> > wonder what is going on here.  The upshot is that this device
> > isn't operating in a triggered buffer style at all so we really
> > shouldn't be using that infrastructure - even if it's convenient.
> > 
> > It'll allocate an interrupt with neither a top half nor a thread
> > function. I'm not sure what the core will do about that but it
> > seems unlikely to be happy about it!
> >   
> 
> The reason for not using triggered_buffer would be because I2C of
> communication is always started by the master device so the ad5933 would
> only wait for being requested for data. It wouldn't be capable of using
> I2C to call for master device nor it has any interrupt pin to do that.
> Is that right?
> 
> >   
> > >  	if (ret)
> > >  		goto error_disable_reg;
> > >  
> > > @@ -759,7 +744,7 @@ static int ad5933_probe(struct i2c_client *client,
> > >  	return 0;
> > >  
> > >  error_unreg_ring:
> > > -	iio_kfifo_free(indio_dev->buffer);
> > > +	iio_triggered_buffer_cleanup(indio_dev);
> > >  error_disable_reg:
> > >  	regulator_disable(st->reg);
> > >  
> > > @@ -772,7 +757,7 @@ static int ad5933_remove(struct i2c_client *client)
> > >  	struct ad5933_state *st = iio_priv(indio_dev);
> > >  
> > >  	iio_device_unregister(indio_dev);
> > > -	iio_kfifo_free(indio_dev->buffer);
> > > +	iio_triggered_buffer_cleanup(indio_dev);
> > >  	regulator_disable(st->reg);
> > >  
> > >  	return 0;  
> >