Re: [PATCH 2/2 v1] iio: light: Add a driver for Sharp GP2AP002x00F

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux