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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

...
> +static int mpl3115_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct mpl3115_data *data = iio_priv(indio_dev);
> +	u8 int_en_mask;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> +		break;
Usual convention in IIO drivers is to return early whenever possible
		return !!(data->ctrl_reg4 & MPL3115_CTRL4_INT_EN_PTH);

> +	case IIO_TEMP:
> +		int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> +		break;
same here
		return !!(data->ctrl_reg4 & MPL3115_CTRL4_INT_EN_TTH);

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return !!(data->ctrl_reg4 & int_en_mask);
> +}
> +
...
> +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,
> +						    (u8 *) &tmp);
> +		if (ret < 0)
> +			return ret;
> +
> +		/**
> +		 * 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;

Looks like this is intended to provide the value in kilopascal. Though, as
specified by pressure_raw ABI [1], we only get to kilopascal after applying
channel _offset and _scale. So, this would have to use _input ABI [2], or
provide a value that can be scaled to kilopascal.
If I'm not missing anything, 

		*val = be16_to_cpu(tmp) << 1;
		return IIO_VAL_INT;

would comply with the _raw ABI.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n397
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n1061

> +	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];
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		val = (val * KILO + val2 / MILLI) >> 1;
same here. You seem to want to use the _input ABI.
Or, if you chose to use the _raw ABI, take the raw threshold value
		val = val >> 1;
		if (val < 0 || val > 0xffff)
			return -EINVAL;
...

Might also use a local variable to hold the adjusted val >> 1 pressure threshold.

You may also add docs for those. e.g.
What:		/sys/.../iio:deviceX/events/in_pressure_thresh_rising_en
and
What:		/sys/.../events/in_pressure_raw_thresh_rising_value
to ABI documentation.
The ABI doc would probably be best appreciated in a separate patch.

> +
> +		if (val < 0 || val > 0xffff)
> +			return -EINVAL;
> +
> +		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);

With best regards,
Marcelo