Message ID | 20230710154932.68377-6-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp5128043vqx; Mon, 10 Jul 2023 09:05:47 -0700 (PDT) X-Google-Smtp-Source: APBJJlGoX/A91RztrEcVw2ZzFEtaYB/+9yA8t1CNklI9SN4WF1S9a1uFHqpQS4NWogUToheeWBx0 X-Received: by 2002:a05:6808:1383:b0:3a1:b9d7:3821 with SMTP id c3-20020a056808138300b003a1b9d73821mr13763324oiw.37.1689005147157; Mon, 10 Jul 2023 09:05:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689005147; cv=none; d=google.com; s=arc-20160816; b=0RTsDQDkw96hWkEJoGBk2ngbGzYtsc5zXDKHpCV0Ue1O6L6JVVtf4xXkLVDm3nUbdK fT6MhYuduS1H9hJdrlJMPX9EUtye3TqHd0mKfxyvI9DK7hJvbsnB5PwOHCGOm0plZyCX UNrG+Vc/gDqeL/Fy6OrzidRec7UhQd0MwHcpJJuF3IX1a9nsWcmLDRtajuJ8qdD0oynf 2S6Yc1NHK+2tBzzvVLFQkVYcVhiNLdXg7iX9LxP9Ok2k3vAmLNhnq0K4kRFKwhxZukqo iuxUxRMD5iSlPrMTriz04vGTVl370VF2Hd1Z28pP+aO/i7W1JyJYNOiBwjKGcdcUHq8r j0Lg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=dnF+LMHcVlGKDCydGVoyV5aZC40d8p/xMENgpGUnHO0=; fh=CWfEt6/vd4IsHMc+T8W3bMkH4b5uOoX72TeJSijIGlU=; b=f+z3WDFbbRsw62SD//sB9m7q9wlNpYKhR6GkSpyI5ra2aC2VX1/47/SjjgcxmIRiOI WKvQ7LXySAfEyvhjye/W/8aiL0DpyGVuDZXWBbejEMCfGf3r+jGLe9NvsCUQyFO8MsOo bVkMeIEYLpapnjw4/JX4ojC1NMSLpfm0ZJGwJU8jZDFVDPOjtX5yXizGlZgU6OM6CNI3 ejOYXUi6ukMyqlvZFeKMm8nOL6iuLJ+Tn1xJrYQax85CLYHPT5LpKZtd96a8JQHWYICQ dnTcx+Gi7Q2eh7egKk+XTOVk6CSCnTq1A08Z3Z7j217evcs3LFcO/t0mDZnYyQRW41qb GB5g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=QZam7m3P; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 26-20020a63195a000000b005577ad28a97si4288817pgz.633.2023.07.10.09.05.34; Mon, 10 Jul 2023 09:05:47 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=QZam7m3P; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233631AbjGJPt5 (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Mon, 10 Jul 2023 11:49:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47644 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233583AbjGJPts (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 10 Jul 2023 11:49:48 -0400 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 25A25137; Mon, 10 Jul 2023 08:49:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1689004186; x=1720540186; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=0v8XTkg22X4+SjZKJZYJseJJcu9T6jyLmPuYe7Mrmmg=; b=QZam7m3PSZMzi8qOur8uz44+WuSV1Ye2Ok33FuV6B0NUgKABvOXLDSeb deibbcVTr5wVEXsyZD4N4ftH4Ba52nW8f2MQtMlI/TucDm7s7IF9ZMFJm oTb0wS6f4qjjh0g/YE8o9kCQ9vHm7/OXRFsO4i0Ct0lLMAU5v/nahU8FS 2kyk4GqO8J2hMrusFVgTlMS9DIL2LGQnKfUFBzdTJFmaY+0t6rQ+MB93a muUyKeFEXzmavWsSR9xEpzWLXmnuWaOXxbL268sLLmVXwftsnSRsWMVsu FfY/ojY3Otv+covAncddYCHfsYyo0BQNgIJRInzb2YVzxLzlWIJTqGcW4 Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10767"; a="349185404" X-IronPort-AV: E=Sophos;i="6.01,194,1684825200"; d="scan'208";a="349185404" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Jul 2023 08:49:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10767"; a="844921854" X-IronPort-AV: E=Sophos;i="6.01,194,1684825200"; d="scan'208";a="844921854" Received: from black.fi.intel.com ([10.237.72.28]) by orsmga004.jf.intel.com with ESMTP; 10 Jul 2023 08:49:33 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id 6932A5FC; Mon, 10 Jul 2023 18:49:34 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Mark Brown <broonie@kernel.org>, Cristian Ciocaltea <cristian.ciocaltea@collabora.com>, Yang Yingliang <yangyingliang@huawei.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Amit Kumar Mahapatra via Alsa-devel <alsa-devel@alsa-project.org>, Neil Armstrong <neil.armstrong@linaro.org>, Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>, Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>, =?utf-8?q?Uwe_Kleine-K?= =?utf-8?q?=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-riscv@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-trace-kernel@vger.kernel.org, netdev@vger.kernel.org Cc: Sanjay R Mehta <sanju.mehta@amd.com>, Radu Pirea <radu_nicolae.pirea@upb.ro>, Nicolas Ferre <nicolas.ferre@microchip.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Claudiu Beznea <claudiu.beznea@microchip.com>, Tudor Ambarus <tudor.ambarus@linaro.org>, Serge Semin <fancer.lancer@gmail.com>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Kevin Hilman <khilman@baylibre.com>, Jerome Brunet <jbrunet@baylibre.com>, Martin Blumenstingl <martin.blumenstingl@googlemail.com>, Matthias Brugger <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Heiko Stuebner <heiko@sntech.de>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Orson Zhai <orsonzhai@gmail.com>, Baolin Wang <baolin.wang@linux.alibaba.com>, Chunyan Zhang <zhang.lyra@gmail.com>, Alain Volmat <alain.volmat@foss.st.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Alexandre Torgue <alexandre.torgue@foss.st.com>, Max Filippov <jcmvbkbc@gmail.com>, Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>, Richard Cochran <richardcochran@gmail.com> Subject: [PATCH v2 05/15] spi: Remove code duplication in spi_add_device_locked() Date: Mon, 10 Jul 2023 18:49:22 +0300 Message-Id: <20230710154932.68377-6-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b In-Reply-To: <20230710154932.68377-1-andriy.shevchenko@linux.intel.com> References: <20230710154932.68377-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771050261138228923 X-GMAIL-MSGID: 1771050261138228923 |
Series |
spi: Header and core clean up and refactoring
|
|
Commit Message
Andy Shevchenko
July 10, 2023, 3:49 p.m. UTC
Seems by unknown reason, probably some kind of mis-rebase,
the commit 0c79378c0199 ("spi: add ancillary device support")
adds a dozen of duplicating lines of code. Drop them.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/spi/spi.c | 11 -----------
1 file changed, 11 deletions(-)
Comments
On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > Seems by unknown reason, probably some kind of mis-rebase, > the commit 0c79378c0199 ("spi: add ancillary device support") > adds a dozen of duplicating lines of code. Drop them. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/spi/spi.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index c99ee4164f11..46cbda383228 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > static int spi_add_device_locked(struct spi_device *spi) > { > struct spi_controller *ctlr = spi->controller; > - struct device *dev = ctlr->dev.parent; > - > - /* Chipselects are numbered 0..max; validate. */ > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > - ctlr->num_chipselect); > - return -EINVAL; > - } > - > - /* Set the bus ID string */ > - spi_dev_set_name(spi); I see that this is duplicating spi_add_device() (and we really could do better with code sharing there I think) but I can't immediately see where the duplication that's intended to be elimiated is here - where else in the one call path that spi_add_device_locked() has would we do the above? Based on the changelog I was expecting to see some duplicated code in the function itself.
On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > > Seems by unknown reason, probably some kind of mis-rebase, > > the commit 0c79378c0199 ("spi: add ancillary device support") > > adds a dozen of duplicating lines of code. Drop them. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/spi/spi.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index c99ee4164f11..46cbda383228 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > > static int spi_add_device_locked(struct spi_device *spi) > > { > > struct spi_controller *ctlr = spi->controller; > > - struct device *dev = ctlr->dev.parent; > > - > > - /* Chipselects are numbered 0..max; validate. */ > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > - ctlr->num_chipselect); > > - return -EINVAL; > > - } > > - > > - /* Set the bus ID string */ > > - spi_dev_set_name(spi); > > I see that this is duplicating spi_add_device() (and we really could do > better with code sharing there I think) but I can't immediately see > where the duplication that's intended to be elimiated is here - where > else in the one call path that spi_add_device_locked() has would we do > the above? Based on the changelog I was expecting to see some > duplicated code in the function itself. Oh, by some reason Sebastian wasn't in this rather long Cc list. Added him. Reading again I don't see any useful explanation why that piece of code has to be duplicated among these two functions. It's 100% a copy. Sebastian, can you shed some light here?
Hi, On Tue, Jul 11, 2023 at 02:06:20PM +0300, Andy Shevchenko wrote: > On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: > > > Seems by unknown reason, probably some kind of mis-rebase, > > > the commit 0c79378c0199 ("spi: add ancillary device support") > > > adds a dozen of duplicating lines of code. Drop them. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/spi/spi.c | 11 ----------- > > > 1 file changed, 11 deletions(-) > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index c99ee4164f11..46cbda383228 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); > > > static int spi_add_device_locked(struct spi_device *spi) > > > { > > > struct spi_controller *ctlr = spi->controller; > > > - struct device *dev = ctlr->dev.parent; > > > - > > > - /* Chipselects are numbered 0..max; validate. */ > > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > > - ctlr->num_chipselect); > > > - return -EINVAL; > > > - } > > > - > > > - /* Set the bus ID string */ > > > - spi_dev_set_name(spi); > > > > I see that this is duplicating spi_add_device() (and we really could do > > better with code sharing there I think) but I can't immediately see > > where the duplication that's intended to be elimiated is here - where > > else in the one call path that spi_add_device_locked() has would we do > > the above? Based on the changelog I was expecting to see some > > duplicated code in the function itself. > > Oh, by some reason Sebastian wasn't in this rather long Cc list. > Added him. > > Reading again I don't see any useful explanation why that piece of code has to > be duplicated among these two functions. It's 100% a copy. > > Sebastian, can you shed some light here? The patch in this thread is obviously wrong. It results in the checks never beeing called for spi_add_device_locked(). The copy is in spi_add_device() and those two are not calling into each other. But it should be fine to move the code to the start of __spi_add_device(), which allows removing the duplication. In that case the code will be run with the add_lock held, which is probably what I was worried about two years ago. Looking at it again, the lock is held anyways in case of spi_add_device_locked(). Greetings, -- Sebastian
On Tue, Jul 11, 2023 at 02:01:33PM +0200, Sebastian Reichel wrote: > On Tue, Jul 11, 2023 at 02:06:20PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 10, 2023 at 06:16:22PM +0100, Mark Brown wrote: > > > On Mon, Jul 10, 2023 at 06:49:22PM +0300, Andy Shevchenko wrote: ... > > > > - struct device *dev = ctlr->dev.parent; > > > > - > > > > - /* Chipselects are numbered 0..max; validate. */ > > > > - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { > > > > - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), > > > > - ctlr->num_chipselect); > > > > - return -EINVAL; > > > > - } > > > > - > > > > - /* Set the bus ID string */ > > > > - spi_dev_set_name(spi); > > > > > > I see that this is duplicating spi_add_device() (and we really could do > > > better with code sharing there I think) but I can't immediately see > > > where the duplication that's intended to be elimiated is here - where > > > else in the one call path that spi_add_device_locked() has would we do > > > the above? Based on the changelog I was expecting to see some > > > duplicated code in the function itself. > > > > Oh, by some reason Sebastian wasn't in this rather long Cc list. > > Added him. > > > > Reading again I don't see any useful explanation why that piece of code has to > > be duplicated among these two functions. It's 100% a copy. > > > > Sebastian, can you shed some light here? > > The patch in this thread is obviously wrong. It results in the > checks never beeing called for spi_add_device_locked(). The copy is > in spi_add_device() and those two are not calling into each other. Ah, now I see, I missed __ in the name. Thank you for opening my eyes! > But it should be fine to move the code to the start of > __spi_add_device(), which allows removing the duplication. In that > case the code will be run with the add_lock held, which is probably > what I was worried about two years ago. Looking at it again, the > lock is held anyways in case of spi_add_device_locked(). Right, I will re-do that.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c99ee4164f11..46cbda383228 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -712,17 +712,6 @@ EXPORT_SYMBOL_GPL(spi_add_device); static int spi_add_device_locked(struct spi_device *spi) { struct spi_controller *ctlr = spi->controller; - struct device *dev = ctlr->dev.parent; - - /* Chipselects are numbered 0..max; validate. */ - if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) { - dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0), - ctlr->num_chipselect); - return -EINVAL; - } - - /* Set the bus ID string */ - spi_dev_set_name(spi); WARN_ON(!mutex_is_locked(&ctlr->add_lock)); return __spi_add_device(spi);