Patchwork Problem with multiple i2c multiplexers on one bus, and mux bus naming

login
register
mail settings
Submitter Guenter Roeck
Date Nov. 17, 2013, 6:56 p.m.
Message ID <20131117185624.GA23639@roeck-us.net>
Download mbox | patch
Permalink /patch/291874/
State New
Headers show

Comments

Guenter Roeck - Nov. 17, 2013, 6:56 p.m.
On Sat, Nov 16, 2013 at 09:43:18PM +0100, Jean Delvare wrote:
> Oh, I forgot to mention...
> 
> > On Sat, 16 Nov 2013 10:31:00 -0800, Guenter Roeck wrote:
> > > I understand this may require an API change, as the mux chip is not necessarily an i2c
> > > and the i2c-mux core code doesn't really care what the mux chip is.
> 
> Such an API change would be perfectly acceptable. Mux drivers are few
> and i2c-mux is still relatively new. Unique i2c adapter name is
> something important, definitely worth changing the API if needed.
> 
I managed to do it without changing the API; see patch below.
This results in

i2c-0/name:MPC adapter at 0xfff703000
i2c-1/name:MPC adapter at 0xfff703100
i2c-10/name:i2c-2-mux-75 (chan_id 7)
i2c-11/name:i2c-2-mux-76 (chan_id 0)
i2c-12/name:i2c-2-mux-76 (chan_id 1)
i2c-13/name:i2c-2-mux-76 (chan_id 2)
i2c-14/name:i2c-2-mux-76 (chan_id 3)
i2c-15/name:i2c-2-mux-76 (chan_id 4)
i2c-16/name:i2c-2-mux-76 (chan_id 5)
i2c-17/name:i2c-2-mux-76 (chan_id 6)
i2c-18/name:i2c-2-mux-76 (chan_id 7)
i2c-19/name:i2c-0-mux (chan_id 1)
i2c-2/name:i2c-0-mux (chan_id 0)
i2c-20/name:i2c-0-mux (chan_id 2)
i2c-21/name:i2c-0-mux (chan_id 3)
i2c-22/name:i2c-0-mux (chan_id 4)
i2c-23/name:i2c-0-mux (chan_id 5)
i2c-24/name:i2c-0-mux (chan_id 6)
i2c-25/name:i2c-0-mux (chan_id 7)
i2c-3/name:i2c-2-mux-75 (chan_id 0)
i2c-4/name:i2c-2-mux-75 (chan_id 1)
i2c-5/name:i2c-2-mux-75 (chan_id 2)
i2c-6/name:i2c-2-mux-75 (chan_id 3)
i2c-7/name:i2c-2-mux-75 (chan_id 4)
i2c-8/name:i2c-2-mux-75 (chan_id 5)
i2c-9/name:i2c-2-mux-75 (chan_id 6)

This retains the old mux name if the mux is not an i2c device, and adds
the i2c device address if it is. This solves the problem for me.

Comments ?

Thanks,
Guenter

---

From 592db23cd3a769a5369e15260ffa6924acf91ddb Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Sun, 17 Nov 2013 09:25:21 -0800
Subject: [PATCH] i2c: mux: Create unique i2c mux name if the parent is an i2c
 device

If an i2c mux parent is an i2c device, there can be more than one such device on
a single i2c bus. The current mux naming scheme does not take this into account
and creates multiple i2c adapters with the same name.

Expand the mux adapter name to include the i2c client's I2C address to solve the
problem.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/i2c/i2c-mux.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
Jean Delvare - Nov. 17, 2013, 8:41 p.m.
Hi Guenter,

