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  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]
Date:	Sun, 14 Oct 2007 16:05:17 -0600
From:	"Grant Likely" <grant.likely@...retlab.ca>
To:	"Domen Puncer" <domen.puncer@...argo.com>
Cc:	galak@...nel.crashing.org, jgarzik@...ox.com,
	linuxppc-dev@...abs.org, tnt@...tnt.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3 4/4] FEC mpc52xx: phy part of the driver

On 10/14/07, Domen Puncer <domen.puncer@...argo.com> wrote:
> PHY part of the driver for mpc5200(b) ethernet.

Assuming I understand correctly, this comment is not correct and this
patch just adds an MDIO bus driver.  PHY drivers are in phylib and
data transfer is setup via the core driver, correct?

It is conceivable that the PHY is connected to an alternate MDIO bus,
or the MDIO bus is used for a PHY connected to an external Ethernet
controller.

Speaking of which, is it possible to use this MDIO bus without the
core FEC being initialized?

> +static struct of_device_id fec_mdio_match[] = {
> +       {
> +               .type = "mdio",
> +               .compatible = "mpc5200b-fec-phy",

This is not a phy; it's an MDIO bus.  Also, shouldn't this be
"mpc5200-..." instead of "mpc5200b-..."?

> +       },
> +       {},
> +};
> +
> +struct of_platform_driver mpc52xx_fec_mdio_driver = {
> +       .name = "mpc5200b-fec-phy",
> +       .probe = fec_mdio_probe,
> +       .remove = fec_mdio_remove,
> +       .match_table = fec_mdio_match,

Inconsistent naming.  Please use the same prefix on all global/static
symbols (ie. use "mpc52xx_mdio_" instead of the mix of
"mpc52xx_fec_mdio_", "fec_mdio_", etc.)  I also thing that "fec_mdio_"
is too generic because there are a number of different incompatible
FEC devices.

> +};
> +
> +/* let fec driver call it, since this has to be registered before it */
> +EXPORT_SYMBOL_GPL(mpc52xx_fec_mdio_driver);

Why not have a module_init()/module_exit() in this file?  I don't
think the FEC driver calls this driver's functions directly anymore,
and it's still dependent on the of_platform bus probe order anyway.

As an added bonus, it makes your Makefile much simpler.

> +
> +
> +MODULE_LICENSE("Dual BSD/GPL");
> Index: linux.git/drivers/net/fec_mpc52xx/Makefile
> ===================================================================
> --- linux.git.orig/drivers/net/fec_mpc52xx/Makefile
> +++ linux.git/drivers/net/fec_mpc52xx/Makefile
> @@ -1,2 +1,7 @@
>  obj-$(CONFIG_FEC_MPC52xx) += fec_mpc52xx.o
>  fec_mpc52xx-objs := fec.o
> +
> +ifeq ($(CONFIG_FEC_MPC52xx_MDIO),y)
> +       obj-$(CONFIG_FEC_MPC52xx) += fec_mpc52xx_phy.o
> +       fec_mpc52xx_phy-objs := fec_phy.o
> +endif
> Index: linux.git/drivers/net/fec_mpc52xx/Kconfig
> ===================================================================
> --- linux.git.orig/drivers/net/fec_mpc52xx/Kconfig
> +++ linux.git/drivers/net/fec_mpc52xx/Kconfig
> @@ -11,5 +11,18 @@ config FEC_MPC52xx
>         ---help---
>           This option enables support for the MPC5200's on-chip
>           Fast Ethernet Controller
> +         If compiled as module, it will be called 'fec_mpc52xx.ko'.

Drop this line and make the help text the same format as the other eth
drivers in drivers/net.

> +
> +config FEC_MPC52xx_MDIO
> +       bool "FEC MII PHY driver"
> +       depends on FEC_MPC52xx
> +       default y
> +       ---help---
> +         The MPC5200's FEC can connect to the Ethernet either with
> +         an external MII PHY chip or 10 Mbps 7-wire interface
> +         (Motorola? industry standard).
> +         If your board uses an external PHY, enable this.

Not strictly true.  This enables talking to a PHY using the internal
MDIO controller.  PHY register access could just as easily be accessed
via an alternate interface.

> +         If not sure, enable.
> +         If compiled as module, it will be called 'fec_mpc52xx_phy.ko'.

Drop the module name comment.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@...retlab.ca
(403) 399-0195
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists