Questions about ad5933 driver

Questions about ad5933 driver

[David Schiller]

Hi!

I attempted to use the ad5933 impedance analyzer driver in combination
with libiio.

Trying to enumerate the local devices with "iio_info" leads to an error,
which I traced down to the naming of the in_voltage_{real,imag}_* sysfs
scan_elements nodes. The name modifiers "real" and "imag" are not valid
sysfs symbols, it seems.
The driver is in the staging directory, so I don't know if it has to
conform to the general IIO sysfs ABI.

I worked around this by patching libiio, but I don't know if that's the
correct approach.

What do you think?

Best regards
David

Re: Questions about ad5933 driver

[Jonathan Cameron]

On Tue, 6 Jun 2023 10:46:14 +0200
David Schiller <david.schiller@jku.at> wrote:

> Hi!
> 
> I attempted to use the ad5933 impedance analyzer driver in combination
> with libiio.
> 
> Trying to enumerate the local devices with "iio_info" leads to an error,
> which I traced down to the naming of the in_voltage_{real,imag}_* sysfs
> scan_elements nodes. The name modifiers "real" and "imag" are not valid
> sysfs symbols, it seems.
> The driver is in the staging directory, so I don't know if it has to
> conform to the general IIO sysfs ABI.

It 'should' but it doesn't which is part of the reason it's still in staging
after all these years.

> 
> I worked around this by patching libiio, but I don't know if that's the
> correct approach.

Ideally clean up the driver.  If you are willing that would be great, if not
would you be able to test changes made by someone else?  You are first person
in years that I know has one! :)  I can't remember how far this driver is
from being ready to move out of staging, but I can probably find some time
to do a thorough review of that in next few weeks.

That may require extra ABI definitions possibly including the real and imag
modifiers at which point your patch to libiio would be correct.

Jonathan

> 
> What do you think?
> 
> Best regards
> David
> 
> 

Re: Questions about ad5933 driver

[David Schiller]

On Tue, 2023-06-06 at 11:30 +0100, Jonathan Cameron wrote:
> Ideally clean up the driver.  If you are willing that would be great,
> if not would you be able to test changes made by someone else?  You
> are first person in years that I know has one! :)  I can't remember
> how far this driver is from being ready to move out of staging, but I
> can probably find some time to do a thorough review of that in next
> few weeks.

Yes, I'm willing to test any patches that are provided to me. :)
I can also try to come up with my own changes, but I'm not that familiar
with the IIO subsystem beyond what I've learned in the past couple days,
so I'd need some assistance.

> That may require extra ABI definitions possibly including the real and
> imag modifiers at which point your patch to libiio would be correct.

Yes, that's what I though too. I wasn't sure how "official" libiio is,
as it's not in the kernel tree. My quick and dirty patch currently looks
like this:


diff --git a/channel.c b/channel.c
index 469d037e..6a57a271 100644
--- a/channel.c
+++ b/channel.c
@@ -114,6 +114,8 @@ static const char * const modifier_names[] = {
        [IIO_MOD_PITCH] = "pitch",
        [IIO_MOD_YAW] = "yaw",
        [IIO_MOD_ROLL] = "roll",
+       [IIO_MOD_REAL] = "real",
+       [IIO_MOD_IMAG] = "imag",
 };
 
 /*
diff --git a/iio.h b/iio.h
index 135e335c..3c803479 100644
--- a/iio.h
+++ b/iio.h
@@ -196,6 +196,8 @@ enum iio_modifier {
        IIO_MOD_PITCH,
        IIO_MOD_YAW,
        IIO_MOD_ROLL,
+       IIO_MOD_REAL,
+       IIO_MOD_IMAG,
 };
 
 /**

Re: Questions about ad5933 driver

[Jonathan Cameron]

On Tue, 6 Jun 2023 12:51:50 +0200
David Schiller <david.schiller@jku.at> wrote:

> On Tue, 2023-06-06 at 11:30 +0100, Jonathan Cameron wrote:
> > Ideally clean up the driver.  If you are willing that would be great,
> > if not would you be able to test changes made by someone else?  You
> > are first person in years that I know has one! :)  I can't remember
> > how far this driver is from being ready to move out of staging, but I
> > can probably find some time to do a thorough review of that in next
> > few weeks.  
> 
> Yes, I'm willing to test any patches that are provided to me. :)
> I can also try to come up with my own changes, but I'm not that familiar
> with the IIO subsystem beyond what I've learned in the past couple days,
> so I'd need some assistance.
> 
> > That may require extra ABI definitions possibly including the real and
> > imag modifiers at which point your patch to libiio would be correct.  
> 
> Yes, that's what I though too. I wasn't sure how "official" libiio is,
> as it's not in the kernel tree.

Not 'official' though it is fairly commonly used, but the documented ABI in
Documentation/ABI/testing/sysfs-bus-iio*
is and these aren't there either.

I'm not 100% sure this is the right way to solve this ABI gap though
so need to have a bit of a think about it.  Using a modifier means we can't
use them for anything else, so need to consider if there are other modifiers
(or it has meaning for differential channels) when deciding if this is
an ABI we want to add.

Thanks,

Jonathan


> My quick and dirty patch currently looks
> like this:
> 
> 
> diff --git a/channel.c b/channel.c
> index 469d037e..6a57a271 100644
> --- a/channel.c
> +++ b/channel.c
> @@ -114,6 +114,8 @@ static const char * const modifier_names[] = {
>         [IIO_MOD_PITCH] = "pitch",
>         [IIO_MOD_YAW] = "yaw",
>         [IIO_MOD_ROLL] = "roll",
> +       [IIO_MOD_REAL] = "real",
> +       [IIO_MOD_IMAG] = "imag",
>  };
>  
>  /*
> diff --git a/iio.h b/iio.h
> index 135e335c..3c803479 100644
> --- a/iio.h
> +++ b/iio.h
> @@ -196,6 +196,8 @@ enum iio_modifier {
>         IIO_MOD_PITCH,
>         IIO_MOD_YAW,
>         IIO_MOD_ROLL,
> +       IIO_MOD_REAL,
> +       IIO_MOD_IMAG,
>  };
>  
>  /**
> 
> 
> 
> 

Re: Questions about ad5933 driver

[David Schiller]

On Wed, 2023-06-07 at 15:52 +0100, Jonathan Cameron wrote:
> Not 'official' though it is fairly commonly used, but the documented
> ABI in
> Documentation/ABI/testing/sysfs-bus-iio*
> is and these aren't there either.
> 
> I'm not 100% sure this is the right way to solve this ABI gap though
> so need to have a bit of a think about it.  Using a modifier means we
> can't
> use them for anything else, so need to consider if there are other
> modifiers
> (or it has meaning for differential channels) when deciding if this is
> an ABI we want to add.

The naming of the modifiers is also somewhat confusing. The HW registers
represent the DFT of the input signal, which together with a gain factor
is used to calculate the impedance. Dimensionally the quantities are
admittances. "voltage_real" and "voltage_imag" are therefore not quite
fitting.

Also the sweep setup commands are only run once per buffer (in
ad5933_ring_preenable). This means a buffer can only be used for one
sweep and then has to be disabled and enabled again. As the driver does
not implement any triggers this is right now the only way to start a
measurement.

Do you think it would make sense to implement a user-space trigger to
start a measurement sweep? This would mean that a buffer can be reused.
Right now the "iio_readdev" test program from libiio does not work due
to this behavior. I don't know how this is handled on other IIO drivers.

Lastly, should I CC the original author (Michael Hennerich) going
forward?

Thanks!

Re: Questions about ad5933 driver

[Jonathan Cameron]

On Tue, 13 Jun 2023 12:37:39 +0200
David Schiller <david.schiller@jku.at> wrote:

> On Wed, 2023-06-07 at 15:52 +0100, Jonathan Cameron wrote:
> > Not 'official' though it is fairly commonly used, but the documented
> > ABI in
> > Documentation/ABI/testing/sysfs-bus-iio*
> > is and these aren't there either.
> > 
> > I'm not 100% sure this is the right way to solve this ABI gap though
> > so need to have a bit of a think about it.  Using a modifier means we
> > can't
> > use them for anything else, so need to consider if there are other
> > modifiers
> > (or it has meaning for differential channels) when deciding if this is
> > an ABI we want to add.  
> 
> The naming of the modifiers is also somewhat confusing. The HW registers
> represent the DFT of the input signal, which together with a gain factor
> is used to calculate the impedance. Dimensionally the quantities are
> admittances. "voltage_real" and "voltage_imag" are therefore not quite
> fitting.

In that case, sounds like we need a new channel type to correctly reflect
that they are admittance rather than pretending they are voltages.

> 
> Also the sweep setup commands are only run once per buffer (in
> ad5933_ring_preenable). This means a buffer can only be used for one
> sweep and then has to be disabled and enabled again. As the driver does
> not implement any triggers this is right now the only way to start a
> measurement.

This sounds a bit like the impact sensors - where an event triggers a series
of measurements of 'something'.

> 
> Do you think it would make sense to implement a user-space trigger to
> start a measurement sweep? This would mean that a buffer can be reused.
> Right now the "iio_readdev" test program from libiio does not work due
> to this behavior. I don't know how this is handled on other IIO drivers.

A single trigger normally results in the capture of a single set
of channels measurements.  Here that's sort of the case, but the set of
channels is huge - because each one corresponds to a particular frequency.
There can I think be up to 512 such points.  It would be messy to represent
so many channels (if nothing else some of the ways we store data in IIO -
particularly events, are limited to 256 channels).

My guess is that the calculations around a sweep are sufficiently complex
that, if libiio handled the re setup of a sweep neatly, it would be fine
to go through the disable / reenable of the buffer?
I think bodging in trigger support where a trigger causes 'N' samples would
be tricky.

There is another option though I'm not sure how closely it fits.
A channel can have a scan_type element of repeats which could be set
to a fixed value of 512 (likely all scan_type things it's not runtime
configurable) as this might be considered to be lots of repeated reads
of a single channel.


> 
> Lastly, should I CC the original author (Michael Hennerich) going
> forward?

Sure - +CC Michael.

> 
> Thanks!
> 
>