Message-ID: <CAEnQRZCT0RidzwkaUZ1KXaMoNzWaaxNyH+SMNSyLmpDEhzdzgA@mail.gmail.com>
Date:	Tue, 31 Mar 2015 16:20:34 +0300
From:	Daniel Baluta <daniel.baluta@...el.com>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Lars-Peter Clausen <lars@...afoo.de>,
	Daniel Baluta <daniel.baluta@...el.com>,
	Joel Becker <jlbec@...lplan.org>,
	Hartmut Knaack <knaack.h@....de>,
	Peter Meerwald <pmeerw@...erw.net>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"octavian.purdila@...el.com" <octavian.purdila@...el.com>
Subject: Re: [PATCH 1/3] iio: configfs: Add configfs support to IIO

On Sat, Mar 28, 2015 at 1:50 PM, Jonathan Cameron <jic23@...nel.org> wrote:
> On 27/03/15 17:17, Lars-Peter Clausen wrote:
>> On 03/25/2015 06:00 PM, Daniel Baluta wrote:
>>> This creates an IIO configfs subsystem named "iio", which has one default
>>> group named "triggers". This allows us to easily create/destroy software
>>> triggers. One must create a driver which implements iio_configfs_trigger.h
>>> interface and then add its trigger type to IIO configfs core.
>>>
>>> See Documentation/iio/iio_configfs.txt for more details on how configfs
>>> support for IIO works.
>>>
>>> Signed-off-by: Daniel Baluta <daniel.baluta@...el.com>
>>
>> Very good stuff, thanks.
>>
>> [...]
>>> diff --git a/drivers/iio/industrialio-configfs.c b/drivers/iio/industrialio-configfs.c
>>> new file mode 100644
>>> index 0000000..4d2133a
>>> --- /dev/null
>>> +++ b/drivers/iio/industrialio-configfs.c
>>> @@ -0,0 +1,297 @@
>>> +/*
>>> + * Industrial I/O configfs bits
>>> + *
>>> + * Copyright (c) 2015 Intel Corporation
>>> + *
>>> + * 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/configfs.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include <linux/iio/iio_configfs_trigger.h>
>>> +
>>> +static const char *trigger_types[] =
>>> +{
>>> +    "none",
>>> +};
>>
>> This doesn't necessarily need to be in the initial implementation, but it would be good instead of having a global registry inside the core module to have a set of functions that allow to register a configfs trigger. These functions can be called from the module that implements the trigger in the init and exit section. A helper macro similar to module_platform_driver that auto-generates those section would be helpful.
>>
>> Cause right now we do have the dependencies wrong. The core module has a symbol dependency on the trigger modules implementing the trigger. That means you need to load the trigger module before the core module and also means that if at least one trigger is build as a module the core also needs to be built as a module.
>>
>> E.g. lets say there are triggerA, triggerB and triggerC. All build as a module. If you want to use any of them you still have to load all of them since the core module has a dependency on all of them.

Good point. I think this is the correct solution. I will add this approach
in v2.

>>
>>> +
>>> +struct iio_configfs_ops iio_none_ops = {
>>> +    .get_freq    = iio_none_get_freq,
>>> +    .set_freq    = iio_none_set_freq,
>>> +    .probe        = iio_none_probe,
>>> +    .remove        = iio_none_remove,
>>> +};
>>> +
>>> +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);
>>> +}
>>> +
>>> +static unsigned int iio_trigger_get_type(const char *type_str)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
>>> +        if (!strncmp(trigger_types[i], type_str,
>>> +                strlen(trigger_types[i])))
>>> +            return i;
>>> +    }
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static
>>> +void iio_trigger_set_configfs_ops(struct iio_configfs_trigger_info *trig_info,
>>> +                  unsigned int type)
>>> +{
>>> +    switch (type) {
>>> +    case IIO_TRIGGER_TYPE_NONE:
>>> +        trig_info->configfs_ops = &iio_none_ops;
>>> +        break;
>>> +    default:
>>> +        pr_err("Setting configfs ops failed! Unknown type %d\n", type);
>>> +        break;
>>> +    }
>>> +}
>>
>
>> I wonder if it is not better to have a sub-directory per trigger type
>> and if you create a trigger in the sub-directory it is automatically
>> of that type.
> I agree, this would perhaps be more flexible / intuitive.

Great, this sounds more in the spirit of configfs. Will add it in v2.

>>
>> The advantage is that you can have e.g. trigger specific properties.
> The initial set of triggers will be (I guess!)
>
> * high resolution timer
> * sysfs (userspace trigger) - in parallel with it's existing sysfs interface
>   which unfortunately we'll have to keep for at least a few years.
> Later on, once we have in kernel interfaces for events I'd expect we'll
> want event fired triggers (grab data based on an event from the same device
> or a different one for that matter)...
>
> Anyhow, definitely going to want different properties for these!
>
>>
>> The downside is that you no longer have a global namespace and there
>> could be name collisions by creating triggers with the same name, but
>> with different types. This could be solved though by making sure that
>> the internal name is pre-fixed by the type. E.g. "hrtimer-foobar" or
>> "hrtimer/foobar".
> Yes, the prefix would be sensible.

It is sensible, but we will enforce it at the driver level.

>
>>> +
>>> +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_type_read(struct iio_trigger_item *item,
>>> +                      char *page)
>>> +{
>>> +    return sprintf(page, "%s\n", trigger_types[item->trigger_info->type]);
>>> +}
>>> +
>>> +static ssize_t iio_trigger_item_type_write(struct iio_trigger_item *item,
>>> +                       const char *page, size_t count)
>>> +{
>>> +    int type;
>>> +
>>> +    if (item->trigger_info->active)
>>> +        return -EBUSY;
>>> +
>>> +    type = iio_trigger_get_type(page);
>>> +    if (type < 0)
>>> +        return -EINVAL;
>>> +
>>> +    item->trigger_info->type = type;
>>> +
>>> +    iio_trigger_set_configfs_ops(item->trigger_info, type);
>>> +
>>> +    return count;
>>> +}
>>> +
>>> +static ssize_t iio_trigger_item_activate_read(struct iio_trigger_item *item,
>>> +                          char *page)
>>> +{
>>> +    return sprintf(page, "%d\n", item->trigger_info->active);
>>> +}
>>> +
>>> +static ssize_t iio_trigger_item_activate_write(struct iio_trigger_item *item,
>>> +                           const char *page, size_t count)
>>> +{
>>> +    bool requested_action;
>>> +    int ret;
>>> +
>>> +    ret = strtobool(page, &requested_action);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (requested_action == item->trigger_info->active)
>>> +        return -EINVAL;
>>> +
>>> +    if (requested_action)
>>> +        item->trigger_info->configfs_ops->probe(item->trigger_info);
>>> +    else
>>> +        item->trigger_info->configfs_ops->remove(item->trigger_info);
>>> +
>>> +    item->trigger_info->active = requested_action;
>>> +
>>> +    return count;
>>> +}
>>
>> Do we need the activate attribute? Shouldn't the trigger be
>> create/destroyed if the type field is changed? Or if we have one
>> directory per trigger type when the directory for the trigger is
>> created using mkdir? I think that would make a nice more natural
>> API.>
>> [...]
> Only exception would be a trigger that has no meaning until we have
> configured it - e.g. the event triggers mentioned above.  That would
> need an explicit activate.  Question is whether we want to start
> out with that interface for all triggers on the basis it will be
> needed later?

Yes, I was thinking at the case where the trigger needs first to be
configured and then activated. But for hrtimers I think we can remove
activate attribute for now.

>
> Actually come to think of it, there is no problem with registering
> an unconfigured trigger - just with using it.  Hence we could do
> so and only fault out when a buffer using the trigger is started.
> That works for the complex usecase (as long as we spit out appropriate
> log messages / error codes).
>
> Hence if we can get away with doing everything on the mkdir that
> would be great.

Thanks for the feedback. I will send v2 asap.

Daniel.
--
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