On September 15, 2014 6:21:37 AM GMT+01:00, Felipe Balbi <balbi@xxxxxx> 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@xxxxxx>
>> > ---
>> >
>> > 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.
I cannot tell you what i want because no data sheet is available so I have to rely on what
you tell me. What you have right now does not make sense or appear to correctly obey
the ABI so I cannot accept it.
I have spent considerable time asking you to explain the use of hysteresis in this driver
which does not appear to be remotely standard. Either explain it or drop that
element and we can revisit it when the data sheet becomes available. Yes, writing drivers
where no data sheet is available does add a burden to the author when they submit it.
Tough.
>
>(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.
Fine. State that and we can move on.
>
>> > +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, ®, 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!!
Read that email again. I specifically say the register function. I do not mention allocation.
>
>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.
When you coherently explain what the hardware actually does rather than
assuming it is obvious we may be able to make some progress.
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 bfore Monday, hurray.
I would like nothing better than not having to ask you the same question again and again
Why should I conjecture what your hardware is doing to try to allow us to move forward?
If you wish to proceed please either answer my questions or drop the hysteresis support
until we can use the docs to figure it out for ourselves.
Jonathan
>chee
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
