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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
