[PATCH RFC v2 0/4] MUX: Add support for reading enable state from DT

[PATCH RFC v2 0/4] MUX: Add support for reading enable state from DT

[Aswath Govindraju]

- 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 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 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                            | 20 ++++++++++++--
 drivers/phy/phy-can-transceiver.c             | 26 +++++++++++++++++++
 include/linux/mux/consumer.h                  |  1 +
 include/linux/mux/driver.h                    |  1 +
 7 files changed, 56 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH RFC v2 1/4] dt-bindings: mux: Increase the number of arguments in mux-controls

[Aswath Govindraju]

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 <a-govindraju@ti.com>
---
 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

[PATCH RFC v2 2/4] dt-bindings: phy: ti,tcan104x-can: Document mux-controls property

[Aswath Govindraju]

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 <a-govindraju@ti.com>
---
 .../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

[PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

In some cases, we might need to provide the state of the mux to be set for
the operation of a given peripheral. Therefore, pass this information using
the second argument of the mux-controls property.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---

Notes:
- The function mux_control_get() always return the mux_control for a single
  line. So, control for mutiple lines cannot be represented in the
  mux-controls property.
- For representing multiple lines of control, multiple entries need to be
  used along with mux-names for reading them.
- If a device uses both the states of the mux line then #mux-control-cells
  can be set to 1 and enable_state will not be set in this case.

 drivers/mux/core.c           | 20 ++++++++++++++++++--
 include/linux/mux/consumer.h |  1 +
 include/linux/mux/driver.h   |  1 +
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 22f4709768d1..51140748d2d6 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -294,6 +294,18 @@ unsigned int mux_control_states(struct mux_control *mux)
 }
 EXPORT_SYMBOL_GPL(mux_control_states);
 
+/**
+ * mux_control_enable_state() - Query for the enable state.
+ * @mux: The mux-control to query.
+ *
+ * Return: State to be set in the mux to enable a given device
+ */
+unsigned int mux_control_enable_state(struct mux_control *mux)
+{
+	return mux->enable_state;
+}
+EXPORT_SYMBOL_GPL(mux_control_enable_state);
+
 /*
  * The mux->lock must be down when calling this function.
  */
@@ -481,8 +493,7 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	if (!mux_chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	if (args.args_count > 1 ||
-	    (!args.args_count && (mux_chip->controllers > 1))) {
+	if (!args.args_count && mux_chip->controllers > 1) {
 		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
 			np, args.np);
 		put_device(&mux_chip->dev);
@@ -500,6 +511,11 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (args.args_count == 2) {
+		mux_chip->mux[controller].enable_state = args.args[1];
+		mux_chip->mux[controller].idle_state = !args.args[1];
+	}
+
 	return &mux_chip->mux[controller];
 }
 EXPORT_SYMBOL_GPL(mux_control_get);
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 7a09b040ac39..cb861eab8aad 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -16,6 +16,7 @@ struct device;
 struct mux_control;
 
 unsigned int mux_control_states(struct mux_control *mux);
+unsigned int mux_control_enable_state(struct mux_control *mux);
 int __must_check mux_control_select_delay(struct mux_control *mux,
 					  unsigned int state,
 					  unsigned int delay_us);
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..7db378dabdb2 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -48,6 +48,7 @@ struct mux_control {
 	int cached_state;
 
 	unsigned int states;
+	unsigned int enable_state;
 	int idle_state;
 
 	ktime_t last_change;
-- 
2.17.1

Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT

[Peter Rosin]

Hi!

On 2021-11-22 13:56, Aswath Govindraju wrote:
> In some cases, we might need to provide the state of the mux to be set for
> the operation of a given peripheral. Therefore, pass this information using
> the second argument of the mux-controls property.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
> 
> Notes:
> - The function mux_control_get() always return the mux_control for a single
>   line. So, control for mutiple lines cannot be represented in the
>   mux-controls property.
> - For representing multiple lines of control, multiple entries need to be
>   used along with mux-names for reading them.
> - If a device uses both the states of the mux line then #mux-control-cells
>   can be set to 1 and enable_state will not be set in this case.
> 
>  drivers/mux/core.c           | 20 ++++++++++++++++++--
>  include/linux/mux/consumer.h |  1 +
>  include/linux/mux/driver.h   |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 22f4709768d1..51140748d2d6 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -294,6 +294,18 @@ unsigned int mux_control_states(struct mux_control *mux)
>  }
>  EXPORT_SYMBOL_GPL(mux_control_states);
>  
> +/**
> + * mux_control_enable_state() - Query for the enable state.
> + * @mux: The mux-control to query.
> + *
> + * Return: State to be set in the mux to enable a given device
> + */
> +unsigned int mux_control_enable_state(struct mux_control *mux)
> +{
> +	return mux->enable_state;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_enable_state);
> +
>  /*
>   * The mux->lock must be down when calling this function.
>   */
> @@ -481,8 +493,7 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  	if (!mux_chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	if (args.args_count > 1 ||
> -	    (!args.args_count && (mux_chip->controllers > 1))) {
> +	if (!args.args_count && mux_chip->controllers > 1) {
>  		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
>  			np, args.np);
>  		put_device(&mux_chip->dev);
> @@ -500,6 +511,11 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (args.args_count == 2) {
> +		mux_chip->mux[controller].enable_state = args.args[1];
> +		mux_chip->mux[controller].idle_state = !args.args[1];

Please leave the idle state alone. The idle state is a property of
the mux-control itself, and not the business of any particular
consumer. Consumers can only say what state the mux control should
have when they have selected the mux-control, and have no say about
what state the mux-control has when they do not have it selected.
There is no conflict with having the same idle state as the state the
mux would normally have. That could even be seen as an optimization,
since then there might be no need to operate the mux for most
accesses.

> +	}
> +
>  	return &mux_chip->mux[controller];
>  }
>  EXPORT_SYMBOL_GPL(mux_control_get);
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 7a09b040ac39..cb861eab8aad 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -16,6 +16,7 @@ struct device;
>  struct mux_control;
>  
>  unsigned int mux_control_states(struct mux_control *mux);
> +unsigned int mux_control_enable_state(struct mux_control *mux);
>  int __must_check mux_control_select_delay(struct mux_control *mux,
>  					  unsigned int state,
>  					  unsigned int delay_us);
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..7db378dabdb2 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -48,6 +48,7 @@ struct mux_control {
>  	int cached_state;
>  
>  	unsigned int states;
> +	unsigned int enable_state;

This is the wrong place to store the state you need. The mux_control
is a shared resource and can have many consumers. Storing the needed
value for exactly one consumer in the mux control is therefore
broken. It would get overwritten when consumer #2 (and 3 etc etc)
wants to use some other state from the same shared mux control.

Doing this properly means that you need a new struct tying together
a mux-control and a state. With an API looking something like this:

struct mux_state {
	struct mux_control *mux;
	unsigned int state;
};

struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
{
	struct mux_state *mux_state = kzalloc(sizeof(*mux_state), GFP_KERNEL);

	if (!mux_state)
		return ERR_PTR(-ENOMEM);

	mux_state->mux = ...; /* mux_control_get(...) perhaps? */
	/* error checking and recovery, etc etc etc */
	mux_state->state = ...;

	return mux_state;
}

void mux_state_put(struct mux_state *mux_state)
{
	mux_control_put(mux_state->mux);
	free(mux_state);
}

int mux_state_select_delay(struct mux_state *mux_state,
 			   unsigned int delay_us)
{
	return mux_control_select_delay(mux_state->mux, mux_state->state,
					delay_us);
}

int mux_state_select(struct mux_state *mux_state)
{
	return mux_state_select_delay(mux_state, 0);
}

int mux_state_try_select_delay(struct mux_state *mux_state)
			       unsigned int delay_us);
{
	return mux_control_try_select_delay(mux_state->mux, mux_state->state,
					    delay_us);
}

int mux_state_try_select(struct mux_state *mux_state)
{
	return mux_state_try_select_delay(mux_state, 0);
}

int mux_state_deselect(struct mux_control *mux)
{
	return mux_control_deselect(mux_state->mux);
}

(written directly in the mail client, never compiled, here be dragons)

mux_state_get is obviously the difficult function to write, and
the above call to mux_control_get is not appropriate as-is. I
think mux_control_get perhaps needs to be refactored into a
flexible helper that takes a couple of extra arguments that
indicate if you want an optional get and/or a particular state.
And then mux_control_get can just be a wrapper around that helper.
Adding mux_control_get_optional would be a matter of adding a new
wrapper. And the mux_state->mux assignment above would need yet
another wrapper for the flexible helper, one that also make the
flexible helper return the requested state from the mux-control
property.

I realize that this might be a big piece to chew, but you want to
do something new here, and I think it is best to do it right from
the start instead of having weird code that only makes it harder
to do it right later. Ad it's not that complicated.

Cheers,
Peter

>  	int idle_state;
>  
>  	ktime_t last_change;
> 

Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

Hi Peter,

On 23/11/21 12:29 am, Peter Rosin wrote:
> Hi!
> 
> On 2021-11-22 13:56, Aswath Govindraju wrote:
>> In some cases, we might need to provide the state of the mux to be set for
>> the operation of a given peripheral. Therefore, pass this information using
>> the second argument of the mux-controls property.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>
>> Notes:
>> - The function mux_control_get() always return the mux_control for a single
>>   line. So, control for mutiple lines cannot be represented in the
>>   mux-controls property.
>> - For representing multiple lines of control, multiple entries need to be
>>   used along with mux-names for reading them.
>> - If a device uses both the states of the mux line then #mux-control-cells
>>   can be set to 1 and enable_state will not be set in this case.
>>
>>  drivers/mux/core.c           | 20 ++++++++++++++++++--
>>  include/linux/mux/consumer.h |  1 +
>>  include/linux/mux/driver.h   |  1 +
>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 22f4709768d1..51140748d2d6 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -294,6 +294,18 @@ unsigned int mux_control_states(struct mux_control *mux)
>>  }
>>  EXPORT_SYMBOL_GPL(mux_control_states);
>>  
>> +/**
>> + * mux_control_enable_state() - Query for the enable state.
>> + * @mux: The mux-control to query.
>> + *
>> + * Return: State to be set in the mux to enable a given device
>> + */
>> +unsigned int mux_control_enable_state(struct mux_control *mux)
>> +{
>> +	return mux->enable_state;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_enable_state);
>> +
>>  /*
>>   * The mux->lock must be down when calling this function.
>>   */
>> @@ -481,8 +493,7 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>  	if (!mux_chip)
>>  		return ERR_PTR(-EPROBE_DEFER);
>>  
>> -	if (args.args_count > 1 ||
>> -	    (!args.args_count && (mux_chip->controllers > 1))) {
>> +	if (!args.args_count && mux_chip->controllers > 1) {
>>  		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
>>  			np, args.np);
>>  		put_device(&mux_chip->dev);
>> @@ -500,6 +511,11 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>  		return ERR_PTR(-EINVAL);
>>  	}
>>  
>> +	if (args.args_count == 2) {
>> +		mux_chip->mux[controller].enable_state = args.args[1];
>> +		mux_chip->mux[controller].idle_state = !args.args[1];
> 
> Please leave the idle state alone. The idle state is a property of
> the mux-control itself, and not the business of any particular
> consumer. Consumers can only say what state the mux control should
> have when they have selected the mux-control, and have no say about
> what state the mux-control has when they do not have it selected.
> There is no conflict with having the same idle state as the state the
> mux would normally have. That could even be seen as an optimization,
> since then there might be no need to operate the mux for most
> accesses.
> 

Got it. Will not touch idle_state.

>> +	}
>> +
>>  	return &mux_chip->mux[controller];
>>  }
>>  EXPORT_SYMBOL_GPL(mux_control_get);
>> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
>> index 7a09b040ac39..cb861eab8aad 100644
>> --- a/include/linux/mux/consumer.h
>> +++ b/include/linux/mux/consumer.h
>> @@ -16,6 +16,7 @@ struct device;
>>  struct mux_control;
>>  
>>  unsigned int mux_control_states(struct mux_control *mux);
>> +unsigned int mux_control_enable_state(struct mux_control *mux);
>>  int __must_check mux_control_select_delay(struct mux_control *mux,
>>  					  unsigned int state,
>>  					  unsigned int delay_us);
>> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
>> index 18824064f8c0..7db378dabdb2 100644
>> --- a/include/linux/mux/driver.h
>> +++ b/include/linux/mux/driver.h
>> @@ -48,6 +48,7 @@ struct mux_control {
>>  	int cached_state;
>>  
>>  	unsigned int states;
>> +	unsigned int enable_state;
> 
> This is the wrong place to store the state you need. The mux_control
> is a shared resource and can have many consumers. Storing the needed
> value for exactly one consumer in the mux control is therefore
> broken. It would get overwritten when consumer #2 (and 3 etc etc)
> wants to use some other state from the same shared mux control.
> 

Sorry, forgot the distinction that multiple consumers can get the mux
and only one can select the mux.

> Doing this properly means that you need a new struct tying together
> a mux-control and a state. With an API looking something like this:
> 
> struct mux_state {
> 	struct mux_control *mux;
> 	unsigned int state;
> };
> 
> struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
> {
> 	struct mux_state *mux_state = kzalloc(sizeof(*mux_state), GFP_KERNEL);
> 
> 	if (!mux_state)
> 		return ERR_PTR(-ENOMEM);
> 
> 	mux_state->mux = ...; /* mux_control_get(...) perhaps? */
> 	/* error checking and recovery, etc etc etc */
> 	mux_state->state = ...;
> 
> 	return mux_state;
> }
> 
> void mux_state_put(struct mux_state *mux_state)
> {
> 	mux_control_put(mux_state->mux);
> 	free(mux_state);
> }
> 
> int mux_state_select_delay(struct mux_state *mux_state,
>  			   unsigned int delay_us)
> {
> 	return mux_control_select_delay(mux_state->mux, mux_state->state,
> 					delay_us);
> }
> 
> int mux_state_select(struct mux_state *mux_state)
> {
> 	return mux_state_select_delay(mux_state, 0);
> }
> 
> int mux_state_try_select_delay(struct mux_state *mux_state)
> 			       unsigned int delay_us);
> {
> 	return mux_control_try_select_delay(mux_state->mux, mux_state->state,
> 					    delay_us);
> }
> 
> int mux_state_try_select(struct mux_state *mux_state)
> {
> 	return mux_state_try_select_delay(mux_state, 0);
> }
> 
> int mux_state_deselect(struct mux_control *mux)
> {
> 	return mux_control_deselect(mux_state->mux);
> }
> 
> (written directly in the mail client, never compiled, here be dragons)
> 
> mux_state_get is obviously the difficult function to write, and
> the above call to mux_control_get is not appropriate as-is. I

