[PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[Heiner Kallweit]

Bit 0 in register 1.5 doesn't represent a device but is a flag that
Clause 22 registers are present. Therefore disregard this bit when
populating the device list. If code needs this information it
should read register 1.5 directly instead of accessing the device
list. Because this bit doesn't represent a device I didn't add a
MDIO_MMD_C22PRESENT constant or similar.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 3 ++-
 include/uapi/linux/mdio.h    | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9369e1323..c2316a117 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -682,7 +682,8 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 	phy_reg = mdiobus_read(bus, addr, reg_addr);
 	if (phy_reg < 0)
 		return -EIO;
-	*devices_in_package |= (phy_reg & 0xffff);
+	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+	*devices_in_package |= (phy_reg & 0xfffe);
 
 	return 0;
 }
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 2e6e309f0..0e012b168 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -115,6 +115,7 @@
 
 /* Device present registers. */
 #define MDIO_DEVS_PRESENT(devad)	(1 << (devad))
+#define MDIO_DEVS_C22PRESENT		MDIO_DEVS_PRESENT(0)
 #define MDIO_DEVS_PMAPMD		MDIO_DEVS_PRESENT(MDIO_MMD_PMAPMD)
 #define MDIO_DEVS_WIS			MDIO_DEVS_PRESENT(MDIO_MMD_WIS)
 #define MDIO_DEVS_PCS			MDIO_DEVS_PRESENT(MDIO_MMD_PCS)
-- 
2.20.1

Re: [PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[Andrew Lunn]

On Fri, Feb 08, 2019 at 08:12:47AM +0100, Heiner Kallweit wrote:
> Bit 0 in register 1.5 doesn't represent a device but is a flag that
> Clause 22 registers are present. Therefore disregard this bit when
> populating the device list. If code needs this information it
> should read register 1.5 directly instead of accessing the device
> list. Because this bit doesn't represent a device I didn't add a
> MDIO_MMD_C22PRESENT constant or similar.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 3 ++-
>  include/uapi/linux/mdio.h    | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 9369e1323..c2316a117 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -682,7 +682,8 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
>  	phy_reg = mdiobus_read(bus, addr, reg_addr);
>  	if (phy_reg < 0)
>  		return -EIO;
> -	*devices_in_package |= (phy_reg & 0xffff);
> +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> +	*devices_in_package |= (phy_reg & 0xfffe);

Hi Heiner

Just for readability, can we use BIT(0) in there somehow?

>  
>  	return 0;
>  }
> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index 2e6e309f0..0e012b168 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -115,6 +115,7 @@
>  
>  /* Device present registers. */
>  #define MDIO_DEVS_PRESENT(devad)	(1 << (devad))
> +#define MDIO_DEVS_C22PRESENT		MDIO_DEVS_PRESENT(0)

Err. The commit message says you did not add this...

     Andrew

Re: [PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[Heiner Kallweit]

On 08.02.2019 15:02, Andrew Lunn wrote:
> On Fri, Feb 08, 2019 at 08:12:47AM +0100, Heiner Kallweit wrote:
>> Bit 0 in register 1.5 doesn't represent a device but is a flag that
>> Clause 22 registers are present. Therefore disregard this bit when
>> populating the device list. If code needs this information it
>> should read register 1.5 directly instead of accessing the device
>> list. Because this bit doesn't represent a device I didn't add a
>> MDIO_MMD_C22PRESENT constant or similar.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 3 ++-
>>  include/uapi/linux/mdio.h    | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 9369e1323..c2316a117 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -682,7 +682,8 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
>>  	phy_reg = mdiobus_read(bus, addr, reg_addr);
>>  	if (phy_reg < 0)
>>  		return -EIO;
>> -	*devices_in_package |= (phy_reg & 0xffff);
>> +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
>> +	*devices_in_package |= (phy_reg & 0xfffe);
> 
> Hi Heiner
> 
> Just for readability, can we use BIT(0) in there somehow?
> 
You think 0xfffe together with the comment is still not clear enough?
But sure, I can make it more explicit.

>>  
>>  	return 0;
>>  }
>> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
>> index 2e6e309f0..0e012b168 100644
>> --- a/include/uapi/linux/mdio.h
>> +++ b/include/uapi/linux/mdio.h
>> @@ -115,6 +115,7 @@
>>  
>>  /* Device present registers. */
>>  #define MDIO_DEVS_PRESENT(devad)	(1 << (devad))
>> +#define MDIO_DEVS_C22PRESENT		MDIO_DEVS_PRESENT(0)
> 
> Err. The commit message says you did not add this...
> 
Maybe I'm not clear enough in the commit message. Typically we have two
constants for a device:

