[PATCH] IIO: TRIGGER: New sysfs based trigger

[PATCH] IIO: TRIGGER: New sysfs based trigger

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

This patch adds a new trigger that can be invoked by writing
the sysfs file: trigger_now. This approach can be valuable during
automated testing or in situations, where other trigger methods
are not applicable. For example no RTC or spare GPIOs.
Last but not least we can allow user space applications to produce triggers.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/trigger/Kconfig          |    6 ++
 drivers/staging/iio/trigger/Makefile         |    1 +
 drivers/staging/iio/trigger/iio-trig-sysfs.c |  108 ++++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c

diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
index d842a58..c185e47 100644
--- a/drivers/staging/iio/trigger/Kconfig
+++ b/drivers/staging/iio/trigger/Kconfig
@@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
 	help
 	  Provides support for using GPIO pins as IIO triggers.
 
+config IIO_SYSFS_TRIGGER
+	tristate "SYSFS trigger"
+	depends on SYSFS
+	help
+	  Provides support for using SYSFS entry as IIO triggers.
+
 endif # IIO_TRIGGER
diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
index 10aeca5..504b9c0 100644
--- a/drivers/staging/iio/trigger/Makefile
+++ b/drivers/staging/iio/trigger/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
 obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
+obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
new file mode 100644
index 0000000..3434f3c
--- /dev/null
+++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * iio-trig-sysfs.c
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "../iio.h"
+#include "../trigger.h"
+
+static ssize_t iio_sysfs_trigger_poll(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	iio_trigger_poll(trig, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
+static IIO_TRIGGER_NAME_ATTR;
+
+static struct attribute *iio_sysfs_trigger_attrs[] = {
+	&dev_attr_trigger_now.attr,
+	&dev_attr_name.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_sysfs_trigger_attr_group = {
+	.attrs = iio_sysfs_trigger_attrs,
+};
+
+static int __devinit iio_sysfs_trigger_probe(struct platform_device *pdev)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_allocate_trigger();
+	if (!trig) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	trig->control_attrs = &iio_sysfs_trigger_attr_group;
+	trig->owner = THIS_MODULE;
+	trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
+	if (trig->name == NULL) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto out3;
+
+	platform_set_drvdata(pdev, trig);
+
+	return 0;
+out3:
+	kfree(trig->name);
+out2:
+	iio_put_trigger(trig);
+out1:
+
+	return ret;
+}
+
+static int __devexit iio_sysfs_trigger_remove(struct platform_device *pdev)
+{
+	struct iio_trigger *trig = platform_get_drvdata(pdev);
+
+	iio_trigger_unregister(trig);
+	kfree(trig->name);
+	iio_put_trigger(trig);
+
+	return 0;
+}
+
+static struct platform_driver iio_sysfs_trigger_driver = {
+	.driver = {
+		.name = "iio_sysfs_trigger",
+		.owner = THIS_MODULE,
+	},
+	.probe = iio_sysfs_trigger_probe,
+	.remove = __devexit_p(iio_sysfs_trigger_remove),
+};
+
+static int __init iio_sysfs_trig_init(void)
+{
+	return platform_driver_register(&iio_sysfs_trigger_driver);
+}
+module_init(iio_sysfs_trig_init);
+
+static void __exit iio_sysfs_trig_exit(void)
+{
+	platform_driver_unregister(&iio_sysfs_trigger_driver);
+}
+module_exit(iio_sysfs_trig_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
+MODULE_LICENSE("GPL v2");
-- 
1.6.0.2

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Jonathan Cameron]

On 02/02/11 13:30, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> This patch adds a new trigger that can be invoked by writing
> the sysfs file: trigger_now. This approach can be valuable during
> automated testing or in situations, where other trigger methods
> are not applicable. For example no RTC or spare GPIOs.
> Last but not least we can allow user space applications to produce triggers.
Excellent.  Very handy for testing as you say.

If we really want to commonly use this from userspace we will want a means
of registering more than one such trigger but this does fine for test
rigs. To do multiple triggers with this we could either just take a module
parameter or have a 'request_trigger' attribute under some parent device.
Still that's a question for another day.  The current functionality is
enough to be getting on with.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>  drivers/staging/iio/trigger/Makefile         |    1 +
>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108 ++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
> 
> diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
> index d842a58..c185e47 100644
> --- a/drivers/staging/iio/trigger/Kconfig
> +++ b/drivers/staging/iio/trigger/Kconfig
> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>  	help
>  	  Provides support for using GPIO pins as IIO triggers.
>  
> +config IIO_SYSFS_TRIGGER
> +	tristate "SYSFS trigger"
> +	depends on SYSFS
> +	help
> +	  Provides support for using SYSFS entry as IIO triggers.
> +
>  endif # IIO_TRIGGER
> diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
> index 10aeca5..504b9c0 100644
> --- a/drivers/staging/iio/trigger/Makefile
> +++ b/drivers/staging/iio/trigger/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
>  obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> new file mode 100644
> index 0000000..3434f3c
> --- /dev/null
> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * iio-trig-sysfs.c
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "../iio.h"
> +#include "../trigger.h"
> +
> +static ssize_t iio_sysfs_trigger_poll(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_trigger *trig = dev_get_drvdata(dev);
> +	iio_trigger_poll(trig, 0);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
> +static IIO_TRIGGER_NAME_ATTR;
> +
> +static struct attribute *iio_sysfs_trigger_attrs[] = {
> +	&dev_attr_trigger_now.attr,
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group iio_sysfs_trigger_attr_group = {
> +	.attrs = iio_sysfs_trigger_attrs,
> +};
> +
> +static int __devinit iio_sysfs_trigger_probe(struct platform_device *pdev)
> +{
> +	struct iio_trigger *trig;
> +	int ret;
> +
> +	trig = iio_allocate_trigger();
> +	if (!trig) {
> +		ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	trig->control_attrs = &iio_sysfs_trigger_attr_group;
> +	trig->owner = THIS_MODULE;
> +	trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
> +	if (trig->name == NULL) {
> +		ret = -ENOMEM;
> +		goto out2;
> +	}
> +
> +	ret = iio_trigger_register(trig);
> +	if (ret)
> +		goto out3;
> +
> +	platform_set_drvdata(pdev, trig);
> +
> +	return 0;
> +out3:
> +	kfree(trig->name);
> +out2:
> +	iio_put_trigger(trig);
> +out1:
> +
> +	return ret;
> +}
> +
> +static int __devexit iio_sysfs_trigger_remove(struct platform_device *pdev)
> +{
> +	struct iio_trigger *trig = platform_get_drvdata(pdev);
> +
> +	iio_trigger_unregister(trig);
> +	kfree(trig->name);
> +	iio_put_trigger(trig);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver iio_sysfs_trigger_driver = {
> +	.driver = {
> +		.name = "iio_sysfs_trigger",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = iio_sysfs_trigger_probe,
> +	.remove = __devexit_p(iio_sysfs_trigger_remove),
> +};
> +
> +static int __init iio_sysfs_trig_init(void)
> +{
> +	return platform_driver_register(&iio_sysfs_trigger_driver);
> +}
> +module_init(iio_sysfs_trig_init);
> +
> +static void __exit iio_sysfs_trig_exit(void)
> +{
> +	platform_driver_unregister(&iio_sysfs_trigger_driver);
> +}
> +module_exit(iio_sysfs_trig_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
> +MODULE_LICENSE("GPL v2");

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Jonathan Cameron wrote on 2011-02-02:
> On 02/02/11 13:30, michael.hennerich@analog.com wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> This patch adds a new trigger that can be invoked by writing the sysfs
>> file: trigger_now. This approach can be valuable during automated
>> testing or in situations, where other trigger methods are not
>> applicable. For example no RTC or spare GPIOs. Last but not least we
>> can allow user space applications to produce
> triggers.
> Excellent.  Very handy for testing as you say.
>
> If we really want to commonly use this from userspace we will want a
> means of registering more than one such trigger but this does fine for
> test rigs. To do multiple triggers with this we could either just take
> a module parameter or have a 'request_trigger' attribute under some
> parent device.
> Still that's a question for another day.  The current functionality is
> enough to be getting on with.

Well - you can register as many triggers as you want.
This is what platform_device.id is for.

See example below -

static struct platform_device iio_sysfs_trigger =3D {
        .name           =3D "iio_sysfs_trigger",
        .id             =3D 0,
        .dev =3D {
                .platform_data =3D NULL,
        },
};

static struct platform_device iio_sysfs_trigger1 =3D {
        .name           =3D "iio_sysfs_trigger",
        .id             =3D 1,
        .dev =3D {
                .platform_data =3D NULL,
        },
};


        &bfin_dpmc,

static struct platform_device *my_devices[] __initdata =3D {

#if defined(CONFIG_IIO_SYSFS_TRIGGER) || defined(CONFIG_IIO_SYSFS_TRIGGER _=
MODULE)
        &iio_sysfs_trigger,
        &iio_sysfs_trigger1,
#endif
}

In the driver use:

> +     trig->name =3D kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);

What the gpio trigger driver does is not necessary
- it only replicates functionality already provided by the platform device =
bus.

-Michael


Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Jonathan Cameron]

On 02/02/11 19:21, Hennerich, Michael wrote:
> Jonathan Cameron wrote on 2011-02-02:
>> On 02/02/11 13:30, michael.hennerich@analog.com wrote:
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>> This patch adds a new trigger that can be invoked by writing the sysfs
>>> file: trigger_now. This approach can be valuable during automated
>>> testing or in situations, where other trigger methods are not
>>> applicable. For example no RTC or spare GPIOs. Last but not least we
>>> can allow user space applications to produce
>> triggers.
>> Excellent.  Very handy for testing as you say.
>>
>> If we really want to commonly use this from userspace we will want a
>> means of registering more than one such trigger but this does fine for
>> test rigs. To do multiple triggers with this we could either just take
>> a module parameter or have a 'request_trigger' attribute under some
>> parent device.
>> Still that's a question for another day.  The current functionality is
>> enough to be getting on with.
> 
> Well - you can register as many triggers as you want.
> This is what platform_device.id is for.
Good point. I was clearly half asleep last night!

Thanks for the clarification.

Jonathan
> 
> See example below -
> 
> static struct platform_device iio_sysfs_trigger = {
>         .name           = "iio_sysfs_trigger",
>         .id             = 0,
>         .dev = {
>                 .platform_data = NULL,
>         },
> };
> 
> static struct platform_device iio_sysfs_trigger1 = {
>         .name           = "iio_sysfs_trigger",
>         .id             = 1,
>         .dev = {
>                 .platform_data = NULL,
>         },
> };
> 
> 
>         &bfin_dpmc,
> 
> static struct platform_device *my_devices[] __initdata = {
> 
> #if defined(CONFIG_IIO_SYSFS_TRIGGER) || defined(CONFIG_IIO_SYSFS_TRIGGER _MODULE)
>         &iio_sysfs_trigger,
>         &iio_sysfs_trigger1,
> #endif
> }
> 
> In the driver use:
> 
>> +     trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
> 
> What the gpio trigger driver does is not necessary
> - it only replicates functionality already provided by the platform device bus.
> 
> -Michael
> 
> 
> Greetings,
> Michael
> 
> --
> Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif
> 
> 

[PATCH] IIO: TRIGGER: New sysfs based trigger

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

This patch adds a new trigger that can be invoked by writing
the sysfs file: trigger_now. This approach can be valuable during
automated testing or in situations, where other trigger methods
are not applicable. For example no RTC or spare GPIOs.
Last but not least we can allow user space applications to produce triggers.

IIO: TRIGGER: Apply review feedback by Greg Kroah-Hartman

Changes since v1:

Add sysfs documentation.
Change license notice.
Add module alias.
Add more Kconfig help text

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 .../iio/Documentation/sysfs-bus-iio-trigger-sysfs  |   11 ++
 drivers/staging/iio/trigger/Kconfig                |   10 ++
 drivers/staging/iio/trigger/Makefile               |    1 +
 drivers/staging/iio/trigger/iio-trig-sysfs.c       |  108 ++++++++++++++++++++
 4 files changed, 130 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
 create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c

diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs b/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
new file mode 100644
index 0000000..5235e6c
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
@@ -0,0 +1,11 @@
+What:		/sys/bus/iio/devices/triggerX/trigger_now
+KernelVersion:	2.6.38
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This file is provided by the iio-trig-sysfs stand-alone trigger
+		driver. Writing this file with any value triggers an event
+		driven driver, associated with this trigger, to capture data
+		into an in kernel buffer. This approach can be valuable during
+		automated testing or in situations, where other trigger methods
+		are not applicable. For example no RTC or spare GPIOs.
+		X is the IIO index of the trigger.
diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
index d842a58..3a82013 100644
--- a/drivers/staging/iio/trigger/Kconfig
+++ b/drivers/staging/iio/trigger/Kconfig
@@ -18,4 +18,14 @@ config IIO_GPIO_TRIGGER
 	help
 	  Provides support for using GPIO pins as IIO triggers.

+config IIO_SYSFS_TRIGGER
+	tristate "SYSFS trigger"
+	depends on SYSFS
+	help
+	  Provides support for using SYSFS entry as IIO triggers.
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called iio-trig-sysfs.
+
 endif # IIO_TRIGGER
diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
index 10aeca5..504b9c0 100644
--- a/drivers/staging/iio/trigger/Makefile
+++ b/drivers/staging/iio/trigger/Makefile
@@ -4,3 +4,4 @@

 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
 obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
+obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
new file mode 100644
index 0000000..127a2a3
--- /dev/null
+++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "../iio.h"
+#include "../trigger.h"
+
+static ssize_t iio_sysfs_trigger_poll(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	iio_trigger_poll(trig, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
+static IIO_TRIGGER_NAME_ATTR;
+
+static struct attribute *iio_sysfs_trigger_attrs[] = {
+	&dev_attr_trigger_now.attr,
+	&dev_attr_name.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_sysfs_trigger_attr_group = {
+	.attrs = iio_sysfs_trigger_attrs,
+};
+
+static int __devinit iio_sysfs_trigger_probe(struct platform_device *pdev)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_allocate_trigger();
+	if (!trig) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	trig->control_attrs = &iio_sysfs_trigger_attr_group;
+	trig->owner = THIS_MODULE;
+	trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
+	if (trig->name == NULL) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto out3;
+
+	platform_set_drvdata(pdev, trig);
+
+	return 0;
+out3:
+	kfree(trig->name);
+out2:
+	iio_put_trigger(trig);
+out1:
+
+	return ret;
+}
+
+static int __devexit iio_sysfs_trigger_remove(struct platform_device *pdev)
+{
+	struct iio_trigger *trig = platform_get_drvdata(pdev);
+
+	iio_trigger_unregister(trig);
+	kfree(trig->name);
+	iio_put_trigger(trig);
+
+	return 0;
+}
+
+static struct platform_driver iio_sysfs_trigger_driver = {
+	.driver = {
+		.name = "iio_sysfs_trigger",
+		.owner = THIS_MODULE,
+	},
+	.probe = iio_sysfs_trigger_probe,
+	.remove = __devexit_p(iio_sysfs_trigger_remove),
+};
+
+static int __init iio_sysfs_trig_init(void)
+{
+	return platform_driver_register(&iio_sysfs_trigger_driver);
+}
+module_init(iio_sysfs_trig_init);
+
+static void __exit iio_sysfs_trig_exit(void)
+{
+	platform_driver_unregister(&iio_sysfs_trigger_driver);
+}
+module_exit(iio_sysfs_trig_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:iio-trig-sysfs");
--
1.6.0.2

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Jonathan Cameron]

On 02/04/11 15:34, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-04:
>> On Fri, Feb 04, 2011 at 08:38:16AM +0000, Hennerich, Michael wrote:
>>> Greg KH wrote on 2011-02-03:
>>>> On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
>>>>> Greg KH wrote on 2011-02-02:
>>>>>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael
>> wrote:
>>>>>>> Greg KH wrote on 2011-02-02:
>>>>>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>>>>>>> michael.hennerich@analog.com
>>>>>>>> wrote:
>>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>>
>>>>>>>>> This patch adds a new trigger that can be invoked by writing the
>>>>>>>>> sysfs file: trigger_now. This approach can be valuable during
>>>>>>>>> automated testing or in situations, where other trigger methods
>>>>>>>>> are not applicable. For example no RTC or spare GPIOs. Last but
>>>>>>>>> not least we can allow user space applications to produce
>>>>>>>>> triggers.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>>>>
>>>>>>>> If you add a new sysfs file, you need to document it.  I'll
>>>>>>>> not take this patch as-is because of that.
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>>>>>> ++++++++++++++++++++++++++ 3 files changed, 115
>>>>>>>>> ++++++++++++++++++++++++++ insertions(+), 0
>>>>>>>>>  deletions(-)  create mode
>>>>>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>>>>>>> b/drivers/staging/iio/trigger/Kconfig
>>>>>>>>> index d842a58..c185e47 100644
>>>>>>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>>>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>>>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>>>>>>      help
>>>>>>>>>        Provides support for using GPIO pins as IIO triggers.
>>>>>>>>> +config IIO_SYSFS_TRIGGER
>>>>>>>>> +    tristate "SYSFS trigger"
>>>>>>>>> +    depends on SYSFS
>>>>>>>>> +    help
>>>>>>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>>>>>>
>>>>>>>> Why would you ever want to not have this enabled?  Why is it a
>>>>>>>> config option?  And shouldn't it depend on the IIO subsystem?
>>>>>>>
>>>>>>> You won't see this if you don't have IIO + triggers enabled.
>>>>>>
>>>>>> Ok, the dependancy on IIO comes from other parts of the Kconfig
>>>>>> file, my mistake.
>>>>>>
>>>>>>> Are you asking me to add more help text here - or you didn't read
>>>>>>> the commit log? Alternatively are you asking why IIO common trigger
>>>>>>> core doesn't provide this flexibility?
>>>>>>>
>>>>>>> Sorry - please explain.
>>>>>>
>>>>>> My question is, why is this a config option at all?  Why would
>>>>>> it not always be enabled?  What benefit is it if it is not enabled?
>>>>>
>>>>> It's a config option, since the user must provide a platform
>>>>> resource for it.
>>>>
>>>> What platform resource?
>>>
>>> Resource is probably a bit misleading - But your platform/board
>>> dependant init code must register a struct platform_device using
>>> platform_add_device() and friends. If it does it multiple times, with
>>> different ids. multiple triggers are created.
>>
>> That's fine, but it's not what you are doing here, right?
> 
> That's what I'm doing in my board setup code, it's just not part of this patch.
> 
> static struct platform_device iio_sysfs_trigger = {
>         .name           = "iio_sysfs_trigger",
>         .id             = 0,
> };
> 
> static struct platform_device iio_sysfs_trigger1 = {
>         .name           = "iio_sysfs_trigger",
>         .id             = 1,
> };
> 
> static struct platform_device *my_devices[] __initdata = {
> 
> #if defined(CONFIG_IIO_SYSFS_TRIGGER) || defined(CONFIG_IIO_SYSFS_TRIGGER _MODULE)
>         &iio_sysfs_trigger,
>         &iio_sysfs_trigger1,
> #endif
> }
> 
>>>>> If you're not planning to use it, why compile and include it at
>> all?
>>>>
>>>> If you are a distro, how are you supposed to know if this is
>>>> something you want or not?
>>>
>>> It's likely something that you don't want.
>>
>> Then say so in the help section of the Kconfig file.
> 
> ok
> 
>> And also consider not including the file at all, if it's not something
>> you really want :)
> 
> It's useful for everyone doing automated driver tests.
> And this driver is provided in the hope it will be useful for others too.
> 
> Jonathan - can do you think it is useful?
It is. I'll probably try adding dynamic instantiation when I have a chance,
but in the meantime definitely a handy bit of functionality to have.
> 
> 
>>>> If this is going to be the proper API for interacting with iio
>>>> devices, then it needs to always be present in the core, otherwise
>>>> your userspace tools are going to get messy, right?
>>>
>>> Consider it as something like all the other iio drivers.
>>> The user has the option to enable the drivers that are required for
>>> its application. The hardware interacting with these drivers need to
>>> be present, and the user has to provide information on how these are
>> physically connected.
>>
>> No, the "other" drivers get loaded on demand if the hardware is
>> present or not, which is a big difference, right?
> 
> None of the IIO drivers work that way.
> 
> Unlike PCI or USB devices, SPI or I2C devices are not enumerated at the hardware level.
> Instead, the software must know which devices are connected on each SPI bus segment,
> and what slave selects/address these devices are using.
> For this reason, the kernel code must instantiate SPI devices explicitly.
> The most common method is to declare the SPI devices by bus number.
> 
>>> If you don't enable a IIO input driver that supports triggered sampling
>>> support. You won't need any trigger driver. If you enable multiple of
>>> such drivers. You may want to trigger a few with GPIO some others with
>>> RTC and maybe one with this sysfs based trigger.
>>
>> And how does a user know to do one vs. the other?
> 
> It's purely application specific.
> It's like connecting wires on a PCB.
Not quite.  There are generic boards which get used for a number of applications
(take the memsic imote2's or gumstix for example). It's this that would motivate
the addition of on demand registration of these.


<snip>

> Isn't is still an argument that you don't enable kernel options or drivers
> that you don't understand or likely going to use?
Not everyone seems to keep to that or to take a look at whether the
things they build are applicable to any box their distro will actually work on.

Ubuntu 10.10 on one of my x86 boxes (2.6.35) currently has:
iio/accel:
adis16209.ko  adis16220.ko  adis16240.ko  kxsd9.ko  lis3l02dq.ko  sca3000.ko
iio/adc:
max1363.ko
iio/gyro:
adis16260.ko
iio/imu:
adis16300.ko  adis16350.ko  adis16400.ko
iio/light:
tsl2563.ko
iio/trigger:
iio-trig-gpio.ko  iio-trig-periodic-rtc.ko

Looking at it another way... We get a lot of free build testing from the distros ;)

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-04:
> On Fri, Feb 04, 2011 at 08:38:16AM +0000, Hennerich, Michael wrote:
>> Greg KH wrote on 2011-02-03:
>>> On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
>>>> Greg KH wrote on 2011-02-02:
>>>>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael
> wrote:
>>>>>> Greg KH wrote on 2011-02-02:
>>>>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>>>>>> michael.hennerich@analog.com
>>>>>>> wrote:
>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>
>>>>>>>> This patch adds a new trigger that can be invoked by writing the
>>>>>>>> sysfs file: trigger_now. This approach can be valuable during
>>>>>>>> automated testing or in situations, where other trigger methods
>>>>>>>> are not applicable. For example no RTC or spare GPIOs. Last but
>>>>>>>> not least we can allow user space applications to produce
>>>>>>>> triggers.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>>>
>>>>>>> If you add a new sysfs file, you need to document it.  I'll
>>>>>>> not take this patch as-is because of that.
>>>>>>>
>>>>>>>> ---
>>>>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>>>>> ++++++++++++++++++++++++++ 3 files changed, 115
>>>>>>>> ++++++++++++++++++++++++++ insertions(+), 0
>>>>>>>>  deletions(-)  create mode
>>>>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>>>>>> b/drivers/staging/iio/trigger/Kconfig
>>>>>>>> index d842a58..c185e47 100644
>>>>>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>>>>>      help
>>>>>>>>        Provides support for using GPIO pins as IIO triggers.
>>>>>>>> +config IIO_SYSFS_TRIGGER
>>>>>>>> +    tristate "SYSFS trigger"
>>>>>>>> +    depends on SYSFS
>>>>>>>> +    help
>>>>>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>>>>>
>>>>>>> Why would you ever want to not have this enabled?  Why is it a
>>>>>>> config option?  And shouldn't it depend on the IIO subsystem?
>>>>>>
>>>>>> You won't see this if you don't have IIO + triggers enabled.
>>>>>
>>>>> Ok, the dependancy on IIO comes from other parts of the Kconfig
>>>>> file, my mistake.
>>>>>
>>>>>> Are you asking me to add more help text here - or you didn't read
>>>>>> the commit log? Alternatively are you asking why IIO common trigger
>>>>>> core doesn't provide this flexibility?
>>>>>>
>>>>>> Sorry - please explain.
>>>>>
>>>>> My question is, why is this a config option at all?  Why would
>>>>> it not always be enabled?  What benefit is it if it is not enabled?
>>>>
>>>> It's a config option, since the user must provide a platform
>>>> resource for it.
>>>
>>> What platform resource?
>>
>> Resource is probably a bit misleading - But your platform/board
>> dependant init code must register a struct platform_device using
>> platform_add_device() and friends. If it does it multiple times, with
>> different ids. multiple triggers are created.
>
> That's fine, but it's not what you are doing here, right?

That's what I'm doing in my board setup code, it's just not part of this pa=
tch.

static struct platform_device iio_sysfs_trigger =3D {
        .name           =3D "iio_sysfs_trigger",
        .id             =3D 0,
};

static struct platform_device iio_sysfs_trigger1 =3D {
        .name           =3D "iio_sysfs_trigger",
        .id             =3D 1,
};

static struct platform_device *my_devices[] __initdata =3D {

#if defined(CONFIG_IIO_SYSFS_TRIGGER) || defined(CONFIG_IIO_SYSFS_TRIGGER _=
MODULE)
        &iio_sysfs_trigger,
        &iio_sysfs_trigger1,
#endif
}

>>>> If you're not planning to use it, why compile and include it at
> all?
>>>
>>> If you are a distro, how are you supposed to know if this is
>>> something you want or not?
>>
>> It's likely something that you don't want.
>
> Then say so in the help section of the Kconfig file.

ok

> And also consider not including the file at all, if it's not something
> you really want :)

It's useful for everyone doing automated driver tests.
And this driver is provided in the hope it will be useful for others too.

Jonathan - can do you think it is useful?


>>> If this is going to be the proper API for interacting with iio
>>> devices, then it needs to always be present in the core, otherwise
>>> your userspace tools are going to get messy, right?
>>
>> Consider it as something like all the other iio drivers.
>> The user has the option to enable the drivers that are required for
>> its application. The hardware interacting with these drivers need to
>> be present, and the user has to provide information on how these are
> physically connected.
>
> No, the "other" drivers get loaded on demand if the hardware is
> present or not, which is a big difference, right?

None of the IIO drivers work that way.

Unlike PCI or USB devices, SPI or I2C devices are not enumerated at the har=
dware level.
Instead, the software must know which devices are connected on each SPI bus=
 segment,
and what slave selects/address these devices are using.
For this reason, the kernel code must instantiate SPI devices explicitly.
The most common method is to declare the SPI devices by bus number.

>> If you don't enable a IIO input driver that supports triggered sampling
>> support. You won't need any trigger driver. If you enable multiple of
>> such drivers. You may want to trigger a few with GPIO some others with
>> RTC and maybe one with this sysfs based trigger.
>
> And how does a user know to do one vs. the other?

It's purely application specific.
It's like connecting wires on a PCB.

>> Userspace tools to date, search the sysfs bus iio device hierarchy
> and query for names.
>
> Where are these tools?

See iio/Documentation: find_type_by_name()

>>>> In embedded systems, people try to minimize kernel or rootfs size.
>>>
>>> Sure, and how much size does this code really take?
>>>
>>>> And last but not least kernel startup time.
>>>
>>> How much extra time does this module take at startup time that you
>>> have measured?
>>>
>>> I've spent weeks working on boot speedup times for products, and a
>>> tiny module like this, built into the kernel, would have no
>>> measurable affect on boot time that I can see.  What am I missing?
>>
>> Ok - I see this argument doesn't count.
>
> So, you tried to make an argument saying something you haven't
> measured or tested?

Isn't is still an argument that you don't enable kernel options or drivers
that you don't understand or likely going to use?

> {sigh}
>
> Why do I waste my time...

It's also my time.

> bah,
>
> greg k-h

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Jonathan Cameron]

Hi All,

On 02/04/11 14:55, Greg KH wrote:
> On Fri, Feb 04, 2011 at 08:38:16AM +0000, Hennerich, Michael wrote:
>> Greg KH wrote on 2011-02-03:
>>> On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
>>>> Greg KH wrote on 2011-02-02:
>>>>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
>>>>>> Greg KH wrote on 2011-02-02:
>>>>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>>>>>> michael.hennerich@analog.com
>>>>>>> wrote:
>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>
>>>>>>>> This patch adds a new trigger that can be invoked by writing the
>>>>>>>> sysfs file: trigger_now. This approach can be valuable during
>>>>>>>> automated testing or in situations, where other trigger methods are
>>>>>>>> not applicable. For example no RTC or spare GPIOs. Last but not
>>>>>>>> least we can allow user space applications to produce triggers.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>>>
>>>>>>> If you add a new sysfs file, you need to document it.  I'll not
>>>>>>> take this patch as-is because of that.
>>>>>>>
>>>>>>>> ---
>>>>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>>>>> ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+),
>>>>>>>> ++++++++++++++++++++++++++ 0
>>>>>>>>  deletions(-)  create mode
>>>>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>>>>>> b/drivers/staging/iio/trigger/Kconfig
>>>>>>>> index d842a58..c185e47 100644
>>>>>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>>>>>      help
>>>>>>>>        Provides support for using GPIO pins as IIO triggers.
>>>>>>>> +config IIO_SYSFS_TRIGGER
>>>>>>>> +    tristate "SYSFS trigger"
>>>>>>>> +    depends on SYSFS
>>>>>>>> +    help
>>>>>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>>>>>
>>>>>>> Why would you ever want to not have this enabled?  Why is it a
>>>>>>> config option?  And shouldn't it depend on the IIO subsystem?
>>>>>>
>>>>>> You won't see this if you don't have IIO + triggers enabled.
>>>>>
>>>>> Ok, the dependancy on IIO comes from other parts of the Kconfig
>>>>> file, my mistake.
>>>>>
>>>>>> Are you asking me to add more help text here - or you didn't read the
>>>>>> commit log? Alternatively are you asking why IIO common trigger core
>>>>>> doesn't provide this flexibility?
>>>>>>
>>>>>> Sorry - please explain.
>>>>>
>>>>> My question is, why is this a config option at all?  Why would it
>>>>> not always be enabled?  What benefit is it if it is not enabled?
>>>>
>>>> It's a config option, since the user must provide a platform
>>>> resource for it.
>>>
>>> What platform resource?
>>
>> Resource is probably a bit misleading -
>> But your platform/board dependant init code must register a struct platform_device
>> using platform_add_device() and friends. If it does it multiple times, with different ids.
>> multiple triggers are created.
> 
> That's fine, but it's not what you are doing here, right?
Over to Michael for that one, but I think that is exactly what he is doing.
> 
>>>> If you're not planning to use it, why compile and include it at all?
>>>
>>> If you are a distro, how are you supposed to know if this is something
>>> you want or not?
>>
>> It's likely something that you don't want.
> 
> Then say so in the help section of the Kconfig file.
> 
> And also consider not including the file at all, if it's not something
> you really want :)
I think moving to a dynamic creation approach as per my other email makes
this discussion moot.  That way we build it into the core (optional for
anyone bothered about code bloat) and userspace apps can
create them only when needed.  That way the 'right' option is 
for them to always be enabled by default. There just wont be any
such triggers created until someone asks for one.
> 
>>> If this is going to be the proper API for interacting with iio
>>> devices, then it needs to always be present in the core, otherwise
>>> your userspace tools are going to get messy, right?
>>
>> Consider it as something like all the other iio drivers.
>> The user has the option to enable the drivers that are required for its
>> application. The hardware interacting with these drivers need to be present,
>> and the user has to provide information on how these are physically connected.
> 
> No, the "other" drivers get loaded on demand if the hardware is present
> or not, which is a big difference, right?
> 
>> If you don't enable a IIO input driver that supports triggered sampling support.
>> You won't need any trigger driver. If you enable multiple of such drivers.
>> You may want to trigger a few with GPIO some others with RTC and maybe one with this
>> sysfs based trigger.
> 
> And how does a user know to do one vs. the other?
Depends entirely on what they are trying to do with the sensor in question. The main
use case for a lot of these sensor (well my main use anyway ;))
is on generic sensor board where people wire them up to all sorts of strange rigs
to grab data with all sorts of inter channel sampling setups.

If a given sensor has it's own internal clock and data ready signal it will have the
default trigger set to point at the one it provides.  The user is welcome to change
this in most cases (some hardware has periods in which you really don't want to attempt
to read from it so this isn't always possible).  Note other sensors can also use this trigger
(e.g. grab a voltage as close as possible to the timing of a an accelerometer sampling).

The gpio case actually exists to sync capture against systems over which we have no
control whatsoever (in our case, motion capture rigs).  The RTC one is a legacy of
an old case of mine and I'm inclined to drop it when we have something equivalent to inputs
polled reading in place.

So the upshot is:  If the user doesn't know what they want, then the default configuration
should be right, but if they need to change the trigger then the functionality is there.

This userspace trigger has been on the todo list for a long time, but until now none of
us writing drivers have actually needed it so it hadn't happened.  It's also needed for
the input bridge code to handle polled devices (not yet done).  Note the input bridge
version will need to do dynamic creation of these triggers, but I can add that when I
need it.
> 
>> Userspace tools to date, search the sysfs bus iio device hierarchy and query for names.
> 
> Where are these tools?
> 
Example code in iio/Documentation does this.

Bus browsing tools (lsusb equivalent) at http://sourceforge.net/projects/iioutils/
(might be lagging kernel tree somewhat).

Jonathan

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Fri, Feb 04, 2011 at 08:38:16AM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-03:
> > On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
> >> Greg KH wrote on 2011-02-02:
> >>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
> >>>> Greg KH wrote on 2011-02-02:
> >>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
> >>>>> michael.hennerich@analog.com
> >>>>> wrote:
> >>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
> >>>>>>
> >>>>>> This patch adds a new trigger that can be invoked by writing the
> >>>>>> sysfs file: trigger_now. This approach can be valuable during
> >>>>>> automated testing or in situations, where other trigger methods are
> >>>>>> not applicable. For example no RTC or spare GPIOs. Last but not
> >>>>>> least we can allow user space applications to produce triggers.
> >>>>>>
> >>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> >>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> >>>>>
> >>>>> If you add a new sysfs file, you need to document it.  I'll not
> >>>>> take this patch as-is because of that.
> >>>>>
> >>>>>> ---
> >>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
> >>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
> >>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
> >>>>>> ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+),
> >>>>>> ++++++++++++++++++++++++++ 0
> >>>>>>  deletions(-)  create mode
> >>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
> >>>>>>
> >>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
> >>>>>> b/drivers/staging/iio/trigger/Kconfig
> >>>>>> index d842a58..c185e47 100644
> >>>>>> --- a/drivers/staging/iio/trigger/Kconfig
> >>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
> >>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
> >>>>>>      help
> >>>>>>        Provides support for using GPIO pins as IIO triggers.
> >>>>>> +config IIO_SYSFS_TRIGGER
> >>>>>> +    tristate "SYSFS trigger"
> >>>>>> +    depends on SYSFS
> >>>>>> +    help
> >>>>>> +      Provides support for using SYSFS entry as IIO triggers.
> >>>>>
> >>>>> Why would you ever want to not have this enabled?  Why is it a
> >>>>> config option?  And shouldn't it depend on the IIO subsystem?
> >>>>
> >>>> You won't see this if you don't have IIO + triggers enabled.
> >>>
> >>> Ok, the dependancy on IIO comes from other parts of the Kconfig
> >>> file, my mistake.
> >>>
> >>>> Are you asking me to add more help text here - or you didn't read the
> >>>> commit log? Alternatively are you asking why IIO common trigger core
> >>>> doesn't provide this flexibility?
> >>>>
> >>>> Sorry - please explain.
> >>>
> >>> My question is, why is this a config option at all?  Why would it
> >>> not always be enabled?  What benefit is it if it is not enabled?
> >>
> >> It's a config option, since the user must provide a platform
> >> resource for it.
> >
> > What platform resource?
> 
> Resource is probably a bit misleading -
> But your platform/board dependant init code must register a struct platform_device
> using platform_add_device() and friends. If it does it multiple times, with different ids.
> multiple triggers are created.

That's fine, but it's not what you are doing here, right?

> >> If you're not planning to use it, why compile and include it at all?
> >
> > If you are a distro, how are you supposed to know if this is something
> > you want or not?
> 
> It's likely something that you don't want.

Then say so in the help section of the Kconfig file.

And also consider not including the file at all, if it's not something
you really want :)

> > If this is going to be the proper API for interacting with iio
> > devices, then it needs to always be present in the core, otherwise
> > your userspace tools are going to get messy, right?
> 
> Consider it as something like all the other iio drivers.
> The user has the option to enable the drivers that are required for its
> application. The hardware interacting with these drivers need to be present,
> and the user has to provide information on how these are physically connected.

No, the "other" drivers get loaded on demand if the hardware is present
or not, which is a big difference, right?

> If you don't enable a IIO input driver that supports triggered sampling support.
> You won't need any trigger driver. If you enable multiple of such drivers.
> You may want to trigger a few with GPIO some others with RTC and maybe one with this
> sysfs based trigger.

And how does a user know to do one vs. the other?

> Userspace tools to date, search the sysfs bus iio device hierarchy and query for names.

Where are these tools?

> >> In embedded systems, people try to minimize kernel or rootfs size.
> >
> > Sure, and how much size does this code really take?
> >
> >> And last but not least kernel startup time.
> >
> > How much extra time does this module take at startup time that you
> > have measured?
> >
> > I've spent weeks working on boot speedup times for products, and a
> > tiny module like this, built into the kernel, would have no measurable
> > affect on boot time that I can see.  What am I missing?
> 
> Ok - I see this argument doesn't count.

