I'm confused. Are you not receiving my emails?
On September 17, 2014 4:00:41 PM GMT+01:00, Felipe Balbi <balbi@xxxxxx> 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@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.
>> >
>> > (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, ®,
>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
--
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
