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

From: William Breathitt Gray
Date: Mon Aug 28 2017 - 11:57:22 EST

Next message: Takashi Iwai: "Re: kvm splat in mmu_spte_clear_track_bits"
Previous message: Haiyang Zhang: "RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Aug 20, 2017 at 12:40:02PM +0100, Jonathan Cameron wrote:
>On Mon, 31 Jul 2017 12:03:23 -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.
>>
>> 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 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 "counter signals" and "counter values," and set and
>> get the "trigger mode" and "function mode" for various "counter signals"
>> and "counter values" respectively.
>>
>> To support a counter device, a driver must first allocate the available
>> "counter signals" via iio_counter_signal structures. These "counter
>> signals" should be stored as an array and set to the init_signals member
>> of an allocated iio_counter structure before the counter is registered.
>>
>> "Counter values" may be allocated via iio_counter_value structures, and
>> respective "counter signal" associations made via iio_counter_trigger
>> structures. Initial associated iio_counter_trigger structures may be
>> stored as an array and set to the the init_triggers member of the
>> respective iio_counter_value structure. These iio_counter_value
>> structures may be set to the init_values member of an allocated
>> iio_counter structure before the counter is registered if so desired.
>>
>> 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.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
>
>Hi William,
>
>A few minor points inline as a starting point. I'm going to want to dig
>into this in a lot more detail but don't have the time today (or possibly
>for a few more weeks - sorry about that!)

Thank you for the quick review. Looking over your review points and my
code again, it's become rather apparent to me that my implementation is
burdenly opaque with its lack of apt comments to document the rationale
of certain sections of code. I'll repair to remedy that in version 2 of
this patchset.

By the way, feel free to take your time reviewing, I'm in no particular
rush myself. In fact, with the anticipated proper documentation coming I
expect version 2 to a bit easily to digest for you than this intial
patchset. As well, in general I prefer to get this interface committed
late but correct, rather than soon but immature.

I've provided my responses inline to your specific review points.

Sincerely,

William Breathitt Gray

