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

[Mohammed, Afzal]

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

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

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

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

Regards
Afzal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/