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

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/24/2012 02:00 PM, Timur Tabi wrote:
> 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.

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.

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

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

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!