Re: [PATCH v3 2/6] iio: Introduce the generic counter interface

[Benjamin Gaignard]

2017-10-08 16:30 GMT+02:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
> On Thu,  5 Oct 2017 14:13:44 -0400
> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
>
>> This patch introduces the IIO generic counter interface for supporting
>> counter devices. The generic counter interface serves as a catch-all to
>> enable rudimentary support for devices that qualify as counters. More
>> specific and apt counter interfaces may be developed on top of the
>> generic counter interface, and those interfaces should be used by
>> drivers when possible rather than the generic counter interface.
>
> I think you probably want to avoid any additional layering and try to
> push things down here (maybe with a few utility functions to simplify
> common cases), but that's a question for another day!
>>
>> In the context of the IIO generic counter interface, a counter is
>> defined as a device that reports one or more "counter values" based on
>> the state changes of one or more "counter signals" as evaluated by a
>> defined "counter function."
>>
>> The IIO generic counter interface piggybacks off of the IIO core. This
>> is primarily used to leverage the existing sysfs setup: the IIO_COUNT
>> channel attributes represent the Counter Value, while the IIO_SIGNAL
>> channel attributes represent the Counter Signal; auxilary IIO_COUNT
>> attributes represent the Counter Signal connections (Triggers) and their
>> respective state change configurations which trigger an associated
>> "counter function" evaluation.
>>
>> The iio_counter_ops structure serves as a container for driver callbacks
>> to communicate with the device; function callbacks are provided to read
>> and write various Signals and Values, and set and get the "trigger mode"
>> and "function mode" for various Triggers and Values respectively.
>>
>> To support a counter device, a driver must first allocate the available
>> Counter Signals via iio_counter_signal structures. These Signals should
>> be stored as an array and set to the signals array member of an
>> allocated iio_counter structure before the Counter is registered to the
>> system.
>>
>> Counter Values may be allocated via iio_counter_value structures, and
>> respective Counter Signal associations (Triggers) made via
>> iio_counter_trigger structures. Associated iio_counter_trigger
>> structures are stored as an array and set to the the triggers array
>> member of the respective iio_counter_value structure. These
>> iio_counter_value structures are set to the values array member of an
>> allocated iio_counter structure before the Counter is registered to the
>> system.
>>
>> A counter device is registered to the system by passing the respective
>> initialized iio_counter structure to the iio_counter_register function;
>> similarly, the iio_counter_unregister function unregisters the
>> respective counter. The devm_iio_counter_register and
>> iio_devm_iio_counter_unregister functions serve as device memory-managed
>> versions of the iio_counter_register and iio_counter unregister
>> functions respectively.
>
> All the find by ids are the main complexity introduced by layering this
> on IIO that you probably wouldn't have if it wasn't so layered.  Hmm.
> I suggest inline that you could allow drivers to provide fast
> versions of that search given they can probably infer it directly from
> the index.  Might be something to ignore for now though in the interests
> of initial simplicity.
>
> Otherwise a few comments inline, but mostly this is coming together
> pretty well.  I'm actually surprised the layering on top of IIO didn't
> end up more painful than it did. It's not horrendous - which is not
> to say you wouldn't be better breaking free of that entirely...
> (I'm not sure either way).
>
> I think the big remaining questions are around naming more than anything
> else.  I don't like the name value when a user will think they are looking
> for a counter.  If you use counter though the current use of counter needs
> to change.
>
> Trigger is also, as has been raised, a rather overloaded term. Not totally
> obvious what a better term is but we shouldn't reuse that one...
>
> I'll try and think of something if you have no luck!
>
> Jonathan
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
>> ---
>>  MAINTAINERS                        |   7 +
>>  drivers/iio/Kconfig                |   8 +
>>  drivers/iio/Makefile               |   1 +
>>  drivers/iio/counter/Kconfig        |   1 +
>>  drivers/iio/industrialio-counter.c | 900 +++++++++++++++++++++++++++++++++++++
>>  include/linux/iio/counter.h        | 166 +++++++
>>  6 files changed, 1083 insertions(+)
>>  create mode 100644 drivers/iio/industrialio-counter.c
>>  create mode 100644 include/linux/iio/counter.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2281af4b41b6..8b7c37bed252 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6693,6 +6693,13 @@ F:     Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
>>  F:   Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
>>  F:   drivers/iio/adc/envelope-detector.c
>>
>> +IIO GENERIC COUNTER INTERFACE
>> +M:   William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
>> +L:   linux-iio@xxxxxxxxxxxxxxx
>> +S:   Maintained
>> +F:   drivers/iio/industrialio-counter.c
>> +F:   include/linux/iio/counter.h
>
> Don't forget the directory the drivers are going in ;)
> *laughs manically*
>> +
>>  IIO MULTIPLEXER
>>  M:   Peter Rosin <peda@xxxxxxxxxx>
>>  L:   linux-iio@xxxxxxxxxxxxxxx
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index b3c8c6ef0dff..78e01f4f5937 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -30,6 +30,14 @@ config IIO_CONFIGFS
>>         (e.g. software triggers). For more info see
>>         Documentation/iio/iio_configfs.txt.
>>
>> +config IIO_COUNTER
>> +     bool "Enable IIO counter support"
>> +     help
>> +       Provides IIO core support for counters. This API provides
>> +       a generic interface that serves as the building blocks to
>> +       create more complex counter interfaces. Rudimentary support
>> +       for counters is enabled.
>> +
>>  config IIO_TRIGGER
>>       bool "Enable triggered sampling support"
>>       help
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 93c769cd99bf..6427ff38f964 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -5,6 +5,7 @@
>>  obj-$(CONFIG_IIO) += industrialio.o
>>  industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>>  industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>> +industrialio-$(CONFIG_IIO_COUNTER) += industrialio-counter.o
>>  industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>
>>  obj-$(CONFIG_IIO_CONFIGFS) += industrialio-configfs.o
>> diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
>> index 474e1ac4e7c0..c8becfe78e28 100644
>> --- a/drivers/iio/counter/Kconfig
>> +++ b/drivers/iio/counter/Kconfig
>> @@ -4,6 +4,7 @@
>>  # When adding new entries keep the list in alphabetical order
>>
>>  menu "Counters"
>> +     depends on IIO_COUNTER
>>
>>  config 104_QUAD_8
>>       tristate "ACCES 104-QUAD-8 driver"
>> diff --git a/drivers/iio/industrialio-counter.c b/drivers/iio/industrialio-counter.c
>> new file mode 100644
>> index 000000000000..dfb982dae3a8
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-counter.c
>> @@ -0,0 +1,900 @@
>> +/*
>> + * Industrial I/O counter interface functions
>> + * Copyright (C) 2017 William Breathitt Gray
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/gfp.h>
>> +#include <linux/kernel.h>
>> +#include <linux/printk.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/types.h>
>> +
>> +#include <linux/iio/counter.h>
>> +
>> +#define iio_priv_get_counter(_indio_dev) \
>> +     (*(struct iio_counter **)iio_priv(_indio_dev))
>> +
>> +static struct iio_counter_signal *iio_counter_signal_find_by_id(
>> +     const struct iio_counter *const counter, const int id)
>> +{
>> +     size_t i;
>> +     struct iio_counter_signal *signal;
>> +
>> +     for (i = 0; i < counter->num_signals; i++) {
>> +             signal = counter->signals + i;
>> +             if (signal->id == id)
>> +                     return signal;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static struct iio_counter_trigger *iio_counter_trigger_find_by_id(
>> +     const struct iio_counter_value *const value, const int id)
>> +{
>> +     size_t i;
>> +     struct iio_counter_trigger *trigger;
>> +
>> +     for (i = 0; i < value->num_triggers; i++) {
>> +             trigger = value->triggers + i;
>> +             if (trigger->signal->id == id)
>> +                     return trigger;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static struct iio_counter_value *iio_counter_value_find_by_id(
>> +     const struct iio_counter *const counter, const int id)
>> +{
>> +     size_t i;
>> +     struct iio_counter_value *value;
>
> In most cases this mapping will be entirely obvious to the
> driver - perhaps provide an optional callback to let it
> provide it directly without any searching?
>
> We could always add this later though if we start getting
> drivers with lots of instances of the various parts...
>
>> +
>> +     for (i = 0; i < counter->num_values; i++) {
>> +             value = counter->values + i;
>> +             if (value->id == id)
>> +                     return value;
>> +     }
>> +
>> +     return NULL;
>> +}
>> +
>> +static ssize_t iio_counter_signal_name_read(struct iio_dev *indio_dev,
>> +     uintptr_t priv, const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     const struct iio_counter_signal *signal;
>> +
>> +     signal = iio_counter_signal_find_by_id(counter, chan->channel2);
>> +     if (!signal)
>> +             return -EINVAL;
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%s\n", signal->name);
>> +}
>> +
>> +static ssize_t iio_counter_value_name_read(struct iio_dev *indio_dev,
>> +     uintptr_t priv, const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     const struct iio_counter_value *value;
>> +
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%s\n", value->name);
>
> I mentioned before but I wonder if we can just push name down into
> the chan spec itself in some fashion.  Whether to associate it with
> existing datasheet_name or not is an open question however.
>
>> +}
>> +
>> +static ssize_t iio_counter_value_triggers_read(struct iio_dev *indio_dev,
>> +     uintptr_t priv, const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     size_t i;
>> +     const struct iio_counter_trigger *trigger;
>> +     ssize_t len = 0;
>> +
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     /* Print out a list of every Signal association to Value */
>> +     for (i = 0; i < value->num_triggers; i++) {
>> +             trigger = value->triggers + i;
>> +             len += snprintf(buf + len, PAGE_SIZE - len, "%d\t%s\t%s\n",
>> +                     trigger->signal->id, trigger->signal->name,
>> +                     trigger->trigger_modes[trigger->mode]);
>> +             if (len >= PAGE_SIZE)
>> +                     return -ENOMEM;
>> +     }
>> +
>> +     return len;
>> +}
>> +
>> +static ssize_t iio_counter_trigger_mode_read(struct iio_dev *indio_dev,
>> +     uintptr_t priv, const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     struct iio_counter_trigger *trigger;
>> +     const int signal_id = *(int *)((void *)priv);
>> +     int mode;
>> +
>> +     if (!counter->ops->trigger_mode_get)
>> +             return -EINVAL;
>> +
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     trigger = iio_counter_trigger_find_by_id(value, signal_id);
>> +     if (!trigger)
>> +             return -EINVAL;
>> +
>> +     mode = counter->ops->trigger_mode_get(counter, value, trigger);
>> +
>> +     if (mode < 0)
>> +             return mode;
>> +     else if (mode >= trigger->num_trigger_modes)
>> +             return -EINVAL;
>> +
>> +     trigger->mode = mode;
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%s\n", trigger->trigger_modes[mode]);
>> +}
>> +
>> +static ssize_t iio_counter_trigger_mode_write(struct iio_dev *indio_dev,
>> +     uintptr_t priv, const struct iio_chan_spec *chan, const char *buf,
>> +     size_t len)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     ssize_t err;
>> +     struct iio_counter_trigger *trigger;
>> +     const int signal_id = *(int *)((void *)priv);
>> +     unsigned int mode;
>> +
>> +     if (!counter->ops->trigger_mode_set)
>> +             return -EINVAL;
>> +
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     trigger = iio_counter_trigger_find_by_id(value, signal_id);
>> +     if (!trigger)
>> +             return -EINVAL;
>> +
>> +     for (mode = 0; mode < trigger->num_trigger_modes; mode++)
>> +             if (sysfs_streq(buf, trigger->trigger_modes[mode]))
>> +                     break;
>> +
>> +     if (mode >= trigger->num_trigger_modes)
>> +             return -EINVAL;
>> +
>> +     err = counter->ops->trigger_mode_set(counter, value, trigger, mode);
>> +     if (err)
>> +             return err;
>> +
>> +     trigger->mode = mode;
>> +
>> +     return len;
>> +}
>> +
>> +static ssize_t iio_counter_trigger_mode_available_read(
>> +     struct iio_dev *indio_dev, uintptr_t priv,
>> +     const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     ssize_t len = 0;
>> +     struct iio_counter_trigger *trigger;
>> +     const int signal_id = *(int *)((void *)priv);
>> +     unsigned int i;
>> +
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     trigger = iio_counter_trigger_find_by_id(value, signal_id);
>> +     if (!trigger)
>> +             return -EINVAL;
>> +
>> +     for (i = 0; i < trigger->num_trigger_modes; i++)
>> +             len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
>> +                     trigger->trigger_modes[i]);
>> +
>> +     buf[len - 1] = '\n';
>> +
>> +     return len;
>> +}
>> +
>> +static int iio_counter_value_function_set(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan, unsigned int mode)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     int err;
>> +
>> +     if (!counter->ops->value_function_set)
>> +             return -EINVAL;
>> +
>> +     /* Find relevant Value */
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     /* Map IIO core function_set to Generic Counter value_function_set */
>> +     err = counter->ops->value_function_set(counter, value, mode);
>> +     if (err)
>> +             return err;
>> +
>> +     value->mode = mode;
>> +
>> +     return 0;
>> +}
>> +
>> +static int iio_counter_value_function_get(struct iio_dev *indio_dev,
>> +     const struct iio_chan_spec *chan)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_value *value;
>> +     int retval;
>> +
>> +     if (!counter->ops->value_function_get)
>> +             return -EINVAL;
>> +
>> +     /* Find relevant Value */
>> +     value = iio_counter_value_find_by_id(counter, chan->channel2);
>
> Same argument as below - would the driver not be able to do this better?
> Often it would know a simple transform to get the right one...
>
>
>> +     if (!value)
>> +             return -EINVAL;
>> +
>> +     /* Map IIO core function_get to Generic Counter value_function_get */
>> +     retval = counter->ops->value_function_get(counter, value);
>> +     if (retval < 0)
>> +             return retval;
>> +     else if (retval >= value->num_function_modes)
>> +             return -EINVAL;
>> +
>> +     value->mode = retval;
>> +
>> +     return retval;
>> +}
>> +
>> +static int iio_counter_value_ext_info_alloc(struct iio_chan_spec *const chan,
>> +     struct iio_counter_value *const value)
>> +{
>> +     const struct iio_chan_spec_ext_info ext_info_default[] = {
>> +             {
>> +                     .name = "name",
>> +                     .shared = IIO_SEPARATE,
>> +                     .read = iio_counter_value_name_read
>> +             },
>> +             IIO_ENUM("function", IIO_SEPARATE, &value->function_enum),
>> +             {
>> +                     .name = "function_available",
>> +                     .shared = IIO_SEPARATE,
>> +                     .read = iio_enum_available_read,
>> +                     .private = (void *)&value->function_enum
>> +             },
>> +             {
>> +                     .name = "triggers",
>> +                     .shared = IIO_SEPARATE,
>> +                     .read = iio_counter_value_triggers_read
>> +             }
>> +     };
>> +     const size_t num_default = ARRAY_SIZE(ext_info_default);
>> +     const struct iio_chan_spec_ext_info ext_info_trigger[] = {
>> +             {
>> +                     .shared = IIO_SEPARATE,
>> +                     .read = iio_counter_trigger_mode_read,
>> +                     .write = iio_counter_trigger_mode_write
>> +             },
>> +             {
>> +                     .shared = IIO_SEPARATE,
>> +                     .read = iio_counter_trigger_mode_available_read
>> +             }
>> +     };
>> +     const size_t num_ext_info_trigger = ARRAY_SIZE(ext_info_trigger);
>> +     const size_t num_triggers_ext_info = num_ext_info_trigger *
>> +             value->num_triggers;
>> +     const size_t num_ext_info = num_default + num_triggers_ext_info + 1;
>> +     int err;
>> +     struct iio_chan_spec_ext_info *ext_info;
>> +     const struct iio_counter_trigger *trigger;
>> +     size_t i;
>> +     size_t j;
>> +
>> +     /* Construct function_enum for Value */
>> +     value->function_enum.items = value->function_modes;
>> +     value->function_enum.num_items = value->num_function_modes;
>> +     value->function_enum.set = iio_counter_value_function_set;
>> +     value->function_enum.get = iio_counter_value_function_get;
>> +
>> +     ext_info = kmalloc_array(num_ext_info, sizeof(*ext_info), GFP_KERNEL);
>> +     if (!ext_info)
>> +             return -ENOMEM;
>> +     ext_info[num_ext_info - 1].name = NULL;
>> +
>> +     /* Add ext_info for the name, function, function_available, and triggers
>> +      * attributes
>> +      */
>> +     memcpy(ext_info, ext_info_default, sizeof(ext_info_default));
>> +     /* Add ext_info for the trigger_signalX-Z and
>> +      * trigger_signalX-Z_available attributes for each Trigger
>> +      */
>> +     for (i = 0; i < num_triggers_ext_info; i += num_ext_info_trigger)
>> +             memcpy(ext_info + num_default + i, ext_info_trigger,
>> +                     sizeof(ext_info_trigger));
>> +
>> +     /* Set name for each trigger_signalX-Z and trigger_signalX-Z_available
>> +      * attribute; store the respective Signal ID address in each ext_info
>> +      * private member
>> +      */
>> +     for (i = num_default, j = 0; j < value->num_triggers; i++, j++) {
>> +             trigger = value->triggers + j;
>> +
>> +             ext_info[i].name = kasprintf(GFP_KERNEL, "trigger_signal%d-%d",
>> +                     chan->channel, trigger->signal->id);
>> +             if (!ext_info[i].name) {
>> +                     err = -ENOMEM;
>> +                     goto err_name_alloc;
>> +             }
>> +             ext_info[i].private = (void *)&trigger->signal->id;
>> +             i++;
>> +
>> +             ext_info[i].name = kasprintf(GFP_KERNEL,
>> +                     "trigger_signal%d-%d_available",
>> +                     chan->channel, trigger->signal->id);
>> +             if (!ext_info[i].name) {
>> +                     err = -ENOMEM;
>> +                     goto err_name_alloc;
>> +             }
>> +             ext_info[i].private = (void *)&trigger->signal->id;
>> +     }
>> +
>> +     chan->ext_info = ext_info;
>> +
>> +     return 0;
>> +
>> +err_name_alloc:
>> +     while (i-- > num_default)
>> +             kfree(ext_info[i].name);
>> +     kfree(ext_info);
>> +     return err;
>> +}
>> +
>> +static void iio_counter_value_ext_info_free(
>> +     const struct iio_chan_spec *const channel)
>> +{
>> +     size_t i;
>> +     const char *const prefix = "trigger_signal";
>> +     const size_t prefix_len = strlen(prefix);
>> +
>> +     for (i = 0; channel->ext_info[i].name; i++)
>> +             if (!strncmp(channel->ext_info[i].name, prefix, prefix_len))
>> +                     kfree(channel->ext_info[i].name);
>> +     kfree(channel->ext_info);
>> +}
>> +
>> +static const struct iio_chan_spec_ext_info iio_counter_signal_ext_info[] = {
>> +     {
>> +             .name = "name",
>> +             .shared = IIO_SEPARATE,
>> +             .read = iio_counter_signal_name_read
>> +     },
>> +     {}
>> +};
>> +
>> +static int iio_counter_channels_alloc(struct iio_counter *const counter)
>> +{
>> +     const size_t num_channels = counter->num_signals + counter->num_values +
>> +             counter->num_channels;
>> +     int err;
>> +     struct iio_chan_spec *channels;
>> +     size_t j;
>> +     struct iio_counter_value *value;
>> +     size_t i = counter->num_channels;
>> +     const struct iio_counter_signal *signal;
>> +
>> +     channels = kcalloc(num_channels, sizeof(*channels), GFP_KERNEL);
>
> Bring the calculation of num_channels down here - it will make it
> more obvious that this allows space for both sources of channel.
>
>> +     if (!channels)
>> +             return -ENOMEM;
>> +
>> +     /* If any channels were supplied on Counter registration,
>> +      * we add them here to the front of the array
>> +      */
>> +     memcpy(channels, counter->channels,
>> +             counter->num_channels * sizeof(*counter->channels));
>> +
>> +     /* Add channel for each Value */
>> +     for (j = 0; j < counter->num_values; j++) {
>> +             value = counter->values + j;
>> +
>> +             channels[i].type = IIO_COUNT;
>> +             channels[i].channel = counter->id;
>> +             channels[i].channel2 = value->id;
>> +             channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +             channels[i].indexed = 1;
>> +             channels[i].counter = 1;
>> +
>> +             /* Add channels for Triggers associated with Value */
> Add channels?  'i' would need incrementing.   Also that's not what this function
> is doing that I can see...
>> +             err = iio_counter_value_ext_info_alloc(channels + i, value);
>> +             if (err)
>> +                     goto err_value_ext_info_alloc;
>> +
>> +             i++;
>> +     }
>> +
>> +     /* Add channel for each Signal */
>> +     for (j = 0; j < counter->num_signals; j++) {
>> +             signal = counter->signals + j;
>> +
>> +             channels[i].type = IIO_SIGNAL;
>> +             channels[i].channel = counter->id;
>> +             channels[i].channel2 = signal->id;
>> +             channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +             channels[i].indexed = 1;
>> +             channels[i].counter = 1;
>> +             channels[i].ext_info = iio_counter_signal_ext_info;
>> +
>> +             i++;
>> +     }
>> +
>> +     counter->indio_dev->num_channels = num_channels;
>> +     counter->indio_dev->channels = channels;
>> +
>> +     return 0;
>> +
>> +err_value_ext_info_alloc:
>> +     while (i-- > counter->num_channels)
>> +             iio_counter_value_ext_info_free(channels + i);
>> +     kfree(channels);
>> +     return err;
>> +}
>> +
>> +static void iio_counter_channels_free(const struct iio_counter *const counter)
>> +{
>> +     size_t i = counter->num_channels + counter->indio_dev->num_channels;
>> +     const struct iio_chan_spec *const chans = counter->indio_dev->channels;
>> +
>> +     while (i-- > counter->num_channels)
>> +             /* Only IIO_COUNT channels need to be freed here */
>> +             if (chans[i].type == IIO_COUNT)
>> +                     iio_counter_value_ext_info_free(chans + i);
>> +
>> +     kfree(chans);
>> +}
>> +
>> +static int iio_counter_read_raw(struct iio_dev *indio_dev,
>> +     struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_signal *signal;
>> +     struct iio_counter_value *value;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     switch (chan->type) {
>> +     /* Map read_raw to signal_read for Signal */
>> +     case IIO_SIGNAL:
>> +             if (!counter->ops->signal_read)
>> +                     return -EINVAL;
>> +
>> +             signal = iio_counter_signal_find_by_id(counter, chan->channel2);
>> +             if (!signal)
>> +                     return -EINVAL;
>> +
>> +             return counter->ops->signal_read(counter, signal, val, val2);
>> +     /* Map read_raw to value_read for Value */
>> +     case IIO_COUNT:
>> +             if (!counter->ops->value_read)
>> +                     return -EINVAL;
>> +
>> +             value = iio_counter_value_find_by_id(counter, chan->channel2);
>> +             if (!value)
>> +                     return -EINVAL;
>> +
>> +             return counter->ops->value_read(counter, value, val, val2);
>> +     /* Map read_raw to read_raw for non-counter channel */
>> +     default:
>> +             if (counter->info && counter->info->read_raw)
>> +                     return counter->info->read_raw(indio_dev, chan, val,
>> +                             val2, mask);
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static int iio_counter_write_raw(struct iio_dev *indio_dev,
>> +     struct iio_chan_spec const *chan, int val, int val2, long mask)
>> +{
>> +     struct iio_counter *const counter = iio_priv_get_counter(indio_dev);
>> +     struct iio_counter_signal *signal;
>> +     struct iio_counter_value *value;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     switch (chan->type) {
>> +     /* Map write_raw to signal_write for Signal */
>> +     case IIO_SIGNAL:
>> +             if (!counter->ops->signal_write)
>> +                     return -EINVAL;
>> +
>> +             signal = iio_counter_signal_find_by_id(counter, chan->channel2);
>> +             if (!signal)
>> +                     return -EINVAL;
>
>         Hmm. have to search is certainly a little ugly.  Particularly as the driver
>         would be able to maintain this as a lookup.  I'd be tempted to pass the
>         signal id down into the callback rather than finding the signal in the core.
>         Might turn out messier though... :)
>
>> +
>> +             return counter->ops->signal_write(counter, signal, val, val2);
>> +     /* Map write_raw to value_write for Value */
>> +     case IIO_COUNT:
>> +             if (!counter->ops->value_write)
>> +                     return -EINVAL;
>> +
>> +             value = iio_counter_value_find_by_id(counter, chan->channel2);
>
> Again, this mapping is a simple lookup in the driver I think, perhaps push
> it down there?
>
>> +             if (!value)
>> +                     return -EINVAL;
>> +
>> +             return counter->ops->value_write(counter, value, val, val2);
>> +     /* Map write_raw to write_raw for non-counter channel */
>> +     default:
>> +             if (counter->info && counter->info->write_raw)
>> +                     return counter->info->write_raw(indio_dev, chan, val,
>> +                             val2, mask);
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static int iio_counter_signals_validate(const struct iio_counter *const counter)
>> +{
>> +     size_t i;
>> +     const struct iio_counter_signal *signal;
>> +     size_t j;
>> +     int curr_id;
>> +
>> +     /* At least one Signal must be defined */
>> +     if (!counter->num_signals || !counter->signals) {
>> +             pr_err("Counter '%d' Signals undefined\n", counter->id);
>> +             return -EINVAL;
>> +     }
>> +
>> +     for (i = 0; i < counter->num_signals; i++) {
>> +             signal = counter->signals + i;
>> +             /* No two Signals may share the same ID */
>> +             for (j = 0; j < i; j++) {
>> +                     curr_id = counter->signals[j].id;
>> +                     if (curr_id == signal->id) {
>> +                             pr_err("Duplicate Counter '%d' Signal '%d'\n",
>> +                                             counter->id, signal->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
>> +             for (j = i + 1; j < counter->num_signals; j++) {
>> +                     curr_id = counter->signals[j].id;
>> +                     if (curr_id == signal->id) {
>> +                             pr_err("Duplicate Counter '%d' Signal '%d'\n",
>> +                                             counter->id, signal->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
>
> Same as below.
>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int iio_counter_triggers_validate(const int counter_id,
>> +     const struct iio_counter_value *const value)
>> +{
>> +     size_t i;
>> +     const struct iio_counter_trigger *trigger;
>> +     size_t j;
>> +     int curr_id;
>> +
>> +     /* At least one Trigger must be defined */
>> +     if (!value->num_triggers || !value->triggers) {
>> +             pr_err("Counter '%d' Value '%d' Triggers undefined\n",
>> +                     counter_id, value->id);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Ensure all Triggers have a defined Signal; this prevents a possible
>> +      * NULL pointer dereference when later validating Trigger Signal IDs
>
> Fix comment syntax throughout...
>
>> +      */
>> +     for (i = 0; i < value->num_triggers; i++)
>> +             if (!value->triggers[i].signal) {
>> +                     pr_err("Counter '%d' Trigger '%zu' Signal undefined\n",
>> +                             counter_id, i);
>> +                     return -EINVAL;
>> +             }
>> +
>> +     /* Verify validity of each Trigger */
>> +     for (i = 0; i < value->num_triggers; i++) {
>> +             trigger = value->triggers + i;
>> +             /* No two Trigger Signals may share the same ID */
>> +             for (j = 0; j < i; j++) {
>> +                     curr_id = value->triggers[j].signal->id;
>> +                     if (curr_id == trigger->signal->id) {
>> +                             pr_err("Signal '%d' is already linked to Counter '%d' Value '%d'\n",
>> +                                     trigger->signal->id, counter_id,
>> +                                     value->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
>> +             for (j = i + 1; j < value->num_triggers; j++) {
>> +                     curr_id = value->triggers[j].signal->id;
>> +                     if (curr_id == trigger->signal->id) {
>> +                             pr_err("Signal '%d' is already linked to Counter '%d' Value '%d'\n",
>> +                                     trigger->signal->id, counter_id,
>> +                                     value->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
> Again, one loop with the condition changed so it doesn't fault on the i = j case -- see below and
> note I'm reviewing backwards...
>
>
>> +
>> +             /* At least one trigger mode must be defined for each Trigger */
>> +             if (!trigger->num_trigger_modes || !trigger->trigger_modes) {
>> +                     pr_err("Counter '%d' Signal '%d' trigger modes undefined\n",
>> +                             counter_id, trigger->signal->id);
>> +                     return -EINVAL;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int iio_counter_values_validate(const struct iio_counter *const counter)
>> +{
>
> Hmm. Pretty heavy weight checking.  Ah well, up to you.
>
>> +     size_t i;
>> +     const struct iio_counter_value *value;
>> +     size_t j;
>> +     int curr_id;
>> +     int err;
>> +
>> +     /* At least one Value must be defined */
>> +     if (!counter->num_values || !counter->values) {
>> +             pr_err("Counter '%d' Values undefined\n", counter->id);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Verify validity of each Value */
>> +     for (i = 0; i < counter->num_values; i++) {
>> +             value = counter->values + i;
>> +             /* No two Values may share the same ID */
>> +             for (j = 0; j < i; j++) {
>
> Single loop with a slightly change to condition
> if ((i != j) and (curr_id == value->id))
>
>> +                     curr_id = counter->values[j].id;
>> +                     if (curr_id == value->id) {
>> +                             pr_err("Duplicate Counter '%d' Value '%d'\n",
>> +                                     counter->id, value->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
>> +             for (j = i + 1; j < counter->num_values; j++) {
>> +                     curr_id = counter->values[j].id;
>> +                     if (curr_id == value->id) {
>> +                             pr_err("Duplicate Counter '%d' Value '%d'\n",
>> +                                     counter->id, value->id);
>> +                             return -EEXIST;
>> +                     }
>> +             }
>> +
>> +             /* At least one function mode must be defined for each Value */
>> +             if (!value->num_function_modes || !value->function_modes) {
>> +                     pr_err("Counter '%d' Value '%d' function modes undefined\n",
>> +                             counter->id, value->id);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             /* Verify the Triggers associated with each Value */
>> +             err = iio_counter_triggers_validate(counter->id, value);
>> +             if (err)
>> +                     return err;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +/**
>> + * iio_counter_register - register Counter to the system
>> + * @counter: pointer to IIO Counter to register
>> + *
>> + * This function piggybacks off of iio_device_register. First, the relevant
>> + * Counter members are validated; the signals and values members must be defined
>> + * and populated by valid Signal and Value structures respectively. Next, a
>> + * struct iio_dev is allocated by a call to iio_device_alloc and initialized for
>> + * the Counter, IIO channels are allocated, the Counter address is stored as the
>> + * private data, and finally iio_device_register is called.
>> + */
>> +int iio_counter_register(struct iio_counter *const counter)
>> +{
>> +     const struct iio_info info_default = {
>> +             .read_raw = iio_counter_read_raw,
>> +             .write_raw = iio_counter_write_raw
>> +     };
>> +     int err;
>> +     struct iio_info *info;
>> +     struct iio_counter **priv;
>> +
>> +     if (!counter)
>> +             return -EINVAL;
>> +
>> +     /* Verify that Signals are valid and IDs do not conflict */
>> +     err = iio_counter_signals_validate(counter);
>> +     if (err)
>> +             return err;
>> +
>> +     /* Verify that Values are valid and IDs do not conflict;
> Fix multiline comment syntax to standard kernel syntax.
> /*
>  * Verify
>  */
>> +      * Triggers for each Value are also verified for validity
>> +      */
>> +     err = iio_counter_values_validate(counter);
>> +     if (err)
>> +             return err;
>> +
>> +     counter->indio_dev = iio_device_alloc(sizeof(counter));
>> +     if (!counter->indio_dev)
>> +             return -ENOMEM;
>> +
>> +     info = kmalloc(sizeof(*info), GFP_KERNEL);
>> +     if (!info) {
>> +             err = -ENOMEM;
>> +             goto err_info_alloc;
>> +     }
>> +     /* If an iio_info has been supplied than we use that,
>> +      * otherwise we set all callbacks to NULL; iio_counter_read_raw
>> +      * and iio_counter_write_raw is used for read_raw and write_raw
>> +      * for either case in order to support counter functionality
>> +      * (supplied read_raw/write_raw will be called from within
>> +      * iio_counter_read_raw/iio_counter_write_raw for non-counter
>> +      * channels)
>> +      */
>
>> +     if (counter->info) {
>> +             memcpy(info, counter->info, sizeof(*counter->info));
>> +             info->read_raw = iio_counter_read_raw;
>> +             info->write_raw = iio_counter_write_raw;
>> +     } else {
>> +             memcpy(info, &info_default, sizeof(info_default));
>> +     }
>> +
>> +     counter->indio_dev->info = info;
>> +     counter->indio_dev->modes = INDIO_DIRECT_MODE;

As I have said in the previous version, stm32 driver will not work with
this mode, so I need way to change it.
One solution could be to use iio_priv to store an iio_counter structure with
ops, arrays etc... Maybe even iio_counter structure could be hiden from drivers.
It could require to split iio_counter allocation and registration to functions.

>> +     counter->indio_dev->name = counter->name;
>> +     counter->indio_dev->dev.parent = counter->dev;
>> +
>> +     /* IIO channels are allocated and set for Signals, Values, and Triggers;
>> +      * any auxiliary IIO channels provided in iio_counter are also set
>> +      */
>> +     err = iio_counter_channels_alloc(counter);
>> +     if (err)
>> +             goto err_channels_alloc;
>> +
>> +     /* Pointer to the counter is stored in indio_dev as a way to refer
>> +      * back to the counter from within various IIO callbacks
>> +      */
>> +     priv = iio_priv(counter->indio_dev);
>> +     memcpy(priv, &counter, sizeof(*priv));
>> +
>> +     err = iio_device_register(counter->indio_dev);
>> +     if (err)
>> +             goto err_iio_device_register;
>> +
>> +     return 0;
>> +
>> +err_iio_device_register:
>> +     iio_counter_channels_free(counter);
>> +err_channels_alloc:
>> +     kfree(info);
>> +err_info_alloc:
>> +     iio_device_free(counter->indio_dev);
>> +     return err;
>> +}
>> +EXPORT_SYMBOL(iio_counter_register);
>> +
>> +/**
>> + * iio_counter_unregister - unregister Counter from the system
>> + * @counter: pointer to IIO Counter to unregister
>> + *
>> + * The Counter is unregistered from the system. The indio_dev is unregistered
>> + * and all allocated memory is freed.
>> + */
>> +void iio_counter_unregister(struct iio_counter *const counter)
>> +{
>> +     const struct iio_info *const info = counter->indio_dev->info;
>> +
>> +     if (!counter)
>> +             return;
>> +
>> +     iio_device_unregister(counter->indio_dev);
>> +
>> +     iio_counter_channels_free(counter);
>> +
>> +     kfree(info);
>> +     iio_device_free(counter->indio_dev);
>> +}
>> +EXPORT_SYMBOL(iio_counter_unregister);
>> +
>> +static void devm_iio_counter_unreg(struct device *dev, void *res)
>> +{
>> +     iio_counter_unregister(*(struct iio_counter **)res);
>> +}
>> +
>> +/**
>> + * devm_iio_counter_register - Resource-managed iio_counter_register
>> + * @dev: Device to allocate iio_counter for
>> + * @counter: pointer to IIO Counter to register
>> + *
>> + * Managed iio_counter_register. The IIO counter registered with this
>> + * function is automatically unregistered on driver detach. This function
>> + * calls iio_counter_register internally. Refer to that function for more
>> + * information.
>> + *
>> + * If an iio counter registered with this function needs to be unregistered
>> + * separately, devm_iio_counter_unregister must be used.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int devm_iio_counter_register(struct device *dev,
>> +     struct iio_counter *const counter)
>> +{
>> +     struct iio_counter **ptr;
>> +     int ret;
>> +
>> +     ptr = devres_alloc(devm_iio_counter_unreg, sizeof(*ptr), GFP_KERNEL);
>> +     if (!ptr)
>> +             return -ENOMEM;
>> +
>> +     *ptr = counter;
>> +     ret = iio_counter_register(counter);
>> +     if (!ret)
>> +             devres_add(dev, ptr);
>> +     else
>> +             devres_free(ptr);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(devm_iio_counter_register);
>> +
>> +static int devm_iio_counter_match(struct device *dev, void *res, void *data)
>> +{
>> +     struct iio_counter **r = res;
>> +
>> +     if (!r || !*r) {
>> +             WARN_ON(!r || !*r);
>> +             return 0;
>> +     }
>> +
>> +     return *r == data;
>> +}
>> +
>> +/**
>> + * devm_iio_counter_unregister - Resource-managed iio_counter_unregister
>> + * @dev: Device this iio_counter belongs to
>> + * @counter: the iio counter associated with the device
>> + *
>> + * Unregister iio counter registered with devm_iio_counter_register.
>> + */
>> +void devm_iio_counter_unregister(struct device *dev,
>> +     struct iio_counter *const counter)
>> +{
>> +     int rc;
>> +
>> +     rc = devres_release(dev, devm_iio_counter_unreg,
>> +             devm_iio_counter_match, counter);
>> +     WARN_ON(rc);
>> +}
>> +EXPORT_SYMBOL(devm_iio_counter_unregister);
>> diff --git a/include/linux/iio/counter.h b/include/linux/iio/counter.h
>> new file mode 100644
>> index 000000000000..35645406711a
>> --- /dev/null
>> +++ b/include/linux/iio/counter.h
>> @@ -0,0 +1,166 @@
>> +/*
>> + * Industrial I/O counter interface
>> + * Copyright (C) 2017 William Breathitt Gray
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + */
>> +#ifndef _IIO_COUNTER_H_
>> +#define _IIO_COUNTER_H_
>> +
>> +#ifdef CONFIG_IIO_COUNTER
>> +
>> +#include <linux/device.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +/**
>> + * struct iio_counter_signal - IIO Counter Signal node
>> + * @id:              [DRIVER] unique ID used to identify signal
>> + * @name:    [DRIVER] device-specific signal name
>> + */
>> +struct iio_counter_signal {
>> +     int             id;
>> +     const char      *name;
>> +};
>> +
>> +/**
>> + * struct iio_counter_trigger - IIO Counter Trigger node
>> + * @mode:            [DRIVER] current trigger mode state
>> + * @trigger_modes:   [DRIVER] available trigger modes
>> + * @num_trigger_modes:       [DRIVER] number of modes specified in @trigger_modes
>> + * @signal:          [DRIVER] pointer to associated signal
>> + */
>> +struct iio_counter_trigger {
>> +     unsigned int                    mode;
>> +     const char *const               *trigger_modes;
>> +     unsigned int                    num_trigger_modes;
>> +     struct iio_counter_signal       *signal;
>
> Are signals actually changeable? - Or is that pointer constant?
>
>> +};
>> +
>> +/**
>> + * struct iio_counter_value - IIO Counter Value node
>> + * @id:                      [DRIVER] unique ID used to identify value
>> + * @name:            [DRIVER] device-specific value name
>> + * @mode:            [DRIVER] current function mode state
>> + * @function_modes:  [DRIVER] available function modes
>> + * @num_function_modes:      [DRIVER] number of modes specified in @function_modes
>> + * @triggers:                [DRIVER] array of triggers for initialization
>> + * @num_triggers:    [DRIVER] number of triggers specified in @triggers
>> + * @function_enum:   [INTERN] used internally to generate function attributes
>> + */
>> +struct iio_counter_value {
>> +     int                     id;
>> +     const char              *name;
>> +     unsigned int            mode;
>> +     const char *const       *function_modes;
>> +     unsigned int            num_function_modes;
>> +
>> +     struct iio_counter_trigger      *triggers;
>> +     size_t                          num_triggers;
>> +
>> +     struct iio_enum         function_enum;
>> +};
>> +
>> +struct iio_counter;
>> +
>> +/**
>> + * struct iio_counter_ops - IIO Counter related callbacks
>> + * @signal_read:     function to request a signal value from the device.
>> + *                   Return value will specify the type of value returned by
>> + *                   the device. val and val2 will contain the elements
>> + *                   making up the returned value.
>
> Add some detail on the importance of the return value.
>
>> + * @signal_write:    function to write a signal value to the device.
>> + *                      Parameters are interpreted the same as signal_read.
>
> You need a means of establishing the correct format for write (we needed it
> in IIO fairly early on too)
>
>> + * @trigger_mode_set:        function to set the trigger mode. mode is the index of
>> + *                   the requested mode from the value trigger_modes array.
>> + * @trigger_mode_get:        function to get the current trigger mode. Return value
>> + *                   will specify the index of the current mode from the
>> + *                   value trigger_modes array.
>> + * @value_read:              function to request a value value from the device.
>> + *                   Return value will specify the type of value returned by
>> + *                   the device. val and val2 will contain the elements
>> + *                   making up the returned value.
>> + * @value_write:     function to write a value value to the device.
>> + *                      Parameters are interpreted the same as value_read.
>> + * @value_function_set: function to set the value function mode. mode is the
>> + *                   index of the requested mode from the value
>> + *                   function_modes array.
>> + * @value_function_get: function to get the current value function mode. Return
>> + *                   value will specify the index of the current mode from
>> + *                   the value function_modes array.
>> + */
>> +struct iio_counter_ops {
>> +     int (*signal_read)(struct iio_counter *counter,
>> +             struct iio_counter_signal *signal, int *val, int *val2);
>> +     int (*signal_write)(struct iio_counter *counter,
>> +             struct iio_counter_signal *signal, int val, int val2);
>> +     int (*trigger_mode_set)(struct iio_counter *counter,
>> +             struct iio_counter_value *value,
>> +             struct iio_counter_trigger *trigger, unsigned int mode);
>> +     int (*trigger_mode_get)(struct iio_counter *counter,
>> +             struct iio_counter_value *value,
>> +             struct iio_counter_trigger *trigger);
>> +     int (*value_read)(struct iio_counter *counter,
>> +             struct iio_counter_value *value, int *val, int *val2);
>> +     int (*value_write)(struct iio_counter *counter,
>> +             struct iio_counter_value *value, int val, int val2);
>> +     int (*value_function_set)(struct iio_counter *counter,
>> +             struct iio_counter_value *value, unsigned int mode);
>> +     int (*value_function_get)(struct iio_counter *counter,
>> +             struct iio_counter_value *value);
>> +};
>> +
>> +/**
>> + * struct iio_counter - IIO Counter data structure
>> + * @id:                      [DRIVER] unique ID used to identify counter
>> + * @name:            [DRIVER] name of the device
>> + * @dev:             [DRIVER] device structure, should be assigned a parent
>> + *                   and owner
>> + * @ops:             [DRIVER] callbacks from driver for counter components
>> + * @signals:         [DRIVER] array of signals for initialization
>> + * @num_signals:     [DRIVER] number of signals specified in @signals
>> + * @values:          [DRIVER] array of values for initialization
>> + * @num_values:              [DRIVER] number of values specified in @values
>> + * @channels:                [DRIVER] channel specification structure table
>> + * @num_channels:    [DRIVER] number of channels specified in @channels
>> + * @info:            [DRIVER] callbacks and constant info from driver
>> + * @indio_dev:               [INTERN] industrial I/O device structure
>> + * @driver_data:     [DRIVER] driver data
>> + */
>> +struct iio_counter {
>
> Mentioned earlier - I think naming the device counter, is confusing.
> The counters should be named counter - call it iio_counter_device
> or iio_counter_group or something and keep counter for the things
> currently called value.
>
>> +     int                             id;
>> +     const char                      *name;
>> +     struct device                   *dev;
>> +     const struct iio_counter_ops    *ops;
>> +
>> +     struct iio_counter_signal       *signals;
>> +     size_t                          num_signals;
>> +     struct iio_counter_value        *values;
>> +     size_t                          num_values;
>> +
>> +     const struct iio_chan_spec      *channels;
>> +     size_t                          num_channels;
>> +     const struct iio_info           *info;
>> +
>> +     struct iio_dev  *indio_dev;
>> +     void            *driver_data;
>> +};
>> +
>> +int iio_counter_register(struct iio_counter *const counter);
>> +void iio_counter_unregister(struct iio_counter *const counter);
>> +int devm_iio_counter_register(struct device *dev,
>> +     struct iio_counter *const counter);
>> +void devm_iio_counter_unregister(struct device *dev,
>> +     struct iio_counter *const counter);
>> +
>> +#endif /* CONFIG_IIO_COUNTER */
>> +
>> +#endif /* _IIO_COUNTER_H_ */
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org â Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog