diff mbox series

[3/3] PCI: imx6: Make fallthrough comments more consistent

Message ID 20181216230916.22982-4-andrew.smirnov@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fixes for "PCIE support for i.MX8MQ" | expand

Commit Message

Andrey Smirnov Dec. 16, 2018, 11:09 p.m. UTC
Convert all fallthrough comments to say "fall through", as well as
modify their placement to the point where the "break" would normally
be.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: linux-imx@nxp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas Dec. 17, 2018, 1:58 p.m. UTC | #1
On Sun, Dec 16, 2018 at 03:09:16PM -0800, Andrey Smirnov wrote:
> Convert all fallthrough comments to say "fall through", as well as
> modify their placement to the point where the "break" would normally
> be.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>

I didn't make it very clear, but my suggestion was really to remove
the annotation completely; see below.

> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 59658577e81d..a0510e185d44 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -362,7 +362,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  
>  	switch (imx6_pcie->variant) {
>  	case IMX7D:
> -	case IMX8MQ: /* FALLTHROUGH */
> +		/* fall through */
> +	case IMX8MQ:
>  		reset_control_assert(imx6_pcie->pciephy_reset);
>  		reset_control_assert(imx6_pcie->apps_reset);
>  		break;

IMO this use of "fall through" is superfluous and unusual in the Linux
source.

A "fall through" comment would be useful if the IMX7D case had
executable code but no "break".  Then the comment shows that the
intent was to execute *both* the IMX7D code and the IMX8MQ code and
the lack of a "break" was intentional.

In this case, the intent is to treat IMX7D and IMX8MQ the same, and
there's no executable code specifically for the IMX7D.  I think it's
easiest to read that when the list of identical cases is all together
without the comment in the middle, i.e., as

>  	case IMX7D:
> 	case IMX8MQ:
>  		reset_control_assert(imx6_pcie->pciephy_reset);

rather than this:

>  	case IMX7D:
> 		/* fall through */
> 	case IMX8MQ:
>  		reset_control_assert(imx6_pcie->pciephy_reset);
Andrey Smirnov Dec. 17, 2018, 6:03 p.m. UTC | #2
On Mon, Dec 17, 2018 at 5:58 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Sun, Dec 16, 2018 at 03:09:16PM -0800, Andrey Smirnov wrote:
> > Convert all fallthrough comments to say "fall through", as well as
> > modify their placement to the point where the "break" would normally
> > be.
> >
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: linux-imx@nxp.com
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-pci@vger.kernel.org
> > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
>
> I didn't make it very clear, but my suggestion was really to remove
> the annotation completely; see below.
>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 59658577e81d..a0510e185d44 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -362,7 +362,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >
> >       switch (imx6_pcie->variant) {
> >       case IMX7D:
> > -     case IMX8MQ: /* FALLTHROUGH */
> > +             /* fall through */
> > +     case IMX8MQ:
> >               reset_control_assert(imx6_pcie->pciephy_reset);
> >               reset_control_assert(imx6_pcie->apps_reset);
> >               break;
>
> IMO this use of "fall through" is superfluous and unusual in the Linux
> source.
>
> A "fall through" comment would be useful if the IMX7D case had
> executable code but no "break".  Then the comment shows that the
> intent was to execute *both* the IMX7D code and the IMX8MQ code and
> the lack of a "break" was intentional.
>
> In this case, the intent is to treat IMX7D and IMX8MQ the same, and
> there's no executable code specifically for the IMX7D.  I think it's
> easiest to read that when the list of identical cases is all together
> without the comment in the middle, i.e., as
>
> >       case IMX7D:
> >       case IMX8MQ:
> >               reset_control_assert(imx6_pcie->pciephy_reset);
>
> rather than this:
>
> >       case IMX7D:
> >               /* fall through */
> >       case IMX8MQ:
> >               reset_control_assert(imx6_pcie->pciephy_reset);

OK, understood, will remove in next version.

Thanks,
Andrey Smirnov
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 59658577e81d..a0510e185d44 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -362,7 +362,8 @@  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 
 	switch (imx6_pcie->variant) {
 	case IMX7D:
-	case IMX8MQ: /* FALLTHROUGH */
+		/* fall through */
+	case IMX8MQ:
 		reset_control_assert(imx6_pcie->pciephy_reset);
 		reset_control_assert(imx6_pcie->apps_reset);
 		break;
@@ -415,7 +416,8 @@  static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
 		break;
-	case IMX6QP:		/* FALLTHROUGH */
+	case IMX6QP:
+		/* fall through */
 	case IMX6Q:
 		/* power up core phy and enable ref clock */
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
@@ -607,7 +609,7 @@  static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX6SX_GPR12_PCIE_RX_EQ_MASK,
 				   IMX6SX_GPR12_PCIE_RX_EQ_2);
-		/* FALLTHROUGH */
+		/* fall through */
 	default:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
@@ -731,7 +733,8 @@  static void imx6_pcie_ltssm_enable(struct device *dev)
 				   IMX6Q_GPR12_PCIE_CTL_2);
 		break;
 	case IMX7D:
-	case IMX8MQ:		/* FALLTHROUGH */
+		/* fall through */
+	case IMX8MQ:
 		reset_control_deassert(imx6_pcie->apps_reset);
 		break;
 	}
@@ -1076,7 +1079,8 @@  static int imx6_pcie_probe(struct platform_device *pdev)
 		}
 		break;
 	case IMX8MQ:
-	case IMX7D:		/* FALLTHROUGH */
+		/* fall through */
+	case IMX7D:
 		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
 									    "pciephy");
 		if (IS_ERR(imx6_pcie->pciephy_reset)) {