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

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

On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:
> Add support for pressure and temperature rising threshold events.

...

> @@ -322,7 +339,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  			.storagebits = 32,
>  			.shift = 12,
>  			.endianness = IIO_BE,
> -		}
> +		},
> +		.event_spec = mpl3115_temp_press_event,
> +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
>  	},
>  	{
>  		.type = IIO_TEMP,
> @@ -338,7 +357,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>  			.storagebits = 16,
>  			.shift = 4,
>  			.endianness = IIO_BE,
> -		}
> +		},

Just a side note below, no action from you required on this!

Yeah, yet another reminder for the comma/not-a-comma choices made initially and
why it's important to follow the advice

> +		.event_spec = mpl3115_temp_press_event,
> +		.num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
>  	},
>  	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };

...

> -	if (!(ret & MPL3115_INT_SRC_DRDY))
> +	if (!(ret & (MPL3115_INT_SRC_DRDY | MPL3115_INT_SRC_PTH |
> +		     MPL3115_INT_SRC_TTH)))

Can we rather keep this split logical?

	if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
		     MPL3115_INT_SRC_DRDY)))

>  		return IRQ_NONE;

...

> -	u8 ctrl_reg1 = data->ctrl_reg1;
> -	u8 ctrl_reg4 = data->ctrl_reg4;
> +	u8 ctrl_reg1, ctrl_reg4;

> +	guard(mutex)(&data->lock);

Why this is moved? Before the access to the data->ctrl* was done without
locking. Is it an existing bug?

> +	ctrl_reg1 = data->ctrl_reg1;
> +	ctrl_reg4 = data->ctrl_reg4;
>  
>  	if (state) {
>  		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
>  		ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
>  	} else {
> -		ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
>  		ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> -	}
>  
> -	guard(mutex)(&data->lock);
> +		if (!ctrl_reg4)
> +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +	}
>  
>  	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);

...

> +static int mpl3115_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      bool state)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 int_en_mask;
> +	u8 ctrl_reg1, ctrl_reg4;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> +		break;
> +	case IIO_TEMP:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	guard(mutex)(&data->lock);

Similar Q here, why do you protect data that was (still is?) not protected before?

> +	ctrl_reg1 = data->ctrl_reg1;
> +	ctrl_reg4 = data->ctrl_reg4;
> +
> +	if (state) {
> +		ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> +		ctrl_reg4 |= int_en_mask;
> +	} else {
> +		ctrl_reg4 &= ~int_en_mask;
> +
> +		if (!ctrl_reg4)
> +			ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +	}
> +
> +	return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> +}

...

> +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, press_pa;
> +	__be16 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, 2,

sizeof() ?

> +						    (u8 *) &tmp);

Here and elsewhere, drop the space between casting and operand.

> +		if (ret < 0)
> +			return ret;
> +
> +		/**

It's not a kernel-doc.

> +		 * Target value for the pressure is
> +		 * 16-bit unsigned value in 2 Pa units
> +		 */
> +		press_pa = be16_to_cpu(tmp) << 1;
> +		*val = press_pa / KILO;
> +		*val2 = (press_pa % KILO) * MILLI;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	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;
> +	}
> +}

...

> +static int mpl3115_write_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);
> +	u8 tmp[2];

Use proper __be16 type.

> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		val = (val * KILO + val2 / MILLI) >> 1;

> +		if (val < 0 || val > 0xffff)
> +			return -EINVAL;

U16_MAX?

> +		tmp[0] = FIELD_GET(GENMASK(15, 8), val);
> +		tmp[1] = FIELD_GET(GENMASK(7, 0), val);
> +
> +		return i2c_smbus_write_i2c_block_data(data->client,
> +						      MPL3115_PRESS_TGT, 2, tmp);

sizeof()

> +	case IIO_TEMP:
> +		if (val < -128 || val > 127)
> +			return -EINVAL;

S8_MIN, S8_MAX ?

> +		return i2c_smbus_write_byte_data(data->client,
> +						 MPL3115_TEMP_TGT, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko