Re: [PATCH v4 6/7] iio: pressure: bmp280: Add data ready trigger support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 28, 2024 at 10:51:26PM +0200, Vasileios Amoiridis 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>
> ---
>  drivers/iio/pressure/bmp280-core.c | 231 ++++++++++++++++++++++++++++-
>  drivers/iio/pressure/bmp280.h      |  21 +++
>  2 files changed, 250 insertions(+), 2 deletions(-)

...

> +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> +				  const struct iio_trigger_ops *trigger_ops,
> +				  int (*int_config)(struct bmp280_data *data),
> +				  irq_handler_t irq_thread_handler)
> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);

With

	struct device *dev = data->dev;

you may shorten some lines below and collapse a few.

> +	struct fwnode_handle *fwnode;
> +	int ret, irq, irq_type;

Why irq_type is signed?

Also try to make that returned variable is closer to the end of the definition
block. And it might be worth to follow reversed xmas tree order (longer lines
first).

> +	struct irq_data *desc;
> +
> +	irq = fwnode_irq_get(dev_fwnode(data->dev), 0);
> +	if (irq < 0)
> +		return dev_err_probe(data->dev, irq, "No interrupt found.\n");
> +
> +	desc = irq_get_irq_data(irq);
> +	irq_type = irqd_get_trigger_type(desc);

So, altogether it may be written as

	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));

And looking further, we have a helper for that:
irq_get_trigger_type(). Why not use it?

> +	switch (irq_type) {
> +	case IRQF_TRIGGER_RISING:
> +		data->trig_active_high = true;
> +		break;
> +	case IRQF_TRIGGER_FALLING:
> +		data->trig_active_high = false;
> +		break;
> +	default:
> +		return dev_err_probe(data->dev, -EINVAL,
> +				     "Invalid interrupt type specified.\n");
> +	}
> +
> +	data->trig_open_drain =
> +		fwnode_property_read_bool(fwnode, "int-open-drain");
> +
> +	ret = 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 = trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, data);
> +
> +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> +					irq_thread_handler, IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "request irq failed.\n");
> +
> +	ret = devm_iio_trigger_register(data->dev, data->trig);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio trigger register failed.\n");
> +
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	return 0;
> +}

...

> +static int bmp380_int_config(struct bmp280_data *data)
> +{
> +	int pin_drive_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
> +				       data->trig_open_drain);
> +	int pin_level_cfg = FIELD_PREP(BMP380_INT_CTRL_LEVEL,
> +				       data->trig_active_high);
> +	int ret, int_cfg = pin_drive_cfg | pin_level_cfg;
> +
> +	ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL,
> +				 BMP380_INT_CTRL_SETTINGS_MASK, int_cfg);
> +	if (ret) {
> +		dev_err(data->dev, "Could not set interrupt settings\n");

> +		return ret;
> +	}
> +
> +	return ret;

One of them is redundant or the last one should be return 0, but why not just
leave one of them?

> +}

...

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

So far you have three different styles in the same patch for this!
Choose one and be consistent with it.

> +}

...

> +	int (*trigger_probe)(struct iio_dev *indio_dev);
>  	irqreturn_t (*trigger_handler)(int irq, void *p);

Yeah, at some point this can be changed to

	irq_handler_t trigger_handler;

-- 
With Best Regards,
Andy Shevchenko