On Sun, 17 Nov 2013 10:56:24 -0800, Guenter Roeck wrote:
> I managed to do it without changing the API; see patch below.
> This results in
> 
> i2c-0/name:MPC adapter at 0xfff703000
> i2c-1/name:MPC adapter at 0xfff703100
> i2c-10/name:i2c-2-mux-75 (chan_id 7)
> i2c-11/name:i2c-2-mux-76 (chan_id 0)
> i2c-12/name:i2c-2-mux-76 (chan_id 1)
> i2c-13/name:i2c-2-mux-76 (chan_id 2)
> i2c-14/name:i2c-2-mux-76 (chan_id 3)
> i2c-15/name:i2c-2-mux-76 (chan_id 4)
> i2c-16/name:i2c-2-mux-76 (chan_id 5)
> i2c-17/name:i2c-2-mux-76 (chan_id 6)
> i2c-18/name:i2c-2-mux-76 (chan_id 7)
> i2c-19/name:i2c-0-mux (chan_id 1)
> i2c-2/name:i2c-0-mux (chan_id 0)
> i2c-20/name:i2c-0-mux (chan_id 2)
> i2c-21/name:i2c-0-mux (chan_id 3)
> i2c-22/name:i2c-0-mux (chan_id 4)
> i2c-23/name:i2c-0-mux (chan_id 5)
> i2c-24/name:i2c-0-mux (chan_id 6)
> i2c-25/name:i2c-0-mux (chan_id 7)
> i2c-3/name:i2c-2-mux-75 (chan_id 0)
> i2c-4/name:i2c-2-mux-75 (chan_id 1)
> i2c-5/name:i2c-2-mux-75 (chan_id 2)
> i2c-6/name:i2c-2-mux-75 (chan_id 3)
> i2c-7/name:i2c-2-mux-75 (chan_id 4)
> i2c-8/name:i2c-2-mux-75 (chan_id 5)
> i2c-9/name:i2c-2-mux-75 (chan_id 6)
> 
> This retains the old mux name if the mux is not an i2c device, and adds
> the i2c device address if it is. This solves the problem for me.
> 
> Comments ?

This "for me" worries me a bit. You basically restored the uniqueness
for the multiple I2C-based multiplexer case. But for multiple
GPIO-based multiplexers, for example, the name confusion still exists.
If we are going to address this problem, I believe we should do so in a
way which doesn't need revisiting in a couple months. You are changing
adapter names people or tools may be relying on, we really don't want
to do this too often.

We could use the first GPIO pin number as the unique identifier for the
GPIO case. However I2C addresses and GPIO pin numbers could collide...
So we'd need separate name spaces, too.

I agree this makes things even more complicated than they are right
now, but I can't think of any other way that would be both robust and
durable.
Guenter Roeck - Nov. 17, 2013, 9:13 p.m.
On 11/17/2013 12:41 PM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sun, 17 Nov 2013 10:56:24 -0800, Guenter Roeck wrote:
>> I managed to do it without changing the API; see patch below.
>> This results in
>>
>> i2c-0/name:MPC adapter at 0xfff703000
>> i2c-1/name:MPC adapter at 0xfff703100
>> i2c-10/name:i2c-2-mux-75 (chan_id 7)
>> i2c-11/name:i2c-2-mux-76 (chan_id 0)
>> i2c-12/name:i2c-2-mux-76 (chan_id 1)
>> i2c-13/name:i2c-2-mux-76 (chan_id 2)
>> i2c-14/name:i2c-2-mux-76 (chan_id 3)
>> i2c-15/name:i2c-2-mux-76 (chan_id 4)
>> i2c-16/name:i2c-2-mux-76 (chan_id 5)
>> i2c-17/name:i2c-2-mux-76 (chan_id 6)
>> i2c-18/name:i2c-2-mux-76 (chan_id 7)
>> i2c-19/name:i2c-0-mux (chan_id 1)
>> i2c-2/name:i2c-0-mux (chan_id 0)
>> i2c-20/name:i2c-0-mux (chan_id 2)
>> i2c-21/name:i2c-0-mux (chan_id 3)
>> i2c-22/name:i2c-0-mux (chan_id 4)
>> i2c-23/name:i2c-0-mux (chan_id 5)
>> i2c-24/name:i2c-0-mux (chan_id 6)
>> i2c-25/name:i2c-0-mux (chan_id 7)
>> i2c-3/name:i2c-2-mux-75 (chan_id 0)
>> i2c-4/name:i2c-2-mux-75 (chan_id 1)
>> i2c-5/name:i2c-2-mux-75 (chan_id 2)
>> i2c-6/name:i2c-2-mux-75 (chan_id 3)
>> i2c-7/name:i2c-2-mux-75 (chan_id 4)
>> i2c-8/name:i2c-2-mux-75 (chan_id 5)
>> i2c-9/name:i2c-2-mux-75 (chan_id 6)
>>
>> This retains the old mux name if the mux is not an i2c device, and adds
>> the i2c device address if it is. This solves the problem for me.
>>
>> Comments ?
>
> This "for me" worries me a bit. You basically restored the uniqueness
> for the multiple I2C-based multiplexer case. But for multiple
> GPIO-based multiplexers, for example, the name confusion still exists.

Only if some genius connects multiple separate gpio based multiplexers
to the same bus. In practice, though, that can be modeled as single
gpio mux with more pins, so I didn't think that is that much of a problem.

> If we are going to address this problem, I believe we should do so in a
> way which doesn't need revisiting in a couple months. You are changing
> adapter names people or tools may be relying on, we really don't want
> to do this too often.
>
Good point. One possible solution might be to add the mux type into the name.
	i2c-2-mux-i2c-76 (chan_id 0)
	i2c-2-mux-gpio-77 (chan_id 0)

We could add an additional parameter to the API, such as
	const char *qualifier
to provide the "i2c-76" or "gpio-77" string. If the qualifier is NULL,
the name would revert to the old naming scheme. Would that make sense ?

Thanks,
Guenter

> We could use the first GPIO pin number as the unique identifier for the
> GPIO case. However I2C addresses and GPIO pin numbers could collide...
> So we'd need separate name spaces, too.
>
> I agree this makes things even more complicated than they are right
> now, but I can't think of any other way that would be both robust and
> durable.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare - Nov. 18, 2013, 2:14 p.m.
Hi Guenter,

On Sun, 17 Nov 2013 13:13:43 -0800, Guenter Roeck wrote:
> On 11/17/2013 12:41 PM, Jean Delvare wrote:
> > On Sun, 17 Nov 2013 10:56:24 -0800, Guenter Roeck wrote:
> >> This retains the old mux name if the mux is not an i2c device, and adds
> >> the i2c device address if it is. This solves the problem for me.
> >>
> >> Comments ?
> >
> > This "for me" worries me a bit. You basically restored the uniqueness
> > for the multiple I2C-based multiplexer case. But for multiple
> > GPIO-based multiplexers, for example, the name confusion still exists.
> 
> Only if some genius connects multiple separate gpio based multiplexers
> to the same bus.

This is totally possible, for a variety of reasons, including physical
location of the GPIO pins and bus segment ends, but also economical
(using the same chips on several boards.)

> In practice, though, that can be modeled as single
> gpio mux with more pins, so I didn't think that is that much of a problem.

I admit I had not considered this. But would it work if the different
GPIO pins are on different chips (say south bridge and Super-I/O for
example)? The i2c-mux-gpio driver assumes a single chip, at least when
passed by name.

> > If we are going to address this problem, I believe we should do so in a
> > way which doesn't need revisiting in a couple months. You are changing
> > adapter names people or tools may be relying on, we really don't want
> > to do this too often.
>
> Good point. One possible solution might be to add the mux type into the name.
> 	i2c-2-mux-i2c-76 (chan_id 0)
> 	i2c-2-mux-gpio-77 (chan_id 0)

Yes, that's pretty much what I had in mind.

> We could add an additional parameter to the API, such as
> 	const char *qualifier
> to provide the "i2c-76" or "gpio-77" string. If the qualifier is NULL,
> the name would revert to the old naming scheme. Would that make sense ?

