Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Peter Rosin]

On 2018-03-21 08:01, Vladimir Zapolskiy wrote:
> On 03/21/2018 03:19 AM, Guenter Roeck wrote:
>> On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:
>>> Hi Peter, Ken,
>>>
>>> On 03/20/2018 11:32 AM, Peter Rosin wrote:
 Make the arbitrate and release_bus implementation chip specific.

>>>
>>> by chance I took a look at the original implementation done by Ken, and
>>> I would say that this 3/3 change is an overkill as a too generic one.
>>> Is there any next observable extension? And do two abstracted (*arbitrate)
>>> and (*release_bus) cover it well? Probably no.
>>>
>>> At first it would be simpler to add a new chip id field into struct pca9541
>>> (struct rename would be needed of course), and do a selection of specific
>>> pca9x41_arbitrate() and pca9x41_release_bus() depending on it:
>>>
>>
>> FWIW, I very much prefer Peter's code. I think it is much cleaner.
> 
> Peter's code is generic, and it makes the change about 3 times longer in lines
> of code, and the following pca9641 change on top of it will be larger as well,
> because generalization requires service.
> 
> My main concern is that if such generalization is really needed in the driver.

I just did a comparison of what would happen if I took the same shortcuts
you did, and I got 18 new lines and 3 changed lines (and some moved lines
that could have been a separate patch). You have 12 new lines and 5 changed
lines.

So, the big difference is that I add the of_match_device call while you
do not. So, it looks like you are comparing apples and oranges. Do you
have a reason for not calling of_match_device? Or were you punting that
for the patch adding PCA9641 support? That's odd, because the point of
the patch is to prepare for smooth addition of that support.

Also, I think my code allows adding support for PCA9641 with only new
lines, while your version requires changing of code.

So, I'm rejecting your arguments that your patch is significantly simpler.
And while I'm obviously a tad bit biased, I do agree with Guenter that
my structure is cleaner.

Cheers,
Peter

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Peter Rosin]

On 2018-03-21 08:01, Vladimir Zapolskiy wrote:
> On 03/21/2018 03:19 AM, Guenter Roeck wrote:
>> On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:
>>> Hi Peter, Ken,
>>>
>>> On 03/20/2018 11:32 AM, Peter Rosin wrote:
 Make the arbitrate and release_bus implementation chip specific.

>>>
>>> by chance I took a look at the original implementation done by Ken, and
>>> I would say that this 3/3 change is an overkill as a too generic one.
>>> Is there any next observable extension? And do two abstracted (*arbitrate)
>>> and (*release_bus) cover it well? Probably no.
>>>
>>> At first it would be simpler to add a new chip id field into struct pca9541
>>> (struct rename would be needed of course), and do a selection of specific
>>> pca9x41_arbitrate() and pca9x41_release_bus() depending on it:
>>>
>>
>> FWIW, I very much prefer Peter's code. I think it is much cleaner.
> 
> Peter's code is generic, and it makes the change about 3 times longer in lines
> of code, and the following pca9641 change on top of it will be larger as well,
> because generalization requires service.
> 
> My main concern is that if such generalization is really needed in the driver.

I just did a comparison of what would happen if I took the same shortcuts
you did, and I got 18 new lines and 3 changed lines (and some moved lines
that could have been a separate patch). You have 12 new lines and 5 changed
lines.

So, the big difference is that I add the of_match_device call while you
do not. So, it looks like you are comparing apples and oranges. Do you
have a reason for not calling of_match_device? Or were you punting that
for the patch adding PCA9641 support? That's odd, because the point of
the patch is to prepare for smooth addition of that support.

Also, I think my code allows adding support for PCA9641 with only new
lines, while your version requires changing of code.

So, I'm rejecting your arguments that your patch is significantly simpler.
And while I'm obviously a tad bit biased, I do agree with Guenter that
my structure is cleaner.

Cheers,
Peter

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 
> Signed-off-by: Peter Rosin 

Reviewed-by: Vladimir Zapolskiy 


The change is really good and correct, it is just too extended IMHO.

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 
> Signed-off-by: Peter Rosin 

Reviewed-by: Vladimir Zapolskiy 


