On Fri, 11 Aug 2023 10:08:00 +0100
Phil Elwell <phil@xxxxxxxxxxxxxxx> wrote:
> Hi Jonathan and linux-iio,
>
> I've run into a problem running a BMP180 over I2C - it's on a
> Raspberry Pi, but I don't think that's particularly relevant. As it's
> a BMP180 it has no interrupt signal, but it apparently shares an ID
> with the BMP085 which does. As a result, the driver and bindings
> sensibly treat the IRQ as optional.
>
> The function bmp280_common_probe contains the following fragment:
>
> /*
> * Attempt to grab an optional EOC IRQ - only the BMP085 has this
> * however as it happens, the BMP085 shares the chip ID of BMP180
> * so we look for an IRQ if we have that.
> */
> if (irq > 0 || (chip_id == BMP180_CHIP_ID)) {
> ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
> if (ret)
> return ret;
> }
>
> where the irq is passed in from the I2C or SPI framework. Inside
> bmp085_fetch_eoc_irq it immediately does:
>
> irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
>
> irq_get_irq_data converts irq to an irqd, returning either a pointer
> to the irq_data or a NULL. irq_get_trigger_type essentially just
> indirects through the pointer to retrieve the irq_common_data pointer.
>
> There is nothing to prevent irq from being 0, and in the case of a
> BMP180 that is the expected value. This is where it gets strange: on
> an ARCH=arm build I'm getting a valid-looking irq_data pointer back
> for IRQ 0, but on ARCH=arm64 I get the more obvious NULL pointer,
> leading to an exception.
>
> I'm hesitant to suggest there's a bug in such old code, but I don't
> understand why the condition in the probe function isn't:
>
> if (irq > 0 && (chip_id == BMP180_CHIP_ID))
Agreed. That was my thought as well when I read the comment you quote
above. Worst that happens is we don't provide an optional interrupt,
so this is a fairly safe fix even if the intent was something different.
Mind you, it's also odd if someone provides an irq to a BMP180.
Jonathan
>
> Do you have any thoughts?
>
> Many thanks,
>
> Phil Elwell
