Message-ID: <552A91F1.70504@kernel.org>
Date:	Sun, 12 Apr 2015 16:40:33 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Daniel Baluta <daniel.baluta@...el.com>
CC:	Joel Becker <jlbec@...lplan.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Hartmut Knaack <knaack.h@....de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"octavian.purdila@...el.com" <octavian.purdila@...el.com>,
	Paul Bolle <pebolle@...cali.nl>, patrick.porlan@...el.com,
	adriana.reus@...el.com
Subject: Re: [PATCH v3 2/3] iio: trigger: Add support for highres timer trigger
 type

On 10/04/15 10:02, Daniel Baluta wrote:
> On Thu, Apr 9, 2015 at 8:09 PM, Jonathan Cameron <jic23@...nel.org> wrote:
>> On 06/04/15 15:18, Daniel Baluta wrote:
>>> This patch adds a new trigger type - the high resoluton timer ("hrtimer")
>>> under /config/iio/triggers/ and also creates a module that implements hrtimer
>>> trigger type functionality.
>>>
>>> A new IIO trigger is allocated when a new directory is created under
>>> /config/iio/triggers/. The name of the trigger is the same as the dir
>>> name. Each trigger will have a "delay" attribute representing the delay
>>> between two successive IIO trigger_poll calls.
>> Cool ;)
>>>
>>> Signed-off-by: Marten Svanfeldt <marten@...uitiveaerial.com>
>>> Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@...el.com>
>> I'd ideally have liked the break up of the patches to be slightly different.
>> There is stuff in here that would have made more sense in the first patch
>> as it isn't specific to this particular trigger.
> 
> Yes :), this was my first thought too (implemented in v1), but then I wanted
> to make a complete example on how to add a new trigger type.
I can see where you are coming from, but really think we need to
make it so there is less of each trigger implementation in the core.
Preferably none!
> 
> People implementing a new trigger type will find this patch very useful.
> 
> Anyhow, I have no objection on this. Can be fixed.
>>> ---
>>> Changes since v2:
>>>       * none
>>> Changes since v1:
>>>       * replaced sampling_frequency with delay to make the in kernel
>>>       processing easier
>> Then along comes me an suggests going back again ;)  Make your case...
> 
> Initial hrtimer trigger implementation was limited to using integer sampling
> frequencies. Adding "rational" frequencies (e.g 12.5 Hz) and converting them
> back to hrtimer trigger delay  would add some complex calculation in the kernel
> with the possibility of losing precision.
> 
> Besides that, for most devices sampling_frequencies (Hz) attribute is natural
> because this is what the devices expose in its registers, while the timers
> API uses delays (nsec).
> 
> Here is how I see the usage of device's sampling_frequency and trigger's
> delay (this should be kept in sync)
> 
> User application reads device's sampling_frequency and then based on that
> it sets trigger's delay to 1/sampling_frequency -- all of that happening in user
> space where floating point is easy :).
I'm not keen on the flipping backwards and forwards going on here.  To my
mind the two should be the same.

I'm a little unclear on when we'd actually hit your example and whether
this is the right approach if we do.

The usual case for using a 'software' trigger is for devices with no
sampling clock of their own.  So devices with a 'sample now' signal
(be this an explicit one or a case where the device samples using the
bus clock to drive the adc).

So here you are dealing with devices that would actually have a dataready
type signal, but it's not exposed? Any attempt to match sampling is just
destined to go wrong.  If you want to guarantee getting all the data you'll
have to over sample, probably by a significant degree given that you'll
have some variation in any software trigger.

In most (possibly) all devices there is a software means of reading whether
their is new data available.  I'd suggest in this case we do actually need
to move the polling logic into the driver.  It simply doesn't
make sense to use the generic timer to do it as we won't sample ever time.

Hence you'll end up with a trigger that has an underlying 'hidden' poll
of the device and only fires when there is new data available - much cleaner
interface to userspace and reliable data capture without repeat samples
and all the mess that will entail.
> 
>>>       * implemented hrtimer driver using the new API proposed in patch 1
>>>
>>>  drivers/iio/industrialio-configfs.c      | 164 +++++++++++++++++++++++++++++++
>>>  drivers/iio/trigger/Kconfig              |   9 ++
>>>  drivers/iio/trigger/Makefile             |   2 +
>>>  drivers/iio/trigger/iio-trig-hrtimer.c   | 154 +++++++++++++++++++++++++++++
>>>  include/linux/iio/iio_configfs_trigger.h |   1 +
>>>  5 files changed, 330 insertions(+)
>>>  create mode 100644 drivers/iio/trigger/iio-trig-hrtimer.c
>>>
>>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>>> index d8ffd76..a0e5d24 100644
>>> --- a/drivers/iio/industrialio-configfs.c
>>> +++ b/drivers/iio/industrialio-configfs.c
>>> @@ -71,7 +71,170 @@ struct iio_configfs_trigger_type *iio_get_configfs_trigger_type(int type)
>>>       return t;
>>>  }
>>>
>>> +struct iio_trigger_item {
>>> +     struct config_item item;
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +};
>>> +
>>> +static
>>> +inline struct iio_trigger_item *to_iio_trigger_item(struct config_item *item)
>>> +{
>>> +     if (!item)
>>> +             return NULL;
>>> +     return container_of(item, struct iio_trigger_item, item);
>>> +}
>>> +
>>> +CONFIGFS_ATTR_STRUCT(iio_trigger_item);
>>> +
>>> +#define IIO_TRIGGER_ITEM_ATTR(_name, _mode, _show, _store) \
>>> +struct iio_trigger_item_attribute iio_trigger_item_attr_##_name = \
>>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>>> +
>>> +static
>>> +ssize_t iio_trigger_item_get_delay(struct iio_trigger_item *item, char *page)
>>> +{
>>> +     unsigned long delay;
>>> +     int ret;
>>> +
>>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>>> +
>>> +     ret = t->trigger_ops->get_delay(item->trigger_info, &delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return snprintf(page, PAGE_SIZE, "%lu\n", delay);
>>> +}
>>> +
>>> +static
>>> +ssize_t iio_trigger_item_set_delay(struct iio_trigger_item *item,
>>> +                                const char *page,
>>> +                                size_t count)
>>> +{
>>> +     int ret;
>>> +     unsigned long delay;
>>> +     struct iio_configfs_trigger_type *t = item->trigger_info->trigger_type;
>>> +
>>> +     ret = kstrtoul(page, 10, &delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = t->trigger_ops->set_delay(item->trigger_info, delay);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return count;
>>> +}
>>> +
>>> +IIO_TRIGGER_ITEM_ATTR(delay, S_IRUGO | S_IWUSR,
>>> +                   iio_trigger_item_get_delay,
>>> +                   iio_trigger_item_set_delay);
>>> +
>>> +static struct configfs_attribute *iio_trigger_item_attrs[] = {
>>> +     &iio_trigger_item_attr_delay.attr,
>>> +     NULL
>>> +};
>>> +
>>> +CONFIGFS_ATTR_OPS(iio_trigger_item);
>>> +
>>> +static struct configfs_item_operations iio_trigger_item_ops = {
>>> +     .show_attribute = iio_trigger_item_attr_show,
>>> +     .store_attribute = iio_trigger_item_attr_store,
>>> +};
>> So everything down to here would have fitted a little better in the previous
>> patch to my mind.
> 
> ok. As said above, one way of doing this would be to start with
> an empty configfs support than build new trigger types on top
> of it.
> 
>>
>>> +
>>> +static struct config_item_type iio_trigger_item_type;
>>> +
>>
>> Why is this hrtimer specific?  Only one real place that I can see...
>> I think we need to get that element out into the hrtimer module rather than
>> here.  A utility function doing most of this makes sense in the core
>> but not the hrtimer bit.
> 
> ok.
> 
>>
>>> +static struct config_item *iio_hrtimer_make_item(struct config_group *group,
>>> +                                              const char *name)
>>> +{
>>> +     struct iio_trigger_item *trigger_item;
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +     struct iio_configfs_trigger_type *trigger_type;
>>> +     int ret;
>>> +
>>> +     trigger_item = kzalloc(sizeof(*trigger_item), GFP_KERNEL);
>>> +     if (!trigger_item)
>>> +             return ERR_PTR(-ENOMEM);
>>> +     trigger_info = kzalloc(sizeof(*trigger_info), GFP_KERNEL);
>>> +     if (!trigger_info) {
>>> +             ret = -ENOMEM;
>>> +             goto out_free_trigger_item;
>>> +     }
>>> +
>>> +     trigger_info->name = kstrdup(name, GFP_KERNEL);
>>> +     if (!trigger_info->name) {
>>> +             ret = -ENOMEM;
>>> +             goto out_free_trigger_info;
>>> +     }
>>> +
>>> +     trigger_type = iio_get_configfs_trigger_type(IIO_TRIGGER_TYPE_HRTIMER);
>>> +     if (!trigger_type) {
>>> +             pr_err("Unable to get hrtimer trigger!\n");
>>> +             ret = -ENODEV;
>>> +             goto out_free_trigger_name;
>>> +     }
>>> +     ret = trigger_type->trigger_ops->probe(trigger_info);
>>> +     if (ret < 0)
>>> +             goto out_error_module_put;
>>> +
>>> +     trigger_info->trigger_type = trigger_type;
>>> +     trigger_item->trigger_info = trigger_info;
>>> +
>>> +     config_item_init_type_name(&trigger_item->item, name,
>>> +                                &iio_trigger_item_type);
>>> +
>>> +     return &trigger_item->item;
>>> +out_error_module_put:
>>> +     module_put(trigger_type->owner);
>>> +out_free_trigger_name:
>>> +     kfree(trigger_info->name);
>>> +out_free_trigger_info:
>>> +     kfree(trigger_info);
>>> +out_free_trigger_item:
>>> +     kfree(trigger_item);
>>> +     return ERR_PTR(ret);
>>> +}
>>> +
>> This one is no way at all hrtimer specific that I can see!
> 
> Well, here we register the iio_trigger_item_type, which has the
> "iio_trigger_item_attrs" attributes field.
Yeah, a slight exageration on my part there ;)
> 
> If a new trigger type has a different set of attributes then
> iio_trigger_item_type
> would need a different instance.
> 
> But I see now how "iio_hrtimer_make_item" needs to be made generic.
cool
> 
>>> +static void iio_hrtimer_drop_item(struct config_group *group,
>>> +                               struct config_item *item)
>>> +{
>>> +     struct iio_trigger_item *trigger_item;
>>> +     struct iio_configfs_trigger_type *trigger_type;
>>> +
>>> +     trigger_item = container_of(item, struct iio_trigger_item, item);
>>> +     trigger_type = trigger_item->trigger_info->trigger_type;
>>> +
>>> +     trigger_type->trigger_ops->remove(trigger_item->trigger_info);
>>> +     module_put(trigger_type->owner);
>>> +     kfree(trigger_item->trigger_info->name);
>>> +     kfree(trigger_item->trigger_info);
>>> +     kfree(trigger_item);
>>> +}
>>> +
>>> +static struct configfs_group_operations iio_triggers_hrtimer_group_ops = {
>> So to make the separation clean we need to create this object dynamically or pass
>> it through from the hrtimer trigger module.
>>> +     .make_item = iio_hrtimer_make_item,
>>> +     .drop_item = iio_hrtimer_drop_item,
>>> +};
>>> +
>>> +static struct config_item_type iio_trigger_item_type = {
>>> +     .ct_owner       = THIS_MODULE,
>>> +     .ct_item_ops    = &iio_trigger_item_ops,
>>> +     .ct_attrs       = iio_trigger_item_attrs,
>>> +};
>>> +
>>> +static struct config_item_type iio_triggers_hrtimer_type = {
>>> +     .ct_owner       = THIS_MODULE,
>>> +     .ct_group_ops   = &iio_triggers_hrtimer_group_ops,
>>> +};
>> So this one also needs to come from the hrtimer module.
> 
> Hmm. I would also like that hrtimer module to be independent of
> configfs stuff. For example, one could use hrtimer trigger from within
> the kernel without even the need of iio configfs support to be compiled
> in.
> 
> I will try to see if this is possible and also address your comment.
> 
>>> +
>>> +static struct config_group iio_triggers_hrtimer_group = {
>>> +     .cg_item = {
>>> +             .ci_namebuf = "hrtimer",
>>> +             .ci_type = &iio_triggers_hrtimer_type,
>>> +     },
>>> +};
>>> +
>>>  static struct config_group *iio_triggers_default_groups[] = {
>>> +     &iio_triggers_hrtimer_group,
>>>       NULL
>>>  };
>>>
>>> @@ -109,6 +272,7 @@ static struct configfs_subsystem iio_configfs_subsys = {
>>>
>>>  static int __init iio_configfs_init(void)
>>>  {
>>> +     config_group_init(&iio_triggers_hrtimer_group);
>> ahh... We have to do the init in this order? No dynamic creation later?
> 
> This can be done. But I'm not sure we really want this.
> 
> Now "hrtimer" is just a default group so that it automagically
> appears in /config/iio/triggers/ when the configfs is mounted.
> 
> If we want this to be dynamically created later then some userspace
> application should be aware of the "hrtimer" trigger type existence
> and create the hrtimer directory at some point.
See the tricks used in the usb gadget driver to solve an equivalent
issue.  There the core has no knowledge at all of what the different
function drivers are called or do.  That's what we want to be aiming for.
> 
>>>       config_group_init(&iio_triggers_group);
>>>       config_group_init(&iio_configfs_subsys.su_group);
>>>
>>> diff --git a/drivers/iio/trigger/Kconfig b/drivers/iio/trigger/Kconfig
>>> index 7999612..e21688e 100644
>>> --- a/drivers/iio/trigger/Kconfig
>>> +++ b/drivers/iio/trigger/Kconfig
>>> @@ -5,6 +5,15 @@
>>>
>>>  menu "Triggers - standalone"
>>>
>>> +config IIO_HRTIMER_TRIGGER
>>> +     tristate "High resolution timer trigger"
>>> +     depends on IIO_CONFIGFS
>>> +     help
>>> +       Provides a frequency based IIO trigger using hrtimers.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called iio-trig-hrtimer.
>>> +
>>>  config IIO_INTERRUPT_TRIGGER
>>>       tristate "Generic interrupt trigger"
>>>       help
>>> diff --git a/drivers/iio/trigger/Makefile b/drivers/iio/trigger/Makefile
>>> index 0694dae..fe06eb5 100644
>>> --- a/drivers/iio/trigger/Makefile
>>> +++ b/drivers/iio/trigger/Makefile
>>> @@ -3,5 +3,7 @@
>>>  #
>>>
>>>  # When adding new entries keep the list in alphabetical order
>>> +
>>> +obj-$(CONFIG_IIO_HRTIMER_TRIGGER) += iio-trig-hrtimer.o
>>>  obj-$(CONFIG_IIO_INTERRUPT_TRIGGER) += iio-trig-interrupt.o
>>>  obj-$(CONFIG_IIO_SYSFS_TRIGGER) += iio-trig-sysfs.o
>>> diff --git a/drivers/iio/trigger/iio-trig-hrtimer.c b/drivers/iio/trigger/iio-trig-hrtimer.c
>>> new file mode 100644
>>> index 0000000..0acde8d
>>> --- /dev/null
>>> +++ b/drivers/iio/trigger/iio-trig-hrtimer.c
>>> @@ -0,0 +1,154 @@
>>> +/**
>>> + * The industrial I/O periodic hrtimer trigger driver
>>> + *
>>> + * Copyright (C) Intuitive Aerial AB
>>> + * Written by Marten Svanfeldt, marten@...uitiveaerial.com
>>> + * Copyright (C) 2012, Analog Device Inc.
>>> + *   Author: Lars-Peter Clausen <lars@...afoo.de>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/hrtimer.h>
>>> +
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/iio_configfs_trigger.h>
>>> +
>>> +/* default delay in ns (100Hz) */
>>> +#define HRTIMER_TRIGGER_DEFAULT_DELAY 100000000
>>> +
>>> +struct iio_hrtimer_trig_info {
>>> +     struct iio_configfs_trigger_info *trigger_info;
>>> +     struct hrtimer timer;
>>> +     unsigned long delay;
>>> +};
>>> +
>>> +static enum hrtimer_restart iio_trig_hrtimer_trig(struct hrtimer *timer)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = container_of(timer, struct iio_hrtimer_trig_info, timer);
>>> +
>>> +     hrtimer_forward_now(timer, ns_to_ktime(trig_info->delay));
>>> +     iio_trigger_poll(trig_info->trigger_info->trigger);
>>> +
>>> +     return HRTIMER_RESTART;
>>> +}
>>> +
>>> +static int iio_trig_hrtimer_set_state(struct iio_trigger *trig, bool state)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(trig);
>>> +
>>> +     if (state)
>>> +             hrtimer_start(&trig_info->timer, ns_to_ktime(trig_info->delay),
>>> +                           HRTIMER_MODE_REL);
>>> +     else
>>> +             hrtimer_cancel(&trig_info->timer);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int iio_trig_hrtimer_get_delay(struct iio_configfs_trigger_info *t,
>>> +                            unsigned long *delay)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +     *delay = trig_info->delay;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int iio_trig_hrtimer_set_delay(struct iio_configfs_trigger_info *t,
>>> +                            unsigned long delay)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +
>>> +     if (!delay)
>>> +             return -EINVAL;
>>> +
>>> +     trig_info->delay = delay;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops iio_hrtimer_trigger_ops = {
>>> +     .owner = THIS_MODULE,
>>> +     .set_trigger_state = iio_trig_hrtimer_set_state,
>>> +};
>>> +
>>> +static int iio_trig_hrtimer_probe(struct iio_configfs_trigger_info *t)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +     int ret;
>>> +
>>> +     trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
>>> +     if (!trig_info)
>>> +             return -ENOMEM;
>>> +
>>> +     trig_info->trigger_info = t;
>>> +
>>> +     t->trigger = iio_trigger_alloc("%s", t->name);
>>> +     if (!t->trigger)
>>> +             return -ENOMEM;
>>> +
>>> +     iio_trigger_set_drvdata(t->trigger, trig_info);
>>> +     t->trigger->ops = &iio_hrtimer_trigger_ops;
>>> +
>>> +     hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>> +     trig_info->timer.function = iio_trig_hrtimer_trig;
>>> +
>>> +     trig_info->delay = HRTIMER_TRIGGER_DEFAULT_DELAY;
>>> +
>>> +     ret = iio_trigger_register(t->trigger);
>>> +     if (ret)
>>> +             goto err_free_trigger;
>>> +
>>> +     return 0;
>>> +err_free_trigger:
>>> +     iio_trigger_free(t->trigger);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int iio_trig_hrtimer_remove(struct iio_configfs_trigger_info *t)
>>> +{
>>> +     struct iio_hrtimer_trig_info *trig_info;
>>> +
>>> +     trig_info = iio_trigger_get_drvdata(t->trigger);
>>> +
>>> +     iio_trigger_unregister(t->trigger);
>>> +     hrtimer_cancel(&trig_info->timer);
>>> +     iio_trigger_free(t->trigger);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +struct iio_configfs_trigger_ops hrtimer_trigger_ops = {
>>> +     .get_delay      = iio_trig_hrtimer_get_delay,
>>> +     .set_delay      = iio_trig_hrtimer_set_delay,
>>> +     .probe          = iio_trig_hrtimer_probe,
>>> +     .remove         = iio_trig_hrtimer_remove,
>>> +};
>>> +
>>> +struct iio_configfs_trigger_type iio_configfs_hrtimer = {
>>> +     .type = IIO_TRIGGER_TYPE_HRTIMER,
>>> +     .owner = THIS_MODULE,
>>> +     .trigger_ops = &hrtimer_trigger_ops,
>>> +};
>>> +
>>> +module_iio_configfs_trigger_driver(iio_configfs_hrtimer);
>>> +
>>> +MODULE_AUTHOR("Marten Svanfeldt <marten@...uitiveaerial.com>");
>>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@...el.com>");
>>> +MODULE_DESCRIPTION("Periodic hrtimer trigger for the IIO subsystem");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/include/linux/iio/iio_configfs_trigger.h b/include/linux/iio/iio_configfs_trigger.h
>>> index 75e76f2..6233733 100644
>>> --- a/include/linux/iio/iio_configfs_trigger.h
>>> +++ b/include/linux/iio/iio_configfs_trigger.h
>>> @@ -10,6 +10,7 @@
>>>                     unregister_configfs_trigger)
>>>
>>>  enum {
>>> +     IIO_TRIGGER_TYPE_HRTIMER = 0,
>>>       IIO_TRIGGER_TYPE_MAX,
>>>  };
>>>
>>>
>>
> 
> Thanks a lot for your valuable feedback Jonathan :).
> 
> Daniel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists