Problems receiving mails from Jonathan and the list? http://marc.info/?l=linux-iio&m=141077611429500&w=2 Felipe Balbi schrieb, Am 18.09.2014 15:36: > 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@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 >> >> >> >> -- >> balbi > > > -- 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
