mail archive of the barebox mailing list
 help / color / mirror / Atom feed
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", &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

  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