[PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[Nathan Chancellor]

Clang warns:

drivers/phy/intel/phy-intel-combo.c:202:34: warning: implicit conversion
from enumeration type 'enum intel_phy_mode' to different enumeration
type 'enum intel_combo_mode' [-Wenum-conversion]
        enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
                              ~~~~~~~   ^~~~~~~~~~~~~
1 warning generated.

The correct enum to use would be PCIE0_PCIE1_MODE. However, eliminating
this assignment is a little better because the switch statement always
assigns a new value to cb_mode, which also takes care of the warning.

Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy")
Link: https://github.com/ClangBuiltLinux/linux/issues/1034
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/phy/intel/phy-intel-combo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
index c2a35be4cdfb..4bc1276ecf23 100644
--- a/drivers/phy/intel/phy-intel-combo.c
+++ b/drivers/phy/intel/phy-intel-combo.c
@@ -199,9 +199,9 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
 
 static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
 {
-	enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
 	enum aggregated_mode aggr = cbphy->aggr_mode;
 	struct device *dev = cbphy->dev;
+	enum intel_combo_mode cb_mode;
 	enum intel_phy_mode mode;
 	int ret;
 

base-commit: c11d28ab4a691736e30b49813fb801847bd44e83
-- 
2.27.0.rc0

Re: [PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[Nick Desaulniers]

On Fri, May 22, 2020 at 8:51 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/phy/intel/phy-intel-combo.c:202:34: warning: implicit conversion
> from enumeration type 'enum intel_phy_mode' to different enumeration
> type 'enum intel_combo_mode' [-Wenum-conversion]
>         enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
>                               ~~~~~~~   ^~~~~~~~~~~~~
> 1 warning generated.
>
> The correct enum to use would be PCIE0_PCIE1_MODE. However, eliminating
> this assignment is a little better because the switch statement always

Indeed, the switch is exhaustive.  Might be a risk if new enumeration
values are added to the enum, though.

Probably should just initialize it to PCIE0_PCIE1_MODE, then you can
simplify the PHY_PCIE_MODE case a little (replace ternary with
if+assignment).

> assigns a new value to cb_mode, which also takes care of the warning.
>
> Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1034
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/phy/intel/phy-intel-combo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
> index c2a35be4cdfb..4bc1276ecf23 100644
> --- a/drivers/phy/intel/phy-intel-combo.c
> +++ b/drivers/phy/intel/phy-intel-combo.c
> @@ -199,9 +199,9 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
>
>  static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
>  {
> -       enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
>         enum aggregated_mode aggr = cbphy->aggr_mode;
>         struct device *dev = cbphy->dev;
> +       enum intel_combo_mode cb_mode;
>         enum intel_phy_mode mode;
>         int ret;
>
>
> base-commit: c11d28ab4a691736e30b49813fb801847bd44e83
> --

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[Nathan Chancellor]

On Tue, May 26, 2020 at 11:12:58AM -0700, Nick Desaulniers wrote:
> On Fri, May 22, 2020 at 8:51 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns:
> >
> > drivers/phy/intel/phy-intel-combo.c:202:34: warning: implicit conversion
> > from enumeration type 'enum intel_phy_mode' to different enumeration
> > type 'enum intel_combo_mode' [-Wenum-conversion]
> >         enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
> >                               ~~~~~~~   ^~~~~~~~~~~~~
> > 1 warning generated.
> >
> > The correct enum to use would be PCIE0_PCIE1_MODE. However, eliminating
> > this assignment is a little better because the switch statement always
> 
> Indeed, the switch is exhaustive.  Might be a risk if new enumeration
> values are added to the enum, though.

Clang will warn about that (and since there is no default case
statement, that would point to a bug).

> Probably should just initialize it to PCIE0_PCIE1_MODE, then you can
> simplify the PHY_PCIE_MODE case a little (replace ternary with
> if+assignment).

I did consider this but every maintainer has their preference around
initializing local variables at the top versus close to their usage...

I do not really have a preference for what happens here, I'm happy to
rework the patch based on maintainer feedback, thanks for the review!

Cheers,
Nathan

> > assigns a new value to cb_mode, which also takes care of the warning.
> >
> > Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1034
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/phy/intel/phy-intel-combo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
> > index c2a35be4cdfb..4bc1276ecf23 100644
> > --- a/drivers/phy/intel/phy-intel-combo.c
> > +++ b/drivers/phy/intel/phy-intel-combo.c
> > @@ -199,9 +199,9 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
> >
> >  static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
> >  {
> > -       enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
> >         enum aggregated_mode aggr = cbphy->aggr_mode;
> >         struct device *dev = cbphy->dev;
> > +       enum intel_combo_mode cb_mode;
> >         enum intel_phy_mode mode;
> >         int ret;
> >
> >
> > base-commit: c11d28ab4a691736e30b49813fb801847bd44e83
> > --
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[Nathan Chancellor]

On Fri, May 22, 2020 at 08:50:43PM -0700, Nathan Chancellor wrote:
> Clang warns:
> 
> drivers/phy/intel/phy-intel-combo.c:202:34: warning: implicit conversion
> from enumeration type 'enum intel_phy_mode' to different enumeration
> type 'enum intel_combo_mode' [-Wenum-conversion]
>         enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
>                               ~~~~~~~   ^~~~~~~~~~~~~
> 1 warning generated.
> 
> The correct enum to use would be PCIE0_PCIE1_MODE. However, eliminating
> this assignment is a little better because the switch statement always
> assigns a new value to cb_mode, which also takes care of the warning.
> 
> Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1034
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/phy/intel/phy-intel-combo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
> index c2a35be4cdfb..4bc1276ecf23 100644
> --- a/drivers/phy/intel/phy-intel-combo.c
> +++ b/drivers/phy/intel/phy-intel-combo.c
> @@ -199,9 +199,9 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
>  
>  static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
>  {
> -	enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
>  	enum aggregated_mode aggr = cbphy->aggr_mode;
>  	struct device *dev = cbphy->dev;
> +	enum intel_combo_mode cb_mode;
>  	enum intel_phy_mode mode;
>  	int ret;
>  
> 
> base-commit: c11d28ab4a691736e30b49813fb801847bd44e83
> -- 
> 2.27.0.rc0
> 

Gentle ping for review. Nick did comment that maybe keeping the
assignment and tidying up one of the switch cases would be better but
every maintainer has their preference. This warning has slipped into
mainline so it would be nice to get it cleaned up.

Cheers,
Nathan

Re: [PATCH] phy: intel: Eliminate unnecessary assignment in intel_cbphy_set_mode

[Vinod Koul]

On 15-06-20, 17:30, Nathan Chancellor wrote:
> On Fri, May 22, 2020 at 08:50:43PM -0700, Nathan Chancellor wrote:
> > Clang warns:
> > 
> > drivers/phy/intel/phy-intel-combo.c:202:34: warning: implicit conversion
> > from enumeration type 'enum intel_phy_mode' to different enumeration
> > type 'enum intel_combo_mode' [-Wenum-conversion]
> >         enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
> >                               ~~~~~~~   ^~~~~~~~~~~~~
> > 1 warning generated.
> > 
> > The correct enum to use would be PCIE0_PCIE1_MODE. However, eliminating
> > this assignment is a little better because the switch statement always
> > assigns a new value to cb_mode, which also takes care of the warning.
> > 
> > Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1034
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/phy/intel/phy-intel-combo.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
> > index c2a35be4cdfb..4bc1276ecf23 100644
> > --- a/drivers/phy/intel/phy-intel-combo.c
> > +++ b/drivers/phy/intel/phy-intel-combo.c
> > @@ -199,9 +199,9 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
> >  
> >  static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
> >  {
> > -	enum intel_combo_mode cb_mode = PHY_PCIE_MODE;
> >  	enum aggregated_mode aggr = cbphy->aggr_mode;
> >  	struct device *dev = cbphy->dev;
> > +	enum intel_combo_mode cb_mode;
> >  	enum intel_phy_mode mode;
> >  	int ret;
> >  
> > 
> > base-commit: c11d28ab4a691736e30b49813fb801847bd44e83
> > -- 
> > 2.27.0.rc0
> > 
> 
> Gentle ping for review. Nick did comment that maybe keeping the
> assignment and tidying up one of the switch cases would be better but
> every maintainer has their preference. This warning has slipped into
> mainline so it would be nice to get it cleaned up.

Sorry for the delay, I have applied Anrd patch for this already and
should be in linux-next tomorrow

-- 
~Vinod