From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751698AbbHPUP0 (ORCPT ); Sun, 16 Aug 2015 16:15:26 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:36456 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751081AbbHPUPX (ORCPT ); Sun, 16 Aug 2015 16:15:23 -0400 Message-ID: <55D0EF57.5050304@cogentembedded.com> Date: Sun, 16 Aug 2015 23:15:19 +0300 From: Vladimir Barinov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Jonathan Cameron CC: Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, cory.tusar@pid1solutions.com Subject: Re: [PATCH v3 5/7] iio: Support triggered events References: <1438174469-19054-1-git-send-email-vladimir.barinov@cogentembedded.com> <1438174648-19296-1-git-send-email-vladimir.barinov@cogentembedded.com> <55D055C9.4020409@kernel.org> In-Reply-To: <55D055C9.4020409@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jonathan, Thank you for review. Find some comments below. Regards, Vladimir On 16.08.2015 12:20, Jonathan Cameron wrote: > On 29/07/15 13:57, Vladimir Barinov wrote: >> Support triggered events. >> >> This is useful for chips that don't have their own interrupt sources. >> It allows to use generic/standalone iio triggers for those drivers. > As we have concluded this is a good thing to have, I've take a closer > look at it. > > I think we need to slightly separate the buffer and event uses of > triggers as it's entirely plausible both might be enabled at the same > time. Don't think this should be hard however. > > I can concieve of weird restrictions from certain hardware (particularly > where hardware buffers are concerned) in which events are not monitored > when we are reading through the buffer but we can tackle that when we > actually need to. > > Anyhow, few bits inline. Thanks for your persistence with this! > > Jonathan > >> Signed-off-by: Vladimir Barinov >> --- >> Changes in version 2: >> - initially added >> Changes in version 3: >> - fixed grammar in patch description >> - changed license header to match "GNU Public License v2 or later" >> >> drivers/iio/Kconfig | 6 +++ >> drivers/iio/Makefile | 1 + >> drivers/iio/industrialio-core.c | 4 +- >> drivers/iio/industrialio-trigger.c | 12 +++++- >> drivers/iio/industrialio-triggered-event.c | 68 ++++++++++++++++++++++++++++++ >> include/linux/iio/iio.h | 1 + >> include/linux/iio/triggered_event.h | 11 +++++ >> 7 files changed, 99 insertions(+), 4 deletions(-) >> create mode 100644 drivers/iio/industrialio-triggered-event.c >> create mode 100644 include/linux/iio/triggered_event.h >> >> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig >> index 4011eff..8fcc92f 100644 >> --- a/drivers/iio/Kconfig >> +++ b/drivers/iio/Kconfig >> @@ -58,6 +58,12 @@ config IIO_CONSUMERS_PER_TRIGGER >> This value controls the maximum number of consumers that a >> given trigger may handle. Default is 2. >> >> +config IIO_TRIGGERED_EVENT >> + tristate >> + select IIO_TRIGGER >> + help >> + Provides helper functions for setting up triggered events. >> + >> source "drivers/iio/accel/Kconfig" >> source "drivers/iio/adc/Kconfig" >> source "drivers/iio/amplifiers/Kconfig" >> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile >> index 698afc2..40dc13e 100644 >> --- a/drivers/iio/Makefile >> +++ b/drivers/iio/Makefile >> @@ -9,6 +9,7 @@ industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o >> industrialio-$(CONFIG_IIO_BUFFER_CB) += buffer_cb.o >> >> obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o >> +obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o >> obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o >> >> obj-y += accel/ >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c >> index 3524b0d..54d71ea 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -948,7 +948,7 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev) >> static void iio_dev_release(struct device *device) >> { >> struct iio_dev *indio_dev = dev_to_iio_dev(device); >> - if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) >> + if (indio_dev->modes & (INDIO_BUFFER_TRIGGERED | INDIO_EVENT_TRIGGERED)) >> iio_device_unregister_trigger_consumer(indio_dev); >> iio_device_unregister_eventset(indio_dev); >> iio_device_unregister_sysfs(indio_dev); >> @@ -1218,7 +1218,7 @@ int iio_device_register(struct iio_dev *indio_dev) >> "Failed to register event set\n"); >> goto error_free_sysfs; >> } >> - if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) >> + if (indio_dev->modes & (INDIO_BUFFER_TRIGGERED | INDIO_EVENT_TRIGGERED)) >> iio_device_register_trigger_consumer(indio_dev); >> >> if ((indio_dev->modes & INDIO_ALL_BUFFER_MODES) && >> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c >> index d31098e..72b63e7 100644 >> --- a/drivers/iio/industrialio-trigger.c >> +++ b/drivers/iio/industrialio-trigger.c >> @@ -345,10 +345,18 @@ static ssize_t iio_trigger_write_current(struct device *dev, >> >> indio_dev->trig = trig; >> >> - if (oldtrig) >> + if (oldtrig) { >> + if (indio_dev->currentmode == INDIO_EVENT_TRIGGERED) >> + iio_trigger_detach_poll_func(oldtrig, >> + indio_dev->pollfunc); >> iio_trigger_put(oldtrig); >> - if (indio_dev->trig) >> + } >> + if (indio_dev->trig) { >> iio_trigger_get(indio_dev->trig); >> + if (indio_dev->currentmode == INDIO_EVENT_TRIGGERED) >> + iio_trigger_attach_poll_func(indio_dev->trig, >> + indio_dev->pollfunc); >> + } >> >> return len; >> } >> diff --git a/drivers/iio/industrialio-triggered-event.c b/drivers/iio/industrialio-triggered-event.c >> new file mode 100644 >> index 0000000..1e5ad33 >> --- /dev/null >> +++ b/drivers/iio/industrialio-triggered-event.c >> @@ -0,0 +1,68 @@ >> + /* >> + * Copyright (C) 2015 Cogent Embedded, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or (at your >> + * option) any later version. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/** >> + * iio_triggered_event_setup() - Setup pollfunc for triggered event >> + * @indio_dev: IIO device structure >> + * @pollfunc_bh: Function which will be used as pollfunc bottom half >> + * @pollfunc_th: Function which will be used as pollfunc top half >> + * >> + * This function combines some common tasks which will normally be performed >> + * when setting up a triggered event. It will allocate the pollfunc and >> + * set mode to use it for triggered event. >> + * >> + * Before calling this function the indio_dev structure should already be >> + * completely initialized, but not yet registered. In practice this means that >> + * this function should be called right before iio_device_register(). >> + * >> + * To free the resources allocated by this function call >> + * iio_triggered_event_cleanup(). >> + */ >> +int iio_triggered_event_setup(struct iio_dev *indio_dev, >> + irqreturn_t (*pollfunc_bh)(int irq, void *p), >> + irqreturn_t (*pollfunc_th)(int irq, void *p)) >> +{ >> + indio_dev->pollfunc = iio_alloc_pollfunc(pollfunc_bh, >> + pollfunc_th, >> + IRQF_ONESHOT, >> + indio_dev, >> + "%s_consumer%d", >> + indio_dev->name, >> + indio_dev->id); >> + if (indio_dev->pollfunc == NULL) >> + return -ENOMEM; >> + >> + /* Flag that pollfunc is used for triggered event */ >> + indio_dev->modes |= INDIO_EVENT_TRIGGERED; >> + indio_dev->currentmode = INDIO_EVENT_TRIGGERED; > Can conceive of a corner case here. What if both buffer > and event are being triggered? No real problem with > running them off the same trigger, but right now enabling > one will take out the other. To avoid conflict it should be separate pollfunc (indio_dev->pollfunc_event = ...) since we do runtime pollfunc attach/detach for both buffer and event interfaces. > > Might also want to not enable triggered event mode > until we enable some events? I think it will need to change most drivers that use event interface since events are enabled inside them via write_event_config() callback or events may have already been enabled during driver probe. > > Perhaps we need separate indio_dev->event_mode and event_modes ? This probably not necessary since iio stack checks for flags set in indio_dev->modes value. > indio_dev->current_event_mode fields to make the two > configurable independently? I think it works, but with separate indio_dev->pollfunc_event. Should it be inidio_dev->current_event_mode or inidio_dev->currentmode_event for consistency? I will verify your idea with using one trigger for both event and buffer interface on hi8435 driver by adding buffer interface that I will try internally (will not send). > > I get the feeling this particular feature is going to evolve > somewhat as we get a feel for how it is being used. > >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(iio_triggered_event_setup); >> + >> +/** >> + * iio_triggered_event_cleanup() - Free resources allocated by iio_triggered_event_setup() >> + * @indio_dev: IIO device structure >> + */ >> +void iio_triggered_event_cleanup(struct iio_dev *indio_dev) >> +{ >> + iio_dealloc_pollfunc(indio_dev->pollfunc); >> +} >> +EXPORT_SYMBOL(iio_triggered_event_cleanup); >> + >> +MODULE_AUTHOR("Vladimir Barinov"); >> +MODULE_DESCRIPTION("IIO helper functions for setting up triggered events"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h >> index f791482..b691ee0 100644 >> --- a/include/linux/iio/iio.h >> +++ b/include/linux/iio/iio.h >> @@ -294,6 +294,7 @@ static inline s64 iio_get_time_ns(void) >> #define INDIO_BUFFER_TRIGGERED 0x02 >> #define INDIO_BUFFER_SOFTWARE 0x04 >> #define INDIO_BUFFER_HARDWARE 0x08 >> +#define INDIO_EVENT_TRIGGERED 0x10 >> >> #define INDIO_ALL_BUFFER_MODES \ >> (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE | INDIO_BUFFER_SOFTWARE) >> diff --git a/include/linux/iio/triggered_event.h b/include/linux/iio/triggered_event.h >> new file mode 100644 >> index 0000000..e9894e9 >> --- /dev/null >> +++ b/include/linux/iio/triggered_event.h >> @@ -0,0 +1,11 @@ >> +#ifndef _LINUX_IIO_TRIGGERED_EVENT_H_ >> +#define _LINUX_IIO_TRIGGERED_EVENT_H_ >> + >> +#include >> + >> +int iio_triggered_event_setup(struct iio_dev *indio_dev, >> + irqreturn_t (*pollfunc_bh)(int irq, void *p), >> + irqreturn_t (*pollfunc_th)(int irq, void *p)); > We renamed the parameters of the equivalent buffer function recently > to avoid some confusion. Could you follow that for this one as well. Ok > >> +void iio_triggered_event_cleanup(struct iio_dev *indio_dev); >> + >> +#endif >>