Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 11619 bytes --]

Alright, this is already ridiculous. Andrew, if I resend the patch can
you apply it since maintainer has been ignoring this thread anyway ?

On Fri, Sep 26, 2014 at 11:19:59PM -0500, Felipe Balbi wrote:
> ping
> 
> On Thu, Sep 25, 2014 at 05:16:19PM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > > > > > ping
> > > > > > 
> > > > > > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > > > > > ping
> > > > > > > 
> > > > > > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > > > > > ping
> > > > > > > > 
> > > > > > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > > > > > ping
> > > > > > > > > 
> > > > > > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > > > > > On 02/09/14 16:17, Felipe Balbi wrote:
> > > > > > > > > > > > TI's opt3001 light sensor is a simple and yet powerful
> > > > > > > > > > > > little device. The device provides 99% IR rejection,
> > > > > > > > > > > > Automatic full-scale, very low power consumption and
> > > > > > > > > > > > measurements from 0.01 to 83k lux.
> > > > > > > > > > > > 
> > > > > > > > > > > > This patch adds support for that device using the IIO
> > > > > > > > > > > > framework.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > > 
> > > > > > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > > > > > Hi Felipe,
> > > > > > > > > > > 
> > > > > > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > > > > > hysteresis bit again!)
> > > > > > > > > > 
> > > > > > > > > > right, this is starting to become way too much headache for such a
> > > > > > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > > > > > window.
> > > > > > > > > > 
> > > > > > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > > > > > 
> > > > > > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > > > > > wrong).
> > > > > > > > > > > 
> > > > > > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > > > > > threshold.
> > > > > > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > > > > > past 10.
> > > > > > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > > > > > not get an event.
> > > > > > > > > > > 
> > > > > > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > > > > > 
> > > > > > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > > > > > previous threshold?
> > > > > > > > > > > 
> > > > > > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > > > > > trigger an event at:
> > > > > > > > > > > 
> > > > > > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > > > > > 
> > > > > > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > > > > > brightness may need adjusting.
> > > > > > > > > > > 
> > > > > > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > > > > > something with sysfs attr naming along the lines of
> > > > > > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > > > > > 
> > > > > > > > > > > etc?
> > > > > > > > > > 
> > > > > > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > > > > > change requests.
> > > > > > > > > > 
> > > > > > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > > > > > much)
> > > > > > > > > > 
> > > > > > > > > > > > +#define OPT3001_RESULT		0x00
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION	0x01
> > > > > > > > > > > > +#define OPT3001_LOW_LIMIT	0x02
> > > > > > > > > > > > +#define OPT3001_HIGH_LIMIT	0x03
> > > > > > > > > > > > +#define OPT3001_MANUFACTURER_ID	0x7e
> > > > > > > > > > > > +#define OPT3001_DEVICE_ID	0x7f
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_RN_MASK (0xf << 12)
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_RN_AUTO (0xc << 12)
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_CT	BIT(11)
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_M_MASK	(3 << 9)
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_M_SINGLE (1 << 9)
> > > > > > > > > > > > +#define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */
> > > > > > > > > > > > +
> > > > > > > > > > > 
> > > > > > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > > > > > 
> > > > > > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > > > > > documentation easier. It's better to have something that matches
> > > > > > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > > > > > Assuming I'll still remember what I meant.
> > > > > > > > > > 
> > > > > > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > > > > > +	int ret;
> > > > > > > > > > > > +	u16 reg;
> > > > > > > > > > > > +
> > > > > > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > > > > > +
> > > > > > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > > > > +		return ret;
> > > > > > > > > > > > +	}
> > > > > > > > > > > > +
> > > > > > > > > > > > +	reg = ret;
> > > > > > > > > > > > +	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
> > > > > > > > > > > > +
> > > > > > > > > > > > +	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
> > > > > > > > > > > > +			reg);
> > > > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > > > +		dev_err(opt->dev, "failed to write register %02x\n",
> > > > > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > > > > +		return ret;
> > > > > > > > > > > > +	}
> > > > > > > > > > > > +
> > > > > > > > > > > > +	iio_device_free(iio);
> > > > > > > > > > >
> > > > > > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > > > > > 
> > > > > > > > > > here's the original driver:
> > > > > > > > > > 
> > > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > > > > > 
> > > > > > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > > > > > 
> > > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > > > > > 
> > > > > > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > > > > > 
> > > > > > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > > > > > 
> > > > > > > > > > cheers
> > > > > > > > > > 
> > > > > > > > > > -- 
> > > > > > > > > > balbi
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > -- 
> > > > > > > > > balbi
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > balbi
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -- 
> > > > > > > balbi
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

