On Mon, 6 Jan 2020 10:08:55 +0100 Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > Hi Jonathan, > > fixed most of the things and resending as v2 soon-ish, > some inline responses, comments: > > On Mon, Dec 30, 2019 at 6:39 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > On Sat, 28 Dec 2019 21:11:09 +0100 Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > > If at all possible I'd like to discourage use of of specific > > calls in favour of the generic ones. It's pretty unlikely we'll > > ever see this driver using anything else, but I'd like to build > > up a good set of examples to point people at now that functionality > > is in place. > > I guess you mean to use fwnode where possible. I comment on this > below. yes, though more specifically PRP0001 usage, which basically puts the DT directly into ACPI. > > > > + iio_push_event(indio_dev, ev, iio_get_time_ns(indio_dev)); > > > + usleep_range(20000, 30000); > > > > What is the basis for these timings? > > Detection cycle, I explained this with an inline comment. > > > > + gp2ap002->is_gp2ap002s00f = > > > + of_device_is_compatible(np, "sharp,gp2ap002s00f"); > > > > Hmm. This rather breaks my comment below about trying to avoid making > > this of specific if we don't need to... > > > > I 'think' we could use device_property_read_string > > There is a bit of precedence for doing so, but it is not common. > > This is the real trick. Using > device_property_read_string(dev, "compatible", str); > isn't going to work as ACPI probes from a unique 4-char > ID not a compatible string this will never work on ACPI > anyways. Is that true for PRP0001? That's the ACPI case we normally care about in cases like this. https://lkml.org/lkml/2019/3/22/1612 Has an explicit "compatible" property. > > I can try to go some extra mile to support a hypothetical > ACPI client by adding a struct with one bool member as > match data and pass that around if you insist, but I think it's > more something appropriate for the first ACPI user to do. > > It's no problem if you want it, but it will add a bunch of > boilerplate just for this. > > > > + /* Check the device tree for the IR LED hysteresis */ > > > + ret = of_property_read_u32(np, "sharp,proximity-far-hysteresis", &val); > > > > Do these belong in DT at all, or are they more of a policy decision? > > Without a datasheet I'm kind of guessing what they actually are. > > There is a datasheet: > https://global.sharp/products/device-china/lineup/data/pdf/datasheet/gp2ap002s00f_appl_e.pdf > > > We have the option for hysterisis controls on events from sysfs if that > > make sense. > > I don't know, these are two hysteresis settings: one that detects an > object close to the sensor and one detecting an object far from > the sensor. > > The two settings are describes as fixed to mode A, B1 and B2 in the > datasheet. However there is a vendor driver in one of the phone > trees that use "mode B 1.5" not documented in the datasheet > (bummer). So given how fluid this all is I opted for just an u8 > in the device tree for "close" and "far" hysteresis setting. OK, lets leave this, but maybe add a comment somewhere to give this bit of detail. > > > Could use the fwnode_get_property_u32 etc to drop reliance on OF. > > Will do if we must support hypotetical non-DT probe. > > > > + /* The GP2AP002A00F has a light sensor too */ > > > + if (!gp2ap002->is_gp2ap002s00f) { > > > > This section is rather 'unusual' and definitely needs some explanatory > > comments - particularly as I can't find any reference docs for the part. > > The only reference for the light sensor part in GP2AP002A00F > is the submission from Samsung mentioned in the header of the > driver submitted by Donggeun Kim & Minkyu Kang in 2011: > https://lore.kernel.org/lkml/1315556546-7445-1-git-send-email-dg77.kim@xxxxxxxxxxx/ > > It also appears in the GPL code from GT-S7710 which seems to > derive from a code drop from Sharp. > > Yep the code is the documentation... :( > > > I'm guessing that the light sensor is simply an analog output? As such > > you need to wire it up to a separate ADC to actually read the light level... > > Yep that's the same method as used for > drivers/iio/light/cm3605.c > Most early Androids do something like that, and all SoCs seem to > provide some ADC to do the conversion. > Fair enough, a few more comments perhaps in the code for when we inevitably forget all this history ;) Thanks, Jonathan > Yours, > Linus Walleij
