| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250818191539.69e1882a@jic23-huawei>
Date: Mon, 18 Aug 2025 19:15:39 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ben Collins <bcollins@...nel.org>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Ben Collins
<bcollins@...ter.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/5] iio: mcp9600: Add support for IIR filter
On Sun, 17 Aug 2025 23:59:53 -0400
Ben Collins <bcollins@...nel.org> wrote:
> From: Ben Collins <bcollins@...ter.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@...ter.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,
Powered by blists - more mailing lists