iio: temperature: ltc2983 probe failure

iio: temperature: ltc2983 probe failure

[Drew Fustini]

Hello - thank you for developing the LTC2983 driver. I am using that
part in a project connected a Zynq-7000 SoC. I first tested using spidev
and I was able to do channel assignment, start conversion and then read
the conversion result. In this test, it was channel 10 configured as a
single-ended direct ADC.

I next replaced spidev with this device tree node inside the spi0 node:

        sensor_ltc2983: ltc2983@0 {
                compatible = "adi,ltc2983";
                reg = <0>;
                spi-max-frequency = <4000000>;
                #address-cells = <1>;
                #size-cells = <0>;
                /* ADC0_DATA_RDY (H11) is connected to MIO GPIO[2]
                 * MIO GPIO[0] is GPIO 54 so MIO GPIO[2] is GPIO 56
		 * /
                interrupts = <56 IRQ_TYPE_EDGE_RISING>;
                interrupt-parent = <&gpio0>;

                adc10: adc@10 {
                        reg = <10>; /* channel 10 */
                        adi,sensor-type = <30>; /* direct adc */
                        adi,single-ended;
                };
        };

The ltc2983 driver probe then runs at boot, but the probe would always
fail. The cause is that ltc2983_setup() would return -ETIMEDOUT. From
drivers/iio/temperature/ltc2983.c:

