From: Aswath Govindraju <a-govindraju@ti.com> To: Peter Rosin <peda@axentia.se>, Marc Kleine-Budde <mkl@pengutronix.de> Cc: Vignesh Raghavendra <vigneshr@ti.com>, Kishon Vijay Abraham I <kishon@ti.com>, Nishanth Menon <nm@ti.com>, Wolfgang Grandegger <wg@grandegger.com>, Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, <linux-can@vger.kernel.org>, <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting mux Date: Fri, 19 Nov 2021 13:12:28 +0530 [thread overview] Message-ID: <c4efbcd3-8071-7fd2-0f3a-bc42acdfd2ac@ti.com> (raw) In-Reply-To: <3f13a769-f8ef-dbe8-f2c6-ff197af8eddf@axentia.se> Hi Peter, On 18/11/21 6:14 pm, Peter Rosin wrote: >>> 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. >>>> 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 > >> Thank you for the comments. I'll post a respin of this series, with the >> above changes.
WARNING: multiple messages have this Message-ID (diff)
From: Aswath Govindraju <a-govindraju@ti.com> To: Peter Rosin <peda@axentia.se>, Marc Kleine-Budde <mkl@pengutronix.de> Cc: Vignesh Raghavendra <vigneshr@ti.com>, Kishon Vijay Abraham I <kishon@ti.com>, Nishanth Menon <nm@ti.com>, Wolfgang Grandegger <wg@grandegger.com>, Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, <linux-can@vger.kernel.org>, <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting mux Date: Fri, 19 Nov 2021 13:12:28 +0530 [thread overview] Message-ID: <c4efbcd3-8071-7fd2-0f3a-bc42acdfd2ac@ti.com> (raw) In-Reply-To: <3f13a769-f8ef-dbe8-f2c6-ff197af8eddf@axentia.se> Hi Peter, On 18/11/21 6:14 pm, Peter Rosin wrote: >>> 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. >>>> 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 > >> 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
next prev parent reply other threads:[~2021-11-19 7:42 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-11 16:43 [PATCH RFC 0/2] CAN TRANSCEIVER: Add support for setting mux Aswath Govindraju 2021-11-11 16:43 ` Aswath Govindraju 2021-11-11 16:43 ` [PATCH RFC 1/2] dt-bindings: phy: ti,tcan104x-can: Document mux-controls property Aswath Govindraju 2021-11-11 16:43 ` [PATCH RFC 1/2] dt-bindings: phy: ti, tcan104x-can: " Aswath Govindraju 2021-11-11 16:43 ` [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting mux Aswath Govindraju 2021-11-11 16:43 ` Aswath Govindraju 2021-11-12 8:40 ` Marc Kleine-Budde 2021-11-12 8:40 ` Marc Kleine-Budde 2021-11-12 13:48 ` Aswath Govindraju 2021-11-12 13:48 ` Aswath Govindraju 2021-11-12 19:15 ` Peter Rosin 2021-11-12 19:15 ` Peter Rosin 2021-11-15 6:31 ` Aswath Govindraju 2021-11-15 6:31 ` Aswath Govindraju 2021-11-17 21:24 ` Peter Rosin 2021-11-17 21:24 ` Peter Rosin 2021-11-18 11:12 ` Aswath Govindraju 2021-11-18 11:12 ` Aswath Govindraju 2021-11-18 12:44 ` Peter Rosin 2021-11-18 12:44 ` Peter Rosin 2021-11-19 7:42 ` Aswath Govindraju [this message] 2021-11-19 7:42 ` Aswath Govindraju 2021-11-22 13:04 ` [PATCH RFC 0/2] CAN TRANSCEIVER: " Aswath Govindraju 2021-11-22 13:04 ` Aswath Govindraju
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c4efbcd3-8071-7fd2-0f3a-bc42acdfd2ac@ti.com \ --to=a-govindraju@ti.com \ --cc=devicetree@vger.kernel.org \ --cc=kishon@ti.com \ --cc=linux-can@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=mkl@pengutronix.de \ --cc=nm@ti.com \ --cc=peda@axentia.se \ --cc=robh+dt@kernel.org \ --cc=vigneshr@ti.com \ --cc=vkoul@kernel.org \ --cc=wg@grandegger.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.