diff mbox series

[v4,1/1] i2c: algo-pca: Reapply i2c bus settings after reset

Message ID 20200908203247.14374-1-evan.nimmo@alliedtelesis.co.nz
State Accepted
Commit 0a355aeb24081e4538d4d424cd189f16c0bbd983
Headers show
Series [v4,1/1] i2c: algo-pca: Reapply i2c bus settings after reset | expand

Commit Message

Evan Nimmo Sept. 8, 2020, 8:32 p.m. UTC 
If something goes wrong (such as the SCL being stuck low) then we need
to reset the PCA chip. The issue with this is that on reset we lose all
config settings and the chip ends up in a disabled state which results
in a lock up/high CPU usage. We need to re-apply any configuration that
had previously been set and re-enable the chip.

Signed-off-by: Evan Nimmo <evan.nimmo@alliedtelesis.co.nz>
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Wolfram Sang <wsa@kernel.org>
---
changes in v2:
- changed lowercase "pca to uppercase "PCA".
- reworded and reformatted the multiline comment.
- moved the clock frequency KERN_INFO closer to the call that sets this.
- moved the i2c_bus_settings struct to the more generic i2c.h and removed
- the comments indicating this as being for the pca chip.

changes in v3:
- changed lowercase "cpu" to uppercase "CPU".

changes in v4:
- renamed the struct i2c_bus_settings struct to struct pca_i2c_bus_settings.
- moved the struct pca_i2c_bus_settings struct from i2c.h to i2c-algo-pca.h.
- adjusted the struct field in the struct i2c_algo_pca_data struct to be of type
- struct pca_i2c_bus settings.
- removed an extra line in i2c-algo-pca.c

 drivers/i2c/algos/i2c-algo-pca.c | 35 +++++++++++++++++++++-----------
 include/linux/i2c-algo-pca.h     | 15 ++++++++++++++
 2 files changed, 38 insertions(+), 12 deletions(-)

Comments

Wolfram Sang Sept. 9, 2020, 8:23 a.m. UTC | #1 
On Wed, Sep 09, 2020 at 08:32:47AM +1200, Evan Nimmo wrote:
> If something goes wrong (such as the SCL being stuck low) then we need
> to reset the PCA chip. The issue with this is that on reset we lose all
> config settings and the chip ends up in a disabled state which results
> in a lock up/high CPU usage. We need to re-apply any configuration that
> had previously been set and re-enable the chip.
> 
> Signed-off-by: Evan Nimmo <evan.nimmo@alliedtelesis.co.nz>
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Wolfram Sang <wsa@kernel.org>

Applied to for-current, thanks!

For the record, were you able to test both, PCA9564 and PCA9665?
Chris Packham Sept. 13, 2020, 9:23 p.m. UTC | #2 
On 12/09/20 7:45 am, Wolfram Sang wrote:
> On Wed, Sep 09, 2020 at 08:39:50PM +0000, Chris Packham wrote:
>> On 9/09/20 8:23 pm, Wolfram Sang wrote:
>>> On Wed, Sep 09, 2020 at 08:32:47AM +1200, Evan Nimmo wrote:
>>>> If something goes wrong (such as the SCL being stuck low) then we need
>>>> to reset the PCA chip. The issue with this is that on reset we lose all
>>>> config settings and the chip ends up in a disabled state which results
>>>> in a lock up/high CPU usage. We need to re-apply any configuration that
>>>> had previously been set and re-enable the chip.
>>>>
>>>> Signed-off-by: Evan Nimmo <evan.nimmo@alliedtelesis.co.nz>
>>>> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>> Reviewed-by: Wolfram Sang <wsa@kernel.org>
>>> Applied to for-current, thanks!
>>>
>>> For the record, were you able to test both, PCA9564 and PCA9665?
>>>
>> Our hardware platforms only have PCA9665 so that's all we can test.
> Okay, good to know. BTW, just after I sent out my pull request
> containing this patch, I noticed there is no Fixes: tag. So, if you want
> this patch to be backported, please send it to stable@ once my
> pull-request is in Linus' tree.

For our purposes being in Linus's tree is good enough. We've already 
back-ported it to our kernel fork (which doesn't really track any of the 
stable branches properly).

I'm happy to route it to stable@ if you think it's worth it but I don't 
think there's a specific Fixes: reference that can be used. The current 
behavior appears to have been that way since before git (looks like we 
noticed in 2014 but it's taken me 6 years to nag people into sending 
their fixes upstream).
Wolfram Sang Sept. 14, 2020, 6:49 a.m. UTC | #3 
> I'm happy to route it to stable@ if you think it's worth it but I don't 
> think there's a specific Fixes: reference that can be used. The current 
> behavior appears to have been that way since before git (looks like we 
> noticed in 2014 but it's taken me 6 years to nag people into sending 
> their fixes upstream).

Better late than never :) Thanks for sending and the heads up. If you
don't need it for stable, then we should maybe not send it. It would be
nice for 9665 but we haven't tested it on 9564. But I'll let it be your
call.
Andy Shevchenko Sept. 14, 2020, 8:50 a.m. UTC | #4 
On Mon, Sep 14, 2020 at 12:27 AM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 12/09/20 7:45 am, Wolfram Sang wrote:

> I'm happy to route it to stable@ if you think it's worth it but I don't
> think there's a specific Fixes: reference that can be used. The current
> behavior appears to have been that way since before git (looks like we
> noticed in 2014 but it's taken me 6 years to nag people into sending
> their fixes upstream).

JFYI: there is a history.git repository from History Group on
kernel.org. You may dig till the very beginning of the kernel (yes,
it's not properly formed Git history, but it will give you a hash
commit as a reference.
Andy Shevchenko Sept. 14, 2020, 11:01 a.m. UTC | #5 
On Mon, Sep 14, 2020 at 1:42 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> On Mon, 14 Sep 2020 11:51:04 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Sep 14, 2020 at 11:50 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Sep 14, 2020 at 12:27 AM Chris Packham
> > > <Chris.Packham@alliedtelesis.co.nz> wrote:
> > > > On 12/09/20 7:45 am, Wolfram Sang wrote:
> > >
> > > > I'm happy to route it to stable@ if you think it's worth it but I don't
> > > > think there's a specific Fixes: reference that can be used. The current
> > > > behavior appears to have been that way since before git (looks like we
> > > > noticed in 2014 but it's taken me 6 years to nag people into sending
> > > > their fixes upstream).
> > >
> > > JFYI: there is a history.git repository from History Group on
> > > kernel.org. You may dig till the very beginning of the kernel (yes,
> > > it's not properly formed Git history, but it will give you a hash
> > > commit as a reference.
> >
> > Stephen, btw, does your scripts that validate Fixes, take into
> > consideration references to history.git?
>
> I assuming you are referring to
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

No.
I'm referring to
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/

> I have found a few by hand in the past (I also missed a few), but I
> guess I could expend the checks.
>
> Maybe that tree could be put somewhere that appears more permanent if
> we are going to permanently refer to it? (Or has that happened already?)

See above. I assume that History Group is something bigger than just Thomas.
Chris Packham Sept. 14, 2020, 8:50 p.m. UTC | #6 
On 14/09/20 6:49 pm, Wolfram Sang wrote:
>> I'm happy to route it to stable@ if you think it's worth it but I don't
>> think there's a specific Fixes: reference that can be used. The current
>> behavior appears to have been that way since before git (looks like we
>> noticed in 2014 but it's taken me 6 years to nag people into sending
>> their fixes upstream).
> Better late than never :) Thanks for sending and the heads up. If you
> don't need it for stable, then we should maybe not send it. It would be
> nice for 9665 but we haven't tested it on 9564. But I'll let it be your
> call.
>
Looks like it's been picked up already. If anyone does hit problems on 
the 9564 hopefully the interweb will lead them to this mailing list 
thread and they'll have some people to hassle.
diff mbox series

Patch

diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 710fbef9a9c2..384af88e58ad 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -41,8 +41,22 @@  static void pca_reset(struct i2c_algo_pca_data *adap)
 		pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
 		pca_outw(adap, I2C_PCA_IND, 0xA5);
 		pca_outw(adap, I2C_PCA_IND, 0x5A);
+
+		/*
+		 * After a reset we need to re-apply any configuration
+		 * (calculated in pca_init) to get the bus in a working state.
+		 */
+		pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IMODE);
+		pca_outw(adap, I2C_PCA_IND, adap->bus_settings.mode);
+		pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLL);
+		pca_outw(adap, I2C_PCA_IND, adap->bus_settings.tlow);
+		pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_ISCLH);
+		pca_outw(adap, I2C_PCA_IND, adap->bus_settings.thi);
+
+		pca_set_con(adap, I2C_PCA_CON_ENSIO);
 	} else {
 		adap->reset_chip(adap->data);
+		pca_set_con(adap, I2C_PCA_CON_ENSIO | adap->bus_settings.clock_freq);
 	}
 }
 
@@ -423,13 +437,14 @@  static int pca_init(struct i2c_adapter *adap)
 				" Use the nominal frequency.\n", adap->name);
 		}
 
-		pca_reset(pca_data);
-
 		clock = pca_clock(pca_data);
 		printk(KERN_INFO "%s: Clock frequency is %dkHz\n",
 		     adap->name, freqs[clock]);
 
-		pca_set_con(pca_data, I2C_PCA_CON_ENSIO | clock);
+		/* Store settings as these will be needed when the PCA chip is reset */
+		pca_data->bus_settings.clock_freq = clock;
+
+		pca_reset(pca_data);
 	} else {
 		int clock;
 		int mode;
@@ -496,19 +511,15 @@  static int pca_init(struct i2c_adapter *adap)
 			thi = tlow * min_thi / min_tlow;
 		}
 
+		/* Store settings as these will be needed when the PCA chip is reset */
+		pca_data->bus_settings.mode = mode;
+		pca_data->bus_settings.tlow = tlow;
+		pca_data->bus_settings.thi = thi;
+
 		pca_reset(pca_data);
 
 		printk(KERN_INFO
 		     "%s: Clock frequency is %dHz\n", adap->name, clock * 100);
-
-		pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_IMODE);
-		pca_outw(pca_data, I2C_PCA_IND, mode);
-		pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLL);
-		pca_outw(pca_data, I2C_PCA_IND, tlow);
-		pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_ISCLH);
-		pca_outw(pca_data, I2C_PCA_IND, thi);
-
-		pca_set_con(pca_data, I2C_PCA_CON_ENSIO);
 	}
 	udelay(500); /* 500 us for oscillator to stabilise */
 
diff --git a/include/linux/i2c-algo-pca.h b/include/linux/i2c-algo-pca.h
index d03071732db4..7c522fdd9ea7 100644
--- a/include/linux/i2c-algo-pca.h
+++ b/include/linux/i2c-algo-pca.h
@@ -53,6 +53,20 @@ 
 #define I2C_PCA_CON_SI		0x08 /* Serial Interrupt */
 #define I2C_PCA_CON_CR		0x07 /* Clock Rate (MASK) */
 
+/**
+ * struct pca_i2c_bus_settings - The configured PCA i2c bus settings
+ * @mode: Configured i2c bus mode
+ * @tlow: Configured SCL LOW period
+ * @thi: Configured SCL HIGH period
+ * @clock_freq: The configured clock frequency
+ */
+struct pca_i2c_bus_settings {
+	int mode;
+	int tlow;
+	int thi;
+	int clock_freq;
+};
+
 struct i2c_algo_pca_data {
 	void 				*data;	/* private low level data */
 	void (*write_byte)		(void *data, int reg, int val);
@@ -64,6 +78,7 @@  struct i2c_algo_pca_data {
 	 * For PCA9665, use the frequency you want here. */
 	unsigned int			i2c_clock;
 	unsigned int			chip;
+	struct pca_i2c_bus_settings		bus_settings;
 };
 
 int i2c_pca_add_bus(struct i2c_adapter *);