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

[Jonathan Cameron <jic23@kernel.org>]

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