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!
