From: Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
To: Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: "Uwe Kleine-König"
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
"Wolfram Sang" <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>,
"Rob Herring" <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
"Pawel Moll" <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
"Mark Rutland" <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
"Ian Campbell"
<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
"Kumar Gala" <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
"Grant Likely"
<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
"Christian Daudt" <bcm-xK7y4jjYLqYh9ZMKESR00Q@public.gmane.org>,
"Matt Porter" <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
"Florian Fainelli"
<f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
"Scott Branden"
<sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Date: Mon, 19 Jan 2015 13:25:05 -0800 [thread overview]
Message-ID: <54BD7631.50205@broadcom.com> (raw)
In-Reply-To: <20150119192805.GF26493-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
On 1/19/2015 11:28 AM, Russell King - ARM Linux wrote:
> On Sat, Jan 17, 2015 at 04:30:33PM -0800, Ray Jui wrote:
>>
>>
>> On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote:
>>> On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote:
>>>> time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
>>>>
>>>> /* disable all interrupts */
>>>> writel(0, iproc_i2c->base + IE_OFFSET);
>>>>
>>>> if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) {
>>>
>>> Why are you using atomic_read() here?
>>>
>> transfer_is_successful 1) will be reset to 0 in this function (before
>> kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal
>> completion of the I2C transfer), and 3) will be checked in this function
>> here. I thought that means I should declare it volatile, because it can
>> be modified in both the process context and interrupt context (and I use
>> atomic because I remember Linux checkpatch warns against using volatile)?
>
> You don't need volatile or atomic_t for that.
>
> Rather than switching to atomic_t when seeing the checkpatch warning,
> you'd do better to read Documentation/volatile-considered-harmful.txt
> to understand why checkpatch issues the warning, and realise that you
> don't need it for the above.
>
> Note that in the above code, the compiler can't make an assumption
> about iproc_i2c->transfer_is_successful because it can't tell whether
> a called function (eg, wait_for_completion_timeout()) could modify it.
>
Got it. Thanks.
> Another possible issue with the above code are these lines:
>
> /* disable all interrupts */
> writel(0, iproc_i2c->base + IE_OFFSET);
>
> It would be nice to think that would hit the hardware immediately, but
> that's making assumptions about hardware which are not necessary true.
> Your interrupt handler could even be running on another CPU after you've
> asked for that register to be written.
>
> Depending on what you're trying to achieve here, you may need:
>
> /* disable all interrupts */
> writel(0, iproc_i2c->base + IE_OFFSET);
> /* read it back to ensure the write has hit */
> readl(iproc_i2c->base + IE_OFFSET);
>
> /* make sure the interrupt handler isn't running */
> synchronize_irq(...->irq);
>
> if what you're trying to do is to ensure that the interrupt handler has
> finished running.
>
This will be the most robust way of handling this. Given that we've
added an additional flag to check to make sure there's no interrupt
missed after wait_for_completion_timeout times out, it makes sense to
ensure that by the time when we check the flag there's no pending irq.
I'll add this to the driver and make 'xfer_is_done' an 'int' instead of
'atomic_t'. I will also add the call to readl to flush the write in the
remove function after interrupts are disabled.
next prev parent reply other threads:[~2015-01-19 21:25 UTC|newest]
Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <Ray Jui <rjui@broadcom.com>
[not found] ` <Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2014-12-10 0:54 ` [PATCH 0/4] Add I2C support to Broadcom iProc Ray Jui
2014-12-10 0:54 ` [PATCH 1/4] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2014-12-10 1:27 ` Varka Bhadram
[not found] ` <5487A16C.5000707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-12-10 1:35 ` Ray Jui
[not found] ` <CAEUmHyb_wu1kVj-G5LQj8Q_A1Tid1gSGbU=cGeBf4EXZgXChbg@mail.gmail.com>
[not found] ` <CAEUmHyb_wu1kVj-G5LQj8Q_A1Tid1gSGbU=cGeBf4EXZgXChbg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-12-10 3:27 ` Ray Jui
2014-12-10 0:54 ` [PATCH 2/4] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
2014-12-10 1:33 ` Varka Bhadram
[not found] ` <5487A2D6.8070901-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-12-10 1:41 ` Ray Jui
[not found] ` <CAEUmHyZ+VqjzL4LkQozGJtnictPNXHYWM2qMKvD=LmfQdcT8iQ@mail.gmail.com>
[not found] ` <CAEUmHyZ86r=7KzJzfE9_upv45vN7geW9woqMkaGaBPwfp3xbMQ@mail.gmail.com>
[not found] ` <CAEUmHyZ86r=7KzJzfE9_upv45vN7geW9woqMkaGaBPwfp3xbMQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-12-10 3:31 ` Ray Jui
2014-12-10 0:54 ` [PATCH 3/4] ARM: mach-bcm: Enable I2C support for iProc Ray Jui
2014-12-10 0:54 ` [PATCH 4/4] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2014-12-10 2:18 ` [PATCH v2 0/4] Add I2C support to Broadcom iProc Ray Jui
2014-12-10 2:18 ` [PATCH v2 1/4] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2014-12-10 2:18 ` [PATCH v2 2/4] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1418177893-22094-1-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2014-12-10 2:18 ` [PATCH v2 3/4] ARM: mach-bcm: Enable I2C support for iProc Ray Jui
[not found] ` <1418177893-22094-4-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2014-12-10 2:20 ` Florian Fainelli
[not found] ` <5487ADE5.4070705-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-12-10 2:24 ` Ray Jui
[not found] ` <5487AEF0.5010404-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2014-12-10 3:20 ` Florian Fainelli
[not found] ` <5487BBE0.4000701-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-12-10 3:58 ` Ray Jui
2014-12-10 2:18 ` [PATCH v2 4/4] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2014-12-10 3:57 ` [PATCH v3 0/3] Add I2C support to Broadcom iProc Ray Jui
2014-12-10 3:57 ` [PATCH v3 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2014-12-10 3:57 ` [PATCH v3 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1418183832-24793-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-13 22:50 ` Uwe Kleine-König
[not found] ` <20150113225012.GK22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-14 2:14 ` Ray Jui
2015-01-14 7:51 ` Uwe Kleine-König
2015-01-14 20:05 ` Ray Jui
[not found] ` <54B5D0F9.8030902-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-15 11:59 ` Wolfram Sang
2015-01-16 22:51 ` Ray Jui
2014-12-10 3:57 ` [PATCH v3 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
[not found] ` <548F577E.7020207@broadcom.com>
[not found] ` <548F577E.7020207-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2014-12-15 21:55 ` [PATCH v3 0/3] Add I2C support to Broadcom iProc Wolfram Sang
2014-12-16 0:12 ` Ray Jui
2015-01-14 22:23 ` [PATCH v4 " Ray Jui
2015-01-14 22:23 ` [PATCH v4 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
[not found] ` <1421274213-3544-1-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-14 22:23 ` [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1421274213-3544-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-15 8:41 ` Uwe Kleine-König
[not found] ` <20150115084119.GN22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-15 12:07 ` Wolfram Sang
2015-01-15 16:32 ` Uwe Kleine-König
2015-01-16 22:52 ` Ray Jui
2015-01-16 22:09 ` Ray Jui
[not found] ` <54B98C18.4080807-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-17 16:01 ` Uwe Kleine-König
[not found] ` <20150117160113.GA22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-17 19:58 ` Ray Jui
[not found] ` <54BABEE9.8070801-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-17 20:18 ` Uwe Kleine-König
[not found] ` <20150117201849.GC22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-17 20:51 ` Ray Jui
[not found] ` <54BACB66.6040909-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-17 21:10 ` Uwe Kleine-König
[not found] ` <20150117211017.GD22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-17 21:26 ` Ray Jui
[not found] ` <54BAD391.9080909-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-17 22:40 ` Russell King - ARM Linux
[not found] ` <20150117224021.GA26493-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2015-01-18 0:30 ` Ray Jui
2015-01-19 19:28 ` Russell King - ARM Linux
[not found] ` <20150119192805.GF26493-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2015-01-19 21:25 ` Ray Jui [this message]
2015-01-14 22:23 ` [PATCH v4 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
[not found] ` <1421274213-3544-4-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-15 8:44 ` Uwe Kleine-König
[not found] ` <20150115084456.GO22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-16 19:24 ` Ray Jui
[not found] ` <54B96559.1010007-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-16 19:48 ` Uwe Kleine-König
[not found] ` <20150116194831.GV22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-16 23:18 ` Ray Jui
2015-01-16 23:42 ` [PATCH v5 0/3] Add I2C support to Broadcom iProc Ray Jui
[not found] ` <1421451737-7107-1-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-16 23:42 ` [PATCH v5 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2015-01-16 23:42 ` [PATCH v5 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1421451737-7107-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-18 9:14 ` Arend van Spriel
[not found] ` <54BB795C.6040402-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-18 9:47 ` Uwe Kleine-König
[not found] ` <20150118094741.GE22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-18 11:06 ` Wolfram Sang
2015-01-18 11:17 ` Uwe Kleine-König
[not found] ` <20150118111759.GG22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-18 11:42 ` Wolfram Sang
2015-01-18 11:46 ` Arend van Spriel
[not found] ` <54BB9D2B.20408-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-18 11:56 ` Uwe Kleine-König
[not found] ` <20150118115650.GH22880-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-01-18 12:13 ` Arend van Spriel
[not found] ` <54BBA36A.10608-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-19 19:15 ` Ray Jui
2015-01-16 23:42 ` [PATCH v5 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2015-01-19 19:23 ` [PATCH v6 0/3] Add I2C support to Broadcom iProc Ray Jui
2015-01-19 19:23 ` [PATCH v6 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1421695428-19102-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-19 19:44 ` Russell King - ARM Linux
[not found] ` <20150119194420.GG26493-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
2015-01-19 21:31 ` Ray Jui
[not found] ` <1421695428-19102-1-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-01-19 19:23 ` [PATCH v6 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2015-01-19 19:23 ` [PATCH v6 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2015-01-19 21:51 ` [PATCH v7 0/3] Add I2C support to Broadcom iProc Ray Jui
2015-01-19 21:51 ` [PATCH v7 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2015-01-19 21:51 ` [PATCH v7 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
2015-02-06 22:31 ` [v7,2/3] " Kevin Cernekee
[not found] ` <20150206223149.GB345-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2015-02-06 22:48 ` Dmitry Torokhov
[not found] ` <CAE_wzQ-POweLLmTyHoMvs_NESjW5UmPxh2ZQCaW4-W74MsrHag-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-02-06 23:01 ` Kevin Cernekee
2015-02-07 0:54 ` Ray Jui
2015-01-19 21:51 ` [PATCH v7 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2015-02-07 1:28 ` [PATCH v8 0/3] Add I2C support to Broadcom iProc Ray Jui
2015-02-07 1:28 ` [PATCH v8 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
2015-02-07 1:28 ` [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1423272507-18459-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-07 17:50 ` Wolfram Sang
2015-02-08 5:08 ` Ray Jui
2015-02-08 11:03 ` Wolfram Sang
2015-02-08 18:10 ` Ray Jui
[not found] ` <54D7A694.4000903-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-09 10:03 ` Wolfram Sang
2015-02-07 1:28 ` [PATCH v8 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2015-02-08 5:25 ` [PATCH v9 0/3] Add I2C support to Broadcom iProc Ray Jui
2015-02-08 5:25 ` [PATCH v9 1/3] i2c: iProc: define Broadcom iProc I2C binding Ray Jui
[not found] ` <1423373126-30024-2-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-09 12:09 ` Wolfram Sang
[not found] ` <1423373126-30024-1-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-08 5:25 ` [PATCH v9 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Ray Jui
[not found] ` <1423373126-30024-3-git-send-email-rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-08 16:29 ` Wolfram Sang
2015-02-08 17:56 ` Ray Jui
2015-02-09 12:10 ` Wolfram Sang
2015-02-10 5:23 ` Ray Jui
[not found] ` <54D995DA.7040201-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-10 8:33 ` Wolfram Sang
2015-02-10 17:10 ` Ray Jui
2015-02-08 5:25 ` [PATCH v9 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus Ray Jui
2015-02-09 12:11 ` Wolfram Sang
2015-02-10 5:24 ` Ray Jui
[not found] ` <54D99606.9070309-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
2015-02-10 5:34 ` Florian Fainelli
[not found] ` <54D99848.2080205-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2015-02-10 5:36 ` Ray Jui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54BD7631.50205@broadcom.com \
--to=rjui-dy08kvg/lbpwk0htik3j/w@public.gmane.org \
--cc=bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org \
--cc=bcm-xK7y4jjYLqYh9ZMKESR00Q@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \
--cc=grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org \
--cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
--cc=mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=pawel.moll-5wv7dgnIgG8@public.gmane.org \
--cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org \
--cc=u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org \
--cc=wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).