[PATCH v4 01/39] ARM: OMAP2+: gpmc: driver conversion

[Jon Hunter]

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