APDS9960 with 'noints' fails to register

[mousethief]

Describe the bug

(firmware/boot/overlays/README line 710, Name: apds9960) documents a param 'noints' which 'disables the interrupt GPIO line'. Therefore in boot/config.txt I have a line 'dtoverlay=apds9960,noints' and on the command line I run 'echo apds9960 0x39 | sudo tee /sys/class/i2c-dev/i2c-1/device/new_device' which returns 'tee: /sys/....: Device or resource busy' and in dmesg 'i2c i2c-1: Failed to register i2c client apds9960 at 0x39 (-16)'. I note the overlay was added late 2019 (https://github.com/raspberrypi/linux/pull/3321) and was only tested with interrupt enabled. I note I have also tried another i2c sensor (bme680) with success, and do have to perform the write to /sys in order to enable it.

Steps to reproduce the behaviour

To a raspberry pi connect an apds9960 sensor to i2c. I use an Adafruit stemma qt breakout board connected to a Sparkfun i2c shim pressed on the appropriate pins. Add the line 'dtoverlay=apds9960,noints' to /boot/config.txt and reboot. At a shell prompt run 'echo apds9960 0x39 | sudo tee /sys/class/i2c-dev/i2c-1/device/new_device'. An error message should be printed and an another error should be written to the kernel message log.

Device (s)

Raspberry Pi Zero 2 W, Raspberry Pi 3 Mod. B, Raspberry Pi 3 Mod. B+

System

beata@raspberrypi:~$ cat /etc/rpi-issue 
Raspberry Pi reference 2023-12-11
Generated using pi-gen, https://github.com/RPi-Distro/pi-gen, 2acf7afcba7d11500313a7b93bb55a2aae20b2d6, stage2
beata@raspberrypi:~$ vcgencmd version
Oct 17 2023 15:42:39 
Copyright (c) 2012 Broadcom
version 30f0c5e4d076da3ab4f341d88e7d505760b93ad7 (clean) (release) (start)
beata@raspberrypi:~$ uname -a
Linux raspberrypi 6.1.0-rpi7-rpi-v8 #1 SMP PREEMPT Debian 1:6.1.63-1+rpt1 (2023-11-24) aarch64 GNU/Linux

Logs

[ 98.008072] i2c i2c-1: Failed to register i2c client apds9960 at 0x39 (-16) [ 488.166438] i2c i2c-1: Failed to register i2c client apds9960 at 0x39 (-16) [ 2160.762815] i2c i2c-1: Failed to register i2c client apds9960 at 0x39 (-16)

Additional context

It could be that the kernel driver never supported 'noints' to begin with. I would prefer this functionality be added to the driver if this is the case, but updating documentation to remove this param is also a valid solution to this bug report.

[6by9]

Why are you combining both dtoverlay=apds9960,noints and echo apds9960 0x39 | sudo tee /sys/class/i2c-dev/i2c-1/device/new_device?

dtoverlay configures and loads a device at boot time. i2c's new_device is for run time loading of I2C drivers.

dtoverlay=apds9960,noints has already configured and loaded the driver on address 0x39. You could confirm this via sudo i2cdetect -y 1 reporting the address as UU. Trying to initialise a new device on address 0x39 fails with -16 aka -EBUSY as the address is already in use.

[mousethief]

A ha! I was combining the two because it appeared to not work at all with just one. BUT! Simplified. 'dtoverlay=apds9960,noints' in /boot/config.txt (reboot) $ dmesg | grep apds [ 13.996264] apds9960 1-0039: no valid irq defined [ 13.996582] apds9960: probe of 1-0039 failed with error -22

[6by9]

It's an unmodified upstream driver, but the driver doesn't match the DT binding.

Bindings - https://github.com/raspberrypi/linux/blob/rpi-6.6.y/Documentation/devicetree/bindings/iio/light/avago%2Capds9960.yaml

required:
  - compatible
  - reg

Driver https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/iio/light/apds9960.c#L1044

    if (client->irq <= 0) {
        dev_err(&client->dev, "no valid irq defined\n");
        ret = -EINVAL;
        goto error_power_down;
    }

which means the interrupt is required.

The overlay was added in commit c74afe88d9c5a2c1719e9df8cd1491a6ddb92760, but it looks like it is just wrong that there is an option for running it without interrupts connected up. I suspect we'd accept a PR adding such a mode (although ideally it should be sent to mainline first), but otherwise I think it's going to be remove that option from the overlay and README.

[pelwell]

In case anyone thinks it's simply a matter of removing the error return, the missing interrupt would need to be replaced with a timer that polls for events - it could just be a wrapper around the existing interrupt handler. One would probably also want a DT property to control how frequently it polls.