| Message ID | 47ec59c0-27c0-32e3-c75e-7bdfa99551c9@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg | expand |
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
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
> >> - *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
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
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)
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(-)