Re: [PATCH v1 09/10] iio: pressure: bmp280: Add data ready trigger support

[Vasileios Amoiridis <vassilisamir@gmail.com>]

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