Re: [RFC PATCH net-next v3 5/8] net: pcs: add driver for MediaTek USXGMII PCS

[Simon Horman]

On Tue, Dec 12, 2023 at 03:47:47AM +0000, Daniel Golle wrote:
> Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting
> USXGMII, 10GBase-R and 5GBase-R interface modes.
> 
> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>

Hi Daniel,

some minor feedback from my side.

...

> diff --git a/drivers/net/pcs/pcs-mtk-usxgmii.c b/drivers/net/pcs/pcs-mtk-usxgmii.c

...

> +static int mtk_usxgmii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +				  phy_interface_t interface,
> +				  const unsigned long *advertising,
> +				  bool permit_pause_to_mac)
> +{
> +	struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
> +	unsigned int an_ctrl = 0, link_timer = 0, xfi_mode = 0, adapt_mode = 0;
> +	bool mode_changed = false;

nit: please consider arranging local variables in networking code
     in reverse xmas tree order - longest line to shortest.

     This may be useful:
     https://github.com/ecree-solarflare/xmastree

...
> diff --git a/include/linux/pcs/pcs-mtk-usxgmii.h b/include/linux/pcs/pcs-mtk-usxgmii.h
> new file mode 100644
> index 0000000000000..ef936d9c5f116
> --- /dev/null
> +++ b/include/linux/pcs/pcs-mtk-usxgmii.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_PCS_MTK_USXGMII_H
> +#define __LINUX_PCS_MTK_USXGMII_H
> +
> +#include <linux/phylink.h>
> +
> +/**
> + * mtk_usxgmii_select_pcs() - Get MediaTek PCS instance
> + * @np:		Pointer to device node indentifying a MediaTek USXGMII PCS

nit: identifying

     ./scripts/checkpatch.py --codespell is your friend here.

> + * @mode:	Ethernet PHY interface mode
> + *
> + * Return PCS identified by a device node and the PHY interface mode in use
> + *
> + * Return:	Pointer to phylink PCS instance of NULL
> + */
> +#if IS_ENABLED(CONFIG_PCS_MTK_USXGMII)
> +struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np);

nit: The kernel doc above does not match the signature
     of mtk_usxgmii_pcs_get().

     ./scripts/kernel-doc -none is helpful here.

> +void mtk_usxgmii_pcs_put(struct phylink_pcs *pcs);
> +#else
> +static inline struct phylink_pcs *mtk_usxgmii_pcs_get(struct device *dev, struct device_node *np)
> +{
> +	return NULL;
> +}
> +static inline void mtk_usxgmii_pcs_put(struct phylink_pcs *pcs) { }
> +#endif /* IS_ENABLED(CONFIG_PCS_MTK_USXGMII) */
> +
> +#endif /* __LINUX_PCS_MTK_USXGMII_H */
> -- 
> 2.43.0