From mboxrd@z Thu Jan 1 00:00:00 1970 From: Timur Tabi Subject: Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device Date: Fri, 24 Aug 2012 15:00:32 -0500 Message-ID: <5037DD60.8090206@freescale.com> References: <1345835453-8611-1-git-send-email-timur@freescale.com> <5037DB43.60704@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5037DB43.60704@wwwdotorg.org> Sender: netdev-owner@vger.kernel.org To: Stephen Warren Cc: Andy Fleming , David Miller , ddaney.cavm@gmail.com, devicetree-discuss@lists.ozlabs.org, netdev@vger.kernel.org List-Id: devicetree@vger.kernel.org Stephen Warren wrote: >> +This is a special case of a MDIO bus multiplexer. A memory-mapped device, >> +like an FPGA, is used to control which child bus is connected. The mdio-mux >> +node must be a child of the memory-mapped device. The driver currently only >> +supports devices with eight-bit registers. > > That last sentence seems like a property of the driver, not the binding; > I could easily anticipate allowing the size to be 1 or 2 or 4, and a > driver adapter to that in the future. True, but I couldn't think of a better place to mention this. Adding support for multi-byte registers also requires handling the endianness of those registers. I have that problem with the mdio-mux-gpio driver. That driver assumes that the GPIO bits are numbered in little-endian order, so my device tree on my big-endian CPU (PowerPC) lists the GPIO pins in reverse order. > Otherwise, this binding looks great now. Do you still want me to scrub any references to the register size requirement from the document? >> +++ b/drivers/net/phy/mdio-mux-mmioreg.c > >> +static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child, >> + void *data) >> +{ >> + struct mdio_mux_mmioreg_state *s = data; >> + >> + if (current_child ^ desired_child) { >> + void *p = ioremap(s->phys, 1); >> + uint8_t x, y; >> + >> + if (!p) >> + return -ENOMEM; > > Why not map it during probe? I thought about that, but I generally don't like mappings that exist for all eternity even though they're rarely used. Once the interface is up, we don't expect any bus muxing to occur. > >> + x = ioread8(p); >> + y = (x & ~s->mask) | desired_child; >> + if (x != y) { > > Isn't that always true, given if (current_child ^ desired_child) above? If current_child == -1, but the bus is already muxed properly, then there's no point in setting it. Do you want me to remove the test, or add a comment? -- Timur Tabi Linux kernel developer at Freescale