[PATCH RESEND] i2c: mux: pca9541: Change bus control commands and release control of bus

[PATCH RESEND] i2c: mux: pca9541: Change bus control commands and release control of bus

[Quentin Strydom]

Change current bus commands to match the pca9541a datasheet
(see table 12 on page 14 of
https://www.nxp.com/docs/en/data-sheet/PCA9541A.pdf).

Also add change so that previous master releases control of bus.

Signed-off-by: Quentin Strydom <qstrydom0@gmail.com>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 50e1fb4aedf5..eb2552fbd0d0 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -134,7 +134,8 @@ static void pca9541_release_bus(struct i2c_client *client)
 	reg = pca9541_reg_read(client, PCA9541_CONTROL);
 	if (reg >= 0 && !busoff(reg) && mybus(reg))
 		pca9541_reg_write(client, PCA9541_CONTROL,
-				  (reg & PCA9541_CTL_NBUSON) >> 1);
+				 (reg & (PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS))
+				 ^ (PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS));
 }
 
 /*
@@ -163,7 +164,7 @@ static void pca9541_release_bus(struct i2c_client *client)
 
 /* Control commands per PCA9541 datasheet */
 static const u8 pca9541_control[16] = {
-	4, 0, 1, 5, 4, 4, 5, 5, 0, 0, 1, 1, 0, 4, 5, 1
+	4, 4, 5, 5, 4, 4, 5, 5, 0, 0, 1, 1, 0, 0, 1, 1
 };
 
 /*
-- 
2.17.1

Re: [PATCH RESEND] i2c: mux: pca9541: Change bus control commands and release control of bus

[Peter Rosin]

On 2020-06-03 21:14, Quentin Strydom wrote:
> Change current bus commands to match the pca9541a datasheet
> (see table 12 on page 14 of
> https://www.nxp.com/docs/en/data-sheet/PCA9541A.pdf).
> 
> Also add change so that previous master releases control of bus.
> 
> Signed-off-by: Quentin Strydom <qstrydom0@gmail.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 50e1fb4aedf5..eb2552fbd0d0 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -134,7 +134,8 @@ static void pca9541_release_bus(struct i2c_client *client)
>  	reg = pca9541_reg_read(client, PCA9541_CONTROL);
>  	if (reg >= 0 && !busoff(reg) && mybus(reg))
>  		pca9541_reg_write(client, PCA9541_CONTROL,
> -				  (reg & PCA9541_CTL_NBUSON) >> 1);
> +				 (reg & (PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS))

Hi!

First, some nits.

The initial ( should be aligned just like the removed line.

> +				 ^ (PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS));

The ^ operator should be on the previous line.

However, this is also a quite complex argument. Some kind of temporary variable
would probably help clarify what is going on. Like so perhaps:

	if (...) {
		reg &= PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS;
		reg ^= PCA9541_CTL_BUSON | PCA9541_CTL_MYBUS;
		pca9541_reg_write(client, PCA9541_CONTROL, reg);
	}

>  }
>  
>  /*
> @@ -163,7 +164,7 @@ static void pca9541_release_bus(struct i2c_client *client)
>  
>  /* Control commands per PCA9541 datasheet */
>  static const u8 pca9541_control[16] = {
> -	4, 0, 1, 5, 4, 4, 5, 5, 0, 0, 1, 1, 0, 4, 5, 1
> +	4, 4, 5, 5, 4, 4, 5, 5, 0, 0, 1, 1, 0, 0, 1, 1
>  };
>  
>  /*
> 

I'm sorry, and I know you're new at this, but this is not a "resend". A resend is
the exakt same patch and exact same commit message etc, and the only difference
is that you point out in the subject that this is a resend so that we maintainers
can spot things that we have failed to take care of in an orderly fashion.

When you change things around like you have done, the protocol is to mark each
new version with [PATCH v2], [PATCH v3] etc. If you have a 2-patch series, the
patches might be [PATCH v4 1/2] and [PATCH v4 2/2]. How else are we going to
know which patch/series *you* think is the latest and greatest?

When you do a new version, it is also extremely welcome with a description of what
changed since the last version, and why this was changed. The why part is very
often much more interesting and revealing than the what. But maintainers want
*both*. At least I do.

Also, you should usually allow some time for maintainers to react. I for one do this
on my spare time. You are the one wanting some change, so you should make it easy
for me to follow your reasoning.

I believe all of this is documented in Documentation/process/submitting-patches.rst

Drilling down in the specifics of this patch, I very much preferred it when these
two completely orthogonal changes were two patches. If one change is problematic,
that makes it much easier to undo just that change. And since you have now
presented four(?) different versions of the release hunk, I need more data on
exactly what was problematic with each of the previous iterations and what made
you send each of those versions out in the first place. Otherwise I'm just going
to expect a fifth version tomorrow, so why bother with the fourth today?

I would also like details on the testing this patch has received, and that
description fit nicely in the commit message. It's small one-liner patches like
this that require the most documentation, because it is so hard to believe that
this has been wrong for years without anyone noticing.

And I don't think pointing to the datasheet is enough in this case, since
datasheets are not always in line with reality. I'd like further descriptions
that details why the new way of doing things is better. You indicated somewhere
that the current code leads to double arbitration. That is exactly the things
that you should put in your commit message, with as much details as you have.

If you have looked with a scope, please state so in the commit message and add
some description of what you saw. Things like that.

Another example, I fail to see why it is a good idea to invert the PCA9541_CTL_BUSON
bit. The naming indicates that the bus is off, and it sure looks like inverting
that bit will make the bus go on. Why would you want that when you release the
bus? It's things like this needs explanations in the commit message and/or
comments in the code.

Gunther stating that he did lots of testing and that it was hard to get this
working originally also raises the bar for you.

I hope I'm not coming down too hard on you, and I do not want you to go away
or anything like that. I very much want you to push through with this! Please!
Because I do believe that your are correct in that the current code is not
right, I just need more information and I see no other source than you when
Gunther no longer has the HW (and datasheets cannot be universally trusted).

And others following this, please test if you can!

Cheers,
Peter