From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bear.ext.ti.com ([192.94.94.41]:46660 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751561AbaIOFWH (ORCPT ); Mon, 15 Sep 2014 01:22:07 -0400 Date: Mon, 15 Sep 2014 00:21:37 -0500 From: Felipe Balbi To: Jonathan Cameron CC: Felipe Balbi , , Subject: Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor Message-ID: <20140915052137.GA11866@saruman.home> Reply-To: References: <1409671066-25747-1-git-send-email-balbi@ti.com> <54147632.9090204@kernel.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="cWoXeonUoKmBZSoM" In-Reply-To: <54147632.9090204@kernel.org> Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org --cWoXeonUoKmBZSoM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > This patch adds support for that device using the IIO > > framework. > >=20 > > Signed-off-by: Felipe Balbi > > --- > >=20 > > Resending as I saw no changes on the thread. > Hi Felipe, >=20 > 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. >=20 > Let me conjecture on what might be going on here (I may be entirely > wrong). >=20 > 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. >=20 > 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? >=20 > 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? >=20 > Thus if we start with a value of 0 and hysteresis is set to 2 it will > trigger an event at: >=20 > 2, 4, 6, 8, 10 as we rise? >=20 > 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. >=20 > 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 >=20 > 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 */ > > + >=20 > 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 =3D i2c_get_clientdata(client); > > + struct opt3001 *opt =3D iio_priv(iio); > > + int ret; > > + u16 reg; > > + > > + free_irq(client->irq, iio); > > + iio_device_unregister(iio); > > + > > + ret =3D i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATIO= N); > > + if (ret < 0) { > > + dev_err(opt->dev, "failed to read register %02x\n", > > + OPT3001_CONFIGURATION); > > + return ret; > > + } > > + > > + reg =3D ret; > > + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SHUTDOWN); > > + > > + ret =3D i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATI= ON, > > + 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 --=20 balbi --cWoXeonUoKmBZSoM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUFndhAAoJEIaOsuA1yqREVtkP/A/29/CjZkqd+flqgpQ1f1ul rMvOESt4w24IVNhOkOBW+QMKQI2PbA7a/3hyaguacDQZ4q3KptgpaYKS3QlPrEfc xDwxZzZ6Ww3sXR8C/W193ulyw5jGnhmzPEnFKujlgIKy2UbstDDJTpwI574Fsjq0 jTAwoD4zLEwhnWmBFQ8FEoakIhVLL7535Pb0zgtvBuazqG+XuF7YQq2tiGtQykZj K311qlu11bpuO7INduMyL6Vw7MwIknIVYazjA9F6WXKTkCFytzYNopriBHp4/GLQ FMJlPHIhPQGazHz5DF+5lJbcRodlI3JGFvrUjsTxVfOG8GsHG/gcQiszzEnT82wz pXqJ+soHb/HURJYXIUNpF+9fA7sAjDDvGsSRfvTEtifymbpYXz9MTglhPVFlu4cV 7yKRp3PBq30f1G0qpVUtICdPkejXuijGsS2SK146aGQR1BiN1l6eFQLxhwYP6lyg YwN20OI71ibLDnbm5ziuWxZTWcjaGrp/NIGcnsdESg5QttZfdcchfvgExl9XTSGT aNHoVpnl654gcL0MkKG17xObSQU/iT9LF8i1cJFmm+1MxJcc13e63Ghem/8G1JYq 4kTXp5DNtOiSgawHWjHV93MfX5dovr7Tut5MulqRhDlVh/Ah2nsTNBiBC4RS0/qZ f3xG4sG12rcFYA3Cxyeh =9qSK -----END PGP SIGNATURE----- --cWoXeonUoKmBZSoM--