* [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
@ 2012-09-13 3:39 Guenter Roeck
[not found] ` <1347507591-32352-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2012-09-13 3:39 UTC (permalink / raw)
To: Jean Delvare, Ben Dooks
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Thomas Kavanagh, Wolfram Sang,
Guenter Roeck
From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
The parameter passed to pca9665_reset is adap->data, not adap. Unless
adap->data happens to point back to adap, this can result in a kernel panic.
Fix re-factoring pca_reset() from a macro to a function to handle chip
specific code, and only call adap->reset_chip() if the chip is not PCA9665.
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
---
drivers/i2c/algos/i2c-algo-pca.c | 27 ++++++++++++++-------------
include/linux/i2c-algo-pca.h | 1 +
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 73133b1..07ccbef 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -46,14 +46,19 @@ static int i2c_debug;
#define pca_set_con(adap, val) pca_outw(adap, I2C_PCA_CON, val)
#define pca_get_con(adap) pca_inw(adap, I2C_PCA_CON)
#define pca_wait(adap) adap->wait_for_completion(adap->data)
-#define pca_reset(adap) adap->reset_chip(adap->data)
-static void pca9665_reset(void *pd)
+static void pca_reset(struct i2c_algo_pca_data *adap)
{
- struct i2c_algo_pca_data *adap = pd;
- pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
- pca_outw(adap, I2C_PCA_IND, 0xA5);
- pca_outw(adap, I2C_PCA_IND, 0x5A);
+ if (adap->chip == I2C_PCA_CHIP_9665) {
+ /* Ignore the reset function from the module,
+ * we can use the parallel bus reset.
+ */
+ pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
+ pca_outw(adap, I2C_PCA_IND, 0xA5);
+ pca_outw(adap, I2C_PCA_IND, 0x5A);
+ } else {
+ adap->reset_chip(adap->data);
+ }
}
/*
@@ -378,11 +383,12 @@ static unsigned int pca_probe_chip(struct i2c_adapter *adap)
pca_outw(pca_data, I2C_PCA_INDPTR, I2C_PCA_IADR);
if (pca_inw(pca_data, I2C_PCA_IND) == 0xAA) {
printk(KERN_INFO "%s: PCA9665 detected.\n", adap->name);
- return I2C_PCA_CHIP_9665;
+ pca_data->chip = I2C_PCA_CHIP_9665;
} else {
printk(KERN_INFO "%s: PCA9564 detected.\n", adap->name);
- return I2C_PCA_CHIP_9564;
+ pca_data->chip = I2C_PCA_CHIP_9564;
}
+ return pca_data->chip;
}
static int pca_init(struct i2c_adapter *adap)
@@ -456,11 +462,6 @@ static int pca_init(struct i2c_adapter *adap)
*/
int raise_fall_time;
- /* Ignore the reset function from the module,
- * we can use the parallel bus reset
- */
- pca_data->reset_chip = pca9665_reset;
-
if (pca_data->i2c_clock > 1265800) {
printk(KERN_WARNING "%s: I2C clock speed too high."
" Using 1265.8kHz.\n", adap->name);
diff --git a/include/linux/i2c-algo-pca.h b/include/linux/i2c-algo-pca.h
index 1364d62..a3c3ecd 100644
--- a/include/linux/i2c-algo-pca.h
+++ b/include/linux/i2c-algo-pca.h
@@ -62,6 +62,7 @@ struct i2c_algo_pca_data {
* 330000, 288000, 217000, 146000, 88000, 59000, 44000, 36000
* For PCA9665, use the frequency you want here. */
unsigned int i2c_clock;
+ unsigned int chip;
};
int i2c_pca_add_bus(struct i2c_adapter *);
--
1.7.9.7
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665
[not found] ` <1347507591-32352-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2012-09-13 3:39 ` Guenter Roeck
[not found] ` <1347507591-32352-2-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 10:14 ` [PATCH 1/2] i2c: (algo-pca) Fix chip reset function " Wolfram Sang
2012-09-14 12:51 ` Wolfram Sang
2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2012-09-13 3:39 UTC (permalink / raw)
To: Jean Delvare, Ben Dooks
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Thomas Kavanagh, Wolfram Sang,
Guenter Roeck
From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
As implemented, the code always selects turbo mode for PCA9665, no matter which
clock frequency is configured. This is because it compares clock boundaries
against constants reflecting (boundary / 100), but uses the actual clock frequency
to compare against. To fix the problem, use a variable reflecting (clock / 100),
not the actual clock frequency, when comparing against (boundary / 100).
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
---
drivers/i2c/algos/i2c-algo-pca.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
index 07ccbef..572251b 100644
--- a/drivers/i2c/algos/i2c-algo-pca.c
+++ b/drivers/i2c/algos/i2c-algo-pca.c
@@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
/* To avoid integer overflow, use clock/100 for calculations */
clock = pca_clock(pca_data) / 100;
- if (pca_data->i2c_clock > 10000) {
+ if (clock > 10000) {
mode = I2C_PCA_MODE_TURBO;
min_tlow = 14;
min_thi = 5;
raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
- } else if (pca_data->i2c_clock > 4000) {
+ } else if (clock > 4000) {
mode = I2C_PCA_MODE_FASTP;
min_tlow = 17;
min_thi = 9;
raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
- } else if (pca_data->i2c_clock > 1000) {
+ } else if (clock > 1000) {
mode = I2C_PCA_MODE_FAST;
min_tlow = 44;
min_thi = 20;
--
1.7.9.7
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665
[not found] ` <1347507591-32352-2-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2012-09-13 10:05 ` Wolfram Sang
[not found] ` <20120913100530.GD14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2012-09-13 10:05 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Thomas Kavanagh, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]
On Wed, Sep 12, 2012 at 08:39:51PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>
> As implemented, the code always selects turbo mode for PCA9665, no matter which
> clock frequency is configured. This is because it compares clock boundaries
> against constants reflecting (boundary / 100), but uses the actual clock frequency
> to compare against. To fix the problem, use a variable reflecting (clock / 100),
> not the actual clock frequency, when comparing against (boundary / 100).
Good point, however...
>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> ---
> drivers/i2c/algos/i2c-algo-pca.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> index 07ccbef..572251b 100644
> --- a/drivers/i2c/algos/i2c-algo-pca.c
> +++ b/drivers/i2c/algos/i2c-algo-pca.c
> @@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
> /* To avoid integer overflow, use clock/100 for calculations */
> clock = pca_clock(pca_data) / 100;
>
> - if (pca_data->i2c_clock > 10000) {
> + if (clock > 10000) {
... we should make the code more readable if we are at it.
I'd suggest to drop the "/ 100" from above and move it below this
if-block. This will allow for much more readable comparisons, e.g.
"clock > 1000000" etc which is really the clock in Hz.
> mode = I2C_PCA_MODE_TURBO;
> min_tlow = 14;
> min_thi = 5;
> raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> - } else if (pca_data->i2c_clock > 4000) {
> + } else if (clock > 4000) {
> mode = I2C_PCA_MODE_FASTP;
> min_tlow = 17;
> min_thi = 9;
> raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> - } else if (pca_data->i2c_clock > 1000) {
> + } else if (clock > 1000) {
> mode = I2C_PCA_MODE_FAST;
> min_tlow = 44;
> min_thi = 20;
> --
> 1.7.9.7
>
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <1347507591-32352-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 3:39 ` [PATCH 2/2] i2c: (algo-pca) Fix mode selection " Guenter Roeck
@ 2012-09-13 10:14 ` Wolfram Sang
[not found] ` <20120913101423.GE14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-09-14 12:51 ` Wolfram Sang
2 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2012-09-13 10:14 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Thomas Kavanagh, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 540 bytes --]
On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.
Like every write and read to a register which uses the same assumption
AFAICS?
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <20120913101423.GE14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2012-09-13 13:41 ` Guenter Roeck
[not found] ` <20120913134128.GA1343-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2012-09-13 13:41 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Thomas Kavanagh, Guenter Roeck
On Thu, Sep 13, 2012 at 12:14:23PM +0200, Wolfram Sang wrote:
> On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> > From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> >
> > The parameter passed to pca9665_reset is adap->data, not adap. Unless
> > adap->data happens to point back to adap, this can result in a kernel panic.
>
> Like every write and read to a register which uses the same assumption
> AFAICS?
>
You lost me there. Other reset functions are aware of and use the passed parameter
(adap->data). pca9665_reset overwrites the original reset function with
pca_data->reset_chip = pca9665_reset;
but not ->data (which it can't overwrite since it is used by the read_byte and
write_byte functions).
static void pca9665_reset(void *pd)
struct i2c_algo_pca_data *adap = pd;
static void i2c_pca_pf_resetchip(void *pd)
struct i2c_pca_pf_data *i2c = pd;
static int i2c_pca_pf_readbyte32(void *pd, int reg)
struct i2c_pca_pf_data *i2c = pd;
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] i2c: (algo-pca) Fix mode selection for PCA9665
[not found] ` <20120913100530.GD14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2012-09-13 13:44 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2012-09-13 13:44 UTC (permalink / raw)
To: Wolfram Sang
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Thomas Kavanagh, Guenter Roeck
On Thu, Sep 13, 2012 at 12:05:31PM +0200, Wolfram Sang wrote:
> On Wed, Sep 12, 2012 at 08:39:51PM -0700, Guenter Roeck wrote:
> > From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> >
> > As implemented, the code always selects turbo mode for PCA9665, no matter which
> > clock frequency is configured. This is because it compares clock boundaries
> > against constants reflecting (boundary / 100), but uses the actual clock frequency
> > to compare against. To fix the problem, use a variable reflecting (clock / 100),
> > not the actual clock frequency, when comparing against (boundary / 100).
>
> Good point, however...
>
> >
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> > ---
> > drivers/i2c/algos/i2c-algo-pca.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/algos/i2c-algo-pca.c b/drivers/i2c/algos/i2c-algo-pca.c
> > index 07ccbef..572251b 100644
> > --- a/drivers/i2c/algos/i2c-algo-pca.c
> > +++ b/drivers/i2c/algos/i2c-algo-pca.c
> > @@ -477,17 +477,17 @@ static int pca_init(struct i2c_adapter *adap)
> > /* To avoid integer overflow, use clock/100 for calculations */
> > clock = pca_clock(pca_data) / 100;
> >
> > - if (pca_data->i2c_clock > 10000) {
> > + if (clock > 10000) {
>
> ... we should make the code more readable if we are at it.
> I'd suggest to drop the "/ 100" from above and move it below this
> if-block. This will allow for much more readable comparisons, e.g.
> "clock > 1000000" etc which is really the clock in Hz.
>
We can simply compare against the real frequency:
if (pca_data->i2c_clock > 1000000) {
which also makes the description a bit easier to understand.
> > mode = I2C_PCA_MODE_TURBO;
> > min_tlow = 14;
> > min_thi = 5;
> > raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> > - } else if (pca_data->i2c_clock > 4000) {
> > + } else if (clock > 4000) {
> > mode = I2C_PCA_MODE_FASTP;
> > min_tlow = 17;
> > min_thi = 9;
> > raise_fall_time = 22; /* Raise 11e-8s, Fall 11e-8s */
> > - } else if (pca_data->i2c_clock > 1000) {
> > + } else if (clock > 1000) {
> > mode = I2C_PCA_MODE_FAST;
> > min_tlow = 44;
> > min_thi = 20;
> > --
> > 1.7.9.7
> >
>
> --
> Pengutronix e.K. | Wolfram Sang |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <20120913134128.GA1343-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
@ 2012-09-13 23:14 ` Thomas Kavanagh
[not found] ` <CC77B638.1126D%tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Kavanagh @ 2012-09-13 23:14 UTC (permalink / raw)
To: Guenter Roeck, Wolfram Sang
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Guenter Roeck
Wolfram,
Hopefully the information below explains the problem with the current pca9665_reset() function.
Regards,
Tom
Let's say mod1 has the following
typedef struct mod1_pca_data_ {
int number;
unsigned int bus;
char name[50];
i2c_algo_pca_data *adap;
} mod1_pca_data_t;
static struct i2c_algo_pca_data mod1_pca_algo_data = {
.write_byte = mod1_writebyte,
.read_byte = mod1_readbyte,
.wait_for_completion = mod1_pca_waitforcompletion,
.reset_chip = mod1_pca_resetchip,
.i2c_clock = 400000,
};
/*
*
*/
static struct i2c_adapter mod1_pca_adapter = {
.owner = THIS_MODULE,
.class = I2C_CLASS_HWMON,
.algo_data = &mod1_pca_algo_data,
.name = "Mod1 PCA Bus Adapter",
.timeout = HZ,
};
void mod1_pca_attach(struct device *dev, mod1_data_t *mod1_data)
{
mod1_pca_algo_data.data = mod1_data;
mod1_data->adap = &mod1_pca_adapter;
mod1_data->adap->dev.parent = dev;
i2c_pca_add_bus(mod1_data->adap);
}
Within the i2c-algo-pca module, all calls to pca_outw(adap) and pca_inw(adap) will be done with an input parameter of type i2c_algo_pca_data. These will intern call mod1's read_byte and write byte functions and pass in adap->data as the first parameter. This data pointer is of type mod1_pca_data_t * in this example.
Now with the pca9665_reset() function, the i2c-algo-pca module sets the mod1's reset_chip function pointer to pca9665_reset. When pca_reset(adap) is called, the input parameter is of type i2c_algo_pca_data as expected. Then pca_reset() calls adap->reset_chip (pca9665_reset) with the first parameter being adap->data. This parameter is of type mod1_pca_data_t * in this example. The problem comes from the fact that pca9665_reset(void *pd) is expecting the parameter to be of type i2c_algp_pca_data:
static void pca9665_reset(void *pd)
{
struct i2c_algo_pca_data *adap = pd;
pca_outw(adap, I2C_PCA_INDPTR, I2C_PCA_IPRESET);
pca_outw(adap, I2C_PCA_IND, 0xA5);
pca_outw(adap, I2C_PCA_IND, 0x5A);
}
All calls to pca_outw(adap) in this function are passing in a pointer to mod1_pca_data_t and not i2c_algo_pca_data which in turn causes the kernel to panic when the pca_outw(adap) macro tries to access adap->write_byte().
On 9/13/12 6:41 AM, "Guenter Roeck" <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org<mailto:linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>> wrote:
On Thu, Sep 13, 2012 at 12:14:23PM +0200, Wolfram Sang wrote:
On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org<mailto:tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>>
>
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.
Like every write and read to a register which uses the same assumption
AFAICS?
You lost me there. Other reset functions are aware of and use the passed parameter
(adap->data). pca9665_reset overwrites the original reset function with
pca_data->reset_chip = pca9665_reset;
but not ->data (which it can't overwrite since it is used by the read_byte and
write_byte functions).
static void pca9665_reset(void *pd)
struct i2c_algo_pca_data *adap = pd;
static void i2c_pca_pf_resetchip(void *pd)
struct i2c_pca_pf_data *i2c = pd;
static int i2c_pca_pf_readbyte32(void *pd, int reg)
struct i2c_pca_pf_data *i2c = pd;
Guenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <CC77B638.1126D%tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
@ 2012-09-14 12:12 ` Wolfram Sang
[not found] ` <20120914121244.GE2630-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Wolfram Sang @ 2012-09-14 12:12 UTC (permalink / raw)
To: Thomas Kavanagh
Cc: Guenter Roeck, Jean Delvare, Ben Dooks,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 1304 bytes --]
Thomas,
> Hopefully the information below explains the problem with the current pca9665_reset() function.
Yes, it does :)
> Let's say mod1 has the following
This was the first needed information. You seem to not use
i2c-pca-platform but a custom driver using the pca-algo, right? May I
ask why the custom one is needed?
> Now with the pca9665_reset() function, the i2c-algo-pca module sets
> the mod1's reset_chip function pointer to pca9665_reset. When
> pca_reset(adap) is called, the input parameter is of type
> i2c_algo_pca_data as expected. Then pca_reset() calls
> adap->reset_chip (pca9665_reset) with the first parameter being
> adap->data. This parameter is of type mod1_pca_data_t * in this
> example. The problem comes from the fact that pca9665_reset(void *pd)
> is expecting the parameter to be of type i2c_algp_pca_data:
To put in other words: All other wrappers from the algo call back to the
bus driver which knows to handle its custom data. Only the pca9665 reset
is staying inside the algo and facing a custom data structure it
doesn't know. Okay, understood now.
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <1347507591-32352-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 3:39 ` [PATCH 2/2] i2c: (algo-pca) Fix mode selection " Guenter Roeck
2012-09-13 10:14 ` [PATCH 1/2] i2c: (algo-pca) Fix chip reset function " Wolfram Sang
@ 2012-09-14 12:51 ` Wolfram Sang
2 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2012-09-14 12:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Ben Dooks, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
Thomas Kavanagh, Guenter Roeck
[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]
On Wed, Sep 12, 2012 at 08:39:50PM -0700, Guenter Roeck wrote:
> From: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
>
> The parameter passed to pca9665_reset is adap->data, not adap. Unless
> adap->data happens to point back to adap, this can result in a kernel panic.
Can you please update this paragraph with a short explanation of what we
discussed (i.e. why exactly the kernel can crash)?
>
> Fix re-factoring pca_reset() from a macro to a function to handle chip
> specific code, and only call adap->reset_chip() if the chip is not PCA9665.
>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Thomas Kavanagh <tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
> Signed-off-by: Guenter Roeck <groeck-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
Patch itself is good.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665
[not found] ` <20120914121244.GE2630-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2012-09-14 18:23 ` Thomas Kavanagh
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Kavanagh @ 2012-09-14 18:23 UTC (permalink / raw)
To: Wolfram Sang
Cc: Guenter Roeck, Jean Delvare, Ben Dooks,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Guenter Roeck
On 9/14/12 5:12 AM, "Wolfram Sang" <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>Thomas,
>
>> Hopefully the information below explains the problem with the current
>>pca9665_reset() function.
>
>Yes, it does :)
>
>> Let's say mod1 has the following
>
>This was the first needed information. You seem to not use
>i2c-pca-platform but a custom driver using the pca-algo, right? May I
>ask why the custom one is needed?
A custom one is required as we have another device that we must go through
in order to access the PCA9665 controller.
>
>> Now with the pca9665_reset() function, the i2c-algo-pca module sets
>> the mod1's reset_chip function pointer to pca9665_reset. When
>> pca_reset(adap) is called, the input parameter is of type
>> i2c_algo_pca_data as expected. Then pca_reset() calls
>> adap->reset_chip (pca9665_reset) with the first parameter being
>> adap->data. This parameter is of type mod1_pca_data_t * in this
>> example. The problem comes from the fact that pca9665_reset(void *pd)
>> is expecting the parameter to be of type i2c_algp_pca_data:
>
>To put in other words: All other wrappers from the algo call back to the
>bus driver which knows to handle its custom data. Only the pca9665 reset
>is staying inside the algo and facing a custom data structure it
>doesn't know. Okay, understood now.
>
>Thanks,
>
> Wolfram
>
>--
>Pengutronix e.K. | Wolfram Sang |
>Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-09-14 18:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-13 3:39 [PATCH 1/2] i2c: (algo-pca) Fix chip reset function for PCA9665 Guenter Roeck
[not found] ` <1347507591-32352-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 3:39 ` [PATCH 2/2] i2c: (algo-pca) Fix mode selection " Guenter Roeck
[not found] ` <1347507591-32352-2-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 10:05 ` Wolfram Sang
[not found] ` <20120913100530.GD14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-09-13 13:44 ` Guenter Roeck
2012-09-13 10:14 ` [PATCH 1/2] i2c: (algo-pca) Fix chip reset function " Wolfram Sang
[not found] ` <20120913101423.GE14237-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-09-13 13:41 ` Guenter Roeck
[not found] ` <20120913134128.GA1343-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
2012-09-13 23:14 ` Thomas Kavanagh
[not found] ` <CC77B638.1126D%tkavanagh-3r7Miqu9kMnR7s880joybQ@public.gmane.org>
2012-09-14 12:12 ` Wolfram Sang
[not found] ` <20120914121244.GE2630-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-09-14 18:23 ` Thomas Kavanagh
2012-09-14 12:51 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.