| Message ID | 20200420185310.6630-2-dmurphy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/3] net: phy: Add a generic phy file for TI generic PHYs | expand |
On 20. 04. 20 20:53, Dan Murphy wrote: > Add phy_set/clear_bit helper routines so that ported drivers from the > kernel can use these functions. > > Signed-off-by: Dan Murphy <dmurphy at ti.com> > --- > include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/include/phy.h b/include/phy.h > index b5de14cbfc29..050c989fa537 100644 > --- a/include/phy.h > +++ b/include/phy.h > @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad, > return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val); > } > kernel-doc description would be good. > +static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad, > + u32 regnum, u16 val) > +{ > + int value; > + int ret; nit: on one line should be better. > + > + value = phy_read_mmd(phydev, devad, regnum); > + if (value < 0) > + return value; > + > + value |= val; > + > + ret = phy_write_mmd(phydev, devad, regnum, value); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static inline int phy_clear_bits_mmd(struct phy_device *phydev, int devad, > + u32 regnum, u16 val) > +{ > + int value; > + int ret; ditto. > + > + value = phy_read_mmd(phydev, devad, regnum); > + if (value < 0) > + return value; > + > + value &= ~val; > + > + ret = phy_write_mmd(phydev, devad, regnum, value); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > #ifdef CONFIG_PHYLIB_10G > extern struct phy_driver gen10g_driver; > Thanks, Michal
Michal Thanks for the review On 4/21/20 3:04 AM, Michal Simek wrote: > On 20. 04. 20 20:53, Dan Murphy wrote: >> Add phy_set/clear_bit helper routines so that ported drivers from the >> kernel can use these functions. >> >> Signed-off-by: Dan Murphy <dmurphy at ti.com> >> --- >> include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/include/phy.h b/include/phy.h >> index b5de14cbfc29..050c989fa537 100644 >> --- a/include/phy.h >> +++ b/include/phy.h >> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad, >> return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val); >> } >> > kernel-doc description would be good. Just following the files coding practice.? No other APIs have kernel-doc. Not that I won't add them but it is worth pointing this out. > >> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad, >> + u32 regnum, u16 val) >> +{ >> + int value; >> + int ret; > nit: on one line should be better. I was under the impression that when code is side ported to uBoot it should be as close to the origin as possible for bug fixes to be easily understood and applied. Please correct my understanding if I had the wrong impression. Not that this specific nit is going to make a difference but it applies to other comments made in the other patches. Dan
On 21. 04. 20 13:43, Dan Murphy wrote: > Michal > > Thanks for the review > > On 4/21/20 3:04 AM, Michal Simek wrote: >> On 20. 04. 20 20:53, Dan Murphy wrote: >>> Add phy_set/clear_bit helper routines so that ported drivers from the >>> kernel can use these functions. >>> >>> Signed-off-by: Dan Murphy <dmurphy at ti.com> >>> --- >>> ? include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++ >>> ? 1 file changed, 38 insertions(+) >>> >>> diff --git a/include/phy.h b/include/phy.h >>> index b5de14cbfc29..050c989fa537 100644 >>> --- a/include/phy.h >>> +++ b/include/phy.h >>> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct >>> phy_device *phydev, int devad, >>> ????? return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val); >>> ? } >>> ? >> kernel-doc description would be good. > > Just following the files coding practice.? No other APIs have kernel-doc. > > Not that I won't add them but it is worth pointing this out. Not sure if this straight copy from Linux but from phy_init() down all functions used that. >> >>> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int >>> devad, >>> +?????????????????? u32 regnum, u16 val) >>> +{ >>> +??? int value; >>> +??? int ret; >> nit: on one line should be better. > > I was under the impression that when code is side ported to uBoot it > should be as close to the origin as possible for bug fixes to be easily > understood and applied. > > Please correct my understanding if I had the wrong impression. > > Not that this specific nit is going to make a difference but it applies > to other comments made in the other patches. then would be good to fix it in origin source. :-) Even this fix is quite trivial. Thanks, Michal
Michal On 4/21/20 6:52 AM, Michal Simek wrote: > On 21. 04. 20 13:43, Dan Murphy wrote: >> Michal >> >> Thanks for the review >> >> On 4/21/20 3:04 AM, Michal Simek wrote: >>> On 20. 04. 20 20:53, Dan Murphy wrote: >>>> Add phy_set/clear_bit helper routines so that ported drivers from the >>>> kernel can use these functions. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy at ti.com> >>>> --- >>>> ? include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++ >>>> ? 1 file changed, 38 insertions(+) >>>> >>>> diff --git a/include/phy.h b/include/phy.h >>>> index b5de14cbfc29..050c989fa537 100644 >>>> --- a/include/phy.h >>>> +++ b/include/phy.h >>>> @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct >>>> phy_device *phydev, int devad, >>>> ????? return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val); >>>> ? } >>>> >>> kernel-doc description would be good. >> Just following the files coding practice.? No other APIs have kernel-doc. >> >> Not that I won't add them but it is worth pointing this out. > Not sure if this straight copy from Linux but from phy_init() down all > functions used that. Yes they do but the inline functions don't. I will actually go one better and add the k-doc to the other in-line functions above for consistency. >>>> +static inline int phy_set_bits_mmd(struct phy_device *phydev, int >>>> devad, >>>> +?????????????????? u32 regnum, u16 val) >>>> +{ >>>> +??? int value; >>>> +??? int ret; >>> nit: on one line should be better. >> I was under the impression that when code is side ported to uBoot it >> should be as close to the origin as possible for bug fixes to be easily >> understood and applied. >> >> Please correct my understanding if I had the wrong impression. >> >> Not that this specific nit is going to make a difference but it applies >> to other comments made in the other patches. > then would be good to fix it in origin source. :-) Even this fix is > quite trivial. I am currently working on bug fixing the upstream DP83869 driver so I can add some of the fixes.? I will throw your Reported-by in the commit message if you want. Dan
diff --git a/include/phy.h b/include/phy.h index b5de14cbfc29..050c989fa537 100644 --- a/include/phy.h +++ b/include/phy.h @@ -257,6 +257,44 @@ static inline int phy_write_mmd(struct phy_device *phydev, int devad, return phy_write(phydev, MDIO_DEVAD_NONE, MII_MMD_DATA, val); } +static inline int phy_set_bits_mmd(struct phy_device *phydev, int devad, + u32 regnum, u16 val) +{ + int value; + int ret; + + value = phy_read_mmd(phydev, devad, regnum); + if (value < 0) + return value; + + value |= val; + + ret = phy_write_mmd(phydev, devad, regnum, value); + if (ret < 0) + return ret; + + return 0; +} + +static inline int phy_clear_bits_mmd(struct phy_device *phydev, int devad, + u32 regnum, u16 val) +{ + int value; + int ret; + + value = phy_read_mmd(phydev, devad, regnum); + if (value < 0) + return value; + + value &= ~val; + + ret = phy_write_mmd(phydev, devad, regnum, value); + if (ret < 0) + return ret; + + return 0; +} + #ifdef CONFIG_PHYLIB_10G extern struct phy_driver gen10g_driver;
Add phy_set/clear_bit helper routines so that ported drivers from the kernel can use these functions. Signed-off-by: Dan Murphy <dmurphy at ti.com> --- include/phy.h | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)