1363 static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
1364 {
1365         u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0;
1366         int ret;
1367         unsigned long time;
1368
1369         /* make sure the device is up */
1370         time = wait_for_completion_timeout(&st->completion,
1371                                             msecs_to_jiffies(250));
1372
1373         if (!time) {
1374                 dev_err(&st->spi->dev, "Device startup timed out\n");
1375                 return -ETIMEDOUT;
1376         }

I found that if I comment out the "return -ETIMEDOUT" on line 1375 then
the drivers work fine. I can read from channel 10 okay:

  # cat /sys/devices/iio:device1/in_voltage0_raw
  4925

I have a logic analyzer connected to test points on all the traces going
to the LTC2983. At power up, the INTERRUPT line is low on and then goes
high after about 100 ms and stays high.  I believe that this is expected
per the datasheet [1] on page 9:

  INTERRUPT (Pin 37): This pin outputs a LOW when the device is busy 
  either during start-up or while a conversion cycle is in progress. 
  This pin goes HIGH at the conclusion of the start-up state or 
  conversion cycle.

From page 16:

  Start-Up. After power is applied to the LTC2983 (V DD > 2.6V), there
  is a 200ms wake up period. During this time, the LDO, charge pump, 
  ADCs, and reference are powered up and the internal RAM is initialized
  Once start-up is complete, the INTERRUPT pin goes HIGH and the command
  status register will return a value of 0x40 (Start bit=0, Done bit=1)
  when read.

Why does ltc2983_setup() call wait_for_completion_timeout()?

I do not think there would be anyway for wait_for_completion_timeout()
to succeed. I don't see a reason to expect that ltc2983_irq_handler()
would run. The handler is installed just prior to ltc2983_setup(). 

1468 static int ltc2983_probe(struct spi_device *spi)
1469 {
<snip>
1495         /*
1496          * let's request the irq now so it is used to sync the device
1497          * startup in ltc2983_setup()
1498          */
1499         ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
1500                                IRQF_TRIGGER_RISING, name, st);
1501         if (ret) {
1502                 dev_err(&spi->dev, "failed to request an irq, %d", ret);
1503                 return ret;
1504         }
1505
1506         ret = ltc2983_setup(st, true);
1507         if (ret)
1508                 return ret;

Why does the probe assume that that there would be a low to high
transistion on the INTERRUPT pin after calling devm_request_irq() and
before calling ltc2983_setup()?

I believe the transistion happens once ~200ms after power on (I see it
happen within 100 ms on logic anaylzer) and when conversion completes.
When I read in_voltage0_raw, I can see on the logic analzyer that the
INTERRUPT goes low when the conversion begins. It later goes high again
and the conversion result is read ok. This works as expected.

I could see using wait_for_completion_timeout() as way to sleep until
the chip startup period is over. However, -ETIMEDOUT should not be
returned in that case as it causes the probe to fail, even though the
chip is actually working ok.

I hope I have been able to explain what I am experiencing. I do have
detailed debug logging and logic analyzer catpures that I can share.
I wanted to first see if my concerns over wait_for_completion_timeout()
make sense or not.

Thank you,
Drew

RE: iio: temperature: ltc2983 probe failure

[Sa, Nuno]

Hi Drew,

Thanks for reporting this...

> -----Original Message-----
> From: Drew Fustini <drew@pdp7.com>
> Sent: Saturday, August 7, 2021 1:20 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org
> Subject: iio: temperature: ltc2983 probe failure
> 
> 
> Hello - thank you for developing the LTC2983 driver. I am using that
> part in a project connected a Zynq-7000 SoC. I first tested using spidev
> and I was able to do channel assignment, start conversion and then
> read
> the conversion result. In this test, it was channel 10 configured as a
> single-ended direct ADC.
> 
> I next replaced spidev with this device tree node inside the spi0 node:
> 
>         sensor_ltc2983: ltc2983@0 {
>                 compatible = "adi,ltc2983";
>                 reg = <0>;
>                 spi-max-frequency = <4000000>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 /* ADC0_DATA_RDY (H11) is connected to MIO GPIO[2]
>                  * MIO GPIO[0] is GPIO 54 so MIO GPIO[2] is GPIO 56
> 		 * /
>                 interrupts = <56 IRQ_TYPE_EDGE_RISING>;
>                 interrupt-parent = <&gpio0>;
> 
>                 adc10: adc@10 {
>                         reg = <10>; /* channel 10 */
>                         adi,sensor-type = <30>; /* direct adc */
>                         adi,single-ended;
>                 };
>         };
> 
> The ltc2983 driver probe then runs at boot, but the probe would
> always
> fail. The cause is that ltc2983_setup() would return -ETIMEDOUT. From
> drivers/iio/temperature/ltc2983.c:
> 
> 1363 static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
> 1364 {
> 1365         u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0;
> 1366         int ret;
> 1367         unsigned long time;
> 1368
> 1369         /* make sure the device is up */
> 1370         time = wait_for_completion_timeout(&st->completion,
> 1371                                             msecs_to_jiffies(250));
> 1372
> 1373         if (!time) {
> 1374                 dev_err(&st->spi->dev, "Device startup timed out\n");
> 1375                 return -ETIMEDOUT;
> 1376         }
> 
> I found that if I comment out the "return -ETIMEDOUT" on line 1375
> then
> the drivers work fine. I can read from channel 10 okay:
> 
>   # cat /sys/devices/iio:device1/in_voltage0_raw
>   4925
> 
> I have a logic analyzer connected to test points on all the traces going
> to the LTC2983. At power up, the INTERRUPT line is low on and then
> goes
> high after about 100 ms and stays high.  I believe that this is expected
> per the datasheet [1] on page 9:
> 
>   INTERRUPT (Pin 37): This pin outputs a LOW when the device is busy
>   either during start-up or while a conversion cycle is in progress.
>   This pin goes HIGH at the conclusion of the start-up state or
>   conversion cycle.
> 
> From page 16:
> 
>   Start-Up. After power is applied to the LTC2983 (V DD > 2.6V), there
>   is a 200ms wake up period. During this time, the LDO, charge pump,
>   ADCs, and reference are powered up and the internal RAM is
> initialized
>   Once start-up is complete, the INTERRUPT pin goes HIGH and the
> command
>   status register will return a value of 0x40 (Start bit=0, Done bit=1)
>   when read.
> 
> Why does ltc2983_setup() call wait_for_completion_timeout()?
> 
> I do not think there would be anyway for
> wait_for_completion_timeout()
> to succeed. I don't see a reason to expect that ltc2983_irq_handler()
> would run. The handler is installed just prior to ltc2983_setup().
> 
> 1468 static int ltc2983_probe(struct spi_device *spi)
> 1469 {
> <snip>
> 1495         /*
> 1496          * let's request the irq now so it is used to sync the device
> 1497          * startup in ltc2983_setup()
> 1498          */
> 1499         ret = devm_request_irq(&spi->dev, spi->irq,
> ltc2983_irq_handler,
> 1500                                IRQF_TRIGGER_RISING, name, st);
> 1501         if (ret) {
> 1502                 dev_err(&spi->dev, "failed to request an irq, %d", ret);
> 1503                 return ret;
> 1504         }
> 1505
> 1506         ret = ltc2983_setup(st, true);
> 1507         if (ret)
> 1508                 return ret;
> 
> Why does the probe assume that that there would be a low to high
> transistion on the INTERRUPT pin after calling devm_request_irq() and
> before calling ltc2983_setup()?

That's a good question. I honestly do to not know why I assumed that
just by requesting the irq before setting up the device, we would get a
low to high transition and enter the handler. And by chance things worked
when I was developing the driver (on a rpi) leaving this unnoticed...

> I believe the transistion happens once ~200ms after power on (I see it
> happen within 100 ms on logic anaylzer) and when conversion
> completes.
> When I read in_voltage0_raw, I can see on the logic analzyer that the
> INTERRUPT goes low when the conversion begins. It later goes high
> again
> and the conversion result is read ok. This works as expected.
> 
> I could see using wait_for_completion_timeout() as way to sleep until
> the chip startup period is over. However, -ETIMEDOUT should not be
> returned in that case as it causes the probe to fail, even though the
> chip is actually working ok.

Using ' wait_for_completion_timeout() ' just for a sleep is also not neat :).
For that we would  replace the whole thing with a 'msleep(250)'.
However, my goal was really to make sure the start up sequence went
fine and the device is in a sane state. And this mechanism is actually ok
when the device is coming out of sleep (on the resume function). Hence,
what we could do here is to poll the status register (with the same
250ms timeout) until we get 0x40. This should work both for probe and
resume... Does this makes sense to you?

Once again, thanks for reporting,
- Nuno Sá