MDIO_MMD_XXX (for the device)
MDIO_DEVS_XXX (for the bit of the device in the device list bitmap)

For the C22PRESENT flag I don't define the first one (because it's
not a device) but the second one (because it uses a bit in the device
list bitmap).

>      Andrew
> 
Heiner

Re: [PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[Andrew Lunn]

> >> -	*devices_in_package |= (phy_reg & 0xffff);
> >> +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> >> +	*devices_in_package |= (phy_reg & 0xfffe);
> > 
> > Hi Heiner
> > 
> > Just for readability, can we use BIT(0) in there somehow?
> > 
> You think 0xfffe together with the comment is still not clear enough?

Hi Heiner

It is more i was wondering why the 0xffff was there in the first
place. PHY registers are 16 bits. Is this because of a compiler
warning? If the use of 0xffff is not obvious, why would 0xfffe be any
better.

> >>  /* Device present registers. */
> >>  #define MDIO_DEVS_PRESENT(devad)	(1 << (devad))
> >> +#define MDIO_DEVS_C22PRESENT		MDIO_DEVS_PRESENT(0)
> > 
> > Err. The commit message says you did not add this...
> > 
> Maybe I'm not clear enough in the commit message. Typically we have two
> constants for a device:
> 
> MDIO_MMD_XXX (for the device)
> MDIO_DEVS_XXX (for the bit of the device in the device list bitmap)
> 
> For the C22PRESENT flag I don't define the first one (because it's
> not a device) but the second one (because it uses a bit in the device
> list bitmap).

This would be better as a separate patch. It is not used here, and the
explanation can then be made clearer.

	    Andrew

Re: [PATCH net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg

[Heiner Kallweit]

On 08.02.2019 19:27, Andrew Lunn wrote:
>>>> -	*devices_in_package |= (phy_reg & 0xffff);
>>>> +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
>>>> +	*devices_in_package |= (phy_reg & 0xfffe);
>>>
>>> Hi Heiner
>>>
>>> Just for readability, can we use BIT(0) in there somehow?
>>>
>> You think 0xfffe together with the comment is still not clear enough?
> 
> Hi Heiner
> 
> It is more i was wondering why the 0xffff was there in the first
> place. PHY registers are 16 bits. Is this because of a compiler
> warning? If the use of 0xffff is not obvious, why would 0xfffe be any
> better.
> 
I think there are more places where this masking is used, most likely
to make clearer that we care about the lower 16 bits of the int only.
And I also wondered when seeing such code whether it's technically
needed.

>>>>  /* Device present registers. */
>>>>  #define MDIO_DEVS_PRESENT(devad)	(1 << (devad))
>>>> +#define MDIO_DEVS_C22PRESENT		MDIO_DEVS_PRESENT(0)
>>>
>>> Err. The commit message says you did not add this...
>>>
>> Maybe I'm not clear enough in the commit message. Typically we have two
>> constants for a device:
>>
>> MDIO_MMD_XXX (for the device)
>> MDIO_DEVS_XXX (for the bit of the device in the device list bitmap)
>>
>> For the C22PRESENT flag I don't define the first one (because it's
>> not a device) but the second one (because it uses a bit in the device
>> list bitmap).
> 
> This would be better as a separate patch. It is not used here, and the
> explanation can then be made clearer.
> 
OK. Definition of this constant is more meant as a favor to developers
who may want to check this flag in the future.

> 	    Andrew
> 
Heiner