Re: [PATCH v5 5/5] iio: mcp9600: Add support for IIR filter

[Ben Collins <bcollins@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]