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