Re: [PATCH v3 1/9] ARM: OMAP2+: gpmc: driver conversion

[Jon Hunter <jon-hunter@ti.com>]

Hi Afzal,

On 04/11/2012 12:11 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Wed, Apr 11, 2012 at 00:53:14, Hunter, Jon wrote:
>> I agree with your argument but I was thinking today only OMAP uses the 
>> GPMC so we could not worry about this. Ok, leave as-is, but can we 
>> modify the code as follows as the "else if" is not really needed...
>>
>> if (gpmc->num_irq < GPMC_NR_IRQ) {
>> 	dev_warn(gpmc->dev, "Insufficient interrupts for device\n");
>> 	return -EINVAL;
>> }
>>
>> gpmc->num_irq = GPMC_NR_IRQ;
> 
> Yes, it is better
> 
>>
>>>>
>>>> Furthermore, GPMC_NR_IRQ is defined as 6 which is correct for OMAP2/3
>>>> but not for OMAP4/5, it is 5. Therefore, we need to detect whether we
>>>> are using an OMAP2/3 or OMAP4+ and set num_irqs based upon this. This
>>>> could be done in the probe and we can avoid passing this.
>>>
>>> Is it dependent on OMAPX or GPMC IP version? if it is IP version, then driver
>>> can be enhanced to handle it, if not, platform has to pass this information.
>>
>> Here are the GPMC IP revisions ...
>>
>> OMAP5430 = 0x00000060
>> OMAP4430 = 0x00000060
>> OMAP3630 = 0x00000050
>> OMAP3430 = 0x00000050
>>
>> So this should work for OMAP. We should check OMAP2 as well. What about 
>> the AMxxx devices?
> 
> 
> I badly needed this information, thanks.
> 
> AM3359 = 0x00000060, it has only 2 waitpin interrupts

Great so this is consistent!

>>>>>>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>>>> +	if (res == NULL)
>>>>>>> +		dev_warn(gpmc->dev, "Failed to get resource: irq\n");
>>>>>>> +	else
>>>>>>> +		gpmc->master_irq = res->start;
>>>>>>
>>>>>> Why not return an error if the IRQ is not found? We don't know if anyone
>>>>>> will be trying to use them.
>>>>>
>>>>> Why do you want to do that ?
>>>>
>>>> Because this indicates a BUG :-)
>>>
>>> I disagree, this need not be considered a bug always,
>>> for eg. If gpmc irq is not connected to intc
>>
>> Ok, so for devices existing today this indicates a bug ;-)
> 
> I do not want to consider that case to be bug enough for probe
> to fail, there are other drivers which does similar enhancing
> its use cases,
> 
> eg. 1e351a9 mfd: Make TPS65910 usable without interrupts

Ok, fine.

>>
>> At a minimum you need to improve the error handing here. If the 
>> platform_get_resource fails you are still calling "gpmc_setup_irq()" 
>> which appears to be pointless. It would be better if the gpmc irq chip 
>> is not initialised in this case so that drivers attempting to request 
>> these irqs failed.
> 
> Please see gpmc_setup_irq, if irq is not present, it returns in the
> beginning, and gpmc_irq_chip is not initialized in that case.

Yes you are right.

>>>>>>> +	for (gdq = gp->device_pdata, gd = gpmc->device; *gdq; gdq++, i++) {
>>>>>>> +		ret = gpmc_setup_device(*gdq, gd, gpmc);
>>>>>>> +		if (IS_ERR_VALUE(ret))
>>>>>>> +			dev_err(gpmc->dev, "gpmc setup on %s failed\n",
>>>>>>> +								(*gdq)->name);
>>>>>>> +		else
>>>>>>> +			gd++;
>>>>>>> +	}
>>>>>>
>>>>>> Would a while loop be simpler?
>>>>>
>>>>> My preference is to go with "for"
>>>>
>>>> Ok, just wondering if this could be cleaned up a little.
>>>
>>> For travelling through array of pointers, for looks natural to me, if you
>>> have a better way, please send it, it can be folded in next version.
>>
>> Could you have num_devices to indicate how many platform devices there 
>> are and then a simple for-loop of 0 to num_devices?
> 
> This will cause coding to be done by platform to be less simple, and my
> preference is not to use another variable

Hehe, I wondered if that would make life a little more difficult. Ok
lets leave it for now.

Jon