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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 13 Jan 2020 22:02:00 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Sun, Jan 12, 2020 at 5:54 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> 
> > It seems to work fine on samsung,golden with gp2ap002s00f.
> > There are interrupts being sent when something moves in front of the
> > sensor and iio-event-monitor looks good too:
> >
> > Tested-by: Stephan Gerhold <stephan@gerhold.net>  
> 
> Thanks! I think I'm up at v7 or something on this driver
> and now it looks good.
> 
> > Out of curiosity: I'm not so familiar with the IIO subsystem,
> > is it intended that the sensor is active the whole time?
> > Wouldn't it be better to activate it only when something is reading
> > /dev/iio:device2?  
> 
> IMO the best way to power manage an IIO sensor is like I do in
> drivers/iio/gyro/mpu3050-core.c
> 
> Of course I think it is excellent code since I wrote it. Beware
> of hubris in the driver.
> 
> The idea is to take the runtime pm handle on the device
> whenever in use (reading values from the hardware or
> using the trigger), and then use autosuspend to completely
> shut it down (including regulators) when not in use. If the power-up
> time for the sensor is long, then the autosuspend timeout
> should be proportionally longer, like a magnitude or two above.
> 
> With the proximity events I have this problem that I don't know
> in the driver if someone had opened the file to read events
> or not, only the core knows, I think. I just emit my events
> and be happy.
> 
> Jonathan: is there a way for the driver to know if someone
> is listening on the event interface?

Hmm. Normally we'd do it on someone actually enabling the events on
the basis I'd expect userspace to open the file then enable the events.

Nothing stops us doing it on file open I guess, but not sure what the
real world benefit would be.

> 
> > There is one problem I noticed, although I'm not sure if it is a problem
> > in this driver. Reading one of the files in the sysfs results in:
> >
> > $ cat /sys/bus/iio/devices/iio\:device2/events/in_proximity_thresh_either_en
> > Segmentation fault  
> 
> That looks like an ordinary bug, but that seems to be for the
> core? Hm...
Ah.. Seems I missed that you have the driver set event spec for enable, but
didn't actually provide the callback function.   + we clearly don't sanity
check that one.  Hence IIO core could do with hardening but driver is
buggy as well.

+static const struct iio_event_spec gp2ap002_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};

Means you must supply read_event_config in iio_info.
It's a bit odd if you don't also supply write event config for that
matter though that wasn't the bug here. It would cause a similar
bug if you wrote the sysfs file though :)

Kind of also explains why my logic about enabling the event
doesn't apply currently.

Good find from Stephan on both fronts.

Jonathan

> 
> Yours,
> Linus Walleij