diff mbox series

[net-next,2/4] net: phy: dp83869: Set opmode from straps

Message ID 20200519141813.28167-3-dmurphy@ti.com
State Changes Requested
Delegated to: David Miller
Headers show
Series DP83869 Enhancements | expand

Commit Message

Dan Murphy May 19, 2020, 2:18 p.m. UTC 
If the op-mode for the device is not set in the device tree then set
the strapped op-mode and store it for later configuration.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski May 19, 2020, 4:58 p.m. UTC | #1 
On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> If the op-mode for the device is not set in the device tree then set
> the strapped op-mode and store it for later configuration.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  171 |  if (val < 0)
      |          ^
kernel test robot May 19, 2020, 5:19 p.m. UTC | #2 
Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^

vim +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dan Murphy May 19, 2020, 5:40 p.m. UTC | #3 
Jakub

On 5/19/20 11:58 AM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>> If the op-mode for the device is not set in the device tree then set
>> the strapped op-mode and store it for later configuration.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>    171 |  if (val < 0)
>        |          ^

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

I did not get that warning.  But I am using 9.2-gcc.

What compiler are you using?

Dan
Dan Murphy May 19, 2020, 5:40 p.m. UTC | #4 
kbuild

On 5/19/20 12:19 PM, kbuild test robot wrote:
> Hi Dan,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on net-next/master]
> [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gcc (GCC) 9.3.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 171 |  if (val < 0)

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

Dan
Dan Murphy May 19, 2020, 5:56 p.m. UTC | #5 
Jakub

On 5/19/20 12:40 PM, Dan Murphy wrote:
> Jakub
>
> On 5/19/20 11:58 AM, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always 
>> false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
>
> This looks to be a false positive.
>
> phy_read_mmd will return an errno or a value from 0->15
>
> So if errno is returned then this will be true.
>
> Unless I have to do IS_ERR.
>
> I did not get that warning.  But I am using 9.2-gcc.
>
> What compiler are you using?
>
I see what the issue is val needs to be an int not a u16

I will fix it

Dan


> Dan
>
Andrew Lunn May 19, 2020, 6:29 p.m. UTC | #6 
On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> > If the op-mode for the device is not set in the device tree then set
> > the strapped op-mode and store it for later configuration.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>   171 |  if (val < 0)
>       |          ^

Hi Jakub

This happens a lot with PHY drivers. The register being read is a u16,
so that is what people use.

Is this now a standard GCC warning? Or have you turned on extra
checking?

	Andrew
Dan Murphy May 19, 2020, 6:41 p.m. UTC | #7 
Andrew

On 5/19/20 1:29 PM, Andrew Lunn wrote:
> On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
> Hi Jakub
>
> This happens a lot with PHY drivers. The register being read is a u16,
> so that is what people use.

Yes this is what happened but phy_read_mmd returns an int so the 
declaration of val should be an int.

I will update that in v2


> Is this now a standard GCC warning? Or have you turned on extra
> checking?
I still was not able to reproduce this warning with gcc-9.2.  I would 
like to know the same


Dan
Jakub Kicinski May 19, 2020, 6:48 p.m. UTC | #8 
On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > Is this now a standard GCC warning? Or have you turned on extra
> > checking?  
> I still was not able to reproduce this warning with gcc-9.2.  I would 
> like to know the same

W=1 + gcc-10 here, also curious to know which one of the two makes 
the difference :)
Dan Murphy May 19, 2020, 6:59 p.m. UTC | #9 
Jakub

On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
>>> Is this now a standard GCC warning? Or have you turned on extra
>>> checking?
>> I still was not able to reproduce this warning with gcc-9.2.  I would
>> like to know the same
> W=1 + gcc-10 here, also curious to know which one of the two makes
> the difference :)

W=1 made the difference I got the warning with gcc-9.2

Dan
Andrew Lunn May 19, 2020, 7:03 p.m. UTC | #10 
On Tue, May 19, 2020 at 01:59:16PM -0500, Dan Murphy wrote:
> Jakub
> 
> On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> > On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > > > Is this now a standard GCC warning? Or have you turned on extra
> > > > checking?
> > > I still was not able to reproduce this warning with gcc-9.2.  I would
> > > like to know the same
> > W=1 + gcc-10 here, also curious to know which one of the two makes
> > the difference :)
> 
> W=1 made the difference I got the warning with gcc-9.2

I wonder if we should turn on this specific warning by default in
drivers/net/phy? I keep making the same mistake, and it would be nice
if GCC actually told me.

   Andrew
kernel test robot May 19, 2020, 7:16 p.m. UTC | #11 
Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sparc64-randconfig-s001-20200519 (attached as .config)
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-193-gb8fad4bc-dirty
        # save the attached .config to linux build tree
        make C=1 ARCH=sparc64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from include/linux/build_bug.h:5,
from include/linux/bits.h:23,
from include/linux/bitops.h:5,
from include/linux/bitmap.h:8,
from include/linux/ethtool.h:16,
from drivers/net/phy/dp83869.c:6:
drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                    ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                             ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
69 |  (cond) ?              |   ^~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
|                            ^~~~~~~~~~~~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~

vim +/if +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Philip Li May 19, 2020, 11:54 p.m. UTC | #12 
On Tue, May 19, 2020 at 12:40:37PM -0500, Dan Murphy wrote:
> kbuild
> 
> On 5/19/20 12:19 PM, kbuild test robot wrote:
> > Hi Dan,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on net-next/master]
> > [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> > config: sh-allmodconfig (attached as .config)
> > compiler: sh4-linux-gcc (GCC) 9.3.0
> > reproduce:
> >          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >          chmod +x ~/bin/make.cross
> >          # save the attached .config to linux build tree
> >          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>, old ones prefixed by <<):
> > 
> > drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
> > > > drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> > 171 |  if (val < 0)
> 
> This looks to be a false positive.
> 
> phy_read_mmd will return an errno or a value from 0->15
thanks, here because val is defined as "u16 val", the comparison
to < 0 can not work as expected. Any err returned from phy_read_mmd,
which is int, will be converted to u16.

> 
> So if errno is returned then this will be true.
> 
> Unless I have to do IS_ERR.
> 
> Dan
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 073a0f7754a5..64fa2d911074 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -65,6 +65,7 @@ 
 #define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
 
 /* STRAP_STS1 bits */
+#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
 #define DP83869_STRAP_STS1_RESERVED		BIT(11)
 #define DP83869_STRAP_MIRROR_ENABLED           BIT(12)
 
@@ -161,6 +162,20 @@  static int dp83869_config_port_mirroring(struct phy_device *phydev)
 					  DP83869_CFG3_PORT_MIRROR_EN);
 }
 
+static int dp83869_set_strapped_mode(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	u16 val;
+
+	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+	if (val < 0)
+		return val;
+
+	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
+
+	return 0;
+}
+
 #ifdef CONFIG_OF_MDIO
 static int dp83869_of_init(struct phy_device *phydev)
 {
@@ -185,6 +200,10 @@  static int dp83869_of_init(struct phy_device *phydev)
 		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
 		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
 			return -EINVAL;
+	} else {
+		ret = dp83869_set_strapped_mode(phydev);
+		if (ret)
+			return ret;
 	}
 
 	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
@@ -218,7 +237,7 @@  static int dp83869_of_init(struct phy_device *phydev)
 #else
 static int dp83869_of_init(struct phy_device *phydev)
 {
-	return 0;
+	return dp83869_set_strapped_mode(phydev);
 }
 #endif /* CONFIG_OF_MDIO */