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

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

On Fri, Aug 23, 2024 at 08:17:13PM +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),

> +				  irqreturn_t (*irq_thread_handler)(int irq, void *p))

irq_handler_t

...

> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;

Why do you need this? The below will fail anyway.

> +	irq = fwnode_irq_get(fwnode, 0);
> +	if (!irq)

Are you sure this is correct check?

> +		return dev_err_probe(data->dev, -ENODEV,

Shadowed error code.

> +				     "No interrupt found.\n");

> +	desc = irq_get_irq_data(irq);
> +	if (!desc)
> +		return -EINVAL;

When may this fail?

> +	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:
> +		return dev_err_probe(data->dev, -EINVAL,
> +				     "Invalid interrupt type specified.\n");
> +	}

> +	data->trig_open_drain = fwnode_property_read_bool(fwnode,
> +							  "int-open-drain");

Better

	data->trig_open_drain =
		fwnode_property_read_bool(fwnode, "int-open-drain");

...

> +static int bmp380_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, BMP380_REG_INT_CONTROL,
> +				 BMP380_INT_CTRL_DRDY_EN,
> +				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN,
> +					    state ? 1 : 0));

				 FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state));

? ( Even <= 80 characters)

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	if (ret)
		dev_err(data->dev, "Could not enable/disable interrupt\n");

	return ret;

?

> +}

...

> +static int bmp380_int_config(struct bmp280_data *data)
> +{
> +	int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
> +				      data->trig_open_drain) |
> +			   FIELD_PREP(BMP380_INT_CTRL_LEVEL,
> +				      data->trig_active_high);

Split these two variables and make the indentation better for int_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 0;

	return ret;

?

> +}

...

> +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 ? 1 : 0));

!!state ?

> +	if (ret) {
> +		dev_err(data->dev, "Could not enable/disable interrupt\n");
> +		return ret;
> +	}
> +
> +	return 0;

	return ret;

?

> +}

...

> +static int bmp580_int_config(struct bmp280_data *data)

Same comments as per above.

...

> +	if (irq > 0) {
> +		if (chip_id == BMP180_CHIP_ID) {
> +			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
> +			if (ret)
> +				return ret;
> +		}
> +		if (data->chip_info->trigger_probe) {
> +			ret = data->chip_info->trigger_probe(indio_dev);
> +			if (ret)
> +				return ret;
> +		}
>  	}

Can be

	if (irq > 0) {
		if (chip_id == BMP180_CHIP_ID)
			ret = bmp085_fetch_eoc_irq(dev, name, irq, data);
		if (data->chip_info->trigger_probe)
			ret = data->chip_info->trigger_probe(indio_dev);
		if (ret)
			return ret;
	}

-- 
With Best Regards,
Andy Shevchenko