Yes, although your first draft gave me some hope that i2c-mux could do
some magic to guess the type and identifier automatically. But maybe
it's not that clean to have to touch i2c-mux for every new i2c-mux-*
driver. Not that I expect too many of them... so it's mostly a matter
of principle. An explicit qualifier as you suggest above might be
cleaner.
Guenter Roeck - Nov. 18, 2013, 5:14 p.m.
On Mon, Nov 18, 2013 at 03:14:41PM +0100, Jean Delvare wrote:
> Hi Guenter,
> 
> On Sun, 17 Nov 2013 13:13:43 -0800, Guenter Roeck wrote:
> > On 11/17/2013 12:41 PM, Jean Delvare wrote:
> > > On Sun, 17 Nov 2013 10:56:24 -0800, Guenter Roeck wrote:
> > >> This retains the old mux name if the mux is not an i2c device, and adds
> > >> the i2c device address if it is. This solves the problem for me.
> > >>
> > >> Comments ?
> > >
> > > This "for me" worries me a bit. You basically restored the uniqueness
> > > for the multiple I2C-based multiplexer case. But for multiple
> > > GPIO-based multiplexers, for example, the name confusion still exists.
> > 
> > Only if some genius connects multiple separate gpio based multiplexers
> > to the same bus.
> 
> This is totally possible, for a variety of reasons, including physical
> location of the GPIO pins and bus segment ends, but also economical
> (using the same chips on several boards.)
> 
Ok.

> > In practice, though, that can be modeled as single
> > gpio mux with more pins, so I didn't think that is that much of a problem.
> 
> I admit I had not considered this. But would it work if the different
> GPIO pins are on different chips (say south bridge and Super-I/O for
> example)? The i2c-mux-gpio driver assumes a single chip, at least when
> passed by name.
> 
... so you could or should not pass a name if that is the case.

> > > If we are going to address this problem, I believe we should do so in a
> > > way which doesn't need revisiting in a couple months. You are changing
> > > adapter names people or tools may be relying on, we really don't want
> > > to do this too often.
> >
> > Good point. One possible solution might be to add the mux type into the name.
> > 	i2c-2-mux-i2c-76 (chan_id 0)
> > 	i2c-2-mux-gpio-77 (chan_id 0)
> 
> Yes, that's pretty much what I had in mind.
> 
> > We could add an additional parameter to the API, such as
> > 	const char *qualifier
> > to provide the "i2c-76" or "gpio-77" string. If the qualifier is NULL,
> > the name would revert to the old naming scheme. Would that make sense ?
> 
> Yes, although your first draft gave me some hope that i2c-mux could do
> some magic to guess the type and identifier automatically. But maybe
> it's not that clean to have to touch i2c-mux for every new i2c-mux-*
> driver. Not that I expect too many of them... so it's mostly a matter
> of principle. An explicit qualifier as you suggest above might be
> cleaner.
> 
Another option would be to use the patch I provided, except to create
names such as i2c-2-mux-i2c-76. The API change could then be added later
if/as needed.

But, yes, an API change would be cleaner. Before I submit a "final" patch,
it would be great to get feedback and direcetion from Wolfram ... after all,
he'll have to approve it at the end.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 797e311..9a49fe9 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -109,7 +109,9 @@  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 				int (*deselect) (struct i2c_adapter *,
 						 void *, u32))
 {
+	struct i2c_client *client = i2c_verify_client(mux_dev);
 	struct i2c_mux_priv *priv;
+	char client_addr[8];
 	int ret;
 
 	priv = kzalloc(sizeof(struct i2c_mux_priv), GFP_KERNEL);
@@ -133,8 +135,15 @@  struct i2c_adapter *i2c_add_mux_adapter(struct i2c_adapter *parent,
 	priv->algo.functionality = i2c_mux_functionality;
 
 	/* Now fill out new adapter structure */
+	if (client)
+		scnprintf(client_addr, sizeof(client_addr), "-%02x",
+			  client->addr);
+	else
+		client_addr[0] = '\0';
+
 	snprintf(priv->adap.name, sizeof(priv->adap.name),
-		 "i2c-%d-mux (chan_id %d)", i2c_adapter_id(parent), chan_id);
+		 "i2c-%d-mux%s (chan_id %d)", i2c_adapter_id(parent),
+		 client_addr, chan_id);
 	priv->adap.owner = THIS_MODULE;
 	priv->adap.algo = &priv->algo;
 	priv->adap.algo_data = priv;