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

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

On Mon, Sep 02, 2024 at 08:42:21PM +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.

...

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

Would it make sense (note, I do *not* know the correct answer!) to have
something like

struct foo {
	const struct iio_trigger_ops *trigger_ops;
	int (*int_config)(struct bmp280_data *data);
	irq_handler_t irq_thread_handler;
};

and pass it around?

Also int_config sounds non-related to interrupt, however it's about interrupt
pin, right? perhaps name it differently here?

E.g.,

	interrupt_pin_config

?

> +{
> +	struct bmp280_data *data = iio_priv(indio_dev);
> +	struct device *dev = data->dev;
> +	struct fwnode_handle *fwnode;
> +	u32 irq_type;
> +	int ret, irq;
> +
> +	irq = fwnode_irq_get(dev_fwnode(dev), 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "No interrupt found.\n");
> +
> +	irq_type = irq_get_trigger_type(irq);
> +	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(dev, -EINVAL, "Invalid interrupt type specified.\n");
> +	}
> +
> +	data->trig_open_drain =
> +		fwnode_property_read_bool(fwnode, "int-open-drain");


Where do you initialise fwnode?

> +	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(dev, ret, "request irq failed.\n");

IRQ

> +	ret = devm_iio_trigger_register(data->dev, data->trig);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "iio trigger register failed.\n");
> +
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	return 0;
> +}

...

> +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));
> +	if (ret)
> +		dev_err(data->dev,
> +			"Could not %s interrupt.\n", str_enable_disable(state));
> +	return ret;

Somewhere above (or in another patch) you used the style with 'return 0;' at
the end. Please, check the resulting code for the consistency and choose one
style for all.

> +}

-- 
With Best Regards,
Andy Shevchenko