Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On Fri, Apr 24, 2020 at 06:42:30PM +0800, Dilip Kota wrote:

> Synchronize tx, rx and error interrupts by registering to the
> same interrupt handler. Interrupt handler will recognize and process
> the appropriate interrupt on the basis of interrupt status register.
> Also, establish synchronization between the interrupt handler and
> transfer operation by taking the locks and registering the interrupt
> handler as thread IRQ which avoids the bottom half.
> Fixes the wrongly populated interrupt register offsets too.

This sounds like at least three different changes mixed together in one
commit, it makes it quite hard to tell what's going on.  If nothing else
the conversion from a workqueue to threaded interrupts should probably
be split out from merging the interrupts.

> -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data)
> +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi)
>  {
> -	struct lantiq_ssc_spi *spi = data;
>  	u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT);
>  
> -	if (!(stat & LTQ_SPI_STAT_ERRORS))
> -		return IRQ_NONE;
> -

Why drop this?

> -	err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt,
> -			       0, LTQ_SPI_RX_IRQ_NAME, spi);
> +	err = devm_request_threaded_irq(dev, rx_irq, NULL, lantiq_ssc_isr,
> +					IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi);
>  	if (err)
>  		goto err_master_put;
>  
> -	err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt,
> -			       0, LTQ_SPI_TX_IRQ_NAME, spi);
> +	err = devm_request_threaded_irq(dev, tx_irq, NULL, lantiq_ssc_isr,
> +					IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi);
>  	if (err)
>  		goto err_master_put;
>  
> -	err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt,
> -			       0, LTQ_SPI_ERR_IRQ_NAME, spi);
> +	err = devm_request_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr,
> +					IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi);

It's not clear to me that it's a benefit to combine all the interrupts
unconditionally - obviously where they're shared we need to but could
that be accomplished with IRQF_SHARED and even if it can't it seems like
something conditional would be better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]