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. > > + 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. 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. > 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. Yours, Linus Walleij
