Questions related to some drivers

[Giuliano Belinassi]

Hello,

We are a student group and we want to contribute to the linux kernel. We
are thinking in put effort to move a driver from staging to the main tree.
We looked into three drivers:

* adc/ad7780.c
* adc/ad7816.c
* impedance-analyzer/ad5933.c

Are these drivers still being worked on? The last commit referencing these
drivers were on march 2018, but they are still in staging since 2010.

We already know that we have to convert from the old api to the new one. It is
OK for us to work on this? Is there something else we can do?

Thank you

[Ardelean, Alexandru]

Hey,

Thanks for the notification.

Yes, all these 3 drivers should be moved out of staging.

We were planning to start working on them (to move them out of staging),
but priorities are shifting from one week to the other.
So, if you guys have the time and are willing to do it, please go ahead,
and add me, michael....@analog.com and stefa...@analog.com to the
CC when sending patches, and we will also take a look over your patches.

If you haven't taken a look already, feel free to also take a look at our
wiki:
https://wiki.analog.com/resources/tools-software/linux-drivers-all
It's more technical for the SW side; it might help with some general info.
And the datasheets should be available on the analog.com/<product_id>
pages.


Thanks
Alex

[Giuliano Belinassi]

On 10/18, Ardelean, Alexandru wrote:

Thank you for the reply :-). We will put effort into at least one driver to
remove it from staging.

> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+...@googlegroups.com.
> To post to this group, send email to kerne...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/d0fc5a236a82d03fdbe4d05eb27ff3bd3ac36958.camel%40analog.com.
> For more options, visit https://groups.google.com/d/optout.

[Giuliano Augusto Faulin Belinassi]

Hello,
I have some questions about the ad7780 driver.

* What is the val2 In the line 96 (*val2 = chan->scan_type.realbits
- 1;)? How it is used? Is it related to the 24-bits ADC precision?
* Why there is a subtraction in the line 99 ( *val -= (1 <<
(chan->scan_type.realbits - 1)); )? Is the *val initialized with 0? Is
that related to the formula described in the DATA OUTPUT CODING, at
page 12?
https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf

With regards

> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20181018114145.amqff57holmecxpo%40smtp.gmail.com.

[Giuliano Augusto Faulin Belinassi]

Hello,
Could you help us figure out what must be added/changed into the
staging/iio/adc/ad7780.c to remove it from staiging?

For instance, should we add an id field in ad7780_state struct that
represents ID1 and ID0 from the status bits (page 13 from datasheet)?
Or check the Status pattern bits PAT1, PAT0 for errors? Or something
else we can work on?

Thank you.
On Tue, Oct 23, 2018 at 6:51 AM Jonathan Cameron
<ji...@jic23.retrosnub.co.uk> wrote:

>
> On Mon, 22 Oct 2018 18:56:16 -0300
> Giuliano Augusto Faulin Belinassi <giuliano....@usp.br> wrote:
>
> > Hello,
> > I have some questions about the ad7780 driver.
> >
> > * What is the val2 In the line 96 (*val2 = chan->scan_type.realbits
> > - 1;)? How it is used? Is it related to the 24-bits ADC precision?
>

> To understand that follow through what the code does when the type of a
> read_raw is IIO_VAL_FRACTIONAL_LOG2;

>
> > * Why there is a subtraction in the line 99 ( *val -= (1 <<
> > (chan->scan_type.realbits - 1)); )? Is the *val initialized with 0? Is
> > that related to the formula described in the DATA OUTPUT CODING, at
> > page 12?
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
>

> Yeah, that -= is definitely wrong. Should just be an assignment to the
> negative value. This is telling userspace that the raw value should be
> offset by half of it's maximum range before applying the scale to get
> a reading in the IIO base units (here mV IIRC).

[Ardelean, Alexandru]

On Mon, 2018-11-05 at 16:46 -0200, Giuliano Augusto Faulin Belinassi wrote:
> Hello,
> Could you help us figure out what must be added/changed into the
> staging/iio/adc/ad7780.c to remove it from staiging?
>
> For instance, should we add an id field in ad7780_state struct that
> represents ID1 and ID0 from the status bits (page 13 from datasheet)?
> Or check the Status pattern bits PAT1, PAT0 for errors? Or something
> else we can work on?
>

Hey,

You don't need to check any patterns, that's already being done in the
driver (see `pattern` & `pattern_mask` fields).
And see function ad7780_postprocess_sample().
Though I will admit it would have been nicer if those patterns were
generated from AD7780_PAT1 & AD7780_PAT0. So (if you want), you could
update those pattern assignments with some macros ; for AD717x you may need
to add AD7170_PAT2 (BIT(2))

I am noticing a small bug with the driver now
(in ad7780_postprocess_sample()).
The AD7780_GAIN bit-field (for AD778x) overlaps with AD7170_PAT2 (for
AD717x). So, maybe you could add a field called `is_ad778x` in
`ad7780_chip_info` and initialize it to true in the `ad7780_chip_info_tbl`
for AD7780 & AD7781 ; and only perform the AD7780_GAIN check for these
devices.
According to the datasheets AD7170 & AD7171 have a fixed gain of 1.

I would ignore the ID0 & ID1 fields ; those seem to overlap a bit between
AD717x & AD778x. And they're not very useful in the driver.

For AD778x you could also add support for the GAIN & FILTER pins via GPIOs,
to control these settings. And then in the ad7780_postprocess_sample()
function check if the GPIO settings (stored on a gpio_desc on ad7780_state)
match the expected GAIN & FILTER bit settings ; if not return -EIO.

All-in-all, the driver is almost ready to move out of staging (from my
point of view).
Not sure if anyone else has anything else to add.
The bug should be fixed, and the rest is nice-to-have, but not necessarily
a must (again, from my point of view) to keep it in staging.

Also, generally: maybe do a comparative sweep between AD717x & AD778x
datasheets and see if there's something left that's incorrect for any of
these chips.

Thanks
Alex

[Renato Lui Geh]

On 11/06, Ardelean, Alexandru wrote:
>Hey,
>
>You don't need to check any patterns, that's already being done in the
>driver (see `pattern` & `pattern_mask` fields).
>And see function ad7780_postprocess_sample().
>Though I will admit it would have been nicer if those patterns were
>generated from AD7780_PAT1 & AD7780_PAT0. So (if you want), you could
>update those pattern assignments with some macros ; for AD717x you may need
>to add AD7170_PAT2 (BIT(2))
>
>I am noticing a small bug with the driver now
>(in ad7780_postprocess_sample()).
>The AD7780_GAIN bit-field (for AD778x) overlaps with AD7170_PAT2 (for
>AD717x). So, maybe you could add a field called `is_ad778x` in
>`ad7780_chip_info` and initialize it to true in the `ad7780_chip_info_tbl`
>for AD7780 & AD7781 ; and only perform the AD7780_GAIN check for these
>devices.
>According to the datasheets AD7170 & AD7171 have a fixed gain of 1.
>
>I would ignore the ID0 & ID1 fields ; those seem to overlap a bit between
>AD717x & AD778x. And they're not very useful in the driver.
>

Hi,

Thank you for the clues on what to work on. We've just now sent a
patchset with the fixes you mentioned in the quoted block above.

>For AD778x you could also add support for the GAIN & FILTER pins via GPIOs,
>to control these settings. And then in the ad7780_postprocess_sample()
>function check if the GPIO settings (stored on a gpio_desc on ad7780_state)
>match the expected GAIN & FILTER bit settings ; if not return -EIO.
>
>All-in-all, the driver is almost ready to move out of staging (from my
>point of view).
>Not sure if anyone else has anything else to add.
>The bug should be fixed, and the rest is nice-to-have, but not necessarily
>a must (again, from my point of view) to keep it in staging.
>
>Also, generally: maybe do a comparative sweep between AD717x & AD778x
>datasheets and see if there's something left that's incorrect for any of
>these chips.

We'll take a look on these issues and study them further and will start
working on them once we're confident we understand them.

>Thanks
>Alex

Thanks,
Renato

[Renato Lui Geh]

Hi,

On 11/06, Ardelean, Alexandru wrote:
>For AD778x you could also add support for the GAIN & FILTER pins via GPIOs,
>to control these settings. And then in the ad7780_postprocess_sample()
>function check if the GPIO settings (stored on a gpio_desc on ad7780_state)
>match the expected GAIN & FILTER bit settings ; if not return -EIO.

We're having some trouble with the GPIOs, and would like some insight on
how to proceed. Any help would be very much appreciated!

We're wondering if we should do something like this in ad7780.c's probe:

st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
"powerdown",
GPIOD_OUT_LOW);

for both gain and filter. Taking a look at driver drivers/iio/adc/hx711.c
(another adc driver out of staging), we have:

hx711_data->gpiod_pd_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_LOW);

So we're assuming "sck" and "powerdown" are the pins we're looking for. Are we
correct to assume that these strings are compared with a table that map the
actual GPIO pins? So we'd have something like:

st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
"gain",
GPIO_DOUT_LOW);

Where "gain" is the pin 4 or 5 on the AD778x
(https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf)
chip.

Where can we find this table that maps these pin names to the actual pin
numbers? We found this link
https://www.kernel.org/doc/html/v4.17/driver-api/gpio/board.html that shows how
to declare table attributes, but we couldn't find this lookup table there.

Are we missing something? Out of curiosity, why do we have to pass a string
(e.g. powerdown, gain, sck, dout) instead of the pin number? We found somewhere
that they are names to functions. Are these functions implemented on the chip?

Thanks,
Renato

[Ardelean, Alexandru]

On Thu, 2018-11-08 at 12:04 -0200, Renato Lui Geh wrote:
> Hi,
>

Hey,

> On 11/06, Ardelean, Alexandru wrote:
> > For AD778x you could also add support for the GAIN & FILTER pins via
> > GPIOs,
> > to control these settings. And then in the ad7780_postprocess_sample()
> > function check if the GPIO settings (stored on a gpio_desc on
> > ad7780_state)
> > match the expected GAIN & FILTER bit settings ; if not return -EIO.
>
> We're having some trouble with the GPIOs, and would like some insight on
> how to proceed. Any help would be very much appreciated!
>
> We're wondering if we should do something like this in ad7780.c's probe:
>
> st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
> "powerdown",
> GPIOD_OUT_LOW);
>

Yes, something like this.

> for both gain and filter. Taking a look at driver drivers/iio/adc/hx711.c
> (another adc driver out of staging), we have:
>
> hx711_data->gpiod_pd_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_LOW);
>
> So we're assuming "sck" and "powerdown" are the pins we're looking for.
> Are we
> correct to assume that these strings are compared with a table that map
> the
> actual GPIO pins? So we'd have something like:
>
> st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> "gain",
> GPIO_DOUT_LOW);

This is exactly a good idea to do for `gain`.
And similar for `filter`

>
> Where "gain" is the pin 4 or 5 on the AD778x

The pin 4 or 5 on the AD778x are not what you define here.
The GPIOs you define/use here are on the host-board.

So,

Host-board [dt-entry] <----> AD778x [HW-pin]

Where:
* `dt-entry` is definted in the device-tree something like:
gain-gpio = <&gpio GPIO_NUMBER_ON_HOST_BOARD GPIO_FLAGS>
* GPIO_NUMBER_ON_HOST_BOARD is a number, which varies ; for example for
Raspberry Pi it could be pin 25 [in the device tree];
* GPIO_FLAGS is usually 0 ; but can be any other numeric value defined
here:

https://github.com/torvalds/linux/blob/master/include/dt-bindings/gpio/gpio.h
* `HW-pin` doesn't matter, because it's a physical pin that can be defined
as anything in SW

> (
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf

> )
> chip.
>
> Where can we find this table that maps these pin names to the actual pin
> numbers? We found this link

The actual pin number doesn't matter in the driver you write.
It matters in the device-tree for the board you use the driver [and chip]
on.

If you were to test your driver change on a RPi board and you physically
connect this on Pin 25 (physical) and in your device-tree
GPIO_NUMBER_ON_HOST_BOARD is the value (in SW) for Pin 25, then that's what
matters when you test/run the code.

In the kernel, a pin numbers table for the RPi can be found here:

https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/bcm283x.dtsi#L185

I keep referring to RPi because we've used it a bit more than other boards,
and also because for RPi usually Pin 25 in HW is also Pin 25 in SW.

> https://www.kernel.org/doc/html/v4.17/driver-api/gpio/board.html that
> shows how
> to declare table attributes, but we couldn't find this lookup table
> there.
>
> Are we missing something? Out of curiosity, why do we have to pass a
> string
> (e.g. powerdown, gain, sck, dout) instead of the pin number? We found
> somewhere
> that they are names to functions. Are these functions implemented on the
> chip?
>

So, the string in the driver, will be used to lookup the physical pin in
the device-tree.
The pin number differs from one host-board to another [as I've said], so
the string is a unique identifier for the driver, which resolves to the
physical pin on the board.

By the way: one work item (maybe after the driver is moved out of staging)
would be to also write a device-tree binding doc.
An example:

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/adi,adau17x1.txt
It's not the best one, and feel free to look for others as well, that
define GPIOs. But in that file, is an example of how to link a reset-gpio
that would be called by that driver to physically reset the device when
probed & initialized.

Usually the arch/arm[64]/boot/dts have a lot of device-trees that may help
shape some thinking about device-trees.

I'm hoping I got to explain things somewhat.
It is a bit late in the afternoon [for me], but I thought I'd give it a try
:)

If there are still questions, feel free to ask.
I think I can get to them tomorrow.

Thanks
Alex

> Thanks,
> Renato
>

[Giuliano Augusto Faulin Belinassi]

Hello. Sorry for the late reply

