Re: [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device

On 08/24/2012 02:00 PM, Timur Tabi wrote:

Stephen Warren wrote:

quoted

quoted

+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.

True. One could always simply assume that the registers are native
endian by default, and then if ever they need not to be, add optional
properties to the binding.

quoted

Otherwise, this binding looks great now.

Do you still want me to scrub any references to the register size
requirement from the document?

I don't feel too strongly about it. It seems cleaner to, but not a big deal.

quoted

quoted

+++ b/drivers/net/phy/mdio-mux-mmioreg.c

quoted

+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.

quoted

quoted

+		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?

Ah right, I suppose that is true. It almost doesn't seem worth writing
the code to ignore that special case, since presumably the register
write is idempotent, but since it's already there you can feel free not
to rip it out!