Re: [PATCH 2/4] input: Convert struct i2c_msg initialization to C99 format
On Wed, Oct 10, 2012 at 05:05:39PM +0530, Shubhrajyoti Datta wrote:
> On Wed, Oct 10, 2012 at 2:32 PM, Jean Delvare wrote:
> > On Tue, 9 Oct 2012 17:01:18 +0530, Shubhrajyoti D wrote:
> [...]
> >
> > Acked-by: Jean Delvare
>
>
> thanks updated patch below
> From 6638ecfa7982f95815382922c50573712c9626d7 Mon Sep 17 00:00:00 2001
> From: Shubhrajyoti D
> Date: Mon, 17 Sep 2012 19:37:17 +0530
> Subject: [PATCHv2 1/2] input: Convert struct i2c_msg initialization to C99
> format
>
> Convert the struct i2c_msg initialization to C99 format. This makes
> maintaining and editing the code simpler. Also helps once other fields
> like transferred are added in future.
>
> Thanks to Julia Lawall for automating the conversion
>
> Acked-by: Jean Delvare
> Signed-off-by: Shubhrajyoti D
Applied both, thanks Shubhrajyoti.
> ---
> drivers/input/touchscreen/cy8ctmg110_ts.c | 13 +++--
> 1 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c
> b/drivers/input/touchscreen/cy8ctmg110_ts.c
> index 464f1bf..ad6a664 100644
> --- a/drivers/input/touchscreen/cy8ctmg110_ts.c
> +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c
> @@ -99,9 +99,18 @@ static int cy8ctmg110_read_regs(struct cy8ctmg110 *tsc,
> int ret;
> struct i2c_msg msg[2] = {
> /* first write slave position to i2c devices */
> - { client->addr, 0, 1, &cmd },
> + {
> + .addr = client->addr,
> + .len = 1,
> + .buf = &cmd
> + },
> /* Second read data from position */
> - { client->addr, I2C_M_RD, len, data }
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = data
> + }
> };
>
> ret = i2c_transfer(client->adapter, msg, 2);
> --
> 1.7.5.4
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman wrote: > From: Kevin Hilman > > Currently, runtime PM is used to keep the device enabled only during > active transfers and for a configurable runtime PM autosuspend timout > after an xfer. > > In addition to idling the device, driver's ->runtime_suspend() method > currently disables device interrupts when idle. However, on some SoCs > (notably OMAP4+), the I2C hardware may shared with other coprocessors. > This means that the MPU will still recieve interrupts if a coprocessor > is using the I2C device. To avoid this, also disable interrupts at > the MPU INTC when idling the device in ->runtime_suspend() (and > re-enable them in ->runtime_resume().) This part based on an original > patch from Shubhrajyoti Datta. NOTE: for proper sharing the I2C with > a coprocessor, this driver still needs hwspinlock support added. > > This change is also meant to address an issue reported by Kalle > Jokiniemi where I2C bus interrupt may be enabled before an I2C device > interrupt handler (e.g. just after noirq resume phase) causing an > interrupt flood on the I2C bus interrupt before the device interrupt > is enabled (e.g. interrupts coming from devices on I2C connected PMIC > before the PMIC chained hanlder is enabled.) This problem is addresed > by ensuring that the I2C bus interrupt left disabled until an I2C xfer > is requested. Looks good to me. Will wait for Kalle though. > > Cc: Kalle Jokiniemi > Cc: Grygorii Strashko > Cc: Shubhrajyoti Datta , > Cc: Huzefa Kankroliwala > Cc: Nishanth Menon > Signed-off-by: Kevin Hilman > --- > drivers/i2c/busses/i2c-omap.c |3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index db31eae..e6413e8 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev) > /* Flush posted write */ > omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); > } > + disable_irq(_dev->irq); > > return 0; > } > @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev) > omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > } > > + enable_irq(_dev->irq); > + > /* > * Don't write to this register if the IE state is 0 as it can > * cause deadlock. > -- > 1.7.9.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
On Saturday 13 October 2012 12:34 AM, Kevin Hilman wrote: > Shubhrajyoti Datta writes: > >> On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman >> wrote: >>> "Strashko, Grygorii" writes: >>> Hi Kevin, yep, [1] is the same fix - thanks. Hi Kalle, i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) Could you check it with your use case, pls? (just to be sure that idea is right) >>> I think the ideas is right, but [1] introduces more potential problems >>> since it disables the IRQ at the INTC well before being disabled at the >>> device. >> I fail to see this point. Current driver supports master mode only. >> So unless there is a msg queued by the core. May be no interrupts should >> fire. >> >> what I mean >> >> msg -> conf -> intr -> completion/error -> autosuspend. >> >>> With runtime PM autosuspend timeouts, that means any IRQs >>> firing during the autosuspend delay will be lost, which basically >>> nullifies the use of autosuspend. >> so I do not expect any interrupts between completion/error -> autosuspend. > Runtime PM is designed for concurrent users (hence the usecounting.) > The I2C driver may not fully support this, since there is a single bus > to share, but the usage of runtime PM currently allows multiple users to > do runtime PM get/put (though in this driver they will block in the > wait_for_bb.) > > So the fundamental problem in doing the enable/disable IRQ in the xfer > function, and not the runtime PM callbacks is that you're ignoring the > runtime PM usecount. You're assuming that the 'get' is *always* > incrementing the usecount from zero to 1, and the 'put' is *always* a > transition from 1 to zero. This may be the case in current usage and > tests, but does not have to be the case. > > Even if that never happens in practice, it can in theory, and to me > leads to confusing code. It simply doesn't make sense in terms of > readability to disable the IRQ at the INTC in xfer, and disable IRQs at > the device level in the runtime PM callback. Agree. > > To put it more simply: anything that needs to be done when the I2C is > about to be idled/enabled should be done in the runtime PM callbacks. > > Please have a look at the patch I just sent[1]. In addition to making > it more readable (IMO), it elminates the need for an extra disable_irq() > in probe. thanks. > > Kevin > > [1] http://marc.info/?l=linux-omap&m=135006723121147&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Shubhrajyoti Datta writes: > On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman > wrote: >> "Strashko, Grygorii" writes: >> >>> Hi Kevin, >>> >>> yep, [1] is the same fix - thanks. >> >>> Hi Kalle, >>> >>> i've applied these changes and PM runtime fix on top of >>> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git >>> (omap2plus_defconfig) >>> Could you check it with your use case, pls? (just to be sure that idea is >>> right) >> >> I think the ideas is right, but [1] introduces more potential problems >> since it disables the IRQ at the INTC well before being disabled at the >> device. > I fail to see this point. Current driver supports master mode only. > So unless there is a msg queued by the core. May be no interrupts should fire. > > what I mean > > msg -> conf -> intr -> completion/error -> autosuspend. > >> With runtime PM autosuspend timeouts, that means any IRQs >> firing during the autosuspend delay will be lost, which basically >> nullifies the use of autosuspend. > > so I do not expect any interrupts between completion/error -> autosuspend. Runtime PM is designed for concurrent users (hence the usecounting.) The I2C driver may not fully support this, since there is a single bus to share, but the usage of runtime PM currently allows multiple users to do runtime PM get/put (though in this driver they will block in the wait_for_bb.) So the fundamental problem in doing the enable/disable IRQ in the xfer function, and not the runtime PM callbacks is that you're ignoring the runtime PM usecount. You're assuming that the 'get' is *always* incrementing the usecount from zero to 1, and the 'put' is *always* a transition from 1 to zero. This may be the case in current usage and tests, but does not have to be the case. Even if that never happens in practice, it can in theory, and to me leads to confusing code. It simply doesn't make sense in terms of readability to disable the IRQ at the INTC in xfer, and disable IRQs at the device level in the runtime PM callback. To put it more simply: anything that needs to be done when the I2C is about to be idled/enabled should be done in the runtime PM callbacks. Please have a look at the patch I just sent[1]. In addition to making it more readable (IMO), it elminates the need for an extra disable_irq() in probe. Kevin [1] http://marc.info/?l=linux-omap&m=135006723121147&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
From: Kevin Hilman Currently, runtime PM is used to keep the device enabled only during active transfers and for a configurable runtime PM autosuspend timout after an xfer. In addition to idling the device, driver's ->runtime_suspend() method currently disables device interrupts when idle. However, on some SoCs (notably OMAP4+), the I2C hardware may shared with other coprocessors. This means that the MPU will still recieve interrupts if a coprocessor is using the I2C device. To avoid this, also disable interrupts at the MPU INTC when idling the device in ->runtime_suspend() (and re-enable them in ->runtime_resume().) This part based on an original patch from Shubhrajyoti Datta. NOTE: for proper sharing the I2C with a coprocessor, this driver still needs hwspinlock support added. This change is also meant to address an issue reported by Kalle Jokiniemi where I2C bus interrupt may be enabled before an I2C device interrupt handler (e.g. just after noirq resume phase) causing an interrupt flood on the I2C bus interrupt before the device interrupt is enabled (e.g. interrupts coming from devices on I2C connected PMIC before the PMIC chained hanlder is enabled.) This problem is addresed by ensuring that the I2C bus interrupt left disabled until an I2C xfer is requested. Cc: Kalle Jokiniemi Cc: Grygorii Strashko Cc: Shubhrajyoti Datta , Cc: Huzefa Kankroliwala Cc: Nishanth Menon Signed-off-by: Kevin Hilman --- drivers/i2c/busses/i2c-omap.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index db31eae..e6413e8 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev) /* Flush posted write */ omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); } + disable_irq(_dev->irq); return 0; } @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev) omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); } + enable_irq(_dev->irq); + /* * Don't write to this register if the IE state is 0 as it can * cause deadlock. -- 1.7.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman wrote: > "Strashko, Grygorii" writes: > >> Hi Kevin, >> >> yep, [1] is the same fix - thanks. > >> Hi Kalle, >> >> i've applied these changes and PM runtime fix on top of >> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git >> (omap2plus_defconfig) >> Could you check it with your use case, pls? (just to be sure that idea is >> right) > > I think the ideas is right, but [1] introduces more potential problems > since it disables the IRQ at the INTC well before being disabled at the > device. I fail to see this point. Current driver supports master mode only. So unless there is a msg queued by the core. May be no interrupts should fire. what I mean msg -> conf -> intr -> completion/error -> autosuspend. > With runtime PM autosuspend timeouts, that means any IRQs > firing during the autosuspend delay will be lost, which basically > nullifies the use of autosuspend. so I do not expect any interrupts between completion/error -> autosuspend. > > Since Shubhrajyoti didn't respond to my runtime PM comments on the > original patch, my apologies for the delay. > I'll respin this patch by moving the INTC disable/enable > into the runtime PM callbacks and make the changelog a bit more clear. > > Grygorii, that pm_runtime_resume() change is needed to. Can you spin a > patch with just that change with a nice descriptive changelog about why > it is needed? Thanks. > > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
On Fri, Oct 12, 2012 at 7:56 PM, Kevin Hilman wrote: > +Kalle, Grygorii, Huzefa > > Shubhrajyoti D writes: > >> Enable the irq in the transfer and disable it after the >> transfer is done. >> >> Signed-off-by: Shubhrajyoti D >> --- >> drivers/i2c/busses/i2c-omap.c |3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index b6c6b95..ce41596 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg >> msgs[], int num) >> if (IS_ERR_VALUE(r)) >> goto out; >> >> + enable_irq(dev->irq); >> r = omap_i2c_wait_for_bb(dev); >> if (r < 0) >> goto out; >> @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg >> msgs[], int num) >> >> omap_i2c_wait_for_bb(dev); >> out: >> + disable_irq(dev->irq); > > When using runtime PM with auto-suspend timeouts, why would you disable > the IRQ before the runtime suspend handlers have run? > > If you really want to do this, you probably should have these in the > runtime PM callbacks. OK. > But I'll wait until you add a more descriptive > changelog before I can really tell why this is being done. Based on the > discussion in the patch from Kalle, I'm assuming this is to prevent > interrups when I2C is being used by co-processors. If so, plese > describe in the changelog. OK. > > That being said, doesn't the runtime suspend callback already disable > IRQs at the device level instead of the INTC level? My idea is that the device may get reconfigured by the other processor. I felt intc is the only way. do you agree? > > Kevin > >> pm_runtime_mark_last_busy(dev->dev); >> pm_runtime_put_autosuspend(dev->dev); >> return r; >> @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev) >> goto err_unuse_clocks; >> } >> >> + disable_irq(dev->irq); >> adap = &dev->adapter; >> i2c_set_adapdata(adap, dev); >> adap->owner = THIS_MODULE; > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: tegra: set irq name as device name
When watching the irqs name of tegra i2c, all instances
irq name shows as tegra_i2c.
Passing the device name properly to have the irq names with
instance like tegra-i2c.0, tegra-i2c.1 etc.
Signed-off-by: Laxman Dewangan
---
drivers/i2c/busses/i2c-tegra.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index f981ac4..dcea77b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -742,7 +742,7 @@ static int __devinit tegra_i2c_probe(struct platform_device
*pdev)
}
ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
- tegra_i2c_isr, 0, pdev->name, i2c_dev);
+ tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
return ret;
--
1.7.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: change the id to let the i2c-gpio work
Hi Bo Shen,
On Fri, Oct 12, 2012 at 11:42 AM, Bo Shen wrote:
> The i2c-gpio driver will turn the platform device ID to busnum.
> When using platfrom device ID as -1, it means dynamically assigned
> the busnum. When writing code, we need to make sure the busnum,
> and call i2c_register_board_info(int busnum, ...) to register device
> if using -1, we do not know the value of busnum.
>
> In order to solve this issue, set the platform device ID as a fix number
> Here using 0 to match the busnum used in i2c_regsiter_board_info().
I have been bitten by this myself on RM9200.
> Signed-off-by: Bo Shen
> ---
> Change since v1
> Make the commit message more clear
> ---
> arch/arm/mach-at91/at91sam9260_devices.c |2 +-
This pattern exist in at91rm9200_devices.c, at91sam9261_devices.c,
at91sam9263_devices.c and at91sam9rl_devices.c you might want to fix
them as well.
I assume we have the same problem if CONFIG_I2C_AT91 is set?
See further down in at91sam9260_devices.c we have another: ".id = -1,"
regards
Joachim Eastwood
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-at91/at91sam9260_devices.c
> b/arch/arm/mach-at91/at91sam9260_devices.c
> index 0f24cfb..805ef95 100644
> --- a/arch/arm/mach-at91/at91sam9260_devices.c
> +++ b/arch/arm/mach-at91/at91sam9260_devices.c
> @@ -389,7 +389,7 @@ static struct i2c_gpio_platform_data pdata = {
>
> static struct platform_device at91sam9260_twi_device = {
> .name = "i2c-gpio",
> - .id = -1,
> + .id = 0,
> .dev.platform_data = &pdata,
> };
>
> --
> 1.7.9.5
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
"Strashko, Grygorii" writes:
> Hi Kevin,
>
> yep, [1] is the same fix - thanks.
> Hi Kalle,
>
> i've applied these changes and PM runtime fix on top of
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
> (omap2plus_defconfig)
> Could you check it with your use case, pls? (just to be sure that idea is
> right)
I think the ideas is right, but [1] introduces more potential problems
since it disables the IRQ at the INTC well before being disabled at the
device. With runtime PM autosuspend timeouts, that means any IRQs
firing during the autosuspend delay will be lost, which basically
nullifies the use of autosuspend.
Since Shubhrajyoti didn't respond to my runtime PM comments on the
original patch, I'll respin this patch by moving the INTC disable/enable
into the runtime PM callbacks and make the changelog a bit more clear.
Grygorii, that pm_runtime_resume() change is needed to. Can you spin a
patch with just that change with a nice descriptive changelog about why
it is needed? Thanks.
Kevin
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a0e49f6..cb09e20 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> msgs[], int num)
> if (IS_ERR_VALUE(r))
> goto out;
>
> + /* We have the bus, enable IRQ */
> + enable_irq(dev->irq);
> +
> r = omap_i2c_wait_for_bb(dev);
> if (r < 0)
> goto out;
> @@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
> msgs[], int num)
> r = num;
>
> omap_i2c_wait_for_bb(dev);
> + disable_irq(dev->irq);
> out:
> pm_runtime_put(dev->dev);
> return r;
> @@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev)
>
> omap_i2c_isr;
> r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev);
>
> + /* We enable IRQ only when request for I2C from master */
> +disable_irq(dev->irq);
> +
> if (r) {
> dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
> goto err_unuse_clocks;
> @@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
> }
> #endif /* CONFIG_PM_RUNTIME */
>
> +static int omap_i2c_suspend(struct device *dev)
> +{
> + int ret;
> +
> + /*
> +* Enable I2C device, so it will be accessible during
> +* later stages of suspending when device Runtime PM is disabled.
> +* I2C device will be turned off at "noirq" suspend stage.
> +*/
> + ret = pm_runtime_resume(dev);
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
> +
> static struct dev_pm_ops omap_i2c_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
> SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
>omap_i2c_runtime_resume, NULL)
> };
>
> - Grygorii
>
> From: Kevin Hilman [khil...@deeprootsystems.com]
> Sent: Friday, October 12, 2012 5:34 PM
> To: Strashko, Grygorii
> Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.s...@pengutronix.de;
> ben-li...@fluff.org; t...@atomide.com; linux-o...@vger.kernel.org; Datta,
> Shubhrajyoti; Kankroliwala, Huzefa
> Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
>
> "Strashko, Grygorii" writes:
>
>> Hi All,
>>
>> Sorry, for the late reply.
>> + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
>
> Hi Grygorii, thanks for reviewing. I was hoping you would have some
> ideas here as this was sounding familiar to something you had
> mentioned elsewhere.
>
>> Regarding this patch - from my point of view, it fixes corner case and not
>> an issue in general.
>> Let take a look on resume sequence:
>>- platform resume
>>- syscore resume
>>- resume_noirq
>>- enable IRQs - resume_device_irqs()
>> |- at this point IRQ handler will be invoked if IRQ state is
>> IRQS_PENDING.
>> |- so, the I2C device IRQ handler may be called at time when I2C
>> adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may
>> fail. (I2C device and I2C adapter may use different physical IRQ lines)
>>- resume_late
>> |- enable I2C bus IRQ
>>
>> Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
>> our case in omap_i2c_xfer().
>> We use such approach in Android kernel 3.4
>> (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
>
> I agree, that should work and cover the cases where I2C is used by other
> processors also. Shubhrajyoti already posted something similar[1] but
> it needed some rework (comments from Russell and myself.)
>
> Huzefa, Shubhrajyoti, who can rework this idea for the ups
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
On Friday 12 October 2012 08:01 PM, Kevin Hilman wrote: > When using runtime PM with auto-suspend timeouts, why would you disable > the IRQ before the runtime suspend handlers have run? > > If you really want to do this, you probably should have these in the > runtime PM callbacks. But I'll wait until you add a more descriptive > changelog before I can really tell why this is being done. Based on the > discussion in the patch from Kalle, I'm assuming this is to prevent > interrups when I2C is being used by co-processors. If so, plese > describe in the changelog. > > That being said, doesn't the runtime suspend callback already disable > IRQs at the device level instead of the INTC level? I thought of not relying on the intc as the registers may be reconfigured. > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi Kevin,
yep, [1] is the same fix - thanks.
Hi Kalle,
i've applied these changes and PM runtime fix on top of
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
(omap2plus_defconfig)
Could you check it with your use case, pls? (just to be sure that idea is right)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..cb09e20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
if (IS_ERR_VALUE(r))
goto out;
+ /* We have the bus, enable IRQ */
+ enable_irq(dev->irq);
+
r = omap_i2c_wait_for_bb(dev);
if (r < 0)
goto out;
@@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
r = num;
omap_i2c_wait_for_bb(dev);
+ disable_irq(dev->irq);
out:
pm_runtime_put(dev->dev);
return r;
@@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev)
omap_i2c_isr;
r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev);
+ /* We enable IRQ only when request for I2C from master */
+disable_irq(dev->irq);
+
if (r) {
dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
goto err_unuse_clocks;
@@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM_RUNTIME */
+static int omap_i2c_suspend(struct device *dev)
+{
+ int ret;
+
+ /*
+* Enable I2C device, so it will be accessible during
+* later stages of suspending when device Runtime PM is disabled.
+* I2C device will be turned off at "noirq" suspend stage.
+*/
+ ret = pm_runtime_resume(dev);
+ if (ret < 0)
+ return ret;
+ return 0;
+}
+
static struct dev_pm_ops omap_i2c_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
- Grygorii
From: Kevin Hilman [khil...@deeprootsystems.com]
Sent: Friday, October 12, 2012 5:34 PM
To: Strashko, Grygorii
Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.s...@pengutronix.de;
ben-li...@fluff.org; t...@atomide.com; linux-o...@vger.kernel.org; Datta,
Shubhrajyoti; Kankroliwala, Huzefa
Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
"Strashko, Grygorii" writes:
> Hi All,
>
> Sorry, for the late reply.
> + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
Hi Grygorii, thanks for reviewing. I was hoping you would have some
ideas here as this was sounding familiar to something you had
mentioned elsewhere.
> Regarding this patch - from my point of view, it fixes corner case and not
> an issue in general.
> Let take a look on resume sequence:
>- platform resume
>- syscore resume
>- resume_noirq
>- enable IRQs - resume_device_irqs()
> |- at this point IRQ handler will be invoked if IRQ state is
> IRQS_PENDING.
> |- so, the I2C device IRQ handler may be called at time when I2C adapter
> IRQ is still disabled and, as result, the I2C device IRQ-handler may fail.
> (I2C device and I2C adapter may use different physical IRQ lines)
>- resume_late
> |- enable I2C bus IRQ
>
> Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
> our case in omap_i2c_xfer().
> We use such approach in Android kernel 3.4
> (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
I agree, that should work and cover the cases where I2C is used by other
processors also. Shubhrajyoti already posted something similar[1] but
it needed some rework (comments from Russell and myself.)
Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or
follow up with the earlier patch[1]?
Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can
come up with a broader solution. Thanks.
Kevin
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
"Strashko, Grygorii" writes: > Hi All, > > Sorry, for the late reply. > + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. Hi Grygorii, thanks for reviewing. I was hoping you would have some ideas here as this was sounding familiar to something you had mentioned elsewhere. > Regarding this patch - from my point of view, it fixes corner case and not > an issue in general. > Let take a look on resume sequence: >- platform resume >- syscore resume >- resume_noirq >- enable IRQs - resume_device_irqs() > |- at this point IRQ handler will be invoked if IRQ state is > IRQS_PENDING. > |- so, the I2C device IRQ handler may be called at time when I2C adapter > IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. > (I2C device and I2C adapter may use different physical IRQ lines) >- resume_late > |- enable I2C bus IRQ > > Possibly, the better way is to enable/disable I2C bus IRQ when needed - in > our case in omap_i2c_xfer(). > We use such approach in Android kernel 3.4 > (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e) I agree, that should work and cover the cases where I2C is used by other processors also. Shubhrajyoti already posted something similar[1] but it needed some rework (comments from Russell and myself.) Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or follow up with the earlier patch[1]? Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can come up with a broader solution. Thanks. Kevin [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
+Kalle, Grygorii, Huzefa Shubhrajyoti D writes: > Enable the irq in the transfer and disable it after the > transfer is done. > > Signed-off-by: Shubhrajyoti D > --- > drivers/i2c/busses/i2c-omap.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index b6c6b95..ce41596 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > if (IS_ERR_VALUE(r)) > goto out; > > + enable_irq(dev->irq); > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > goto out; > @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > > omap_i2c_wait_for_bb(dev); > out: > + disable_irq(dev->irq); When using runtime PM with auto-suspend timeouts, why would you disable the IRQ before the runtime suspend handlers have run? If you really want to do this, you probably should have these in the runtime PM callbacks. But I'll wait until you add a more descriptive changelog before I can really tell why this is being done. Based on the discussion in the patch from Kalle, I'm assuming this is to prevent interrups when I2C is being used by co-processors. If so, plese describe in the changelog. That being said, doesn't the runtime suspend callback already disable IRQs at the device level instead of the INTC level? Kevin > pm_runtime_mark_last_busy(dev->dev); > pm_runtime_put_autosuspend(dev->dev); > return r; > @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev) > goto err_unuse_clocks; > } > > + disable_irq(dev->irq); > adap = &dev->adapter; > i2c_set_adapdata(adap, dev); > adap->owner = THIS_MODULE; -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
+Kalle, Grygorii, Huzefa Shubhrajyoti D writes: > Enable the irq in the transfer and disable it after the > transfer is done. > > Signed-off-by: Shubhrajyoti D > --- > drivers/i2c/busses/i2c-omap.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index b6c6b95..ce41596 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > if (IS_ERR_VALUE(r)) > goto out; > > + enable_irq(dev->irq); > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > goto out; > @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg > msgs[], int num) > > omap_i2c_wait_for_bb(dev); > out: > + disable_irq(dev->irq); When using runtime PM with auto-suspend timeouts, why would you disable the IRQ before the runtime suspend handlers have run? If you really want to do this, you probably should have these in the runtime PM callbacks. But I'll wait until you add a more descriptive changelog before I can really tell why this is being done. Based on the discussion in the patch from Kalle, I'm assuming this is to prevent interrups when I2C is being used by co-processors. If so, plese describe in the changelog. That being said, doesn't the runtime suspend callback already disable IRQs at the device level instead of the INTC level? Kevin > pm_runtime_mark_last_busy(dev->dev); > pm_runtime_put_autosuspend(dev->dev); > return r; > @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev) > goto err_unuse_clocks; > } > > + disable_irq(dev->irq); > adap = &dev->adapter; > i2c_set_adapdata(adap, dev); > adap->owner = THIS_MODULE; -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
"Strashko, Grygorii" writes: > Hi All, > > Sorry, for the late reply. > + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. Hi Grygorii, thanks for reviewing. I was hoping you would have some ideas here as this was sounding familiar to something you had mentioned elsewhere. > Regarding this patch - from my point of view, it fixes corner case and not > an issue in general. > Let take a look on resume sequence: >- platform resume >- syscore resume >- resume_noirq >- enable IRQs - resume_device_irqs() > |- at this point IRQ handler will be invoked if IRQ state is > IRQS_PENDING. > |- so, the I2C device IRQ handler may be called at time when I2C adapter > IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. > (I2C device and I2C adapter may use different physical IRQ lines) >- resume_late > |- enable I2C bus IRQ > > Possibly, the better way is to enable/disable I2C bus IRQ when needed - in > our case in omap_i2c_xfer(). > We use such approach in Android kernel 3.4 > (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e) I agree, that should work and cover the cases where I2C is used by other processors also. Shubhrajyoti already posted something similar[1] but it needed some rework (comments from Russell and myself.) Huzefa, Shubhrajyoti, who can rework this idea for the upstream driver and submit a patch? Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can come up with a broader solution. Thanks. Kevin [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi,
pe, 2012-10-12 kello 10:18 +, Strashko, Grygorii kirjoitti:
> Hi All,
>
> Sorry, for the late reply.
> + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
>
> Regarding this patch - from my point of view, it fixes corner case and not
> an issue in general.
> Let take a look on resume sequence:
>- platform resume
>- syscore resume
>- resume_noirq
>- enable IRQs - resume_device_irqs()
> |- at this point IRQ handler will be invoked if IRQ state is
> IRQS_PENDING.
> |- so, the I2C device IRQ handler may be called at time when I2C
> adapter IRQ is still disabled and, as result, the I2C device
> IRQ-handler may fail. (I2C device and I2C adapter may use different
> physical IRQ lines)
The use case is to wake up the the device from suspend via twl4030 power
key line. The twl4030 then will interrupt host via the i2c bus. And on
the host the i2c bus is then read by the twl4030-pwrkey threaded SW
interrupt (which is disabled at the point when i2c bus HW interrupt
occurs).
> - resume_late
> |- enable I2C bus IRQ
>
> Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
> our case in omap_i2c_xfer().
> We use such approach in Android kernel 3.4
> (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
Sounds like it could work... however I tested the patch above, but my
device now dies when waking it from suspend (autosleep, actually) with
power key :(
Have you guys considered reworking that patch for upstream? There seems
to be some spinlocking there that was not in linux-omap that I tested
on.
- Kalle
>
> Grygorii
>
> From: Kevin Hilman [khil...@deeprootsystems.com]
> Sent: Friday, October 12, 2012 12:08 AM
> To: Kalle Jokiniemi
> Cc: linux-i2c@vger.kernel.org; w.s...@pengutronix.de; ben-li...@fluff.org;
> t...@atomide.com; linux-o...@vger.kernel.org; Strashko, Grygorii; Datta,
> Shubhrajyoti
> Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
>
> Hi Kalle,
>
> Kalle Jokiniemi writes:
>
> > The resume_noirq enables interrupts one-by-one starting from
> > first one. Now if the wake up event for suspend came from i2c
> > device, the i2c bus irq gets enabled before the threaded
> > i2c device irq, causing a flood of i2c bus interrupts as the
> > threaded irq that should clear the event is not enabled yet.
> >
> > Fixed the issue by adding suspend_noirq and resume_early
> > functions that keep i2c bus interrupts disabled until
> > resume_noirq has run completely.
> >
> > Issue was detected doing a wake up from autosleep with
> > twl4030 power key on N9. Patch tested on N9.
> >
> > Signed-off-by: Kalle Jokiniemi
>
> This version looks good, thanks for the extra comments.
>
> Reviewed-by: Kevin Hilman
> Tested-by: Kevin Hilman
>
> Wolfram, This should also probably be Cc'd to stable since it affects
> earlier kernels as well. Thanks,
>
> Kevin
>
> > ---
> > drivers/i2c/busses/i2c-omap.c | 34 ++
> > 1 files changed, 34 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index a0e49f6..991341b 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
> > platform_device *pdev)
> > }
> >
> > #ifdef CONFIG_PM
> > +#ifdef CONFIG_PM_SLEEP
> > +static int omap_i2c_suspend_noirq(struct device *dev)
> > +{
> > +
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > + /* Disabling irq here to balance the enable in resume_early */
> > + disable_irq(_dev->irq);
> > + return 0;
> > +}
> > +
> > +static int omap_i2c_resume_early(struct device *dev)
> > +{
> > +
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> > +
> > + /*
> > + * The noirq_resume enables the interrupts one by one,
> > + * this causes a interrupt flood if the SW irq actually reading
> > + * event from i2c device is enabled only after i2c bus irq, as the
> > + * irq that should clear the event is still disabled. We have to
> > + * keep the bus irq disabled until all other irqs have been enabled.
> > + */
> > + enable_irq(_dev->irq);
> > +
> > + return 0;
> > +}
> > +#endif
> > #ifdef CONFIG_PM_RUNTIME
> > static int omap_i2c_runtime_suspend(struct device *dev)
> > {
> > @@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device
> > *dev)
> > #endif /* CONFIG_PM_RUNTIME */
> >
> > static struct dev_pm_ops omap_i2c_pm_ops = {
> > +#ifdef CONFIG_PM_SLEEP
> > + .suspend_noirq = omap_i2c_suspend_noirq,
> > + .resume_early = omap_i2c_resume_early,
> > +#endif
> > SET_RUNTIME_PM_OPS(om
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Shubro, As Grygorii pointed out we had a generic issue of arm getting interrupted for i2c accesses outside it (IPU in case of omap4/omap5) and to avoid that we would enable/disable mpu level interrupts only when required from i2c-omap driver. Regards Huzefa -Original Message- From: Datta, Shubhrajyoti Sent: Friday, October 12, 2012 6:33 AM To: Strashko, Grygorii Cc: Kevin Hilman; Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.s...@pengutronix.de; ben-li...@fluff.org; t...@atomide.com; linux-o...@vger.kernel.org; Kankroliwala, Huzefa Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume On Friday 12 October 2012 03:48 PM, Strashko, Grygorii wrote: > Hi All, > > Sorry, for the late reply. > + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. > > Regarding this patch hmm I had similar ideas however my idea was to pacify the arm interrupts when the ip is used by another processor.(something my hw spin patch missed) Any-ways will wait for Huzefa to comment. > - from my point of view, it fixes corner case and not an issue in general. > Let take a look on resume sequence: >- platform resume >- syscore resume >- resume_noirq >- enable IRQs - resume_device_irqs() > |- at this point IRQ handler will be invoked if IRQ state is > IRQS_PENDING. > |- so, the I2C device IRQ handler may be called at time when I2C adapter > IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. > (I2C device and I2C adapter may use different physical IRQ lines) >- resume_late > |- enable I2C bus IRQ > > Possibly, the better way is to enable/disable I2C bus IRQ when needed - in > our case in omap_i2c_xfer(). > We use such approach in Android kernel 3.4 > (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e) > > Grygorii > __ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
On Friday 12 October 2012 03:48 PM, Strashko, Grygorii wrote: > Hi All, > > Sorry, for the late reply. > + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. > > Regarding this patch hmm I had similar ideas however my idea was to pacify the arm interrupts when the ip is used by another processor.(something my hw spin patch missed) Any-ways will wait for Huzefa to comment. > - from my point of view, it fixes corner case and not an issue in general. > Let take a look on resume sequence: >- platform resume >- syscore resume >- resume_noirq >- enable IRQs - resume_device_irqs() > |- at this point IRQ handler will be invoked if IRQ state is > IRQS_PENDING. > |- so, the I2C device IRQ handler may be called at time when I2C adapter > IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. > (I2C device and I2C adapter may use different physical IRQ lines) >- resume_late > |- enable I2C bus IRQ > > Possibly, the better way is to enable/disable I2C bus IRQ when needed - in > our case in omap_i2c_xfer(). > We use such approach in Android kernel 3.4 > (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e) > > Grygorii > __ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi All,
Sorry, for the late reply.
+ CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
Regarding this patch - from my point of view, it fixes corner case and not an
issue in general.
Let take a look on resume sequence:
- platform resume
- syscore resume
- resume_noirq
- enable IRQs - resume_device_irqs()
|- at this point IRQ handler will be invoked if IRQ state is IRQS_PENDING.
|- so, the I2C device IRQ handler may be called at time when I2C adapter
IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. (I2C
device and I2C adapter may use different physical IRQ lines)
- resume_late
|- enable I2C bus IRQ
Possibly, the better way is to enable/disable I2C bus IRQ when needed - in our
case in omap_i2c_xfer().
We use such approach in Android kernel 3.4
(http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
Grygorii
From: Kevin Hilman [khil...@deeprootsystems.com]
Sent: Friday, October 12, 2012 12:08 AM
To: Kalle Jokiniemi
Cc: linux-i2c@vger.kernel.org; w.s...@pengutronix.de; ben-li...@fluff.org;
t...@atomide.com; linux-o...@vger.kernel.org; Strashko, Grygorii; Datta,
Shubhrajyoti
Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi Kalle,
Kalle Jokiniemi writes:
> The resume_noirq enables interrupts one-by-one starting from
> first one. Now if the wake up event for suspend came from i2c
> device, the i2c bus irq gets enabled before the threaded
> i2c device irq, causing a flood of i2c bus interrupts as the
> threaded irq that should clear the event is not enabled yet.
>
> Fixed the issue by adding suspend_noirq and resume_early
> functions that keep i2c bus interrupts disabled until
> resume_noirq has run completely.
>
> Issue was detected doing a wake up from autosleep with
> twl4030 power key on N9. Patch tested on N9.
>
> Signed-off-by: Kalle Jokiniemi
This version looks good, thanks for the extra comments.
Reviewed-by: Kevin Hilman
Tested-by: Kevin Hilman
Wolfram, This should also probably be Cc'd to stable since it affects
earlier kernels as well. Thanks,
Kevin
> ---
> drivers/i2c/busses/i2c-omap.c | 34 ++
> 1 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a0e49f6..991341b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
> platform_device *pdev)
> }
>
> #ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
> +static int omap_i2c_suspend_noirq(struct device *dev)
> +{
> +
> + struct platform_device *pdev = to_platform_device(dev);
> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> +
> + /* Disabling irq here to balance the enable in resume_early */
> + disable_irq(_dev->irq);
> + return 0;
> +}
> +
> +static int omap_i2c_resume_early(struct device *dev)
> +{
> +
> + struct platform_device *pdev = to_platform_device(dev);
> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> +
> + /*
> + * The noirq_resume enables the interrupts one by one,
> + * this causes a interrupt flood if the SW irq actually reading
> + * event from i2c device is enabled only after i2c bus irq, as the
> + * irq that should clear the event is still disabled. We have to
> + * keep the bus irq disabled until all other irqs have been enabled.
> + */
> + enable_irq(_dev->irq);
> +
> + return 0;
> +}
> +#endif
> #ifdef CONFIG_PM_RUNTIME
> static int omap_i2c_runtime_suspend(struct device *dev)
> {
> @@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device *dev)
> #endif /* CONFIG_PM_RUNTIME */
>
> static struct dev_pm_ops omap_i2c_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> + .suspend_noirq = omap_i2c_suspend_noirq,
> + .resume_early = omap_i2c_resume_early,
> +#endif
> SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> omap_i2c_runtime_resume, NULL)
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c: change the id to let the i2c-gpio work
On Fri, 12 Oct 2012 17:42:51 +0800, Bo Shen wrote:
> The i2c-gpio driver will turn the platform device ID to busnum.
> When using platfrom device ID as -1, it means dynamically assigned
> the busnum. When writing code, we need to make sure the busnum,
> and call i2c_register_board_info(int busnum, ...) to register device
> if using -1, we do not know the value of busnum.
>
> In order to solve this issue, set the platform device ID as a fix number
> Here using 0 to match the busnum used in i2c_regsiter_board_info().
>
> Signed-off-by: Bo Shen
> ---
> Change since v1
> Make the commit message more clear
> ---
> arch/arm/mach-at91/at91sam9260_devices.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-at91/at91sam9260_devices.c
> b/arch/arm/mach-at91/at91sam9260_devices.c
> index 0f24cfb..805ef95 100644
> --- a/arch/arm/mach-at91/at91sam9260_devices.c
> +++ b/arch/arm/mach-at91/at91sam9260_devices.c
> @@ -389,7 +389,7 @@ static struct i2c_gpio_platform_data pdata = {
>
> static struct platform_device at91sam9260_twi_device = {
> .name = "i2c-gpio",
> - .id = -1,
> + .id = 0,
> .dev.platform_data = &pdata,
> };
>
Acked-by: Jean Delvare
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] i2c: change the id to let the i2c-gpio work
The i2c-gpio driver will turn the platform device ID to busnum.
When using platfrom device ID as -1, it means dynamically assigned
the busnum. When writing code, we need to make sure the busnum,
and call i2c_register_board_info(int busnum, ...) to register device
if using -1, we do not know the value of busnum.
In order to solve this issue, set the platform device ID as a fix number
Here using 0 to match the busnum used in i2c_regsiter_board_info().
Signed-off-by: Bo Shen
---
Change since v1
Make the commit message more clear
---
arch/arm/mach-at91/at91sam9260_devices.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-at91/at91sam9260_devices.c
b/arch/arm/mach-at91/at91sam9260_devices.c
index 0f24cfb..805ef95 100644
--- a/arch/arm/mach-at91/at91sam9260_devices.c
+++ b/arch/arm/mach-at91/at91sam9260_devices.c
@@ -389,7 +389,7 @@ static struct i2c_gpio_platform_data pdata = {
static struct platform_device at91sam9260_twi_device = {
.name = "i2c-gpio",
- .id = -1,
+ .id = 0,
.dev.platform_data = &pdata,
};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: change the id to let the i2c device work
On 10/12/2012 16:21, Mark Brown wrote: On Fri, Oct 12, 2012 at 10:05:21AM +0200, Jean Delvare wrote: Ah sorry I misread Mark's request. i2c-gpio will turn the platform device ID into bus number, it can indeed not be forced through platform data. But I don't think any other i2c bus driver allows this either. I don't quite see the problem with setting a platform device ID even if there's only one instance of the platform device. I have many examples of this on my machine: Fixed MDIO bus.0 coretemp.0 vesafb.0 This is generally bad style; if it's required by APIs we really should be fixing the APIs to remove this sort of dependency. Aside from the ugliness it tends to be fragile. So please just set the platform device ID to 0 (or whatever i2c adapter number you want) and your problem is solved. As you just proposed initially, actually :) Though it *does* need a comprehensible commit message so people can understand what on earth the change is intended to do. I will update the commit message and send v2 patch. Thanks BR, Bo Shen -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: change the id to let the i2c device work
On Fri, Oct 12, 2012 at 10:05:21AM +0200, Jean Delvare wrote: > Ah sorry I misread Mark's request. i2c-gpio will turn the platform > device ID into bus number, it can indeed not be forced through platform > data. But I don't think any other i2c bus driver allows this either. I > don't quite see the problem with setting a platform device ID even if > there's only one instance of the platform device. I have many examples > of this on my machine: > Fixed MDIO bus.0 > coretemp.0 > vesafb.0 This is generally bad style; if it's required by APIs we really should be fixing the APIs to remove this sort of dependency. Aside from the ugliness it tends to be fragile. > So please just set the platform device ID to 0 (or whatever i2c adapter > number you want) and your problem is solved. As you just proposed > initially, actually :) Though it *does* need a comprehensible commit message so people can understand what on earth the change is intended to do. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: change the id to let the i2c device work
On Fri, 12 Oct 2012 15:55:32 +0800, Bo Shen wrote:
> vim I only see the i2c-gpio platform data structure as following:
> --<-
> struct i2c_gpio_platform_data {
> unsigned intsda_pin;
> unsigned intscl_pin;
> int udelay;
> int timeout;
> unsigned intsda_is_open_drain:1;
> unsigned intscl_is_open_drain:1;
> unsigned intscl_is_output_only:1;
> };
> -->-
Ah sorry I misread Mark's request. i2c-gpio will turn the platform
device ID into bus number, it can indeed not be forced through platform
data. But I don't think any other i2c bus driver allows this either. I
don't quite see the problem with setting a platform device ID even if
there's only one instance of the platform device. I have many examples
of this on my machine:
Fixed MDIO bus.0
coretemp.0
vesafb.0
So please just set the platform device ID to 0 (or whatever i2c adapter
number you want) and your problem is solved. As you just proposed
initially, actually :)
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: change the id to let the i2c device work
Hi Jean Delvare,
On 10/12/2012 15:14, Jean Delvare wrote:
On Fri, 12 Oct 2012 14:53:02 +0900, Mark Brown wrote:
On Fri, Oct 12, 2012 at 01:45:48PM +0800, Bo Shen wrote:
So, in this case, if the id = -1, i2c core will dynamically assign a
bus id to this bus when running, the dynamically assigned bus id is
unknown when writing the code. So, when we use
i2c_register_board_info(int busnum, ...) to register device on
busnum. we don't know which value to be use.
The I2C bus number assigned to the controller should be independant of
the platform device ID used to register the device; a better fix if this
is an issue would be to update the i2c-gpio driver to allow a fixed bus
number to be specified in platform data.
i2c-gpio does support this already.
vim I only see the i2c-gpio platform data structure as following:
--<-
struct i2c_gpio_platform_data {
unsigned intsda_pin;
unsigned intscl_pin;
int udelay;
int timeout;
unsigned intsda_is_open_drain:1;
unsigned intscl_is_open_drain:1;
unsigned intscl_is_output_only:1;
};
-->-
So, how to allow a fixed bus number to be specified in platform data?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: change the id to let the i2c device work
On Fri, 12 Oct 2012 14:53:02 +0900, Mark Brown wrote: > On Fri, Oct 12, 2012 at 01:45:48PM +0800, Bo Shen wrote: > > So, in this case, if the id = -1, i2c core will dynamically assign a > > bus id to this bus when running, the dynamically assigned bus id is > > unknown when writing the code. So, when we use > > i2c_register_board_info(int busnum, ...) to register device on > > busnum. we don't know which value to be use. > > The I2C bus number assigned to the controller should be independant of > the platform device ID used to register the device; a better fix if this > is an issue would be to update the i2c-gpio driver to allow a fixed bus > number to be specified in platform data. i2c-gpio does support this already. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
