From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net ([212.227.15.15]:63763 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755222AbaIOWtJ (ORCPT ); Mon, 15 Sep 2014 18:49:09 -0400 Message-ID: <54176CDE.8030104@gmx.de> Date: Tue, 16 Sep 2014 00:49:02 +0200 From: Hartmut Knaack MIME-Version: 1.0 To: balbi@ti.com, Jonathan Cameron CC: linux-iio@vger.kernel.org, pmeerw@pmeerw.net Subject: Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor References: <1409671066-25747-1-git-send-email-balbi@ti.com> <54147632.9090204@kernel.org> <20140915052137.GA11866@saruman.home> In-Reply-To: <20140915052137.GA11866@saruman.home> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org 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 >>> --- >>> >>> 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 >