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 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
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