Re: [PATCH 1/1] netdev/phy: add MDIO bus multiplexer driven by a regmap

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Jan 30, 2019 at 11:22:00AM +0000, Pankaj Bansal wrote:
> Add support for an MDIO bus multiplexer controlled by a regmap
> device, like an FPGA.
> 
> Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
> attached to the i2c bus.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  drivers/net/phy/Makefile          |   2 +-
>  drivers/net/phy/mdio-mux-regmap.c | 170 ++++++++++++++++++++++++++++
>  include/linux/mdio-mux.h          |  20 ++++
>  3 files changed, 191 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index f41b14115fde..16145973a42f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -25,7 +25,7 @@ obj-$(CONFIG_PHYLIB)		+= libphy.o
>  obj-$(CONFIG_MDIO_BCM_IPROC)	+= mdio-bcm-iproc.o
>  obj-$(CONFIG_MDIO_BCM_UNIMAC)	+= mdio-bcm-unimac.o
>  obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
> -obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
> +obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o mdio-mux-regmap.o

Please add a new KCONFIG symbol for it. And think about the
depend/select statement, since you need regmap.

> +/* MDIO multiplexing switch function
> + *
> + * This function is called by the mdio-mux layer when it thinks the mdio bus
> + * multiplexer needs to switch.
> + *
> + * 'current_child' is the current value of the mux register (masked via
> + * s->mask).
> + *
> + * 'desired_child' is the value of the 'reg' property of the target child MDIO
> + * node.
> + *
> + * The first time this function is called, current_child == -1.
> + *
> + * If current_child == desired_child, then the mux is already set to the
> + * correct bus.
> + */

Please use kerneldoc formatting for this function documentation.

> +int mdio_mux_regmap_init(struct device *dev,
> +			 struct device_node *mux_node,
> +			 void **data)
> +{

> +	/* Verify that the 'reg' property of each child MDIO bus does not
> +	 * set any bits outside of the 'mask'.
> +	 */
> +	for_each_available_child_of_node(mux_node, child) {
> +		ret = of_property_read_u32(child, "reg", &val);
> +		if (ret) {
> +			dev_err(dev, "mdio-mux child node %pOF is missing a 'reg' property\n", child);

You can probably remove "mdio-mux child node " making the line < 80,
but still retain the meaning. The child node name should be sufficient
to identify it.


> +			of_node_put(child);
> +			return -ENODEV;
> +		}
> +		if (val & ~s->mask) {
> +			dev_err(dev, "mdio-mux child node %pOF has a 'reg' value with unmasked bits\n", child);

Same here.

     Andrew