- The following series of patches add support for reading the state of the
mux to be set for enabling given device.
- As these are RFC patches I have combined them into a single series for
better understanding of the reason behind making this change.
Changes since v2:
- Fixed changes in phy-can-transceiver based on comments
- added select MULTIPLEXER in drivers/phy/Kconfig
- Changed the implemetation of getting state by adding new apis
Changes since v1:
- Added support for reading the enable state from DT instead of hardcoding
the state to be set to 1.
- Made relavent changes in the bindings
Link to v2:
- https://patchwork.kernel.org/project/linux-phy/list/?series=583917
Link to v1,
- https://patchwork.kernel.org/project/linux-phy/list/?series=578863&state=*
Aswath Govindraju (4):
dt-bindings: mux: Increase the number of arguments in mux-controls
dt-bindings: phy: ti,tcan104x-can: Document mux-controls property
mux: Add support for reading mux enable state from DT
phy: phy-can-transceiver: Add support for setting mux
.../devicetree/bindings/mux/gpio-mux.yaml | 2 +-
.../bindings/mux/mux-controller.yaml | 2 +-
.../bindings/phy/ti,tcan104x-can.yaml | 8 +
drivers/mux/core.c | 146 +++++++++++++++++-
drivers/phy/Kconfig | 1 +
drivers/phy/phy-can-transceiver.c | 22 +++
include/linux/mux/consumer.h | 19 ++-
include/linux/mux/driver.h | 13 ++
8 files changed, 206 insertions(+), 7 deletions(-)
--
2.17.1
Increase the allowed number of arguments in mux-controls to add support for
passing information regarding the state of the mux to be set, for a given
device.
Signed-off-by: Aswath Govindraju <[email protected]>
---
Documentation/devicetree/bindings/mux/gpio-mux.yaml | 2 +-
Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
index 0a7c8d64981a..c810b7df39de 100644
--- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
+++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
@@ -26,7 +26,7 @@ properties:
List of gpios used to control the multiplexer, least significant bit first.
'#mux-control-cells':
- const: 0
+ enum: [ 0, 1, 2 ]
idle-state:
default: -1
diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
index 736a84c3b6a5..0b4b067a97bf 100644
--- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
+++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
@@ -73,7 +73,7 @@ properties:
pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
'#mux-control-cells':
- enum: [ 0, 1 ]
+ enum: [ 0, 1, 2 ]
idle-state:
$ref: /schemas/types.yaml#/definitions/int32
--
2.17.1
On some boards, for routing CAN signals from controller to transceivers,
muxes might need to be set. Use mux-controls property can be used for model
this.
Signed-off-by: Aswath Govindraju <[email protected]>
---
.../devicetree/bindings/phy/ti,tcan104x-can.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
index 6107880e5246..7392088cf060 100644
--- a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
@@ -37,6 +37,13 @@ properties:
max bit rate supported in bps
minimum: 1
+ mux-controls:
+ description:
+ mux controller node to route the signals from controller to
+ transceiver. The first and second arguments can be used
+ representing the line in the mux-controller to control and
+ the state to be set in the given line respectively.
+
required:
- compatible
- '#phy-cells'
@@ -53,4 +60,5 @@ examples:
max-bitrate = <5000000>;
standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_HIGH>;
+ mux-controls = <&mux0 0 1>;
};
--
2.17.1
Hi!
You need to have some description on how #mux-control-cells now work.
The previous description is in mux-consumer.yaml and an update there
is needed.
However, I have realized that the adg792a binding uses #mux-control-cells
to indicate if it should expose its three muxes with one mux-control
and operate the muxes in parallel, or if it should be expose three
independent mux-controls. So, the approach in this series to always
have the #mux-control-cells property fixed at <2> when indicating a
state will not work for that binding. And I see no fix for that binding
without adding a new property.
So, I would like a different approach. Since I dislike how mux-controls
-after this series- is not (always) specifying a mux-control like the name
says, but instead optionally a specific state, the new property I would
like to add is #mux-state-cells such that it would always be one more
than #mux-control-cells.
mux: mux-controller {
compatible = "gpio-mux";
#mux-control-cells = <0>;
#mux-state-cells = <1>;
mux-gpios = <...>;
};
can-phy {
compatible = "ti,tcan1043";
...
mux-states = <&mux 1>;
};
That solves the naming issue, the unused argument for mux-conrtrollers
that previously had #mux-control-cells = <0>, and the binding for adg792a
need no longer be inconsistent.
Or, how should this be solved? I'm sure there are other options...
Cheers,
Peter
On 2021-11-23 09:12, Aswath Govindraju wrote:
> Increase the allowed number of arguments in mux-controls to add support for
> passing information regarding the state of the mux to be set, for a given
> device.
>
> Signed-off-by: Aswath Govindraju <[email protected]>
> ---
> Documentation/devicetree/bindings/mux/gpio-mux.yaml | 2 +-
> Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> index 0a7c8d64981a..c810b7df39de 100644
> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
> @@ -26,7 +26,7 @@ properties:
> List of gpios used to control the multiplexer, least significant bit first.
>
> '#mux-control-cells':
> - const: 0
> + enum: [ 0, 1, 2 ]
>
> idle-state:
> default: -1
> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> index 736a84c3b6a5..0b4b067a97bf 100644
> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
> @@ -73,7 +73,7 @@ properties:
> pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>
> '#mux-control-cells':
> - enum: [ 0, 1 ]
> + enum: [ 0, 1, 2 ]
>
> idle-state:
> $ref: /schemas/types.yaml#/definitions/int32
>
Hi Peter,
On 25/11/21 7:05 pm, Peter Rosin wrote:
> Hi!
>
> You need to have some description on how #mux-control-cells now work.
> The previous description is in mux-consumer.yaml and an update there
> is needed.
>
> However, I have realized that the adg792a binding uses #mux-control-cells
> to indicate if it should expose its three muxes with one mux-control
> and operate the muxes in parallel, or if it should be expose three
> independent mux-controls. So, the approach in this series to always
> have the #mux-control-cells property fixed at <2> when indicating a
> state will not work for that binding. And I see no fix for that binding
> without adding a new property.
>
> So, I would like a different approach. Since I dislike how mux-controls
> -after this series- is not (always) specifying a mux-control like the name
> says, but instead optionally a specific state, the new property I would
> like to add is #mux-state-cells such that it would always be one more
> than #mux-control-cells.
>
> mux: mux-controller {
> compatible = "gpio-mux";
> #mux-control-cells = <0>;
> #mux-state-cells = <1>;
>
> mux-gpios = <...>;
> };
>
> can-phy {
> compatible = "ti,tcan1043";
> ...
> mux-states = <&mux 1>;
> };
>
> That solves the naming issue, the unused argument for mux-conrtrollers
> that previously had #mux-control-cells = <0>, and the binding for adg792a
> need no longer be inconsistent.
>
> Or, how should this be solved? I'm sure there are other options...
>
I feel that the new approach using mux-state-cells seems to be
overpopulating the device tree nodes, when the state can be represented
using the control cells. I understand that the definition for
mux-controls is to only specify the control line to be used in a given
mux. Can't it now be upgraded to also represent the state at which the
control line has to be set to?
With respect to adg792a, it is inline with the current implementation
and the only change I think would be required in the driver is,
diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
index e8fc2fc1ab09..2cd3bb8a40d4 100644
--- a/drivers/mux/adg792a.c
+++ b/drivers/mux/adg792a.c
@@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
if (ret < 0)
return ret;
- if (cells >= 2)
- return -EINVAL;
mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
if (IS_ERR(mux_chip))
And the following series should be compatible with it. If adg792a driver
is the only issue then would there be any issue with only changing it
and using this implementation?
Thanks,
Aswath
> Cheers,
> Peter
>
> On 2021-11-23 09:12, Aswath Govindraju wrote:
>> Increase the allowed number of arguments in mux-controls to add support for
>> passing information regarding the state of the mux to be set, for a given
>> device.
>>
>> Signed-off-by: Aswath Govindraju <[email protected]>
>> ---
>> Documentation/devicetree/bindings/mux/gpio-mux.yaml | 2 +-
>> Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> index 0a7c8d64981a..c810b7df39de 100644
>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>> @@ -26,7 +26,7 @@ properties:
>> List of gpios used to control the multiplexer, least significant bit first.
>>
>> '#mux-control-cells':
>> - const: 0
>> + enum: [ 0, 1, 2 ]
>>
>> idle-state:
>> default: -1
>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> index 736a84c3b6a5..0b4b067a97bf 100644
>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>> @@ -73,7 +73,7 @@ properties:
>> pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>
>> '#mux-control-cells':
>> - enum: [ 0, 1 ]
>> + enum: [ 0, 1, 2 ]
>>
>> idle-state:
>> $ref: /schemas/types.yaml#/definitions/int32
>>
On 2021-11-29 05:36, Aswath Govindraju wrote:
> Hi Peter,
>
> On 25/11/21 7:05 pm, Peter Rosin wrote:
>> Hi!
>>
>> You need to have some description on how #mux-control-cells now work.
>> The previous description is in mux-consumer.yaml and an update there
>> is needed.
>>
>> However, I have realized that the adg792a binding uses #mux-control-cells
>> to indicate if it should expose its three muxes with one mux-control
>> and operate the muxes in parallel, or if it should be expose three
>> independent mux-controls. So, the approach in this series to always
>> have the #mux-control-cells property fixed at <2> when indicating a
>> state will not work for that binding. And I see no fix for that binding
>> without adding a new property.
>>
>> So, I would like a different approach. Since I dislike how mux-controls
>> -after this series- is not (always) specifying a mux-control like the name
>> says, but instead optionally a specific state, the new property I would
>> like to add is #mux-state-cells such that it would always be one more
>> than #mux-control-cells.
>>
>> mux: mux-controller {
>> compatible = "gpio-mux";
>> #mux-control-cells = <0>;
>> #mux-state-cells = <1>;
>>
>> mux-gpios = <...>;
>> };
>>
>> can-phy {
>> compatible = "ti,tcan1043";
>> ...
>> mux-states = <&mux 1>;
>> };
>>
>> That solves the naming issue, the unused argument for mux-conrtrollers
>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>> need no longer be inconsistent.
>>
>> Or, how should this be solved? I'm sure there are other options...
>>
>
>
> I feel that the new approach using mux-state-cells seems to be
> overpopulating the device tree nodes, when the state can be represented
> using the control cells. I understand that the definition for
> mux-controls is to only specify the control line to be used in a given
> mux. Can't it now be upgraded to also represent the state at which the
> control line has to be set to?
>
> With respect to adg792a, it is inline with the current implementation
> and the only change I think would be required in the driver is,
No, that does not work. See below.
>
> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
> index e8fc2fc1ab09..2cd3bb8a40d4 100644
> --- a/drivers/mux/adg792a.c
> +++ b/drivers/mux/adg792a.c
> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
> ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
> if (ret < 0)
> return ret;
> - if (cells >= 2)
> - return -EINVAL;
>
> mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
When you add cell #2 with the state, the cells variable will end up
as 2 always. Which means that there is no way to alloc one mux
control since "cells ? 3 : 1" will always end up as "3", with no
easy fix.
So, your approach does not work for this driver.
Cheers,
Peter
> if (IS_ERR(mux_chip))
>
> And the following series should be compatible with it. If adg792a driver
> is the only issue then would there be any issue with only changing it
> and using this implementation?
>
> Thanks,
> Aswath
>
>
>
>
>> Cheers,
>> Peter
>>
>> On 2021-11-23 09:12, Aswath Govindraju wrote:
>>> Increase the allowed number of arguments in mux-controls to add support for
>>> passing information regarding the state of the mux to be set, for a given
>>> device.
>>>
>>> Signed-off-by: Aswath Govindraju <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/mux/gpio-mux.yaml | 2 +-
>>> Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> index 0a7c8d64981a..c810b7df39de 100644
>>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>> @@ -26,7 +26,7 @@ properties:
>>> List of gpios used to control the multiplexer, least significant bit first.
>>>
>>> '#mux-control-cells':
>>> - const: 0
>>> + enum: [ 0, 1, 2 ]
>>>
>>> idle-state:
>>> default: -1
>>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> index 736a84c3b6a5..0b4b067a97bf 100644
>>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>> @@ -73,7 +73,7 @@ properties:
>>> pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>>
>>> '#mux-control-cells':
>>> - enum: [ 0, 1 ]
>>> + enum: [ 0, 1, 2 ]
>>>
>>> idle-state:
>>> $ref: /schemas/types.yaml#/definitions/int32
>>>
>
Hi Peter,
On 29/11/21 1:45 pm, Peter Rosin wrote:
>
>
> On 2021-11-29 05:36, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>> Hi!
>>>
>>> You need to have some description on how #mux-control-cells now work.
>>> The previous description is in mux-consumer.yaml and an update there
>>> is needed.
>>>
>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>> to indicate if it should expose its three muxes with one mux-control
>>> and operate the muxes in parallel, or if it should be expose three
>>> independent mux-controls. So, the approach in this series to always
>>> have the #mux-control-cells property fixed at <2> when indicating a
>>> state will not work for that binding. And I see no fix for that binding
>>> without adding a new property.
>>>
>>> So, I would like a different approach. Since I dislike how mux-controls
>>> -after this series- is not (always) specifying a mux-control like the name
>>> says, but instead optionally a specific state, the new property I would
>>> like to add is #mux-state-cells such that it would always be one more
>>> than #mux-control-cells.
>>>
>>> mux: mux-controller {
>>> compatible = "gpio-mux";
>>> #mux-control-cells = <0>;
>>> #mux-state-cells = <1>;
>>>
>>> mux-gpios = <...>;
>>> };
>>>
>>> can-phy {
>>> compatible = "ti,tcan1043";
>>> ...
>>> mux-states = <&mux 1>;
>>> };
>>>
>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>> need no longer be inconsistent.
>>>
>>> Or, how should this be solved? I'm sure there are other options...
>>>
>>
>>
>> I feel that the new approach using mux-state-cells seems to be
>> overpopulating the device tree nodes, when the state can be represented
>> using the control cells. I understand that the definition for
>> mux-controls is to only specify the control line to be used in a given
>> mux. Can't it now be upgraded to also represent the state at which the
>> control line has to be set to?
>>
>> With respect to adg792a, it is inline with the current implementation
>> and the only change I think would be required in the driver is,
>
> No, that does not work. See below.
>
>>
>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>> --- a/drivers/mux/adg792a.c
>> +++ b/drivers/mux/adg792a.c
>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>> ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>> if (ret < 0)
>> return ret;
>> - if (cells >= 2)
>> - return -EINVAL;
>>
>> mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
>
> When you add cell #2 with the state, the cells variable will end up
> as 2 always. Which means that there is no way to alloc one mux
> control since "cells ? 3 : 1" will always end up as "3", with no
> easy fix.
>
> So, your approach does not work for this driver.
>
Sorry, but how is this different from the case of
#mux-control-cells = 1
If #mux-control-cells is equal to 1 it means the consumer will use a
given control line from the mux chip. The same would be the case when we
will be using, #mux-control-cells is equal to 2, except we can also
provide the state.
If the consumer will use all the lines then #mux-control-cells will be
set to 0. In this condition the state can't be provided from the DT and
the consumer will be controlling the entire mux chip. If
#mux-control-cells is greater than 0 then we will not be able to provide
multiple lines of control using a single mux-controls entry(mux-controls
= <...>;) right? We would have the using multiple mux-controls
entries(mux-controls = <...>, <...>;).
Thanks,
Aswath
> Cheers,
> Peter
>
>> if (IS_ERR(mux_chip))
>>
>> And the following series should be compatible with it. If adg792a driver
>> is the only issue then would there be any issue with only changing it
>> and using this implementation?
>>
>> Thanks,
>> Aswath
>>
>>
>>
>>
>>> Cheers,
>>> Peter
>>>
>>> On 2021-11-23 09:12, Aswath Govindraju wrote:
>>>> Increase the allowed number of arguments in mux-controls to add support for
>>>> passing information regarding the state of the mux to be set, for a given
>>>> device.
>>>>
>>>> Signed-off-by: Aswath Govindraju <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/mux/gpio-mux.yaml | 2 +-
>>>> Documentation/devicetree/bindings/mux/mux-controller.yaml | 2 +-
>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mux/gpio-mux.yaml b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> index 0a7c8d64981a..c810b7df39de 100644
>>>> --- a/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> +++ b/Documentation/devicetree/bindings/mux/gpio-mux.yaml
>>>> @@ -26,7 +26,7 @@ properties:
>>>> List of gpios used to control the multiplexer, least significant bit first.
>>>>
>>>> '#mux-control-cells':
>>>> - const: 0
>>>> + enum: [ 0, 1, 2 ]
>>>>
>>>> idle-state:
>>>> default: -1
>>>> diff --git a/Documentation/devicetree/bindings/mux/mux-controller.yaml b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> index 736a84c3b6a5..0b4b067a97bf 100644
>>>> --- a/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/mux/mux-controller.yaml
>>>> @@ -73,7 +73,7 @@ properties:
>>>> pattern: '^mux-controller(@.*|-[0-9a-f]+)?$'
>>>>
>>>> '#mux-control-cells':
>>>> - enum: [ 0, 1 ]
>>>> + enum: [ 0, 1, 2 ]
>>>>
>>>> idle-state:
>>>> $ref: /schemas/types.yaml#/definitions/int32
>>>>
>>
>
Hi Aswath,
On 2021-11-29 10:31, Aswath Govindraju wrote:
> Hi Peter,
>
> On 29/11/21 1:45 pm, Peter Rosin wrote:
>>
>>
>> On 2021-11-29 05:36, Aswath Govindraju wrote:
>>> Hi Peter,
>>>
>>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> You need to have some description on how #mux-control-cells now work.
>>>> The previous description is in mux-consumer.yaml and an update there
>>>> is needed.
>>>>
>>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>>> to indicate if it should expose its three muxes with one mux-control
>>>> and operate the muxes in parallel, or if it should be expose three
>>>> independent mux-controls. So, the approach in this series to always
>>>> have the #mux-control-cells property fixed at <2> when indicating a
>>>> state will not work for that binding. And I see no fix for that binding
>>>> without adding a new property.
>>>>
>>>> So, I would like a different approach. Since I dislike how mux-controls
>>>> -after this series- is not (always) specifying a mux-control like the name
>>>> says, but instead optionally a specific state, the new property I would
>>>> like to add is #mux-state-cells such that it would always be one more
>>>> than #mux-control-cells.
>>>>
>>>> mux: mux-controller {
>>>> compatible = "gpio-mux";
>>>> #mux-control-cells = <0>;
>>>> #mux-state-cells = <1>;
>>>>
>>>> mux-gpios = <...>;
>>>> };
>>>>
>>>> can-phy {
>>>> compatible = "ti,tcan1043";
>>>> ...
>>>> mux-states = <&mux 1>;
>>>> };
>>>>
>>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>>> need no longer be inconsistent.
>>>>
>>>> Or, how should this be solved? I'm sure there are other options...
>>>>
>>>
>>>
>>> I feel that the new approach using mux-state-cells seems to be
>>> overpopulating the device tree nodes, when the state can be represented
>>> using the control cells. I understand that the definition for
>>> mux-controls is to only specify the control line to be used in a given
>>> mux. Can't it now be upgraded to also represent the state at which the
>>> control line has to be set to?
>>>
>>> With respect to adg792a, it is inline with the current implementation
>>> and the only change I think would be required in the driver is,
>>
>> No, that does not work. See below.
>>
>>>
>>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>>> --- a/drivers/mux/adg792a.c
>>> +++ b/drivers/mux/adg792a.c
>>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>>> ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>>> if (ret < 0)
>>> return ret;
>>> - if (cells >= 2)
>>> - return -EINVAL;
>>>
>>> mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
>>
>> When you add cell #2 with the state, the cells variable will end up
>> as 2 always. Which means that there is no way to alloc one mux
>> control since "cells ? 3 : 1" will always end up as "3", with no
>> easy fix.
>>
>> So, your approach does not work for this driver.
>>
>
> Sorry, but how is this different from the case of
>
> #mux-control-cells = 1
>
> If #mux-control-cells is equal to 1 it means the consumer will use a
> given control line from the mux chip. The same would be the case when we
> will be using, #mux-control-cells is equal to 2, except we can also
> provide the state.
>
> If the consumer will use all the lines then #mux-control-cells will be
> set to 0. In this condition the state can't be provided from the DT and
> the consumer will be controlling the entire mux chip. If
> #mux-control-cells is greater than 0 then we will not be able to provide
> multiple lines of control using a single mux-controls entry(mux-controls
> = <...>;) right? We would have the using multiple mux-controls
> entries(mux-controls = <...>, <...>;).
I think you misunderstand. The adg792a driver operates the chip in
different modes depending on if you specify 0 or 1 cells. With 0,
it's not just that the consumer operates three muxes. It is also, and
more importantly, that the three muxes are operated in parallel without
the consumer doing anything different with the single mux control it
sees (even if there are three muxes operated by that single mux
control).
That said, yes, you can make it limp along like you describe above.
But why should it not be possible to specify a specific state when
the adg792a driver operates the muxes in parallel? And yes, you could
add some other flag to indicate this mode, but my point is that it
is silly to add special cases like this if you don't need to. Since
adding a specific state is the new thing, that is what should be
added in a way that fits with the old stuff without imposing new
flags on that old stuff.
An example: the three muxes in an adg792a chip could be used as
two muxes for some I2C bus (SCL and SDA signals) and the third mux
for something unrelated. Suppose that you want to operate the adg792a
as three parallel muxes so that you mux SCL and SDA simultaneously
(as is expected by the i2c-mux-gpmux binding, it only expects one
mux control), and that you want to use the third mux as the enable-
state for your phy. With your suggested binding you cannot, unless
you add a mechanism to make the adg792a driver operate its muxes in
parallel even if there are two cells instead of zero. I.e. without
that new flag the i2c-mux-gpmux binding needs to see
#mux-control-cells = <0>;
while your new phy binding instead needs to see
#mux-control-cells = <2>;
And you obviously can't have it both ways.
(Sure, it would not be possible to mux the I2C bus while the phy
is enabled in the above example, but there could be some other
limitation in place that makes that invalid anyway. And it's just
an example anyway...)
A mux-control is potentially a shared resource, and bindings have
to take this into account.
Cheers,
Peter
Hi Peter,
On 29/11/21 5:58 pm, Peter Rosin wrote:
> Hi Aswath,
>
> On 2021-11-29 10:31, Aswath Govindraju wrote:
>> Hi Peter,
>>
>> On 29/11/21 1:45 pm, Peter Rosin wrote:
>>>
>>>
>>> On 2021-11-29 05:36, Aswath Govindraju wrote:
>>>> Hi Peter,
>>>>
>>>> On 25/11/21 7:05 pm, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> You need to have some description on how #mux-control-cells now work.
>>>>> The previous description is in mux-consumer.yaml and an update there
>>>>> is needed.
>>>>>
>>>>> However, I have realized that the adg792a binding uses #mux-control-cells
>>>>> to indicate if it should expose its three muxes with one mux-control
>>>>> and operate the muxes in parallel, or if it should be expose three
>>>>> independent mux-controls. So, the approach in this series to always
>>>>> have the #mux-control-cells property fixed at <2> when indicating a
>>>>> state will not work for that binding. And I see no fix for that binding
>>>>> without adding a new property.
>>>>>
>>>>> So, I would like a different approach. Since I dislike how mux-controls
>>>>> -after this series- is not (always) specifying a mux-control like the name
>>>>> says, but instead optionally a specific state, the new property I would
>>>>> like to add is #mux-state-cells such that it would always be one more
>>>>> than #mux-control-cells.
>>>>>
>>>>> mux: mux-controller {
>>>>> compatible = "gpio-mux";
>>>>> #mux-control-cells = <0>;
>>>>> #mux-state-cells = <1>;
>>>>>
>>>>> mux-gpios = <...>;
>>>>> };
>>>>>
>>>>> can-phy {
>>>>> compatible = "ti,tcan1043";
>>>>> ...
>>>>> mux-states = <&mux 1>;
>>>>> };
>>>>>
>>>>> That solves the naming issue, the unused argument for mux-conrtrollers
>>>>> that previously had #mux-control-cells = <0>, and the binding for adg792a
>>>>> need no longer be inconsistent.
>>>>>
>>>>> Or, how should this be solved? I'm sure there are other options...
>>>>>
>>>>
>>>>
>>>> I feel that the new approach using mux-state-cells seems to be
>>>> overpopulating the device tree nodes, when the state can be represented
>>>> using the control cells. I understand that the definition for
>>>> mux-controls is to only specify the control line to be used in a given
>>>> mux. Can't it now be upgraded to also represent the state at which the
>>>> control line has to be set to?
>>>>
>>>> With respect to adg792a, it is inline with the current implementation
>>>> and the only change I think would be required in the driver is,
>>>
>>> No, that does not work. See below.
>>>
>>>>
>>>> diff --git a/drivers/mux/adg792a.c b/drivers/mux/adg792a.c
>>>> index e8fc2fc1ab09..2cd3bb8a40d4 100644
>>>> --- a/drivers/mux/adg792a.c
>>>> +++ b/drivers/mux/adg792a.c
>>>> @@ -73,8 +73,6 @@ static int adg792a_probe(struct i2c_client *i2c)
>>>> ret = device_property_read_u32(dev, "#mux-control-cells", &cells);
>>>> if (ret < 0)
>>>> return ret;
>>>> - if (cells >= 2)
>>>> - return -EINVAL;
>>>>
>>>> mux_chip = devm_mux_chip_alloc(dev, cells ? 3 : 1, 0);
>>>
>>> When you add cell #2 with the state, the cells variable will end up
>>> as 2 always. Which means that there is no way to alloc one mux
>>> control since "cells ? 3 : 1" will always end up as "3", with no
>>> easy fix.
>>>
>>> So, your approach does not work for this driver.
>>>
>>
>> Sorry, but how is this different from the case of
>>
>> #mux-control-cells = 1
>>
>> If #mux-control-cells is equal to 1 it means the consumer will use a
>> given control line from the mux chip. The same would be the case when we
>> will be using, #mux-control-cells is equal to 2, except we can also
>> provide the state.
>>
>> If the consumer will use all the lines then #mux-control-cells will be
>> set to 0. In this condition the state can't be provided from the DT and
>> the consumer will be controlling the entire mux chip. If
>> #mux-control-cells is greater than 0 then we will not be able to provide
>> multiple lines of control using a single mux-controls entry(mux-controls
>> = <...>;) right? We would have the using multiple mux-controls
>> entries(mux-controls = <...>, <...>;).
>
> I think you misunderstand. The adg792a driver operates the chip in
> different modes depending on if you specify 0 or 1 cells. With 0,
> it's not just that the consumer operates three muxes. It is also, and
> more importantly, that the three muxes are operated in parallel without
> the consumer doing anything different with the single mux control it
> sees (even if there are three muxes operated by that single mux
> control).
>
> That said, yes, you can make it limp along like you describe above.
> But why should it not be possible to specify a specific state when
> the adg792a driver operates the muxes in parallel? And yes, you could
> add some other flag to indicate this mode, but my point is that it
> is silly to add special cases like this if you don't need to. Since
> adding a specific state is the new thing, that is what should be
> added in a way that fits with the old stuff without imposing new
> flags on that old stuff.
>
> An example: the three muxes in an adg792a chip could be used as
> two muxes for some I2C bus (SCL and SDA signals) and the third mux
> for something unrelated. Suppose that you want to operate the adg792a
> as three parallel muxes so that you mux SCL and SDA simultaneously
> (as is expected by the i2c-mux-gpmux binding, it only expects one
> mux control), and that you want to use the third mux as the enable-
> state for your phy. With your suggested binding you cannot, unless
> you add a mechanism to make the adg792a driver operate its muxes in
> parallel even if there are two cells instead of zero. I.e. without
> that new flag the i2c-mux-gpmux binding needs to see
>
> #mux-control-cells = <0>;
>
> while your new phy binding instead needs to see
>
> #mux-control-cells = <2>;
>
> And you obviously can't have it both ways.
>
> (Sure, it would not be possible to mux the I2C bus while the phy
> is enabled in the above example, but there could be some other
> limitation in place that makes that invalid anyway. And it's just
> an example anyway...)
>
> A mux-control is potentially a shared resource, and bindings have
> to take this into account.
>
Understood. Thank you for the explanation. Will correct the
implementation and post a respin.
Regards,
Aswath
> Cheers,
> Peter
>