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@xxxxxxxxx> 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@xxxxxxxxx> > > 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; > > +} > > > > >