| Message ID | 20210719112156.27087-4-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: bmc150: Add support for INT2 and BMC156 | expand |
On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > Some Bosch accelerometers have two interrupt pins (INT1 and INT2). > At the moment, the driver uses only the first one, which is fine for > most situations. However, some boards might only have INT2 connected > for some reason. > > Add the necessary bits and configuration to set up INT2. Then try > to detect this situation at least for device tree setups by checking > if the first interrupt (the one picked by the I2C/SPI core) is actually > named "INT2" using the interrupt-names property. > > of_irq_get_byname() returns either 0 or some error code in case > the driver probed without device tree, so in all other cases we fall > back to configuring INT1 as before. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > #include <linux/acpi.h> > +#include <linux/of_irq.h> (...) > + irq_info = bmc150_accel_interrupts_int1; > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > + irq_info = bmc150_accel_interrupts_int2; This looks a bit DT-specific, but I don't see that ACPI has named IRQs so I don't know what to do about it either. What does platform_get_irq_byname() do on ACPI systems? If there is no obvious fix I would leave it like this until the first ACPI used needing this comes along, but I think maybe Andy has suggestions. Yours, Linus Walleij
On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > Some Bosch accelerometers have two interrupt pins (INT1 and INT2). > > At the moment, the driver uses only the first one, which is fine for > > most situations. However, some boards might only have INT2 connected > > for some reason. > > > > Add the necessary bits and configuration to set up INT2. Then try > > to detect this situation at least for device tree setups by checking > > if the first interrupt (the one picked by the I2C/SPI core) is actually > > named "INT2" using the interrupt-names property. > > > > of_irq_get_byname() returns either 0 or some error code in case > > the driver probed without device tree, so in all other cases we fall > > back to configuring INT1 as before. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > #include <linux/acpi.h> > > +#include <linux/of_irq.h> > (...) > > + irq_info = bmc150_accel_interrupts_int1; > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > + irq_info = bmc150_accel_interrupts_int2; > > This looks a bit DT-specific, but I don't see that ACPI has > named IRQs so I don't know what to do about it either. Yeah, we only have so far the (de facto) established way of naming GPIO based IRQs, and not IOxAPIC ones. > What does platform_get_irq_byname() do on ACPI systems? See above. > If there is no obvious fix I would leave it like this until the > first ACPI used needing this comes along, but I think maybe > Andy has suggestions. The platform_get_irq_byname() should do something similar that has been done in platform_get_irq() WRT ACPI. Here for sure the platform_get_irq_byname() or its optional variant should be used.
On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > Some Bosch accelerometers have two interrupt pins (INT1 and INT2). > > > At the moment, the driver uses only the first one, which is fine for > > > most situations. However, some boards might only have INT2 connected > > > for some reason. > > > > > > Add the necessary bits and configuration to set up INT2. Then try > > > to detect this situation at least for device tree setups by checking > > > if the first interrupt (the one picked by the I2C/SPI core) is actually > > > named "INT2" using the interrupt-names property. > > > > > > of_irq_get_byname() returns either 0 or some error code in case > > > the driver probed without device tree, so in all other cases we fall > > > back to configuring INT1 as before. > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > > > #include <linux/acpi.h> > > > +#include <linux/of_irq.h> > > (...) > > > + irq_info = bmc150_accel_interrupts_int1; > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > + irq_info = bmc150_accel_interrupts_int2; > > > > This looks a bit DT-specific, but I don't see that ACPI has > > named IRQs so I don't know what to do about it either. > > Yeah, we only have so far the (de facto) established way of naming > GPIO based IRQs, and not IOxAPIC ones. > > > What does platform_get_irq_byname() do on ACPI systems? > > See above. > > > If there is no obvious fix I would leave it like this until the > > first ACPI used needing this comes along, but I think maybe > > Andy has suggestions. > > The platform_get_irq_byname() should do something similar that has > been done in platform_get_irq() WRT ACPI. > Here for sure the platform_get_irq_byname() or its optional variant > should be used. > I don't think there is a platform device here, we only have the i2c_client or spi_device. That's why I didn't use platform_get_irq_byname(). :) Is there something equivalent for I2C/SPI drivers? Thanks! Stephan
On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: ... > > > > #include <linux/acpi.h> > > > > +#include <linux/of_irq.h> > > > (...) > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > named IRQs so I don't know what to do about it either. > > > > Yeah, we only have so far the (de facto) established way of naming > > GPIO based IRQs, and not IOxAPIC ones. > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > See above. > > > > > If there is no obvious fix I would leave it like this until the > > > first ACPI used needing this comes along, but I think maybe > > > Andy has suggestions. > > > > The platform_get_irq_byname() should do something similar that has > > been done in platform_get_irq() WRT ACPI. > > Here for sure the platform_get_irq_byname() or its optional variant > > should be used. > > I don't think there is a platform device here, we only have the > i2c_client or spi_device. That's why I didn't use > platform_get_irq_byname(). :) > > Is there something equivalent for I2C/SPI drivers? Not yet. You probably need to supply some code there to allow multi-IRQ devices (in resource provider agnostic way). You need to provide fwnode_get_irq_byname() to be similar with https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 Then use it in the drivers. And/or integrate into frameworks somehow (something in between the lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461).
On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > ... > > > > > > #include <linux/acpi.h> > > > > > +#include <linux/of_irq.h> > > > > (...) > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > named IRQs so I don't know what to do about it either. > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > See above. > > > > > > > If there is no obvious fix I would leave it like this until the > > > > first ACPI used needing this comes along, but I think maybe > > > > Andy has suggestions. > > > > > > The platform_get_irq_byname() should do something similar that has > > > been done in platform_get_irq() WRT ACPI. > > > Here for sure the platform_get_irq_byname() or its optional variant > > > should be used. > > > > I don't think there is a platform device here, we only have the > > i2c_client or spi_device. That's why I didn't use > > platform_get_irq_byname(). :) > > > > Is there something equivalent for I2C/SPI drivers? > > Not yet. You probably need to supply some code there to allow > multi-IRQ devices (in resource provider agnostic way). > > You need to provide fwnode_get_irq_byname() to be similar with > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > Then use it in the drivers. > > And/or integrate into frameworks somehow (something in between the > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > Well, I don't think anyone has an ACPI use case for this right now so it's probably better if this is done by someone who actually needs this and can test it somewhere. :) I actually just "copied" this approach from some other IIO drivers where this is done similarly (and additionally checked the source code to make sure this won't break anything for ACPI platforms). Stephan
On Mon, Jul 19, 2021 at 8:29 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: ... > > > > > > #include <linux/acpi.h> > > > > > > +#include <linux/of_irq.h> > > > > > (...) > > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > > named IRQs so I don't know what to do about it either. > > > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > > > See above. > > > > > > > > > If there is no obvious fix I would leave it like this until the > > > > > first ACPI used needing this comes along, but I think maybe > > > > > Andy has suggestions. > > > > > > > > The platform_get_irq_byname() should do something similar that has > > > > been done in platform_get_irq() WRT ACPI. > > > > Here for sure the platform_get_irq_byname() or its optional variant > > > > should be used. > > > > > > I don't think there is a platform device here, we only have the > > > i2c_client or spi_device. That's why I didn't use > > > platform_get_irq_byname(). :) > > > > > > Is there something equivalent for I2C/SPI drivers? > > > > Not yet. You probably need to supply some code there to allow > > multi-IRQ devices (in resource provider agnostic way). > > > > You need to provide fwnode_get_irq_byname() to be similar with > > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > > > Then use it in the drivers. > > > > And/or integrate into frameworks somehow (something in between the > > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > > > > Well, I don't think anyone has an ACPI use case for this right now so > it's probably better if this is done by someone who actually needs this > and can test it somewhere. :) > > I actually just "copied" this approach from some other IIO drivers where > this is done similarly (and additionally checked the source code to make > sure this won't break anything for ACPI platforms). I see in today's Linux Next snapshot: drivers/iio/accel/fxls8962af-core.c:774: irq = of_irq_get_byname(of_node, "INT2"); drivers/iio/accel/mma8452.c:1616: irq2 = of_irq_get_byname(client->dev.of_node, "INT2"); drivers/iio/gyro/fxas21002c_core.c:834: irq1 = of_irq_get_byname(np, "INT1"); drivers/iio/imu/adis16480.c:1265: irq = of_irq_get_byname(of_node, adis16480_int_pin_names[i]); drivers/iio/imu/bmi160/bmi160_core.c:655: irq = of_irq_get_byname(of_node, "INT1"); drivers/iio/imu/bmi160/bmi160_core.c:661: irq = of_irq_get_byname(of_node, "INT2"); I believe we may stop distributing this and actually start using a common API. I don't want this to be spread again over all IIO. Btw, I have LSM9DS0, which supports two INT pins for IMU and currently it uses hard coded pin mapping. Side note to Jonathan, I believe the below may be (lazily) converted to fwnode / device properties APIs. drivers/iio/adc/ab8500-gpadc.c:1041: nchans = of_get_available_child_count(np); drivers/iio/adc/ad7124.c:747: st->num_channels = of_get_available_child_count(np); drivers/iio/adc/berlin2-adc.c:287: struct device_node *parent_np = of_get_parent(pdev->dev.of_node); drivers/iio/adc/qcom-pm8xxx-xoadc.c:830: adc->nchans = of_get_available_child_count(np); drivers/iio/adc/qcom-spmi-adc5.c:650: channel_name = of_get_property(node, drivers/iio/adc/qcom-spmi-adc5.c:813: adc->nchannels = of_get_available_child_count(node); drivers/iio/adc/qcom-spmi-vadc.c:743: vadc->nchannels = of_get_available_child_count(node); drivers/iio/adc/stm32-adc.c:1630:static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev) drivers/iio/adc/stm32-adc.c:1935: ret = stm32_adc_of_get_resolution(indio_dev); drivers/iio/adc/xilinx-xadc-core.c:1248: chan_node = of_get_child_by_name(np, "xlnx,channels"); drivers/iio/imu/adis16480.c:1293:static int adis16480_of_get_ext_clk_pin(struct adis16480 *st, drivers/iio/imu/adis16480.c:1328: pin = adis16480_of_get_ext_clk_pin(st, of_node); drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c:68: mux_node = of_get_child_by_name(dev->of_node, "i2c-gate") ; drivers/iio/industrialio-core.c:1877: label = of_get_property(indio_dev->dev.of_node, "label", NULL); drivers/iio/inkern.c:228: if (np && !of_get_property(np, "io-channel-ranges", NULL)) drivers/iio/temperature/ltc2983.c:1275: st->num_channels = of_get_available_child_count(dev->of_node);
On Mon, Jul 19, 2021 at 09:05:48PM +0300, Andy Shevchenko wrote: > On Mon, Jul 19, 2021 at 8:29 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > > > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > ... > > > > > > > > #include <linux/acpi.h> > > > > > > > +#include <linux/of_irq.h> > > > > > > (...) > > > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > > > named IRQs so I don't know what to do about it either. > > > > > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > > > > > See above. > > > > > > > > > > > If there is no obvious fix I would leave it like this until the > > > > > > first ACPI used needing this comes along, but I think maybe > > > > > > Andy has suggestions. > > > > > > > > > > The platform_get_irq_byname() should do something similar that has > > > > > been done in platform_get_irq() WRT ACPI. > > > > > Here for sure the platform_get_irq_byname() or its optional variant > > > > > should be used. > > > > > > > > I don't think there is a platform device here, we only have the > > > > i2c_client or spi_device. That's why I didn't use > > > > platform_get_irq_byname(). :) > > > > > > > > Is there something equivalent for I2C/SPI drivers? > > > > > > Not yet. You probably need to supply some code there to allow > > > multi-IRQ devices (in resource provider agnostic way). > > > > > > You need to provide fwnode_get_irq_byname() to be similar with > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > > > > > Then use it in the drivers. > > > > > > And/or integrate into frameworks somehow (something in between the > > > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > > > > > > > Well, I don't think anyone has an ACPI use case for this right now so > > it's probably better if this is done by someone who actually needs this > > and can test it somewhere. :) > > > > I actually just "copied" this approach from some other IIO drivers where > > this is done similarly (and additionally checked the source code to make > > sure this won't break anything for ACPI platforms). > > I see in today's Linux Next snapshot: > > drivers/iio/accel/fxls8962af-core.c:774: irq = > of_irq_get_byname(of_node, "INT2"); > drivers/iio/accel/mma8452.c:1616: irq2 = > of_irq_get_byname(client->dev.of_node, "INT2"); > drivers/iio/gyro/fxas21002c_core.c:834: irq1 = of_irq_get_byname(np, "INT1"); > drivers/iio/imu/adis16480.c:1265: irq = > of_irq_get_byname(of_node, adis16480_int_pin_names[i]); > drivers/iio/imu/bmi160/bmi160_core.c:655: irq = > of_irq_get_byname(of_node, "INT1"); > drivers/iio/imu/bmi160/bmi160_core.c:661: irq = > of_irq_get_byname(of_node, "INT2"); > > I believe we may stop distributing this and actually start using a > common API. I don't want this to be spread again over all IIO. Btw, I > have LSM9DS0, which supports two INT pins for IMU and currently it > uses hard coded pin mapping. > Hm, I'm not quite sure how to implement this. Could you prepare a patch that would implement such a common API? I would be happy to test it for the device tree and make use of it in this patch. To be honest, I mainly implemented support for the interrupt-names because Jonathan mentioned this would be nice to have [1] and it kind of fit well together with the BMC156 patch that needs the INT2 support. I actually just use the if (data->type == BOSCH_BMC156) part from PATCH 4/4 which does not depend on of_irq_get_byname(). Thanks, Stephan [1]: https://lore.kernel.org/linux-iio/20210611185941.3487efc6@jic23-huawei/
On Mon, Jul 19, 2021 at 9:37 PM Stephan Gerhold <stephan@gerhold.net> wrote: > On Mon, Jul 19, 2021 at 09:05:48PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 19, 2021 at 8:29 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > > > > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: ... > > > > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > > > > named IRQs so I don't know what to do about it either. > > > > > > > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > > > > > > > See above. > > > > > > > > > > > > > If there is no obvious fix I would leave it like this until the > > > > > > > first ACPI used needing this comes along, but I think maybe > > > > > > > Andy has suggestions. > > > > > > > > > > > > The platform_get_irq_byname() should do something similar that has > > > > > > been done in platform_get_irq() WRT ACPI. > > > > > > Here for sure the platform_get_irq_byname() or its optional variant > > > > > > should be used. > > > > > > > > > > I don't think there is a platform device here, we only have the > > > > > i2c_client or spi_device. That's why I didn't use > > > > > platform_get_irq_byname(). :) > > > > > > > > > > Is there something equivalent for I2C/SPI drivers? > > > > > > > > Not yet. You probably need to supply some code there to allow > > > > multi-IRQ devices (in resource provider agnostic way). > > > > > > > > You need to provide fwnode_get_irq_byname() to be similar with > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > > > > > > > Then use it in the drivers. > > > > > > > > And/or integrate into frameworks somehow (something in between the > > > > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > > > > > > > > > > Well, I don't think anyone has an ACPI use case for this right now so > > > it's probably better if this is done by someone who actually needs this > > > and can test it somewhere. :) > > > > > > I actually just "copied" this approach from some other IIO drivers where > > > this is done similarly (and additionally checked the source code to make > > > sure this won't break anything for ACPI platforms). > > > > I see in today's Linux Next snapshot: > > > > drivers/iio/accel/fxls8962af-core.c:774: irq = > > of_irq_get_byname(of_node, "INT2"); > > drivers/iio/accel/mma8452.c:1616: irq2 = > > of_irq_get_byname(client->dev.of_node, "INT2"); > > drivers/iio/gyro/fxas21002c_core.c:834: irq1 = of_irq_get_byname(np, "INT1"); > > drivers/iio/imu/adis16480.c:1265: irq = > > of_irq_get_byname(of_node, adis16480_int_pin_names[i]); > > drivers/iio/imu/bmi160/bmi160_core.c:655: irq = > > of_irq_get_byname(of_node, "INT1"); > > drivers/iio/imu/bmi160/bmi160_core.c:661: irq = > > of_irq_get_byname(of_node, "INT2"); > > > > I believe we may stop distributing this and actually start using a > > common API. I don't want this to be spread again over all IIO. Btw, I > > have LSM9DS0, which supports two INT pins for IMU and currently it > > uses hard coded pin mapping. > > > > Hm, I'm not quite sure how to implement this. Could you prepare a patch > that would implement such a common API? I would be happy to test it for > the device tree and make use of it in this patch. Unfortunately I have no time to fulfil the required process. The idea in general is like this: if (is_of_node(...)) return of_irq_get_byname(...); if (is_acpi_node(...)) return acpi_gpio_irq_get_byname(...); Everything else is quite similar to fwnode_irq_get(). > To be honest, I mainly implemented support for the interrupt-names > because Jonathan mentioned this would be nice to have [1] and it kind of > fit well together with the BMC156 patch that needs the INT2 support. > I actually just use the if (data->type == BOSCH_BMC156) part from > PATCH 4/4 which does not depend on of_irq_get_byname(). Then I leave it to Jonathan and other maintainers. > [1]: https://lore.kernel.org/linux-iio/20210611185941.3487efc6@jic23-huawei/
On Tue, 20 Jul 2021 18:04:22 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 19, 2021 at 9:37 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Jul 19, 2021 at 09:05:48PM +0300, Andy Shevchenko wrote: > > > On Mon, Jul 19, 2021 at 8:29 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > > > > > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > > > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > ... > > > > > > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > > > > > named IRQs so I don't know what to do about it either. > > > > > > > > > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > > > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > > > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > > > > > > > > > See above. > > > > > > > > > > > > > > > If there is no obvious fix I would leave it like this until the > > > > > > > > first ACPI used needing this comes along, but I think maybe > > > > > > > > Andy has suggestions. > > > > > > > > > > > > > > The platform_get_irq_byname() should do something similar that has > > > > > > > been done in platform_get_irq() WRT ACPI. > > > > > > > Here for sure the platform_get_irq_byname() or its optional variant > > > > > > > should be used. > > > > > > > > > > > > I don't think there is a platform device here, we only have the > > > > > > i2c_client or spi_device. That's why I didn't use > > > > > > platform_get_irq_byname(). :) > > > > > > > > > > > > Is there something equivalent for I2C/SPI drivers? > > > > > > > > > > Not yet. You probably need to supply some code there to allow > > > > > multi-IRQ devices (in resource provider agnostic way). > > > > > > > > > > You need to provide fwnode_get_irq_byname() to be similar with > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > > > > > > > > > Then use it in the drivers. > > > > > > > > > > And/or integrate into frameworks somehow (something in between the > > > > > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > > > > > > > > > > > > > Well, I don't think anyone has an ACPI use case for this right now so > > > > it's probably better if this is done by someone who actually needs this > > > > and can test it somewhere. :) > > > > > > > > I actually just "copied" this approach from some other IIO drivers where > > > > this is done similarly (and additionally checked the source code to make > > > > sure this won't break anything for ACPI platforms). > > > > > > I see in today's Linux Next snapshot: > > > > > > drivers/iio/accel/fxls8962af-core.c:774: irq = > > > of_irq_get_byname(of_node, "INT2"); > > > drivers/iio/accel/mma8452.c:1616: irq2 = > > > of_irq_get_byname(client->dev.of_node, "INT2"); > > > drivers/iio/gyro/fxas21002c_core.c:834: irq1 = of_irq_get_byname(np, "INT1"); > > > drivers/iio/imu/adis16480.c:1265: irq = > > > of_irq_get_byname(of_node, adis16480_int_pin_names[i]); > > > drivers/iio/imu/bmi160/bmi160_core.c:655: irq = > > > of_irq_get_byname(of_node, "INT1"); > > > drivers/iio/imu/bmi160/bmi160_core.c:661: irq = > > > of_irq_get_byname(of_node, "INT2"); > > > > > > I believe we may stop distributing this and actually start using a > > > common API. I don't want this to be spread again over all IIO. Btw, I > > > have LSM9DS0, which supports two INT pins for IMU and currently it > > > uses hard coded pin mapping. > > > > > > > Hm, I'm not quite sure how to implement this. Could you prepare a patch > > that would implement such a common API? I would be happy to test it for > > the device tree and make use of it in this patch. > > Unfortunately I have no time to fulfil the required process. The idea > in general is like this: > > if (is_of_node(...)) > return of_irq_get_byname(...); > if (is_acpi_node(...)) > return acpi_gpio_irq_get_byname(...); > > Everything else is quite similar to fwnode_irq_get(). > > > To be honest, I mainly implemented support for the interrupt-names > > because Jonathan mentioned this would be nice to have [1] and it kind of > > fit well together with the BMC156 patch that needs the INT2 support. > > I actually just use the if (data->type == BOSCH_BMC156) part from > > PATCH 4/4 which does not depend on of_irq_get_byname(). > > Then I leave it to Jonathan and other maintainers. I'd be rather nervous about this one myself unless I have a test setup where I can poke all the paths. My current qemu hack setup doesn't do full enough ACPI so whilst I'd take a look at this myself it might take me a little while to hack in the ACPI tables needed to bring up a suitable bus on that device. I'll get to it if no one else does, but I'm not keen to block any drivers that just use the of route in the meantime. Should be easier to do a sweep of the ones Andy has highlighted + this one when we have the support ready. The patch looks fine to me otherwise. Thanks, Jonathan > > > [1]: https://lore.kernel.org/linux-iio/20210611185941.3487efc6@jic23-huawei/ >
On Mon, 19 Jul 2021 21:05:48 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 19, 2021 at 8:29 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Jul 19, 2021 at 07:19:05PM +0300, Andy Shevchenko wrote: > > > On Mon, Jul 19, 2021 at 6:11 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Jul 19, 2021 at 06:01:01PM +0300, Andy Shevchenko wrote: > > > > > On Mon, Jul 19, 2021 at 5:07 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Mon, Jul 19, 2021 at 1:26 PM Stephan Gerhold <stephan@gerhold.net> wrote: > > ... > > > > > > > > #include <linux/acpi.h> > > > > > > > +#include <linux/of_irq.h> > > > > > > (...) > > > > > > > + irq_info = bmc150_accel_interrupts_int1; > > > > > > > + if (irq == of_irq_get_byname(dev->of_node, "INT2")) > > > > > > > + irq_info = bmc150_accel_interrupts_int2; > > > > > > > > > > > > This looks a bit DT-specific, but I don't see that ACPI has > > > > > > named IRQs so I don't know what to do about it either. > > > > > > > > > > Yeah, we only have so far the (de facto) established way of naming > > > > > GPIO based IRQs, and not IOxAPIC ones. > > > > > > > > > > > What does platform_get_irq_byname() do on ACPI systems? > > > > > > > > > > See above. > > > > > > > > > > > If there is no obvious fix I would leave it like this until the > > > > > > first ACPI used needing this comes along, but I think maybe > > > > > > Andy has suggestions. > > > > > > > > > > The platform_get_irq_byname() should do something similar that has > > > > > been done in platform_get_irq() WRT ACPI. > > > > > Here for sure the platform_get_irq_byname() or its optional variant > > > > > should be used. > > > > > > > > I don't think there is a platform device here, we only have the > > > > i2c_client or spi_device. That's why I didn't use > > > > platform_get_irq_byname(). :) > > > > > > > > Is there something equivalent for I2C/SPI drivers? > > > > > > Not yet. You probably need to supply some code there to allow > > > multi-IRQ devices (in resource provider agnostic way). > > > > > > You need to provide fwnode_get_irq_byname() to be similar with > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1010 > > > > > > Then use it in the drivers. > > > > > > And/or integrate into frameworks somehow (something in between the > > > lines: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L461). > > > > > > > Well, I don't think anyone has an ACPI use case for this right now so > > it's probably better if this is done by someone who actually needs this > > and can test it somewhere. :) > > > > I actually just "copied" this approach from some other IIO drivers where > > this is done similarly (and additionally checked the source code to make > > sure this won't break anything for ACPI platforms). > > I see in today's Linux Next snapshot: > > drivers/iio/accel/fxls8962af-core.c:774: irq = > of_irq_get_byname(of_node, "INT2"); > drivers/iio/accel/mma8452.c:1616: irq2 = > of_irq_get_byname(client->dev.of_node, "INT2"); > drivers/iio/gyro/fxas21002c_core.c:834: irq1 = of_irq_get_byname(np, "INT1"); > drivers/iio/imu/adis16480.c:1265: irq = > of_irq_get_byname(of_node, adis16480_int_pin_names[i]); > drivers/iio/imu/bmi160/bmi160_core.c:655: irq = > of_irq_get_byname(of_node, "INT1"); > drivers/iio/imu/bmi160/bmi160_core.c:661: irq = > of_irq_get_byname(of_node, "INT2"); > > I believe we may stop distributing this and actually start using a > common API. I don't want this to be spread again over all IIO. Btw, I > have LSM9DS0, which supports two INT pins for IMU and currently it > uses hard coded pin mapping. I'm definitely keen to tidy this up, though I'd also rather not tie it to this particular series. > > Side note to Jonathan, I believe the below may be (lazily) converted > to fwnode / device properties APIs. Yup. I know we still have a bunch of these to tidy up. Might take a while to get to them though! > > drivers/iio/adc/ab8500-gpadc.c:1041: nchans = > of_get_available_child_count(np); > drivers/iio/adc/ad7124.c:747: st->num_channels = > of_get_available_child_count(np); > drivers/iio/adc/berlin2-adc.c:287: struct device_node *parent_np > = of_get_parent(pdev->dev.of_node); > drivers/iio/adc/qcom-pm8xxx-xoadc.c:830: adc->nchans = > of_get_available_child_count(np); > drivers/iio/adc/qcom-spmi-adc5.c:650: channel_name = of_get_property(node, > drivers/iio/adc/qcom-spmi-adc5.c:813: adc->nchannels = > of_get_available_child_count(node); > drivers/iio/adc/qcom-spmi-vadc.c:743: vadc->nchannels = > of_get_available_child_count(node); > drivers/iio/adc/stm32-adc.c:1630:static int > stm32_adc_of_get_resolution(struct iio_dev *indio_dev) > drivers/iio/adc/stm32-adc.c:1935: ret = > stm32_adc_of_get_resolution(indio_dev); > drivers/iio/adc/xilinx-xadc-core.c:1248: chan_node = > of_get_child_by_name(np, "xlnx,channels"); > drivers/iio/imu/adis16480.c:1293:static int > adis16480_of_get_ext_clk_pin(struct adis16480 *st, > drivers/iio/imu/adis16480.c:1328: pin = > adis16480_of_get_ext_clk_pin(st, of_node); > drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c:68: mux_node = > of_get_child_by_name(dev->of_node, "i2c-gate") > ; > drivers/iio/industrialio-core.c:1877: label = > of_get_property(indio_dev->dev.of_node, "label", NULL); > drivers/iio/inkern.c:228: if (np && !of_get_property(np, > "io-channel-ranges", NULL)) > drivers/iio/temperature/ltc2983.c:1275: st->num_channels = > of_get_available_child_count(dev->of_node); > >
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c index 5ce384ebe6c7..8d3dd3c2bcc2 100644 --- a/drivers/iio/accel/bmc150-accel-core.c +++ b/drivers/iio/accel/bmc150-accel-core.c @@ -10,6 +10,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/acpi.h> +#include <linux/of_irq.h> #include <linux/pm.h> #include <linux/pm_runtime.h> #include <linux/iio/iio.h> @@ -57,12 +58,18 @@ #define BMC150_ACCEL_RESET_VAL 0xB6 #define BMC150_ACCEL_REG_INT_MAP_0 0x19 -#define BMC150_ACCEL_INT_MAP_0_BIT_SLOPE BIT(2) +#define BMC150_ACCEL_INT_MAP_0_BIT_INT1_SLOPE BIT(2) #define BMC150_ACCEL_REG_INT_MAP_1 0x1A -#define BMC150_ACCEL_INT_MAP_1_BIT_DATA BIT(0) -#define BMC150_ACCEL_INT_MAP_1_BIT_FWM BIT(1) -#define BMC150_ACCEL_INT_MAP_1_BIT_FFULL BIT(2) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT1_DATA BIT(0) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT1_FWM BIT(1) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT1_FFULL BIT(2) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT2_FFULL BIT(5) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT2_FWM BIT(6) +#define BMC150_ACCEL_INT_MAP_1_BIT_INT2_DATA BIT(7) + +#define BMC150_ACCEL_REG_INT_MAP_2 0x1B +#define BMC150_ACCEL_INT_MAP_2_BIT_INT2_SLOPE BIT(2) #define BMC150_ACCEL_REG_INT_RST_LATCH 0x21 #define BMC150_ACCEL_INT_MODE_LATCH_RESET 0x80 @@ -81,6 +88,7 @@ #define BMC150_ACCEL_REG_INT_OUT_CTRL 0x20 #define BMC150_ACCEL_INT_OUT_CTRL_INT1_LVL BIT(0) +#define BMC150_ACCEL_INT_OUT_CTRL_INT2_LVL BIT(2) #define BMC150_ACCEL_REG_INT_5 0x27 #define BMC150_ACCEL_SLOPE_DUR_MASK 0x03 @@ -476,21 +484,24 @@ static bool bmc150_apply_acpi_orientation(struct device *dev, } #endif -static const struct bmc150_accel_interrupt_info { +struct bmc150_accel_interrupt_info { u8 map_reg; u8 map_bitmask; u8 en_reg; u8 en_bitmask; -} bmc150_accel_interrupts[BMC150_ACCEL_INTERRUPTS] = { +}; + +static const struct bmc150_accel_interrupt_info +bmc150_accel_interrupts_int1[BMC150_ACCEL_INTERRUPTS] = { { /* data ready interrupt */ .map_reg = BMC150_ACCEL_REG_INT_MAP_1, - .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_DATA, + .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_INT1_DATA, .en_reg = BMC150_ACCEL_REG_INT_EN_1, .en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN, }, { /* motion interrupt */ .map_reg = BMC150_ACCEL_REG_INT_MAP_0, - .map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_SLOPE, + .map_bitmask = BMC150_ACCEL_INT_MAP_0_BIT_INT1_SLOPE, .en_reg = BMC150_ACCEL_REG_INT_EN_0, .en_bitmask = BMC150_ACCEL_INT_EN_BIT_SLP_X | BMC150_ACCEL_INT_EN_BIT_SLP_Y | @@ -498,19 +509,55 @@ static const struct bmc150_accel_interrupt_info { }, { /* fifo watermark interrupt */ .map_reg = BMC150_ACCEL_REG_INT_MAP_1, - .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_FWM, + .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_INT1_FWM, + .en_reg = BMC150_ACCEL_REG_INT_EN_1, + .en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN, + }, +}; + +static const struct bmc150_accel_interrupt_info +bmc150_accel_interrupts_int2[BMC150_ACCEL_INTERRUPTS] = { + { /* data ready interrupt */ + .map_reg = BMC150_ACCEL_REG_INT_MAP_1, + .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_INT2_DATA, + .en_reg = BMC150_ACCEL_REG_INT_EN_1, + .en_bitmask = BMC150_ACCEL_INT_EN_BIT_DATA_EN, + }, + { /* motion interrupt */ + .map_reg = BMC150_ACCEL_REG_INT_MAP_2, + .map_bitmask = BMC150_ACCEL_INT_MAP_2_BIT_INT2_SLOPE, + .en_reg = BMC150_ACCEL_REG_INT_EN_0, + .en_bitmask = BMC150_ACCEL_INT_EN_BIT_SLP_X | + BMC150_ACCEL_INT_EN_BIT_SLP_Y | + BMC150_ACCEL_INT_EN_BIT_SLP_Z + }, + { /* fifo watermark interrupt */ + .map_reg = BMC150_ACCEL_REG_INT_MAP_1, + .map_bitmask = BMC150_ACCEL_INT_MAP_1_BIT_INT2_FWM, .en_reg = BMC150_ACCEL_REG_INT_EN_1, .en_bitmask = BMC150_ACCEL_INT_EN_BIT_FWM_EN, }, }; static void bmc150_accel_interrupts_setup(struct iio_dev *indio_dev, - struct bmc150_accel_data *data) + struct bmc150_accel_data *data, int irq) { + const struct bmc150_accel_interrupt_info *irq_info = NULL; + struct device *dev = regmap_get_device(data->regmap); int i; + /* + * For now we map all interrupts to the same output pin. + * However, some boards may have just INT2 (and not INT1) connected, + * so we try to detect which IRQ it is based on the interrupt-names. + * Without interrupt-names, we assume the irq belongs to INT1. + */ + irq_info = bmc150_accel_interrupts_int1; + if (irq == of_irq_get_byname(dev->of_node, "INT2")) + irq_info = bmc150_accel_interrupts_int2; + for (i = 0; i < BMC150_ACCEL_INTERRUPTS; i++) - data->interrupts[i].info = &bmc150_accel_interrupts[i]; + data->interrupts[i].info = &irq_info[i]; } static int bmc150_accel_set_interrupt(struct bmc150_accel_data *data, int i, @@ -1714,7 +1761,7 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, goto err_buffer_cleanup; } - bmc150_accel_interrupts_setup(indio_dev, data); + bmc150_accel_interrupts_setup(indio_dev, data, irq); ret = bmc150_accel_triggers_setup(indio_dev, data); if (ret)
Some Bosch accelerometers have two interrupt pins (INT1 and INT2). At the moment, the driver uses only the first one, which is fine for most situations. However, some boards might only have INT2 connected for some reason. Add the necessary bits and configuration to set up INT2. Then try to detect this situation at least for device tree setups by checking if the first interrupt (the one picked by the I2C/SPI core) is actually named "INT2" using the interrupt-names property. of_irq_get_byname() returns either 0 or some error code in case the driver probed without device tree, so in all other cases we fall back to configuring INT1 as before. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- drivers/iio/accel/bmc150-accel-core.c | 71 ++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 12 deletions(-)