| Message ID | 20240711211558.106327-10-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | BMP280: Minor cleanup and interrupt support | expand |
On Thu, 11 Jul 2024 23:15:57 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > a trigger for when there are data ready in the sensor for pick up. > > This use case is used along with NORMAL_MODE in the sensor, which allows > the sensor to do consecutive measurements depending on the ODR rate value. > > The trigger pin can be configured to be open-drain or push-pull and either > rising or falling edge. > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > values. > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> A few minor things inline. It might be worth thinking a bit about future fifo support as that can get a little messy in a driver that already supports a dataready trigger. We end up with no trigger being set meaning use the fifo. Sometimes it makes more sense to not support triggers at all. What you have here is fine though as we have a bunch of drivers that grew dataready trigger support before adding fifos later particularly as often it's a 'new chip' that brings the fifo support but maintains backwards compatibility if you don't use it. > + > +static int bmp380_trigger_probe(struct iio_dev *indio_dev) > +{ > + struct bmp280_data *data = iio_priv(indio_dev); > + struct fwnode_handle *fwnode; > + int ret, irq, irq_type; > + struct irq_data *desc; > + > + fwnode = dev_fwnode(data->dev); > + if (!fwnode) > + return -ENODEV; > + > + irq = fwnode_irq_get_byname(fwnode, "DRDY"); > + if (!irq) { > + dev_err(data->dev, "No DRDY interrupt found\n"); > + return -ENODEV; As below. return dev_err_probe() for anything that is only called from probe() > + } > + > + desc = irq_get_irq_data(irq); > + if (!desc) > + return -EINVAL; > + > + irq_type = irqd_get_trigger_type(desc); > + switch (irq_type) { > + case IRQF_TRIGGER_RISING: > + data->trig_active_high = true; > + break; > + case IRQF_TRIGGER_FALLING: > + data->trig_active_high = false; > + break; > + default: > + dev_err(data->dev, "Invalid interrupt type specified\n"); > + return -EINVAL; > + } > + > + data->trig_open_drain = fwnode_property_read_bool(fwnode, > + "int-open-drain"); > + > + ret = bmp380_int_config(data); > + if (ret) > + return ret; > + > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!data->trig) > + return -ENOMEM; > + > + data->trig->ops = &bmp380_trigger_ops; > + iio_trigger_set_drvdata(data->trig, data); > + > + ret = devm_request_irq(data->dev, irq, > + &iio_trigger_generic_data_rdy_poll, > + IRQF_ONESHOT, indio_dev->name, data->trig); > + if (ret) { > + dev_err(data->dev, "request irq failed\n"); > + return ret; > + } > + > + ret = devm_iio_trigger_register(data->dev, data->trig); > + if (ret) { > + dev_err(data->dev, "iio trigger register failed\n"); > + return ret; > + } > + > + indio_dev->trig = iio_trigger_get(data->trig); > + > + return 0; > +} > + > + One blank line only. > static irqreturn_t bmp380_trigger_handler(int irq, void *p) > + > +static int bmp580_trigger_probe(struct iio_dev *indio_dev) > +{ > + struct bmp280_data *data = iio_priv(indio_dev); > + struct fwnode_handle *fwnode; > + int ret, irq, irq_type; > + struct irq_data *desc; > + > + fwnode = dev_fwnode(data->dev); > + if (!fwnode) > + return -ENODEV; > + > + irq = fwnode_irq_get_byname(fwnode, "DRDY"); > + if (!irq) { > + dev_err(data->dev, "No DRDY interrupt found\n"); As this only gets called from probe(), use return dev_err_probe() throughout. > + return -ENODEV; > + } > + > + desc = irq_get_irq_data(irq); > + if (!desc) > + return -EINVAL; > + > + irq_type = irqd_get_trigger_type(desc); > + switch (irq_type) { > + case IRQF_TRIGGER_RISING: > + data->trig_active_high = true; > + break; > + case IRQF_TRIGGER_FALLING: > + data->trig_active_high = false; > + break; > + default: > + dev_err(data->dev, "Invalid interrupt type specified\n"); > + return -EINVAL; > + } > + > + data->trig_open_drain = fwnode_property_read_bool(fwnode, > + "int-open-drain"); > + > + ret = bmp580_int_config(data); > + if (ret) > + return ret; > + > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!data->trig) > + return -ENOMEM; > + > + data->trig->ops = &bmp580_trigger_ops; > + iio_trigger_set_drvdata(data->trig, data); > + > + ret = devm_request_irq(data->dev, irq, > + &iio_trigger_generic_data_rdy_poll, > + IRQF_ONESHOT, indio_dev->name, data->trig); > + if (ret) { > + dev_err(data->dev, "request irq failed\n"); > + return ret; > + } > + > + ret = devm_iio_trigger_register(data->dev, data->trig); > + if (ret) { > + dev_err(data->dev, "iio trigger register failed\n"); > + return ret; > + } > + > + indio_dev->trig = iio_trigger_get(data->trig); > + > + return 0; > +} > >
On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote: > On Thu, 11 Jul 2024 23:15:57 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > > a trigger for when there are data ready in the sensor for pick up. > > > > This use case is used along with NORMAL_MODE in the sensor, which allows > > the sensor to do consecutive measurements depending on the ODR rate value. > > > > The trigger pin can be configured to be open-drain or push-pull and either > > rising or falling edge. > > > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > > values. > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > A few minor things inline. > > It might be worth thinking a bit about future fifo support as that can > get a little messy in a driver that already supports a dataready trigger. > We end up with no trigger being set meaning use the fifo. Sometimes > it makes more sense to not support triggers at all. > > What you have here is fine though as we have a bunch of drivers > that grew dataready trigger support before adding fifos later > particularly as often it's a 'new chip' that brings the fifo > support but maintains backwards compatibility if you don't use it. > Hi Jonathan, Thank you very much for your thorough review again! What I could do to make the code even better to be able to accept FIFO irq support are the following: 1) in the bmp{380/580}_trigger_handler() currently, the data registers are being read. What I could do is to move the reading of registers to a separe function like bmpxxx_drdy_trigger_handler() and calling it inside the bmp{380/580}_trigger_handler() when I have DRDY or sysfs irq. In order to check the enabled irqs I propose also no.2 2) in the following bmp{380/580}_trigger_probe() functions instead of just doing: irq = fwnode_irq_get_byname(fwnode, "DRDY"); if (!irq) { dev_err(data->dev, "No DRDY interrupt found\n"); return -ENODEV; } I could also use some type of variable like we do for the active channels in order to track "active/existing irqs". Like this it would be easier to track the active irqs of the sensor. Let me know what you think, or if I manage to have time I might send a v2 with those changes even earlier :) Cheers, Vasilis > > + > > +static int bmp380_trigger_probe(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + struct fwnode_handle *fwnode; > > + int ret, irq, irq_type; > > + struct irq_data *desc; > > + > > + fwnode = dev_fwnode(data->dev); > > + if (!fwnode) > > + return -ENODEV; > > + > > + irq = fwnode_irq_get_byname(fwnode, "DRDY"); > > + if (!irq) { > > + dev_err(data->dev, "No DRDY interrupt found\n"); > > + return -ENODEV; > As below. return dev_err_probe() for anything that is only > called from probe() > > > + } > > + > > + desc = irq_get_irq_data(irq); > > + if (!desc) > > + return -EINVAL; > > + > > + irq_type = irqd_get_trigger_type(desc); > > + switch (irq_type) { > > + case IRQF_TRIGGER_RISING: > > + data->trig_active_high = true; > > + break; > > + case IRQF_TRIGGER_FALLING: > > + data->trig_active_high = false; > > + break; > > + default: > > + dev_err(data->dev, "Invalid interrupt type specified\n"); > > + return -EINVAL; > > + } > > + > > + data->trig_open_drain = fwnode_property_read_bool(fwnode, > > + "int-open-drain"); > > + > > + ret = bmp380_int_config(data); > > + if (ret) > > + return ret; > > + > > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", > > + indio_dev->name, > > + iio_device_id(indio_dev)); > > + if (!data->trig) > > + return -ENOMEM; > > + > > + data->trig->ops = &bmp380_trigger_ops; > > + iio_trigger_set_drvdata(data->trig, data); > > + > > + ret = devm_request_irq(data->dev, irq, > > + &iio_trigger_generic_data_rdy_poll, > > + IRQF_ONESHOT, indio_dev->name, data->trig); > > + if (ret) { > > + dev_err(data->dev, "request irq failed\n"); > > + return ret; > > + } > > + > > + ret = devm_iio_trigger_register(data->dev, data->trig); > > + if (ret) { > > + dev_err(data->dev, "iio trigger register failed\n"); > > + return ret; > > + } > > + > > + indio_dev->trig = iio_trigger_get(data->trig); > > + > > + return 0; > > +} > > + > > + > One blank line only. > > > static irqreturn_t bmp380_trigger_handler(int irq, void *p) > > > > + > > +static int bmp580_trigger_probe(struct iio_dev *indio_dev) > > +{ > > + struct bmp280_data *data = iio_priv(indio_dev); > > + struct fwnode_handle *fwnode; > > + int ret, irq, irq_type; > > + struct irq_data *desc; > > + > > + fwnode = dev_fwnode(data->dev); > > + if (!fwnode) > > + return -ENODEV; > > + > > + irq = fwnode_irq_get_byname(fwnode, "DRDY"); > > + if (!irq) { > > + dev_err(data->dev, "No DRDY interrupt found\n"); > > As this only gets called from probe(), use return dev_err_probe() throughout. > > > + return -ENODEV; > > + } > > + > > + desc = irq_get_irq_data(irq); > > + if (!desc) > > + return -EINVAL; > > + > > + irq_type = irqd_get_trigger_type(desc); > > + switch (irq_type) { > > + case IRQF_TRIGGER_RISING: > > + data->trig_active_high = true; > > + break; > > + case IRQF_TRIGGER_FALLING: > > + data->trig_active_high = false; > > + break; > > + default: > > + dev_err(data->dev, "Invalid interrupt type specified\n"); > > + return -EINVAL; > > + } > > + > > + data->trig_open_drain = fwnode_property_read_bool(fwnode, > > + "int-open-drain"); > > + > > + ret = bmp580_int_config(data); > > + if (ret) > > + return ret; > > + > > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", > > + indio_dev->name, > > + iio_device_id(indio_dev)); > > + if (!data->trig) > > + return -ENOMEM; > > + > > + data->trig->ops = &bmp580_trigger_ops; > > + iio_trigger_set_drvdata(data->trig, data); > > + > > + ret = devm_request_irq(data->dev, irq, > > + &iio_trigger_generic_data_rdy_poll, > > + IRQF_ONESHOT, indio_dev->name, data->trig); > > + if (ret) { > > + dev_err(data->dev, "request irq failed\n"); > > + return ret; > > + } > > + > > + ret = devm_iio_trigger_register(data->dev, data->trig); > > + if (ret) { > > + dev_err(data->dev, "iio trigger register failed\n"); > > + return ret; > > + } > > + > > + indio_dev->trig = iio_trigger_get(data->trig); > > + > > + return 0; > > +} > > > > >
On Mon, 22 Jul 2024 01:51:13 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote: > > On Thu, 11 Jul 2024 23:15:57 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > > > a trigger for when there are data ready in the sensor for pick up. > > > > > > This use case is used along with NORMAL_MODE in the sensor, which allows > > > the sensor to do consecutive measurements depending on the ODR rate value. > > > > > > The trigger pin can be configured to be open-drain or push-pull and either > > > rising or falling edge. > > > > > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > > > values. > > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > A few minor things inline. > > > > It might be worth thinking a bit about future fifo support as that can > > get a little messy in a driver that already supports a dataready trigger. > > We end up with no trigger being set meaning use the fifo. Sometimes > > it makes more sense to not support triggers at all. > > > > What you have here is fine though as we have a bunch of drivers > > that grew dataready trigger support before adding fifos later > > particularly as often it's a 'new chip' that brings the fifo > > support but maintains backwards compatibility if you don't use it. > > > > Hi Jonathan, > > Thank you very much for your thorough review again! > > What I could do to make the code even better to be able to accept > FIFO irq support are the following: > > 1) in the bmp{380/580}_trigger_handler() currently, the data registers > are being read. What I could do is to move the reading of registers > to a separe function like bmpxxx_drdy_trigger_handler() and calling > it inside the bmp{380/580}_trigger_handler() when I have DRDY or > sysfs irq. In order to check the enabled irqs I propose also no.2 You shouldn't get to the trigger_handler by other paths. But sure a bit of code reuse might make sense if fifo read out path is same as for other data reads. Superficially it looks totally different on the bmp380 though as there is a separate fifo register. > > 2) in the following bmp{380/580}_trigger_probe() functions instead of > just doing: > > irq = fwnode_irq_get_byname(fwnode, "DRDY"); > if (!irq) { > dev_err(data->dev, "No DRDY interrupt found\n"); > return -ENODEV; > } > > I could also use some type of variable like we do for the active > channels in order to track "active/existing irqs". I think there is only one IRQ on the 380 at least. So you should only request it once for this driver. Then software gets to configure what it is for. However it shouldn't be called DRDY for these parts at least. It's just INT on the datasheet. The interrupt control register value will tell you what is enabled. No need to track it separately. If you mean track the one from the poll function registered for handling triggers - that's an internal detail but you would indeed need to track in your data structures whether that's the trigger currently being used or not (easy to do by comparing iio_dev->trig with a pointer for each trigger in iio_priv() data - so should be no need for separate tracking. Jonathan > > Like this it would be easier to track the active irqs of the sensor. > > Let me know what you think, or if I manage to have time I might send > a v2 with those changes even earlier :) > > Cheers, > Vasilis >
On Mon, Jul 22, 2024 at 08:31:38PM +0100, Jonathan Cameron wrote: > On Mon, 22 Jul 2024 01:51:13 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote: > > > On Thu, 11 Jul 2024 23:15:57 +0200 > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > > > > a trigger for when there are data ready in the sensor for pick up. > > > > > > > > This use case is used along with NORMAL_MODE in the sensor, which allows > > > > the sensor to do consecutive measurements depending on the ODR rate value. > > > > > > > > The trigger pin can be configured to be open-drain or push-pull and either > > > > rising or falling edge. > > > > > > > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > > > > values. > > > > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > A few minor things inline. > > > > > > It might be worth thinking a bit about future fifo support as that can > > > get a little messy in a driver that already supports a dataready trigger. > > > We end up with no trigger being set meaning use the fifo. Sometimes > > > it makes more sense to not support triggers at all. > > > > > > What you have here is fine though as we have a bunch of drivers > > > that grew dataready trigger support before adding fifos later > > > particularly as often it's a 'new chip' that brings the fifo > > > support but maintains backwards compatibility if you don't use it. > > > > > > > Hi Jonathan, > > > > Thank you very much for your thorough review again! > > > > What I could do to make the code even better to be able to accept > > FIFO irq support are the following: > > > > 1) in the bmp{380/580}_trigger_handler() currently, the data registers > > are being read. What I could do is to move the reading of registers > > to a separe function like bmpxxx_drdy_trigger_handler() and calling > > it inside the bmp{380/580}_trigger_handler() when I have DRDY or > > sysfs irq. In order to check the enabled irqs I propose also no.2 > > You shouldn't get to the trigger_handler by other paths. But sure > a bit of code reuse might make sense if fifo read out path is same > as for other data reads. Superficially it looks totally different > on the bmp380 though as there is a separate fifo register. > So, I don't mean getting into the trigger_handler by other paths. I will always end up in the trigger_handler and then, depending on the interrupt that was triggered (DRDY, FIFO, etc...) I choose different actions. > > > > 2) in the following bmp{380/580}_trigger_probe() functions instead of > > just doing: > > > > irq = fwnode_irq_get_byname(fwnode, "DRDY"); > > if (!irq) { > > dev_err(data->dev, "No DRDY interrupt found\n"); > > return -ENODEV; > > } > > > > I could also use some type of variable like we do for the active > > channels in order to track "active/existing irqs". > > I think there is only one IRQ on the 380 at least. So > you should only request it once for this driver. Then software > gets to configure what it is for. > > However it shouldn't be called DRDY for these parts at least. It's > just INT on the datasheet. > The interrupt control register value will tell you what is enabled. > No need to track it separately. > So I am a bit confused. Indeed, BMP380 has only irq line. So I understand why I should call it INT. The actual IRQ inside the chip that will be triggered needs to be configured by software. I do it through the bmp{3/5}80_int_config() function. How am I supposed to know which IRQ to enable? Options are: a) DRDY only b) FIFO only c) both How can I inform the driver about which to enable? Shouldn't this go through the device-tree? > If you mean track the one from the poll function registered for > handling triggers - that's an internal detail but you would indeed > need to track in your data structures whether that's the trigger > currently being used or not (easy to do by comparing iio_dev->trig > with a pointer for each trigger in iio_priv() data - so should be no > need for separate tracking. > My idea is that there is one trigger_handler which inside you check which interrupt triggered and you choose which path to choose. If that's wrong, do you know any specific driver that implements it in the correct way? Because in my mind, the iio_dev->trig is one and just inside the handler, different functions are called. > Jonathan > > Thanks for the feedback :) Cheers, Vasilis > > > > > Like this it would be easier to track the active irqs of the sensor. > > > > Let me know what you think, or if I manage to have time I might send > > a v2 with those changes even earlier :) > > > > Cheers, > > Vasilis > > >
On Mon, 22 Jul 2024 22:38:34 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > On Mon, Jul 22, 2024 at 08:31:38PM +0100, Jonathan Cameron wrote: > > On Mon, 22 Jul 2024 01:51:13 +0200 > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > On Sat, Jul 20, 2024 at 12:37:27PM +0100, Jonathan Cameron wrote: > > > > On Thu, 11 Jul 2024 23:15:57 +0200 > > > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > > > > > > > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > > > > > a trigger for when there are data ready in the sensor for pick up. > > > > > > > > > > This use case is used along with NORMAL_MODE in the sensor, which allows > > > > > the sensor to do consecutive measurements depending on the ODR rate value. > > > > > > > > > > The trigger pin can be configured to be open-drain or push-pull and either > > > > > rising or falling edge. > > > > > > > > > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > > > > > values. > > > > > > > > > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> > > > > > > > > A few minor things inline. > > > > > > > > It might be worth thinking a bit about future fifo support as that can > > > > get a little messy in a driver that already supports a dataready trigger. > > > > We end up with no trigger being set meaning use the fifo. Sometimes > > > > it makes more sense to not support triggers at all. > > > > > > > > What you have here is fine though as we have a bunch of drivers > > > > that grew dataready trigger support before adding fifos later > > > > particularly as often it's a 'new chip' that brings the fifo > > > > support but maintains backwards compatibility if you don't use it. > > > > > > > > > > Hi Jonathan, > > > > > > Thank you very much for your thorough review again! > > > > > > What I could do to make the code even better to be able to accept > > > FIFO irq support are the following: > > > > > > 1) in the bmp{380/580}_trigger_handler() currently, the data registers > > > are being read. What I could do is to move the reading of registers > > > to a separe function like bmpxxx_drdy_trigger_handler() and calling > > > it inside the bmp{380/580}_trigger_handler() when I have DRDY or > > > sysfs irq. In order to check the enabled irqs I propose also no.2 > > > > You shouldn't get to the trigger_handler by other paths. But sure > > a bit of code reuse might make sense if fifo read out path is same > > as for other data reads. Superficially it looks totally different > > on the bmp380 though as there is a separate fifo register. > > > > So, I don't mean getting into the trigger_handler by other paths. I will > always end up in the trigger_handler and then, depending on the interrupt > that was triggered (DRDY, FIFO, etc...) I choose different actions. Often the right thing to do for a fifo is to not use a trigger at all. So instead it becomes the main interrupt handler that runs that path. The reason being that triggers are typically one per 'scan' i.e. set of channels and fifo interrupts are much less common. That makes a mess of things like timestamps etc that means different handling is necessary. So for fifo paths we often just don't use a trigger. > > > > > > > 2) in the following bmp{380/580}_trigger_probe() functions instead of > > > just doing: > > > > > > irq = fwnode_irq_get_byname(fwnode, "DRDY"); > > > if (!irq) { > > > dev_err(data->dev, "No DRDY interrupt found\n"); > > > return -ENODEV; > > > } > > > > > > I could also use some type of variable like we do for the active > > > channels in order to track "active/existing irqs". > > > > I think there is only one IRQ on the 380 at least. So > > you should only request it once for this driver. Then software > > gets to configure what it is for. > > > > However it shouldn't be called DRDY for these parts at least. It's > > just INT on the datasheet. > > The interrupt control register value will tell you what is enabled. > > No need to track it separately. > > > > So I am a bit confused. Indeed, BMP380 has only irq line. So I understand > why I should call it INT. The actual IRQ inside the chip that will be > triggered needs to be configured by software. I do it through the > bmp{3/5}80_int_config() function. How am I supposed to know > which IRQ to enable? Options are: > > a) DRDY only > b) FIFO only > c) both > > How can I inform the driver about which to enable? Shouldn't this go > through the device-tree? No. This is a policy question for the driver, not something to do with wiring (which is what belongs in device tree). You choose how it is used based on what userspace configures the device to do. DRDY if it uses that trigger. FIFO typically if the buffer is enabled without a trigger (though we have a few cases where a dummy trigger is used for this). > > > If you mean track the one from the poll function registered for > > handling triggers - that's an internal detail but you would indeed > > need to track in your data structures whether that's the trigger > > currently being used or not (easy to do by comparing iio_dev->trig > > with a pointer for each trigger in iio_priv() data - so should be no > > need for separate tracking. > > > > My idea is that there is one trigger_handler which inside you check > which interrupt triggered and you choose which path to choose. If that's > wrong, do you know any specific driver that implements it in the correct > way? Because in my mind, the iio_dev->trig is one and just inside the > handler, different functions are called. You don't have to use a trigger. Just look for drivers that handle a fifo. This is pretty common situation and there are a couple of different solutions but often the cleanest is to not use the trigger infrastructure at all if the fifo is in use. Jonathan > > > Jonathan > > > > > > Thanks for the feedback :) > > Cheers, > Vasilis > > > > > > > > > Like this it would be easier to track the active irqs of the sensor. > > > > > > Let me know what you think, or if I manage to have time I might send > > > a v2 with those changes even earlier :) > > > > > > Cheers, > > > Vasilis > > > > >
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index fc8d42880eb8..ee9b9676ad10 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -37,12 +37,14 @@ #include <linux/module.h> #include <linux/nvmem-provider.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <linux/random.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/iio/buffer.h> #include <linux/iio/iio.h> +#include <linux/iio/trigger.h> #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> @@ -1770,6 +1772,129 @@ static int bmp380_chip_config(struct bmp280_data *data) return 0; } +static void bmp380_trigger_reenable(struct iio_trigger *trig) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + unsigned int tmp; + int ret; + + ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &tmp); + if (ret) + dev_err(data->dev, "Failed to reset interrupt\n"); +} + +static int bmp380_data_rdy_trigger_set_state(struct iio_trigger *trig, + bool state) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + int ret; + + guard(mutex)(&data->lock); + + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL, + BMP380_INT_CTRL_DRDY_EN, + FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, + state ? 1 : 0)); + if (ret) { + dev_err(data->dev, "Could not enable/disable interrupt\n"); + return ret; + } + + return 0; +} + +static const struct iio_trigger_ops bmp380_trigger_ops = { + .set_trigger_state = &bmp380_data_rdy_trigger_set_state, + .reenable = &bmp380_trigger_reenable, +}; + +static int bmp380_int_config(struct bmp280_data *data) +{ + int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN, + data->trig_open_drain) | + FIELD_PREP(BMP380_INT_CTRL_LEVEL, + data->trig_active_high); + + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CTRL, + BMP380_INT_CTRL_SETTINGS_MASK, int_cfg); + if (ret) { + dev_err(data->dev, "Could not set interrupt settings\n"); + return ret; + } + + return 0; +} + +static int bmp380_trigger_probe(struct iio_dev *indio_dev) +{ + struct bmp280_data *data = iio_priv(indio_dev); + struct fwnode_handle *fwnode; + int ret, irq, irq_type; + struct irq_data *desc; + + fwnode = dev_fwnode(data->dev); + if (!fwnode) + return -ENODEV; + + irq = fwnode_irq_get_byname(fwnode, "DRDY"); + if (!irq) { + dev_err(data->dev, "No DRDY interrupt found\n"); + return -ENODEV; + } + + desc = irq_get_irq_data(irq); + if (!desc) + return -EINVAL; + + irq_type = irqd_get_trigger_type(desc); + switch (irq_type) { + case IRQF_TRIGGER_RISING: + data->trig_active_high = true; + break; + case IRQF_TRIGGER_FALLING: + data->trig_active_high = false; + break; + default: + dev_err(data->dev, "Invalid interrupt type specified\n"); + return -EINVAL; + } + + data->trig_open_drain = fwnode_property_read_bool(fwnode, + "int-open-drain"); + + ret = bmp380_int_config(data); + if (ret) + return ret; + + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!data->trig) + return -ENOMEM; + + data->trig->ops = &bmp380_trigger_ops; + iio_trigger_set_drvdata(data->trig, data); + + ret = devm_request_irq(data->dev, irq, + &iio_trigger_generic_data_rdy_poll, + IRQF_ONESHOT, indio_dev->name, data->trig); + if (ret) { + dev_err(data->dev, "request irq failed\n"); + return ret; + } + + ret = devm_iio_trigger_register(data->dev, data->trig); + if (ret) { + dev_err(data->dev, "iio trigger register failed\n"); + return ret; + } + + indio_dev->trig = iio_trigger_get(data->trig); + + return 0; +} + + static irqreturn_t bmp380_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; @@ -1863,6 +1988,7 @@ const struct bmp280_chip_info bmp380_chip_info = { .wait_conv = bmp380_wait_conv, .preinit = bmp380_preinit, + .trigger_probe = bmp380_trigger_probe, .trigger_handler = bmp380_trigger_handler, }; EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280); @@ -2400,6 +2526,135 @@ static int bmp580_chip_config(struct bmp280_data *data) return 0; } +static void bmp580_trigger_reenable(struct iio_trigger *trig) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + unsigned int tmp; + int ret; + + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp); + if (ret) + dev_err(data->dev, "Failed to reset interrupt\n"); +} + +static int bmp580_data_rdy_trigger_set_state(struct iio_trigger *trig, + bool state) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + int ret; + + guard(mutex)(&data->lock); + + ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG, + BMP580_INT_CONFIG_INT_EN, + FIELD_PREP(BMP580_INT_CONFIG_INT_EN, + state ? 1 : 0)); + if (ret) { + dev_err(data->dev, "Could not enable/disable interrupt\n"); + return ret; + } + + return 0; +} + +static const struct iio_trigger_ops bmp580_trigger_ops = { + .set_trigger_state = &bmp580_data_rdy_trigger_set_state, + .reenable = &bmp580_trigger_reenable, +}; + +static int bmp580_int_config(struct bmp280_data *data) +{ + int ret, int_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN, + data->trig_open_drain) | + FIELD_PREP(BMP580_INT_CONFIG_LEVEL, + data->trig_active_high); + + ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG, + BMP580_INT_CONFIG_MASK, int_cfg); + if (ret) { + dev_err(data->dev, "Could not set interrupt settings\n"); + return ret; + } + + ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE, + BMP580_INT_SOURCE_DRDY); + if (ret) { + dev_err(data->dev, "Could not set interrupt source\n"); + return ret; + } + + return 0; +} + +static int bmp580_trigger_probe(struct iio_dev *indio_dev) +{ + struct bmp280_data *data = iio_priv(indio_dev); + struct fwnode_handle *fwnode; + int ret, irq, irq_type; + struct irq_data *desc; + + fwnode = dev_fwnode(data->dev); + if (!fwnode) + return -ENODEV; + + irq = fwnode_irq_get_byname(fwnode, "DRDY"); + if (!irq) { + dev_err(data->dev, "No DRDY interrupt found\n"); + return -ENODEV; + } + + desc = irq_get_irq_data(irq); + if (!desc) + return -EINVAL; + + irq_type = irqd_get_trigger_type(desc); + switch (irq_type) { + case IRQF_TRIGGER_RISING: + data->trig_active_high = true; + break; + case IRQF_TRIGGER_FALLING: + data->trig_active_high = false; + break; + default: + dev_err(data->dev, "Invalid interrupt type specified\n"); + return -EINVAL; + } + + data->trig_open_drain = fwnode_property_read_bool(fwnode, + "int-open-drain"); + + ret = bmp580_int_config(data); + if (ret) + return ret; + + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!data->trig) + return -ENOMEM; + + data->trig->ops = &bmp580_trigger_ops; + iio_trigger_set_drvdata(data->trig, data); + + ret = devm_request_irq(data->dev, irq, + &iio_trigger_generic_data_rdy_poll, + IRQF_ONESHOT, indio_dev->name, data->trig); + if (ret) { + dev_err(data->dev, "request irq failed\n"); + return ret; + } + + ret = devm_iio_trigger_register(data->dev, data->trig); + if (ret) { + dev_err(data->dev, "iio trigger register failed\n"); + return ret; + } + + indio_dev->trig = iio_trigger_get(data->trig); + + return 0; +} + static irqreturn_t bmp580_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; @@ -2476,6 +2731,7 @@ const struct bmp280_chip_info bmp580_chip_info = { .wait_conv = bmp580_wait_conv, .preinit = bmp580_preinit, + .trigger_probe = bmp580_trigger_probe, .trigger_handler = bmp580_trigger_handler, }; EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); @@ -3020,10 +3276,17 @@ int bmp280_common_probe(struct device *dev, * 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; + if (irq > 0) { + if (chip_id == BMP180_CHIP_ID) { + ret = bmp085_fetch_eoc_irq(dev, name, irq, data); + if (ret) + return ret; + } + if (data->chip_info->trigger_probe) { + ret = data->chip_info->trigger_probe(indio_dev); + if (ret) + return ret; + } } ret = data->chip_info->set_mode(data, BMP280_SLEEP); diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c index d27d68edd906..cccdf8fc6c09 100644 --- a/drivers/iio/pressure/bmp280-regmap.c +++ b/drivers/iio/pressure/bmp280-regmap.c @@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg) case BMP380_REG_FIFO_WATERMARK_LSB: case BMP380_REG_FIFO_WATERMARK_MSB: case BMP380_REG_POWER_CONTROL: - case BMP380_REG_INT_CONTROL: + case BMP380_REG_INT_CTRL: case BMP380_REG_IF_CONFIG: case BMP380_REG_ODR: case BMP380_REG_OSR: diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index 93c006c33552..73d0d25ae0f4 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -55,8 +55,17 @@ #define BMP580_CMD_NVM_WRITE_SEQ_1 0xA0 #define BMP580_CMD_SOFT_RESET 0xB6 +#define BMP580_INT_STATUS_DRDY_MASK BIT(0) #define BMP580_INT_STATUS_POR_MASK BIT(4) +#define BMP580_INT_SOURCE_DRDY BIT(0) + +#define BMP580_INT_CONFIG_MASK GENMASK(3, 0) +#define BMP580_INT_CONFIG_LATCH BIT(0) +#define BMP580_INT_CONFIG_LEVEL BIT(1) +#define BMP580_INT_CONFIG_OPEN_DRAIN BIT(2) +#define BMP580_INT_CONFIG_INT_EN BIT(3) + #define BMP580_STATUS_CORE_RDY_MASK BIT(0) #define BMP580_STATUS_NVM_RDY_MASK BIT(1) #define BMP580_STATUS_NVM_ERR_MASK BIT(2) @@ -117,7 +126,7 @@ #define BMP380_REG_OSR 0x1C #define BMP380_REG_POWER_CONTROL 0x1B #define BMP380_REG_IF_CONFIG 0x1A -#define BMP380_REG_INT_CONTROL 0x19 +#define BMP380_REG_INT_CTRL 0x19 #define BMP380_REG_INT_STATUS 0x11 #define BMP380_REG_EVENT 0x10 #define BMP380_REG_STATUS 0x03 @@ -175,6 +184,14 @@ #define BMP380_TEMP_MEAS_OFFSET 163 #define BMP380_PRESS_MEAS_OFFSET 392 +#define BMP380_INT_STATUS_DRDY BIT(3) + +#define BMP380_INT_CTRL_SETTINGS_MASK GENMASK(2, 0) +#define BMP380_INT_CTRL_OPEN_DRAIN BIT(0) +#define BMP380_INT_CTRL_LEVEL BIT(1) +#define BMP380_INT_CTRL_LATCH BIT(2) +#define BMP380_INT_CTRL_DRDY_EN BIT(6) + #define BMP380_MIN_TEMP -4000 #define BMP380_MAX_TEMP 8500 #define BMP380_MIN_PRES 3000000 @@ -399,6 +416,9 @@ struct bmp280_data { struct regmap *regmap; struct completion done; bool use_eoc; + bool trig_open_drain; + bool trig_active_high; + struct iio_trigger *trig; const struct bmp280_chip_info *chip_info; union { struct bmp180_calib bmp180; @@ -497,6 +517,7 @@ struct bmp280_chip_info { int (*set_mode)(struct bmp280_data *data, u8 mode); int (*wait_conv)(struct bmp280_data *data); + int (*trigger_probe)(struct iio_dev *indio_dev); irqreturn_t (*trigger_handler)(int irq, void *p); };
The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as a trigger for when there are data ready in the sensor for pick up. This use case is used along with NORMAL_MODE in the sensor, which allows the sensor to do consecutive measurements depending on the ODR rate value. The trigger pin can be configured to be open-drain or push-pull and either rising or falling edge. No support is added yet for interrupts for FIFO, WATERMARK and out of range values. Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 271 ++++++++++++++++++++++++++- drivers/iio/pressure/bmp280-regmap.c | 2 +- drivers/iio/pressure/bmp280.h | 23 ++- 3 files changed, 290 insertions(+), 6 deletions(-)