Felipe Balbi schrieb, Am 15.09.2014 07:21:
> 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.
>
Easy, dude. I don't know how much you followed this list, but during that mentioned time, there were over 500 mails exchanged. As the maintainer, I expect Jonathan to have read all of them more or less intensively. The time he sacrifices for this project is quite massive, considering that he doesn't do this for a living. So please respect, that he also has other activities, which may have higher priorities, and that he doesn't schedule his life according to kernel deadlines. He is doing a pretty good job here.
Thanks
Hartmut
>> 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
>
--
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
