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.
Signed-off-by: Ben Collins <bcollins@watter.com>
---
drivers/iio/temperature/mcp9600.c | 157 ++++++++++++++++++++++++++++++
1 file changed, 157 insertions(+)
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index fa382a988a991..54bc657460e5d 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -31,6 +31,7 @@
#define MCP9600_STATUS_ALERT(x) BIT(x)
#define MCP9600_SENSOR_CFG 0x05
#define MCP9600_SENSOR_TYPE_MASK GENMASK(6, 4)
+#define MCP9600_FILTER_MASK GENMASK(2, 0)
#define MCP9600_ALERT_CFG1 0x08
#define MCP9600_ALERT_CFG(x) (MCP9600_ALERT_CFG1 + (x - 1))
#define MCP9600_ALERT_CFG_ENABLE BIT(0)
@@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
[THERMOCOUPLE_TYPE_R] = 'R',
};
+enum mcp9600_filter {
+ MCP9600_FILTER_TYPE_NONE,
+ MCP9600_FILTER_TYPE_EMA,
+};
+
+static const char * const mcp9600_filter_type[] = {
+ [MCP9600_FILTER_TYPE_NONE] = "none",
+ [MCP9600_FILTER_TYPE_EMA] = "ema",
+};
+
+static const int mcp_iir_coefficients_avail[7][2] = {
+ /* Level 0 is no filter */
+ { 0, 524549 },
+ { 0, 243901 },
+ { 0, 119994 },
+ { 0, 59761 },
+ { 0, 29851 },
+ { 0, 14922 },
+ { 0, 7461 },
+};
+
static const struct iio_event_spec mcp9600_events[] = {
{
.type = IIO_EV_TYPE_THRESH,
@@ -118,7 +140,11 @@ static const struct iio_event_spec mcp9600_events[] = {
.address = MCP9600_HOT_JUNCTION, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = \
+ BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
+ .ext_info = mcp9600_ext_filter, \
.event_spec = &mcp9600_events[hj_ev_spec_off], \
.num_event_specs = hj_num_ev, \
}, \
@@ -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);
+
+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 const struct iio_chan_spec_ext_info mcp9600_ext_filter[] = {
+ IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &mcp9600_filter_enum),
+ IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
+ &mcp9600_filter_enum),
+ { }
+};
+
static const struct iio_chan_spec mcp9600_channels[][2] = {
MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
@@ -161,6 +207,8 @@ struct mcp_chip_info {
struct mcp9600_data {
struct i2c_client *client;
u32 thermocouple_type;
+ /* Filter enabled is 1-7, with 0 being off (filter_type none) */
+ u8 filter_level;
};
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;
+
case IIO_CHAN_INFO_SCALE:
*val = 62;
*val2 = 500000;
return IIO_VAL_INT_PLUS_MICRO;
+
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 -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;
+
+ *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;
+ 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)
+ 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;
+ 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,
--
2.39.5On 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
Thanks,
Jonathan
>
> Signed-off-by: Ben Collins <bcollins@watter.com>
> ---
> drivers/iio/temperature/mcp9600.c | 157 ++++++++++++++++++++++++++++++
> 1 file changed, 157 insertions(+)
>
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index fa382a988a991..54bc657460e5d 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -31,6 +31,7 @@
> #define MCP9600_STATUS_ALERT(x) BIT(x)
> #define MCP9600_SENSOR_CFG 0x05
> #define MCP9600_SENSOR_TYPE_MASK GENMASK(6, 4)
> +#define MCP9600_FILTER_MASK GENMASK(2, 0)
> #define MCP9600_ALERT_CFG1 0x08
> #define MCP9600_ALERT_CFG(x) (MCP9600_ALERT_CFG1 + (x - 1))
> #define MCP9600_ALERT_CFG_ENABLE BIT(0)
> @@ -94,6 +95,27 @@ static const int mcp9600_tc_types[] = {
> [THERMOCOUPLE_TYPE_R] = 'R',
> };
>
> +enum mcp9600_filter {
> + MCP9600_FILTER_TYPE_NONE,
> + MCP9600_FILTER_TYPE_EMA,
> +};
> +
> +static const char * const mcp9600_filter_type[] = {
> + [MCP9600_FILTER_TYPE_NONE] = "none",
> + [MCP9600_FILTER_TYPE_EMA] = "ema",
> +};
> +
> +static const int mcp_iir_coefficients_avail[7][2] = {
> + /* Level 0 is no filter */
> + { 0, 524549 },
> + { 0, 243901 },
> + { 0, 119994 },
> + { 0, 59761 },
> + { 0, 29851 },
> + { 0, 14922 },
> + { 0, 7461 },
> +};
> +
> static const struct iio_event_spec mcp9600_events[] = {
> {
> .type = IIO_EV_TYPE_THRESH,
> @@ -118,7 +140,11 @@ static const struct iio_event_spec mcp9600_events[] = {
> .address = MCP9600_HOT_JUNCTION, \
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
Probably just one space before \ is the most consistent choice.
> BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
> + .ext_info = mcp9600_ext_filter, \
> .event_spec = &mcp9600_events[hj_ev_spec_off], \
> .num_event_specs = hj_num_ev, \
> }, \
> @@ -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.
> +
> +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.
> 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.
> + 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.
> + 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,
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
> > > 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? So I was thinking of this like other things with 'enables' such as events. For those you always set the value first. They don't really have a type field though (well they do but the ABI allows multiple at once unlike filters so we end up with a quite different looking ABI). Agreed it gets challenging with multiple filter types. If it weren't for advertising the range I'd suggest just stashing whatever was written and then mapping it to nearest possible when the filter type is set. That's what the ad7124 does for changing between filters anyway though oddly it doesn't seem to have a control for filter type. This is a good argument against the whole 'none' value for filter type - that's not much used so we could deprecate it for new drivers. I'm not particularly keen on filter_enable but seems we are coming back around to that option to avoid this corner case. Alternative being what you have here which isn't great for ease of use. So for next version let's go for that. Make sure to include Documentation in a separate patch though so it's easy to see an poke holes in. ABI design is a pain sometimes. Thanks, Jonathan
On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote: > > > > > 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? > > So I was thinking of this like other things with 'enables' such as events. > For those you always set the value first. They don't really have a type > field though (well they do but the ABI allows multiple at once unlike filters > so we end up with a quite different looking ABI). > > Agreed it gets challenging with multiple filter types. If it weren't for > advertising the range I'd suggest just stashing whatever was written and > then mapping it to nearest possible when the filter type is set. > That's what the ad7124 does for changing between filters anyway > though oddly it doesn't seem to have a control for filter type. > > This is a good argument against the whole 'none' value for filter type > - that's not much used so we could deprecate it for new drivers. > > I'm not particularly keen on filter_enable but seems we are coming back > around to that option to avoid this corner case. Alternative being what > you have here which isn't great for ease of use. I'm somewhat wondering if the filter frequency and frequency_available attributes should not even show in sysfs unless the filter_type was something other than "none". > So for next version let's go for that. Make sure to include Documentation > in a separate patch though so it's easy to see an poke holes in. Just to make sure I understand, you'd like to see a filter_enable attribute and filter_type would not contain "none", then frequency and frequency_available would always show something for whatever was in filter_type? > ABI design is a pain sometimes. The epitome of being able to paint yourself into a corner. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
On Mon, 18 Aug 2025 16:00:20 -0400 Ben Collins <bcollins@kernel.org> wrote: > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote: > > > > > > > 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? > > > > So I was thinking of this like other things with 'enables' such as events. > > For those you always set the value first. They don't really have a type > > field though (well they do but the ABI allows multiple at once unlike filters > > so we end up with a quite different looking ABI). > > > > Agreed it gets challenging with multiple filter types. If it weren't for > > advertising the range I'd suggest just stashing whatever was written and > > then mapping it to nearest possible when the filter type is set. > > That's what the ad7124 does for changing between filters anyway > > though oddly it doesn't seem to have a control for filter type. > > > > This is a good argument against the whole 'none' value for filter type > > - that's not much used so we could deprecate it for new drivers. > > > > I'm not particularly keen on filter_enable but seems we are coming back > > around to that option to avoid this corner case. Alternative being what > > you have here which isn't great for ease of use. > > I'm somewhat wondering if the filter frequency and frequency_available > attributes should not even show in sysfs unless the filter_type was > something other than "none". > I'm not keen on that and trying to bolt is_visible into the mess of how we generate attributes would be hard and actual add and remove of attributes is horrible for races with userspace. > > So for next version let's go for that. Make sure to include Documentation > > in a separate patch though so it's easy to see an poke holes in. > > Just to make sure I understand, you'd like to see a filter_enable > attribute and filter_type would not contain "none", then frequency and > frequency_available would always show something for whatever was in > filter_type? Yes. I think that is best way forwards. If we want to retrofit the one user of none to support the new ABI as well it should be easy to do. > > > ABI design is a pain sometimes. > > The epitome of being able to paint yourself into a corner. > Yup. J
On Tue, Aug 19, 2025 at 07:28:51PM -0500, Jonathan Cameron wrote: > On Mon, 18 Aug 2025 16:00:20 -0400 > Ben Collins <bcollins@kernel.org> wrote: > > > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote: > > > > > > > > > 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? > > > > > > So I was thinking of this like other things with 'enables' such as events. > > > For those you always set the value first. They don't really have a type > > > field though (well they do but the ABI allows multiple at once unlike filters > > > so we end up with a quite different looking ABI). > > > > > > Agreed it gets challenging with multiple filter types. If it weren't for > > > advertising the range I'd suggest just stashing whatever was written and > > > then mapping it to nearest possible when the filter type is set. > > > That's what the ad7124 does for changing between filters anyway > > > though oddly it doesn't seem to have a control for filter type. > > > > > > This is a good argument against the whole 'none' value for filter type > > > - that's not much used so we could deprecate it for new drivers. > > > > > > I'm not particularly keen on filter_enable but seems we are coming back > > > around to that option to avoid this corner case. Alternative being what > > > you have here which isn't great for ease of use. > > > > I'm somewhat wondering if the filter frequency and frequency_available > > attributes should not even show in sysfs unless the filter_type was > > something other than "none". > > > I'm not keen on that and trying to bolt is_visible into the mess of how > we generate attributes would be hard and actual add and remove of attributes > is horrible for races with userspace. > > > > So for next version let's go for that. Make sure to include Documentation > > > in a separate patch though so it's easy to see an poke holes in. > > > > Just to make sure I understand, you'd like to see a filter_enable > > attribute and filter_type would not contain "none", then frequency and > > frequency_available would always show something for whatever was in > > filter_type? > > Yes. I think that is best way forwards. If we want to retrofit the one > user of none to support the new ABI as well it should be easy to do. I'm good with that. I'll get a patch to update the ABI, add filter_enable usage in mcp9600 IIR patch, and provide another patch for ad4080 to convert to this. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
On Tue, 19 Aug 2025 19:28:51 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 18 Aug 2025 16:00:20 -0400 > Ben Collins <bcollins@kernel.org> wrote: > > > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote: > > > > > > > > > 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? > > > > > > So I was thinking of this like other things with 'enables' such as events. > > > For those you always set the value first. They don't really have a type > > > field though (well they do but the ABI allows multiple at once unlike filters > > > so we end up with a quite different looking ABI). > > > > > > Agreed it gets challenging with multiple filter types. If it weren't for > > > advertising the range I'd suggest just stashing whatever was written and > > > then mapping it to nearest possible when the filter type is set. > > > That's what the ad7124 does for changing between filters anyway > > > though oddly it doesn't seem to have a control for filter type. > > > > > > This is a good argument against the whole 'none' value for filter type > > > - that's not much used so we could deprecate it for new drivers. > > > > > > I'm not particularly keen on filter_enable but seems we are coming back > > > around to that option to avoid this corner case. Alternative being what > > > you have here which isn't great for ease of use. > > > > I'm somewhat wondering if the filter frequency and frequency_available > > attributes should not even show in sysfs unless the filter_type was > > something other than "none". > > > I'm not keen on that and trying to bolt is_visible into the mess of how > we generate attributes would be hard and actual add and remove of attributes > is horrible for races with userspace. > > > > So for next version let's go for that. Make sure to include Documentation > > > in a separate patch though so it's easy to see an poke holes in. > > > > Just to make sure I understand, you'd like to see a filter_enable > > attribute and filter_type would not contain "none", then frequency and > > frequency_available would always show something for whatever was in > > filter_type? > > Yes. I think that is best way forwards. If we want to retrofit the one > user of none to support the new ABI as well it should be easy to do. > Ignore that. David convinced me otherwise. Lets take this a bit slow and discuss it fully in other branch of this thread. J > > > > > ABI design is a pain sometimes. > > > > The epitome of being able to paint yourself into a corner. > > > Yup. > > J >
On Tue, Aug 19, 2025 at 07:38:37PM -0500, Jonathan Cameron wrote: > On Tue, 19 Aug 2025 19:28:51 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Mon, 18 Aug 2025 16:00:20 -0400 > > Ben Collins <bcollins@kernel.org> wrote: > > > > > On Mon, Aug 18, 2025 at 08:10:35PM -0500, Jonathan Cameron wrote: > > > > > > > > > > > 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? > > > > > > > > So I was thinking of this like other things with 'enables' such as events. > > > > For those you always set the value first. They don't really have a type > > > > field though (well they do but the ABI allows multiple at once unlike filters > > > > so we end up with a quite different looking ABI). > > > > > > > > Agreed it gets challenging with multiple filter types. If it weren't for > > > > advertising the range I'd suggest just stashing whatever was written and > > > > then mapping it to nearest possible when the filter type is set. > > > > That's what the ad7124 does for changing between filters anyway > > > > though oddly it doesn't seem to have a control for filter type. > > > > > > > > This is a good argument against the whole 'none' value for filter type > > > > - that's not much used so we could deprecate it for new drivers. > > > > > > > > I'm not particularly keen on filter_enable but seems we are coming back > > > > around to that option to avoid this corner case. Alternative being what > > > > you have here which isn't great for ease of use. > > > > > > I'm somewhat wondering if the filter frequency and frequency_available > > > attributes should not even show in sysfs unless the filter_type was > > > something other than "none". > > > > > I'm not keen on that and trying to bolt is_visible into the mess of how > > we generate attributes would be hard and actual add and remove of attributes > > is horrible for races with userspace. > > > > > > So for next version let's go for that. Make sure to include Documentation > > > > in a separate patch though so it's easy to see an poke holes in. > > > > > > Just to make sure I understand, you'd like to see a filter_enable > > > attribute and filter_type would not contain "none", then frequency and > > > frequency_available would always show something for whatever was in > > > filter_type? > > > > Yes. I think that is best way forwards. If we want to retrofit the one > > user of none to support the new ABI as well it should be easy to do. > > > > Ignore that. David convinced me otherwise. Lets take this a bit slow > and discuss it fully in other branch of this thread. Maybe a working solution would help the conversation to see how it would look. I think I can rework ad4080 so that it keeps "none" filter_type, but also can make use of filter_enable. That way it can continue to work for users while still providing a better interface for anything else moving forward. I can send this as a separate RFC series from my current mcp9600 changes to break this out. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
On 8/18/25 1:47 PM, Ben Collins wrote: > 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. Actually, folks will be happier if you slow down a bit. General advice is only submit one revision per week since some people only have time to review once per week. If you are really in a hurry, there should still be no more than one revision per day. Otherwise, it is really hard for reviewers to keep up. As it is, the subject of what I presume is v6 still says v5 in the cover letter and doesn't have a version in the rest of the patches. And there are still some of the same problems with the devicetree patch that didn't get addressed. If you slow down a bit and take a little more care before firing off the next one, it will likely be better received. > > I'll send a v7 with these changes and anything else that comes up. >
On Mon, Aug 18, 2025 at 01:59:34PM -0500, David Lechner wrote: > On 8/18/25 1:47 PM, Ben Collins wrote: > > 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. > > Actually, folks will be happier if you slow down a bit. General > advice is only submit one revision per week since some people only > have time to review once per week. I appreciate the info. Admittedly, when I was more prolific with kernel development, the pipline and protocol was much leaner, so I'm not used to the more recent "norm" quite yet. Not complaining, just noting. I'll get there. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
© 2016 - 2025 Red Hat, Inc.