| 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: <2025081814-grumpy-prawn-ef1a0e@boujee-and-buff>
Date: Mon, 18 Aug 2025 14:47:16 -0400
From: Ben Collins <bcollins@...nel.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/5] iio: mcp9600: Add support for IIR filter
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@...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
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@...ter.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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists