Message-ID: <3f13a769-f8ef-dbe8-f2c6-ff197af8eddf@axentia.se>
Date:   Thu, 18 Nov 2021 13:44:02 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Aswath Govindraju <a-govindraju@...com>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     Vignesh Raghavendra <vigneshr@...com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Nishanth Menon <nm@...com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, linux-can@...r.kernel.org,
        linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting
 mux

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

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

Cheers,
Peter

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

Powered by blists - more mailing lists