Hi Andrew.

I would prefer to assume there is some issue with emails from myself
to Felipe than he is completely ignoring the requests to actually
respond in a technical fashion to my review.

http://marc.info/?l=linux-iio&m=141077611429500&w=2
was my last request for clarification on ABI usage within driver.
Given Felipe's last substantial email I think the lack of
communications may be a generous interpretation...
http://marc.info/?l=linux-iio&m=141075853024132&w=2

Harmut also emailed to see if Felipe was having communication problems
and does not seem to have received a public response.

The driver as proposed, under review with the information available,
does not appear to conform to the published ABI and without further
clarification or restriction to those elements within the ABI (as
suggested as an easy way of moving forward), should not be applied.

Rather than getting worked up, I'll leave interested readers to draw
their own conclusions from the email thread.

Sorry that this is wasting everyone's time.  I am quite happy to
either personally respond to any technical response to the review
with the assistance of anyone else who wishes to review the code.

Greg cc'd as upstream maintainer for IIO to keep him informed.

Thanks,

Jonathan

On 29/09/14 15:02, Felipe Balbi wrote:
> Alright, this is already ridiculous. Andrew, if I resend the patch can
> you apply it since maintainer has been ignoring this thread anyway ?
> 
> On Fri, Sep 26, 2014 at 11:19:59PM -0500, Felipe Balbi wrote:
>> ping
>>
>> On Thu, Sep 25, 2014 at 05:16:19PM -0500, Felipe Balbi wrote:
>>> ping
>>>
>>> On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
>>>> ping
>>>>
>>>> On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
>>>>> ping
>>>>>
>>>>> On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
>>>>>> ping
>>>>>>
>>>>>> On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
>>>>>>> ping
>>>>>>>
>>>>>>> On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
>>>>>>>> ping
>>>>>>>>
>>>>>>>> On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
>>>>>>>>> ping
>>>>>>>>>
>>>>>>>>> On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
>>>>>>>>>> ping
>>>>>>>>>>
>>>>>>>>>> On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>>>>>>>>>>>> On 02/09/14 16:17, Felipe Balbi wrote:
>>>>>>>>>>>>> TI's opt3001 light sensor is a simple and yet powerful
>>>>>>>>>>>>> little device. The device provides 99% IR rejection,
>>>>>>>>>>>>> Automatic full-scale, very low power consumption and
>>>>>>>>>>>>> measurements from 0.01 to 83k lux.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This patch adds support for that device using the IIO
>>>>>>>>>>>>> framework.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> Resending as I saw no changes on the thread.
>>>>>>>>>>>> Hi Felipe,
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry for the delay on this, entirely my fault - been busy and forgot
>>>>>>>>>>>> I still had questions about what was going on in here (yup its the
>>>>>>>>>>>> hysteresis bit again!)
>>>>>>>>>>>
>>>>>>>>>>> right, this is starting to become way too much headache for such a
>>>>>>>>>>> simple device. Sorry will not help me getting this driver upstream. When
>>>>>>>>>>> I first sent this (August 6), we didn't even have v3.17-rc1, now we're
>>>>>>>>>>> about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
>>>>>>>>>>> window.
>>>>>>>>>>>
>>>>>>>>>>>> Anyhow, I'm afraid I am still a little confused about the meaning you
>>>>>>>>>>>> have assigned to Hysteresis in this driver.
>>>>>>>>>>>>
>>>>>>>>>>>> Let me conjecture on what might be going on here (I may be entirely
>>>>>>>>>>>> wrong).
>>>>>>>>>>>>
>>>>>>>>>>>> Normally a hysteresis value in IIO is defined as the 'distance' back
>>>>>>>>>>>> from a threshold that a signal must go before it may retrip the
>>>>>>>>>>>> threshold.
>>>>>>>>>>>> This threshold value is separately controlled. Thus if we have a
>>>>>>>>>>>> rising threshold of 10 and an hysteresis of 2 - to get two events the
>>>>>>>>>>>> signal must first rise past 10, then drop back below 8 and rise again
>>>>>>>>>>>> past 10.
>>>>>>>>>>>> If it drops below 10 but not 8 and rises again past 10 then we will
>>>>>>>>>>>> not get an event.
>>>>>>>>>>>>
>>>>>>>>>>>> So having the same register for both the hysteresis and the threshold
>>>>>>>>>>>> doesn't with this description make much sense.  It would mean that you
>>>>>>>>>>>> could only have a threshold of say 10 and a hysteresis of 10, thus in
>>>>>>>>>>>> effect meaning the signal would always have to cross 0 before the next
>>>>>>>>>>>> event whatever the combined threshold / hysteresis value?
>>>>>>>>>>>>
>>>>>>>>>>>> Perhaps instead the device is automatically adjusting the threshold
>>>>>>>>>>>> when we cross it and the 'hysteresis' here is with respect to a the
>>>>>>>>>>>> previous threshold?
>>>>>>>>>>>>
>>>>>>>>>>>> Thus if we start with a value of 0 and hysteresis is set to 2 it will
>>>>>>>>>>>> trigger an event at:
>>>>>>>>>>>>
>>>>>>>>>>>> 2, 4, 6, 8, 10 as we rise?
>>>>>>>>>>>>
>>>>>>>>>>>> This sort of auto adjustment of levels isn't uncommon in light sensors
>>>>>>>>>>>> (where the point of the interrupt is to notify the operating system
>>>>>>>>>>>> that a 'significant' change has occurred and things like screen
>>>>>>>>>>>> brightness may need adjusting.
>>>>>>>>>>>>
>>>>>>>>>>>> If so then the current hysteresis interface does not apply, nor does
>>>>>>>>>>>> the Rate of Change (ROC) interface as this is dependent on amount of
>>>>>>>>>>>> change, not how fast it changed.  Hence we needs something new to
>>>>>>>>>>>> handle this cleanly. I would suggest a new event type.  Perhaps
>>>>>>>>>>>> something with sysfs attr naming along the lines of
>>>>>>>>>>>> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>>>>>>>>>>>> What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
>>>>>>>>>>>>
>>>>>>>>>>>> etc?
>>>>>>>>>>>
>>>>>>>>>>> will you just tell me what you want ? I really cannot give a crap
>>>>>>>>>>> anymore. This has already taken me over a month of my time for such a
>>>>>>>>>>> simple little device, not to mention your confusing and contradicting
>>>>>>>>>>> change requests.
>>>>>>>>>>>
>>>>>>>>>>> (could you also trim your responses ? it's very annoying to scroll so
>>>>>>>>>>> much)
>>>>>>>>>>>
>>>>>>>>>>>>> +#define OPT3001_RESULT		0x00
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION	0x01
>>>>>>>>>>>>> +#define OPT3001_LOW_LIMIT	0x02
>>>>>>>>>>>>> +#define OPT3001_HIGH_LIMIT	0x03
>>>>>>>>>>>>> +#define OPT3001_MANUFACTURER_ID	0x7e
>>>>>>>>>>>>> +#define OPT3001_DEVICE_ID	0x7f
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_RN_MASK (0xf << 12)
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_RN_AUTO (0xc << 12)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_CT	BIT(11)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_M_MASK	(3 << 9)
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9)
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_M_SINGLE (1 << 9)
>>>>>>>>>>>>> +#define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> I guess this naming is straight off the datasheet, but it is rather
>>>>>>>>>>>> more cryptic than perhaps it needs to be!  That's kind of an issue
>>>>>>>>>>>> with the datasheet choices rather than what you have here however!
>>>>>>>>>>>
>>>>>>>>>>> man, are you nit-picky!! These are named as such to make grepping on
>>>>>>>>>>> documentation easier. It's better to have something that matches
>>>>>>>>>>> documentation, don't you think ? otherwise, future users/developers of
>>>>>>>>>>> these driver will need either a shit ton of comments explaining that A
>>>>>>>>>>> maps to B in docs, or will need a fscking crystal ball to read my mind.
>>>>>>>>>>> Assuming I'll still remember what I meant.
>>>>>>>>>>>
>>>>>>>>>>>>> +static int opt3001_remove(struct i2c_client *client)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +	struct iio_dev *iio = i2c_get_clientdata(client);
>>>>>>>>>>>>> +	struct opt3001 *opt = iio_priv(iio);
>>>>>>>>>>>>> +	int ret;
>>>>>>>>>>>>> +	u16 reg;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	free_irq(client->irq, iio);
>>>>>>>>>>>>> +	iio_device_unregister(iio);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>>>>>>>>>>>> +	if (ret < 0) {
>>>>>>>>>>>>> +		dev_err(opt->dev, "failed to read register %02x\n",
>>>>>>>>>>>>> +				OPT3001_CONFIGURATION);
>>>>>>>>>>>>> +		return ret;
>>>>>>>>>>>>> +	}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	reg = ret;
>>>>>>>>>>>>> +	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
>>>>>>>>>>>>> +			reg);
>>>>>>>>>>>>> +	if (ret < 0) {
>>>>>>>>>>>>> +		dev_err(opt->dev, "failed to write register %02x\n",
>>>>>>>>>>>>> +				OPT3001_CONFIGURATION);
>>>>>>>>>>>>> +		return ret;
>>>>>>>>>>>>> +	}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	iio_device_free(iio);
>>>>>>>>>>>>
>>>>>>>>>>>> Use the devm_iio_device_alloc and you can drop the need to free it.
>>>>>>>>>>>> I don't really mind, but I'll almost guarantee that someone will post
>>>>>>>>>>>> a follow up patch doing this if you don't.  As it will be ever so
>>>>>>>>>>>> slightly cleaner, I'll probably take that patch.
>>>>>>>>>>>
>>>>>>>>>>> here's the original driver:
>>>>>>>>>>>
>>>>>>>>>>> http://www.spinics.net/lists/linux-iio/msg14331.html
>>>>>>>>>>>
>>>>>>>>>>> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>>>>>>>>>>>
>>>>>>>>>>> http://www.spinics.net/lists/linux-iio/msg14421.html
>>>>>>>>>>>
>>>>>>>>>>> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
>>>>>>>>>>>
>>>>>>>>>>> So figure out what you really want, let me know and I'll code it all up
>>>>>>>>>>> quickly and hopefully still get this into v3.18 merge window. I sent
>>>>>>>>>>> this driver *very* early to be doubly sure it would hit v3.18 and there
>>>>>>>>>>> was a long hiatus from yourself which is now jeopardizing what I was
>>>>>>>>>>> expecting. That, coupled with your contradicting requests, has just put
>>>>>>>>>>> me in a bad mood, even before Monday, hurray.
>>>>>>>>>>>
>>>>>>>>>>> cheers
>>>>>>>>>>>
>>>>>>>>>>> -- 
>>>>>>>>>>> balbi
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> balbi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> balbi
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> balbi
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> balbi
>>>>>>
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> balbi
>>>>>
>>>>>
>>>>>
>>>>> -- 
>>>>> balbi
>>>>
>>>>
>>>>
>>>> -- 
>>>> balbi
>>>
>>>
>>>
>>> -- 
>>> balbi
>>
>>
>>
>> -- 
>> balbi
> 
> 
> 

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi 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?

