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