Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1721787 > unrolled thread

Re: [PATCH 3/3] iio: 104-quad-8: Add IIO generic counter interface support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] iio: 104-quad-8: Add IIO generic counter interface  support William Breathitt Gray <vilhelm.gray@gmail.com> - 2017-08-28 18:30 +0200
    Re: [PATCH 3/3] iio: 104-quad-8: Add IIO generic counter interface  support Jonathan Cameron <jic23@kernel.org> - 2017-09-03 20:00 +0200
      Re: [PATCH 3/3] iio: 104-quad-8: Add IIO generic counter interface  support Jonathan Cameron <jic23@kernel.org> - 2017-09-03 20:30 +0200

#1721787 — Re: [PATCH 3/3] iio: 104-quad-8: Add IIO generic counter interface support

On Sun, Aug 20, 2017 at 01:11:18PM +0100, Jonathan Cameron wrote:
>On Mon, 31 Jul 2017 12:03:46 -0400
>William Breathitt Gray <vilhelm.gray@gmail.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."
>> 
>> A quad8_remove function is defined to call iio_counter_unregister. This
>> function can be eliminated once a devm_iio_counter_register function is
>> defined.
>> 
>> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
>A good example.
>
>I think it does make it clear that we need to be very careful on how much of
>the interface is defined by freeform strings.  Even if we export other means
>of establishing the associations to userspace, the moment there are strings
>available giving them names, software will start to use those.
>
>May be fine but we need to be very careful.

I would like to limit the amount of strings as well; the availability of
freeform strings has an unfortunate tendency to create situations where
different drivers form separate conventions for duplicate functionality.

The reason freeform strings are available for the generic counter
interface is to provide the flexibility to support more complex classes
of counters. More specific counter class interfaces such as the future
quadrature counter interface will likely expose predefined constants
rather than allow drivers to create their own strings. In general
though, I believe your warning is a good word of caution and I'll keep
an eye on reducing the amount of freeform strings we allow.

In truth, while this is a good example of how a driver would utilize the
generic counter interface with real hardware, it's not a perfect case
for quadrature counters in general. As you noticed, the dynamic aspects
of the generic counter interface are not needed by the 104-QUAD-8. The
future quadrature counter interface would be more fitting for the
104-QUAD-8.

In addition, I may provide a dummy software counter driver in version 2
of this patchset to showcase and exemplify the functionality of the
generic counter interface more directly and aptly.

Sincerely,

William Breathitt Gray

>
>Jonathan
>
>> ---
>>  drivers/iio/counter/104-quad-8.c | 306 ++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 289 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/iio/counter/104-quad-8.c b/drivers/iio/counter/104-quad-8.c
>> index ba3d9030cd51..72b88b7de5b3 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];
>> @@ -528,33 +532,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;
>The meaning of the return values in here is obscure to put
>it lightly.  Use and enum or defines or something to make it clear
>what is going on!  Even comments would help.
>> +}
>> +
>> +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")
>
>This naming needs to take on a very standard format or it will become hard
>for userspace to use (when it is dynamic anyway).
>
>> +};
>> +
>> +#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")
>> +};
>
>Likewise.  These need to be very formally defined. Userspace may need to
>parse these...
> 
>> +
>> +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 *init_signals;
>> +	const size_t num_init_signals = ARRAY_SIZE(quad8_signals);
>> +	struct iio_counter_value *init_values;
>> +	const size_t num_init_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;
>> +	init_signals = devm_kmalloc(dev, sizeof(quad8_signals), GFP_KERNEL);
>> +	if (!init_signals)
>> +		return -ENOMEM;
>> +
>> +	memcpy(init_signals, quad8_signals, sizeof(quad8_signals));
>> +
>> +	init_values = devm_kmalloc(dev, sizeof(quad8_values), GFP_KERNEL);
>> +	if (!init_values)
>> +		return -ENOMEM;
>> +
>> +	memcpy(init_values, quad8_values, sizeof(quad8_values));
>> +
>> +	/* Associate values with their respective signals */
>> +	for (i = 0; i < num_init_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;
>> +		triggers[0].trigger_modes = quad8_trigger_modes;
>> +		triggers[0].num_trigger_modes = ARRAY_SIZE(quad8_trigger_modes);
>> +		triggers[0].signal = &init_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 = &init_signals[2 * i + 1];
>Can these not be defined as constants? I'm not immediately seeing anything
>dynamic in here. It would make it easier to eyeball how everything fits together.
>In this case where everything is constant can we define it as such?
>
>Fiddly perhaps ;)
>> +
>> +		init_values[i].init_triggers = triggers;
>> +		init_values[i].num_init_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.init_signals = init_signals;
>> +	quad8iio->counter.num_init_signals = num_init_signals;
>> +	quad8iio->counter.init_values = init_values;
>> +	quad8iio->counter.num_init_values = num_init_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);
>> @@ -580,11 +840,23 @@ 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);
>> +	dev_set_drvdata(dev, &quad8iio->counter);
>> +
>> +	return iio_counter_register(&quad8iio->counter);
>> +}
>> +
>> +static int quad8_remove(struct device *dev, unsigned int id)
>> +{
>> +	struct iio_counter *counter = dev_get_drvdata(dev);
>> +
>> +	iio_counter_unregister(counter);
>> +
>> +	return 0;
>>  }
>>  
>>  static struct isa_driver quad8_driver = {
>>  	.probe = quad8_probe,
>> +	.remove = quad8_remove,
>>  	.driver = {
>>  		.name = "104-quad-8"
>>  	}
>

[toc] | [next] | [standalone]

#1725719

On Mon, 28 Aug 2017 12:23:45 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Sun, Aug 20, 2017 at 01:11:18PM +0100, Jonathan Cameron wrote:
> >On Mon, 31 Jul 2017 12:03:46 -0400
> >William Breathitt Gray <vilhelm.gray@gmail.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."
> >> 
> >> A quad8_remove function is defined to call iio_counter_unregister. This
> >> function can be eliminated once a devm_iio_counter_register function is
> >> defined.
> >> 
> >> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>  
> >
> >A good example.
> >
> >I think it does make it clear that we need to be very careful on how much of
> >the interface is defined by freeform strings.  Even if we export other means
> >of establishing the associations to userspace, the moment there are strings
> >available giving them names, software will start to use those.
> >
> >May be fine but we need to be very careful.  
> 
> I would like to limit the amount of strings as well; the availability of
> freeform strings has an unfortunate tendency to create situations where
> different drivers form separate conventions for duplicate functionality.

Absolutely.

> 
> The reason freeform strings are available for the generic counter
> interface is to provide the flexibility to support more complex classes
> of counters. More specific counter class interfaces such as the future
> quadrature counter interface will likely expose predefined constants
> rather than allow drivers to create their own strings. In general
> though, I believe your warning is a good word of caution and I'll keep
> an eye on reducing the amount of freeform strings we allow.

OK. That could work fine - we enforce the usage by review rather than
code.
> 
> In truth, while this is a good example of how a driver would utilize the
> generic counter interface with real hardware, it's not a perfect case
> for quadrature counters in general. As you noticed, the dynamic aspects
> of the generic counter interface are not needed by the 104-QUAD-8. The
> future quadrature counter interface would be more fitting for the
> 104-QUAD-8.
> 
> In addition, I may provide a dummy software counter driver in version 2
> of this patchset to showcase and exemplify the functionality of the
> generic counter interface more directly and aptly.

That could be very useful.  An alternative would be to look at a simple
device (if we can find one) and implement a userspace fake for it
(similar to what Guenter has done with lots of hwmon devices).

That way we can play with a real driver against fake hardware and get
the best of all possible worlds.  I've been meaning to look at doing this
for various IIO drivers for a while (most complete for i2c devices I think).

As you might imagine I don't actually have that many parts (and most of them
aren't connected to boards at any given time) so any form of emulation can be
very helpful.

I'll see if I can dig up any interesting devices beyond the ones we
already know are integrated in various SoCs.

I think we really need say 2+ devices to justify decisions in the
core code. I did the original IIO version against 3ish devices but
that was more varied (and that wasn't nearly enough with hindsight!)

Jonathan

[toc] | [prev] | [next] | [standalone]

#1725727

On Sun, 3 Sep 2017 18:50:01 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 28 Aug 2017 12:23:45 -0400
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Sun, Aug 20, 2017 at 01:11:18PM +0100, Jonathan Cameron wrote:  
> > >On Mon, 31 Jul 2017 12:03:46 -0400
> > >William Breathitt Gray <vilhelm.gray@gmail.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."
> > >> 
> > >> A quad8_remove function is defined to call iio_counter_unregister. This
> > >> function can be eliminated once a devm_iio_counter_register function is
> > >> defined.
> > >> 
> > >> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>    
> > >
> > >A good example.
> > >
> > >I think it does make it clear that we need to be very careful on how much of
> > >the interface is defined by freeform strings.  Even if we export other means
> > >of establishing the associations to userspace, the moment there are strings
> > >available giving them names, software will start to use those.
> > >
> > >May be fine but we need to be very careful.    
> > 
> > I would like to limit the amount of strings as well; the availability of
> > freeform strings has an unfortunate tendency to create situations where
> > different drivers form separate conventions for duplicate functionality.  
> 
> Absolutely.
> 
> > 
> > The reason freeform strings are available for the generic counter
> > interface is to provide the flexibility to support more complex classes
> > of counters. More specific counter class interfaces such as the future
> > quadrature counter interface will likely expose predefined constants
> > rather than allow drivers to create their own strings. In general
> > though, I believe your warning is a good word of caution and I'll keep
> > an eye on reducing the amount of freeform strings we allow.  
> 
> OK. That could work fine - we enforce the usage by review rather than
> code.
> > 
> > In truth, while this is a good example of how a driver would utilize the
> > generic counter interface with real hardware, it's not a perfect case
> > for quadrature counters in general. As you noticed, the dynamic aspects
> > of the generic counter interface are not needed by the 104-QUAD-8. The
> > future quadrature counter interface would be more fitting for the
> > 104-QUAD-8.
> > 
> > In addition, I may provide a dummy software counter driver in version 2
> > of this patchset to showcase and exemplify the functionality of the
> > generic counter interface more directly and aptly.  
> 
> That could be very useful.  An alternative would be to look at a simple
> device (if we can find one) and implement a userspace fake for it
> (similar to what Guenter has done with lots of hwmon devices).
> 
> That way we can play with a real driver against fake hardware and get
> the best of all possible worlds.  I've been meaning to look at doing this
> for various IIO drivers for a while (most complete for i2c devices I think).
> 
> As you might imagine I don't actually have that many parts (and most of them
> aren't connected to boards at any given time) so any form of emulation can be
> very helpful.
> 
> I'll see if I can dig up any interesting devices beyond the ones we
> already know are integrated in various SoCs.

A quick search turned up a few parts that might do the job as additional
test parts.

lsi ls7366r - spi encoder to count chip. 
www.lsisci.com  - lots of parts made by them...

The broadcom (now - been various people) hctl-2032 - parallel output
but otherwise, does basic counting index etc.

https://www.tindie.com/products/Renbotics/tinyqed/
looks quite cute... Just does it using at little micro to do the hard work.
(long dead as a product by the look of it though..) Easy to emulate perhaps
but none of the links work..

Another thought would be to do a quick fpga version of whatever we
fancy playing with and use that.  I can think of a few people to
ask if this would be useful (could do it myself, but low on time
for the next few months at least).

Jonathan

> 
> I think we really need say 2+ devices to justify decisions in the
> core code. I did the original IIO version against 3ish devices but
> that was more varied (and that wasn't nearly enough with hindsight!)
> 
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web