On 2017-01-16 10:24, Andy Shevchenko wrote: > On Mon, 2017-01-16 at 10:05 +0100, Jan Kiszka wrote: >> When using the a device with edge-triggered interrupts, such as MSIs, >> the interrupt handler has to ensure that there is a point in time >> during >> its execution where all interrupts sources are silent so that a new >> event can trigger a new interrupt again. >> >> This is achieved here by looping over SSSR evaluation. We need to take >> into account that SSCR1 may be changed by the transfer handler, thus >> we >> need to redo the mask calculation, at least regarding the volatile >> interrupt enable bit (TIE). > > Could you split this to two patches, one just move the code under > question to a helper function (no functional change), the other does > what you state in commit message here?
IMHO, factoring out some helper called from the loop in ssp_int won't be a natural split due to the large number of local variables being shared here. But maybe I'm not seeing the design you have in mind, so please propose a useful helper function signature. Thanks, Jan > >> >> Signed-off-by: Jan Kiszka <[email protected]> >> --- >> drivers/spi/spi-pxa2xx.c | 50 +++++++++++++++++++++++++++---------- >> ----------- >> 1 file changed, 28 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c >> index dd7b5b4..24bf549 100644 >> --- a/drivers/spi/spi-pxa2xx.c >> +++ b/drivers/spi/spi-pxa2xx.c >> @@ -737,6 +737,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id) >> struct driver_data *drv_data = dev_id; >> u32 sccr1_reg; >> u32 mask = drv_data->mask_sr; >> + irqreturn_t ret = IRQ_NONE; >> u32 status; >> >> /* >> @@ -760,37 +761,42 @@ static irqreturn_t ssp_int(int irq, void >> *dev_id) >> >> sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1); >> >> - /* Ignore possible writes if we don't need to write */ >> - if (!(sccr1_reg & SSCR1_TIE)) >> - mask &= ~SSSR_TFS; >> - >> /* Ignore RX timeout interrupt if it is disabled */ >> if (!(sccr1_reg & SSCR1_TINTE)) >> mask &= ~SSSR_TINT; >> >> - if (!(status & mask)) >> - return IRQ_NONE; >> + while (1) { >> + /* Ignore possible writes if we don't need to write >> */ >> + if (!(sccr1_reg & SSCR1_TIE)) >> + mask &= ~SSSR_TFS; >> >> - if (!drv_data->master->cur_msg) { >> + if (!(status & mask)) >> + return ret; >> >> - pxa2xx_spi_write(drv_data, SSCR0, >> - pxa2xx_spi_read(drv_data, SSCR0) >> - & ~SSCR0_SSE); >> - pxa2xx_spi_write(drv_data, SSCR1, >> - pxa2xx_spi_read(drv_data, SSCR1) >> - & ~drv_data->int_cr1); >> - if (!pxa25x_ssp_comp(drv_data)) >> - pxa2xx_spi_write(drv_data, SSTO, 0); >> - write_SSSR_CS(drv_data, drv_data->clear_sr); >> + if (!drv_data->master->cur_msg) { >> >> - dev_err(&drv_data->pdev->dev, >> - "bad message state in interrupt handler\n"); >> + pxa2xx_spi_write(drv_data, SSCR0, >> + pxa2xx_spi_read(drv_data, >> SSCR0) >> + & ~SSCR0_SSE); >> + pxa2xx_spi_write(drv_data, SSCR1, >> + pxa2xx_spi_read(drv_data, >> SSCR1) >> + & ~drv_data->int_cr1); >> + if (!pxa25x_ssp_comp(drv_data)) >> + pxa2xx_spi_write(drv_data, SSTO, 0); >> + write_SSSR_CS(drv_data, drv_data->clear_sr); >> >> - /* Never fail */ >> - return IRQ_HANDLED; >> - } >> + dev_err(&drv_data->pdev->dev, >> + "bad message state in interrupt >> handler\n"); >> >> - return drv_data->transfer_handler(drv_data); >> + /* Never fail */ >> + return IRQ_HANDLED; >> + } >> + >> + ret |= drv_data->transfer_handler(drv_data); >> + >> + status = pxa2xx_spi_read(drv_data, SSSR); >> + sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1); >> + } >> } >> >> /* > -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
