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 05:42:48PM +0200, Andrew Lunn wrote:
> FYI:
> 
> 	Andrew

Thanks for forwarding - Colton, please copy me directly.

> 
> ----- Forwarded message from Colton Lewis <colton.w.lewis@protonmail.com> -----
> 
> Date: Sun, 21 Jun 2020 02:23:04 +0000
> From: Colton Lewis <colton.w.lewis@protonmail.com>
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org, Colton Lewis <colton.w.lewis@protonmail.com>
> Subject: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies
> X-Spam-Status: No, score=-1.0 required=4.0 tests=BAYES_00,DKIM_SIGNED,
> 	DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,
> 	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS autolearn=no autolearn_force=no
> 	version=3.4.4
> 
> Silence documentation build warnings by correcting kernel-doc
> comments. In the case of pcs_{config,an_restart,link_up}, change the
> declaration to a normal function since these only there for
> documentation anyway.
> 
> ./include/linux/phylink.h:74: warning: Function parameter or member 'poll_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:74: warning: Function parameter or member 'get_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:336: warning: Function parameter or member 'pcs_config' not described in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'config' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'mode' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'interface' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'advertising' description in 'int'
> ./include/linux/phylink.h:345: warning: Function parameter or member 'pcs_an_restart' not described in 'void'
> ./include/linux/phylink.h:345: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Function parameter or member 'pcs_link_up' not described in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'mode' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'interface' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'speed' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'duplex' description in 'void'

Not all of those are valid complaints against the code.

> Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
> ---
>  include/linux/phylink.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index cc5b452a184e..cb3230590a1f 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -62,6 +62,8 @@ enum phylink_op_type {
>   * @dev: a pointer to a struct device associated with the MAC
>   * @type: operation type of PHYLINK instance
>   * @pcs_poll: MAC PCS cannot provide link change interrupt
> + * @poll_fixed_state: poll link state with @get_fixed_state
> + * @get_fixed_state: read link state into struct phylink_link_state

Ack for this change.

>   */
>  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.

>  
>  /**
> @@ -341,7 +343,7 @@ int (*pcs_config)(struct phylink_config *config, unsigned int mode,
>   * When PCS ops are present, this overrides mac_an_restart() in &struct
>   * phylink_mac_ops.
>   */
> -void (*pcs_an_restart)(struct phylink_config *config);
> +void *pcs_an_restart(struct phylink_config *config);
>  
>  /**
>   * pcs_link_up() - program the PCS for the resolved link configuration
> @@ -356,7 +358,7 @@ void (*pcs_an_restart)(struct phylink_config *config);
>   * mode without in-band AN needs to be manually configured for the link
>   * and duplex setting. Otherwise, this should be a no-op.
>   */
> -void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
> +void *pcs_link_up(struct phylink_config *config, unsigned int mode,
>  		    phy_interface_t interface, int speed, int duplex);
>  #endif
>  
> -- 
> 2.26.2
> 
> 
> 
> ----- End forwarded message -----
> 

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