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 9b017820efc1..0ec47cbeb88c 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 8/18/25 1:32 PM, Ben Collins 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.
>
> 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 9b017820efc1..0ec47cbeb88c 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;
To implement Jonathan's request from v5, drop this error return.
We'll also need a separate bool data->is_filter_enabled field so
that we can keep the last set filter_level even when the filter
is disabled. (i.e. data->filter_level is never == 0).
This way, if you set the filter level, you can enable and disable
the filter via filter_type and still have the same filter level.
> +
> + *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);
And change the logic here to:
FIELD_MODIFY(MCP9600_FILTER_MASK, &cfg, data->is_filter_enabled ?
data->filter_level : 0);
>
> 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;
Don't touch data->filter_level here, just set data->is_filter_enabled = false.
> + return mcp9600_config(data);
> +
> + case MCP9600_FILTER_TYPE_EMA:
> + if (data->filter_level == 0) {
> + /* Minimum filter by default */
> + data->filter_level = 1;
And similar, just set data->is_filter_enabled = true without changing filter_level.
> + 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 Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote: > On 8/18/25 1:32 PM, Ben Collins 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. > > > > Signed-off-by: Ben Collins <bcollins@watter.com> > > --- > > + if (data->filter_level == 0) > > + return -EINVAL; > > To implement Jonathan's request from v5, drop this error return. > We'll also need a separate bool data->is_filter_enabled field so > that we can keep the last set filter_level even when the filter > is disabled. (i.e. data->filter_level is never == 0). > > This way, if you set the filter level, you can enable and disable > the filter via filter_type and still have the same filter level. > Thanks, David. This is exactly what I've implemented, plus the filter_enable attribute. Adding the ABI doc updates as well. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
On 8/19/25 9:11 AM, Ben Collins wrote: > On Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote: >> On 8/18/25 1:32 PM, Ben Collins 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. >>> >>> Signed-off-by: Ben Collins <bcollins@watter.com> >>> --- >>> + if (data->filter_level == 0) >>> + return -EINVAL; >> >> To implement Jonathan's request from v5, drop this error return. >> We'll also need a separate bool data->is_filter_enabled field so >> that we can keep the last set filter_level even when the filter >> is disabled. (i.e. data->filter_level is never == 0). >> >> This way, if you set the filter level, you can enable and disable >> the filter via filter_type and still have the same filter level. >> > > Thanks, David. This is exactly what I've implemented, plus the > filter_enable attribute. > > Adding the ABI doc updates as well. > Don't add the filter_enable attribute. The filter_type attribute already does the job.
On Tue, Aug 19, 2025 at 09:15:23AM -0500, David Lechner wrote: > On 8/19/25 9:11 AM, Ben Collins wrote: > > On Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote: > >> On 8/18/25 1:32 PM, Ben Collins 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. > >>> > >>> Signed-off-by: Ben Collins <bcollins@watter.com> > >>> --- > >>> + if (data->filter_level == 0) > >>> + return -EINVAL; > >> > >> To implement Jonathan's request from v5, drop this error return. > >> We'll also need a separate bool data->is_filter_enabled field so > >> that we can keep the last set filter_level even when the filter > >> is disabled. (i.e. data->filter_level is never == 0). > >> > >> This way, if you set the filter level, you can enable and disable > >> the filter via filter_type and still have the same filter level. > >> > > > > Thanks, David. This is exactly what I've implemented, plus the > > filter_enable attribute. > > > > Adding the ABI doc updates as well. > > > > > Don't add the filter_enable attribute. The filter_type attribute > already does the job. That doesn't solve the problem at hand. An example: - Driver has 3 possible filter_type's, plus "none" - User cats filter_type_available and sees [none, sinc4, sinc5, sinc5+avg] - User cats filter_type and sees "none" - User cats frequency_available: What do they see? - User cats frequency: What do they see? Without filter_enable, [none, ema] driver works just fine. But the above driver does not. -- Ben Collins https://libjwt.io https://github.com/benmcollins -- 3EC9 7598 1672 961A 1139 173A 5D5A 57C7 242B 22CF
On 8/19/25 9:32 AM, Ben Collins wrote: > On Tue, Aug 19, 2025 at 09:15:23AM -0500, David Lechner wrote: >> On 8/19/25 9:11 AM, Ben Collins wrote: >>> On Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote: >>>> On 8/18/25 1:32 PM, Ben Collins 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. >>>>> >>>>> Signed-off-by: Ben Collins <bcollins@watter.com> >>>>> --- >>>>> + if (data->filter_level == 0) >>>>> + return -EINVAL; >>>> >>>> To implement Jonathan's request from v5, drop this error return. >>>> We'll also need a separate bool data->is_filter_enabled field so >>>> that we can keep the last set filter_level even when the filter >>>> is disabled. (i.e. data->filter_level is never == 0). >>>> >>>> This way, if you set the filter level, you can enable and disable >>>> the filter via filter_type and still have the same filter level. >>>> >>> >>> Thanks, David. This is exactly what I've implemented, plus the >>> filter_enable attribute. >>> >>> Adding the ABI doc updates as well. >>> >> >> >> Don't add the filter_enable attribute. The filter_type attribute >> already does the job. > > That doesn't solve the problem at hand. An example: > > - Driver has 3 possible filter_type's, plus "none" > - User cats filter_type_available and sees [none, sinc4, sinc5, sinc5+avg] > - User cats filter_type and sees "none" > - User cats frequency_available: What do they see? > - User cats frequency: What do they see? The ones for the last selected filter before it was changed to "none". If the driver starts in the "none" state a probe, just pick sinc4 as the default. > > Without filter_enable, [none, ema] driver works just fine. But the > above driver does not. > We can wait and see what Jonathan thinks. But if we introduce a new filter_enable attribute, then we need to think about what to do about the ad4080 driver since it was the one that recently introduced the filter_type = "none". Ideally we would change it to work the same so that we are consistent between drivers. But there is always the consideration that we can't go breaking existing ABI.
On Tue, 19 Aug 2025 09:43:48 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 8/19/25 9:32 AM, Ben Collins wrote: > > On Tue, Aug 19, 2025 at 09:15:23AM -0500, David Lechner wrote: > >> On 8/19/25 9:11 AM, Ben Collins wrote: > >>> On Tue, Aug 19, 2025 at 09:05:39AM -0500, David Lechner wrote: > >>>> On 8/18/25 1:32 PM, Ben Collins 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. > >>>>> > >>>>> Signed-off-by: Ben Collins <bcollins@watter.com> > >>>>> --- > >>>>> + if (data->filter_level == 0) > >>>>> + return -EINVAL; > >>>> > >>>> To implement Jonathan's request from v5, drop this error return. > >>>> We'll also need a separate bool data->is_filter_enabled field so > >>>> that we can keep the last set filter_level even when the filter > >>>> is disabled. (i.e. data->filter_level is never == 0). > >>>> > >>>> This way, if you set the filter level, you can enable and disable > >>>> the filter via filter_type and still have the same filter level. > >>>> > >>> > >>> Thanks, David. This is exactly what I've implemented, plus the > >>> filter_enable attribute. > >>> > >>> Adding the ABI doc updates as well. > >>> > >> > >> > >> Don't add the filter_enable attribute. The filter_type attribute > >> already does the job. > > > > That doesn't solve the problem at hand. An example: > > > > - Driver has 3 possible filter_type's, plus "none" > > - User cats filter_type_available and sees [none, sinc4, sinc5, sinc5+avg] > > - User cats filter_type and sees "none" > > - User cats frequency_available: What do they see? > > - User cats frequency: What do they see? > > The ones for the last selected filter before it was changed to "none". > If the driver starts in the "none" state a probe, just pick sinc4 > as the default. That works, or presenting no available frequencies if "none" - empty list. Though check the standard wrapper for available works with a list of size 0. Not something we've done before. Maybe a risk of tripping up some userspace code? Unlike some attribute/controls this one is unlikely to ever be destructive if we have to pass through unusual states. Might have a slightly slower transition to steady state if we are going through something inappropriate briefly. > > > > > Without filter_enable, [none, ema] driver works just fine. But the > > above driver does not. > > > > We can wait and see what Jonathan thinks. But if we introduce a > new filter_enable attribute, then we need to think about what to > do about the ad4080 driver since it was the one that recently > introduced the filter_type = "none". Ideally we would change it > to work the same so that we are consistent between drivers. But > there is always the consideration that we can't go breaking existing > ABI. I was thinking we could paper over it (hence the email I sent 30 seconds before opening this) with a bonus attribute and basing both filter_enable and none setting on the same underlying state. Not that intuitive though as I think more about it. Setting off none would enable the filter when maybe a user was just expecting to be able to see what was available (as will work for any driver not implementing the 'none' value). The ABI has always allowed for interactions like this as sometimes we can't avoid them but here maybe we can. So ignore email of 2 mins ago, David's suggestion works better (with modification for not showing any frequencies when on none possibly?) Anyhow, lets take a little more time on this. I for one shouldn't make any decisions quickly as is clear here! Jonathan
© 2016 - 2025 Red Hat, Inc.