From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751534AbdAPLSv (ORCPT ); Mon, 16 Jan 2017 06:18:51 -0500 Received: from david.siemens.de ([192.35.17.14]:48688 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751033AbdAPLSt (ORCPT ); Mon, 16 Jan 2017 06:18:49 -0500 Subject: Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts To: Andy Shevchenko , linux-arm-kernel@lists.infradead.org References: <6fe26505e67790b27eed28fd7451b51e70b7e8ba.1484557560.git.jan.kiszka@siemens.com> <1484558690.2133.132.camel@linux.intel.com> Cc: Daniel Mack , Haojian Zhuang , Robert Jarzmik , linux-kernel@vger.kernel.org, Mika Westerberg , Jarkko Nikula , Sascha Weisenberger From: Jan Kiszka Message-ID: <7cb06bc7-bcb6-a212-8241-20ad4b2a4a90@siemens.com> Date: Mon, 16 Jan 2017 12:18:10 +0100 User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 In-Reply-To: <1484558690.2133.132.camel@linux.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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