I am sorry but I did not understand why mux_control_get is not
appropriate. We should be able to call the function and get the mux right?

> think mux_control_get perhaps needs to be refactored into a
> flexible helper that takes a couple of extra arguments that
> indicate if you want an optional get and/or a particular state.
> And then mux_control_get can just be a wrapper around that helper.
> Adding mux_control_get_optional would be a matter of adding a new
> wrapper. And the mux_state->mux assignment above would need yet
> another wrapper for the flexible helper, one that also make the
> flexible helper return the requested state from the mux-control
> property.
> 

The problem that I see with the optional apis as wrappers around
mux_control_get is the following print in mux_control_get,

dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",

This gets printed whenever it can't find mux-controls device tree property.


Thank you providing your comments and reference implementation.

Regards,
Aswath

> I realize that this might be a big piece to chew, but you want to
> do something new here, and I think it is best to do it right from
> the start instead of having weird code that only makes it harder
> to do it right later. Ad it's not that complicated.
> 
> Cheers,
> Peter
> 
>>  	int idle_state;
>>  
>>  	ktime_t last_change;
>>

Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju]

Hi Peter,

On 23/11/21 9:44 am, Aswath Govindraju wrote:
> Hi Peter,
> 
> On 23/11/21 12:29 am, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-22 13:56, Aswath Govindraju wrote:
>>> In some cases, we might need to provide the state of the mux to be set for
>>> the operation of a given peripheral. Therefore, pass this information using
>>> the second argument of the mux-controls property.
>>>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>> ---
>>>
>>> Notes:
>>> - The function mux_control_get() always return the mux_control for a single
>>>   line. So, control for mutiple lines cannot be represented in the
>>>   mux-controls property.
>>> - For representing multiple lines of control, multiple entries need to be
>>>   used along with mux-names for reading them.
>>> - If a device uses both the states of the mux line then #mux-control-cells
>>>   can be set to 1 and enable_state will not be set in this case.
>>>
>>>  drivers/mux/core.c           | 20 ++++++++++++++++++--
>>>  include/linux/mux/consumer.h |  1 +
>>>  include/linux/mux/driver.h   |  1 +
>>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>> index 22f4709768d1..51140748d2d6 100644
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -294,6 +294,18 @@ unsigned int mux_control_states(struct mux_control *mux)
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_states);
>>>  
>>> +/**
>>> + * mux_control_enable_state() - Query for the enable state.
>>> + * @mux: The mux-control to query.
>>> + *
>>> + * Return: State to be set in the mux to enable a given device
>>> + */
>>> +unsigned int mux_control_enable_state(struct mux_control *mux)
>>> +{
>>> +	return mux->enable_state;
>>> +}
>>> +EXPORT_SYMBOL_GPL(mux_control_enable_state);
>>> +
>>>  /*
>>>   * The mux->lock must be down when calling this function.
>>>   */
>>> @@ -481,8 +493,7 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>  	if (!mux_chip)
>>>  		return ERR_PTR(-EPROBE_DEFER);
>>>  
>>> -	if (args.args_count > 1 ||
>>> -	    (!args.args_count && (mux_chip->controllers > 1))) {
>>> +	if (!args.args_count && mux_chip->controllers > 1) {
>>>  		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
>>>  			np, args.np);
>>>  		put_device(&mux_chip->dev);
>>> @@ -500,6 +511,11 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>  		return ERR_PTR(-EINVAL);
>>>  	}
>>>  
>>> +	if (args.args_count == 2) {
>>> +		mux_chip->mux[controller].enable_state = args.args[1];
>>> +		mux_chip->mux[controller].idle_state = !args.args[1];
>>
>> Please leave the idle state alone. The idle state is a property of
>> the mux-control itself, and not the business of any particular
>> consumer. Consumers can only say what state the mux control should
>> have when they have selected the mux-control, and have no say about
>> what state the mux-control has when they do not have it selected.
>> There is no conflict with having the same idle state as the state the
>> mux would normally have. That could even be seen as an optimization,
>> since then there might be no need to operate the mux for most
>> accesses.
>>
> 
> Got it. Will not touch idle_state.
> 
>>> +	}
>>> +
>>>  	return &mux_chip->mux[controller];
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_get);
>>> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
>>> index 7a09b040ac39..cb861eab8aad 100644
>>> --- a/include/linux/mux/consumer.h
>>> +++ b/include/linux/mux/consumer.h
>>> @@ -16,6 +16,7 @@ struct device;
>>>  struct mux_control;
>>>  
>>>  unsigned int mux_control_states(struct mux_control *mux);
>>> +unsigned int mux_control_enable_state(struct mux_control *mux);
>>>  int __must_check mux_control_select_delay(struct mux_control *mux,
>>>  					  unsigned int state,
>>>  					  unsigned int delay_us);
>>> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
>>> index 18824064f8c0..7db378dabdb2 100644
>>> --- a/include/linux/mux/driver.h
>>> +++ b/include/linux/mux/driver.h
>>> @@ -48,6 +48,7 @@ struct mux_control {
>>>  	int cached_state;
>>>  
>>>  	unsigned int states;
>>> +	unsigned int enable_state;
>>
>> This is the wrong place to store the state you need. The mux_control
>> is a shared resource and can have many consumers. Storing the needed
>> value for exactly one consumer in the mux control is therefore
>> broken. It would get overwritten when consumer #2 (and 3 etc etc)
>> wants to use some other state from the same shared mux control.
>>
> 
> Sorry, forgot the distinction that multiple consumers can get the mux
> and only one can select the mux.
> 
>> Doing this properly means that you need a new struct tying together
>> a mux-control and a state. With an API looking something like this:
>>
>> struct mux_state {
>> 	struct mux_control *mux;
>> 	unsigned int state;
>> };
>>
>> struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
>> {
>> 	struct mux_state *mux_state = kzalloc(sizeof(*mux_state), GFP_KERNEL);
>>
>> 	if (!mux_state)
>> 		return ERR_PTR(-ENOMEM);
>>
>> 	mux_state->mux = ...; /* mux_control_get(...) perhaps? */
>> 	/* error checking and recovery, etc etc etc */
>> 	mux_state->state = ...;
>>
>> 	return mux_state;
>> }
>>
>> void mux_state_put(struct mux_state *mux_state)
>> {
>> 	mux_control_put(mux_state->mux);
>> 	free(mux_state);
>> }
>>
>> int mux_state_select_delay(struct mux_state *mux_state,
>>  			   unsigned int delay_us)
>> {
>> 	return mux_control_select_delay(mux_state->mux, mux_state->state,
>> 					delay_us);
>> }
>>
>> int mux_state_select(struct mux_state *mux_state)
>> {
>> 	return mux_state_select_delay(mux_state, 0);
>> }
>>
>> int mux_state_try_select_delay(struct mux_state *mux_state)
>> 			       unsigned int delay_us);
>> {
>> 	return mux_control_try_select_delay(mux_state->mux, mux_state->state,
>> 					    delay_us);
>> }
>>
>> int mux_state_try_select(struct mux_state *mux_state)
>> {
>> 	return mux_state_try_select_delay(mux_state, 0);
>> }
>>
>> int mux_state_deselect(struct mux_control *mux)
>> {
>> 	return mux_control_deselect(mux_state->mux);
>> }
>>
>> (written directly in the mail client, never compiled, here be dragons)
>>
>> mux_state_get is obviously the difficult function to write, and
>> the above call to mux_control_get is not appropriate as-is. I
> 
> I am sorry but I did not understand why mux_control_get is not
> appropriate. We should be able to call the function and get the mux right?
> 

