[PATCH 0/7] Add Mule I2C multiplexer support

[PATCH 0/7] Add Mule I2C multiplexer support

[Farouk Bouabid]

Mule is an mcu that emulates a set of i2c devices which are reacheable
through an i2c-mux.

The emulated devices share a single i2c address with the mux itself where
the requested register is what determines which logic is executed (mux or
device):

1- The devices on the mux can be selected (mux function) by writing the
appropriate device number to an i2c config register (0xff) that is not
used by any device logic.

2- Any access to a register other than the config register will be
handled by the previously selected device.

      +-------------------------------------------------------+
      |  Mule                                                 |
      |        +---------------+                              |
    ----+-(1)->|Config register|-----+                        |
      | |      +---------------+     |                        |
      | |                            V_                       |
      | |                            |  \          +--------+ |
      | |                            |   \-------->| dev #0 | |
      | |                            |   |         +--------+ |
      | |                            | M |-------->| dev #1 | |
      | +-----------(2)------------->| U |         +--------+ |
      |                              | X |-------->| dev #2 | |
      |                              |   |         +--------+ |
      |                              |   /-------->| dev #3 | |
      |                              |__/          +--------+ |
      +-------------------------------------------------------+

The current i2c-mux implementation does not allow the mux to share the i2c
address of a child device. As a workaround, when creating each i2c child
adapter we do not assign the parent adapter to avoid the address-match with
the mux.

This patch-series adds support for this multiplexer. Mule is integrated
as part of rk3399-puma, px30-ringneck, rk3588-tiger and rk3588-jaguar
boards.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
Farouk Bouabid (7):
      i2c: mux: add the ability to share mux-address with child nodes
      dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer
      i2c: muxes: add support for mule i2c multiplexer
      arm64: dts: rockchip: add mule i2c mux (0x18) on rk3399-puma
      arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-tiger
      arm64: dts: rockchip: add mule i2c mux (0x18) on px30-ringneck
      arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-jaguar

 .../devicetree/bindings/i2c/i2c-mux-mule.yaml      |  80 +++++++++++
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi    |  20 ++-
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi      |  20 ++-
 arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts     |  19 ++-
 arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi     |  19 ++-
 drivers/i2c/i2c-mux.c                              |  10 +-
 drivers/i2c/muxes/Kconfig                          |  11 ++
 drivers/i2c/muxes/Makefile                         |   1 +
 drivers/i2c/muxes/i2c-mux-mule.c                   | 157 +++++++++++++++++++++
 include/linux/i2c-mux.h                            |   1 +
 10 files changed, 327 insertions(+), 11 deletions(-)
---
base-commit: c85af715cac0a951eea97393378e84bb49384734
change-id: 20240404-dev-mule-i2c-mux-9103cde07021

Best regards,
-- 
Farouk Bouabid <farouk.bouabid@theobroma-systems.com>

[PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes

[Farouk Bouabid]

Allow the mux to have the same address as a child device. This is useful
when the mux can only use an i2c-address that is used by a child device
because no other addresses are free to use. eg. the mux can only use
address 0x18 which is used by amc6821 connected to the mux.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 drivers/i2c/i2c-mux.c   | 10 +++++++++-
 include/linux/i2c-mux.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 57ff09f18c37..f5357dff8cc5 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	priv->adap.owner = THIS_MODULE;
 	priv->adap.algo = &priv->algo;
 	priv->adap.algo_data = priv;
-	priv->adap.dev.parent = &parent->dev;
 	priv->adap.retries = parent->retries;
 	priv->adap.timeout = parent->timeout;
 	priv->adap.quirks = parent->quirks;
@@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	else
 		priv->adap.class = class;
 
+	/*
+	 * When creating the adapter, the node devices are checked for i2c address
+	 * match with other devices on the parent adapter, among which is the mux itself.
+	 * If a match is found the node device is not probed successfully.
+	 * Allow the mux to have the same address as a child device by skipping this check.
+	 */
+	if (!(muxc->share_addr_with_children))
+		priv->adap.dev.parent = &parent->dev;
+
 	/*
 	 * Try to populate the mux adapter's of_node, expands to
 	 * nothing if !CONFIG_OF.
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 98ef73b7c8fd..17ac68bf1703 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -21,6 +21,7 @@ struct i2c_mux_core {
 	unsigned int mux_locked:1;
 	unsigned int arbitrator:1;
 	unsigned int gate:1;
+	unsigned int share_addr_with_children:1;
 
 	void *priv;
 

-- 
2.34.1

Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes

[Peter Rosin]

Hi!

2024-04-26 at 18:49, Farouk Bouabid wrote:
> Allow the mux to have the same address as a child device. This is useful
> when the mux can only use an i2c-address that is used by a child device
> because no other addresses are free to use. eg. the mux can only use
> address 0x18 which is used by amc6821 connected to the mux.
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> ---
>  drivers/i2c/i2c-mux.c   | 10 +++++++++-
>  include/linux/i2c-mux.h |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 57ff09f18c37..f5357dff8cc5 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  	priv->adap.owner = THIS_MODULE;
>  	priv->adap.algo = &priv->algo;
>  	priv->adap.algo_data = priv;
> -	priv->adap.dev.parent = &parent->dev;
>  	priv->adap.retries = parent->retries;
>  	priv->adap.timeout = parent->timeout;
>  	priv->adap.quirks = parent->quirks;
> @@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  	else
>  		priv->adap.class = class;
>  
> +	/*
> +	 * When creating the adapter, the node devices are checked for i2c address
> +	 * match with other devices on the parent adapter, among which is the mux itself.
> +	 * If a match is found the node device is not probed successfully.
> +	 * Allow the mux to have the same address as a child device by skipping this check.
> +	 */
> +	if (!(muxc->share_addr_with_children))
> +		priv->adap.dev.parent = &parent->dev;

This is a dirty hack that will not generally do the right thing.

The adapter device parent is not there solely for the purpose of
detecting address clashes, so the above has other implications
that are not desirable.

Therefore, NACK on this approach. It simply needs to be more involved.
Sorry.

Cheers,
Peter

> +
>  	/*
>  	 * Try to populate the mux adapter's of_node, expands to
>  	 * nothing if !CONFIG_OF.
> diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
> index 98ef73b7c8fd..17ac68bf1703 100644
> --- a/include/linux/i2c-mux.h
> +++ b/include/linux/i2c-mux.h
> @@ -21,6 +21,7 @@ struct i2c_mux_core {
>  	unsigned int mux_locked:1;
>  	unsigned int arbitrator:1;
>  	unsigned int gate:1;
> +	unsigned int share_addr_with_children:1;
>  
>  	void *priv;
>  
> 

Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes

[Farouk Bouabid]

Hi Peter,

On 29.04.24 17:46, Peter Rosin wrote:
> Hi!
>
> 2024-04-26 at 18:49, Farouk Bouabid wrote:
>> Allow the mux to have the same address as a child device. This is useful
>> when the mux can only use an i2c-address that is used by a child device
>> because no other addresses are free to use. eg. the mux can only use
>> address 0x18 which is used by amc6821 connected to the mux.
>>
>> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
>> ---
>>   drivers/i2c/i2c-mux.c   | 10 +++++++++-
>>   include/linux/i2c-mux.h |  1 +
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>> index 57ff09f18c37..f5357dff8cc5 100644
>> --- a/drivers/i2c/i2c-mux.c
>> +++ b/drivers/i2c/i2c-mux.c
>> @@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>   	priv->adap.owner = THIS_MODULE;
>>   	priv->adap.algo = &priv->algo;
>>   	priv->adap.algo_data = priv;
>> -	priv->adap.dev.parent = &parent->dev;
>>   	priv->adap.retries = parent->retries;
>>   	priv->adap.timeout = parent->timeout;
>>   	priv->adap.quirks = parent->quirks;
>> @@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>   	else
>>   		priv->adap.class = class;
>>   
>> +	/*
>> +	 * When creating the adapter, the node devices are checked for i2c address
>> +	 * match with other devices on the parent adapter, among which is the mux itself.
>> +	 * If a match is found the node device is not probed successfully.
>> +	 * Allow the mux to have the same address as a child device by skipping this check.
>> +	 */
>> +	if (!(muxc->share_addr_with_children))
>> +		priv->adap.dev.parent = &parent->dev;
> This is a dirty hack that will not generally do the right thing.
>
> The adapter device parent is not there solely for the purpose of
> detecting address clashes, so the above has other implications
> that are not desirable.
>
> Therefore, NACK on this approach. It simply needs to be more involved.
> Sorry.
>
> Cheers,
> Peter
>

Another way to approach this is by implementing this flag as a quirk for 
the added adapter:

(tested but not cleaned up)

"""

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ff5c486a1dbb..6a0237f750db 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -821,9 +821,21 @@ static int i2c_check_mux_children(struct device 
*dev, void *addrp)
  static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
  {
         struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
+       bool skip_check = false;
         int result = 0;

-       if (parent)
+       if (adapter->quirks) {
+                if (adapter->quirks->flags & I2C_AQ_SHARE_ADDR) {
+                       struct i2c_client *client = 
of_find_i2c_device_by_node(adapter->dev.of_node->parent);
+
+                       if (client) {
+                               skip_check = client->addr == addr;
+                               put_device(&client->dev);
+                       }
+                }
+       }
+
+       if (parent && !skip_check)
                 result = i2c_check_mux_parents(parent, addr);

         if (!result)
diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 57ff09f18c37..e87cb0e43725 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -334,7 +334,26 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
         priv->adap.dev.parent = &parent->dev;
         priv->adap.retries = parent->retries;
         priv->adap.timeout = parent->timeout;
-       priv->adap.quirks = parent->quirks;
+       /*
+        * When creating the adapter, the node devices are checked for 
i2c address
+        * match with other devices on the parent adapter, among which 
is the mux itself.
+        * If a match is found the node device is not probed successfully.
+        * Allow the mux to have the same address as a child device by 
skipping this check.
+        */
+       if (!muxc->share_addr_with_children)
+               priv->adap.quirks = parent->quirks;
+       else {
+               struct i2c_adapter_quirks *quirks = 
kzalloc(sizeof(*quirks), GFP_KERNEL);
+               if (!quirks)
+                       return -ENOMEM;
+
+               if (parent->quirks)
+                       *quirks = *(parent->quirks); // @fixme memcpy
+
+               quirks->flags |= I2C_AQ_SHARE_ADDR;
+               priv->adap.quirks = quirks;
+       }
+
         if (muxc->mux_locked)
                 priv->adap.lock_ops = &i2c_mux_lock_ops;
         else
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index 98ef73b7c8fd..17ac68bf1703 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -21,6 +21,7 @@ struct i2c_mux_core {
         unsigned int mux_locked:1;
         unsigned int arbitrator:1;
         unsigned int gate:1;
+       unsigned int share_addr_with_children:1;

         void *priv;

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5e6cd43a6dbd..2ebac9e672ef 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -711,6 +711,8 @@ struct i2c_adapter_quirks {
  #define I2C_AQ_NO_ZERO_LEN             (I2C_AQ_NO_ZERO_LEN_READ | 
I2C_AQ_NO_ZERO_LEN_WRITE)
  /* adapter cannot do repeated START */
  #define I2C_AQ_NO_REP_START            BIT(7)
+/* @fixme document and find proper name */
+#define I2C_AQ_SHARE_ADDR              BIT(8)

  /*
   * i2c_adapter is the structure used to identify a physical i2c bus along

"""

This works, however this only supports device-tree because of 
of_find_i2c_device_by_node. If we want to support acpi then we can either:


1. Get the Mule i2c device address from fwnode_get_next_parent_dev but 
this is static since v6.9-rcx.

2. Pass the Mule i2c device address as a new member of  struct 
i2c_adapter_quirks.


I would go for 2. Do you suggest something else?

Best regards

Farouk

Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes

[Peter Rosin]

Hi!

2024-05-02 at 17:01, Farouk Bouabid wrote:
> Hi Peter,
> 
> On 29.04.24 17:46, Peter Rosin wrote:
>> Hi!
>>
>> 2024-04-26 at 18:49, Farouk Bouabid wrote:
>>> Allow the mux to have the same address as a child device. This is useful
>>> when the mux can only use an i2c-address that is used by a child device
>>> because no other addresses are free to use. eg. the mux can only use
>>> address 0x18 which is used by amc6821 connected to the mux.
>>>
>>> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
>>> ---
>>>   drivers/i2c/i2c-mux.c   | 10 +++++++++-
>>>   include/linux/i2c-mux.h |  1 +
>>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>>> index 57ff09f18c37..f5357dff8cc5 100644
>>> --- a/drivers/i2c/i2c-mux.c
>>> +++ b/drivers/i2c/i2c-mux.c
>>> @@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>>       priv->adap.owner = THIS_MODULE;
>>>       priv->adap.algo = &priv->algo;
>>>       priv->adap.algo_data = priv;
>>> -    priv->adap.dev.parent = &parent->dev;
>>>       priv->adap.retries = parent->retries;
>>>       priv->adap.timeout = parent->timeout;
>>>       priv->adap.quirks = parent->quirks;
>>> @@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>>       else
>>>           priv->adap.class = class;
>>>   +    /*
>>> +     * When creating the adapter, the node devices are checked for i2c address
>>> +     * match with other devices on the parent adapter, among which is the mux itself.
>>> +     * If a match is found the node device is not probed successfully.
>>> +     * Allow the mux to have the same address as a child device by skipping this check.
>>> +     */
>>> +    if (!(muxc->share_addr_with_children))
>>> +        priv->adap.dev.parent = &parent->dev;
>> This is a dirty hack that will not generally do the right thing.
>>
>> The adapter device parent is not there solely for the purpose of
>> detecting address clashes, so the above has other implications
>> that are not desirable.
>>
>> Therefore, NACK on this approach. It simply needs to be more involved.
>> Sorry.
>>
>> Cheers,
>> Peter
>>
> 
> Another way to approach this is by implementing this flag as a quirk for the added adapter:
> 
> (tested but not cleaned up)

Yes, good idea, this is much more targeted and generally feels a lot
better.

> 
> """
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ff5c486a1dbb..6a0237f750db 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -821,9 +821,21 @@ static int i2c_check_mux_children(struct device *dev, void *addrp)
>  static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
>  {
>         struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
> +       bool skip_check = false;
>         int result = 0;
> 
> -       if (parent)
> +       if (adapter->quirks) {
> +                if (adapter->quirks->flags & I2C_AQ_SHARE_ADDR) {
> +                       struct i2c_client *client = of_find_i2c_device_by_node(adapter->dev.of_node->parent);
> +
> +                       if (client) {
> +                               skip_check = client->addr == addr;
> +                               put_device(&client->dev);
> +                       }
> +                }
> +       }
> +
> +       if (parent && !skip_check)
>                 result = i2c_check_mux_parents(parent, addr);
> 
>         if (!result)
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 57ff09f18c37..e87cb0e43725 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -334,7 +334,26 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>         priv->adap.dev.parent = &parent->dev;
>         priv->adap.retries = parent->retries;
>         priv->adap.timeout = parent->timeout;
> -       priv->adap.quirks = parent->quirks;
> +       /*
> +        * When creating the adapter, the node devices are checked for i2c address
> +        * match with other devices on the parent adapter, among which is the mux itself.
> +        * If a match is found the node device is not probed successfully.
> +        * Allow the mux to have the same address as a child device by skipping this check.
> +        */
> +       if (!muxc->share_addr_with_children)
> +               priv->adap.quirks = parent->quirks;
> +       else {
> +               struct i2c_adapter_quirks *quirks = kzalloc(sizeof(*quirks), GFP_KERNEL);

This leaks, dev_kzalloc?

> +               if (!quirks)
> +                       return -ENOMEM;
> +
> +               if (parent->quirks)
> +                       *quirks = *(parent->quirks); // @fixme memcpy
> +
> +               quirks->flags |= I2C_AQ_SHARE_ADDR;
> +               priv->adap.quirks = quirks;
> +       }
> +
>         if (muxc->mux_locked)
>                 priv->adap.lock_ops = &i2c_mux_lock_ops;
>         else
> diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
> index 98ef73b7c8fd..17ac68bf1703 100644
> --- a/include/linux/i2c-mux.h
> +++ b/include/linux/i2c-mux.h
> @@ -21,6 +21,7 @@ struct i2c_mux_core {
>         unsigned int mux_locked:1;
>         unsigned int arbitrator:1;
>         unsigned int gate:1;
> +       unsigned int share_addr_with_children:1;
> 
>         void *priv;
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 5e6cd43a6dbd..2ebac9e672ef 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -711,6 +711,8 @@ struct i2c_adapter_quirks {
>  #define I2C_AQ_NO_ZERO_LEN             (I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE)
>  /* adapter cannot do repeated START */
>  #define I2C_AQ_NO_REP_START            BIT(7)
> +/* @fixme document and find proper name */
> +#define I2C_AQ_SHARE_ADDR              BIT(8)
> 
>  /*
>   * i2c_adapter is the structure used to identify a physical i2c bus along
> 
> """
> 
> This works, however this only supports device-tree because of of_find_i2c_device_by_node. If we want to support acpi then we can either:
> 
> 
> 1. Get the Mule i2c device address from fwnode_get_next_parent_dev but this is static since v6.9-rcx.
> 
> 2. Pass the Mule i2c device address as a new member of  struct i2c_adapter_quirks.
> 
> 
> I would go for 2. Do you suggest something else?

Yes, 2 is definitely neater.

Thanks!

Cheers,
Peter

Re: [PATCH 1/7] i2c: mux: add the ability to share mux-address with child nodes

[Quentin Schulz]

Hi Peter,

On 5/3/24 7:30 AM, Peter Rosin wrote:
> Hi!
> 
> 2024-05-02 at 17:01, Farouk Bouabid wrote:
>> Hi Peter,
>>
>> On 29.04.24 17:46, Peter Rosin wrote:
>>> Hi!
>>>
>>> 2024-04-26 at 18:49, Farouk Bouabid wrote:
>>>> Allow the mux to have the same address as a child device. This is useful
>>>> when the mux can only use an i2c-address that is used by a child device
>>>> because no other addresses are free to use. eg. the mux can only use
>>>> address 0x18 which is used by amc6821 connected to the mux.
>>>>
>>>> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
>>>> ---
>>>>    drivers/i2c/i2c-mux.c   | 10 +++++++++-
>>>>    include/linux/i2c-mux.h |  1 +
>>>>    2 files changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>>>> index 57ff09f18c37..f5357dff8cc5 100644
>>>> --- a/drivers/i2c/i2c-mux.c
>>>> +++ b/drivers/i2c/i2c-mux.c
>>>> @@ -331,7 +331,6 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>>>        priv->adap.owner = THIS_MODULE;
>>>>        priv->adap.algo = &priv->algo;
>>>>        priv->adap.algo_data = priv;
>>>> -    priv->adap.dev.parent = &parent->dev;
>>>>        priv->adap.retries = parent->retries;
>>>>        priv->adap.timeout = parent->timeout;
>>>>        priv->adap.quirks = parent->quirks;
>>>> @@ -348,6 +347,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>>>        else
>>>>            priv->adap.class = class;
>>>>    +    /*
>>>> +     * When creating the adapter, the node devices are checked for i2c address
>>>> +     * match with other devices on the parent adapter, among which is the mux itself.
>>>> +     * If a match is found the node device is not probed successfully.
>>>> +     * Allow the mux to have the same address as a child device by skipping this check.
>>>> +     */
>>>> +    if (!(muxc->share_addr_with_children))
>>>> +        priv->adap.dev.parent = &parent->dev;
>>> This is a dirty hack that will not generally do the right thing.
>>>
>>> The adapter device parent is not there solely for the purpose of
>>> detecting address clashes, so the above has other implications
>>> that are not desirable.
>>>
>>> Therefore, NACK on this approach. It simply needs to be more involved.
>>> Sorry.
>>>
>>> Cheers,
>>> Peter
>>>
>>
>> Another way to approach this is by implementing this flag as a quirk for the added adapter:
>>
>> (tested but not cleaned up)
> 
> Yes, good idea, this is much more targeted and generally feels a lot
> better.
> 
>>
>> """
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index ff5c486a1dbb..6a0237f750db 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -821,9 +821,21 @@ static int i2c_check_mux_children(struct device *dev, void *addrp)
>>   static int i2c_check_addr_busy(struct i2c_adapter *adapter, int addr)
>>   {
>>          struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
>> +       bool skip_check = false;
>>          int result = 0;
>>
>> -       if (parent)
>> +       if (adapter->quirks) {
>> +                if (adapter->quirks->flags & I2C_AQ_SHARE_ADDR) {
>> +                       struct i2c_client *client = of_find_i2c_device_by_node(adapter->dev.of_node->parent);
>> +
>> +                       if (client) {
>> +                               skip_check = client->addr == addr;
>> +                               put_device(&client->dev);
>> +                       }
>> +                }
>> +       }
>> +
>> +       if (parent && !skip_check)
>>                  result = i2c_check_mux_parents(parent, addr);
>>
>>          if (!result)
>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>> index 57ff09f18c37..e87cb0e43725 100644
>> --- a/drivers/i2c/i2c-mux.c
>> +++ b/drivers/i2c/i2c-mux.c
>> @@ -334,7 +334,26 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>          priv->adap.dev.parent = &parent->dev;
>>          priv->adap.retries = parent->retries;
>>          priv->adap.timeout = parent->timeout;
>> -       priv->adap.quirks = parent->quirks;
>> +       /*
>> +        * When creating the adapter, the node devices are checked for i2c address
>> +        * match with other devices on the parent adapter, among which is the mux itself.
>> +        * If a match is found the node device is not probed successfully.
>> +        * Allow the mux to have the same address as a child device by skipping this check.
>> +        */
>> +       if (!muxc->share_addr_with_children)
>> +               priv->adap.quirks = parent->quirks;
>> +       else {
>> +               struct i2c_adapter_quirks *quirks = kzalloc(sizeof(*quirks), GFP_KERNEL);
> 
> This leaks, dev_kzalloc?
> 

Quick questions about this though.

priv is allocated with kzalloc and not devm_kzalloc and is then manually 
kfree'd either as part of the error path or in i2c_del_mux_adapters. Is 
there a reason for this? Shouldn't we migrate this to devm allocation as 
well?

Similarly, I was wondering if we couldn't add a devm_add_action_or_reset 
for i2c_del_mux_adapters in i2c_add_mux_adapter? Is there something that 
prevents us from doing this?

Cheers,
Quentin

[PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Farouk Bouabid]

This patch adds support for the Mule I2C multiplexer.

Mule is an mcu that emulates a set of i2c devices which are reacheable
through an i2c-mux.

The emulated devices share a single i2c address with the mux itself where
the requested register is what determines which logic is executed (mux or
device).

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 .../devicetree/bindings/i2c/i2c-mux-mule.yaml      | 80 ++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml
new file mode 100644
index 000000000000..458e4661cbc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/i2c-mux-mule.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mule I2C multiplexer
+
+maintainers:
+  - Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
+  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
+
+description: |
+  Mule is an mcu that emulates a set of i2c devices which are reacheable
+  through an i2c-mux.
+
+  The emulated devices share a single i2c address with the mux itself where
+  the requested register is what determines which logic is executed (mux or
+  device)
+
+      +--------------------------------------------------+
+      | Mule                                             |
+      |    +---------------+                             |
+  ------+->|Config register|----+                        |
+      | |  +---------------+    |                        |
+      | |                       V_                       |
+      | |                      |  \          +--------+  |
+      | |                      |   \-------->| dev #0 |  |
+      | |                      |   |         +--------+  |
+      | |                      | M |-------->| dev #1 |  |
+      | +--------------------->| U |         +--------+  |
+      |                        | X |-------->| dev #2 |  |
+      |                        |   |         +--------+  |
+      |                        |   /-------->| dev #3 |  |
+      |                        |__/          +--------+  |
+      +--------------------------------------------------+
+
+
+allOf:
+  - $ref: /schemas/i2c/i2c-mux.yaml#
+
+properties:
+  compatible:
+    const: tsd,mule-i2c-mux
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        i2c-mux@18 {
+            compatible = "tsd,mule-i2c-mux";
+            reg = <0x18>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            i2c@0 {
+                reg = <0x0>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                fan: fan@18 {
+                  compatible = "ti,amc6821";
+                  reg = <0x18>;
+                  #cooling-cells = <2>;
+                };
+            };
+        };
+    };
+...

-- 
2.34.1

Re: [PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Rob Herring]

On Fri, 26 Apr 2024 18:49:33 +0200, Farouk Bouabid wrote:
> This patch adds support for the Mule I2C multiplexer.
> 
> Mule is an mcu that emulates a set of i2c devices which are reacheable
> through an i2c-mux.
> 
> The emulated devices share a single i2c address with the mux itself where
> the requested register is what determines which logic is executed (mux or
> device).
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> ---
>  .../devicetree/bindings/i2c/i2c-mux-mule.yaml      | 80 ++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-mux-mule.example.dtb: fan@18: '#cooling-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240426-dev-mule-i2c-mux-v1-2-045a482f6ffb@theobroma-systems.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Farouk Bouabid]

Hi Rob,

On 26.04.24 20:22, Rob Herring wrote:
> On Fri, 26 Apr 2024 18:49:33 +0200, Farouk Bouabid wrote:
>> This patch adds support for the Mule I2C multiplexer.
>>
>> Mule is an mcu that emulates a set of i2c devices which are reacheable
>> through an i2c-mux.
>>
>> The emulated devices share a single i2c address with the mux itself where
>> the requested register is what determines which logic is executed (mux or
>> device).
>>
>> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
>> ---
>>   .../devicetree/bindings/i2c/i2c-mux-mule.yaml      | 80 ++++++++++++++++++++++
>>   1 file changed, 80 insertions(+)
>>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/i2c-mux-mule.example.dtb: fan@18: '#cooling-cells' does not match any of the regexes: 'pinctrl-[0-9]+'


Currently ti,amc6821 uses trivial devices dt-bindings which does not 
support "#cooling-cells". We can fix this in a different patch though.


Best regards

Farouk

Re: [PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Krzysztof Kozlowski]

Except that this was not fully tested few comments.

A nit, subject: drop second/last, redundant "dt-bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


On 26/04/2024 18:49, Farouk Bouabid wrote:
> This patch adds support for the Mule I2C multiplexer.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Mule is an mcu that emulates a set of i2c devices which are reacheable
> through an i2c-mux.
> 
> The emulated devices share a single i2c address with the mux itself where
> the requested register is what determines which logic is executed (mux or
> device).
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> ---
>  .../devicetree/bindings/i2c/i2c-mux-mule.yaml      | 80 ++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml
> new file mode 100644
> index 000000000000..458e4661cbc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-mule.yaml

Use compatible as filename.

> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/i2c-mux-mule.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mule I2C multiplexer
> +
> +maintainers:
> +  - Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> +  - Quentin Schulz <quentin.schulz@theobroma-systems.com>
> +
> +description: |
> +  Mule is an mcu that emulates a set of i2c devices which are reacheable

typo, reachable

> +  through an i2c-mux.
> +
> +  The emulated devices share a single i2c address with the mux itself where
> +  the requested register is what determines which logic is executed (mux or
> +  device)
> +
> +      +--------------------------------------------------+
> +      | Mule                                             |
> +      |    +---------------+                             |
> +  ------+->|Config register|----+                        |
> +      | |  +---------------+    |                        |
> +      | |                       V_                       |
> +      | |                      |  \          +--------+  |
> +      | |                      |   \-------->| dev #0 |  |
> +      | |                      |   |         +--------+  |
> +      | |                      | M |-------->| dev #1 |  |
> +      | +--------------------->| U |         +--------+  |
> +      |                        | X |-------->| dev #2 |  |
> +      |                        |   |         +--------+  |
> +      |                        |   /-------->| dev #3 |  |
> +      |                        |__/          +--------+  |
> +      +--------------------------------------------------+
> +
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-mux.yaml#
> +
> +properties:
> +  compatible:
> +    const: tsd,mule-i2c-mux
> +
> +  reg:
> +    maxItems: 1
> +

Best regards,
Krzysztof

Re: [PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Rob Herring]

On Fri, Apr 26, 2024 at 11:50 AM Farouk Bouabid
<farouk.bouabid@theobroma-systems.com> wrote:
>
> This patch adds support for the Mule I2C multiplexer.
>
> Mule is an mcu that emulates a set of i2c devices which are reacheable

MCU

reachable

> through an i2c-mux.
>
> The emulated devices share a single i2c address with the mux itself where
> the requested register is what determines which logic is executed (mux or
> device).

Just to be sure, we need a complete binding for the MCU. Is this the
only thing the MCU does?

Rob

Re: [PATCH 2/7] dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer

[Farouk Bouabid]

Hi Rob,

On 29.04.24 15:56, Rob Herring wrote:
> On Fri, Apr 26, 2024 at 11:50 AM Farouk Bouabid
> <farouk.bouabid@theobroma-systems.com> wrote:
>> This patch adds support for the Mule I2C multiplexer.
>>
>> Mule is an mcu that emulates a set of i2c devices which are reacheable
> MCU
>
> reachable
>
>> through an i2c-mux.
>>
>> The emulated devices share a single i2c address with the mux itself where
>> the requested register is what determines which logic is executed (mux or
>> device).
> Just to be sure, we need a complete binding for the MCU. Is this the
> only thing the MCU does?


Currently that is all the MCU does. We plan to add more features to the 
MCU firmware: Buzzer over i2c, watchdog over i2c ...


Best regards

Farouk

[PATCH 3/7] i2c: muxes: add support for mule i2c multiplexer

[Farouk Bouabid]

Mule is an mcu that emulates a set of i2c devices which are reacheable
through an i2c-mux.

The emulated devices share a single i2c address with the mux itself where
the requested register is what determines which logic is executed (mux or
device):

1- The devices on the mux can be selected (mux function) by writing the
appropriate device number to an i2c config register (0xff) that is not
used by any device logic.

2- Any access to a register other than the config register will be
handled by the previously selected device.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 drivers/i2c/muxes/Kconfig        |  11 +++
 drivers/i2c/muxes/Makefile       |   1 +
 drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index db1b9057612a..593a20a6ac51 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-mlxcpld.
 
+config I2C_MUX_MULE
+	tristate "Mule I2C device multiplexer"
+	depends on OF
+	help
+	  If you say yes to this option, support will be included for a
+	  Mule I2C device multiplexer. This driver provides access to
+	  I2C devices connected on the Mule I2C mux.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-mux-mule.
+
 endmenu
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 6d9d865e8518..4b24f49515a7 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
 obj-$(CONFIG_I2C_MUX_GPMUX)	+= i2c-mux-gpmux.o
 obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
 obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
+obj-$(CONFIG_I2C_MUX_MULE)	+= i2c-mux-mule.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
 obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
diff --git a/drivers/i2c/muxes/i2c-mux-mule.c b/drivers/i2c/muxes/i2c-mux-mule.c
new file mode 100644
index 000000000000..da2a9526522e
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-mule.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mule I2C device multiplexer
+ *
+ * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
+ */
+
+#include <linux/i2c-mux.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define MUX_CONFIG_REG	0xff
+#define MUX_DEFAULT_DEV	0x0
+
+struct mule_i2c_reg_mux {
+	struct regmap *regmap;
+};
+
+static const struct regmap_config mule_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct of_device_id mule_i2c_mux_of_match[] = {
+	{.compatible = "tsd,mule-i2c-mux",},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match);
+
+static inline int __mux_select(struct regmap *regmap, u32 dev)
+{
+	return regmap_write(regmap, MUX_CONFIG_REG, dev);
+}
+
+static int mux_select(struct i2c_mux_core *muxc, u32 dev)
+{
+	struct mule_i2c_reg_mux *mux = muxc->priv;
+
+	return __mux_select(mux->regmap, dev);
+}
+
+static int mux_deselect(struct i2c_mux_core *muxc, u32 dev)
+{
+	return mux_select(muxc, MUX_DEFAULT_DEV);
+}
+
+static void mux_remove(void *data)
+{
+	struct i2c_mux_core *muxc = data;
+
+	i2c_mux_del_adapters(muxc);
+
+	mux_deselect(muxc, MUX_DEFAULT_DEV);
+}
+
+static int mule_i2c_mux_probe(struct i2c_client *client)
+{
+	struct i2c_adapter *adap = client->adapter;
+	struct mule_i2c_reg_mux *priv;
+	struct i2c_mux_core *muxc;
+	struct device_node *dev;
+	unsigned int readback;
+	bool old_fw;
+	int ndev, ret;
+
+	/* Count devices on the mux */
+	ndev = of_get_child_count(client->dev.of_node);
+	dev_dbg(&client->dev, "%u devices on the mux\n", ndev);
+
+	muxc = i2c_mux_alloc(adap, &client->dev,
+						 ndev, sizeof(*priv),
+						 I2C_MUX_LOCKED,
+						 mux_select, mux_deselect);
+	if (!muxc)
+		return -ENOMEM;
+
+	muxc->share_addr_with_children = 1;
+	priv = i2c_mux_priv(muxc);
+
+	priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(priv->regmap),
+							 "Failed to allocate i2c register map\n");
+
+	i2c_set_clientdata(client, muxc);
+
+	/*
+	 * Mux 0 is guaranteed to exist on all old and new mule fw.
+	 * mule fw without mux support will accept write ops to the
+	 * config register, but readback returns 0xff (register not updated).
+	 */
+	ret = mux_select(muxc, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback);
+	if (ret)
+		return ret;
+
+	old_fw = (readback == 0);
+
+	ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc);
+	if (ret)
+		return ret;
+
+	/* Create device adapters */
+	for_each_child_of_node(client->dev.of_node, dev) {
+		u32 reg;
+
+		ret = of_property_read_u32(dev, "reg", &reg);
+		if (ret) {
+			dev_err(&client->dev, "No reg property found for %s: %d\n",
+					of_node_full_name(dev), ret);
+			return ret;
+		}
+
+		if (!old_fw && reg != 0) {
+			dev_warn(&client->dev,
+					 "Mux %d not supported, please update Mule FW\n", reg);
+			continue;
+		}
+
+		ret = mux_select(muxc, reg);
+		if (ret) {
+			dev_warn(&client->dev,
+					 "Mux %d not supported, please update Mule FW\n", reg);
+			continue;
+		}
+
+		ret = i2c_mux_add_adapter(muxc, 0, reg, 0);
+		if (ret) {
+			dev_err(&client->dev, "Failed to add i2c mux adapter %d: %d\n", reg, ret);
+			return ret;
+		}
+	}
+
+	mux_deselect(muxc, MUX_DEFAULT_DEV);
+
+	return 0;
+}
+
+static struct i2c_driver mule_i2c_mux_driver = {
+	.driver		= {
+		.name	= "mule-i2c-mux",
+		.of_match_table = mule_i2c_mux_of_match,
+	},
+	.probe		= mule_i2c_mux_probe,
+};
+
+module_i2c_driver(mule_i2c_mux_driver);
+
+MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@theobroma-systems.com>");
+MODULE_DESCRIPTION("I2C mux driver for Mule");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH 3/7] i2c: muxes: add support for mule i2c multiplexer

[Krzysztof Kozlowski]

On 26/04/2024 18:49, Farouk Bouabid wrote:
> Mule is an mcu that emulates a set of i2c devices which are reacheable
> through an i2c-mux.
> 
> The emulated devices share a single i2c address with the mux itself where
> the requested register is what determines which logic is executed (mux or
> device):
> 
> 1- The devices on the mux can be selected (mux function) by writing the
> appropriate device number to an i2c config register (0xff) that is not
> used by any device logic.
> 
> 2- Any access to a register other than the config register will be
> handled by the previously selected device.
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> ---
>  drivers/i2c/muxes/Kconfig        |  11 +++
>  drivers/i2c/muxes/Makefile       |   1 +
>  drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index db1b9057612a..593a20a6ac51 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-mlxcpld.
>  
> +config I2C_MUX_MULE
> +	tristate "Mule I2C device multiplexer"
> +	depends on OF
> +	help
> +	  If you say yes to this option, support will be included for a
> +	  Mule I2C device multiplexer. This driver provides access to
> +	  I2C devices connected on the Mule I2C mux.

Describe what is Mule. Here and in bindings documentation.

> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-mux-mule.
> +
>  endmenu
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 6d9d865e8518..4b24f49515a7 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_I2C_MUX_GPIO)	+= i2c-mux-gpio.o
>  obj-$(CONFIG_I2C_MUX_GPMUX)	+= i2c-mux-gpmux.o
>  obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
>  obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
> +obj-$(CONFIG_I2C_MUX_MULE)	+= i2c-mux-mule.o
>  obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>  obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
>  obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
> diff --git a/drivers/i2c/muxes/i2c-mux-mule.c b/drivers/i2c/muxes/i2c-mux-mule.c
> new file mode 100644
> index 000000000000..da2a9526522e
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-mule.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mule I2C device multiplexer
> + *
> + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +#include <linux/i2c-mux.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define MUX_CONFIG_REG	0xff
> +#define MUX_DEFAULT_DEV	0x0
> +
> +struct mule_i2c_reg_mux {
> +	struct regmap *regmap;
> +};
> +
> +static const struct regmap_config mule_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct of_device_id mule_i2c_mux_of_match[] = {
> +	{.compatible = "tsd,mule-i2c-mux",},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mule_i2c_mux_of_match);

This goes after or before probe. Don't introduce unusual coding style.

> +

...

> +static void mux_remove(void *data)
> +{
> +	struct i2c_mux_core *muxc = data;
> +
> +	i2c_mux_del_adapters(muxc);
> +
> +	mux_deselect(muxc, MUX_DEFAULT_DEV);
> +}
> +
> +static int mule_i2c_mux_probe(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	struct mule_i2c_reg_mux *priv;
> +	struct i2c_mux_core *muxc;
> +	struct device_node *dev;
> +	unsigned int readback;
> +	bool old_fw;
> +	int ndev, ret;
> +
> +	/* Count devices on the mux */
> +	ndev = of_get_child_count(client->dev.of_node);
> +	dev_dbg(&client->dev, "%u devices on the mux\n", ndev);
> +
> +	muxc = i2c_mux_alloc(adap, &client->dev,
> +						 ndev, sizeof(*priv),
> +						 I2C_MUX_LOCKED,
> +						 mux_select, mux_deselect);

Very odd alignment. This is absolutely unreadable.

Please properly align with opening (.

> +	if (!muxc)
> +		return -ENOMEM;
> +
> +	muxc->share_addr_with_children = 1;
> +	priv = i2c_mux_priv(muxc);
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(priv->regmap),
> +							 "Failed to allocate i2c register map\n");
> +
> +	i2c_set_clientdata(client, muxc);
> +
> +	/*
> +	 * Mux 0 is guaranteed to exist on all old and new mule fw.
> +	 * mule fw without mux support will accept write ops to the
> +	 * config register, but readback returns 0xff (register not updated).
> +	 */
> +	ret = mux_select(muxc, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback);
> +	if (ret)
> +		return ret;
> +
> +	old_fw = (readback == 0);
> +
> +	ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc);

This is really odd. Why do you call remove callback as devm action?

I have serious doubts this was really tested.

> +	if (ret)
> +		return ret;
> +
> +	/* Create device adapters */
> +	for_each_child_of_node(client->dev.of_node, dev) {
> +		u32 reg;
> +
> +		ret = of_property_read_u32(dev, "reg", &reg);
> +		if (ret) {
> +			dev_err(&client->dev, "No reg property found for %s: %d\n",
> +					of_node_full_name(dev), ret);

Very odd alignment. Please properly align with opening (.

> +			return ret;
> +		}
> +
> +		if (!old_fw && reg != 0) {
> +			dev_warn(&client->dev,
> +					 "Mux %d not supported, please update Mule FW\n", reg);
> +			continue;
> +		}
> +
> +		ret = mux_select(muxc, reg);
> +		if (ret) {
> +			dev_warn(&client->dev,
> +					 "Mux %d not supported, please update Mule FW\n", reg);
> +			continue;
> +		}
> +
> +		ret = i2c_mux_add_adapter(muxc, 0, reg, 0);
> +		if (ret) {
> +			dev_err(&client->dev, "Failed to add i2c mux adapter %d: %d\n", reg, ret);
> +			return ret;
> +		}
> +	}
> +
> +	mux_deselect(muxc, MUX_DEFAULT_DEV);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver mule_i2c_mux_driver = {
> +	.driver		= {
> +		.name	= "mule-i2c-mux",
> +		.of_match_table = mule_i2c_mux_of_match,
> +	},
> +	.probe		= mule_i2c_mux_probe,
> +};
> +

Anyway, all this looks like i2c-mux-reg. Please provide rationale in
commit msg WHY you need one more driver.


Best regards,
Krzysztof

Re: [PATCH 3/7] i2c: muxes: add support for mule i2c multiplexer

[Farouk Bouabid]

Hi Krzysztof,

On 29.04.24 08:33, Krzysztof Kozlowski wrote:
> [You don't often get email from krzysztof.kozlowski@linaro.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 26/04/2024 18:49, Farouk Bouabid wrote:
>> Mule is an mcu that emulates a set of i2c devices which are reacheable
>> through an i2c-mux.
>>
>> The emulated devices share a single i2c address with the mux itself where
>> the requested register is what determines which logic is executed (mux or
>> device):
>>
>> 1- The devices on the mux can be selected (mux function) by writing the
>> appropriate device number to an i2c config register (0xff) that is not
>> used by any device logic.
>>
>> 2- Any access to a register other than the config register will be
>> handled by the previously selected device.
>>
>> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
>> ---
>>   drivers/i2c/muxes/Kconfig        |  11 +++
>>   drivers/i2c/muxes/Makefile       |   1 +
>>   drivers/i2c/muxes/i2c-mux-mule.c | 157 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 169 insertions(+)
>>
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index db1b9057612a..593a20a6ac51 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -119,4 +119,15 @@ config I2C_MUX_MLXCPLD
>>          This driver can also be built as a module.  If so, the module
>>          will be called i2c-mux-mlxcpld.
>>
>> +config I2C_MUX_MULE
>> +     tristate "Mule I2C device multiplexer"
>> +     depends on OF
>> +     help
>> +       If you say yes to this option, support will be included for a
>> +       Mule I2C device multiplexer. This driver provides access to
>> +       I2C devices connected on the Mule I2C mux.
> Describe what is Mule. Here and in bindings documentation.


Is the description in bindings documentation good enough to be copied 
here ? If not, any suggestions?


[snip]


>> +     if (!muxc)
>> +             return -ENOMEM;
>> +
>> +     muxc->share_addr_with_children = 1;
>> +     priv = i2c_mux_priv(muxc);
>> +
>> +     priv->regmap = devm_regmap_init_i2c(client, &mule_regmap_config);
>> +     if (IS_ERR(priv->regmap))
>> +             return dev_err_probe(&client->dev, PTR_ERR(priv->regmap),
>> +                                                      "Failed to allocate i2c register map\n");
>> +
>> +     i2c_set_clientdata(client, muxc);
>> +
>> +     /*
>> +      * Mux 0 is guaranteed to exist on all old and new mule fw.
>> +      * mule fw without mux support will accept write ops to the
>> +      * config register, but readback returns 0xff (register not updated).
>> +      */
>> +     ret = mux_select(muxc, 0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_read(priv->regmap, MUX_CONFIG_REG, &readback);
>> +     if (ret)
>> +             return ret;
>> +
>> +     old_fw = (readback == 0);
>> +
>> +     ret = devm_add_action_or_reset(&client->dev, mux_remove, muxc);
> This is really odd. Why do you call remove callback as devm action?
>
> I have serious doubts this was really tested.


This was tested in both scenarios where the probe fails and while 
removing the driver. The remove function is added as devm action to the 
unwinding path, which will basically be called when removing the driver 
OR when the probe fails after this call.

https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L734


[snip]


> Anyway, all this looks like i2c-mux-reg. Please provide rationale in
> commit msg WHY you need one more driver.


We are basically using an i2c device (mux) and an i2c register to handle 
the devices behind the mux which is not what the "i2c-mux-reg" offers, 
where the mux is a platform device and the register used is part of the 
io-memory.

Also to check that backward compatibility is valid on older Mule 
versions, we perform some tests in the probe function that are specific 
to Mule.

Did I miss something?


Best regards

Farouk

[PATCH 4/7] arm64: dts: rockchip: add mule i2c mux (0x18) on rk3399-puma

[Farouk Bouabid]

Mule emulates an i2c mux (address 0x18). The amc6821 is exposed behind
this bus.

Add the mux node and amc6821 as a default device.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
index c08e69391c01..e7313be24c1a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
@@ -10,6 +10,7 @@
 / {
 	aliases {
 		ethernet0 = &gmac;
+		i2c10 = &i2c10;
 		mmc0 = &sdhci;
 	};
 
@@ -357,10 +358,23 @@ &i2c7 {
 	status = "okay";
 	clock-frequency = <400000>;
 
-	fan: fan@18 {
-		compatible = "ti,amc6821";
+	i2c-mux@18 {
+		compatible = "tsd,mule-i2c-mux";
 		reg = <0x18>;
-		#cooling-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c10: i2c@0 {
+			reg = <0x0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			fan: fan@18 {
+				compatible = "ti,amc6821";
+				reg = <0x18>;
+				#cooling-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 5/7] arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-tiger

[Farouk Bouabid]

Mule emulates an i2c mux (address 0x18). The amc6821 is exposed behind
this bus.

Add the mux node and amc6821 as a default device.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
index 1eb2543a5fde..5ed7d51717bb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
@@ -12,6 +12,7 @@ / {
 	compatible = "tsd,rk3588-tiger", "rockchip,rk3588";
 
 	aliases {
+		i2c10 = &i2c10;
 		mmc0 = &sdhci;
 		rtc0 = &rtc_twi;
 	};
@@ -210,9 +211,23 @@ &i2c6 {
 	clock-frequency = <400000>;
 	status = "okay";
 
-	fan@18 {
-		compatible = "ti,amc6821";
+	i2c-mux@18 {
+		compatible = "tsd,mule-i2c-mux";
 		reg = <0x18>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c10: i2c@0 {
+			reg = <0x0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			fan: fan@18 {
+				compatible = "ti,amc6821";
+				reg = <0x18>;
+				#cooling-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 6/7] arm64: dts: rockchip: add mule i2c mux (0x18) on px30-ringneck

[Farouk Bouabid]

Mule emulates an i2c mux (address 0x18). The amc6821 is exposed behind
this bus.

Add the mux node and amc6821 as a default device.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
index bb1aea82e666..eea906379983 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
@@ -9,6 +9,7 @@
 
 / {
 	aliases {
+		i2c10 = &i2c10;
 		mmc0 = &emmc;
 		mmc1 = &sdio;
 		rtc0 = &rtc_twi;
@@ -291,10 +292,23 @@ &i2c1 {
 	/* SE05x is limited to Fast Mode */
 	clock-frequency = <400000>;
 
-	fan: fan@18 {
-		compatible = "ti,amc6821";
+	i2c-mux@18 {
+		compatible = "tsd,mule-i2c-mux";
 		reg = <0x18>;
-		#cooling-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c10: i2c@0 {
+			reg = <0x0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			fan: fan@18 {
+				compatible = "ti,amc6821";
+				reg = <0x18>;
+				#cooling-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 7/7] arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-jaguar

[Farouk Bouabid]

Mule emulates an i2c mux (address 0x18). The amc6821 is exposed behind
this bus.

Add the mux node and amc6821 as a default device.

Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
index 39d65002add1..14f1322c162f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
@@ -32,6 +32,7 @@ button-bios-disable {
 
 	aliases {
 		ethernet0 = &gmac0;
+		i2c10 = &i2c10;
 		mmc0 = &sdhci;
 		mmc1 = &sdmmc;
 		rtc0 = &rtc_twi;
@@ -249,9 +250,23 @@ &i2c0 {
 	pinctrl-0 = <&i2c0m2_xfer>;
 	status = "okay";
 
-	fan@18 {
-		compatible = "ti,amc6821";
+	i2c-mux@18 {
+		compatible = "tsd,mule-i2c-mux";
 		reg = <0x18>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c10: i2c@0 {
+			reg = <0x0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			fan: fan@18 {
+				compatible = "ti,amc6821";
+				reg = <0x18>;
+				#cooling-cells = <2>;
+			};
+		};
 	};
 
 	vdd_npu_s0: regulator@42 {

-- 
2.34.1

Re: [PATCH 0/7] Add Mule I2C multiplexer support

[Rob Herring]

On Fri, 26 Apr 2024 18:49:31 +0200, Farouk Bouabid wrote:
> Mule is an mcu that emulates a set of i2c devices which are reacheable
> through an i2c-mux.
> 
> The emulated devices share a single i2c address with the mux itself where
> the requested register is what determines which logic is executed (mux or
> device):
> 
> 1- The devices on the mux can be selected (mux function) by writing the
> appropriate device number to an i2c config register (0xff) that is not
> used by any device logic.
> 
> 2- Any access to a register other than the config register will be
> handled by the previously selected device.
> 
>       +-------------------------------------------------------+
>       |  Mule                                                 |
>       |        +---------------+                              |
>     ----+-(1)->|Config register|-----+                        |
>       | |      +---------------+     |                        |
>       | |                            V_                       |
>       | |                            |  \          +--------+ |
>       | |                            |   \-------->| dev #0 | |
>       | |                            |   |         +--------+ |
>       | |                            | M |-------->| dev #1 | |
>       | +-----------(2)------------->| U |         +--------+ |
>       |                              | X |-------->| dev #2 | |
>       |                              |   |         +--------+ |
>       |                              |   /-------->| dev #3 | |
>       |                              |__/          +--------+ |
>       +-------------------------------------------------------+
> 
> The current i2c-mux implementation does not allow the mux to share the i2c
> address of a child device. As a workaround, when creating each i2c child
> adapter we do not assign the parent adapter to avoid the address-match with
> the mux.
> 
> This patch-series adds support for this multiplexer. Mule is integrated
> as part of rk3399-puma, px30-ringneck, rk3588-tiger and rk3588-jaguar
> boards.
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> ---
> Farouk Bouabid (7):
>       i2c: mux: add the ability to share mux-address with child nodes
>       dt-bindings: i2c: mux: mule: add dt-bindings for mule i2c multiplexer
>       i2c: muxes: add support for mule i2c multiplexer
>       arm64: dts: rockchip: add mule i2c mux (0x18) on rk3399-puma
>       arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-tiger
>       arm64: dts: rockchip: add mule i2c mux (0x18) on px30-ringneck
>       arm64: dts: rockchip: add mule i2c mux (0x18) on rk3588-jaguar
> 
>  .../devicetree/bindings/i2c/i2c-mux-mule.yaml      |  80 +++++++++++
>  arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi    |  20 ++-
>  arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi      |  20 ++-
>  arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts     |  19 ++-
>  arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi     |  19 ++-
>  drivers/i2c/i2c-mux.c                              |  10 +-
>  drivers/i2c/muxes/Kconfig                          |  11 ++
>  drivers/i2c/muxes/Makefile                         |   1 +
>  drivers/i2c/muxes/i2c-mux-mule.c                   | 157 +++++++++++++++++++++
>  include/linux/i2c-mux.h                            |   1 +
>  10 files changed, 327 insertions(+), 11 deletions(-)
> ---
> base-commit: c85af715cac0a951eea97393378e84bb49384734
> change-id: 20240404-dev-mule-i2c-mux-9103cde07021
> 
> Best regards,
> --
> Farouk Bouabid <farouk.bouabid@theobroma-systems.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y rockchip/rk3588-jaguar.dtb' for 20240426-dev-mule-i2c-mux-v1-0-045a482f6ffb@theobroma-systems.com:

arch/arm64/boot/dts/rockchip/rk3588-jaguar.dtb: fan@18: '#cooling-cells' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/trivial-devices.yaml#