Message-ID: <20171008144117.5ba4d6a6@archlinux>
Date:   Sun, 8 Oct 2017 14:41:17 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     William Breathitt Gray <vilhelm.gray@...il.com>
Cc:     benjamin.gaignard@...aro.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] iio: Add dummy counter driver

On Thu,  5 Oct 2017 14:14:38 -0400
William Breathitt Gray <vilhelm.gray@...il.com> 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 :)
> 
> 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@...il.com>

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@...il.com>");
> +MODULE_DESCRIPTION("Dummy counter driver");
> +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists