netdev - RE: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a regmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <HE1PR0402MB33238F8C82D7D19D7E321F54F1F80@HE1PR0402MB3323.eurprd04.prod.outlook.com>
Date:   Thu, 18 Oct 2018 04:35:35 +0000
From:   Pankaj Bansal <pankaj.bansal@....com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a
 regmap



> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@...il.com]
> Sent: Sunday, October 7, 2018 11:39 PM
> To: Pankaj Bansal <pankaj.bansal@....com>; Andrew Lunn <andrew@...n.ch>
> Cc: netdev@...r.kernel.org
> Subject: Re: [PATCH v2 2/2] netdev/phy: add MDIO bus multiplexer driven by a
> regmap
> 
> 
> 
> On 10/07/18 11:24, 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@....com>
> > ---
> >
> > Notes:
> >     V2:
> >      - replaced be32_to_cpup with of_property_read_u32
> >      - incorporated Andrew's comments
> >
> >  drivers/net/phy/Kconfig           |  13 +++
> >  drivers/net/phy/Makefile          |   1 +
> >  drivers/net/phy/mdio-mux-regmap.c | 171 ++++++++++++++++++++++++++++
> >  3 files changed, 185 insertions(+)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > 82070792edbb..d1ac9e70cbb2 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -87,6 +87,19 @@ config MDIO_BUS_MUX_MMIOREG
> >
> >  	  Currently, only 8/16/32 bits registers are supported.
> >
> > +config MDIO_BUS_MUX_REGMAP
> > +	tristate "REGMAP controlled MDIO bus multiplexers"
> > +	depends on OF_MDIO && REGMAP
> > +	select MDIO_BUS_MUX
> > +	help
> > +	  This module provides a driver for MDIO bus multiplexers that
> > +	  are controlled via a regmap device, like an FPGA connected to i2c.
> > +	  The multiplexer connects one of several child MDIO busses to a
> > +	  parent bus.Child bus selection is under the control of one of
> > +	  the FPGA's registers.
> > +
> > +	  Currently, only upto 32 bits registers are supported.
> > +
> >  config MDIO_CAVIUM
> >  	tristate
> >
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 5805c0b7d60e..33053f9f320d 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_MDIO_BUS_MUX)	+= mdio-mux.o
> >  obj-$(CONFIG_MDIO_BUS_MUX_BCM_IPROC)	+= mdio-mux-bcm-iproc.o
> >  obj-$(CONFIG_MDIO_BUS_MUX_GPIO)	+= mdio-mux-gpio.o
> >  obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
> > +obj-$(CONFIG_MDIO_BUS_MUX_REGMAP) += mdio-mux-regmap.o
> >  obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
> >  obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
> >  obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
> > diff --git a/drivers/net/phy/mdio-mux-regmap.c
> > b/drivers/net/phy/mdio-mux-regmap.c
> > new file mode 100644
> > index 000000000000..6068d05a728a
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-mux-regmap.c
> > @@ -0,0 +1,171 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/* Simple regmap based MDIO MUX driver
> > + *
> > + * Copyright 2018 NXP
> > + *
> > + * Based on mdio-mux-mmioreg.c by Timur Tabi
> > + *
> > + * Author:
> > + *     Pankaj Bansal <pankaj.bansal@....com>
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/device.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/module.h>
> > +#include <linux/phy.h>
> > +#include <linux/mdio-mux.h>
> > +#include <linux/regmap.h>
> > +
> > +struct mdio_mux_regmap_state {
> > +	void		*mux_handle;
> > +	struct regmap	*regmap;
> > +	u32		mux_reg;
> > +	u32		mask;
> > +};
> > +
> > +/* 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.
> > + */
> > +static int mdio_mux_regmap_switch_fn(int current_child, int desired_child,
> > +				     void *data)
> > +{
> > +	struct mdio_mux_regmap_state *s = data;
> > +	bool change;
> > +	int ret;
> > +
> > +	ret = regmap_update_bits_check(s->regmap,
> > +				       s->mux_reg,
> > +				       s->mask,
> > +				       desired_child,
> > +				       &change);
> > +
> > +	if (ret)
> > +		return ret;
> > +	if (change)
> > +		pr_debug("%s %d -> %d\n", __func__, current_child,
> > +			 desired_child);
> 
> If you add a struct platform_device or struct device reference to struct
> mdio_mux_regmap_state, the you can use dev_dbg() here with the correct
> device, which would be helpful if you are debugging problems, and there are
> more than once instance of them in the system.

Ok, I will add platform_device reference to struct.

> 
> > +	return ret;
> > +}
> > +
> > +static int mdio_mux_regmap_probe(struct platform_device *pdev) {
> > +	struct device_node *np2, *np = pdev->dev.of_node;
> 
> How about naming "np2", "child" instead?

Ok. I will rename this variable

> 
> Everything else looks fine to me, thanks!
> --
> Florian