Perhaps it would be better to explain what each field of the
configuration register does and then we can move on.

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?

Either have TI release the documentation or add comments to each of the
fields of each of the registers such that we can understand what exactly
they are doing.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

Hi,

On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi 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?
> 
> Either have TI release the documentation or add comments to each of the
> fields of each of the registers such that we can understand what exactly
> they are doing.

TI will release the documentation when that's all cleared up with Legal.
You can't expect it to be any earlier than that.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Daniel Baluta]

On Mon, Sep 29, 2014 at 9:53 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
>> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi 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.

This is ridiculous! Jonathan asked you some questions about the implementation,
but you ignored the request. Instead you've send 9 ping emails!

Daniel.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On Mon, Sep 29, 2014 at 10:07:32PM +0300, Daniel Baluta wrote:
> On Mon, Sep 29, 2014 at 9:53 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> >> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi 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.
> 
> This is ridiculous! Jonathan asked you some questions about the implementation,
> but you ignored the request. Instead you've send 9 ping emails!

I ignored ? I replied straight away asking him to tell me exactly what
he needed and pointing out all the things which he's complaining about
that he asked me to get rid of himself. For example, original driver was
using devm_*, Jonathan asked to remove, I tried arguing and he said he
wanted them out, so I removed, now he's asking me why I didn't use
devm_* to start with.

