lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
From
Date

On 4/24/2020 7:25 PM, Mark Brown wrote:
> 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.
While preparing the patches, i got puzzled to go with separate patches
(for threaded interrupts, unified interrupt handler and fixing the
register offset) or as a single patch!!.
Finally i choose to go with single patch, because establishing
synchronization is the major reason for this change, for that reason
threaded interrupts and unified interrupts changes are done. And the
fixing offset is a single line change, so included in this patch itself.
And, on a lighter note, the whole patch is coming under 45 lines of code
changes.
Please let me know your view.
>
>> -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?
lantiq_ssc_err_interrupt() getting called, only if LTQ_SPI_IRNEN_E is
set in the interrupt status register.
Once the 'LTQ_SPI_IRNEN_E' bit is set, there is no chance of all error
bits being unset in the SPI_STAT register, so the 'if condition' will
never be successful. Hence dropped it.
>
>> - 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.

Lets take a case where Tx/Rx transfer interrupt got triggered and
followed by error interrupt(before finishing the tx/rx interrupt
execution) which is very less likely to occur, unified interrupt handler
establishes synchronization.
Comparatively, unified interrupt handler is better for adding support to
the latest SoCs on which SPI have single interrupt line for tx,rx and
errors.
On basis of these two points i felt to go with unified interrupt handler.

Regards,
Dilip

\
 
 \ /
  Last update: 2020-04-27 08:02    [W:0.092 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site