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@xxxxxxxxx> > --- > 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
