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

[Mohammed, Afzal]

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.

Regards
Afzal