That's ridiculous, that's what pointless and a total waste of time.

Not to mention that there was a long hiatus from Jonathan for weeks
until he said "yeah, sorry, I had forgotten about this". Because of
Jonathan alone, we lost a merge window. I send the original driver
because v3.17-rc1 was even tagged and now you tell me that it's
ridiculous I'm upset with Jonathan's lazy responses ?

gimme a break

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Mon, Sep 29, 2014 at 01:53:25PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> > On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi 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?

> > 
> > Either have TI release the documentation or add comments to each of the
> > fields of each of the registers such that we can understand what exactly
> > they are doing.
> 
> TI will release the documentation when that's all cleared up with Legal.
> You can't expect it to be any earlier than that.

I am a little fuzzy on how the source code can be released when an NDA
is required to access the datasheet.

Isn't the source code going to be breaking the NDA by releasing information
that is in the datasheet?

> 
> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

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 ?

> > > Either have TI release the documentation or add comments to each of the
> > > fields of each of the registers such that we can understand what exactly
> > > they are doing.
> > 
> > TI will release the documentation when that's all cleared up with Legal.
> > You can't expect it to be any earlier than that.
> 
> I am a little fuzzy on how the source code can be released when an NDA
> is required to access the datasheet.
>
> Isn't the source code going to be breaking the NDA by releasing information
> that is in the datasheet?

that's really not my role inside TI, though. I have no degree by any law
school from any country. When I get asked to write a driver, all I do is
request permission to release it, if that says "okay, release it", I
don't go after the Lawyer who decided it was okay to release the driver.

On top of that, what does that has anything to do with anything ? I'm
pretty sure many have released code based off of either simulation or
pre-release HW. Lack of public documentation does not prevent source
code from being released at all.

Try to get documentation for most of SoCs supported under the ARM tree
and you'll see at least 80% of them will require NDA and/or a big
purchase order of many SoCs before you can get documentation.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

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?

> 
> > > > Either have TI release the documentation or add comments to each of the
> > > > fields of each of the registers such that we can understand what exactly
> > > > they are doing.
> > > 
> > > TI will release the documentation when that's all cleared up with Legal.
> > > You can't expect it to be any earlier than that.
> > 
> > I am a little fuzzy on how the source code can be released when an NDA
> > is required to access the datasheet.
> >
> > Isn't the source code going to be breaking the NDA by releasing information
> > that is in the datasheet?
>
> that's really not my role inside TI, though. I have no degree by any law
> school from any country. When I get asked to write a driver, all I do is
> request permission to release it, if that says "okay, release it", I
> don't go after the Lawyer who decided it was okay to release the driver.
> 
> On top of that, what does that has anything to do with anything ? I'm
> pretty sure many have released code based off of either simulation or
> pre-release HW. Lack of public documentation does not prevent source
> code from being released at all.
>
> Try to get documentation for most of SoCs supported under the ARM tree
> and you'll see at least 80% of them will require NDA and/or a big
> purchase order of many SoCs before you can get documentation.
>

I don't know I am just trying to the more facts so my mailbox will stop
pinging. :)

> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]

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:

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.

[...]

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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

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

> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

On Sat, Oct 04, 2014 at 10:43:57AM +0100, Jonathan Cameron wrote:
> 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.

alright, np. Device will be launched soonish, so full datasheet should
be in TI's website in the coming weeks or so.

I'll wait until v3.18-rc1 is tagged before working on this again.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]