So, you tried to make an argument saying something you haven't measured
or tested?

{sigh}

Why do I waste my time...

bah,

greg k-h

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Jonathan Cameron]

On 02/04/11 08:38, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-03:
>> On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
>>> Greg KH wrote on 2011-02-02:
>>>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
>>>>> Greg KH wrote on 2011-02-02:
>>>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>>>>> michael.hennerich@analog.com
>>>>>> wrote:
>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>
>>>>>>> This patch adds a new trigger that can be invoked by writing the
>>>>>>> sysfs file: trigger_now. This approach can be valuable during
>>>>>>> automated testing or in situations, where other trigger methods are
>>>>>>> not applicable. For example no RTC or spare GPIOs. Last but not
>>>>>>> least we can allow user space applications to produce triggers.
>>>>>>>
>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>>
>>>>>> If you add a new sysfs file, you need to document it.  I'll not
>>>>>> take this patch as-is because of that.
>>>>>>
>>>>>>> ---
>>>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>>>> ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+),
>>>>>>> ++++++++++++++++++++++++++ 0
>>>>>>>  deletions(-)  create mode
>>>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>>>>
>>>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>>>>> b/drivers/staging/iio/trigger/Kconfig
>>>>>>> index d842a58..c185e47 100644
>>>>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>>>>      help
>>>>>>>        Provides support for using GPIO pins as IIO triggers.
>>>>>>> +config IIO_SYSFS_TRIGGER
>>>>>>> +    tristate "SYSFS trigger"
>>>>>>> +    depends on SYSFS
>>>>>>> +    help
>>>>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>>>>
>>>>>> Why would you ever want to not have this enabled?  Why is it a
>>>>>> config option?  And shouldn't it depend on the IIO subsystem?
>>>>>
>>>>> You won't see this if you don't have IIO + triggers enabled.
>>>>
>>>> Ok, the dependancy on IIO comes from other parts of the Kconfig
>>>> file, my mistake.
>>>>
>>>>> Are you asking me to add more help text here - or you didn't read the
>>>>> commit log? Alternatively are you asking why IIO common trigger core
>>>>> doesn't provide this flexibility?
>>>>>
>>>>> Sorry - please explain.
>>>>
>>>> My question is, why is this a config option at all?  Why would it
>>>> not always be enabled?  What benefit is it if it is not enabled?
>>>
>>> It's a config option, since the user must provide a platform
>>> resource for it.
>>
>> What platform resource?
> 
> Resource is probably a bit misleading -
> But your platform/board dependant init code must register a struct platform_device
> using platform_add_device() and friends. If it does it multiple times, with different ids.
> multiple triggers are created.
> 
>>> If you're not planning to use it, why compile and include it at all?
>>
>> If you are a distro, how are you supposed to know if this is something
>> you want or not?
> 
> It's likely something that you don't want.
> IMHO this trigger doesn't differ from all the other standalone trigger drivers.
> - Except that it is not directly hardware dependant.
> 
>> If this is going to be the proper API for interacting with iio
>> devices, then it needs to always be present in the core, otherwise
>> your userspace tools are going to get messy, right?
> 
> Consider it as something like all the other iio drivers.
> The user has the option to enable the drivers that are required for its
> application. The hardware interacting with these drivers need to be present,
> and the user has to provide information on how these are physically connected.
> 
> If you don't enable a IIO input driver that supports triggered sampling support.
> You won't need any trigger driver. If you enable multiple of such drivers.
> You may want to trigger a few with GPIO some others with RTC and maybe one with this
> sysfs based trigger.
> 
> Userspace tools to date, search the sysfs bus iio device hierarchy and query for names.
The other approach and I think closer to what Greg is suggesting, would be to have
(in the core - probably as an option element) a top level pair of attributes such as
sysfs_trigger_create and  sysfs_trigger_destroy (probably in /sys/bus/iio - or
in a device hanging off there with the individual triggers as it's children).

I guess writing an unused id to these would then create the trigger and everything
is otherwise the same as your driver.

Basically that gives us a way of adding these 'virtual' devices on demand. It's
similar to how the various drivers for 'dummy' networking devices etc work.

Ultimately I want the functionality your patch provides and am really not all that
fussy about the exact configuration.  It ought to be possible to support both
registration methods but I haven't actually thought through how to do this in
any depth!
> 
>>> In embedded systems, people try to minimize kernel or rootfs size.
>>
>> Sure, and how much size does this code really take?
>>
>>> And last but not least kernel startup time.
>>
>> How much extra time does this module take at startup time that you
>> have measured?
>>
>> I've spent weeks working on boot speedup times for products, and a
>> tiny module like this, built into the kernel, would have no measurable
>> affect on boot time that I can see.  What am I missing?
> 
> Ok - I see this argument doesn't count.
> 
> Greetings,
> Michael

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-03:
> On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
>> Greg KH wrote on 2011-02-02:
>>> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
>>>> Greg KH wrote on 2011-02-02:
>>>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>>>> michael.hennerich@analog.com
>>>>> wrote:
>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>
>>>>>> This patch adds a new trigger that can be invoked by writing the
>>>>>> sysfs file: trigger_now. This approach can be valuable during
>>>>>> automated testing or in situations, where other trigger methods are
>>>>>> not applicable. For example no RTC or spare GPIOs. Last but not
>>>>>> least we can allow user space applications to produce triggers.
>>>>>>
>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>
>>>>> If you add a new sysfs file, you need to document it.  I'll not
>>>>> take this patch as-is because of that.
>>>>>
>>>>>> ---
>>>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>>> ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+),
>>>>>> ++++++++++++++++++++++++++ 0
>>>>>>  deletions(-)  create mode
>>>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>>>
>>>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>>>> b/drivers/staging/iio/trigger/Kconfig
>>>>>> index d842a58..c185e47 100644
>>>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>>>      help
>>>>>>        Provides support for using GPIO pins as IIO triggers.
>>>>>> +config IIO_SYSFS_TRIGGER
>>>>>> +    tristate "SYSFS trigger"
>>>>>> +    depends on SYSFS
>>>>>> +    help
>>>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>>>
>>>>> Why would you ever want to not have this enabled?  Why is it a
>>>>> config option?  And shouldn't it depend on the IIO subsystem?
>>>>
>>>> You won't see this if you don't have IIO + triggers enabled.
>>>
>>> Ok, the dependancy on IIO comes from other parts of the Kconfig
>>> file, my mistake.
>>>
>>>> Are you asking me to add more help text here - or you didn't read the
>>>> commit log? Alternatively are you asking why IIO common trigger core
>>>> doesn't provide this flexibility?
>>>>
>>>> Sorry - please explain.
>>>
>>> My question is, why is this a config option at all?  Why would it
>>> not always be enabled?  What benefit is it if it is not enabled?
>>
>> It's a config option, since the user must provide a platform
>> resource for it.
>
> What platform resource?

