Message-ID: <542FC15D.3070503@kernel.org>
Date:	Sat, 04 Oct 2014 10:43:57 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Michael Welling <mwelling@...e.org>, Felipe Balbi <balbi@...com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-iio@...r.kernel.org, pmeerw@...erw.net,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light
 sensor

On 04/10/14 04:17, Michael Welling wrote:
> On Thu, Oct 02, 2014 at 01:05:53PM -0500, Felipe Balbi wrote:
>> Hi,
>>
>> On Tue, Sep 30, 2014 at 04:22:04PM -0500, Felipe Balbi wrote:
>>> On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
>>>> On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
>>>>>>>>> Alright, this is already ridiculous. Andrew, if I resend the patch can
>>>>>>>>> you apply it since maintainer has been ignoring this thread anyway ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Do you reall think this is the best way to approach this?
>>>>>>>
>>>>>>> when maintainer doesn't respond for weeks, yeah! Sure it is.
>>>>>>>
>>>>>>>> Perhaps it would be better to explain what each field of the
>>>>>>>> configuration register does and then we can move on.
>>>>>>>
>>>>>>> perhaps Jonathan could tell me exactly what he wants because I can't
>>>>>>> handle back-and-forth anymore. Specially when he complains about stuff
>>>>>>> he asked me to modify himself.
>>>>>>>
>>>>>>>> In particular the OPT3001_CONFIGURATION_L field needs to be explained
>>>>>>>> such that the ABI can be properly applied.
>>>>>>>>
>>>>>>>> Looking at the docs for the Windows demo program the field is associated
>>>>>>>> with a latch configuration. What does this bit field actually do?
>>>>>>
>>>>>> Still no technical information. Without all the facts how can you expect
>>>>>> him to tell you what he wants?
>>>>>
>>>>> yeah, because clearly he doesn't know himself, right ?
>>>>
>>>> Could you explain how it works to me then?
>>>
>>> checking how much of the docs I can expose now, gimme a couple days.
>>
>> alright, here's a snippet of what's on preliminary docs:
>>
Firstly, thanks Felipe for going through the process to get permission
to release the snippets.  I know this can be a real pain to do.
> 
> Firstly, there is logical error in the latest code.
> The hysteresis setting is 0 not 1.
> 
> +       if (opt->hysteresis)
> +               reg |= OPT3001_CONFIGURATION_L;
> +       else
> +               reg &= ~OPT3001_CONFIGURATION_L;
> 
> 
>> Latch field (read or write).
>> The latch field controls the functionality of the interrupt reporting
>> mechanisms: the INT pin, the flag high field (FH), and flag low field (FL).
>> This bit selects the reporting style between a latched window-style comparison
>> and a transparent hysteresis-style comparison.
>>
>> 0 = The device functions in transparent hysteresis-style comparison operation,
>> where the three interrupt reporting mechanisms directly reflect the comparison
>> of the result register with the high- and low-limit registers with no
>> user-controlled clearing event. See the Interrupt Operation, INT Pin, and
>> Interrupt Reporting Mechanisms section for further details.
>>
>> 1 = The device functions in latched window-style comparison operation, latching
>> the interrupt reporting mechanisms until a user-controlled clearing event.
>>
> 
> How is the interrupt cleared in mode 1 in the latest version of your
> code?
> 
>> [...]
>>
>> 7.4.2.2 Transparent Hysteresis-Style Comparison Mode
>> The transparent hysteresis-style comparison mode is typically used when a
>> single digital signal is desired that indicates whether the input light is
>> higher than or lower than a light level of interest. If the result register is
>> higher than the high-limit register for a consecutive number of events set by
>> the fault count field, the INT line is set to active, the flag high field is
>> set to 1, and the flag low field is set to 0. If the result register is lower
>> than the lowlimit register for a consecutive number of events set by the fault
>> count field, the INT line is set to inactive, the flag low field is set to 1,
>> and the flag high field is set to 0. The INT pin and flag high and flag low
>> fields do not change state with configuration reads and writes. The INT pin and
>> flag fields continually report the appropriate comparison of the light to the
>> low-limit and high-limit registers. The device does not respond to the SMBus
>> alert response protocol while in either of the two transparent comparison modes
>> (configuration register, latch field = 0).
>>
> 
> The ABI confusion starts here.
> 
> You are dealing with a mode enable and IIO_EV_INFO_HYSTERESIS is associated
> with a the hysteresis level of a threshold event.
> 
> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L605
> 
> You are going to have abstract the register access to conform to the ABI or add
> to the ABI as Jonathan suggests.

Hysteresis in IIO is the equivalent of the action of a Schmitt trigger
Clearly there are other more complex forms of hysteresis, though a
purely temporal one is an unusual use of the term.

As I read this description what we have here corresponds to
*_period though that ABI requires the units to be seconds rather
than samples hence a conversion will be necessary.

I guess that the OPT3001_CONFIGURATION_FC_MASK relates to this
fault count? Hence your *_period controls IIO_EV_INFO_PERIOD
will presumably write that.

Michael has noted the other complexity above.  In non hysteresis
mode we have a conventional interrupt that can be cleared one
the driver has dealt with it. That simple interrupt corresponds
to a period of 0 (though that might also be a valid value for your
fault count field).

For these others cases that 'interrupt' line has become a simple
indicator of state. What state is it in if the value is between the
two thresholds?

Anyhow, you handle this by triggering on either rising or falling edge,
thus catching any change of state but not causing an interrupt storm.
This works in most cases but would it not miss a change of state from
initially between the thresholds to then being outside of them?
Perhaps I'm missing something here.

Thanks,

Jonathan



> 
>> -- 
>> balbi
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists