Message-ID: <YCYaaYb1T3lt+zAN@shinobu>
Date:   Fri, 12 Feb 2021 15:04:25 +0900
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     kernel@...gutronix.de, linux-stm32@...md-mailman.stormreply.com,
        a.fatoum@...gutronix.de, kamel.bouhara@...tlin.com,
        gwendal@...omium.org, alexandre.belloni@...tlin.com,
        david@...hnology.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        syednwaris@...il.com, patrick.havelange@...ensium.com,
        fabrice.gasnier@...com, mcoquelin.stm32@...il.com,
        alexandre.torgue@...com
Subject: Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the
 ACCES 104-QUAD-8

On Wed, Dec 30, 2020 at 03:31:15PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:38 -0500
> William Breathitt Gray <vilhelm.gray@...il.com> wrote:
> 
> > The LSI/CSI LS7266R1 chip provides programmable output via the FLG pins.
> > When interrupts are enabled on the ACCES 104-QUAD-8, they occur whenever
> > FLG1 is active. Four functions are available for the FLG1 signal: Carry,
> > Compare, Carry-Borrow, and Index.
> > 
> > 	Carry:
> > 		Interrupt generated on active low Carry signal. Carry
> > 		signal toggles every time the respective channel's
> > 		counter overflows.
> > 
> > 	Compare:
> > 		Interrupt generated on active low Compare signal.
> > 		Compare signal toggles every time respective channel's
> > 		preset register is equal to the respective channel's
> > 		counter.
> > 
> > 	Carry-Borrow:
> > 		Interrupt generated on active low Carry signal and
> > 		active low Borrow signal. Carry signal toggles every
> > 		time the respective channel's counter overflows. Borrow
> > 		signal toggles every time the respective channel's
> > 		counter underflows.
> > 
> > 	Index:
> > 		Interrupt generated on active high Index signal.
> > 
> > The irq_trigger Count extension is introduced to allow the selection of
> > the desired IRQ trigger function per channel. Interrupts push Counter
> > events to event channel X, where 'X' is the respective channel whose
> > FLG1 activated.
> > 
> > This patch adds IRQ support for the ACCES 104-QUAD-8. The interrupt line
> > numbers for the devices may be configured via the irq array module
> > parameter.
> > 
> > Reviewed-by: Syed Nayyar Waris <syednwaris@...il.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@...il.com>
> 
> Immediate thought on this is that you should pull the lock type change
> out as a precursor patch that simply says we need it to be a spin_lock
> for the following patch.  That should be an obvious change to review
> and then leave a much shorter patch to focus on here.
> 
> Jonathan

Ack.

> > ---
> >  .../ABI/testing/sysfs-bus-counter-104-quad-8  |  25 ++
> >  drivers/counter/104-quad-8.c                  | 318 ++++++++++++++----
> >  drivers/counter/Kconfig                       |   6 +-
> >  3 files changed, 276 insertions(+), 73 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > index eac32180c40d..0ecba24d43aa 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8
> > @@ -1,3 +1,28 @@
> > +What:		/sys/bus/counter/devices/counterX/countY/irq_trigger
> 
> A warning on this.   The ABI docs are now built into the main kernel
> html docs build.  The snag is that it doesn't cope will with specialising
> or different devices having the same named file with different allowed values.
> You have to unify them all into one place.   It may be worth just doing that
> from the start.

Ack.

> > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> > index f4fb36b751c4..7537575568d0 100644
> > --- a/drivers/counter/104-quad-8.c
> > +++ b/drivers/counter/104-quad-8.c
> > @@ -13,23 +13,30 @@
> >  #include <linux/iio/types.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/isa.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/types.h>
> > +#include <linux/spinlock.h>
> >  
> >  #define QUAD8_EXTENT 32
> >  
> >  static unsigned int base[max_num_isa_dev(QUAD8_EXTENT)];
> >  static unsigned int num_quad8;
> > -module_param_array(base, uint, &num_quad8, 0);
> > +module_param_hw_array(base, uint, ioport, &num_quad8, 0);
> 
> Why this change?  Perhaps a note in the patch description on why
> this is needed?

This change is unrelated to this patch so I'll move it to a precursor
patch with an explanation of the reason for the change.

> >  MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
> >  
> > +static unsigned int irq[max_num_isa_dev(QUAD8_EXTENT)];
> > +module_param_hw_array(irq, uint, irq, NULL, 0);
> > +MODULE_PARM_DESC(irq, "ACCES 104-QUAD-8 interrupt line numbers");
> > +
> >  #define QUAD8_NUM_COUNTERS 8
> >  
> >  /**
> >   * struct quad8_iio - IIO device private data structure
> > + * @lock:		synchronization lock to prevent I/O race conditions
> 
> Probably want to be more specific. RMW, indexed writes or what sort of race?

Ack.

> >   * @counter:		instance of the counter_device
> >   * @fck_prescaler:	array of filter clock prescaler configurations
> >   * @preset:		array of preset values
> > @@ -38,13 +45,14 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses");
> >   * @quadrature_scale:	array of quadrature mode scale configurations
> >   * @ab_enable:		array of A and B inputs enable configurations
> >   * @preset_enable:	array of set_to_preset_on_index attribute configurations
> > + * @irq_trigger:	array of interrupt trigger function configurations
> >   * @synchronous_mode:	array of index function synchronous mode configurations
> >   * @index_polarity:	array of index function polarity configurations
> >   * @cable_fault_enable:	differential encoder cable status enable configurations
> >   * @base:		base port address of the IIO device
> >   */
> >  struct quad8_iio {
> > -	struct mutex lock;
> > +	raw_spinlock_t lock;
> 
> So it was here before but not documented.  You should fix that with a precusor patch.
> Why raw_spinlock_t rather than spinlock_t?

Ack.

> > +static int quad8_watch_validate(struct counter_device *counter,
> > +				const struct counter_watch *watch)
> > +{
> > +	struct quad8_iio *const priv = counter->priv;
> > +
> > +	if (watch->channel > QUAD8_NUM_COUNTERS - 1)
> > +		return -EINVAL;
> > +
> > +	switch (watch->event) {
> > +	case COUNTER_EVENT_OVERFLOW:
> > +		if (priv->irq_trigger[watch->channel] != 0)
> > +			return -EINVAL;
> > +		break;
> > +	case COUNTER_EVENT_THRESHOLD:
> > +		if (priv->irq_trigger[watch->channel] != 1)
> > +			return -EINVAL;
> > +		break;
> > +	case COUNTER_EVENT_OVERFLOW_UNDERFLOW:
> > +		if (priv->irq_trigger[watch->channel] != 2)
> > +			return -EINVAL;
> > +		break;
> > +	case COUNTER_EVENT_INDEX:
> > +		if (priv->irq_trigger[watch->channel] != 3)
> > +			return -EINVAL;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> 
> I'm a great fan of early returns if nothing to be done at
> the end of a switch.  Much cleaner than having to follow the
> breaks and see where they end up (nothing in this case!)

Ack.

> > +static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
> > +{
> > +	struct quad8_iio *const priv = quad8iio;
> > +	const unsigned long base = priv->base;
> > +	unsigned long irq_status;
> > +	unsigned long channel;
> > +	u8 event;
> > +
> 
> Take the spin lock? 

A lock isn't necessary for this particular function. This is the only place
where the device's pending interrupt state is read and reset, so there is no
risk of stale data nor of clobbering the device state because interrupts are
currently disabled.

> > +	irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
> > +	if (!irq_status)
> > +		return IRQ_NONE;
> > +
> > +	for_each_set_bit(channel, &irq_status, QUAD8_NUM_COUNTERS) {
> > +		switch (priv->irq_trigger[channel]) {
> > +		case 0:
> > +			event = COUNTER_EVENT_OVERFLOW;
> > +				break;
> > +		case 1:
> > +			event = COUNTER_EVENT_THRESHOLD;
> > +				break;
> > +		case 2:
> > +			event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
> > +				break;
> > +		case 3:
> > +			event = COUNTER_EVENT_INDEX;
> > +				break;
> > +		default:
> > +			/* should never reach this path */
> > +			WARN_ONCE(true, "invalid interrupt trigger function %u configured for channel %lu\n",
> > +				  priv->irq_trigger[channel], channel);
> > +			continue;
> > +		}
> > +
> > +		counter_push_event(&priv->counter, event, channel);
> > +	}
> > +
> > +	/* Clear pending interrupts on device */
> > +	outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int quad8_probe(struct device *dev, unsigned int id)
> >  {
> >  	struct iio_dev *indio_dev;
> > @@ -1510,9 +1682,10 @@ static int quad8_probe(struct device *dev, unsigned int id)
> >  	quad8iio->counter.priv = quad8iio;
> >  	quad8iio->base = base[id];
> >  
> > -	/* Initialize mutex */
> > -	mutex_init(&quad8iio->lock);
> > +	raw_spin_lock_init(&quad8iio->lock);
> >  
> > +	/* Reset Index/Interrupt Register */
> > +	outb(0x00, base[id] + QUAD8_REG_INDEX_INTERRUPT);
> >  	/* Reset all counters and disable interrupt function */
> >  	outb(QUAD8_CHAN_OP_RESET_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> >  	/* Set initial configuration for all counters */
> > @@ -1542,8 +1715,8 @@ static int quad8_probe(struct device *dev, unsigned int id)
> >  	}
> >  	/* Disable Differential Encoder Cable Status for all channels */
> >  	outb(0xFF, base[id] + QUAD8_DIFF_ENCODER_CABLE_STATUS);
> > -	/* Enable all counters */
> > -	outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
> > +	/* Enable all counters and enable interrupt function */
> > +	outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP);
> >  
> >  	/* Register IIO device */
> >  	err = devm_iio_device_register(dev, indio_dev);
> > @@ -1551,7 +1724,12 @@ static int quad8_probe(struct device *dev, unsigned int id)
> >  		return err;
> >  
> >  	/* Register Counter device */
> > -	return devm_counter_register(dev, &quad8iio->counter);
> > +	err = devm_counter_register(dev, &quad8iio->counter);
> > +	if (err)
> > +		return err;
> > +
> > +	return devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
> > +				quad8iio->counter.name, quad8iio);
> You probably want to do that before the counter_register above
> as I assume that's where the userspace interface gets exposed.
> Otherwise there is probably a theoretical race.
> If not then I'd expect a comment here to explain why it needs to be
> in this unexpected order.

Ack.

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists