Hi Peter,

On 18/11/21 6:14 pm, Peter Rosin wrote:

quoted

quoted

Ok, I see what you mean now, sorry for being dense. If we allow this then
there is a need to add a special value that means all/many states (such as
-1 or something such) so that a mux-control can be used simultaneously by
drivers "pointing at" a specific state like you want to do, and by the
existing "application" style drivers that wraps the whole mux control.

I.e. something like this

	mux: mux {
		compatible = "mux-gpio";
		...

		#mux-control-cells = <1>; /* one more than previously */
	};

	phy {
		...

		mux-control = <&mux 3>; /* point to specific state */
	};

	i2c-mux {
		compatible = "i2c-mux-gpmux";
		parent = <&i2c0>
		mux-control = <&mux (-1)>; /* many states needed */

		...

		i2c@1 {
			eeprom@50 {
				...
			};
		};

		i2c@2 {
			...
		};
	};

Yes, I realize that accesses to the eeprom cannot happen if the mux is
constantly selected and locked in state 3 by the phy, and that a mux with
one channel being a phy and other channels being I2C might not be
realistic, but the same gpio lines might control several muxes that are
used for separate signals solving at least the latter "problem" with this
completely made up example. Anyway, the above is in principle, and HW
designs are sometimes too weird for words.

This is almost exactly what I was intending to implement except for one
more change. The state of the mux will always be represented using the
second argument(i.e. #mux-control-cells = <2>).

For example,
mux-controls = <&mux 0 1>, <&mux 1 0>;


With this I think we wouldn't need a special value for all or many states.

But you do. Several consumers need to be able to point to the same mux
control. If some of these consumers need one state, and some other need
all/many, the consumers needing many needs to be able to say that. Listing
many entries in mux-control = <>; is misleading since then the binding implies
that you could have different mux controls for each state, which is not
possible, at least not in the current implementations. It would also be
wasteful to needlessly establish links to the same mux control multiple
times, and the binding would cause bloated device trees even if you tried
to optimize this in the drivers. Therefore, I require a special value so
that consumers can continue to point at the mux control as a whole, even
if some other consumers of the same mux control wants to point at a specific
state.

Understood. One issue that I see is that we certainly can not use the
first argument for representing state as it will result in errors for
current users.

I feel that the safest way to go would be by using a second argument to
represent the state or to represent multiple states can be used by the
driver. The issue that I see with this approach is that currently the
fist argument is used to select the line number from the mux and if the
we use two arguments like this,

mux-controls = <&mux 0 -1>

then this would mean that line nnumber 0 in the mux could use multiple
states and for a driver to use mutiple lines we would need to add an
entry for each line which would bloat the code a well increase the
complexity in the drivers while using devm_mux_get(). So, one solution
that I could think of is to use a "-1" for the first argument too. This
would indicate that the driver would need to toggle multiple lines in
the mux

For example,

1) mux-controls = <&mux -1 3> // the driver would need to set the mux
lines to 3 for enabling it

2) mux-controls = <&mux -1 -1> //the driver would need to set the mux
lines and multiple states in the mux

3) mux-controls = <&mux 0 1> // the driver would need to set the zeroth
mux line to 1

I do see that, going with this method would make <&mux ^\d*$ ^\d*$>(i.e.
any positive number in the first argument) redundant as it can be
represented with <&mux -1 *>. However, I think is the only way so that
existing users will not see issues.

quoted

quoted

quoted

One more question that I had is, if the number of arguments match the
#mux-control-cells and if the number of arguments are greater than 1 why
is an error being returned?

Changing that would require a bindings update anyway, so I simply
disallowed it as an error. Not much thought went into the decision,
as it couldn't be wrong to do what is being done with the bindings
that exist. That said, I have no problem lifting this restriction,
if there's a matching bindings update that makes it all fit.

Sure, I think making a change in

Documentation/devicetree/bindings/mux/gpio-mux.yaml, should be good
enough I assume.

Well, the new way to bind has very little to do with this being a gpio
mux. There is no reason not to allow this way to bind for any of the
other muxes. That said, the reg-mux binding has this:

  '#mux-control-cells':
    const: 1

Similarly, the adi,adg792a has explicit wording on how #mux-control-cells
works (but being a txt binding it is not checked, but that does not matter,
bindings should be correct). I now notice that this is missing from the
adi,adgs1408 binding, but that's an oversight.

The mux-controller binding has this:
  '#mux-control-cells':
    enum: [ 0, 1 ]

The mux-consumer binding should probably be updated with some words
on this subject too.

So, all mux bindings need updates when this door is opened. And, in order
to add this in a compatible way, the old way to bind with 0/1 cells needs
to continue to both work and be allowed.

I think it is easiest to add something common to the mux-controller
binding and then have the specific bindings simply inherit it from there
instead of adding (almost) the same words to all the driver bindings.

Understood, I will try to add changes in the common mux-controller
bindings itself and then reference it in the gpio-mux bindings

Thanks,
Aswath

Cheers,
Peter

quoted

Thank you for the comments. I'll post a respin of this series, with the
above changes.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy