Re: [PATCH v3 2/3] iio: mpl3115: add threshold events support

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

On Wed, Nov 05, 2025 at 10:56:14AM +0100, Antoni Pokusinski wrote:
> Add support for pressure and temperature rising threshold events. For
> both channels *_en and *_value (in raw units) attributes are exposed.
> 
> Since in write_event_config() the ctrl_reg1.active and ctrl_reg4
> are modified, accessing the data->ctrl_reg{1,4} in set_trigger_state()
> and write_event_config() needs to be now guarded by data->lock.
> Otherwise, it would be possible that 2 concurrent threads executing
> these functions would access the data->ctrl_reg{1,4} at the same time
> and then one would overwrite the other's result.

...

> static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)

>  	struct iio_dev *indio_dev = private;
>  	struct mpl3115_data *data = iio_priv(indio_dev);
>  	int ret;
> +	u8 val_press[3];
> +	__be16 val_temp;

s/_temp/$SOMETHING meaningful/ ?

>  	ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
>  	if (ret < 0)
>  		return IRQ_HANDLED;
>  
> -	if (!(ret & MPL3115_INT_SRC_DRDY))
> +	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> +		     MPL3115_INT_SRC_DRDY)))
>  		return IRQ_NONE;
>  
> -	iio_trigger_poll_nested(data->drdy_trig);
> +	if (ret & MPL3115_INT_SRC_DRDY)
> +		iio_trigger_poll_nested(data->drdy_trig);
> +
> +	if (ret & MPL3115_INT_SRC_PTH) {
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_PRESSURE, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_RISING),
> +						    iio_get_time_ns(indio_dev));
> +
> +		/* Reset the SRC_PTH bit in INT_SOURCE */
> +		i2c_smbus_read_i2c_block_data(data->client,
> +					      MPL3115_OUT_PRESS,
> +					      sizeof(val_press), val_press);
> +	}
> +
> +	if (ret & MPL3115_INT_SRC_TTH) {
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_RISING),
> +						    iio_get_time_ns(indio_dev));
> +
> +		/* Reset the SRC_TTH bit in INT_SOURCE */
> +		i2c_smbus_read_i2c_block_data(data->client,
> +					      MPL3115_OUT_TEMP,
> +					      2, (u8 *)&val_temp);

Why not sizeof() here ?

> +	}
>  
>  	return IRQ_HANDLED;
>  }

...

> +static int mpl3115_read_thresh(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info,
> +			       int *val, int *val2)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	int ret;
> +	__be16 tmp;

Also name can be improved. And notice confusion between temp
(temporary?  temperature?) and tmp.

> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		ret = i2c_smbus_read_i2c_block_data(data->client,
> +						    MPL3115_PRESS_TGT,
> +						    sizeof(tmp), (u8 *)&tmp);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * Target value for the pressure is 16-bit unsigned value,
> +		 * expressed in 2 Pa units
> +		 */
> +		*val = be16_to_cpu(tmp) << 1;
> +
> +		return IIO_VAL_INT;
> +	case IIO_TEMP:
> +		ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Target value for the temperature is 8-bit 2's complement */
> +		*val = sign_extend32(ret, 7);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko