* [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset
@ 2020-09-01 0:57 Evan Nimmo
2020-09-01 8:20 ` Andy Shevchenko
2020-09-01 20:58 ` Chris Packham
0 siblings, 2 replies; 5+ messages in thread
From: Evan Nimmo @ 2020-09-01 0:57 UTC (permalink / raw)
To: wsa, andriy.shevchenko, jdelvare, jarkko.nikula, chris.packham
Cc: linux-i2c, linux-kernel, Evan Nimmo
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>
---
drivers/i2c/algos/i2c-algo-pca.c | 36 +++++++++++++++++++++-----------
include/linux/i2c-algo-pca.h | 15 +++++++++++++
2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 710fbef9a9c2..2e4e27073f40 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);
+
+ /* We need to apply any configuration settings that
+ * were calculated in the pca_init function. The reset
+ * results in these changes being set back to defaults.
+ */
+ 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,15 @@ static int pca_init(struct i2c_adapter *adap)
" Use the nominal frequency.\n", adap->name);
}
+ clock = pca_clock(pca_data);
+
+ /* Store settings as these will be needed when the pca chip is reset */
+ pca_data->bus_settings.clock_freq = clock;
+
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);
} else {
int clock;
int mode;
@@ -496,19 +512,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..ebeadb80c797 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 i2c_bus_settings - The configured i2c bus settings
+ * @mode: Configured i2c bus mode (PCA9665)
+ * @tlow: Configured SCL LOW period (PCA9665)
+ * @thi: Configured SCL HIGH period (PCA9665)
+ * @clock_freq: The configured clock frequency (PCA9564)
+ */
+struct 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 i2c_bus_settings bus_settings;
};
int i2c_pca_add_bus(struct i2c_adapter *);
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset
2020-09-01 0:57 [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset Evan Nimmo
@ 2020-09-01 8:20 ` Andy Shevchenko
2020-09-01 8:46 ` Wolfram Sang
2020-09-01 20:58 ` Chris Packham
1 sibling, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2020-09-01 8:20 UTC (permalink / raw)
To: Evan Nimmo
Cc: wsa, jdelvare, jarkko.nikula, chris.packham, linux-i2c, linux-kernel
On Tue, Sep 01, 2020 at 12:57:13PM +1200, Evan Nimmo wrote:
Thanks for the patch, my nit-picks, comments below.
> 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
pca -> PCA (here and where it applicable below).
> 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.
...
> + /* We need to apply any configuration settings that
> + * were calculated in the pca_init function. The reset
> + * results in these changes being set back to defaults.
> + */
/*
* Multi-line comments usually go
* like this.
*/
...
> + clock = pca_clock(pca_data);
> +
> + /* Store settings as these will be needed when the pca chip is reset */
> + pca_data->bus_settings.clock_freq = clock;
> +
> pca_reset(pca_data);
>
> - clock = pca_clock(pca_data);
> printk(KERN_INFO "%s: Clock frequency is %dkHz\n",
> adap->name, freqs[clock]);
Seems this message is attached to above call. Can we move it as well?
...
> +/**
> + * struct i2c_bus_settings - The configured i2c bus settings
> + * @mode: Configured i2c bus mode (PCA9665)
> + * @tlow: Configured SCL LOW period (PCA9665)
> + * @thi: Configured SCL HIGH period (PCA9665)
> + * @clock_freq: The configured clock frequency (PCA9564)
> + */
> +struct i2c_bus_settings {
> + int mode;
> + int tlow;
> + int thi;
> + int clock_freq;
> +};
This is a good candidate to have it in generic header for sake of the
unification. Possible users:
struct tegra_i2c_hw_feature
struct cci_data::struct hw_params
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset
2020-09-01 8:20 ` Andy Shevchenko
@ 2020-09-01 8:46 ` Wolfram Sang
2020-09-01 8:50 ` Andy Shevchenko
0 siblings, 1 reply; 5+ messages in thread
From: Wolfram Sang @ 2020-09-01 8:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Evan Nimmo, jdelvare, jarkko.nikula, chris.packham, linux-i2c,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 749 bytes --]
> > +/**
> > + * struct i2c_bus_settings - The configured i2c bus settings
> > + * @mode: Configured i2c bus mode (PCA9665)
> > + * @tlow: Configured SCL LOW period (PCA9665)
> > + * @thi: Configured SCL HIGH period (PCA9665)
> > + * @clock_freq: The configured clock frequency (PCA9564)
> > + */
> > +struct i2c_bus_settings {
> > + int mode;
> > + int tlow;
> > + int thi;
> > + int clock_freq;
> > +};
>
> This is a good candidate to have it in generic header for sake of the
> unification. Possible users:
>
> struct tegra_i2c_hw_feature
> struct cci_data::struct hw_params
Could be evaluated, but outside the scope of this patch, I think. This
is a bugfix, so a self-contained patch makes backporting a lot easier.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset
2020-09-01 8:46 ` Wolfram Sang
@ 2020-09-01 8:50 ` Andy Shevchenko
0 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-09-01 8:50 UTC (permalink / raw)
To: Wolfram Sang
Cc: Andy Shevchenko, Evan Nimmo, Jean Delvare, Jarkko Nikula,
Chris Packham, linux-i2c, Linux Kernel Mailing List
On Tue, Sep 1, 2020 at 11:48 AM Wolfram Sang <wsa@kernel.org> wrote:
> > > +/**
> > > + * struct i2c_bus_settings - The configured i2c bus settings
> > > + * @mode: Configured i2c bus mode (PCA9665)
> > > + * @tlow: Configured SCL LOW period (PCA9665)
> > > + * @thi: Configured SCL HIGH period (PCA9665)
> > > + * @clock_freq: The configured clock frequency (PCA9564)
> > > + */
> > > +struct i2c_bus_settings {
> > > + int mode;
> > > + int tlow;
> > > + int thi;
> > > + int clock_freq;
> > > +};
> >
> > This is a good candidate to have it in generic header for sake of the
> > unification. Possible users:
> >
> > struct tegra_i2c_hw_feature
> > struct cci_data::struct hw_params
>
> Could be evaluated, but outside the scope of this patch, I think. This
> is a bugfix, so a self-contained patch makes backporting a lot easier.
True, my point here is to think if we were beneficial of this
definition in the i2c.h.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset
2020-09-01 0:57 [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset Evan Nimmo
2020-09-01 8:20 ` Andy Shevchenko
@ 2020-09-01 20:58 ` Chris Packham
1 sibling, 0 replies; 5+ messages in thread
From: Chris Packham @ 2020-09-01 20:58 UTC (permalink / raw)
To: Evan Nimmo, wsa, andriy.shevchenko, jdelvare, jarkko.nikula
Cc: linux-i2c, linux-kernel
Hi Evan,
One minor comment from me below. With that
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
On 1/09/20 12:57 pm, 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>
> ---
> drivers/i2c/algos/i2c-algo-pca.c | 36 +++++++++++++++++++++-----------
> include/linux/i2c-algo-pca.h | 15 +++++++++++++
> 2 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> index 710fbef9a9c2..2e4e27073f40 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);
> +
> + /* We need to apply any configuration settings that
> + * were calculated in the pca_init function. The reset
> + * results in these changes being set back to defaults.
> + */
"these changes" doesn't read well. How about
/*
* 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,15 @@ static int pca_init(struct i2c_adapter *adap)
> " Use the nominal frequency.\n", adap->name);
> }
>
> + clock = pca_clock(pca_data);
> +
> + /* Store settings as these will be needed when the pca chip is reset */
> + pca_data->bus_settings.clock_freq = clock;
> +
> 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);
> } else {
> int clock;
> int mode;
> @@ -496,19 +512,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..ebeadb80c797 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 i2c_bus_settings - The configured i2c bus settings
> + * @mode: Configured i2c bus mode (PCA9665)
> + * @tlow: Configured SCL LOW period (PCA9665)
> + * @thi: Configured SCL HIGH period (PCA9665)
> + * @clock_freq: The configured clock frequency (PCA9564)
> + */
> +struct 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 i2c_bus_settings bus_settings;
> };
>
> int i2c_pca_add_bus(struct i2c_adapter *);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-01 20:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 0:57 [PATCH] i2c: algo-pca: Reapply i2c bus settings after reset Evan Nimmo
2020-09-01 8:20 ` Andy Shevchenko
2020-09-01 8:46 ` Wolfram Sang
2020-09-01 8:50 ` Andy Shevchenko
2020-09-01 20:58 ` Chris Packham
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).