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

[Jonathan Cameron <jic23@kernel.org>]

> > >  	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.

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.

ABI design is a pain sometimes.

Thanks,

Jonathan