From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D1D2C43381 for ; Thu, 18 Feb 2021 16:45:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6BEA964EB8 for ; Thu, 18 Feb 2021 16:45:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233811AbhBRQmv (ORCPT ); Thu, 18 Feb 2021 11:42:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39218 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232041AbhBRN6H (ORCPT ); Thu, 18 Feb 2021 08:58:07 -0500 Received: from mail.marcansoft.com (marcansoft.com [IPv6:2a01:298:fe:f::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9755AC061574 for ; Thu, 18 Feb 2021 05:57:26 -0800 (PST) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id C0AB541E96; Thu, 18 Feb 2021 13:53:12 +0000 (UTC) To: Krzysztof Kozlowski Cc: linux-arm-kernel@lists.infradead.org, Marc Zyngier , Rob Herring , Arnd Bergmann , Olof Johansson , Mark Kettenis , Tony Lindgren , Mohamed Mediouni , Stan Skowronek , Alexander Graf , Will Deacon , Linus Walleij , Mark Rutland , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20210215121713.57687-1-marcan@marcan.st> <20210215121713.57687-20-marcan@marcan.st> <20210215184012.sf6p6dbk5c25phdm@kozik-lap> From: Hector Martin Subject: Re: [PATCH v2 19/25] tty: serial: samsung_tty: IRQ rework Message-ID: <31068a51-736b-08f6-6c00-1779734465ea@marcan.st> Date: Thu, 18 Feb 2021 22:53:10 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210215184012.sf6p6dbk5c25phdm@kozik-lap> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: es-ES Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16/02/2021 03.40, Krzysztof Kozlowski wrote: > On Mon, Feb 15, 2021 at 09:17:07PM +0900, Hector Martin wrote: >> * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, >> where only the latter acquires the port lock. > > I miss here information why you do all this. Added an explanation for v3. This is used by a subsequent patch in the series. >> >> * For S3C64xx, return IRQ_NONE if no flag bits were set, so the >> interrupt core can detect IRQ storms. Note that both IRQ handlers >> always return IRQ_HANDLED anyway, so 'or' logic isn't necessary here, >> if either handler ran we are always going to return IRQ_HANDLED. > > It looks like separate patch. Your patches should do only one thing at > once. The fact that you have here three bullet points is a warning > sign. This is even more important if you do some refactorings and > cleanups which should not affect functionality. Hiding there changes > which could affect functionality is a no-go. I've reverted this one. I don't think it should affect functionality, but I don't have any way to test on these devices, so I'll leave it to someone else to be safe :) >> >> * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for >> consistency with the above. All it does now is call two other >> functions anyway. > > Separate patch for trivial renaming. I think it makes sense to do this rename together with the first change above, as it keeps both functions symmetric. Otherwise we end up with an inconsistent function naming between both patches. If you really want it separate though, I can do that. > >> >> Signed-off-by: Hector Martin >> --- >> drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- >> 1 file changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index 21955be680a4..821cd0e4f870 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { >> #endif >> }; >> >> +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); >> + >> /* conversion functions */ >> >> #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) >> @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) >> ourport->tx_mode = 0; >> } >> >> -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> - > > Why moving this? Why adding s3c24xx_serial_tx_chars() forward > declaration? This should've gone in the next patch. A previous reviewer told me to put declarations at the top of the file, so I put it there and moved this one along with it, but I'll keep it to the additon only for v3. -- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 784EEC433DB for ; Thu, 18 Feb 2021 13:55:48 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 09B8064E33 for ; Thu, 18 Feb 2021 13:55:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 09B8064E33 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=marcan.st Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:Subject: From:References:To:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=tlLJz9BfMsXRW4vuQi9FOYWsvDlqF/7+5EyL8xErDwY=; b=gkcj0zv3qQ7zkhXhcaKx2yD69 evFT44bJxBpLEFDoeGViU8Mbtq4K6m0CcIwiZ/JBcNi3FJy6R9tnBOnOnW3MsOHSV1QoUwMNGc14x 5c8VShgEARxcjq8zEcG7l1hUIbntkfDHePS+hkOyM19CPT313n+MQ/PrELfxN7nGneKC1SXvn0Hpm hBLWFOILcCRgjN7iJ+JWaVBOthP/Dg5p+yYeBtxpX1CO8n7SCeBDZMs/myLU7RPcLylRN60s7jmdE EjdFYmpCPlcQdCtdoQYnPGs2/mQtsGRhgGMEWgkUK6l9uo+r92Oak9XaVveFur4UPriXSmnsb8B+7 RDtE5hk0g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lCjkC-00040Q-FX; Thu, 18 Feb 2021 13:53:24 +0000 Received: from marcansoft.com ([2a01:298:fe:f::2] helo=mail.marcansoft.com) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lCjk9-000406-9U for linux-arm-kernel@lists.infradead.org; Thu, 18 Feb 2021 13:53:22 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id C0AB541E96; Thu, 18 Feb 2021 13:53:12 +0000 (UTC) To: Krzysztof Kozlowski References: <20210215121713.57687-1-marcan@marcan.st> <20210215121713.57687-20-marcan@marcan.st> <20210215184012.sf6p6dbk5c25phdm@kozik-lap> From: Hector Martin Subject: Re: [PATCH v2 19/25] tty: serial: samsung_tty: IRQ rework Message-ID: <31068a51-736b-08f6-6c00-1779734465ea@marcan.st> Date: Thu, 18 Feb 2021 22:53:10 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210215184012.sf6p6dbk5c25phdm@kozik-lap> Content-Language: es-ES X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210218_085321_478180_6F4EE193 X-CRM114-Status: GOOD ( 25.75 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Arnd Bergmann , Rob Herring , Tony Lindgren , Marc Zyngier , Linus Walleij , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Alexander Graf , Olof Johansson , Mohamed Mediouni , Stan Skowronek , Will Deacon , linux-arm-kernel@lists.infradead.org, Mark Kettenis Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 16/02/2021 03.40, Krzysztof Kozlowski wrote: > On Mon, Feb 15, 2021 at 09:17:07PM +0900, Hector Martin wrote: >> * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, >> where only the latter acquires the port lock. > > I miss here information why you do all this. Added an explanation for v3. This is used by a subsequent patch in the series. >> >> * For S3C64xx, return IRQ_NONE if no flag bits were set, so the >> interrupt core can detect IRQ storms. Note that both IRQ handlers >> always return IRQ_HANDLED anyway, so 'or' logic isn't necessary here, >> if either handler ran we are always going to return IRQ_HANDLED. > > It looks like separate patch. Your patches should do only one thing at > once. The fact that you have here three bullet points is a warning > sign. This is even more important if you do some refactorings and > cleanups which should not affect functionality. Hiding there changes > which could affect functionality is a no-go. I've reverted this one. I don't think it should affect functionality, but I don't have any way to test on these devices, so I'll leave it to someone else to be safe :) >> >> * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for >> consistency with the above. All it does now is call two other >> functions anyway. > > Separate patch for trivial renaming. I think it makes sense to do this rename together with the first change above, as it keeps both functions symmetric. Otherwise we end up with an inconsistent function naming between both patches. If you really want it separate though, I can do that. > >> >> Signed-off-by: Hector Martin >> --- >> drivers/tty/serial/samsung_tty.c | 41 +++++++++++++++++++------------- >> 1 file changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index 21955be680a4..821cd0e4f870 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -151,6 +151,9 @@ struct s3c24xx_uart_port { >> #endif >> }; >> >> +static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport); >> + >> /* conversion functions */ >> >> #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev) >> @@ -316,8 +319,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port) >> ourport->tx_mode = 0; >> } >> >> -static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport); >> - > > Why moving this? Why adding s3c24xx_serial_tx_chars() forward > declaration? This should've gone in the next patch. A previous reviewer told me to put declarations at the top of the file, so I put it there and moved this one along with it, but I'll keep it to the additon only for v3. -- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel