Hi Drew,
Thanks for reporting this...
> -----Original Message-----
> From: Drew Fustini <drew@xxxxxxxx>
> Sent: Saturday, August 7, 2021 1:20 AM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx
> 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á
