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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
