| Message ID | 20231128133630.7829-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/3] net: phy: extend PHY package API to support multiple global address | expand |
On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote: > +/** > + * phy_package_write_mmd - Convenience function for writing a register > + * on an MMD on a given PHY using the PHY package base addr, added of > + * the addr_offset value. > + * @phydev: The phy_device struct > + * @addr_offset: The offset to be added to PHY package base_addr > + * @devad: The MMD to read from > + * @regnum: The register on the MMD to read > + * @val: value to write to @regnum > + * > + * Same rules as for phy_write(); > + * > + * NOTE: It's assumed that the entire PHY package is either C22 or C45. > + */ > +/* > + * phy_package_write_mmd - Convenience function for writing a register > + * on an MMD on a given PHY using the PHY package base addr, added of > + * the addr_offset value. > + */ > +int phy_package_write_mmd(struct phy_device *phydev, > + unsigned int addr_offset, int devad, > + u32 regnum, u16 val); Hm, I see there's some precedent here already for this duplicated semi-kdoc. It seems a bit unusual. If I was looking for kdoc and found the header one I'd probably not look at the source file at all. Andrew, WDYT?
On Mon, Dec 04, 2023 at 06:17:52PM -0800, Jakub Kicinski wrote: > On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote: > > +/** > > + * phy_package_write_mmd - Convenience function for writing a register > > + * on an MMD on a given PHY using the PHY package base addr, added of > > + * the addr_offset value. > > + * @phydev: The phy_device struct > > + * @addr_offset: The offset to be added to PHY package base_addr > > + * @devad: The MMD to read from > > + * @regnum: The register on the MMD to read > > + * @val: value to write to @regnum > > + * > > + * Same rules as for phy_write(); > > + * > > + * NOTE: It's assumed that the entire PHY package is either C22 or C45. > > + */ > > > +/* > > + * phy_package_write_mmd - Convenience function for writing a register > > + * on an MMD on a given PHY using the PHY package base addr, added of > > + * the addr_offset value. > > + */ > > +int phy_package_write_mmd(struct phy_device *phydev, > > + unsigned int addr_offset, int devad, > > + u32 regnum, u16 val); > > Hm, I see there's some precedent here already for this duplicated > semi-kdoc. It seems a bit unusual. If I was looking for kdoc and > found the header one I'd probably not look at the source file at all. > > Andrew, WDYT? I tend to agree. These functions should be documented once in kdoc, and only once. I don't really care if its in the header, or the C code, but not both. Andrew
On Tue, Dec 05, 2023 at 03:37:55AM +0100, Andrew Lunn wrote: > On Mon, Dec 04, 2023 at 06:17:52PM -0800, Jakub Kicinski wrote: > > On Tue, 28 Nov 2023 14:36:30 +0100 Christian Marangi wrote: > > > +/** > > > + * phy_package_write_mmd - Convenience function for writing a register > > > + * on an MMD on a given PHY using the PHY package base addr, added of > > > + * the addr_offset value. > > > + * @phydev: The phy_device struct > > > + * @addr_offset: The offset to be added to PHY package base_addr > > > + * @devad: The MMD to read from > > > + * @regnum: The register on the MMD to read > > > + * @val: value to write to @regnum > > > + * > > > + * Same rules as for phy_write(); > > > + * > > > + * NOTE: It's assumed that the entire PHY package is either C22 or C45. > > > + */ > > > > > +/* > > > + * phy_package_write_mmd - Convenience function for writing a register > > > + * on an MMD on a given PHY using the PHY package base addr, added of > > > + * the addr_offset value. > > > + */ > > > +int phy_package_write_mmd(struct phy_device *phydev, > > > + unsigned int addr_offset, int devad, > > > + u32 regnum, u16 val); > > > > Hm, I see there's some precedent here already for this duplicated > > semi-kdoc. It seems a bit unusual. If I was looking for kdoc and > > found the header one I'd probably not look at the source file at all. > > > > Andrew, WDYT? > > I tend to agree. These functions should be documented once in kdoc, > and only once. I don't really care if its in the header, or the C > code, but not both. > Ok just to make sure, I should keep the kdoc in the .c and drop them in .h ? (or should I move the more complete kdoc in .c to .h and remove kdoc in .c?) I followed the pattern for the other API but I get they are very old code.
> > I tend to agree. These functions should be documented once in kdoc, > > and only once. I don't really care if its in the header, or the C > > code, but not both. > > > > Ok just to make sure, I should keep the kdoc in the .c and drop them in > .h ? (or should I move the more complete kdoc in .c to .h and remove > kdoc in .c?) Please put the kdoc in the header file and remove if from the .c file. Thanks Andrew
On Tue, Dec 05, 2023 at 03:54:04PM +0100, Andrew Lunn wrote: > > > I tend to agree. These functions should be documented once in kdoc, > > > and only once. I don't really care if its in the header, or the C > > > code, but not both. > > > > > > > Ok just to make sure, I should keep the kdoc in the .c and drop them in > > .h ? (or should I move the more complete kdoc in .c to .h and remove > > kdoc in .c?) > > Please put the kdoc in the header file and remove if from the .c file. phy-core.c follows the style that the kdoc is in the .c file rather than the header file. I've raised this before in other subsystems, and it's suggested that it's better to have it in the .c file. I guess the reason is that it's more obvious that the function is documented when modifying it, so there's a higher probability that the kdoc will get updated when the function is altered. I guess a question to ask is how often do people modify a function and then check the header for any documentation?
On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote: > I've raised this before in other subsystems, and it's suggested that > it's better to have it in the .c file. I guess the reason is that it's > more obvious that the function is documented when modifying it, so > there's a higher probability that the kdoc will get updated when the > function is altered. Plus I think people using IDEs (i.e. not me) may use the "jump to definition" functionality, to find the doc? TBH I thought putting kdoc in the C source was documented in the coding style, but I can't find any mention of it now.
On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote: > On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote: > > I've raised this before in other subsystems, and it's suggested that > > it's better to have it in the .c file. I guess the reason is that it's > > more obvious that the function is documented when modifying it, so > > there's a higher probability that the kdoc will get updated when the > > function is altered. > > Plus I think people using IDEs (i.e. not me) may use the "jump to > definition" functionality, to find the doc? > > TBH I thought putting kdoc in the C source was documented in the coding > style, but I can't find any mention of it now. Well, in Documentation/doc-guide/kernel-doc.rst: The function and type kernel-doc comments should be placed just before the function or type being described in order to maximise the chance that somebody changing the code will also change the documentation. That implies (but not explicitly) that it should be at the function definition site, since "changing the code" is used as an argument as I did in my previous email. Secondly, this document goes on to give an example of running scripts/kernel-doc on a .c file. Thirdly, there are seven references in this document of kernel-doc in .c files, and only one for kernel-doc in a .h file. So this suggests that "it will be in a .c file" isn't a rule (it can't be because of documenting structures!) So let's not get hung up on whether it should be in .c or .h because I think that isn't relevant. Instead, I think it's about "it should be at the definition site" - that being a structure definition or a function definition, and not at a function prototype. The only exception I can think of is the style I've used in linux/phylink.h for the _method_ definitions which look like function prototypes - that's just a work-around because one can't kernel-doc the structure-of-function-pointers and document the function parameters without jumping through that hoop, and it would be silly to document the methods in some random driver!
On 12/5/2023 8:11 AM, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote: >> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote: >>> I've raised this before in other subsystems, and it's suggested that >>> it's better to have it in the .c file. I guess the reason is that it's >>> more obvious that the function is documented when modifying it, so >>> there's a higher probability that the kdoc will get updated when the >>> function is altered. >> >> Plus I think people using IDEs (i.e. not me) may use the "jump to >> definition" functionality, to find the doc? >> >> TBH I thought putting kdoc in the C source was documented in the coding >> style, but I can't find any mention of it now. > > Well, in Documentation/doc-guide/kernel-doc.rst: > > The function and type kernel-doc comments should be placed just before > the function or type being described in order to maximise the chance > that somebody changing the code will also change the documentation. > > That implies (but not explicitly) that it should be at the function > definition site, since "changing the code" is used as an argument as > I did in my previous email. > > Secondly, this document goes on to give an example of running > scripts/kernel-doc on a .c file. > > Thirdly, there are seven references in this document of kernel-doc > in .c files, and only one for kernel-doc in a .h file. So this suggests > that "it will be in a .c file" isn't a rule (it can't be because of > documenting structures!) > > So let's not get hung up on whether it should be in .c or .h because I > think that isn't relevant. Instead, I think it's about "it should be at > the definition site" - that being a structure definition or a function > definition, and not at a function prototype. > > The only exception I can think of is the style I've used in > linux/phylink.h for the _method_ definitions which look like function > prototypes - that's just a work-around because one can't kernel-doc > the structure-of-function-pointers and document the function parameters > without jumping through that hoop, and it would be silly to document > the methods in some random driver! > The Linux Kernel philosophy of documenting functions instead of prototypes has always bothered me since I'm "old school" and am ingrained with the software engineering philosophy that you document interfaces, not implementations. This was reinforced early in my career by working on multiple projects in different programming languages using processes outlined in DOD-STD-2167A, and for some projects, especially ones written in Ada, the header files were the design and the documentation. This philosophy was further enforced when working with closed source projects (Windows, IOS, VxWorks) where all the documentation was contained in shared header files. So in my experience a function prototype IS the function definition, and the actual function is just the implementation of that definition. But that thinking obviously isn't shared by others. /jeff
On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote: > On 12/5/2023 8:11 AM, Russell King (Oracle) wrote: > > On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote: > >> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote: > >>> I've raised this before in other subsystems, and it's suggested that > >>> it's better to have it in the .c file. I guess the reason is that it's > >>> more obvious that the function is documented when modifying it, so > >>> there's a higher probability that the kdoc will get updated when the > >>> function is altered. > >> > >> Plus I think people using IDEs (i.e. not me) may use the "jump to > >> definition" functionality, to find the doc? > >> > >> TBH I thought putting kdoc in the C source was documented in the coding > >> style, but I can't find any mention of it now. > > > > Well, in Documentation/doc-guide/kernel-doc.rst: > > > > The function and type kernel-doc comments should be placed just before > > the function or type being described in order to maximise the chance > > that somebody changing the code will also change the documentation. > > > > That implies (but not explicitly) that it should be at the function > > definition site, since "changing the code" is used as an argument as > > I did in my previous email. > > > > Secondly, this document goes on to give an example of running > > scripts/kernel-doc on a .c file. > > > > Thirdly, there are seven references in this document of kernel-doc > > in .c files, and only one for kernel-doc in a .h file. So this suggests > > that "it will be in a .c file" isn't a rule (it can't be because of > > documenting structures!) > > > > So let's not get hung up on whether it should be in .c or .h because I > > think that isn't relevant. Instead, I think it's about "it should be at > > the definition site" - that being a structure definition or a function > > definition, and not at a function prototype. > > > > The only exception I can think of is the style I've used in > > linux/phylink.h for the _method_ definitions which look like function > > prototypes - that's just a work-around because one can't kernel-doc > > the structure-of-function-pointers and document the function parameters > > without jumping through that hoop, and it would be silly to document > > the methods in some random driver! > > > > The Linux Kernel philosophy of documenting functions instead of > prototypes has always bothered me since I'm "old school" and am > ingrained with the software engineering philosophy that you document > interfaces, not implementations. This was reinforced early in my career > by working on multiple projects in different programming languages using > processes outlined in DOD-STD-2167A, and for some projects, especially > ones written in Ada, the header files were the design and the documentation. > > This philosophy was further enforced when working with closed source > projects (Windows, IOS, VxWorks) where all the documentation was > contained in shared header files. > > So in my experience a function prototype IS the function definition, and > the actual function is just the implementation of that definition. > > But that thinking obviously isn't shared by others. Interestingly, the view that a function prototype is a function definition does not seem to be shared by w3school, Microsoft, IBM, and many more. If we look at the C99 standard, then 6.9.1 Function definitions gives the syntax as including a compound-statement, which is defined as requiring the curley braces and contents. Therefore, a function definition as defined by the C standard includes its body.
On 12/5/2023 10:14 AM, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote: >> So in my experience a function prototype IS the function definition, and >> the actual function is just the implementation of that definition. >> >> But that thinking obviously isn't shared by others. > > Interestingly, the view that a function prototype is a function > definition does not seem to be shared by w3school, Microsoft, IBM, > and many more. > > If we look at the C99 standard, then 6.9.1 Function definitions gives > the syntax as including a compound-statement, which is defined as > requiring the curley braces and contents. Therefore, a function > definition as defined by the C standard includes its body. > Note I was speaking in terms of functional languages in general, not C specifically. Perhaps I should have used the term "specification" instead of "definition" (which would align with the Ada terminology). Having worked with closed-source systems, especially VxWorks, for many years (where the header files contain all the documentation), it just seems strange to embed the documentation in the .c files. /jeff
> Having worked with closed-source systems, especially VxWorks, for many > years (where the header files contain all the documentation), it just > seems strange to embed the documentation in the .c files. The key words here might be closed-source. With such black boxes, you don't have access the sources. You cannot look at the source to understand how a function works. In the open source world, the comments partially function as an introduction to reading the code and understanding what it does. You are also encouraged to change the code if needed, which in the closed source world you cannot do. Given this discussion, i now think putting the documentation in the .c file makes more sense. For the generated documentation it does not matter, but for the reader of the code, having it in the .c files does seem to make sense. Andrew
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c index b729ac8b2640..b5868282def1 100644 --- a/drivers/net/phy/phy-core.c +++ b/drivers/net/phy/phy-core.c @@ -650,6 +650,142 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val) } EXPORT_SYMBOL(phy_write_mmd); +/** + * __phy_package_read_mmd - Convenience function for reading a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + * @phydev: The phy_device struct + * @addr_offset: The offset to be added to PHY package base_addr + * @devad: The MMD to read from + * @regnum: The register on the MMD to read + * + * Same rules as for __phy_read(); + * + * NOTE: It's assumed that the entire PHY package is either C22 or C45. + */ +int __phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + int addr = shared->base_addr + addr_offset; + + if (addr >= PHY_MAX_ADDR) + return -EIO; + + if (regnum > (u16)~0 || devad > 32) + return -EINVAL; + + return mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad, + regnum); +} +EXPORT_SYMBOL(__phy_package_read_mmd); + +/** + * phy_package_read_mmd - Convenience function for reading a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + * @phydev: The phy_device struct + * @addr_offset: The offset to be added to PHY package base_addr + * @devad: The MMD to read from + * @regnum: The register on the MMD to read + * + * Same rules as for phy_read(); + * + * NOTE: It's assumed that the entire PHY package is either C22 or C45. + */ +int phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + int addr = shared->base_addr + addr_offset; + int val; + + if (addr >= PHY_MAX_ADDR) + return -EIO; + + if (regnum > (u16)~0 || devad > 32) + return -EINVAL; + + phy_lock_mdio_bus(phydev); + val = mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad, + regnum); + phy_unlock_mdio_bus(phydev); + + return val; +} +EXPORT_SYMBOL(phy_package_read_mmd); + +/** + * __phy_package_write_mmd - Convenience function for writing a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + * @phydev: The phy_device struct + * @addr_offset: The offset to be added to PHY package base_addr + * @devad: The MMD to read from + * @regnum: The register on the MMD to read + * @val: value to write to @regnum + * + * Same rules as for __phy_write(); + * + * NOTE: It's assumed that the entire PHY package is either C22 or C45. + */ +int __phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + int addr = shared->base_addr + addr_offset; + + if (addr >= PHY_MAX_ADDR) + return -EIO; + + if (regnum > (u16)~0 || devad > 32) + return -EINVAL; + + return mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad, + regnum, val); +} +EXPORT_SYMBOL(__phy_package_write_mmd); + +/** + * phy_package_write_mmd - Convenience function for writing a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + * @phydev: The phy_device struct + * @addr_offset: The offset to be added to PHY package base_addr + * @devad: The MMD to read from + * @regnum: The register on the MMD to read + * @val: value to write to @regnum + * + * Same rules as for phy_write(); + * + * NOTE: It's assumed that the entire PHY package is either C22 or C45. + */ +int phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + int addr = shared->base_addr + addr_offset; + int ret; + + if (addr >= PHY_MAX_ADDR) + return -EIO; + + if (regnum > (u16)~0 || devad > 32) + return -EINVAL; + + phy_lock_mdio_bus(phydev); + ret = mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad, + regnum, val); + phy_unlock_mdio_bus(phydev); + + return ret; +} +EXPORT_SYMBOL(phy_package_write_mmd); + /** * phy_modify_changed - Function for modifying a PHY register * @phydev: the phy_device struct diff --git a/include/linux/phy.h b/include/linux/phy.h index 51702e349d83..41e0698a3685 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -2049,6 +2049,39 @@ static inline int __phy_package_write(struct phy_device *phydev, return __mdiobus_write(phydev->mdio.bus, addr, regnum, val); } +/* + * __phy_package_read_mmd - Convenience function for reading a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + */ +int __phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum); +/* + * phy_package_read_mmd - Convenience function for reading a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + */ +int phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum); +/* + * __phy_package_write_mmd - Convenience function for writing a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + */ +int __phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val); +/* + * phy_package_write_mmd - Convenience function for writing a register + * on an MMD on a given PHY using the PHY package base addr, added of + * the addr_offset value. + */ +int phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val); + static inline bool __phy_package_set_once(struct phy_device *phydev, unsigned int b) {
Some PHY in PHY package may require to read/write MMD regs to correctly configure the PHY package. Add support for these additional required function in both lock and no lock variant. It's assumed that the entire PHY package is either C22 or C45. We use C22 or C45 way of writing/reading to mmd regs based on the passed phydev whether it's C22 or C45. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- Changes v3: - Move in phy-core.c from phy.h - Base c45 from phydev Changes v2: - Rework to use newly introduced helper - Add common check for regnum and devad drivers/net/phy/phy-core.c | 136 +++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 33 +++++++++ 2 files changed, 169 insertions(+)