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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2025081916-auspicious-centipede-ef9f92@boujee-and-buff>
Date: Tue, 19 Aug 2025 16:41:05 -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 Tue, Aug 19, 2025 at 07:38:37PM -0500, Jonathan Cameron wrote:
> On Tue, 19 Aug 2025 19:28:51 +0100
> Jonathan Cameron <jic23@...nel.org> wrote:
> 
> > On Mon, 18 Aug 2025 16:00:20 -0400
> > Ben Collins <bcollins@...nel.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