From: Bjorn Helgaas <helgaas@kernel.org> To: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: linux-pci@vger.kernel.org, Fabio Estevam <fabio.estevam@nxp.com>, cphealy@gmail.com, l.stach@pengutronix.de, Leonard Crestez <leonard.crestez@nxp.com>, "A.s. Dong" <aisheng.dong@nxp.com>, Richard Zhu <hongxing.zhu@nxp.com>, linux-imx@nxp.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>, Rob Herring <robh@kernel.org>, devicetree@vger.kernel.org, "Gustavo A. R. Silva" <gustavo@embeddedor.com> Subject: Re: [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ Date: Fri, 14 Dec 2018 14:30:42 -0600 [thread overview] Message-ID: <20181214203042.GC20725@google.com> (raw) In-Reply-To: <20181206073545.10967-4-andrew.smirnov@gmail.com> [+cc Gustavo for fallthrough annotation] On Wed, Dec 05, 2018 at 11:35:45PM -0800, Andrey Smirnov wrote: > Add code needed to support i.MX8MQ variant. > @@ -245,7 +253,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie) > { > u32 tmp; > > - if (imx6_pcie->variant == IMX7D) > + if (imx6_pcie->variant == IMX7D || > + imx6_pcie->variant == IMX8MQ) This style looks like a maintenance problem: the code below is probably IMX6-specific, and you should test for *that* instead of adding to this list of things that are *not* IMX6, because that list is likely to continue growing. There are more occurrences below. > @@ -301,6 +312,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > > switch (imx6_pcie->variant) { > case IMX7D: > + case IMX8MQ: /* FALLTHROUGH */ > reset_control_assert(imx6_pcie->pciephy_reset); > reset_control_assert(imx6_pcie->apps_reset); > break; I'm not an expert on fallthrough annotation (Gustavo, cc'd, is), but this looks wrong. It's the IMX7D case that falls through, not the IMX8MQ case. The recent annotations added by Gustavo are at the point where the "break" would normally be, e.g., case IMX7D: /* fall through */ <--- annotation case IMX8MQ: <code> break; But in this case there's actually no IMX7D-specific *code* there, so I suspect the annotation is unnecessary. It's obvious that IMX7D and IMX8MQ are handled the same, so there's really no opportunity for the "forgotten break" mistake -Wimplicit-fallthrough is trying to find. If we *do* want this annotation, we should spell it the same as Gustavo has been, i.e., "fall through". Again, more occurrences below. Bjorn
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: "A.s. Dong" <aisheng.dong@nxp.com>, Mark Rutland <mark.rutland@arm.com>, Richard Zhu <hongxing.zhu@nxp.com>, linux-arm-kernel@lists.infradead.org, Rob Herring <robh@kernel.org>, linux-pci@vger.kernel.org, "Gustavo A. R. Silva" <gustavo@embeddedor.com>, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-imx@nxp.com, Fabio Estevam <fabio.estevam@nxp.com>, Leonard Crestez <leonard.crestez@nxp.com>, cphealy@gmail.com, l.stach@pengutronix.de Subject: Re: [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ Date: Fri, 14 Dec 2018 14:30:42 -0600 [thread overview] Message-ID: <20181214203042.GC20725@google.com> (raw) In-Reply-To: <20181206073545.10967-4-andrew.smirnov@gmail.com> [+cc Gustavo for fallthrough annotation] On Wed, Dec 05, 2018 at 11:35:45PM -0800, Andrey Smirnov wrote: > Add code needed to support i.MX8MQ variant. > @@ -245,7 +253,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie) > { > u32 tmp; > > - if (imx6_pcie->variant == IMX7D) > + if (imx6_pcie->variant == IMX7D || > + imx6_pcie->variant == IMX8MQ) This style looks like a maintenance problem: the code below is probably IMX6-specific, and you should test for *that* instead of adding to this list of things that are *not* IMX6, because that list is likely to continue growing. There are more occurrences below. > @@ -301,6 +312,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > > switch (imx6_pcie->variant) { > case IMX7D: > + case IMX8MQ: /* FALLTHROUGH */ > reset_control_assert(imx6_pcie->pciephy_reset); > reset_control_assert(imx6_pcie->apps_reset); > break; I'm not an expert on fallthrough annotation (Gustavo, cc'd, is), but this looks wrong. It's the IMX7D case that falls through, not the IMX8MQ case. The recent annotations added by Gustavo are at the point where the "break" would normally be, e.g., case IMX7D: /* fall through */ <--- annotation case IMX8MQ: <code> break; But in this case there's actually no IMX7D-specific *code* there, so I suspect the annotation is unnecessary. It's obvious that IMX7D and IMX8MQ are handled the same, so there's really no opportunity for the "forgotten break" mistake -Wimplicit-fallthrough is trying to find. If we *do* want this annotation, we should spell it the same as Gustavo has been, i.e., "fall through". Again, more occurrences below. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2018-12-14 20:30 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-06 7:35 [PATCH v2 0/3] PCIE support for i.MX8MQ Andrey Smirnov 2018-12-06 7:35 ` Andrey Smirnov 2018-12-06 7:35 ` [PATCH v2 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D Andrey Smirnov 2018-12-06 7:35 ` Andrey Smirnov 2018-12-06 9:41 ` Lucas Stach 2018-12-06 9:41 ` Lucas Stach 2018-12-06 7:35 ` [PATCH v2 2/3] PCI: imx: No-op imx6_pcie_reset_phy() " Andrey Smirnov 2018-12-06 7:35 ` Andrey Smirnov 2018-12-06 9:41 ` Lucas Stach 2018-12-06 9:41 ` Lucas Stach 2018-12-06 7:35 ` [PATCH v2 3/3] PCI: imx: Add support for i.MX8MQ Andrey Smirnov 2018-12-06 7:35 ` Andrey Smirnov 2018-12-06 10:23 ` Lucas Stach 2018-12-06 10:23 ` Lucas Stach 2018-12-14 20:30 ` Bjorn Helgaas [this message] 2018-12-14 20:30 ` Bjorn Helgaas 2018-12-14 20:55 ` Gustavo A. R. Silva 2018-12-14 20:55 ` Gustavo A. R. Silva 2018-12-15 5:22 ` Andrey Smirnov 2018-12-15 5:22 ` Andrey Smirnov 2018-12-15 5:22 ` Andrey Smirnov 2018-12-06 12:15 ` [PATCH v2 0/3] PCIE " Lorenzo Pieralisi 2018-12-06 12:15 ` Lorenzo Pieralisi 2018-12-07 2:44 ` Andrey Smirnov 2018-12-07 2:44 ` Andrey Smirnov 2018-12-07 2:44 ` Andrey Smirnov 2018-12-14 20:38 ` Bjorn Helgaas 2018-12-14 20:38 ` Bjorn Helgaas 2018-12-15 5:25 ` Andrey Smirnov 2018-12-15 5:25 ` Andrey Smirnov 2018-12-15 5:25 ` Andrey Smirnov 2018-12-17 10:12 ` Lucas Stach 2018-12-17 10:12 ` Lucas Stach 2018-12-17 10:12 ` Lucas Stach 2018-12-17 10:26 ` Lorenzo Pieralisi 2018-12-17 10:26 ` Lorenzo Pieralisi 2018-12-17 10:26 ` Lorenzo Pieralisi 2018-12-17 11:11 ` Lorenzo Pieralisi 2018-12-17 11:11 ` Lorenzo Pieralisi 2018-12-17 11:11 ` Lorenzo Pieralisi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181214203042.GC20725@google.com \ --to=helgaas@kernel.org \ --cc=aisheng.dong@nxp.com \ --cc=andrew.smirnov@gmail.com \ --cc=cphealy@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=fabio.estevam@nxp.com \ --cc=gustavo@embeddedor.com \ --cc=hongxing.zhu@nxp.com \ --cc=l.stach@pengutronix.de \ --cc=leonard.crestez@nxp.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.