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

[Jonathan Cameron <jic23@kernel.org>]

On 03/07/16 11:37, Jonathan Cameron wrote:
> On 29/06/16 16:41, Srinivas Pandruvada wrote:
>> 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.
> Cool.
> 
> Applied to the togreg branch of iio.git and initially pushed out as
> testing for the autobuilders to play cricket with it...
> 
> Thanks,
> 
> Jonathan
I completely failed to notice that data->dev got dropped in the meantime
fixed up and pushed out.  Please sanity check the testing branch of iio.git.

thanks,

Jonathan
>>
>> Thanks,
>> Srinivas
>>>
>>> Best regards,
>>> Steffen
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>