Resource is probably a bit misleading -
But your platform/board dependant init code must register a struct platform=
_device
using platform_add_device() and friends. If it does it multiple times, with=
 different ids.
multiple triggers are created.

>> If you're not planning to use it, why compile and include it at all?
>
> If you are a distro, how are you supposed to know if this is something
> you want or not?

It's likely something that you don't want.
IMHO this trigger doesn't differ from all the other standalone trigger driv=
ers.
- Except that it is not directly hardware dependant.

> If this is going to be the proper API for interacting with iio
> devices, then it needs to always be present in the core, otherwise
> your userspace tools are going to get messy, right?

Consider it as something like all the other iio drivers.
The user has the option to enable the drivers that are required for its
application. The hardware interacting with these drivers need to be present=
,
and the user has to provide information on how these are physically connect=
ed.

If you don't enable a IIO input driver that supports triggered sampling sup=
port.
You won't need any trigger driver. If you enable multiple of such drivers.
You may want to trigger a few with GPIO some others with RTC and maybe one =
with this
sysfs based trigger.

Userspace tools to date, search the sysfs bus iio device hierarchy and quer=
y for names.

>> In embedded systems, people try to minimize kernel or rootfs size.
>
> Sure, and how much size does this code really take?
>
>> And last but not least kernel startup time.
>
> How much extra time does this module take at startup time that you
> have measured?
>
> I've spent weeks working on boot speedup times for products, and a
> tiny module like this, built into the kernel, would have no measurable
> affect on boot time that I can see.  What am I missing?

Ok - I see this argument doesn't count.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Thu, Feb 03, 2011 at 09:58:33AM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-02:
> > On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
> >> Greg KH wrote on 2011-02-02:
> >>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
> >>> michael.hennerich@analog.com
> >>> wrote:
> >>>> From: Michael Hennerich <michael.hennerich@analog.com>
> >>>>
> >>>> This patch adds a new trigger that can be invoked by writing the
> >>>> sysfs
> >>>> file: trigger_now. This approach can be valuable during automated
> >>>> testing or in situations, where other trigger methods are not
> >>>> applicable. For example no RTC or spare GPIOs. Last but not least
> >>>> we can allow user space applications to produce triggers.
> >>>>
> >>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> >>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> >>>
> >>> If you add a new sysfs file, you need to document it.  I'll not take
> >>> this patch as-is because of that.
> >>>
> >>>> ---
> >>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
> >>>>  drivers/staging/iio/trigger/Makefile         |    1 +
> >>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
> >>>>  ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 0
> >>>>  deletions(-)  create mode
> >>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
> >>>>
> >>>> diff --git a/drivers/staging/iio/trigger/Kconfig
> >>>> b/drivers/staging/iio/trigger/Kconfig
> >>>> index d842a58..c185e47 100644
> >>>> --- a/drivers/staging/iio/trigger/Kconfig
> >>>> +++ b/drivers/staging/iio/trigger/Kconfig
> >>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
> >>>>      help
> >>>>        Provides support for using GPIO pins as IIO triggers.
> >>>> +config IIO_SYSFS_TRIGGER
> >>>> +    tristate "SYSFS trigger"
> >>>> +    depends on SYSFS
> >>>> +    help
> >>>> +      Provides support for using SYSFS entry as IIO triggers.
> >>>
> >>> Why would you ever want to not have this enabled?  Why is it a
> >>> config option?  And shouldn't it depend on the IIO subsystem?
> >>
> >> You won't see this if you don't have IIO + triggers enabled.
> >
> > Ok, the dependancy on IIO comes from other parts of the Kconfig file,
> > my mistake.
> >
> >> Are you asking me to add more help text here - or you didn't read the
> >> commit log? Alternatively are you asking why IIO common trigger core
> >> doesn't provide this flexibility?
> >>
> >> Sorry - please explain.
> >
> > My question is, why is this a config option at all?  Why would it not
> > always be enabled?  What benefit is it if it is not enabled?
> 
> It's a config option, since the user must provide a platform resource for
> it.

What platform resource?

> If you're not planning to use it, why compile and include it at all?

If you are a distro, how are you supposed to know if this is something
you want or not?

If this is going to be the proper API for interacting with iio devices,
then it needs to always be present in the core, otherwise your userspace
tools are going to get messy, right?

> In embedded systems, people try to minimize kernel or rootfs size.

Sure, and how much size does this code really take?

> And last but not least kernel startup time.

How much extra time does this module take at startup time that you have
measured?

I've spent weeks working on boot speedup times for products, and a tiny
module like this, built into the kernel, would have no measurable affect
on boot time that I can see.  What am I missing?

thanks,

greg k-h

[PATCH] IIO: TRIGGER: New sysfs based trigger

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

This patch adds a new trigger that can be invoked by writing
the sysfs file: trigger_now. This approach can be valuable during
automated testing or in situations, where other trigger methods
are not applicable. For example no RTC or spare GPIOs.
Last but not least we can allow user space applications to produce triggers.

IIO: TRIGGER: Apply review feedback by Greg Kroah-Hartman

Changes since v1:

Add sysfs documentation.
Change license notice.
Add module alias.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 .../iio/Documentation/sysfs-bus-iio-trigger-sysfs  |   11 ++
 drivers/staging/iio/trigger/Kconfig                |    6 +
 drivers/staging/iio/trigger/Makefile               |    1 +
 drivers/staging/iio/trigger/iio-trig-sysfs.c       |  108 ++++++++++++++++++++
 4 files changed, 126 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
 create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c

diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs b/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
new file mode 100644
index 0000000..5235e6c
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-trigger-sysfs
@@ -0,0 +1,11 @@
+What:		/sys/bus/iio/devices/triggerX/trigger_now
+KernelVersion:	2.6.38
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This file is provided by the iio-trig-sysfs stand-alone trigger
+		driver. Writing this file with any value triggers an event
+		driven driver, associated with this trigger, to capture data
+		into an in kernel buffer. This approach can be valuable during
+		automated testing or in situations, where other trigger methods
+		are not applicable. For example no RTC or spare GPIOs.
+		X is the IIO index of the trigger.
diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
index d842a58..c185e47 100644
--- a/drivers/staging/iio/trigger/Kconfig
+++ b/drivers/staging/iio/trigger/Kconfig
@@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
 	help
 	  Provides support for using GPIO pins as IIO triggers.
 
+config IIO_SYSFS_TRIGGER
+	tristate "SYSFS trigger"
+	depends on SYSFS
+	help
+	  Provides support for using SYSFS entry as IIO triggers.
+
 endif # IIO_TRIGGER
diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
index 10aeca5..504b9c0 100644
--- a/drivers/staging/iio/trigger/Makefile
+++ b/drivers/staging/iio/trigger/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
 obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
+obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
new file mode 100644
index 0000000..127a2a3
--- /dev/null
+++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "../iio.h"
+#include "../trigger.h"
+
+static ssize_t iio_sysfs_trigger_poll(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	iio_trigger_poll(trig, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
+static IIO_TRIGGER_NAME_ATTR;
+
+static struct attribute *iio_sysfs_trigger_attrs[] = {
+	&dev_attr_trigger_now.attr,
+	&dev_attr_name.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_sysfs_trigger_attr_group = {
+	.attrs = iio_sysfs_trigger_attrs,
+};
+
+static int __devinit iio_sysfs_trigger_probe(struct platform_device *pdev)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_allocate_trigger();
+	if (!trig) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	trig->control_attrs = &iio_sysfs_trigger_attr_group;
+	trig->owner = THIS_MODULE;
+	trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
+	if (trig->name == NULL) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto out3;
+
+	platform_set_drvdata(pdev, trig);
+
+	return 0;
+out3:
+	kfree(trig->name);
+out2:
+	iio_put_trigger(trig);
+out1:
+
+	return ret;
+}
+
+static int __devexit iio_sysfs_trigger_remove(struct platform_device *pdev)
+{
+	struct iio_trigger *trig = platform_get_drvdata(pdev);
+
+	iio_trigger_unregister(trig);
+	kfree(trig->name);
+	iio_put_trigger(trig);
+
+	return 0;
+}
+
+static struct platform_driver iio_sysfs_trigger_driver = {
+	.driver = {
+		.name = "iio_sysfs_trigger",
+		.owner = THIS_MODULE,
+	},
+	.probe = iio_sysfs_trigger_probe,
+	.remove = __devexit_p(iio_sysfs_trigger_remove),
+};
+
+static int __init iio_sysfs_trig_init(void)
+{
+	return platform_driver_register(&iio_sysfs_trigger_driver);
+}
+module_init(iio_sysfs_trig_init);
+
+static void __exit iio_sysfs_trig_exit(void)
+{
+	platform_driver_unregister(&iio_sysfs_trigger_driver);
+}
+module_exit(iio_sysfs_trig_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:iio-trig-sysfs");
-- 
1.6.0.2

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-02:
> On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
>> Greg KH wrote on 2011-02-02:
>>> On Wed, Feb 02, 2011 at 08:21:08PM +0100,
>>> michael.hennerich@analog.com
>>> wrote:
>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>>> This patch adds a new trigger that can be invoked by writing the
>>>> sysfs
>>>> file: trigger_now. This approach can be valuable during automated
>>>> testing or in situations, where other trigger methods are not
>>>> applicable. For example no RTC or spare GPIOs. Last but not least
>>>> we can allow user space applications to produce triggers.
>>>>
>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>
>>> If you add a new sysfs file, you need to document it.  I'll not take
>>> this patch as-is because of that.
>>>
>>>> ---
>>>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>>>  ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 0
>>>>  deletions(-)  create mode
>>>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>>
>>>> diff --git a/drivers/staging/iio/trigger/Kconfig
>>>> b/drivers/staging/iio/trigger/Kconfig
>>>> index d842a58..c185e47 100644
>>>> --- a/drivers/staging/iio/trigger/Kconfig
>>>> +++ b/drivers/staging/iio/trigger/Kconfig
>>>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>>>      help
>>>>        Provides support for using GPIO pins as IIO triggers.
>>>> +config IIO_SYSFS_TRIGGER
>>>> +    tristate "SYSFS trigger"
>>>> +    depends on SYSFS
>>>> +    help
>>>> +      Provides support for using SYSFS entry as IIO triggers.
>>>
>>> Why would you ever want to not have this enabled?  Why is it a
>>> config option?  And shouldn't it depend on the IIO subsystem?
>>
>> You won't see this if you don't have IIO + triggers enabled.
>
> Ok, the dependancy on IIO comes from other parts of the Kconfig file,
> my mistake.
>
>> Are you asking me to add more help text here - or you didn't read the
>> commit log? Alternatively are you asking why IIO common trigger core
>> doesn't provide this flexibility?
>>
>> Sorry - please explain.
>
> My question is, why is this a config option at all?  Why would it not
> always be enabled?  What benefit is it if it is not enabled?

It's a config option, since the user must provide a platform resource for
it. If you're not planning to use it, why compile and include it at all?
In embedded systems, people try to minimize kernel or rootfs size.
And last but not least kernel startup time.

>>>> +
>>>>  endif # IIO_TRIGGER
>>>> diff --git a/drivers/staging/iio/trigger/Makefile
>>>> b/drivers/staging/iio/trigger/Makefile
>>>> index 10aeca5..504b9c0 100644
>>>> --- a/drivers/staging/iio/trigger/Makefile
>>>> +++ b/drivers/staging/iio/trigger/Makefile
>>>> @@ -4,3 +4,4 @@
>>>>
>>>>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) +=3D iio-trig-periodic-rtc.o
>>>>  obj-$(CONFIG_IIO_GPIO_TRIGGER) +=3D iio-trig-gpio.o
>>>> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) +=3D iio-trig-sysfs.o
>>>> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>> b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>> new file mode 100644
>>>> index 0000000..3434f3c
>>>> --- /dev/null
>>>> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>>>> @@ -0,0 +1,108 @@
>>>> +/*
>>>> + * Copyright 2011 Analog Devices Inc.
>>>> + *
>>>> + * Licensed under the GPL-2 or later.
>>>
>>> Are you sure you really mean "or later"?  Seriously, do your lawyers
>>> agree that this is what they want to have happen to this file?
>>
>> Why not later?
>
> Do you really mean for this code to be used in GPLv3 and newer
> licensed bodies of work, as well as other types of code bases?
>
> Also, as you have probably based it on gplv2 only code, you might not
> be able to make this type of claim.
>
> My original question remains, is this ok with your company's lawyers
> to have the file licensed in this manner?  If so, that's fine, but I
> need to ask.

Thanks you're right - I don't like the 'or later' anymore.

>>>> + *
>>>> + * iio-trig-sysfs.c
>>>
>>> Don't put the file name in the file itself, that's pointless :)
>>>
>>>> +MODULE_AUTHOR("Michael Hennerich
>>>> +<hennerich@blackfin.uclinux.org>");
>>>> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
>>>> +MODULE_LICENSE("GPL v2");
>>>
>>> What is going to cause this module to be loaded in the system?
>>> Are you relying on userspace to load it if it "knows" it is needed?
>>> That's really not nice, we should trigger off of some type of
> hardware.
>>
>> Well again - I don't understand what you mean here - you can load this
>> driver as module or more likely build into the kernel - which both
> works fine.
>
> Sure, but how will a system know to load it automatically, like all
> other modules are?  You should never have to manually load it, that's
> not the way Linux works anymore thanks to udev and friends.

I'll add the MODULE_ALIAS.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-02:
> > On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> > wrote:
> >> From: Michael Hennerich <michael.hennerich@analog.com>
> >>
> >> This patch adds a new trigger that can be invoked by writing the sysfs
> >> file: trigger_now. This approach can be valuable during automated
> >> testing or in situations, where other trigger methods are not
> >> applicable. For example no RTC or spare GPIOs. Last but not least we
> >> can allow user space applications to produce triggers.
> >>
> >> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> >> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> >
> > If you add a new sysfs file, you need to document it.  I'll not take
> > this patch as-is because of that.
> >
> >> ---
> >>  drivers/staging/iio/trigger/Kconfig          |    6 ++
> >>  drivers/staging/iio/trigger/Makefile         |    1 +
> >>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
> >>  ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 0
> >>  deletions(-)  create mode
> >> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
> >>
> >> diff --git a/drivers/staging/iio/trigger/Kconfig
> >> b/drivers/staging/iio/trigger/Kconfig
> >> index d842a58..c185e47 100644
> >> --- a/drivers/staging/iio/trigger/Kconfig
> >> +++ b/drivers/staging/iio/trigger/Kconfig
> >> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
> >>      help
> >>        Provides support for using GPIO pins as IIO triggers.
> >> +config IIO_SYSFS_TRIGGER
> >> +    tristate "SYSFS trigger"
> >> +    depends on SYSFS
> >> +    help
> >> +      Provides support for using SYSFS entry as IIO triggers.
> >
> > Why would you ever want to not have this enabled?  Why is it a config
> > option?  And shouldn't it depend on the IIO subsystem?
> 
> You won't see this if you don't have IIO + triggers enabled.

Ok, the dependancy on IIO comes from other parts of the Kconfig file, my
mistake.

> Are you asking me to add more help text here - or you didn't read the commit log?
> Alternatively are you asking why IIO common trigger core doesn't provide this flexibility?
> 
> Sorry - please explain.

My question is, why is this a config option at all?  Why would it not
always be enabled?  What benefit is it if it is not enabled?

> >> +
> >>  endif # IIO_TRIGGER
> >> diff --git a/drivers/staging/iio/trigger/Makefile
> >> b/drivers/staging/iio/trigger/Makefile
> >> index 10aeca5..504b9c0 100644
> >> --- a/drivers/staging/iio/trigger/Makefile
> >> +++ b/drivers/staging/iio/trigger/Makefile
> >> @@ -4,3 +4,4 @@
> >>
> >>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
> >>  obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
> >> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
> >> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
> >> b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> >> new file mode 100644
> >> index 0000000..3434f3c
> >> --- /dev/null
> >> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> >> @@ -0,0 +1,108 @@
> >> +/*
> >> + * Copyright 2011 Analog Devices Inc.
> >> + *
> >> + * Licensed under the GPL-2 or later.
> >
> > Are you sure you really mean "or later"?  Seriously, do your lawyers
> > agree that this is what they want to have happen to this file?
> 
> Why not later?

Do you really mean for this code to be used in GPLv3 and newer licensed
bodies of work, as well as other types of code bases?

Also, as you have probably based it on gplv2 only code, you might not be
able to make this type of claim.

My original question remains, is this ok with your company's lawyers to
have the file licensed in this manner?  If so, that's fine, but I need
to ask.

> >> + *
> >> + * iio-trig-sysfs.c
> >
> > Don't put the file name in the file itself, that's pointless :)
> >
> >> +MODULE_AUTHOR("Michael Hennerich
> >> +<hennerich@blackfin.uclinux.org>");
> >> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
> >> +MODULE_LICENSE("GPL v2");
> >
> > What is going to cause this module to be loaded in the system?  Are
> > you relying on userspace to load it if it "knows" it is needed?
> > That's really not nice, we should trigger off of some type of hardware.
> 
> Well again - I don't understand what you mean here - you can load this driver as module
> or more likely build into the kernel - which both works fine.

Sure, but how will a system know to load it automatically, like all
other modules are?  You should never have to manually load it, that's
not the way Linux works anymore thanks to udev and friends.

thanks,

greg k-h

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 08:31:07PM +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 12:26:41PM -0800, Greg KH wrote:
> > On Wed, Feb 02, 2011 at 07:50:01PM +0000, Mark Brown wrote:
> 
> > > - the MFD subsystem is one of the most obvious examples here.
> 
> > Don't make a device a "platform" device unless it really is one.  This
> > one isn't one, it's a "virtual" device, right?
> 
> The discussion then always comes down to the fact that this then means
> we have to define a new bus type which ends up being an exact copy of
> the platform bus which isn't exactly ideal.

Yeah, I understand :(

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Mark Brown]

On Wed, Feb 02, 2011 at 08:36:27PM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-02:

> >> +MODULE_AUTHOR("Michael Hennerich
> >> +<hennerich@blackfin.uclinux.org>");
> >> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
> >> +MODULE_LICENSE("GPL v2");

> > What is going to cause this module to be loaded in the system?  Are
> > you relying on userspace to load it if it "knows" it is needed?
> > That's really not nice, we should trigger off of some type of hardware.

> Well again - I don't understand what you mean here - you can load this
> driver as module or more likely build into the kernel - which both
> works fine.

You need a MODULE_ALIAS so udev/modprobe can figure out how to load the
right driver when the device appears on the bus.

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-02:
> On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> This patch adds a new trigger that can be invoked by writing the sysfs
>> file: trigger_now. This approach can be valuable during automated
>> testing or in situations, where other trigger methods are not
>> applicable. For example no RTC or spare GPIOs. Last but not least we
>> can allow user space applications to produce triggers.
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>
> If you add a new sysfs file, you need to document it.  I'll not take
> this patch as-is because of that.
>
>> ---
>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>  ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 0
>>  deletions(-)  create mode
>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>
>> diff --git a/drivers/staging/iio/trigger/Kconfig
>> b/drivers/staging/iio/trigger/Kconfig
>> index d842a58..c185e47 100644
>> --- a/drivers/staging/iio/trigger/Kconfig
>> +++ b/drivers/staging/iio/trigger/Kconfig
>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>      help
>>        Provides support for using GPIO pins as IIO triggers.
>> +config IIO_SYSFS_TRIGGER
>> +    tristate "SYSFS trigger"
>> +    depends on SYSFS
>> +    help
>> +      Provides support for using SYSFS entry as IIO triggers.
>
> Why would you ever want to not have this enabled?  Why is it a config
> option?  And shouldn't it depend on the IIO subsystem?

You won't see this if you don't have IIO + triggers enabled.
Are you asking me to add more help text here - or you didn't read the commi=
t log?
Alternatively are you asking why IIO common trigger core doesn't provide th=
is flexibility?

Sorry - please explain.

>> +
>>  endif # IIO_TRIGGER
>> diff --git a/drivers/staging/iio/trigger/Makefile
>> b/drivers/staging/iio/trigger/Makefile
>> index 10aeca5..504b9c0 100644
>> --- a/drivers/staging/iio/trigger/Makefile
>> +++ b/drivers/staging/iio/trigger/Makefile
>> @@ -4,3 +4,4 @@
>>
>>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) +=3D iio-trig-periodic-rtc.o
>>  obj-$(CONFIG_IIO_GPIO_TRIGGER) +=3D iio-trig-gpio.o
>> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) +=3D iio-trig-sysfs.o
>> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> new file mode 100644
>> index 0000000..3434f3c
>> --- /dev/null
>> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright 2011 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2 or later.
>
> Are you sure you really mean "or later"?  Seriously, do your lawyers
> agree that this is what they want to have happen to this file?

Why not later?

>> + *
>> + * iio-trig-sysfs.c
>
> Don't put the file name in the file itself, that's pointless :)
>
>> +MODULE_AUTHOR("Michael Hennerich
>> +<hennerich@blackfin.uclinux.org>");
>> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
>> +MODULE_LICENSE("GPL v2");
>
> What is going to cause this module to be loaded in the system?  Are
> you relying on userspace to load it if it "knows" it is needed?
> That's really not nice, we should trigger off of some type of hardware.

Well again - I don't understand what you mean here - you can load this driv=
er as module
or more likely build into the kernel - which both works fine.

Sorry in my pervious reply I missed your comments further below - because I=
 don't expected them.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Mark Brown]

On Wed, Feb 02, 2011 at 12:26:41PM -0800, Greg KH wrote:
> On Wed, Feb 02, 2011 at 07:50:01PM +0000, Mark Brown wrote:

> > - the MFD subsystem is one of the most obvious examples here.

> Don't make a device a "platform" device unless it really is one.  This
> one isn't one, it's a "virtual" device, right?

The discussion then always comes down to the fact that this then means
we have to define a new bus type which ends up being an exact copy of
the platform bus which isn't exactly ideal.

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 08:13:56PM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-02:
> > On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> > wrote:
> >> +static struct platform_driver iio_sysfs_trigger_driver = {
> >> +    .driver = {
> >> +            .name = "iio_sysfs_trigger",
> >> +            .owner = THIS_MODULE,
> >> +    },
> >> +    .probe = iio_sysfs_trigger_probe,
> >> +    .remove = __devexit_p(iio_sysfs_trigger_remove),
> >> +};
> >
> > Why is this a platform device?  It doesn't seem to be platform
> > specific at all, does it?
> 
> What else do you think it should be?

Not a platform one, as it really isn't assigned to a "platform", right?

> Construct driver on initcall, and use module parameters?
> IMHO it is platform specific, since platform device bus allows me to
> register and run multiple instances.
> Depending on the use case, which may vary from board/platform -
> multiple instances are required.
> 
> Like Mark indicated - in EMBEDDED if it doesn't belong to any other
> bus - it's likely a platform bus driver.

CONFIG_EMBEDDED is gone :)

But, I guess there's not really a way to create a simple "virtual"
device in any other manner, is there.  Maybe I need to fix that one of
these days...

Anyway, I guess it's ok, I just don't like it.

thanks,

greg k-h

> 
> Greetings,
> Michael
> 
> --
> Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif
> 

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 07:55:23PM +0000, Hennerich, Michael wrote:
> Greg KH wrote on 2011-02-02:
> > On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> > wrote:
> >> From: Michael Hennerich <michael.hennerich@analog.com>
> >>
> >> This patch adds a new trigger that can be invoked by writing the sysfs
> >> file: trigger_now. This approach can be valuable during automated
> >> testing or in situations, where other trigger methods are not
> >> applicable. For example no RTC or spare GPIOs. Last but not least we
> >> can allow user space applications to produce triggers.
> >>
> >> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> >> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> >
> > If you add a new sysfs file, you need to document it.  I'll not take
> > this patch as-is because of that.
> 
> Fair enough - I'll resubmit with the file added do the documentation.

What about my other questions?

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 07:50:01PM +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 11:43:18AM -0800, Greg KH wrote:
> > On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com wrote:
> 
> > > +static struct platform_driver iio_sysfs_trigger_driver = {
> > > +	.driver = {
> > > +		.name = "iio_sysfs_trigger",
> > > +		.owner = THIS_MODULE,
> > > +	},
> > > +	.probe = iio_sysfs_trigger_probe,
> > > +	.remove = __devexit_p(iio_sysfs_trigger_remove),
> > > +};
> 
> > Why is this a platform device?  It doesn't seem to be platform specific
> > at all, does it?
> 
> Platform devices are used throughout the embedded kernel for virtual
> devices

Yes, but that doesn't mean they should be.

> - the MFD subsystem is one of the most obvious examples here.

Don't make a device a "platform" device unless it really is one.  This
one isn't one, it's a "virtual" device, right?

thanks,

greg k-h

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-02:
> On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> wrote:
>> +static struct platform_driver iio_sysfs_trigger_driver =3D {
>> +    .driver =3D {
>> +            .name =3D "iio_sysfs_trigger",
>> +            .owner =3D THIS_MODULE,
>> +    },
>> +    .probe =3D iio_sysfs_trigger_probe,
>> +    .remove =3D __devexit_p(iio_sysfs_trigger_remove),
>> +};
>
> Why is this a platform device?  It doesn't seem to be platform
> specific at all, does it?

What else do you think it should be?
Construct driver on initcall, and use module parameters?
IMHO it is platform specific, since platform device bus allows me to regist=
er and run multiple instances.
Depending on the use case, which may vary from board/platform - multiple in=
stances are required.

Like Mark indicated - in EMBEDDED if it doesn't belong to any other bus - i=
t's likely a platform bus driver.

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

RE: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Hennerich, Michael]

Greg KH wrote on 2011-02-02:
> On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com
> wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> This patch adds a new trigger that can be invoked by writing the sysfs
>> file: trigger_now. This approach can be valuable during automated
>> testing or in situations, where other trigger methods are not
>> applicable. For example no RTC or spare GPIOs. Last but not least we
>> can allow user space applications to produce triggers.
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>
> If you add a new sysfs file, you need to document it.  I'll not take
> this patch as-is because of that.

Fair enough - I'll resubmit with the file added do the documentation.

>> ---
>>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>>  drivers/staging/iio/trigger/Makefile         |    1 +
>>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108
>>  ++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 0
>>  deletions(-)  create mode
>> 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
>>
>> diff --git a/drivers/staging/iio/trigger/Kconfig
>> b/drivers/staging/iio/trigger/Kconfig
>> index d842a58..c185e47 100644
>> --- a/drivers/staging/iio/trigger/Kconfig
>> +++ b/drivers/staging/iio/trigger/Kconfig
>> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>>      help
>>        Provides support for using GPIO pins as IIO triggers.
>> +config IIO_SYSFS_TRIGGER
>> +    tristate "SYSFS trigger"
>> +    depends on SYSFS
>> +    help
>> +      Provides support for using SYSFS entry as IIO triggers.
>
> Why would you ever want to not have this enabled?  Why is it a config
> option?  And shouldn't it depend on the IIO subsystem?
>
>
>
>> +
>>  endif # IIO_TRIGGER
>> diff --git a/drivers/staging/iio/trigger/Makefile
>> b/drivers/staging/iio/trigger/Makefile
>> index 10aeca5..504b9c0 100644
>> --- a/drivers/staging/iio/trigger/Makefile
>> +++ b/drivers/staging/iio/trigger/Makefile
>> @@ -4,3 +4,4 @@
>>
>>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) +=3D iio-trig-periodic-rtc.o
>>  obj-$(CONFIG_IIO_GPIO_TRIGGER) +=3D iio-trig-gpio.o
>> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) +=3D iio-trig-sysfs.o
>> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> new file mode 100644
>> index 0000000..3434f3c
>> --- /dev/null
>> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * Copyright 2011 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2 or later.
>
> Are you sure you really mean "or later"?  Seriously, do your lawyers
> agree that this is what they want to have happen to this file?
>
>> + *
>> + * iio-trig-sysfs.c
>
> Don't put the file name in the file itself, that's pointless :)
>
>> +MODULE_AUTHOR("Michael Hennerich
>> +<hennerich@blackfin.uclinux.org>");
>> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
>> +MODULE_LICENSE("GPL v2");
>
> What is going to cause this module to be loaded in the system?  Are
> you relying on userspace to load it if it "knows" it is needed?
> That's really not nice, we should trigger off of some type of hardware.
>
> thanks,
>
> greg k-h

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Gesch=
aeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret=
 Seif

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Mark Brown]

On Wed, Feb 02, 2011 at 11:43:18AM -0800, Greg KH wrote:
> On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com wrote:

> > +static struct platform_driver iio_sysfs_trigger_driver = {
> > +	.driver = {
> > +		.name = "iio_sysfs_trigger",
> > +		.owner = THIS_MODULE,
> > +	},
> > +	.probe = iio_sysfs_trigger_probe,
> > +	.remove = __devexit_p(iio_sysfs_trigger_remove),
> > +};

> Why is this a platform device?  It doesn't seem to be platform specific
> at all, does it?

Platform devices are used throughout the embedded kernel for virtual
devices - the MFD subsystem is one of the most obvious examples here.

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com wrote:
> +static struct platform_driver iio_sysfs_trigger_driver = {
> +	.driver = {
> +		.name = "iio_sysfs_trigger",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = iio_sysfs_trigger_probe,
> +	.remove = __devexit_p(iio_sysfs_trigger_remove),
> +};

Why is this a platform device?  It doesn't seem to be platform specific
at all, does it?

thanks,

greg k-h

Re: [PATCH] IIO: TRIGGER: New sysfs based trigger

[Greg KH]

On Wed, Feb 02, 2011 at 08:21:08PM +0100, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> This patch adds a new trigger that can be invoked by writing
> the sysfs file: trigger_now. This approach can be valuable during
> automated testing or in situations, where other trigger methods
> are not applicable. For example no RTC or spare GPIOs.
> Last but not least we can allow user space applications to produce triggers.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

If you add a new sysfs file, you need to document it.  I'll not take
this patch as-is because of that.

> ---
>  drivers/staging/iio/trigger/Kconfig          |    6 ++
>  drivers/staging/iio/trigger/Makefile         |    1 +
>  drivers/staging/iio/trigger/iio-trig-sysfs.c |  108 ++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c
> 
> diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
> index d842a58..c185e47 100644
> --- a/drivers/staging/iio/trigger/Kconfig
> +++ b/drivers/staging/iio/trigger/Kconfig
> @@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
>  	help
>  	  Provides support for using GPIO pins as IIO triggers.
>  
> +config IIO_SYSFS_TRIGGER
> +	tristate "SYSFS trigger"
> +	depends on SYSFS
> +	help
> +	  Provides support for using SYSFS entry as IIO triggers.

Why would you ever want to not have this enabled?  Why is it a config
option?  And shouldn't it depend on the IIO subsystem?



> +
>  endif # IIO_TRIGGER
> diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
> index 10aeca5..504b9c0 100644
> --- a/drivers/staging/iio/trigger/Makefile
> +++ b/drivers/staging/iio/trigger/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
>  obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
> +obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> new file mode 100644
> index 0000000..3434f3c
> --- /dev/null
> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2 or later.

Are you sure you really mean "or later"?  Seriously, do your lawyers
agree that this is what they want to have happen to this file?

> + *
> + * iio-trig-sysfs.c

Don't put the file name in the file itself, that's pointless :)

> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
> +MODULE_LICENSE("GPL v2");

What is going to cause this module to be loaded in the system?  Are you
relying on userspace to load it if it "knows" it is needed?  That's
really not nice, we should trigger off of some type of hardware.

thanks,

greg k-h

[PATCH] IIO: TRIGGER: New sysfs based trigger

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

This patch adds a new trigger that can be invoked by writing
the sysfs file: trigger_now. This approach can be valuable during
automated testing or in situations, where other trigger methods
are not applicable. For example no RTC or spare GPIOs.
Last but not least we can allow user space applications to produce triggers.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/trigger/Kconfig          |    6 ++
 drivers/staging/iio/trigger/Makefile         |    1 +
 drivers/staging/iio/trigger/iio-trig-sysfs.c |  108 ++++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/trigger/iio-trig-sysfs.c

diff --git a/drivers/staging/iio/trigger/Kconfig b/drivers/staging/iio/trigger/Kconfig
index d842a58..c185e47 100644
--- a/drivers/staging/iio/trigger/Kconfig
+++ b/drivers/staging/iio/trigger/Kconfig
@@ -18,4 +18,10 @@ config IIO_GPIO_TRIGGER
 	help
 	  Provides support for using GPIO pins as IIO triggers.
 
+config IIO_SYSFS_TRIGGER
+	tristate "SYSFS trigger"
+	depends on SYSFS
+	help
+	  Provides support for using SYSFS entry as IIO triggers.
+
 endif # IIO_TRIGGER
diff --git a/drivers/staging/iio/trigger/Makefile b/drivers/staging/iio/trigger/Makefile
index 10aeca5..504b9c0 100644
--- a/drivers/staging/iio/trigger/Makefile
+++ b/drivers/staging/iio/trigger/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_IIO_PERIODIC_RTC_TRIGGER) += iio-trig-periodic-rtc.o
 obj-$(CONFIG_IIO_GPIO_TRIGGER) += iio-trig-gpio.o
+obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c b/drivers/staging/iio/trigger/iio-trig-sysfs.c
new file mode 100644
index 0000000..3434f3c
--- /dev/null
+++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * iio-trig-sysfs.c
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "../iio.h"
+#include "../trigger.h"
+
+static ssize_t iio_sysfs_trigger_poll(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	iio_trigger_poll(trig, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
+static IIO_TRIGGER_NAME_ATTR;
+
+static struct attribute *iio_sysfs_trigger_attrs[] = {
+	&dev_attr_trigger_now.attr,
+	&dev_attr_name.attr,
+	NULL,
+};
+
+static const struct attribute_group iio_sysfs_trigger_attr_group = {
+	.attrs = iio_sysfs_trigger_attrs,
+};
+
+static int __devinit iio_sysfs_trigger_probe(struct platform_device *pdev)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_allocate_trigger();
+	if (!trig) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	trig->control_attrs = &iio_sysfs_trigger_attr_group;
+	trig->owner = THIS_MODULE;
+	trig->name = kasprintf(GFP_KERNEL, "sysfstrig%d", pdev->id);
+	if (trig->name == NULL) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto out3;
+
+	platform_set_drvdata(pdev, trig);
+
+	return 0;
+out3:
+	kfree(trig->name);
+out2:
+	iio_put_trigger(trig);
+out1:
+
+	return ret;
+}
+
+static int __devexit iio_sysfs_trigger_remove(struct platform_device *pdev)
+{
+	struct iio_trigger *trig = platform_get_drvdata(pdev);
+
+	iio_trigger_unregister(trig);
+	kfree(trig->name);
+	iio_put_trigger(trig);
+
+	return 0;
+}
+
+static struct platform_driver iio_sysfs_trigger_driver = {
+	.driver = {
+		.name = "iio_sysfs_trigger",
+		.owner = THIS_MODULE,
+	},
+	.probe = iio_sysfs_trigger_probe,
+	.remove = __devexit_p(iio_sysfs_trigger_remove),
+};
+
+static int __init iio_sysfs_trig_init(void)
+{
+	return platform_driver_register(&iio_sysfs_trigger_driver);
+}
+module_init(iio_sysfs_trig_init);
+
+static void __exit iio_sysfs_trig_exit(void)
+{
+	platform_driver_unregister(&iio_sysfs_trigger_driver);
+}
+module_exit(iio_sysfs_trig_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Sysfs based trigger for the iio subsystem");
+MODULE_LICENSE("GPL v2");
-- 
1.6.0.2