Re: [PATCH] iio: bmg160: add callbacks for the filter frequency

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

On Wed, 2016-06-29 at 17:13 +0200, Steffen Trumtrar wrote:
> On Wed, May 04, 2016 at 10:55:56AM +0100, Jonathan Cameron wrote:
> > 
> > On 01/05/16 21:02, Jonathan Cameron wrote:
> > > 
> > > On 26/04/16 22:36, Srinivas Pandruvada wrote:
> > > > 
> > > > On Tue, 2016-04-26 at 22:15 +0100, Jonathan Cameron wrote:
> > > > > 
> > > > > 
> > > > > On 26 April 2016 21:25:22 BST, Srinivas Pandruvada
> > > > > <srinivas.pandruva
> > > > > da@linux.intel.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > On Mon, 2016-04-25 at 20:31 +0100, Jonathan Cameron wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 21/04/16 11:49, Steffen Trumtrar wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > The filter frequency and sample rate have a fixed
> > > > > > > > relationship.
> > > > > > > > Only the filter frequency is unique, however.
> > > > > > > > Currently the driver ignores the filter settings for 32
> > > > > > > > Hz and
> > > > > > > > 64 Hz.
> > > > > > > > 
> > > > > > > > This patch adds the necessary callbacks to be able to
> > > > > > > > configure
> > > > > > > > and read the filter setting from sysfs.
> > > > > > > > 
> > > > > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix
> > > > > > > > .de>
> > > > > > > cc'd Srinivas as it's his driver...  Looks superficially
> > > > > > > fine to
> > > > > > > me.
> > > > > > > 
> > > > > > > Jonathan
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Changing the sample rate will result in using the first
> > > > > > > > match
> > > > > > > > and therefore selecting the filter accordingly. Is this
> > > > > > > > a
> > > > > > > > misuse
> > > > > > > > of the ABI and should be handled differently or is this
> > > > > > > > okay?
> > > > > > > > 
> > > > > > This is the reason they were omitted. Now you can't
> > > > > > uniquely set
> > > > > > 100Hz
> > > > > > sampling frequency. Depending on filter it will have
> > > > > > different
> > > > > > results.
> > > > > > 
> > > > > > I think this needs some ABI level changes, where you
> > > > > > display
> > > > > > available
> > > > > > and allow to specify both ODR and Filter to uniquely
> > > > > > select.
> > > > > Unfortunately the moment the ABI allows for combined elements
> > > > > it
> > > > > becomes a
> > > > >  nightmare for complexity. In some devices a single parameter
> > > > > change
> > > > > can
> > > > >  change everything else. There are no simple rules
> > > > > unfortunately. 
> > > > > 
> > > > > The way we avoid this being a problem is that we very
> > > > > deliberately
> > > > > allow any ABI element
> > > > > to be able to result in a change in any other.  This includes
> > > > > changing the
> > > > >  available values list as here. It might be slightly nicer to
> > > > > jump to
> > > > > the nearest
> > > > >  available option though.
> > > > > 
> > > > > An alternative would be to have an interface to fake such
> > > > > changes
> > > > > then
> > > > > apply them atomically if possible. 
> > > > > That level of complexity just does seem warranted here and
> > > > > would
> > > > > still need userspace to check valid ranges as it
> > > > >  pretends to change the state. Hence no real gain....
> > > > > 
> > > > I think we should add some documentation for this driver about
> > > > this.
> > > > They should rather change filer rather than sampling freq to
> > > > have
> > > > unique setting.
> > > whilst that would get around the problem, people are going to be
> > > expecting
> > > to have explicit control of sampling frequency if they see it is
> > > variable for
> > > the part...
> > > 
> > > Tricky unfortunately.
> > So Srinivas, I'm in favour of the patch as it stands.  Have I
> > convinced you?
> So, any conclusion ? :-)
Sorry, I missed this. I am fine with this change.

Thanks,
Srinivas
> 
> Best regards,
> Steffen
>