From: Sam Ravnborg <sam@ravnborg.org>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v3 20/22] net: phy: Add basic driver for MV88E6XXX switches from Marvell
Date: Mon, 15 Oct 2018 23:19:09 +0200 [thread overview]
Message-ID: <20181015211909.GB17344@ravnborg.org> (raw)
In-Reply-To: <20181015022125.24020-21-andrew.smirnov@gmail.com>
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 <andrew.smirnov@gmail.com>
>
> 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 000000000..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.
> +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, &of_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 low
> + * and give the switch time to initialize before
> + * trying to read its registers
> + */
> + mv88e6xxx_hardware_reset_delay();
> + }
> +
> + err = mv88e6xxx_detect(chip);
> + if (err)
> + return err;
> +
> + err = mv88e6xxx_switch_reset(chip);
> + if (err)
> + return err;
> + /*
> + * In single-chip address mode addresses 0x10 - 0x1f are
> + * reserved to access various switch registers and do not
> + * correspond to any PHYs, so we mask them to pervent from
> + * being exposed as phy devices
> + */
> + chip->parent_miibus->phy_mask |= GENMASK(0x1f, 0x10);
> + /*
> + * Address 0x0f on internal bus is dedicated to SERDES
> + * registers and won't be very useful against standard PHY
> + * driver
> + */
> + chip->miibus.phy_mask |= GENMASK(0x1f, 0x0f);
> +
> + chip->miibus.read = mv88e6xxx_mdio_read;
> + chip->miibus.write = mv88e6xxx_mdio_write;
The function pointers are hardcoded here.
But we have them in chip->info->ops - where we can
have chip specific variants.
I assume it would be more correct to copy them from the ops structure?
> +#endif /* _MV88E6XXX_CHIP_H */
> \ No newline at end of file
Hmm...
> +#endif /* _MV88E6XXX_GLOBAL2_H */
> \ No newline at end of file
Hmmm...
> +#endif /* _MV88E6XXX_PORT_H */
> \ No newline at end of file
Again
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
next prev parent reply other threads:[~2018-10-15 21:19 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-15 2:21 [PATCH v3 00/22] MV88E6xxx switch family support Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 01/22] ARM: Do not expose ARMv8 functions on ARMv7 Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 02/22] clocksource: Add ARM global timer support Andrey Smirnov
2018-10-15 20:46 ` Sam Ravnborg
2018-10-15 21:27 ` Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 03/22] VFxxx: Select CLOCKSOURCE_ARM_GLOBAL_TIMER Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 04/22] i.MX: Move GPT driver to drivers/clocksource Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 05/22] clocksource: Introduce ARCH_HAS_IMX_GPT Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 06/22] of: Demote "Bad cell count for" to debug Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 07/22] aiodev: Don't try to use DT node name as aiodev->name Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 08/22] aiodev: imx_thermal: Give aiodev a more descriptive name Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 09/22] aiodev: qoriq_thermal: " Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 10/22] drivers: Introduce dev_set_name() Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 11/22] base: Don't use shared buffer for results of dev_id() Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 12/22] drivers: base: Convert device_d name to be dynamically allocated Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 13/22] linux: string: Port kbasename() Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 14/22] of: Port latest of_device_make_bus_id() implementation Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 15/22] mdio_bus: Fix documentation for mdio_bus_match() Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 16/22] include: linux: phy: Add missing PHY_INTERFACE_* constants Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 17/22] include: linux: ethtool: Add missing *_UNKNOWN constants Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 18/22] net: phy: Check phy_mask in get_phy_device() Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 19/22] mdio_bus: Allow for non PHY-devices on MDIO buses Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 20/22] net: phy: Add basic driver for MV88E6XXX switches from Marvell Andrey Smirnov
2018-10-15 21:19 ` Sam Ravnborg [this message]
2018-10-15 21:35 ` Andrey Smirnov
2018-10-16 5:46 ` Sam Ravnborg
2018-10-15 2:21 ` [PATCH v3 21/22] net: phy: mv88e6xxx: Port EEPROM support code Andrey Smirnov
2018-10-15 2:21 ` [PATCH v3 22/22] net: phy: mv88e6xxx: Add support for MAC ports Andrey Smirnov
2018-10-15 3:33 ` [PATCH v3 00/22] MV88E6xxx switch family support Andrey Smirnov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181015211909.GB17344@ravnborg.org \
--to=sam@ravnborg.org \
--cc=andrew.smirnov@gmail.com \
--cc=barebox@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox