2017-10-08 15:41 GMT+02:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
> On Thu, 5 Oct 2017 14:14:38 -0400
> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
>
>> This patch introduces the dummy counter driver. The dummy counter driver
>> serves as a reference implementation of a driver that utilizes the
>> Generic Counter interface.
>
> This is great - I was planning to write one of these to try out the
> interface and you've already done it :)
May I suggest you to write a gpio-counter driver instead ? (that was
in my todo list)
It is more or less the same than this driver but instead of simulate the signal
it could use gpio to them provide a software quadratic counter.
Benjamin
>>
>> Writing individual '1' and '0' characters to the Signal attributes
>> allows a user to simulate a typical Counter Signal input stream for
>> evaluation; the Counter will evaluate the Signal data based on the
>> respective trigger mode for the associated Signal, and trigger the
>> associated counter function specified by the respective function mode.
>> The current Value value may be read, and the Value value preset by a
>> write.
>>
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
>
> Comments are more generic suggestions for improving the example than
> general comments on the ABI - that all seems to make reasonable sense
> (other than when the documents contain the wonderful
> Value value - not confusing at all ;)
>
>> ---
>> drivers/iio/counter/Kconfig | 15 ++
>> drivers/iio/counter/Makefile | 1 +
>> drivers/iio/counter/dummy-counter.c | 293 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 309 insertions(+)
>> create mode 100644 drivers/iio/counter/dummy-counter.c
>>
>> diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
>> index c8becfe78e28..494aed40e9c9 100644
>> --- a/drivers/iio/counter/Kconfig
>> +++ b/drivers/iio/counter/Kconfig
>> @@ -22,6 +22,21 @@ config 104_QUAD_8
>> The base port addresses for the devices may be configured via the base
>> array module parameter.
>>
>> +config DUMMY_COUNTER
>> + tristate "Dummy counter driver"
>> + help
>> + Select this option to enable the dummy counter driver. The dummy
>> + counter driver serves as a reference implementation of a driver that
>> + utilizes the Generic Counter interface.
>> +
>> + Writing individual '1' and '0' characters to the Signal attributes
>> + allows a user to simulate a typical Counter Signal input stream for
>> + evaluation; the Counter will evaluate the Signal data based on the
>> + respective trigger mode for the associated Signal, and trigger the
>> + associated counter function specified by the respective function mode.
>> + The current Value value may be read, and the Value value preset by a
>> + write.
>> +
>> config STM32_LPTIMER_CNT
>> tristate "STM32 LP Timer encoder counter driver"
>> depends on MFD_STM32_LPTIMER || COMPILE_TEST
>> diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
>> index 1b9a896eb488..8c2ef0115426 100644
>> --- a/drivers/iio/counter/Makefile
>> +++ b/drivers/iio/counter/Makefile
>> @@ -5,4 +5,5 @@
>> # When adding new entries keep the list in alphabetical order
>>
>> obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
>> +obj-$(CONFIG_DUMMY_COUNTER) += dummy-counter.o
>> obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
>> diff --git a/drivers/iio/counter/dummy-counter.c b/drivers/iio/counter/dummy-counter.c
>> new file mode 100644
>> index 000000000000..6ecc9854894f
>> --- /dev/null
>> +++ b/drivers/iio/counter/dummy-counter.c
>> @@ -0,0 +1,293 @@
>> +/*
>> + * Dummy counter driver
>> + * 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/errno.h>
>> +#include <linux/iio/counter.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
>> +
>> +#define DUMCNT_NUM_COUNTERS 2
>> +/**
>> + * struct dumcnt - private data structure
>> + * @counter: instance of the iio_counter
>> + * @counts: array of accumulation values
>> + * @states: array of input line states
>> + */
>> +struct dumcnt {
>> + struct iio_counter counter;
>> + unsigned int counts[DUMCNT_NUM_COUNTERS];
>> + unsigned int states[DUMCNT_NUM_COUNTERS];
>> +};
>> +
>> +static int dumcnt_signal_read(struct iio_counter *counter,
>> + struct iio_counter_signal *signal, int *val, int *val2)
>> +{
>> + struct dumcnt *const priv = counter->driver_data;
>> + *val = priv->states[signal->id];
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static int dumcnt_signal_write(struct iio_counter *counter,
>> + struct iio_counter_signal *signal, int val, int val2)
>> +{
>
> This is an odd one to have in the generic interface.
> What real hardware does writing the state make sense for?
> If it is only fake drivers - figure out a way to do it without
> having to add to the generic interfaces.
>
>> + struct dumcnt *const priv = counter->driver_data;
>> + const unsigned int id = signal->id;
>> + const unsigned int prev_state = priv->states[id];
>> + struct iio_counter_value *const value = counter->values + id;
>> + const unsigned int function_mode = value->mode;
>> + const unsigned int trigger_mode = value->triggers[0].mode;
>> + unsigned int triggered = 0;
>> +
>> + if (val && val != 1)
>> + return -EINVAL;
>> +
>> + /* If no state change then just exit */
>> + if (prev_state == val)
>> + return 0;
>> +
>> + priv->states[id] = val;
>> +
>> + switch (trigger_mode) {
>> + /* "none" case */
>> + case 0:
>> + return 0;
>> + /* "rising edge" case */
>> + case 1:
>> + if (!prev_state)
>> + triggered = 1;
>> + break;
>> + /* "falling edge" case */
>> + case 2:
>> + if (prev_state)
>> + triggered = 1;
>> + break;
>> + /* "both edges" case */
>> + case 3:
>> + triggered = 1;
>> + break;
>> + }
>> +
>> + /* If counter function triggered */
>> + if (triggered)
>> + /* "increase" case */
>> + if (function_mode)
>> + priv->counts[id]++;
>> + /* "decrease" case */
>> + else
>> + priv->counts[id]--;
>> +
>> + return 0;
>> +}
>> +
>> +static int dumcnt_trigger_mode_set(struct iio_counter *counter,
>> + struct iio_counter_value *value, struct iio_counter_trigger *trigger,
>> + unsigned int mode)
>> +{
>> + if (mode >= trigger->num_trigger_modes)
>> + return -EINVAL;
>> +
>> + trigger->mode = mode;
>> +
>> + return 0;
>> +}
>> +
>> +static int dumcnt_trigger_mode_get(struct iio_counter *counter,
>> + struct iio_counter_value *value, struct iio_counter_trigger *trigger)
>> +{
>> + return trigger->mode;
>> +}
>> +
>> +static int dumcnt_value_read(struct iio_counter *counter,
>> + struct iio_counter_value *value, int *val, int *val2)
>> +{
>> + struct dumcnt *const priv = counter->driver_data;
>> +
>> + *val = priv->counts[value->id];
>> +
>> + return IIO_VAL_INT;
>> +}
>> +
>> +static int dumcnt_value_write(struct iio_counter *counter,
>> + struct iio_counter_value *value, int val, int val2)
>> +{
>> + struct dumcnt *const priv = counter->driver_data;
>> +
>> + priv->counts[value->id] = val;
>> +
>> + return 0;
>> +}
>> +
>> +static int dumcnt_value_function_set(struct iio_counter *counter,
>> + struct iio_counter_value *value, unsigned int mode)
>> +{
>> + if (mode >= value->num_function_modes)
>> + return -EINVAL;
>> +
>> + value->mode = mode;
>> +
>> + return 0;
>> +}
>> +
>> +static int dumcnt_value_function_get(struct iio_counter *counter,
>> + struct iio_counter_value *value)
>> +{
>> + return value->mode;
>
> If it's called function in the function name, call it function in
> the structure as well rather than mode.
>
>> +}
>> +
>> +static const struct iio_counter_ops dumcnt_ops = {
>> + .signal_read = dumcnt_signal_read,
>> + .signal_write = dumcnt_signal_write,
>> + .trigger_mode_get = dumcnt_trigger_mode_get,
>> + .trigger_mode_set = dumcnt_trigger_mode_set,
>> + .value_read = dumcnt_value_read,
>> + .value_write = dumcnt_value_write,
>> + .value_function_set = dumcnt_value_function_set,
>> + .value_function_get = dumcnt_value_function_get
>> +};
>> +
>> +static const char *const dumcnt_function_modes[] = {
>> + "decrease",
>
> I think increment was used somewhere in the docs... It's
> clearer, but you need to document this ABI to stop having
> subtle variations of it like this (even if I imagined it ;)
>
>> + "increase"
>> +};
>> +
>> +#define DUMCNT_SIGNAL(_id, _name) { \
>> + .id = _id, \
>> + .name = _name \
>> +}
>> +
>> +static const struct iio_counter_signal dumcnt_signals[] = {
>> + DUMCNT_SIGNAL(0, "Signal A"), DUMCNT_SIGNAL(1, "Signal B")
>> +};
>> +
>> +#define DUMCNT_VALUE(_id, _name) { \
>> + .id = _id, \
>> + .name = _name, \
>> + .mode = 0, \
>> + .function_modes = dumcnt_function_modes, \
>> + .num_function_modes = ARRAY_SIZE(dumcnt_function_modes) \
>> +}
>> +
>> +static const struct iio_counter_value dumcnt_values[] = {
>> + DUMCNT_VALUE(0, "Count A"), DUMCNT_VALUE(1, "Count B")
>> +};
>> +
>> +static const char *const dumcnt_trigger_modes[] = {
>
> As mentioned below, use an enum for the index as then you can make it obvious
> what 0 means when you set the mode to it later.
>
>> + "none",
>> + "rising edge",
>> + "falling edge",
>> + "both edges"
>> +};
>> +
>> +static int dumcnt_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct iio_counter_signal *signals;
>> + const size_t num_signals = ARRAY_SIZE(dumcnt_signals);
>
> Don't bother with local variable - makes it less obvious what
> is going on.
>
>> + struct iio_counter_value *values;
>> + const size_t num_values = ARRAY_SIZE(dumcnt_values);
>
> Local variable doesn't add anything and if anything makes
> it slightly harder to check what is going on.
>
>> + struct iio_counter_trigger *triggers;
>> + int i;
>> + struct dumcnt *dumcnt;
>> +
>> + signals = devm_kmalloc(dev, sizeof(dumcnt_signals), GFP_KERNEL);
>> + if (!signals)
>> + return -ENOMEM;
>> +
>> + memcpy(signals, dumcnt_signals, sizeof(dumcnt_signals));
>
> devm_kmemdup?
>> +
>> + values = devm_kmalloc(dev, sizeof(dumcnt_values), GFP_KERNEL);
>> + if (!values)
>> + return -ENOMEM;
>> +
>> + memcpy(values, dumcnt_values, sizeof(dumcnt_values));
>
> devm_kmemdup?
>
>> +
>> + /* Associate values with their respective signals */
>> + for (i = 0; i < num_values; i++) {
>> + triggers = devm_kmalloc(dev, sizeof(*triggers), GFP_KERNEL);
>> + if (!triggers)
>> + return -ENOMEM;
>> +
>> + triggers->mode = 0;
>
> Use an enum for the dumcn_trigger_modes array index then specify by enum value
> here. Will make it more readable.
>
>> + triggers->trigger_modes = dumcnt_trigger_modes;
>> + triggers->num_trigger_modes = ARRAY_SIZE(dumcnt_trigger_modes);
>> + triggers->signal = &signals[i];
>> +
>> + values[i].triggers = triggers;
>> + values[i].num_triggers = 1;
>> + }
>> +
>> + dumcnt = devm_kzalloc(dev, sizeof(*dumcnt), GFP_KERNEL);
>> + if (!dumcnt)
>> + return -ENOMEM;
>> +
>> + dumcnt->counter.name = dev_name(dev);
>> + dumcnt->counter.dev = dev;
>> + dumcnt->counter.ops = &dumcnt_ops;
>> + dumcnt->counter.signals = signals;
>> + dumcnt->counter.num_signals = num_signals;
>> + dumcnt->counter.values = values;
>> + dumcnt->counter.num_values = num_values;
>> + dumcnt->counter.driver_data = dumcnt;
>> +
>> + return devm_iio_counter_register(dev, &dumcnt->counter);
>> +}
>> +
>> +static struct platform_device *dumcnt_device;
>
> Support multiple instances - nick this stuff from the
> IIO dummy driver or more specifically the
> industrialio-sw-device.c
>
>> +
>> +static struct platform_driver dumcnt_driver = {
>> + .driver = {
>> + .name = "104-quad-8"
>
> Don't do that! Give it it's own name.
>
>> + }
>> +};
>> +
>> +static void __exit dumcnt_exit(void)
>> +{
>> + platform_device_unregister(dumcnt_device);
>> + platform_driver_unregister(&dumcnt_driver);
>> +}
>> +
>> +static int __init dumcnt_init(void)
>> +{
>> + int err;
>> +
>
> General thing, but if we are going to upstream this with the subsystem,
> make device instantiation happen via configfs.
>
>> + dumcnt_device = platform_device_alloc(dumcnt_driver.driver.name, -1);
>> + if (!dumcnt_device)
>> + return -ENOMEM;
>> +
>> + err = platform_device_add(dumcnt_device);
>> + if (err)
>> + goto err_platform_device;
>> +
>> + err = platform_driver_probe(&dumcnt_driver, dumcnt_probe);
>> + if (err)
>> + goto err_platform_driver;
>> +
>> + return 0;
>> +
>> +err_platform_driver:
>> + platform_device_del(dumcnt_device);
>> +err_platform_device:
>> + platform_device_put(dumcnt_device);
>> + return err;
>> +}
>> +
>> +module_init(dumcnt_init);
>> +module_exit(dumcnt_exit);
>> +
>> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("Dummy counter driver");
>> +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