Understood why mux_control_get() is not appropriate. There would be no
way for us to get the information about the state from the DT.

Thanks,
Aswath

>> think mux_control_get perhaps needs to be refactored into a
>> flexible helper that takes a couple of extra arguments that
>> indicate if you want an optional get and/or a particular state.
>> And then mux_control_get can just be a wrapper around that helper.
>> Adding mux_control_get_optional would be a matter of adding a new
>> wrapper. And the mux_state->mux assignment above would need yet
>> another wrapper for the flexible helper, one that also make the
>> flexible helper return the requested state from the mux-control
>> property.
>>
> 
> The problem that I see with the optional apis as wrappers around
> mux_control_get is the following print in mux_control_get,
> 
> dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
> 
> This gets printed whenever it can't find mux-controls device tree property.
> 
> 
> Thank you providing your comments and reference implementation.
> 
> Regards,
> Aswath
> 
>> I realize that this might be a big piece to chew, but you want to
>> do something new here, and I think it is best to do it right from
>> the start instead of having weird code that only makes it harder
>> to do it right later. Ad it's not that complicated.
>>
>> Cheers,
>> Peter
>>
>>>  	int idle_state;
>>>  
>>>  	ktime_t last_change;
>>>
> 

[PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Aswath Govindraju]

On some boards, for routing CAN signals from controller to transceiver,
muxes might need to be set. Therefore, add support for setting the mux by
reading the mux-controls property from the device tree node.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/phy/phy-can-transceiver.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 6f3fe37dee0e..15056b9d68ba 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -10,6 +10,7 @@
 #include<linux/module.h>
 #include<linux/gpio.h>
 #include<linux/gpio/consumer.h>
+#include <linux/mux/consumer.h>
 
 struct can_transceiver_data {
 	u32 flags;
@@ -21,13 +22,23 @@ struct can_transceiver_phy {
 	struct phy *generic_phy;
 	struct gpio_desc *standby_gpio;
 	struct gpio_desc *enable_gpio;
+	struct mux_control *mux_ctrl;
 };
 
 /* Power on function */
 static int can_transceiver_phy_power_on(struct phy *phy)
 {
+	int ret;
 	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
 
+	if (can_transceiver_phy->mux_ctrl) {
+		ret = mux_control_select(can_transceiver_phy->mux_ctrl,
+					 mux_control_enable_state(can_transceiver_phy->mux_ctrl));
+		if (ret) {
+			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
+			return ret;
+		}
+	}
 	if (can_transceiver_phy->standby_gpio)
 		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
 	if (can_transceiver_phy->enable_gpio)
@@ -45,6 +56,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
 		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
 	if (can_transceiver_phy->enable_gpio)
 		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
+	if (can_transceiver_phy->mux_ctrl)
+		mux_control_deselect(can_transceiver_phy->mux_ctrl);
 
 	return 0;
 }
@@ -95,6 +108,19 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
 	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
 	drvdata = match->data;
 
+	if (of_property_read_bool(dev->of_node, "mux-controls")) {
+		struct mux_control *control;
+		int ret;
+
+		control = devm_mux_control_get(dev, NULL);
+		if (IS_ERR(control)) {
+			ret = PTR_ERR(control);
+			dev_err_probe(&pdev->dev, ret, "failed to get mux\n");
+			return PTR_ERR(control);
+		}
+		can_transceiver_phy->mux_ctrl = control;
+	}
+
 	phy = devm_phy_create(dev, dev->of_node,
 			      &can_transceiver_phy_ops);
 	if (IS_ERR(phy)) {
-- 
2.17.1

Re: [PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

On 22.11.2021 18:26:24, Aswath Govindraju wrote:
> On some boards, for routing CAN signals from controller to transceiver,
> muxes might need to be set. Therefore, add support for setting the mux by
> reading the mux-controls property from the device tree node.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/phy/phy-can-transceiver.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..15056b9d68ba 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -10,6 +10,7 @@
>  #include<linux/module.h>
>  #include<linux/gpio.h>
>  #include<linux/gpio/consumer.h>
> +#include <linux/mux/consumer.h>
>  
>  struct can_transceiver_data {
>  	u32 flags;
> @@ -21,13 +22,23 @@ struct can_transceiver_phy {
>  	struct phy *generic_phy;
>  	struct gpio_desc *standby_gpio;
>  	struct gpio_desc *enable_gpio;
> +	struct mux_control *mux_ctrl;
>  };
>  
>  /* Power on function */
>  static int can_transceiver_phy_power_on(struct phy *phy)
>  {
> +	int ret;
>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>  
> +	if (can_transceiver_phy->mux_ctrl) {
> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl,
> +					 mux_control_enable_state(can_transceiver_phy->mux_ctrl));
> +		if (ret) {
> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  	if (can_transceiver_phy->standby_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>  	if (can_transceiver_phy->enable_gpio)
> @@ -45,6 +56,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>  	if (can_transceiver_phy->enable_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
> +	if (can_transceiver_phy->mux_ctrl)
> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>  
>  	return 0;
>  }
> @@ -95,6 +108,19 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>  	drvdata = match->data;
>  
> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
> +		struct mux_control *control;
> +		int ret;
> +
> +		control = devm_mux_control_get(dev, NULL);
> +		if (IS_ERR(control)) {
> +			ret = PTR_ERR(control);
> +			dev_err_probe(&pdev->dev, ret, "failed to get mux\n");
> +			return PTR_ERR(control);
> +		}

	if (IS_ERR(control))
		return dev_err_probe(&pdev, PTR_ERR(control),
                        "failed to get mux\n");

> +		can_transceiver_phy->mux_ctrl = control;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
> -- 
> 2.17.1
> 
>

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]

On 22.11.2021 18:26:24, Aswath Govindraju wrote:
> On some boards, for routing CAN signals from controller to transceiver,
> muxes might need to be set. Therefore, add support for setting the mux by
> reading the mux-controls property from the device tree node.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/phy/phy-can-transceiver.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..15056b9d68ba 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -10,6 +10,7 @@
>  #include<linux/module.h>
>  #include<linux/gpio.h>
>  #include<linux/gpio/consumer.h>
> +#include <linux/mux/consumer.h>
>  
>  struct can_transceiver_data {
>  	u32 flags;
> @@ -21,13 +22,23 @@ struct can_transceiver_phy {
>  	struct phy *generic_phy;
>  	struct gpio_desc *standby_gpio;
>  	struct gpio_desc *enable_gpio;
> +	struct mux_control *mux_ctrl;
>  };
>  
>  /* Power on function */
>  static int can_transceiver_phy_power_on(struct phy *phy)
>  {
> +	int ret;
>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>  
> +	if (can_transceiver_phy->mux_ctrl) {
> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl,
> +					 mux_control_enable_state(can_transceiver_phy->mux_ctrl));
> +		if (ret) {
> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  	if (can_transceiver_phy->standby_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>  	if (can_transceiver_phy->enable_gpio)
> @@ -45,6 +56,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>  	if (can_transceiver_phy->enable_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
> +	if (can_transceiver_phy->mux_ctrl)
> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>  
>  	return 0;
>  }
> @@ -95,6 +108,19 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>  	drvdata = match->data;
>  
> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
> +		struct mux_control *control;
> +		int ret;
> +
> +		control = devm_mux_control_get(dev, NULL);
> +		if (IS_ERR(control)) {
> +			ret = PTR_ERR(control);
> +			dev_err_probe(&pdev->dev, ret, "failed to get mux\n");
> +			return PTR_ERR(control);
> +		}
> +		can_transceiver_phy->mux_ctrl = control;
> +	}

What about adding a devm_mux_control_get_optional(), which doesn't
return a -ENODEV but a NULL pointer if the device doesn't exist?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Aswath Govindraju]

Hi Marc,

On 22/11/21 6:42 pm, Marc Kleine-Budde wrote:
> On 22.11.2021 18:26:24, Aswath Govindraju wrote:
>> On some boards, for routing CAN signals from controller to transceiver,
>> muxes might need to be set. Therefore, add support for setting the mux by
>> reading the mux-controls property from the device tree node.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/phy/phy-can-transceiver.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> index 6f3fe37dee0e..15056b9d68ba 100644
>> --- a/drivers/phy/phy-can-transceiver.c
>> +++ b/drivers/phy/phy-can-transceiver.c
>> @@ -10,6 +10,7 @@
>>  #include<linux/module.h>
>>  #include<linux/gpio.h>
>>  #include<linux/gpio/consumer.h>
>> +#include <linux/mux/consumer.h>
>>  
>>  struct can_transceiver_data {
>>  	u32 flags;
>> @@ -21,13 +22,23 @@ struct can_transceiver_phy {
>>  	struct phy *generic_phy;
>>  	struct gpio_desc *standby_gpio;
>>  	struct gpio_desc *enable_gpio;
>> +	struct mux_control *mux_ctrl;
>>  };
>>  
>>  /* Power on function */
>>  static int can_transceiver_phy_power_on(struct phy *phy)
>>  {
>> +	int ret;
>>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>>  
>> +	if (can_transceiver_phy->mux_ctrl) {
>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl,
>> +					 mux_control_enable_state(can_transceiver_phy->mux_ctrl));
>> +		if (ret) {
>> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>  	if (can_transceiver_phy->standby_gpio)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>>  	if (can_transceiver_phy->enable_gpio)
>> @@ -45,6 +56,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>>  	if (can_transceiver_phy->enable_gpio)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
>> +	if (can_transceiver_phy->mux_ctrl)
>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>  
>>  	return 0;
>>  }
>> @@ -95,6 +108,19 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>>  	drvdata = match->data;
>>  
>> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
>> +		struct mux_control *control;
>> +		int ret;
>> +
>> +		control = devm_mux_control_get(dev, NULL);
>> +		if (IS_ERR(control)) {
>> +			ret = PTR_ERR(control);
>> +			dev_err_probe(&pdev->dev, ret, "failed to get mux\n");
>> +			return PTR_ERR(control);
>> +		}
>> +		can_transceiver_phy->mux_ctrl = control;
>> +	}
> 
> What about adding a devm_mux_control_get_optional(), which doesn't
> return a -ENODEV but a NULL pointer if the device doesn't exist?
> 

I tried adding it in the following manner,

+/**
+ * devm_mux_control_optional_get() - Optionally get the mux-control for a
+ *                                  device, with resource management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * This differs from devm_mux_control_get in that if the mux does not
+ * exist, it is not considered an error and -ENODEV will not be
+ * returned. Instead the NULL is returned.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *devm_mux_control_optional_get(struct device *dev,
+                                                 const char *mux_name)
+{
+       struct mux_control *mux_ctrl;
+
+       mux_ctrl = devm_mux_control_get(dev, mux_name);
+       if (PTR_ERR(mux_ctrl) == -ENOENT)
+                mux_ctrl = NULL;
+
+       return mux_ctrl;
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_optional_get);
+

However the issue is that there is a print in mux_control_get()
 dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",

which is getting printed, whenever mux-controls property is not found.
Therefore, I was not sure about how to go about this issue and did not
implement it.

Thanks,
Aswath

> Marc
> 

Re: [PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

On 22.11.2021 18:50:00, Aswath Govindraju wrote:
> > What about adding a devm_mux_control_get_optional(), which doesn't
> > return a -ENODEV but a NULL pointer if the device doesn't exist?
> > 
> 
> I tried adding it in the following manner,
> 
> +/**
> + * devm_mux_control_optional_get() - Optionally get the mux-control for a
> + *                                  device, with resource management.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * This differs from devm_mux_control_get in that if the mux does not
> + * exist, it is not considered an error and -ENODEV will not be
> + * returned. Instead the NULL is returned.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_optional_get(struct device *dev,
> +                                                 const char *mux_name)
> +{
> +       struct mux_control *mux_ctrl;
> +
> +       mux_ctrl = devm_mux_control_get(dev, mux_name);
> +       if (PTR_ERR(mux_ctrl) == -ENOENT)
> +                mux_ctrl = NULL;
> +
> +       return mux_ctrl;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_optional_get);
> +
> 
> However the issue is that there is a print in mux_control_get()
>  dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
> 
> which is getting printed, whenever mux-controls property is not found.
> Therefore, I was not sure about how to go about this issue and did not
> implement it.

Ok, this would require more tweaking in the mux layer. Then leave it as
is.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH RFC v2 4/4] phy: phy-can-transceiver: Add support for setting mux

[Kishon Vijay Abraham I]

Hi Aswath,

On 22/11/21 6:26 pm, Aswath Govindraju wrote:
> On some boards, for routing CAN signals from controller to transceiver,
> muxes might need to be set. Therefore, add support for setting the mux by
> reading the mux-controls property from the device tree node.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/phy/phy-can-transceiver.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..15056b9d68ba 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -10,6 +10,7 @@
>  #include<linux/module.h>
>  #include<linux/gpio.h>
>  #include<linux/gpio/consumer.h>
> +#include <linux/mux/consumer.h>
>  
>  struct can_transceiver_data {
>  	u32 flags;
> @@ -21,13 +22,23 @@ struct can_transceiver_phy {
>  	struct phy *generic_phy;
>  	struct gpio_desc *standby_gpio;
>  	struct gpio_desc *enable_gpio;
> +	struct mux_control *mux_ctrl;
>  };
>  
>  /* Power on function */
>  static int can_transceiver_phy_power_on(struct phy *phy)
>  {
> +	int ret;
>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>  
> +	if (can_transceiver_phy->mux_ctrl) {
> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl,
> +					 mux_control_enable_state(can_transceiver_phy->mux_ctrl));

Would need 'select MULTIPLEXER' in Kconfig.

Thanks,
Kishon
> +		if (ret) {
> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
> +			return ret;
> +		}
> +	}
>  	if (can_transceiver_phy->standby_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>  	if (can_transceiver_phy->enable_gpio)
> @@ -45,6 +56,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>  	if (can_transceiver_phy->enable_gpio)
>  		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
> +	if (can_transceiver_phy->mux_ctrl)
> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>  
>  	return 0;
>  }
> @@ -95,6 +108,19 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>  	drvdata = match->data;
>  
> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
> +		struct mux_control *control;
> +		int ret;
> +
> +		control = devm_mux_control_get(dev, NULL);
> +		if (IS_ERR(control)) {
> +			ret = PTR_ERR(control);
> +			dev_err_probe(&pdev->dev, ret, "failed to get mux\n");
> +			return PTR_ERR(control);
> +		}
> +		can_transceiver_phy->mux_ctrl = control;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
> 

Re: [PATCH RFC v2 0/4] MUX: Add support for reading enable state from DT

[Aswath Govindraju]

Hi All,

On 22/11/21 6:26 pm, Aswath Govindraju wrote:
> - 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 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 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
> 

Thank you for the comments. I have made changes based on the comments
received and posted a v3 of this series.

Regards,
Aswath

>  .../devicetree/bindings/mux/gpio-mux.yaml     |  2 +-
>  .../bindings/mux/mux-controller.yaml          |  2 +-
>  .../bindings/phy/ti,tcan104x-can.yaml         |  8 ++++++
>  drivers/mux/core.c                            | 20 ++++++++++++--
>  drivers/phy/phy-can-transceiver.c             | 26 +++++++++++++++++++
>  include/linux/mux/consumer.h                  |  1 +
>  include/linux/mux/driver.h                    |  1 +
>  7 files changed, 56 insertions(+), 4 deletions(-)
>