The change is really good and correct, it is just too extended IMHO.

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

On 03/21/2018 03:19 AM, Guenter Roeck wrote:
> On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:
>> Hi Peter, Ken,
>>
>> On 03/20/2018 11:32 AM, Peter Rosin wrote:
>>> Make the arbitrate and release_bus implementation chip specific.
>>>
>>
>> by chance I took a look at the original implementation done by Ken, and
>> I would say that this 3/3 change is an overkill as a too generic one.
>> Is there any next observable extension? And do two abstracted (*arbitrate)
>> and (*release_bus) cover it well? Probably no.
>>
>> At first it would be simpler to add a new chip id field into struct pca9541
>> (struct rename would be needed of course), and do a selection of specific
>> pca9x41_arbitrate() and pca9x41_release_bus() depending on it:
>>
> 
> FWIW, I very much prefer Peter's code. I think it is much cleaner.

Peter's code is generic, and it makes the change about 3 times longer in lines
of code, and the following pca9641 change on top of it will be larger as well,
because generalization requires service.

My main concern is that if such generalization is really needed in the driver.

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

On 03/21/2018 03:19 AM, Guenter Roeck wrote:
> On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:
>> Hi Peter, Ken,
>>
>> On 03/20/2018 11:32 AM, Peter Rosin wrote:
>>> Make the arbitrate and release_bus implementation chip specific.
>>>
>>
>> by chance I took a look at the original implementation done by Ken, and
>> I would say that this 3/3 change is an overkill as a too generic one.
>> Is there any next observable extension? And do two abstracted (*arbitrate)
>> and (*release_bus) cover it well? Probably no.
>>
>> At first it would be simpler to add a new chip id field into struct pca9541
>> (struct rename would be needed of course), and do a selection of specific
>> pca9x41_arbitrate() and pca9x41_release_bus() depending on it:
>>
> 
> FWIW, I very much prefer Peter's code. I think it is much cleaner.

Peter's code is generic, and it makes the change about 3 times longer in lines
of code, and the following pca9641 change on top of it will be larger as well,
because generalization requires service.

My main concern is that if such generalization is really needed in the driver.

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Guenter Roeck]

On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:

Hi Peter, Ken,

On 03/20/2018 11:32 AM, Peter Rosin wrote:

Make the arbitrate and release_bus implementation chip specific.



by chance I took a look at the original implementation done by Ken, and
I would say that this 3/3 change is an overkill as a too generic one.
Is there any next observable extension? And do two abstracted (*arbitrate)
and (*release_bus) cover it well? Probably no.

At first it would be simpler to add a new chip id field into struct pca9541
(struct rename would be needed of course), and do a selection of specific
pca9x41_arbitrate() and pca9x41_release_bus() depending on it:



FWIW, I very much prefer Peter's code. I think it is much cleaner.

Guenter


8<
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb..a40e6d8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -70,8 +70,13 @@
  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
+enum chip_id {

+   pca9541,
+};
+
  struct pca9541 {
struct i2c_client *client;
+   enum chip_id id;
unsigned long select_timeout;
unsigned long arb_timeout;
  };
@@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 
command)
   */
  
  /* Release bus. Also reset NTESTON and BUSINIT if it was set. */

-static void pca9541_release_bus(struct i2c_client *client)
+static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
  {
int reg;
  
+	if (id != pca9541)

+   return;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
pca9541_reg_write(client, PCA9541_CONTROL,
@@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
   *  0 : bus not acquired
   *  1 : bus acquired
   */
-static int pca9541_arbitrate(struct i2c_client *client)
+static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
  {
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
struct pca9541 *data = i2c_mux_priv(muxc);
int reg;
  
+	if (id != pca9541)

+   return -EOPNOTSUPP;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg < 0)
return reg;
@@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
  
  	do {

-   ret = pca9541_arbitrate(client);
+   ret = pca9541_arbitrate(client, data->id);
if (ret)
return ret < 0 ? ret : 0;
  
@@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)

struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
  
-	pca9541_release_bus(client);

+   pca9541_release_bus(client, data->id);
return 0;
  }
  
@@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,

 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, pca9541);
i2c_unlock_adapter(adap);
  
  	/* Create mux adapter */

@@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
  
  	data = i2c_mux_priv(muxc);

data->client = client;
+   data->id = pca9541;
  
  	i2c_set_clientdata(client, muxc);
  
8<




Signed-off-by: Peter Rosin 
---
  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++--
  1 file changed, 45 insertions(+), 17 deletions(-)



The change above is trivial and it does not cancel any further
extensions similar to your idea, the open question is if there
is a demand right at the moment.


diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb4e0e9..cac629e36bf8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -70,26 +71,22 @@

  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
-struct pca9541 {

-   struct i2c_client *client;
-   unsigned long select_timeout;
-   unsigned long arb_timeout;
+enum chip_name {


chip_name sound like a string storage, chip_id might be better here.


+   pca9541,
  };
  


--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Guenter Roeck]

On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:

Hi Peter, Ken,

On 03/20/2018 11:32 AM, Peter Rosin wrote:

Make the arbitrate and release_bus implementation chip specific.



by chance I took a look at the original implementation done by Ken, and
I would say that this 3/3 change is an overkill as a too generic one.
Is there any next observable extension? And do two abstracted (*arbitrate)
and (*release_bus) cover it well? Probably no.

At first it would be simpler to add a new chip id field into struct pca9541
(struct rename would be needed of course), and do a selection of specific
pca9x41_arbitrate() and pca9x41_release_bus() depending on it:



FWIW, I very much prefer Peter's code. I think it is much cleaner.

Guenter


8<
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb..a40e6d8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -70,8 +70,13 @@
  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
+enum chip_id {

+   pca9541,
+};
+
  struct pca9541 {
struct i2c_client *client;
+   enum chip_id id;
unsigned long select_timeout;
unsigned long arb_timeout;
  };
@@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 
command)
   */
  
  /* Release bus. Also reset NTESTON and BUSINIT if it was set. */

-static void pca9541_release_bus(struct i2c_client *client)
+static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
  {
int reg;
  
+	if (id != pca9541)

+   return;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
pca9541_reg_write(client, PCA9541_CONTROL,
@@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
   *  0 : bus not acquired
   *  1 : bus acquired
   */
-static int pca9541_arbitrate(struct i2c_client *client)
+static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
  {
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
struct pca9541 *data = i2c_mux_priv(muxc);
int reg;
  
+	if (id != pca9541)

+   return -EOPNOTSUPP;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg < 0)
return reg;
@@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
  
  	do {

-   ret = pca9541_arbitrate(client);
+   ret = pca9541_arbitrate(client, data->id);
if (ret)
return ret < 0 ? ret : 0;
  
@@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)

struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
  
-	pca9541_release_bus(client);

+   pca9541_release_bus(client, data->id);
return 0;
  }
  
@@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,

 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, pca9541);
i2c_unlock_adapter(adap);
  
  	/* Create mux adapter */

@@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
  
  	data = i2c_mux_priv(muxc);

data->client = client;
+   data->id = pca9541;
  
  	i2c_set_clientdata(client, muxc);
  
8<




Signed-off-by: Peter Rosin 
---
  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++--
  1 file changed, 45 insertions(+), 17 deletions(-)



The change above is trivial and it does not cancel any further
extensions similar to your idea, the open question is if there
is a demand right at the moment.


diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb4e0e9..cac629e36bf8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -70,26 +71,22 @@

  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
-struct pca9541 {

-   struct i2c_client *client;
-   unsigned long select_timeout;
-   unsigned long arb_timeout;
+enum chip_name {


chip_name sound like a string storage, chip_id might be better here.


+   pca9541,
  };
  


--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

Hi Peter, Ken,

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 

by chance I took a look at the original implementation done by Ken, and
I would say that this 3/3 change is an overkill as a too generic one.
Is there any next observable extension? And do two abstracted (*arbitrate)
and (*release_bus) cover it well? Probably no.

At first it would be simpler to add a new chip id field into struct pca9541
(struct rename would be needed of course), and do a selection of specific
pca9x41_arbitrate() and pca9x41_release_bus() depending on it:

8<
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb..a40e6d8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -70,8 +70,13 @@
 #define SELECT_DELAY_SHORT 50
 #define SELECT_DELAY_LONG  1000
 
+enum chip_id {
+   pca9541,
+};
+
 struct pca9541 {
struct i2c_client *client;
+   enum chip_id id;
unsigned long select_timeout;
unsigned long arb_timeout;
 };
@@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 
command)
  */
 
 /* Release bus. Also reset NTESTON and BUSINIT if it was set. */
-static void pca9541_release_bus(struct i2c_client *client)
+static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
 {
int reg;
 
+   if (id != pca9541)
+   return;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
pca9541_reg_write(client, PCA9541_CONTROL,
@@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
  *  0 : bus not acquired
  *  1 : bus acquired
  */
-static int pca9541_arbitrate(struct i2c_client *client)
+static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
 {
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
struct pca9541 *data = i2c_mux_priv(muxc);
int reg;
 
+   if (id != pca9541)
+   return -EOPNOTSUPP;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg < 0)
return reg;
@@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
 
do {
-   ret = pca9541_arbitrate(client);
+   ret = pca9541_arbitrate(client, data->id);
if (ret)
return ret < 0 ? ret : 0;
 
@@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, 
u32 chan)
struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
 
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, data->id);
return 0;
 }
 
@@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,
 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, pca9541);
i2c_unlock_adapter(adap);
 
/* Create mux adapter */
@@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
 
data = i2c_mux_priv(muxc);
data->client = client;
+   data->id = pca9541;
 
i2c_set_clientdata(client, muxc);
 
8<


> Signed-off-by: Peter Rosin 
> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 
> +++--
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 

The change above is trivial and it does not cancel any further
extensions similar to your idea, the open question is if there
is a demand right at the moment.

> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
> b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 47685eb4e0e9..cac629e36bf8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -70,26 +71,22 @@
>  #define SELECT_DELAY_SHORT   50
>  #define SELECT_DELAY_LONG1000
>  
> -struct pca9541 {
> - struct i2c_client *client;
> - unsigned long select_timeout;
> - unsigned long arb_timeout;
> +enum chip_name {

chip_name sound like a string storage, chip_id might be better here.

> + pca9541,
>  };
>  

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Vladimir Zapolskiy]

Hi Peter, Ken,

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 

by chance I took a look at the original implementation done by Ken, and
I would say that this 3/3 change is an overkill as a too generic one.
Is there any next observable extension? And do two abstracted (*arbitrate)
and (*release_bus) cover it well? Probably no.

At first it would be simpler to add a new chip id field into struct pca9541
(struct rename would be needed of course), and do a selection of specific
pca9x41_arbitrate() and pca9x41_release_bus() depending on it:

8<
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb..a40e6d8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -70,8 +70,13 @@
 #define SELECT_DELAY_SHORT 50
 #define SELECT_DELAY_LONG  1000
 
+enum chip_id {
+   pca9541,
+};
+
 struct pca9541 {
struct i2c_client *client;
+   enum chip_id id;
unsigned long select_timeout;
unsigned long arb_timeout;
 };
@@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 
command)
  */
 
 /* Release bus. Also reset NTESTON and BUSINIT if it was set. */
