On 11 August 2015 13:21:08 BST, Lars-Peter Clausen [off-list ref] wrote:

On 08/08/2015 07:56 PM, Jonathan Cameron wrote:

quoted

On 29/07/15 13:56, Vladimir Barinov wrote:

quoted

Add Holt threshold detector driver for HI-8435 chip

Signed-off-by: Vladimir Barinov

[off-list ref]

quoted

Because it usually reads better that way, I review from the bottom.
Might make more sense if you read the comments that way around ;)

I'm going to guess that the typical usecase for this device is in

realtime

quoted

systems where polling gives a nice predictable timing (hence the lack

of any

quoted

interrupt support).  These typically operate as 'scanning' devices
(e.g. you update all state at approximately the same time, by reading
one input after another in some predefined order).

Does the use of events actually map well to this use case?  You are

taking

quoted

something that (other than the results being a bit different) is

basically

quoted

a digital I/O interface and mapping it (sort of) to an interrupt chip
when it isn't nothing of the sort.

I'd like more opinions on this, but my gut reaction is that we would
be better making the normal buffered infrastructure handle this data
properly rather than pushing it through the events intrastructure.

Your solution is neat and tidy in someways but feels like it is

mashing

quoted

data into a particular format.

To add to the complexity we could have debounce logic.  If we mapped

to a

quoted

fill the buffer with a scan mode, how would we handle this?
My guess is that it would simply delay transistions.  Perhaps we even
support reading both the value right now and the debounced value if

that

quoted

is possible?

However, here the debounce is all software.  To my mind, move this

into

quoted

userspace entirely. We aren't dealing with big data here so it's

hardly

quoted

going to be particularly challenging.

So my gut feeling is definitely we need to make the buffer

infrastructure

quoted

handle 1 bit values (in groups) then push this data out that way.

Still I would like other opinions on this!

Having thought about this a bit having support for event triggers seems
like
a nice addition to me. When you think about it it is not too different
from
buffer triggers. Some devices are able to generate their own trigger
events
using a IRQ, others might need a software controlled trigger. You could
argue that the same is true for events. Having support for software
based
event triggers e.g. allows support for devices which have events and
have an
IRQ, but where the IRQ is simply not connected.

Whether it makes sense for this device is a different question though I
guess.

Since this is a threshold detector events seem to be the right approach
to
me. You could have similar hardware which actually generates IRQs, so
you'd
have the same interface. Additionally if we expects changes only to
occur at
a much lower rate than the polling rate only sending something to
userspace
when it changes keeps the amount of data to transfer lower. On the
other
hand if changes happen a lot using the event based approach would
certainly
create a higher load. And another thing to consider is that some
applications might be interested in getting the raw data. So in
conclusion,
I don't know ;). Both approaches seem sensible enough to me.

I am coming around to the view that events make some sense in this case.

Definitely also want 1 bit channel support in IIO though.

[...]

quoted

quoted

static void hi8435_debounce_work(struct work_struct *work)
+{
+	struct hi8435_priv *priv = container_of(work, struct hi8435_priv,
+						work.work);
+	struct iio_dev *idev = spi_get_drvdata(priv->spi);
+	u32 val;
+	int ret;
+
+	ret = hi8435_readl(priv, HI8435_SO31_0_REG, &val);
+	if (ret < 0)
+		return;
+
+	if (val == priv->debounce_val)
+		hi8435_iio_push_event(idev, val);
+	else
+		dev_warn(&priv->spi->dev, "filtered by software debounce");

Definitely not.  Warning every time a standard event occurs?  Not
a good idea.  Use the debug stuff if you want a way of knowing this
happened, then it will silent under normal use.

quoted

+}
+
+static irqreturn_t hi8435_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *idev = pf->indio_dev;
+	struct hi8435_priv *priv = iio_priv(idev);
+	u32 val;
+	int ret;
+
+	ret = hi8435_readl(priv, HI8435_SO31_0_REG, &val);
+	if (ret < 0)
+		goto err_read;
+
+	if (priv->debounce_interval) {
+		priv->debounce_val = val;
+		schedule_delayed_work(&priv->work,
+				msecs_to_jiffies(priv->debounce_interval));

This is another element that makes me doubt that the event interface
is the way to do this.  I'd much rather we pushed the debouncing out
to userspace where cleverer things can be done (and more adaptive

things).

quoted

Here to keep the event flow low you have to do it in the kernel.

Yes, debouncing should probably not be done in kernel space and the
debounce
interval should not be a devicetree property.

quoted

quoted

+	} else {
+		hi8435_iio_push_event(idev, val);
+	}
+
+err_read:
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static void hi8435_parse_dt(struct hi8435_priv *priv)
+{
+	struct device_node *np = priv->spi->dev.of_node;
+	int ret;
+
+	ret = of_get_named_gpio(np, "holt,reset-gpios", 0);
+	priv->reset_gpio = ret < 0 ? 0 : ret;

Silly question, but can gpio0 exist on a board?  I suspect you
may need an additional variable to hold that this request failed
rather than using the magic value of 0.

It can, but new stuff should just use the GPIO descriptor API where
NULL can
be used to indicate a invalid GPIO.

- Lars

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.