From: jon-hunter@ti•com (Jon Hunter)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion
Date: Tue, 8 May 2012 10:08:24 -0500 [thread overview]
Message-ID: <4FA936E8.1080801@ti.com> (raw)
In-Reply-To: <C8443D0743D26F4388EA172BF4E2A7A93E9878EC@DBDE01.ent.ti.com>
Hi Afzal,
On 05/08/2012 01:18 AM, Mohammed, Afzal wrote:
> Hi Jon,
>
> On Mon, May 07, 2012 at 21:32:58, Hunter, Jon wrote:
>
>>>>> + /* no waitpin */
>>>>> + case 0:
>>>>> + break;
>>>>> + default:
>>>>> + dev_err(gpmc->dev, "multiple waitpins selected on CS:%u\n", cs);
>>>>> + return -EINVAL;
>>>>> + break;
>>>>> + }
>>>>
>>>> Why not combined case 0 and default? Both are invalid configurations so
>>>> just report invalid selection.
>>>
>>> Case 0 is not invalid, a case where waitpin is not used, default refers
>>> to when a user selects multiple waitpins wrongly.
>>
>> Ok. Then for case 0, just return here. If the polarity is set, you could
>> print an error here.
>
> Different ways of doing things, this looks cleaner to me as already it is
> checked, and time of execution in both cases would not differ much.
Sure. However, I think that this could be simplified so that it is
easier to read. At a minimum you may wish to add some comments to
explain the purposes of the multi-level if-statements as it is not
intuitive for the reader.
>>>>> + if (gd->have_waitpin) {
>>>>> + if (gd->waitpin != idx ||
>>>>> + gd->waitpin_polarity != polarity) {
>>>>> + dev_err(gpmc->dev, "error: conflict: waitpin %u with polarity %d on device %s.%d\n",
>>>>> + gd->waitpin, gd->waitpin_polarity,
>>>>> + gd->name, gd->id);
>>>>> + return -EBUSY;
>>>>> + }
>>>>> + } else {
>>>>
>>>> Don't need the else as you are going to return in the above.
>>>
>>> Not always, only in case of error
>>
>> Ok, but seems that it can be simplified a little.
>>
>> What happens if a device uses more than one wait-pin? In other words a
>> device with two chip-selects that uses two wait-pins?
>
> Please re-read http://www.mail-archive.com/linux-omap at vger.kernel.org/msg67702.html
> and your reply
Ok thats fine. Sorry for my repeated questions, but I think that this
just highlights that this code is not clear in it purpose. So again may
be some comments would make this all clearer.
Jon
next prev parent reply other threads:[~2012-05-08 15:08 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-01 12:19 [PATCH v4 00/39] OMAP GPMC driver conversion Afzal Mohammed
2012-05-01 12:19 ` [PATCH v4 01/39] ARM: OMAP2+: gpmc: " Afzal Mohammed
2012-05-01 17:53 ` Jon Hunter
2012-05-03 8:23 ` Mohammed, Afzal
2012-05-04 16:20 ` Jon Hunter
2012-05-07 10:57 ` Mohammed, Afzal
2012-05-07 16:23 ` Jon Hunter
2012-05-08 6:27 ` Mohammed, Afzal
2012-05-04 16:27 ` Jon Hunter
2012-05-07 11:01 ` Mohammed, Afzal
2012-05-07 16:02 ` Jon Hunter
2012-05-08 6:18 ` Mohammed, Afzal
2012-05-08 15:08 ` Jon Hunter [this message]
2012-05-10 5:10 ` Mohammed, Afzal
2012-05-01 12:19 ` [PATCH v4 02/39] ARM: OMAP2+: gpmc: Adapt to HWMOD Afzal Mohammed
2012-05-01 20:41 ` Jon Hunter
2012-05-03 8:37 ` Mohammed, Afzal
2012-05-04 16:30 ` Jon Hunter
2012-05-07 11:02 ` Mohammed, Afzal
2012-05-07 16:12 ` Jon Hunter
2012-05-08 6:24 ` Mohammed, Afzal
2012-05-08 15:13 ` Jon Hunter
2012-05-10 5:17 ` Mohammed, Afzal
2012-05-01 12:19 ` [PATCH v4 03/39] ARM: OMAP2+: gpmc: register details for nand driver Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 04/39] ARM: OMAP2+: gpmc: Acquire NAND CS value Afzal Mohammed
2012-05-01 21:16 ` Jon Hunter
2012-05-03 8:42 ` Mohammed, Afzal
2012-05-01 12:20 ` [PATCH v4 05/39] ARM: OMAP2+: nand: create platform data structure Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 06/39] ARM: OMAP2+: onenand: return value in init function Afzal Mohammed
2012-05-01 16:49 ` Sergei Shtylyov
2012-05-03 8:54 ` Mohammed, Afzal
2012-05-01 12:20 ` [PATCH v4 07/39] ARM: OMAP2+: gpmc-nand: Adapt to use gpmc driver Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 08/39] ARM: OMAP2+: gpmc-onenand: " Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 09/39] ARM: OMAP2+: flash: Adapt to " Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 10/39] ARM: OMAP2+: gpmc-smsc911x: Adapt to use " Afzal Mohammed
2012-05-01 12:20 ` [PATCH v4 11/39] ARM: OMAP2+: gpmc-smc91x: " Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 12/39] ARM: OMAP2+: gpmc-tusb6010: Adapt to " Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 13/39] ARM: OMAP3: hwmod data: add gpmc Afzal Mohammed
2012-05-06 2:09 ` Paul Walmsley
2012-05-01 12:21 ` [PATCH v4 14/39] ARM: OMAP2xxx: " Afzal Mohammed
2012-05-06 2:09 ` Paul Walmsley
2012-05-01 12:21 ` [PATCH v4 15/39] mtd: nand: omap2: obtain memory from resource Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 16/39] mtd: nand: omap2: use gpmc provided irqs Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 17/39] mtd: nand: omap2: handle nand on gpmc Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 18/39] mtd: onenand: omap: obtain memory from resource Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 19/39] ARM: OMAP2+: board omap3evm: gpmc driver adaptation Afzal Mohammed
2012-05-01 12:21 ` [PATCH v4 20/39] ARM: OMAP2+: board omap3beagle: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 21/39] ARM: OMAP2+: board apollon: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 22/39] ARM: OMAP2+: board h4: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 23/39] ARM: OMAP2+: board 3630sdp: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 24/39] ARM: OMAP2+: board 3430sdp: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 25/39] ARM: OMAP2+: board 2430sdp: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 26/39] ARM: OMAP2+: board cm-t3517: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 27/39] ARM: OMAP2+: board cm-t35: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 28/39] ARM: OMAP2+: board ldp: " Afzal Mohammed
2012-05-01 12:22 ` [PATCH v4 29/39] ARM: OMAP2+: board n8x0: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 30/39] ARM: OMAP2+: board omap3logic: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 31/39] ARM: OMAP2+: board omap3pandora: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 32/39] ARM: OMAP2+: board omap3stalker: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 33/39] ARM: OMAP2+: board omap4pcm049: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 34/39] ARM: OMAP2+: board overo: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 35/39] ARM: OMAP2+: board rm680: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 36/39] ARM: OMAP2+: board rx51: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 37/39] ARM: OMAP2+: board zoom-debugboard: " Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 38/39] OMAP3: igep0020: Add support for Micron NAND Flash storage memory Afzal Mohammed
2012-05-01 12:23 ` [PATCH v4 39/39] ARM: OMAP2+: board igep0020: gpmc driver adaptation Afzal Mohammed
2012-05-08 11:27 ` [PATCH v4 00/39] OMAP GPMC driver conversion Mohammed, Afzal
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=4FA936E8.1080801@ti.com \
--to=jon-hunter@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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