| 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 |
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);
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 --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)) {
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(-)