On Mon, Aug 18, 2025 at 07:15:39PM -0500, Jonathan Cameron wrote: > On Sun, 17 Aug 2025 23:59:53 -0400 > Ben Collins wrote: > > > From: Ben Collins > > > > 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 > > --- > > @@ -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