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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote:
> 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
> 

But the function returns an irqreturn_t type, no?

> ...
> 
> > +	fwnode = dev_fwnode(data->dev);
> > +	if (!fwnode)
> > +		return -ENODEV;
> 
> Why do you need this? The below will fail anyway.

Because If I don't make this check then fwnode might be garbage and I will
pass garbage to the fwnode_irq_get() function. Or do I miss something?

> 
> > +	irq = fwnode_irq_get(fwnode, 0);
> > +	if (!irq)
> 
> Are you sure this is correct check?
> 
Well, I think yes, because the function return either the Linux IRQ number
on success or a negative errno on failure.

https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987

> > +		return dev_err_probe(data->dev, -ENODEV,
> 
> Shadowed error code.
> 

I am not sure I understand what you mean here. You mean that there is no
chance that the first one will pass and this one will fail?

> > +				     "No interrupt found.\n");
> 
> > +	desc = irq_get_irq_data(irq);
> > +	if (!desc)
> > +		return -EINVAL;
> 
> When may this fail?
> 

I think that this will fail when Linux were not able to actually
register that interrupt.

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

Indeed, thanks!

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

Well, that's true.

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

All the other if statements follow the style that I typed. If I
follow yours, will make it different just for this one, does it
make sense?

Cheers,
Vasilis
> 
> > +}
> 
> ...
> 
> > +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.
> 

True, makes sense.

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

Yes, you are right.

> 
> > +}
> 
> ...
> 
> > +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 ?
> 

ACK.

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

Well, it looks much more beautiful indeed. Thanks again for the feedback
Andy, I really appreciate it a lot!

Cheers,
Vasilis