Hi Andrew
> -----Original Message-----
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Friday, October 5, 2018 9:22 AM
> To: Pankaj Bansal <pankaj.ban...@nxp.com>
> Cc: Florian Fainelli <f.faine...@gmail.com>; netdev@vger.kernel.org;
> Alexandru Marginean <alexandru.margin...@nxp.com>
> Subject: Re: [PATCH 2/2] netdev/phy: add MDIO bus multiplexer driven by a
> regmap
>
> On Fri, Oct 05, 2018 at 01:59:26PM +0530, 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.ban...@nxp.com>
> > ---
> > drivers/net/phy/Kconfig | 13 ++
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/mdio-mux-regmap.c | 177
> ++++++++++++++++++++++++++++
> > 3 files changed, 191 insertions(+)
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > 82070792edbb..bb894ede7464 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 && HAS_IOMEM
>
> Hi Pankaj
>
> Doesn't it also depend on REGMAP?
I will add
>
> > + select MDIO_BUS_MUX
> > + help
> > + This module provides a driver for MDIO bus multiplexers that
> > + are controlled via a regmap, like an FPGA.
> > + 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..b0882273fd47
> > --- /dev/null
> > +++ b/drivers/net/phy/mdio-mux-regmap.c
> > @@ -0,0 +1,177 @@
> > +// 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.ban...@nxp.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 = 0;
> > +
>
> No need to initialise ret.
Ok
>
> > + ret = regmap_update_bits_check(s->regmap,
> > + s->mux_reg,
> > + s->mask,
> > + desired_child,
> > + &change);
>
> When getting the mask from DT, you use be32_to_cpup().
> When testing the reg value against the mask, you use be32_to_cpup().
> Here you do not use be32_to_cpup()?
Can you please tell me for which variable you mean I should use be32_to_cpup?
I use be32_to_cpup when reading device tree entries.
After being read, their values are stored in structure. After that no need to
do be32_to_cpup
>
> > +
> > + if (ret)
> > + goto out;
>
> Just do the return here.
Ok
>
> > + if (change)
> > + pr_debug("%s %d -> %d\n", __func__, current_child,
> > + desired_child);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int mdio_mux_regmap_probe(struct platform_device *pdev) {
> > + struct device_node *np2, *np = pdev->dev.of_node;
> > + struct mdio_mux_regmap_state *s;
> > + const __be32 *iprop;
> > + int len, ret;
> > + u32 val;
> > +
> > + dev_dbg(&pdev->dev, "probing node %pOF\n", np);
> > +
> > + s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
> > + if (!s)
> > + return -ENOMEM;
> > +
> > + s->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > + if (IS_ERR(s->regmap)) {
> > + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > + ret = PTR_ERR(s->regmap);
> > + return ret;
>
> return PTR_ERR(s->regmap);
Ok
>
> > + }
> > +
> > + iprop = of_get_property(np, "reg", &len);
> > + if (!iprop || len != sizeof(u32)) {
> > + dev_err(&pdev->dev, "missing or invalid reg property\n");
> > + return -ENODEV;
> > + }
> > + s->mux_reg = (u32)be32_to_cpup(iprop);
> > +
> > + /* Test Register read write */
> > + ret = regmap_read(s->regmap, s->mux_reg, &val);
> > + if (ret) {
> > + dev_err(&pdev->dev, "error while reading reg\n");
> > + return ret;
> > + }
> > +
> > + ret = regmap_write(s->regmap, s->mux_reg, val);
> > + if (ret) {
> > + dev_err(&pdev->dev, "error while writing reg\n");
> > + return ret;
> > + }
> > +
> > + iprop = of_get_property(np, "mux-mask", &len);
> > + if (!iprop || len != sizeof(u32)) {
> > + dev_err(&pdev->dev, "missing or invalid mux-mask
> property\n");
> > + return -ENODEV;
> > + }
> > + s->mask = (uint32_t)be32_to_cpup(iprop);
>
> Is this cast needed?
I will remove the typecasts.
>
> > +
> > + /* 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(np, np2) {
> > + iprop = of_get_property(np2, "reg", &len);
> > + if (!iprop || len != sizeof(u32)) {
> > + dev_err(&pdev->dev, "mdio-mux child node %pOF is
> missing a 'reg' property\n", np2);
> > + of_node_put(np2);
> > + return -ENODEV;
> > + }
> > + if (be32_to_cpup(iprop) & ~s->mask) {
> > + dev_err(&pdev->dev, "mdio-mux child node %pOF
> has a 'reg' value with unmasked bits\n", np2);
> > + of_node_put(np2);
> > + return -ENODEV;
> > + }
> > + }
> > +
> > + ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> > + mdio_mux_regmap_switch_fn,
> > + &s->mux_handle, s, NULL);
> > + if (ret) {
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev,
> > + "failed to register mdio-mux bus %pOF\n",
> np);
> > + return ret;
> > + }
> > +
> > + pdev->dev.platform_data = s;
> > + return 0;
> > +}
> > +
> > +static int mdio_mux_regmap_remove(struct platform_device *pdev) {
> > + struct mdio_mux_regmap_state *s = dev_get_platdata(&pdev-
> >dev);
> > +
> > + mdio_mux_uninit(s->mux_handle);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id mdio_mux_regmap_match[] = {
> > + {
> > + .compatible = "mdio-mux-regmap",
> > + },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mdio_mux_regmap_match);
> > +
> > +static struct platform_driver mdio_mux_regmap_driver = {
> > + .driver = {
> > + .name = "mdio-mux-regmap",
> > + .of_match_table = mdio_mux_regmap_match,
> > + },
> > + .probe = mdio_mux_regmap_probe,
> > + .remove = mdio_mux_regmap_remove,
> > +};
> > +
> > +module_platform_driver(mdio_mux_regmap_driver);
> > +
> > +MODULE_AUTHOR("Pankaj Bansal <pankaj.ban...@nxp.com>");
> > +MODULE_DESCRIPTION("I2c device MDIO MUX driver");
>
> I2C should be replaced by regmap.
Nice catch. I will replace it.
>
> Andrew
