Re: [PATCH v5 5/5] iio: mcp9600: Add support for IIR filter

[Ben Collins <bcollins@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7618 bytes --]

On Mon, Aug 18, 2025 at 07:15:39PM -0500, Jonathan Cameron wrote:
> On Sun, 17 Aug 2025 23:59:53 -0400
> Ben Collins <bcollins@kernel.org> wrote:
> 
> > From: Ben Collins <bcollins@watter.com>
> > 
> > MCP9600 supports an IIR filter with 7 levels. Add IIR attribute
> > to allow get/set of this value.
> > 
> > Use a filter_type[none, ema] for enabling the IIR filter.
> Hi Ben,
> 
> A few comments inline. You also need to send an additional patch to update
> the filter_type docs in Documentation/ABI/testing/sysfs-bus-iio

Hi Jonathan,

I just sent a v6 because I was getting too many comments on the
dt-bindings patch.

I'll send a v7 with these changes and anything else that comes up.

More comments below.

> > 
> > Signed-off-by: Ben Collins <bcollins@watter.com>
> > ---
> > @@ -134,6 +160,26 @@ static const struct iio_event_spec mcp9600_events[] = {
> >  		},							       \
> >  	}
> >  
> > +static int mcp9600_get_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan);
> > +static int mcp9600_set_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      unsigned int mode);
> 
> shuffle the code around so you don't need a forward definition..
> There is no particular reason I can see to have get and set later.

The set function, for one, calls mcp9600_config, which comes after the
use of the get/set in the ext_info. I'll see if I can shuffle that
around in the prior patch to avoid this.

> > +
> > +static const struct iio_enum mcp9600_filter_enum = {
> > +	.items = mcp9600_filter_type,
> > +	.num_items = ARRAY_SIZE(mcp9600_filter_type),
> > +	.get = mcp9600_get_filter,
> > +	.set = mcp9600_set_filter,
> > +};
> 
> >  static int mcp9600_read(struct mcp9600_data *data,
> > @@ -191,13 +239,45 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
> >  		if (ret)
> >  			return ret;
> >  		return IIO_VAL_INT;
> > +
> 
> Unrelated changes. White space changes should not be mixed in a patch
> doing anything significant.  If you want to do this, extra patch needed.

Noted.

> >  	case IIO_CHAN_INFO_SCALE:
> >  		*val = 62;
> >  		*val2 = 500000;
> >  		return IIO_VAL_INT_PLUS_MICRO;
> > +
> If you want the extra space put it in previous patch.
> 
> >  	case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> >  		*val = mcp9600_tc_types[data->thermocouple_type];
> >  		return IIO_VAL_CHAR;
> > +
> > +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +		if (data->filter_level == 0)
> 
> Return the current requested value. An error is just going to confuse
> someone who tried to write this before enabling the filter and then
> checked to see if the write was successful.

I could not get a concensus on this. On the one hand, if a user sets a
value here, would they not assume that the filter was enabled? What
about cases where a filter_type can be more than one valid type with
different available coefficients for each? What should it show then?

> > +			return -EINVAL;
> > +
> > +		*val = mcp_iir_coefficients_avail[data->filter_level - 1][0];
> > +		*val2 = mcp_iir_coefficients_avail[data->filter_level - 1][1];
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mcp9600_read_avail(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      const int **vals, int *type, int *length,
> > +			      long mask)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +		if (data->filter_level == 0)
> > +			return -EINVAL;
> Don't block this.  It makes for a confusing interface.
> > +
> > +		*vals = (int *)mcp_iir_coefficients_avail;
> > +		*type = IIO_VAL_INT_PLUS_MICRO;
> > +		*length = 2 * ARRAY_SIZE(mcp_iir_coefficients_avail);
> > +		return IIO_AVAIL_LIST;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > @@ -211,6 +291,7 @@ static int mcp9600_config(struct mcp9600_data *data)
> >  
> >  	cfg  = FIELD_PREP(MCP9600_SENSOR_TYPE_MASK,
> >  			  mcp9600_type_map[data->thermocouple_type]);
> > +	FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->filter_level);
> >  
> >  	ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
> >  	if (ret < 0) {
> > @@ -221,6 +302,79 @@ static int mcp9600_config(struct mcp9600_data *data)
> >  	return 0;
> >  }
> >  
> > +static int mcp9600_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				     struct iio_chan_spec const *chan,
> > +				     long mask)
> > +{
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +		return IIO_VAL_INT_PLUS_MICRO;
> 
> That's the default so you shouldn't need a write_raw_get_fmt for this.

Ok.

> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mcp9600_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +	int i;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +		for (i = 0; i < ARRAY_SIZE(mcp_iir_coefficients_avail); i++) {
> > +			if (mcp_iir_coefficients_avail[i][0] == val &&
> > +			    mcp_iir_coefficients_avail[i][1] == val2)
> > +				break;
> > +		}
> > +
> > +		if (i == ARRAY_SIZE(mcp_iir_coefficients_avail))
> > +			return -EINVAL;
> > +
> > +		data->filter_level = i + 1;
> > +		return mcp9600_config(data);
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mcp9600_get_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +
> > +	if (data->filter_level == 0)
> I'd use a separate enable flag for this.
> 
> > +		return MCP9600_FILTER_TYPE_NONE;
> > +
> > +	return MCP9600_FILTER_TYPE_EMA;
> > +}
> > +
> > +static int mcp9600_set_filter(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      unsigned int mode)
> > +{
> > +	struct mcp9600_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mode) {
> > +	case MCP9600_FILTER_TYPE_NONE:
> > +		data->filter_level = 0;
> > +		return mcp9600_config(data);
> > +
> > +	case MCP9600_FILTER_TYPE_EMA:
> > +		if (data->filter_level == 0) {
> > +			/* Minimum filter by default */
> > +			data->filter_level = 1;
> As above, I'd just let the user write it whenever they like
> (default to 1 on boot) and then they have to see if it is
> set to none to see whether it has an effect.
> 
> > +			return mcp9600_config(data);
> > +		}
> > +		return 0;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> >  {
> >  	if (channel2 == IIO_MOD_TEMP_AMBIENT) {
> > @@ -358,6 +512,9 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
> >  
> >  static const struct iio_info mcp9600_info = {
> >  	.read_raw = mcp9600_read_raw,
> > +	.read_avail = mcp9600_read_avail,
> > +	.write_raw = mcp9600_write_raw,
> > +	.write_raw_get_fmt = mcp9600_write_raw_get_fmt,
> >  	.read_event_config = mcp9600_read_event_config,
> >  	.write_event_config = mcp9600_write_event_config,
> >  	.read_event_value = mcp9600_read_thresh,
> 

-- 
 Ben Collins
 https://libjwt.io
 https://github.com/benmcollins
 --
 3EC9 7598 1672 961A 1139  173A 5D5A 57C7 242B 22CF

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]