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