On Mon, Oct 15, 2018 at 2:19 PM Sam Ravnborg wrote:
>
> Hi Andrey.
>
> Some random nits while browsing the code.
>
> On Sun, Oct 14, 2018 at 07:21:23PM -0700, Andrey Smirnov wrote:
> > Port a very abridged version of MV88E6XXX DSA driver from Linux
> > kernel. Currently only internal MDIO bus connected to switch PHYs is
> > exposed.
> >
> > Signed-off-by: Andrey Smirnov
> >
> > net: phy: mv88e6xxx: Expose internal MDIO bus
> > ---
> > drivers/net/phy/Kconfig | 6 +
> > drivers/net/phy/Makefile| 1 +
> > drivers/net/phy/mv88e6xxx/Makefile | 5 +
> > drivers/net/phy/mv88e6xxx/chip.c| 723
> > drivers/net/phy/mv88e6xxx/chip.h| 61 +++
> > drivers/net/phy/mv88e6xxx/global2.c | 124 +
> > drivers/net/phy/mv88e6xxx/global2.h | 41 ++
> > drivers/net/phy/mv88e6xxx/port.c| 20 +
> > drivers/net/phy/mv88e6xxx/port.h| 89
> > 9 files changed, 1070 insertions(+)
> > create mode 100644 drivers/net/phy/mv88e6xxx/Makefile
> > create mode 100644 drivers/net/phy/mv88e6xxx/chip.c
> > create mode 100644 drivers/net/phy/mv88e6xxx/chip.h
> > create mode 100644 drivers/net/phy/mv88e6xxx/global2.c
> > create mode 100644 drivers/net/phy/mv88e6xxx/global2.h
> > create mode 100644 drivers/net/phy/mv88e6xxx/port.c
> > create mode 100644 drivers/net/phy/mv88e6xxx/port.h
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 79fb917ee..3b1a6ea7e 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -48,6 +48,12 @@ config SMSC_PHY
> > ---help---
> > Currently supports the LAN83C185, LAN8187 and LAN8700 PHYs
> >
> > +config NET_DSA_MV88E6XXX
> > + tristate "Marvell 88E6xxx Ethernet switch fabric support"
> > + help
> > + This driver adds support for most of the Marvell 88E6xxx models of
> > + Ethernet switch chips, except 88E6060.
> > +
> > comment "MII bus device drivers"
> >
> > config MDIO_MVEBU
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 4424054d9..e4d9ec65a 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_MARVELL_PHY) += marvell.o
> > obj-$(CONFIG_MICREL_PHY) += micrel.o
> > obj-$(CONFIG_NATIONAL_PHY) += national.o
> > obj-$(CONFIG_SMSC_PHY) += smsc.o
> > +obj-$(CONFIG_NET_DSA_MV88E6XXX) += mv88e6xxx/
> >
> > obj-$(CONFIG_MDIO_MVEBU) += mdio-mvebu.o
> > obj-$(CONFIG_MDIO_BITBANG) += mdio-bitbang.o
> > diff --git a/drivers/net/phy/mv88e6xxx/Makefile
> > b/drivers/net/phy/mv88e6xxx/Makefile
> > new file mode 100644
> > index 0..8c8ba78cd
> > --- /dev/null
> > +++ b/drivers/net/phy/mv88e6xxx/Makefile
> > @@ -0,0 +1,5 @@
> > +obj-y += mv88e6xxx.o
> > +
> > +mv88e6xxx-objs := chip.o
> > +mv88e6xxx-objs += global2.o
> > +mv88e6xxx-objs += port.o
> > \ No newline at end of file
>
> Another way to write the above would be:
> mv88e6xxx-y := chip.o
> mv88e6xxx-y += global2.o
> mv88e6xxx-y += port.o
>
> And if you change this then you can alos add the missing newline
>
> As the kernel uses -objs you will likely keep current syntax,
> just wanted to point in the direction of the new way to express this.
>
I'll fix the lack of newline in v4. As you mentioned this is a
straight copy of what's in kernel and I tried to keep as much original
code intact as possible. Thanks for the pointer, though.
>
> > +static int mv88e6xxx_probe(struct device_d *dev)
> > +{
> > + struct device_node *np = dev->device_node;
> > + struct device_node *mdio_node;
> > + struct mv88e6xxx_chip *chip;
> > + enum of_gpio_flags of_flags;
> > + int err;
> > + u32 reg;
> > +
> > + err = of_property_read_u32(np, "reg", );
> > + if (err) {
> > + dev_err(dev, "Couldn't determine switch MIDO address\n");
> > + return err;
> > + }
> > +
> > + if (reg) {
> > + dev_err(dev, "Only single-chip address mode is supported\n");
> > + return -ENOTSUPP;
> > + }
> > +
> > + chip = xzalloc(sizeof(struct mv88e6xxx_chip));
> > + chip->dev = dev;
> > + chip->info = of_device_get_match_data(dev);
> > +
> > + chip->parent_miibus = of_mdio_find_bus(np->parent);
> > + if (!chip->parent_miibus)
> > + return -EPROBE_DEFER;
> > +
> > + chip->reset = of_get_named_gpio_flags(np, "reset-gpios", 0,
> > _flags);
> > + if (gpio_is_valid(chip->reset)) {
> > + unsigned long flags = GPIOF_OUT_INIT_INACTIVE;
> > + char *name;
> > +
> > + if (of_flags & OF_GPIO_ACTIVE_LOW)
> > + flags |= GPIOF_ACTIVE_LOW;
> > +
> > + name = basprintf("%s reset", dev_name(dev));
> > + err = gpio_request_one(chip->reset, flags, name);
> > + if (err < 0)
> > + return err;
> > + /*
> > + * We assume that reset line was previously held