> On a more private note: do you guys have HW to test your code/changes on ?
No, we don't have it. I think it would be really nice if we could have
access to the
hardware to work on. :-)

> Jonathan did ask us about this in private, but I think the discussion has
> stalled a bit (time & priorities).
>
> We could send a few samples if there's anything you'd like to test this
> code on.

That would be really helpful :-)

> FWIW: we usually test quite a few of this simple ADC/DAC changes on
> Raspberry PI ; it's a pretty useful board for testing stuff.

[Giuliano Augusto Faulin Belinassi]

Hello,
We have some doubts regarding the FILTER and GAIN pins. Should
these pins be set in userspace, and therefore there must be a
'ad7780_write_raw' function? Or should we use the device tree with a
value set on initialization?

Thank you

On Thu, Nov 8, 2018 at 12:46 PM Ardelean, Alexandru
<alexandru...@analog.com> wrote:
>

[Ardelean, Alexandru]

On Tue, 2018-11-13 at 11:56 -0200, Giuliano Augusto Faulin Belinassi wrote:
> Hello,
> We have some doubts regarding the FILTER and GAIN pins. Should
> these pins be set in userspace, and therefore there must be a
> 'ad7780_write_raw' function? Or should we use the device tree with a
> value set on initialization?
>

So, definitely add the code that initializes the pins via device-tree.
Quite a few users of the driver may only want a single gain value for the
entire life-cycle of the chip running ; and this can be neatly initialized
from DT.

Adding a 'ad7780_write_raw' function for updating these pins, also sounds
like a neat idea.
Then you could update the GAIN and FILTER pins via user-space calls.
I guess it's up to you if you want to do this. But I don't know how many
users of the driver would require/want this flexibility with this
particular chip [we haven't received any requests for this (yet) AFAIK].

Doing this is a bit more work, because you'll have to create a new macro
like AD_SD_CHANNEL() which allows to set custom bit/mask fields [1], or
just create a AD_SD_CHANNEL_GAIN() or AD_SD_CHANNEL_GAIN_FILTER() macro.
[ see include/linux/iio/adc/ad_sigma_delta.h for AD_SD_CHANNEL() ].

I guess the [1] variant would be more flexible/interesting.
The AD7780 uses quite a bit of logic from the generic ad_sigma_delta
driver.

> Thank you
> On Thu, Nov 8, 2018 at 12:46 PM Ardelean, Alexandru
> <alexandru...@analog.com> wrote:
> >
> > On Thu, 2018-11-08 at 12:04 -0200, Renato Lui Geh wrote:
> > > Hi,
> > >
> >
> > Hey,
> >
> > > On 11/06, Ardelean, Alexandru wrote:
> > > > For AD778x you could also add support for the GAIN & FILTER pins
> > > > via
> > > > GPIOs,
> > > > to control these settings. And then in the
> > > > ad7780_postprocess_sample()
> > > > function check if the GPIO settings (stored on a gpio_desc on
> > > > ad7780_state)
> > > > match the expected GAIN & FILTER bit settings ; if not return -EIO.

By the way: looking at this suggestion now, I'm not sure [now] if it's a
good idea to read GPIOs in the ad7780_postprocess_sample() function as that
could slow down the data-rate when reading from the ADC.
So, I would drop this idea.

[Jonathan Cameron]

On Wed, 14 Nov 2018 09:55:08 +0000
"Ardelean, Alexandru" <alexandru...@analog.com> wrote:

> On Tue, 2018-11-13 at 11:56 -0200, Giuliano Augusto Faulin Belinassi wrote:
> > Hello,
> > We have some doubts regarding the FILTER and GAIN pins. Should
> > these pins be set in userspace, and therefore there must be a
> > 'ad7780_write_raw' function? Or should we use the device tree with a
> > value set on initialization?
> >
>
> So, definitely add the code that initializes the pins via device-tree.
> Quite a few users of the driver may only want a single gain value for the
> entire life-cycle of the chip running ; and this can be neatly initialized
> from DT.

Actually I'll come down on the other side in that argument. They are both
better set from just userspace, not in DT. DT bindings for gain in particular
have always been controversial as it's is sometimes more of a policy decision
(and hence generally shouldn't be in DT) rather than one dictated by hardware.

However any defaults should then be conservative (minimum gain, widest filter)
so peoples till get a reasonable answer when not controlling these explicitly.

Now, if there is a 'strong' hardware reason for having a particular set of
value (typical one is the hardware is actually damaged if we go too far
out of range) then they can go in DT. Otherwise I would definitely prefer
userspace control only.

Should definitely avoid any actual hardware reads / writes as these may
well be on some gpio chip out on a slow bus.