lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 8 Oct 2017 14:44:55 +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 6/6] iio: 104-quad-8: Add IIO generic counter
 interface support

On Thu,  5 Oct 2017 14:14:53 -0400
William Breathitt Gray <vilhelm.gray@...il.com> wrote:

> This patch adds support for the IIO generic counter interface to the
> 104-QUAD-8 driver. The existing 104-QUAD-8 device interface should not
> be affected by this patch; all changes are intended as supplemental
> additions as perceived by the user.
> 
> IIO Counter Signals are defined for all quadrature input pairs
> (A and B), as well as index input lines. However, IIO Counter Triggers
> are not created for the index input Signals. IIO Counter Values are
> created for the eight quadrature channel counts, and their respective
> Signals are associated via IIO Counter Triggers.
> 
> The new generic counter interface sysfs attributes expose the same
> functionality and data available via the existing 104-QUAD-8 device
> interface. Four IIO Counter Value function modes are available,
> correlating to the four possible quadrature mode configurations:
> "non-quadrature," "quadrature x1," "quadrature x2," and "quadrature x4."
> 
> Signed-off-by: William Breathitt Gray <vilhelm.gray@...il.com>

A few generic comments inline - nothing significant.

Jonathan
> ---
>  drivers/iio/counter/104-quad-8.c | 294 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 277 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/counter/104-quad-8.c b/drivers/iio/counter/104-quad-8.c
> index b56985078d8c..690a520e70df 100644
> --- a/drivers/iio/counter/104-quad-8.c
> +++ b/drivers/iio/counter/104-quad-8.c
> @@ -16,6 +16,7 @@
>  #include <linux/bitops.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
> +#include <linux/iio/counter.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
>  #include <linux/io.h>
> @@ -24,6 +25,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/string.h>
>  #include <linux/types.h>
>  
>  #define QUAD8_EXTENT 32
> @@ -37,6 +39,7 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
>  
>  /**
>   * struct quad8_iio - IIO device private data structure
> + * @counter:		instance of the iio_counter
>   * @preset:		array of preset values
>   * @count_mode:		array of count mode configurations
>   * @quadrature_mode:	array of quadrature mode configurations
> @@ -48,6 +51,7 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
>   * @base:		base port address of the IIO device
>   */
>  struct quad8_iio {
> +	struct iio_counter counter;
>  	unsigned int preset[QUAD8_NUM_COUNTERS];
>  	unsigned int count_mode[QUAD8_NUM_COUNTERS];
>  	unsigned int quadrature_mode[QUAD8_NUM_COUNTERS];
> @@ -527,33 +531,289 @@ static const struct iio_chan_spec quad8_channels[] = {
>  	QUAD8_COUNT_CHAN(7), QUAD8_INDEX_CHAN(7)
>  };
>  
> +static int quad8_signal_read(struct iio_counter *counter,
> +	struct iio_counter_signal *signal, int *val, int *val2)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +
> +	if (signal->id < 16)
> +		return -EINVAL;
> +
> +	*val = !!(inb(priv->base + 0x16) & BIT(signal->id - 16));
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int quad8_trigger_mode_get(struct iio_counter *counter,
> +	struct iio_counter_value *value, struct iio_counter_trigger *trigger)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +	const unsigned int mode = priv->quadrature_mode[value->id];
> +	const unsigned int scale = priv->quadrature_scale[value->id];
> +	unsigned int direction;
> +	const unsigned int flag_addr = priv->base + 2 * value->id + 1;
> +	const int signal_id = trigger->signal->id % 2;
> +
> +	if (mode)
> +		switch (scale) {
> +		case 0:
> +			/* U/D flag: 1 = up, 0 = down */
> +			/* direction: 0 = up, 1 = down */
> +			direction = !(inb(flag_addr) & BIT(5));
> +			if (!signal_id)
> +				return direction + 1;
> +			break;
> +		case 1:
> +			if (!signal_id)
> +				return 3;
> +			break;
> +		case 2:
> +			return 3;
> +		}
> +	else
> +		if (!signal_id)
> +			return 1;
> +
> +	return 0;
> +}
> +
> +static int quad8_value_read(struct iio_counter *counter,
> +	struct iio_counter_value *value, int *val, int *val2)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +	const int base_offset = priv->base + 2 * value->id;
> +	unsigned int flags;
> +	unsigned int borrow;
> +	unsigned int carry;
> +	int i;
> +
> +	flags = inb(base_offset + 1);
> +	borrow = flags & BIT(0);
> +	carry = !!(flags & BIT(1));
> +
> +	/* Borrow XOR Carry effectively doubles count range */
> +	*val = (borrow ^ carry) << 24;
> +
> +	/* Reset Byte Pointer; transfer Counter to Output Latch */
> +	outb(0x11, base_offset + 1);
> +
> +	for (i = 0; i < 3; i++)
> +		*val |= (unsigned int)inb(base_offset) << (8 * i);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int quad8_value_write(struct iio_counter *counter,
> +	struct iio_counter_value *value, int val, int val2)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +	const int base_offset = priv->base + 2 * value->id;
> +	int i;
> +
> +	/* Only 24-bit values are supported */
> +	if ((unsigned int)val > 0xFFFFFF)
> +		return -EINVAL;
> +
> +	/* Reset Byte Pointer */
> +	outb(0x01, base_offset + 1);
> +
> +	/* Counter can only be set via Preset Register */
> +	for (i = 0; i < 3; i++)
> +		outb(val >> (8 * i), base_offset);
> +
> +	/* Transfer Preset Register to Counter */
> +	outb(0x08, base_offset + 1);
> +
> +	/* Reset Byte Pointer */
> +	outb(0x01, base_offset + 1);
> +
> +	/* Set Preset Register back to original value */
> +	val = priv->preset[value->id];
> +	for (i = 0; i < 3; i++)
> +		outb(val >> (8 * i), base_offset);
> +
> +	/* Reset Borrow, Carry, Compare, and Sign flags */
> +	outb(0x02, base_offset + 1);
> +	/* Reset Error flag */
> +	outb(0x06, base_offset + 1);
> +
> +	return 0;
> +}
> +
> +static int quad8_value_function_set(struct iio_counter *counter,
> +	struct iio_counter_value *value, unsigned int mode)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +	const unsigned int mode_cfg = mode << 3 |
> +		priv->count_mode[value->id] << 1;
> +	const unsigned int idr_cfg = priv->index_polarity[value->id] << 1;
> +	const int base_offset = priv->base + 2 * value->id + 1;
> +
> +	if (mode)
> +		priv->quadrature_scale[value->id] = mode - 1;
> +	else {
> +		/* Quadrature scaling only available in quadrature mode */
> +		priv->quadrature_scale[value->id] = 0;
> +
> +		/* Synchronous function not supported in non-quadrature mode */
> +		if (priv->synchronous_mode[value->id]) {
> +			priv->synchronous_mode[value->id] = 0;
> +			outb(0x60 | idr_cfg, base_offset);
> +		}
> +	}
> +
> +	priv->quadrature_mode[value->id] = !!mode;
> +
> +	/* Load mode configuration to Counter Mode Register */
> +	outb(0x20 | mode_cfg, base_offset);
> +
> +	return 0;
> +}
> +
> +static int quad8_value_function_get(struct iio_counter *counter,
> +	struct iio_counter_value *value)
> +{
> +	struct quad8_iio *const priv = counter->driver_data;
> +	unsigned int quadrature_mode = priv->quadrature_mode[value->id];
> +
> +	return (quadrature_mode) ? priv->quadrature_scale[value->id] + 1 : 0;
> +}
> +
> +static const struct iio_counter_ops quad8_ops = {
> +	.signal_read = quad8_signal_read,
> +	.trigger_mode_get = quad8_trigger_mode_get,
> +	.value_read = quad8_value_read,
> +	.value_write = quad8_value_write,
> +	.value_function_set = quad8_value_function_set,
> +	.value_function_get = quad8_value_function_get
> +};
> +
> +static const char *const quad8_function_modes[] = {
> +	"non-quadrature",
> +	"quadrature x1",
> +	"quadrature x2",
> +	"quadrature x4"
> +};
> +
> +#define QUAD8_SIGNAL(_id, _name) {	\
> +	.id = _id,			\
> +	.name = _name			\
> +}
> +
> +static const struct iio_counter_signal quad8_signals[] = {
> +	QUAD8_SIGNAL(0, "Channel 1 Quadrature A"),
> +	QUAD8_SIGNAL(1, "Channel 1 Quadrature B"),
> +	QUAD8_SIGNAL(2, "Channel 2 Quadrature A"),
> +	QUAD8_SIGNAL(3, "Channel 2 Quadrature B"),
> +	QUAD8_SIGNAL(4, "Channel 3 Quadrature A"),
> +	QUAD8_SIGNAL(5, "Channel 3 Quadrature B"),
> +	QUAD8_SIGNAL(6, "Channel 4 Quadrature A"),
> +	QUAD8_SIGNAL(7, "Channel 4 Quadrature B"),
> +	QUAD8_SIGNAL(8, "Channel 5 Quadrature A"),
> +	QUAD8_SIGNAL(9, "Channel 5 Quadrature B"),
> +	QUAD8_SIGNAL(10, "Channel 6 Quadrature A"),
> +	QUAD8_SIGNAL(11, "Channel 6 Quadrature B"),
> +	QUAD8_SIGNAL(12, "Channel 7 Quadrature A"),
> +	QUAD8_SIGNAL(13, "Channel 7 Quadrature B"),
> +	QUAD8_SIGNAL(14, "Channel 8 Quadrature A"),
> +	QUAD8_SIGNAL(15, "Channel 8 Quadrature B"),
> +	QUAD8_SIGNAL(16, "Channel 1 Index"),
> +	QUAD8_SIGNAL(17, "Channel 2 Index"),
> +	QUAD8_SIGNAL(18, "Channel 3 Index"),
> +	QUAD8_SIGNAL(19, "Channel 4 Index"),
> +	QUAD8_SIGNAL(20, "Channel 5 Index"),
> +	QUAD8_SIGNAL(21, "Channel 6 Index"),
> +	QUAD8_SIGNAL(22, "Channel 7 Index"),
> +	QUAD8_SIGNAL(23, "Channel 8 Index")
> +};
> +
> +#define QUAD8_VALUE(_id, _name) {					\
> +	.id = _id,							\
> +	.name = _name,							\
> +	.mode = 0,							\
> +	.function_modes = quad8_function_modes,				\
> +	.num_function_modes = ARRAY_SIZE(quad8_function_modes)		\
> +}
> +
> +static const struct iio_counter_value quad8_values[] = {
> +	QUAD8_VALUE(0, "Channel 1 Count"), QUAD8_VALUE(1, "Channel 2 Count"),
> +	QUAD8_VALUE(2, "Channel 3 Count"), QUAD8_VALUE(3, "Channel 4 Count"),
> +	QUAD8_VALUE(4, "Channel 5 Count"), QUAD8_VALUE(5, "Channel 6 Count"),
> +	QUAD8_VALUE(6, "Channel 7 Count"), QUAD8_VALUE(7, "Channel 8 Count")
> +};
> +
> +static const char *const quad8_trigger_modes[] = {
> +	"none",
> +	"rising edge",
> +	"falling edge",
> +	"both edges"
> +};
> +
>  static int quad8_probe(struct device *dev, unsigned int id)
>  {
> -	struct iio_dev *indio_dev;
> -	struct quad8_iio *priv;
> +	struct iio_counter_signal *signals;
> +	const size_t num_signals = ARRAY_SIZE(quad8_signals);
> +	struct iio_counter_value *values;
> +	const size_t num_values = ARRAY_SIZE(quad8_values);
> +	struct iio_counter_trigger *triggers;
> +	struct quad8_iio *quad8iio;
>  	int i, j;
>  	unsigned int base_offset;
>  
> -	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -
> -	if (!devm_request_region(dev, base[id], QUAD8_EXTENT,
> -		dev_name(dev))) {
> +	if (!devm_request_region(dev, base[id], QUAD8_EXTENT, dev_name(dev))) {
>  		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
>  			base[id], base[id] + QUAD8_EXTENT);
>  		return -EBUSY;
>  	}
>  
> -	indio_dev->info = &quad8_info;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->num_channels = ARRAY_SIZE(quad8_channels);
> -	indio_dev->channels = quad8_channels;
> -	indio_dev->name = dev_name(dev);
> -	indio_dev->dev.parent = dev;
> +	signals = devm_kmalloc(dev, sizeof(quad8_signals), GFP_KERNEL);
> +	if (!signals)
> +		return -ENOMEM;
> +
> +	memcpy(signals, quad8_signals, sizeof(quad8_signals));
> +
> +	values = devm_kmalloc(dev, sizeof(quad8_values), GFP_KERNEL);
> +	if (!values)
> +		return -ENOMEM;
> +
> +	memcpy(values, quad8_values, sizeof(quad8_values));

devm_kmemdup

> +
> +	/* Associate values with their respective signals */
> +	for (i = 0; i < num_values; i++) {
> +		triggers = devm_kmalloc(dev, 2 * sizeof(*triggers), GFP_KERNEL);
> +		if (!triggers)
> +			return -ENOMEM;
> +
> +		/* Starts up in non-quadrature mode */
> +		triggers[0].mode = 1;

As I suggested for the dummy driver - use an enum, then the enum naming will
make it obvious what the value means here without a comment.

> +		triggers[0].trigger_modes = quad8_trigger_modes;
> +		triggers[0].num_trigger_modes = ARRAY_SIZE(quad8_trigger_modes);
> +		triggers[0].signal = &signals[2 * i];
> +		triggers[1].mode = 0;
> +		triggers[1].trigger_modes = quad8_trigger_modes;
> +		triggers[1].num_trigger_modes = ARRAY_SIZE(quad8_trigger_modes);
> +		triggers[1].signal = &signals[2 * i + 1];
> +
> +		values[i].triggers = triggers;
> +		values[i].num_triggers = 2;
> +	}
> +
> +	quad8iio = devm_kzalloc(dev, sizeof(*quad8iio), GFP_KERNEL);
> +	if (!quad8iio)
> +		return -ENOMEM;
>  
> -	priv = iio_priv(indio_dev);
> -	priv->base = base[id];
> +	quad8iio->counter.name = dev_name(dev);
> +	quad8iio->counter.dev = dev;
> +	quad8iio->counter.ops = &quad8_ops;
> +	quad8iio->counter.signals = signals;
> +	quad8iio->counter.num_signals = num_signals;
> +	quad8iio->counter.values = values;
> +	quad8iio->counter.num_values = num_values;
> +	quad8iio->counter.channels = quad8_channels;
> +	quad8iio->counter.num_channels = ARRAY_SIZE(quad8_channels);
> +	quad8iio->counter.info = &quad8_info;
> +	quad8iio->counter.driver_data = quad8iio;
> +	quad8iio->base = base[id];
>  
>  	/* Reset all counters and disable interrupt function */
>  	outb(0x01, base[id] + 0x11);
> @@ -579,7 +839,7 @@ static int quad8_probe(struct device *dev, unsigned int id)
>  	/* Enable all counters */
>  	outb(0x00, base[id] + 0x11);
>  
> -	return devm_iio_device_register(dev, indio_dev);
> +	return devm_iio_counter_register(dev, &quad8iio->counter);
>  }
>  
>  static struct isa_driver quad8_driver = {

Powered by blists - more mailing lists