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

[Jon Hunter]

Hi Afzal,

On 04/10/2012 06:00 AM, Mohammed, Afzal wrote:
> Hi Jon,
>
> On Tue, Apr 10, 2012 at 01:20:37, Hunter, Jon wrote:
>>> Because num_irq (or available irqs for fictitious irq chip) is platform specific.
>>
>> Ok, so OMAP_GPMC_NR_IRQS is defined and will not vary from device to
>> device, so why pass this? Why not use it directly?
>
> Because OMAP_GPMC_NR_IRQS is platform specific, final intention is to
> not have any platform specific header files in GPMC driver, not sure
> as of now whether it would be possible. So keeping platform specific
> things away from the driver as much as possible.
>
> And consider scenario where GPMC IP is used in other than OMAP family,
> even though this a theoretical case, wanted to stress the point that
> intention is to keep driver platform independent.
>
> Or else dynamic allocation of irqs may have to be used.

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;

>>
>> 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?

>>>>> +	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 ;-)

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.

Also in gpmc_setup_irq() you have two for loops incrementing through 0 
to gpmc->num_irq, can these for loops be combined?

>>> If someone (say NAND) wants to work with irq, and if interrupt is not
>>> available, NAND driver can fail, and suppose smsc911x ethernet is present
>>> and he is not using gpmc irq, if we fail here, smsc911x also would
>>> not work, which would have worked otherwise.
>>>
>>> It is a different matter that even NAND setup will happen properly,
>>> even if interrupt is not available, it is only when NAND is told to
>>> work with IRQ, it fails, please see nand patches.
>>>
>>> And as of now in mainline (with the code as is), there is not a single
>>> board that will need gpmc irq for gpmc peripherals to work.
>>>
>>> I feel we should try to get more devices working rather than preventing
>>> more devices from working, when it could have.
>>
>> I understand your point, but then you are hiding a BUG. If someone
>> introduces a BUG that causes this to fail, then it is easier to detect,
>> find and fix.
>>
>>   From my perspective get the resources should never fail and if it does
>> and break the driver for all devices, then so be it, because a BUG has
>> been introduced. Ok, this may not be critical at this point but still is
>> should not fail.
>
> Agree for resources which are a must for device to work, not for resources
> that can enhance its capability.

If we can ensure that a device using the gpmc driver would fail when 
requesting a specific gpmc irq (if the gpmc driver has failed to 
initialise its irqs) then this would be ok. In other words, a device 
calling request_irq for one of the pseudo gpmc irqs returns failure if 
the gpmc itself failed to setup them up.

>>
>>>>> +	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?

Cheers
Jon