Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Sun, Jun 21, 2020 at 11:02:30PM +0000, Colton Lewis wrote:
> On Sunday, June 21, 2020 10:53:45 AM CDT Russell King - ARM Linux admin wrote:
> > > ---
> > >   */
> > >  struct phylink_config {
> > >  	struct device *dev;
> > > @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
> > >   *
> > >   * For most 10GBASE-R, there is no advertisement.
> > >   */
> > > -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> > > +int *pcs_config(struct phylink_config *config, unsigned int mode,
> > >  		  phy_interface_t interface, const unsigned long *advertising);
> > 
> > *Definitely* a NAK on this and two changes below.  You're changing the
> > function signature to be incorrect.  If the documentation can't parse
> > a legitimate C function pointer declaration and allow it to be
> > documented, then that's a problem with the documentation's parsing of
> > C code, rather than a problem with the C code itself.
> 
> I realize this changes the signature, but this declaration is not compiled. It is under an #if 0 with a comment stating it exists for kernel-doc purposes only. The *real* function pointer declaration exists in struct phylink_pcs_ops.
> 
> Given the declaration is there exclusively for documentation, it makes sense to change it so the documentation system can parse it.

My objection is that you are changing the return type from (e.g.)
"int" to "int *", which will then end up in the documentation as
such, and the documentation will, therefore, be incorrect.

I have subsequently realised that I didn't follow my own pattern
for documenting phylink_mac_ops - a correct solution would be to
drop the parens _and_ the "*" preceding the function name, so:

int pcs_config(struct phylink_config *config, unsigned int mode,
...

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!