-static void pca9541_release_bus(struct i2c_client *client)
+static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
 {
int reg;
 
+   if (id != pca9541)
+   return;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
pca9541_reg_write(client, PCA9541_CONTROL,
@@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
  *  0 : bus not acquired
  *  1 : bus acquired
  */
-static int pca9541_arbitrate(struct i2c_client *client)
+static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
 {
struct i2c_mux_core *muxc = i2c_get_clientdata(client);
struct pca9541 *data = i2c_mux_priv(muxc);
int reg;
 
+   if (id != pca9541)
+   return -EOPNOTSUPP;
+
reg = pca9541_reg_read(client, PCA9541_CONTROL);
if (reg < 0)
return reg;
@@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
 
do {
-   ret = pca9541_arbitrate(client);
+   ret = pca9541_arbitrate(client, data->id);
if (ret)
return ret < 0 ? ret : 0;
 
@@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, 
u32 chan)
struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
 
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, data->id);
return 0;
 }
 
@@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,
 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   pca9541_release_bus(client, pca9541);
i2c_unlock_adapter(adap);
 
/* Create mux adapter */
@@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
 
data = i2c_mux_priv(muxc);
data->client = client;
+   data->id = pca9541;
 
i2c_set_clientdata(client, muxc);
 
8<


> Signed-off-by: Peter Rosin 
> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 
> +++--
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 

The change above is trivial and it does not cancel any further
extensions similar to your idea, the open question is if there
is a demand right at the moment.

> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
> b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 47685eb4e0e9..cac629e36bf8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -70,26 +71,22 @@
>  #define SELECT_DELAY_SHORT   50
>  #define SELECT_DELAY_LONG1000
>  
> -struct pca9541 {
> - struct i2c_client *client;
> - unsigned long select_timeout;
> - unsigned long arb_timeout;
> +enum chip_name {

chip_name sound like a string storage, chip_id might be better here.

> + pca9541,
>  };
>  

--
With best wishes,
Vladimir

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Guenter Roeck]

On 03/20/2018 02:32 AM, Peter Rosin wrote:

Make the arbitrate and release_bus implementation chip specific.

Signed-off-by: Peter Rosin 


Reviewed-by: Guenter Roeck 


---
  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++--
  1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb4e0e9..cac629e36bf8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -70,26 +71,22 @@

  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
-struct pca9541 {

-   struct i2c_client *client;
-   unsigned long select_timeout;
-   unsigned long arb_timeout;
+enum chip_name {
+   pca9541,
  };
  
-static const struct i2c_device_id pca9541_id[] = {

-   {"pca9541", 0},
-   {}
+struct chip_desc {
+   int (*arbitrate)(struct i2c_client *client);
+   void (*release_bus)(struct i2c_client *client);
  };
  
-MODULE_DEVICE_TABLE(i2c, pca9541_id);

+struct pca9541 {
+   const struct chip_desc *chip;
  
-#ifdef CONFIG_OF

-static const struct of_device_id pca9541_of_match[] = {
-   { .compatible = "nxp,pca9541" },
-   {}
+   struct i2c_client *client;
+   unsigned long select_timeout;
+   unsigned long arb_timeout;
  };
-MODULE_DEVICE_TABLE(of, pca9541_of_match);
-#endif
  
  static int pca9541_mybus(int ctl)

  {
@@ -318,7 +315,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
  
  	do {

-   ret = pca9541_arbitrate(client);
+   ret = data->chip->arbitrate(client);
if (ret)
return ret < 0 ? ret : 0;
  
@@ -336,10 +333,32 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)

struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
  
-	pca9541_release_bus(client);

+   data->chip->release_bus(client);
return 0;
  }
  
+static const struct chip_desc chips[] = {

+   [pca9541] = {
+   .arbitrate = pca9541_arbitrate,
+   .release_bus = pca9541_release_bus,
+   },
+};
+
+static const struct i2c_device_id pca9541_id[] = {
+   { "pca9541", pca9541 },
+   {}
+};
+
+MODULE_DEVICE_TABLE(i2c, pca9541_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id pca9541_of_match[] = {
+   { .compatible = "nxp,pca9541", .data = [pca9541] },
+   {}
+};
+MODULE_DEVICE_TABLE(of, pca9541_of_match);
+#endif
+
  /*
   * I2C init/probing/exit functions
   */
@@ -348,6 +367,8 @@ static int pca9541_probe(struct i2c_client *client,
  {
struct i2c_adapter *adap = client->adapter;
struct pca954x_platform_data *pdata = dev_get_platdata(>dev);
+   const struct of_device_id *match;
+   const struct chip_desc *chip;
struct i2c_mux_core *muxc;
struct pca9541 *data;
int force;
@@ -356,12 +377,18 @@ static int pca9541_probe(struct i2c_client *client,
if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
return -ENODEV;
  
+	match = of_match_device(of_match_ptr(pca9541_of_match), >dev);

+   if (match)
+   chip = of_device_get_match_data(>dev);
+   else
+   chip = [id->driver_data];
+
/*
 * I2C accesses are unprotected here.
 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   chip->release_bus(client);
i2c_unlock_adapter(adap);
  
  	/* Create mux adapter */

@@ -376,6 +403,7 @@ static int pca9541_probe(struct i2c_client *client,
return -ENOMEM;
  
  	data = i2c_mux_priv(muxc);

+   data->chip = chip;
data->client = client;
  
  	i2c_set_clientdata(client, muxc);

Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

[Guenter Roeck]

On 03/20/2018 02:32 AM, Peter Rosin wrote:

Make the arbitrate and release_bus implementation chip specific.

Signed-off-by: Peter Rosin 


Reviewed-by: Guenter Roeck 


---
  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++--
  1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c 
b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb4e0e9..cac629e36bf8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -70,26 +71,22 @@

  #define SELECT_DELAY_SHORT50
  #define SELECT_DELAY_LONG 1000
  
-struct pca9541 {

-   struct i2c_client *client;
-   unsigned long select_timeout;
-   unsigned long arb_timeout;
+enum chip_name {
+   pca9541,
  };
  
-static const struct i2c_device_id pca9541_id[] = {

-   {"pca9541", 0},
-   {}
+struct chip_desc {
+   int (*arbitrate)(struct i2c_client *client);
+   void (*release_bus)(struct i2c_client *client);
  };
  
-MODULE_DEVICE_TABLE(i2c, pca9541_id);

+struct pca9541 {
+   const struct chip_desc *chip;
  
-#ifdef CONFIG_OF

-static const struct of_device_id pca9541_of_match[] = {
-   { .compatible = "nxp,pca9541" },
-   {}
+   struct i2c_client *client;
+   unsigned long select_timeout;
+   unsigned long arb_timeout;
  };
-MODULE_DEVICE_TABLE(of, pca9541_of_match);
-#endif
  
  static int pca9541_mybus(int ctl)

  {
@@ -318,7 +315,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, 
u32 chan)
/* force bus ownership after this time */
  
  	do {

-   ret = pca9541_arbitrate(client);
+   ret = data->chip->arbitrate(client);
if (ret)
return ret < 0 ? ret : 0;
  
@@ -336,10 +333,32 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)

struct pca9541 *data = i2c_mux_priv(muxc);
struct i2c_client *client = data->client;
  
-	pca9541_release_bus(client);

+   data->chip->release_bus(client);
return 0;
  }
  
+static const struct chip_desc chips[] = {

+   [pca9541] = {
+   .arbitrate = pca9541_arbitrate,
+   .release_bus = pca9541_release_bus,
+   },
+};
+
+static const struct i2c_device_id pca9541_id[] = {
+   { "pca9541", pca9541 },
+   {}
+};
+
+MODULE_DEVICE_TABLE(i2c, pca9541_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id pca9541_of_match[] = {
+   { .compatible = "nxp,pca9541", .data = [pca9541] },
+   {}
+};
+MODULE_DEVICE_TABLE(of, pca9541_of_match);
+#endif
+
  /*
   * I2C init/probing/exit functions
   */
@@ -348,6 +367,8 @@ static int pca9541_probe(struct i2c_client *client,
  {
struct i2c_adapter *adap = client->adapter;
struct pca954x_platform_data *pdata = dev_get_platdata(>dev);
+   const struct of_device_id *match;
+   const struct chip_desc *chip;
struct i2c_mux_core *muxc;
struct pca9541 *data;
int force;
@@ -356,12 +377,18 @@ static int pca9541_probe(struct i2c_client *client,
if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
return -ENODEV;
  
+	match = of_match_device(of_match_ptr(pca9541_of_match), >dev);

+   if (match)
+   chip = of_device_get_match_data(>dev);
+   else
+   chip = [id->driver_data];
+
/*
 * I2C accesses are unprotected here.
 * We have to lock the adapter before releasing the bus.
 */
i2c_lock_adapter(adap);
-   pca9541_release_bus(client);
+   chip->release_bus(client);
i2c_unlock_adapter(adap);
  
  	/* Create mux adapter */

@@ -376,6 +403,7 @@ static int pca9541_probe(struct i2c_client *client,
return -ENOMEM;
  
  	data = i2c_mux_priv(muxc);

+   data->chip = chip;
data->client = client;
  
  	i2c_set_clientdata(client, muxc);