>
>Thanks,
>
>Jonathan
>> ---
>> MAINTAINERS | 7 +
>> drivers/iio/Kconfig | 8 +
>> drivers/iio/Makefile | 1 +
>> drivers/iio/counter/Kconfig | 1 +
>> drivers/iio/industrialio-counter.c | 1157 ++++++++++++++++++++++++++++++++++++
>> include/linux/iio/counter.h | 221 +++++++
>> 6 files changed, 1395 insertions(+)
>> create mode 100644 drivers/iio/industrialio-counter.c
>> create mode 100644 include/linux/iio/counter.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f6ef3f3e5000..44345ef25066 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6480,6 +6480,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
>> +
>> IIO SUBSYSTEM AND DRIVERS
>> M: Jonathan Cameron <jic23@xxxxxxxxxx>
>> R: Hartmut Knaack <knaack.h@xxxxxx>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index a918270d6f54..5ab8ed9ab7fc 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 33fa4026f92c..ca79f3860d1c 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 b37e5fc03149..3d46a790d8db 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..27c20919af62
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-counter.c
>> @@ -0,0 +1,1157 @@
>> +/*
>> + * 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/err.h>
>> +#include <linux/export.h>
>> +#include <linux/gfp.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/mutex.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>
>> +
>> +static struct iio_counter_signal *__iio_counter_signal_find_by_id(
>> + const struct iio_counter *const counter, const int id)
>> +{
>> + struct iio_counter_signal *iter;
>> +
>> + list_for_each_entry(iter, &counter->signal_list, list)
>> + if (iter->id == id)
>> + return iter;
>> +
>> + return NULL;
>> +}
>> +
>> +static struct iio_counter_trigger *__iio_counter_trigger_find_by_id(
>> + const struct iio_counter_value *const value, const int id)
>> +{
>> + struct iio_counter_trigger *iter;
>> +
>> + list_for_each_entry(iter, &value->trigger_list, list)
>> + if (iter->signal->id == id)
>> + return iter;
>> +
>> + return NULL;
>> +}
>> +
>> +static struct iio_counter_value *__iio_counter_value_find_by_id(
>> + const struct iio_counter *const counter, const int id)
>> +{
>> + struct iio_counter_value *iter;
>> +
>> + list_for_each_entry(iter, &counter->value_list, list)
>> + if (iter->id == id)
>> + return iter;
>> +
>> + return NULL;
>> +}
>> +
>> +static void __iio_counter_trigger_unregister_all(
>> + struct iio_counter_value *const value)
>> +{
>> + struct iio_counter_trigger *iter, *tmp_iter;
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> + list_for_each_entry_safe(iter, tmp_iter, &value->trigger_list, list)
>> + list_del(&iter->list);
>> + mutex_unlock(&value->trigger_list_lock);
>> +}
>> +
>> +static void __iio_counter_signal_unregister_all(
>> + struct iio_counter *const counter)
>> +{
>> + struct iio_counter_signal *iter, *tmp_iter;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + list_for_each_entry_safe(iter, tmp_iter, &counter->signal_list, list)
>> + list_del(&iter->list);
>> + mutex_unlock(&counter->signal_list_lock);
>> +}
>> +
>> +static void __iio_counter_value_unregister_all(
>> + struct iio_counter *const counter)
>> +{
>> + struct iio_counter_value *iter, *tmp_iter;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + list_for_each_entry_safe(iter, tmp_iter, &counter->value_list, list) {
>> + __iio_counter_trigger_unregister_all(iter);
>> +
>> + list_del(&iter->list);
>> + }
>> + mutex_unlock(&counter->value_list_lock);
>> +}
>> +
>> +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(indio_dev);
>> + const struct iio_counter_signal *signal;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + signal = __iio_counter_signal_find_by_id(counter, chan->channel2);
>> + mutex_unlock(&counter->signal_list_lock);
>> + 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(indio_dev);
>> + const struct iio_counter_value *value;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + mutex_unlock(&counter->value_list_lock);
>> + if (!value)
>> + return -EINVAL;
>> +
>> + return scnprintf(buf, PAGE_SIZE, "%s\n", value->name);
>> +}
>> +
>> +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(indio_dev);
>> + struct iio_counter_value *value;
>> + const struct iio_counter_trigger *trigger;
>> + ssize_t len = 0;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + len = -EINVAL;
>> + goto err_find_value;
>> + }
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> + list_for_each_entry(trigger, &value->trigger_list, list) {
>> + 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) {
>> + len = -ENOMEM;
>> + goto err_no_buffer_space;
>> + }
>> + }
>> +err_no_buffer_space:
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> +err_find_value:
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + 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(indio_dev);
>> + struct iio_counter_value *value;
>> + ssize_t ret;
>> + struct iio_counter_trigger *trigger;
>> + const int signal_id = *((int *)priv);
>> + int mode;
>> +
>> + if (!counter->ops->trigger_mode_get)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + ret = -EINVAL;
>> + goto err_value;
>> + }
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> +
>> + trigger = __iio_counter_trigger_find_by_id(value, signal_id);
>> + if (!trigger) {
>> + ret = -EINVAL;
>> + goto err_trigger;
>> + }
>> +
>> + mode = counter->ops->trigger_mode_get(counter, value, trigger);
>> +
>> + if (mode < 0) {
>> + ret = mode;
>> + goto err_trigger;
>> + } else if (mode >= trigger->num_trigger_modes) {
>> + ret = -EINVAL;
>> + goto err_trigger;
>> + }
>> +
>> + trigger->mode = mode;
>> +
>> + ret = scnprintf(buf, PAGE_SIZE, "%s\n", trigger->trigger_modes[mode]);
>> +
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return ret;
>> +
>> +err_trigger:
>> + mutex_unlock(&value->trigger_list_lock);
>> +err_value:
>> + mutex_unlock(&counter->value_list_lock);
>> + return ret;
>> +}
>> +
>> +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(indio_dev);
>> + struct iio_counter_value *value;
>> + ssize_t err;
>> + struct iio_counter_trigger *trigger;
>> + const int signal_id = *(int *)((void *)priv);
>Given you don't go through the void * cast in _read I'm thinking it's not
>needed here either.

I'm aware that we typically do not follow the C99 standard in the Linux
kernel code, but since the uintptr_t typedef is modeled after the C99
uintptr_t, I considered it appropriate to follow C99 in this case: the
C99 standard requires an explicit conversion to void * from intptr_t
before converting to another pointer type for dereferencing.

Despite the standard requirement, I agree that it is awkward to see the
explicit cast to void * (likely because uintptr_t is not intended to be
dereferenced in the context of the standard), so I'll be willing to
remove it in this case if you believe it'll make the code clearer to
understand.

Just out of curiousity: why was uintptr_t choosen for this parameter
rather than void *?

>
>> + unsigned int mode;
>> +
>> + if (!counter->ops->trigger_mode_set)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + err = -EINVAL;
>> + goto err_value;
>> + }
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> +
>> + trigger = __iio_counter_trigger_find_by_id(value, signal_id);
>> + if (!trigger) {
>> + err = -EINVAL;
>> + goto err_trigger;
>> + }
>> +
>> + for (mode = 0; mode < trigger->num_trigger_modes; mode++)
>> + if (sysfs_streq(buf, trigger->trigger_modes[mode]))
>> + break;
>> +
>> + if (mode >= trigger->num_trigger_modes) {
>> + err = -EINVAL;
>> + goto err_trigger;
>> + }
>> +
>> + err = counter->ops->trigger_mode_set(counter, value, trigger, mode);
>> + if (err)
>> + goto err_trigger;
>> +
>> + trigger->mode = mode;
>> +
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return len;
>> +
>> +err_trigger:
>> + mutex_unlock(&value->trigger_list_lock);
>> +err_value:
>> + mutex_unlock(&counter->value_list_lock);
>> + return err;
>> +}
>> +
>> +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(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;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + len = -EINVAL;
>> + goto err_no_value;
>> + }
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> +
>> + trigger = __iio_counter_trigger_find_by_id(value, signal_id);
>> + if (!trigger) {
>> + len = -EINVAL;
>> + goto err_no_trigger;
>> + }
>> +
>> + for (i = 0; i < trigger->num_trigger_modes; i++)
>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
>> + trigger->trigger_modes[i]);
>> +
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + buf[len - 1] = '\n';
>> +
>> + return len;
>> +
>> +err_no_trigger:
>> + mutex_unlock(&value->trigger_list_lock);
>> +err_no_value:
>> + mutex_unlock(&counter->value_list_lock);
>> + 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(indio_dev);
>> + struct iio_counter_value *value;
>> + int err;
>> +
>> + if (!counter->ops->trigger_mode_get)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + err = -EINVAL;
>> + goto err_value;
>> + }
>> +
>> + err = counter->ops->value_function_set(counter, value, mode);
>> + if (err)
>> + goto err_value;
>> +
>> + value->mode = mode;
>> +
>> +err_value:
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return err;
>> +}
>> +
>> +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(indio_dev);
>> + struct iio_counter_value *value;
>> + int retval;
>> +
>> + if (!counter->ops->trigger_mode_get)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + retval = -EINVAL;
>> + goto err_value;
>> + }
>> +
>> + retval = counter->ops->value_function_get(counter, value);
>> + if (retval < 0)
>> + goto err_value;
>> + else if (retval >= value->num_function_modes) {
>> + retval = -EINVAL;
>> + goto err_value;
>> + }
>> +
>> + value->mode = retval;
>> +
>> +err_value:
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + 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 = (uintptr_t)((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 struct list_head *pos;
>> + size_t num_triggers = 0;
>> + size_t num_triggers_ext_info;
>> + size_t num_ext_info;
>> + int err;
>> + struct iio_chan_spec_ext_info *ext_info;
>> + const struct iio_counter_trigger *trigger_pos;
>> + size_t i;
>> +
>> + 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;
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> +
>> + list_for_each(pos, &value->trigger_list)
>> + num_triggers++;
>> +
>> + num_triggers_ext_info = num_ext_info_trigger * num_triggers;
>> + num_ext_info = num_default + num_triggers_ext_info + 1;
>> +
>> + ext_info = kmalloc_array(num_ext_info, sizeof(*ext_info), GFP_KERNEL);
>> + if (!ext_info) {
>> + err = -ENOMEM;
>> + goto err_ext_info_alloc;
>> + }
>> + ext_info[num_ext_info - 1].name = NULL;
>> +
>> + memcpy(ext_info, ext_info_default, sizeof(ext_info_default));
>> + 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));
>> +
>> + i = num_default;
>> + list_for_each_entry(trigger_pos, &value->trigger_list, list) {
>> + ext_info[i].name = kasprintf(GFP_KERNEL, "trigger_signal%d-%d",
>> + chan->channel, trigger_pos->signal->id);
>> + if (!ext_info[i].name) {
>> + err = -ENOMEM;
>> + goto err_name_alloc;
>> + }
>> + ext_info[i].private = (void *)&trigger_pos->signal->id;
>> + i++;
>> +
>> + ext_info[i].name = kasprintf(GFP_KERNEL,
>> + "trigger_signal%d-%d_available",
>> + chan->channel, trigger_pos->signal->id);
>> + if (!ext_info[i].name) {
>> + err = -ENOMEM;
>> + goto err_name_alloc;
>> + }
>> + ext_info[i].private = (void *)&trigger_pos->signal->id;
>> + i++;
>> + }
>> +
>> + chan->ext_info = ext_info;
>> +
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> + return 0;
>> +
>> +err_name_alloc:
>> + while (i-- > num_default)
>> + kfree(ext_info[i].name);
>> + kfree(ext_info);
>> +err_ext_info_alloc:
>> + mutex_unlock(&value->trigger_list_lock);
>> + 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 struct list_head *pos;
>> + size_t num_channels = 0;
>> + int err;
>> + struct iio_chan_spec *channels;
>> + struct iio_counter_value *value_pos;
>> + size_t i = counter->num_channels;
>> + const struct iio_counter_signal *signal_pos;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> +
>> + list_for_each(pos, &counter->signal_list)
>> + num_channels++;
>> +
>> + if (!num_channels) {
>> + err = -EINVAL;
>> + goto err_no_signals;
>> + }
>> +
>> + mutex_lock(&counter->value_list_lock);
>> +
>> + list_for_each(pos, &counter->value_list)
>> + num_channels++;
>> +
>> + num_channels += counter->num_channels;
>> +
>> + channels = kcalloc(num_channels, sizeof(*channels), GFP_KERNEL);
>> + if (!channels) {
>> + err = -ENOMEM;
>> + goto err_channels_alloc;
>> + }
>> +
>> + memcpy(channels, counter->channels,
>> + counter->num_channels * sizeof(*counter->channels));
>> +
>> + list_for_each_entry(value_pos, &counter->value_list, list) {
>> + channels[i].type = IIO_COUNT;
>> + channels[i].channel = counter->id;
>> + channels[i].channel2 = value_pos->id;
>> + channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + channels[i].indexed = 1;
>> + channels[i].counter = 1;
>> +
>> + err = __iio_counter_value_ext_info_alloc(channels + i,
>> + value_pos);
>> + if (err)
>> + goto err_value_ext_info_alloc;
>> +
>> + i++;
>> + }
>> +
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + list_for_each_entry(signal_pos, &counter->signal_list, list) {
>> + channels[i].type = IIO_SIGNAL;
>> + channels[i].channel = counter->id;
>> + channels[i].channel2 = signal_pos->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++;
>> + }
>> +
>> + mutex_unlock(&counter->signal_list_lock);
>> +
>> + 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);
>> +err_channels_alloc:
>> + mutex_unlock(&counter->value_list_lock);
>> +err_no_signals:
>> + mutex_unlock(&counter->signal_list_lock);
>> + 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)
>> + 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(indio_dev);
>> + struct iio_counter_signal *signal;
>> + int retval;
>> + struct iio_counter_value *value;
>> +
>> + if (mask != IIO_CHAN_INFO_RAW)
>> + return -EINVAL;
>> +
>> + switch (chan->type) {
>> + case IIO_SIGNAL:
>> + if (!counter->ops->signal_read)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + signal = __iio_counter_signal_find_by_id(counter,
>> + chan->channel2);
>> + if (!signal) {
>> + mutex_unlock(&counter->signal_list_lock);
>> + return -EINVAL;
>> + }
>> +
>> + retval = counter->ops->signal_read(counter, signal, val, val2);
>> + mutex_unlock(&counter->signal_list_lock);
>> +
>> + return retval;
>> + case IIO_COUNT:
>> + if (!counter->ops->value_read)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + mutex_unlock(&counter->value_list_lock);
>> + return -EINVAL;
>> + }
>> +
>> + retval = counter->ops->value_read(counter, value, val, val2);
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return retval;
>> + 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(indio_dev);
>> + struct iio_counter_signal *signal;
>> + int retval;
>> + struct iio_counter_value *value;
>> +
>> + if (mask != IIO_CHAN_INFO_RAW)
>> + return -EINVAL;
>> +
>> + switch (chan->type) {
>> + case IIO_SIGNAL:
>> + if (!counter->ops->signal_write)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + signal = __iio_counter_signal_find_by_id(counter,
>> + chan->channel2);
>> + if (!signal) {
>> + mutex_unlock(&counter->signal_list_lock);
>> + return -EINVAL;
>> + }
>> +
>> + retval = counter->ops->signal_write(counter, signal, val, val2);
>> + mutex_unlock(&counter->signal_list_lock);
>> +
>> + return retval;
>> + case IIO_COUNT:
>> + if (!counter->ops->value_write)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + value = __iio_counter_value_find_by_id(counter, chan->channel2);
>> + if (!value) {
>> + mutex_unlock(&counter->value_list_lock);
>> + return -EINVAL;
>> + }
>> +
>> + retval = counter->ops->value_write(counter, value, val, val2);
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return retval;
>> + default:
>
>This case definitely needs a comment!
>I think you overwrite any write_raw callbacks passed in and hence this
>is a infinite recursion...
>Could be wrong though ;)

I apologize, this looks like one of those cases where I should have
provided some better documentation about the architecture of generic
counter interface implementation. I'll make sure to make this clearer in
the version 2 documentation and comments.

However, I'll try to explain what's going on. The generic counter
interface sits a layer above the IIO core, so drivers which use the
generic counter interface do not directly supply IIO core write_raw
callbacks, but rather provide generic counter signal_write/value_write
callbacks (a passed write_raw callback is possible for non-counter IIO
channels).

The generic counter interface hooks itself via __iio_counter_write_raw
to the IIO core write_raw. The __iio_counter_write_raw function handles
the mapping to ensure that signal_write gets called for a generic
counter Signal, value_write gets called for a generic counter Value, and
a passed write_raw gets called for a passed non-counter IIO channel.

The reason for this __iio_counter_write_raw function is to provide an
abstraction for the signal_write/value_write functions to have access to
generic counter interface parameters not available via the regular
write_raw parameters. In theory, a driver utilizing the generic counter
interface for a counter device does not need to know that it's utilizing
IIO core under the hood since to it can handle all its counter device
needs via the signal_write/value_write callbacks.

>
>
>> + if (counter->info && counter->info->write_raw)
>> + return counter->info->write_raw(indio_dev, chan, val,
>> + val2, mask);
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int __iio_counter_signal_register(struct iio_counter *const counter,
>> + struct iio_counter_signal *const signal)
>> +{
>> + int err;
>> +
>> + if (!counter || !signal)
>> + return -EINVAL;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + if (__iio_counter_signal_find_by_id(counter, signal->id)) {
>> + pr_err("Duplicate counter signal ID '%d'\n", signal->id);
>> + err = -EEXIST;
>> + goto err_duplicate_id;
>> + }
>> + list_add_tail(&signal->list, &counter->signal_list);
>> + mutex_unlock(&counter->signal_list_lock);
>> +
>> + return 0;
>> +
>> +err_duplicate_id:
>> + mutex_unlock(&counter->signal_list_lock);
>> + return err;
>> +}
>> +
>> +static void __iio_counter_signal_unregister(struct iio_counter *const counter,
>> + struct iio_counter_signal *const signal)
>> +{
>> + if (!counter || !signal)
>> + return;
>> +
>> + mutex_lock(&counter->signal_list_lock);
>> + list_del(&signal->list);
>> + mutex_unlock(&counter->signal_list_lock);
>> +}
>> +
>> +static int __iio_counter_signals_register(struct iio_counter *const counter,
>> + struct iio_counter_signal *const signals, const size_t num_signals)
>> +{
>> + size_t i;
>> + int err;
>> +
>> + if (!counter || !signals)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < num_signals; i++) {
>> + err = __iio_counter_signal_register(counter, signals + i);
>> + if (err)
>> + goto err_signal_register;
>> + }
>> +
>> + return 0;
>> +
>> +err_signal_register:
>> + while (i--)
>> + __iio_counter_signal_unregister(counter, signals + i);
>> + return err;
>> +}
>> +
>> +static void __iio_counter_signals_unregister(struct iio_counter *const counter,
>> + struct iio_counter_signal *signals, size_t num_signals)
>> +{
>> + if (!counter || !signals)
>> + return;
>> +
>> + while (num_signals--) {
>> + __iio_counter_signal_unregister(counter, signals);
>> + signals++;
>> + }
>> +}
>> +
>> +/**
>> + * iio_counter_trigger_register - register Trigger to Value
>> + * @value: pointer to IIO Counter Value for association
>> + * @trigger: pointer to IIO Counter Trigger to register
>> + *
>> + * The Trigger is added to the Value's trigger_list. A check is first performed
>> + * to verify that the respective Signal is not already linked to the Value; if
>> + * the respective Signal is already linked to the Value, the Trigger is not
>> + * added to the Value's trigger_list.
>> + *
>> + * NOTE: This function will acquire and release the Value's trigger_list_lock
>> + * during execution.
>> + */
>> +int iio_counter_trigger_register(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const trigger)
>> +{
>> + if (!value || !trigger || !trigger->signal)
>> + return -EINVAL;
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> + if (__iio_counter_trigger_find_by_id(value, trigger->signal->id)) {
>> + pr_err("Signal%d is already linked to counter value%d\n",
>> + trigger->signal->id, value->id);
>> + return -EEXIST;
>> + }
>> + list_add_tail(&trigger->list, &value->trigger_list);
>> + mutex_unlock(&value->trigger_list_lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(iio_counter_trigger_register);
>> +
>> +/**
>> + * iio_counter_trigger_unregister - unregister Trigger from Value
>> + * @value: pointer to IIO Counter Value of association
>> + * @trigger: pointer to IIO Counter Trigger to unregister
>> + *
>> + * The Trigger is removed from the Value's trigger_list.
>> + *
>> + * NOTE: This function will acquire and release the Value's trigger_list_lock
>> + * during execution.
>> + */
>> +void iio_counter_trigger_unregister(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const trigger)
>> +{
>> + if (!value || !trigger || !trigger->signal)
>> + return;
>> +
>> + mutex_lock(&value->trigger_list_lock);
>> + list_del(&trigger->list);
>> + mutex_unlock(&value->trigger_list_lock);
>> +}
>> +EXPORT_SYMBOL(iio_counter_trigger_unregister);
>> +
>> +/**
>> + * iio_counter_triggers_register - register an array of Triggers to Value
>> + * @value: pointer to IIO Counter Value for association
>> + * @triggers: array of pointers to IIO Counter Triggers to register
>> + *
>> + * The iio_counter_trigger_register function is called for each Trigger in the
>> + * array. The @triggers array is traversed for the first @num_triggers Triggers.
>> + *
>> + * NOTE: @num_triggers must not be greater than the size of the @triggers array.
>> + */
>> +int iio_counter_triggers_register(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const triggers, const size_t num_triggers)
>> +{
>> + size_t i;
>> + int err;
>> +
>> + if (!value || !triggers)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < num_triggers; i++) {
>> + err = iio_counter_trigger_register(value, triggers + i);
>> + if (err)
>> + goto err_trigger_register;
>> + }
>> +
>> + return 0;
>> +
>> +err_trigger_register:
>> + while (i--)
>> + iio_counter_trigger_unregister(value, triggers + i);
>> + return err;
>> +}
>> +EXPORT_SYMBOL(iio_counter_triggers_register);
>> +
>> +/**
>> + * iio_counter_triggers_unregister - unregister Triggers from Value
>> + * @value: pointer to IIO Counter Value of association
>> + * @triggers: array of pointers to IIO Counter Triggers to unregister
>> + *
>> + * The iio_counter_trigger_unregister function is called for each Trigger in the
>> + * array. The @triggers array is traversed for the first @num_triggers Triggers.
>> + *
>> + * NOTE: @num_triggers must not be greater than the size of the @triggers array.
>> + */
>> +void iio_counter_triggers_unregister(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *triggers, size_t num_triggers)
>> +{
>> + if (!value || !triggers)
>> + return;
>> +
>> + while (num_triggers--) {
>> + iio_counter_trigger_unregister(value, triggers);
>> + triggers++;
>> + }
>> +}
>> +EXPORT_SYMBOL(iio_counter_triggers_unregister);
>> +
>> +/**
>> + * iio_counter_value_register - register Value to Counter
>> + * @counter: pointer to IIO Counter for association
>> + * @value: pointer to IIO Counter Value to register
>> + *
>> + * The registration process occurs in two major steps. First, the Value is
>> + * initialized: trigger_list_lock is initialized, trigger_list is initialized,
>> + * and init_triggers if not NULL is passed to iio_counter_triggers_register.
>> + * Second, the Value is added to the Counter's value_list. A check is first
>> + * performed to verify that the Value is not already associated to the Counter
>> + * (via the Value's unique ID); if the Value is already associated to the
>> + * Counter, the Value is not added to the Counter's value_list and all of the
>> + * Value's Triggers are unregistered.
>> + *
>> + * NOTE: This function will acquire and release the Counter's value_list_lock
>> + * during execution.
>> + */
>> +int iio_counter_value_register(struct iio_counter *const counter,
>> + struct iio_counter_value *const value)
>> +{
>> + int err;
>> +
>> + if (!counter || !value)
>> + return -EINVAL;
>> +
>> + mutex_init(&value->trigger_list_lock);
>> + INIT_LIST_HEAD(&value->trigger_list);
>> +
>> + if (value->init_triggers) {
>> + err = iio_counter_triggers_register(value,
>> + value->init_triggers, value->num_init_triggers);
>> + if (err)
>> + return err;
>> + }
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + if (__iio_counter_value_find_by_id(counter, value->id)) {
>> + pr_err("Duplicate counter value ID '%d'\n", value->id);
>> + err = -EEXIST;
>> + goto err_duplicate_id;
>> + }
>> + list_add_tail(&value->list, &counter->value_list);
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + return 0;
>> +
>> +err_duplicate_id:
>> + mutex_unlock(&counter->value_list_lock);
>> + __iio_counter_trigger_unregister_all(value);
>> + return err;
>> +}
>> +EXPORT_SYMBOL(iio_counter_value_register);
>> +
>> +/**
>> + * iio_counter_value_unregister - unregister Value from Counter
>> + * @counter: pointer to IIO Counter of association
>> + * @value: pointer to IIO Counter Value to unregister
>> + *
>> + * The Value is removed from the Counter's value_list and all of the Value's
>> + * Triggers are unregistered.
>> + *
>> + * NOTE: This function will acquire and release the Counter's value_list_lock
>> + * during execution.
>> + */
>> +void iio_counter_value_unregister(struct iio_counter *const counter,
>> + struct iio_counter_value *const value)
>> +{
>> + if (!counter || !value)
>> + return;
>> +
>> + mutex_lock(&counter->value_list_lock);
>> + list_del(&value->list);
>> + mutex_unlock(&counter->value_list_lock);
>> +
>> + __iio_counter_trigger_unregister_all(value);
>> +}
>> +EXPORT_SYMBOL(iio_counter_value_unregister);
>> +
>> +/**
>> + * iio_counter_values_register - register an array of Values to Counter
>> + * @counter: pointer to IIO Counter for association
>> + * @values: array of pointers to IIO Counter Values to register
>> + *
>> + * The iio_counter_value_register function is called for each Value in the
>> + * array. The @values array is traversed for the first @num_values Values.
>> + *
>> + * NOTE: @num_values must not be greater than the size of the @values array.
>> + */
>> +int iio_counter_values_register(struct iio_counter *const counter,
>> + struct iio_counter_value *const values, const size_t num_values)
>> +{
>> + size_t i;
>> + int err;
>> +
>> + if (!counter || !values)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < num_values; i++) {
>> + err = iio_counter_value_register(counter, values + i);
>> + if (err)
>> + goto err_values_register;
>> + }
>> +
>> + return 0;
>> +
>> +err_values_register:
>> + while (i--)
>> + iio_counter_value_unregister(counter, values + i);
>> + return err;
>> +}
>> +EXPORT_SYMBOL(iio_counter_values_register);
>> +
>> +/**
>> + * iio_counter_values_unregister - unregister Values from Counter
>> + * @counter: pointer to IIO Counter of association
>> + * @values: array of pointers to IIO Counter Values to unregister
>> + *
>> + * The iio_counter_value_unregister function is called for each Value in the
>> + * array. The @values array is traversed for the first @num_values Values.
>> + *
>> + * NOTE: @num_values must not be greater than the size of the @values array.
>> + */
>> +void iio_counter_values_unregister(struct iio_counter *const counter,
>> + struct iio_counter_value *values, size_t num_values)
>> +{
>> + if (!counter || !values)
>> + return;
>> +
>> + while (num_values--) {
>> + iio_counter_value_unregister(counter, values);
>> + values++;
>> + }
>> +}
>> +EXPORT_SYMBOL(iio_counter_values_unregister);
>> +
>> +/**
>> + * 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 initialized; if init_signals is not NULL it is passed to
>> + * iio_counter_signals_register, and similarly if init_values is not NULL it is
>> + * passed to iio_counter_values_register. 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 is copied 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 = {
>> + .driver_module = THIS_MODULE,
>> + .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;
>> +
>> + mutex_init(&counter->signal_list_lock);
>> + INIT_LIST_HEAD(&counter->signal_list);
>> +
>> + if (counter->init_signals) {
>> + err = __iio_counter_signals_register(counter,
>> + counter->init_signals, counter->num_init_signals);
>> + if (err)
>> + return err;
>> + }
>> +
>> + mutex_init(&counter->value_list_lock);
>> + INIT_LIST_HEAD(&counter->value_list);
>> +
>> + if (counter->init_values) {
>> + err = iio_counter_values_register(counter,
>> + counter->init_values, counter->num_init_values);
>> + if (err)
>> + goto err_values_register;
>> + }
>> +
>> + counter->indio_dev = iio_device_alloc(sizeof(*counter));
>
>This is all getting a bit nasty and recursive. You have a
>counter containing an iio_dev containing a copy of the counter.
>
>I'd just put a pointer to the outer counter in there instead.

I agree that it does look like a roundabout method, and I should have
commented it better. I may not need to create a copy of the counter, so
I'll see if I can simply utilize the provided pointer to the outer
counter.

>
>
>> + if (!counter->indio_dev) {
>> + err = -ENOMEM;
>> + goto err_iio_device_alloc;
>> + }
>> +
>> + info = kmalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info) {
>> + err = -ENOMEM;
>> + goto err_info_alloc;
>> + }
>> + 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;
>> + counter->indio_dev->name = counter->name;
>> + counter->indio_dev->dev.parent = counter->dev;
>> +
>> + err = __iio_counter_channels_alloc(counter);
>> + if (err)
>> + goto err_channels_alloc;
>> +
>> + priv = iio_priv(counter->indio_dev);
>> + memcpy(priv, counter, sizeof(*priv));
>> +
>> + err = iio_device_register(priv->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);
>> +err_iio_device_alloc:
>> + iio_counter_values_unregister(counter, counter->init_values,
>> + counter->num_init_values);
>> +err_values_register:
>> + __iio_counter_signals_unregister(counter, counter->init_signals,
>> + counter->num_init_signals);
>> + 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,
>> + * allocated memory is freed, and all associated Values and Signals are
>> + * unregistered.
>> + */
>> +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);
>> +
>> + __iio_counter_value_unregister_all(counter);
>> + __iio_counter_signal_unregister_all(counter);
>> +}
>> +EXPORT_SYMBOL(iio_counter_unregister);
>> diff --git a/include/linux/iio/counter.h b/include/linux/iio/counter.h
>> new file mode 100644
>> index 000000000000..9a67c2a7cc46
>> --- /dev/null
>> +++ b/include/linux/iio/counter.h
>> @@ -0,0 +1,221 @@
>> +/*
>> + * 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/mutex.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
>> + * @list: [INTERN] list of all signals currently registered to counter
>> + */
>> +struct iio_counter_signal {
>> + int id;
>> + const char *name;
>> +
>> + struct list_head list;
>> +};
>> +
>> +/**
>> + * 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
>> + * @list: [INTERN] list of all triggers currently registered to
>> + * value
>> + */
>> +struct iio_counter_trigger {
>> + unsigned int mode;
>> + const char *const *trigger_modes;
>> + unsigned int num_trigger_modes;
>> + struct iio_counter_signal *signal;
>> +
>> + struct list_head list;
>> +};
>> +
>> +/**
>> + * 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
>> + * @init_triggers: [DRIVER] array of triggers for initialization
>> + * @num_init_triggers: [DRIVER] number of triggers specified in @init_triggers
>> + * @function_enum: [INTERN] used internally to generate function attributes
>> + * @trigger_list_lock: [INTERN] lock for accessing @trigger_list
>> + * @trigger_list: [INTERN] list of all triggers currently registered to
>> + * value
>> + * @list: [INTERN] list of all values currently registered to
>> + * counter
>> + */
>> +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 *init_triggers;
>> + size_t num_init_triggers;
>> +
>> + struct iio_enum function_enum;
>> + struct mutex trigger_list_lock;
>> + struct list_head trigger_list;
>> +
>> + struct list_head list;
>> +};
>> +
>> +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. Note that the counter
>> + * signal_list_lock is acquired before this function is
>> + * called, and released after this function returns.
>> + * @signal_write: function to write a signal value to the device.
>> + * Parameters and locking behavior are the same as
>> + * signal_read.
>> + * @trigger_mode_set: function to set the trigger mode. mode is the index of
>> + * the requested mode from the value trigger_modes array.
>> + * Note that the counter value_list_lock and value
>> + * trigger_list_lock are acquired before this function is
>> + * called, and released after this function returns.
>> + * @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. Locking behavior is the same
>> + * as trigger_mode_set.
>> + * @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. Note that the counter
>> + * value_list_lock is acquired before this function is
>> + * called, and released after this function returns.
>> + * @value_write: function to write a value value to the device.
>> + * Parameters and locking behavior are 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. Note that the counter
>> + * value_list_lock is acquired before this function is
>> + * called, and released after this function returns.
>> + * @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. Locking behavior is the
>> + * same as value_function_get.
>> + */
>> +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 for from driver
>> + * @init_signals: [DRIVER] array of signals for initialization
>> + * @num_init_signals: [DRIVER] number of signals specified in @init_signals
>> + * @init_values: [DRIVER] array of values for initialization
>> + * @num_init_values: [DRIVER] number of values specified in @init_values
>> + * @signal_list_lock: [INTERN] lock for accessing @signal_list
>> + * @signal_list: [INTERN] list of all signals currently registered to
>> + * counter
>> + * @value_list_lock: [INTERN] lock for accessing @value_list
>> + * @value_list: [INTERN] list of all values currently registered to
>> + * counter
>> + * @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 {
>> + int id;
>> + const char *name;
>> + struct device *dev;
>> + const struct iio_counter_ops *ops;
>> +
>> + struct iio_counter_signal *init_signals;
>> + size_t num_init_signals;
>I'm missing if there is any chance of signals or values being
>registered later? If not then drop the init_.

That's a good point, I'll update it to remove the init_ prefixes.

>> + struct iio_counter_value *init_values;
>> + size_t num_init_values;
>> +
>> + struct mutex signal_list_lock;
>> + struct list_head signal_list;
>> + struct mutex value_list_lock;
>> + struct list_head value_list;
>> +
>> + 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_trigger_register(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const trigger);
>> +void iio_counter_trigger_unregister(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const trigger);
>> +int iio_counter_triggers_register(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *const triggers, const size_t num_triggers);
>> +void iio_counter_triggers_unregister(struct iio_counter_value *const value,
>> + struct iio_counter_trigger *triggers, size_t num_triggers);
>> +
>> +int iio_counter_value_register(struct iio_counter *const counter,
>> + struct iio_counter_value *const value);
>> +void iio_counter_value_unregister(struct iio_counter *const counter,
>> + struct iio_counter_value *const value);
>> +int iio_counter_values_register(struct iio_counter *const counter,
>> + struct iio_counter_value *const values, const size_t num_values);
>> +void iio_counter_values_unregister(struct iio_counter *const counter,
>> + struct iio_counter_value *values, size_t num_values);
>> +
>> +int iio_counter_register(struct iio_counter *const counter);
>> +void iio_counter_unregister(struct iio_counter *const counter);
>> +
>> +#endif /* CONFIG_IIO_COUNTER */
>> +
>> +#endif /* _IIO_COUNTER_H_ */
>

Next message: Takashi Iwai: "Re: kvm splat in mmu_spte_clear_track_bits"
Previous message: Haiyang Zhang: "RE: [PATCHv2] hv_set_ifconfig.sh double check before setting ip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]