Re: [PATCH RFC net-next] net: pcs: add helper module for standalone drivers

[Simon Horman]

On Thu, Jul 25, 2024 at 01:44:49PM +0100, Daniel Golle wrote:
> Implement helper module for standalone PCS drivers which allows
> standaline PCS drivers to register and users to get instances of
> 'struct phylink_pcs' using device tree nodes.
> 
> At this point only a single instance for each device tree node is
> supported, once we got devices providing more than one PCS we can
> extend it and introduce an xlate function as well as '#pcs-cells',
> similar to how this is done by the PHY framework.
> 
> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> ---
> This is meant to provide the infrastructure suggested by
> Russell King in an earlier review. It just took me a long while to
> find the time to implement this.
> Users are going to be the standalone PCS drivers for 8/10 LynxI as
> well as 64/66 USXGMII PCS found on MediaTek MT7988 SoC.
> See also https://patchwork.kernel.org/comment/25636726/
> 
> The full tree where this is being used can be found at
> 
> https://github.com/dangowrt/linux/commits/mt7988-for-next/

Hi Daniel,

I realise this is an RFC, but I'm guessing a user will need to be submitted
for this to progress into net-next.

...

> +++ b/drivers/net/pcs/pcs-standalone.c

...

> +static struct pcs_standalone *of_pcs_locate(const struct device_node *_np, u32 index)

nit: This could trivially be line wrapped to 80 columns wide

> +{
> +	struct device_node *np;
> +	struct pcs_standalone *iter, *pcssa = NULL;

nit: Reverse xmas tree

...

> +struct phylink_pcs *devm_of_pcs_get(struct device *dev,
> +				    const struct device_node *np,
> +				    unsigned int index)
> +{
> +	struct pcs_standalone *pcssa;
> +
> +	pcssa = of_pcs_locate(np ?: dev->of_node, index);
> +	if (IS_ERR_OR_NULL(pcssa))
> +		return ERR_PTR(PTR_ERR(pcssa));

nit: Perhaps ERR_CAST() ?

> +
> +	device_link_add(dev, pcssa->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> +	return pcssa->pcs;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_pcs_get);
> +
> +MODULE_DESCRIPTION("Helper for standalone PCS drivers");
> +MODULE_AUTHOR("Daniel Golle <daniel@xxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/pcs/pcs-standalone.h b/include/linux/pcs/pcs-standalone.h

Please consider adding this file to MAINTAINERS.

...

> +static inline int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs);
> +	return -EOPNOTSUPP;
> +}

The above does not compile.

...