Hi Afzal,
On 04/06/2012 01:52 AM, Mohammed, Afzal wrote:
Hi Jon,
On Fri, Apr 06, 2012 at 01:51:41, Hunter, Jon wrote:
+struct gpmc_irq {
+ unsigned irq;
+ u32 regval;
Are you using regval to indicate the bit-mask? If so, maybe call it
"bitmask" instead.
Yes, bitmask would be better.
+ switch (gpmc->irq[i].regval) {
+ case GPMC_IRQ_WAIT0EDGEDETECTION:
+ case GPMC_IRQ_WAIT1EDGEDETECTION:
+ case GPMC_IRQ_WAIT2EDGEDETECTION:
+ case GPMC_IRQ_WAIT3EDGEDETECTION:
+ val = __ffs(gpmc->irq[i].regval);
+ val -= __ffs(GPMC_IRQ_WAIT0EDGEDETECTION);
+ gpmc_cs_configure(cs->cs,
+ GPMC_CONFIG_WAITPIN, val);
Why is the configuration of the wait pin done here? It is possible to
use the wait pin may be used without enabling the interrupt.
Where do you handle allocating the wait pins to ensure two devices don't
attempt to use the same one? Like how the CS are managed.
Also, where you you configure the polarity of the wait pins? I would
have thought it would make sense to have the wait pin configured as part
of the cs configuration.
I will revisit waitpin configurations.
Ok.
+ for (i = 0, j = 0, cs = gdp->cs_data; i< gdp->num_cs; cs++, i++) {
+ dev->gpmc_res[j] = gpmc_setup_cs_mem(cs, gdp, gpmc);
+ if (dev->gpmc_res[j++].flags& IORESOURCE_MEM)
+ j += gpmc_setup_cs_irq(gpmc, gdp, cs,
+ dev->gpmc_res + j);
+ else {
+ dev_err(gpmc->dev, "error: setup for %s\n", gdp->name);
+ devm_kfree(gpmc->dev, dev->gpmc_res);
+ dev->gpmc_res = NULL;
+ dev->num_gpmc_res = 0;
+ return -EINVAL;
+ }
}
This section of code is not straight-forward to read. I see what you are
doing, but I am wondering if this could be improved.
First of all, returning a structure from a function is making this code
harder to read. Per the CodingStyle document in the kernel, it is
preferred for a function to return success or failure if the function is
an action, like a setup.
Secondly, do you need to pass cs, gdp and gpmc to gpmc_setup_cs_mem()?
It appears that gdp and gpmc are only used for prints. You could
probably avoid passing gdp and move the print to outside this function.
This section will be modified to make it clearer.
Thanks.
+ if (gpmc->num_irq< GPMC_NR_IRQ) {
+ dev_warn(gpmc->dev, "Insufficient interrupts for device\n");
+ return -EINVAL;
+ } else if (gpmc->num_irq> GPMC_NR_IRQ)
+ gpmc->num_irq = GPMC_NR_IRQ;
Hmmm ... why not just have ...
if (gpmc->num_irq != GPMC_NR_IRQ) {
dev_warn(...);
return -EINVAL;
}
This will cause irq setup to fail if num_irq> GPMC_NR_IRQ, even though
irq setup could have been done w/o any problem, only because platform
indicated willingness to accommodate more number of interrupts than
actually required for this device.
Ok, so num_irqs represents OMAP_GPMC_NR_IRQS and we are making sure we
have enough IRQs allocated by the platform.
This also raises the question why bother passing num_irq if we always
want it to be GPMC_NR_IRQ? Why not always initialise them all driver?
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?
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.
So we could simplify this by configuring num_irqs in the probe function
and then just make sure num_irqs is less than OMAP_GPMC_NR_IRQS above.
+ 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 :-)
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.
+ 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.
The above loop appears to terminate when *gdq == 0. Is this always
guaranteed? In other words, safe?
This is a standard idiom for array of pointers
Ok.
I see that "i" is not being used in the above loop either.
That was left over code, it will be removed.
Thanks.
+struct gpmc_cs_data {
+ unsigned cs;
+ unsigned long mem_size;
+ unsigned long mem_start;
+ unsigned long mem_offset;
+ struct gpmc_config *config;
+ unsigned num_config;
+ struct gpmc_timings *timing;
+ unsigned irq_flags;
I found the name irq_flags a bit unclear. This appears to be a mask for
the interrupts used. So may be this should be "irq_mask" or just "irqs".
Probably, this confusion arose as an attempt was made to club irq bit fields
with irq capability, so as to reduce additional macro definitions.
Here my preference is to keep irq_flags (or can be irq_capabilities), define
a new set of macros, and use bitmask in struct gpmc_irq
Ok, how about irq_config? The name irq_flags make me think of flags
passed to configure the irq as in the set_irq_flags() API.
Thanks for the comments.
No problem.
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
