[PATCH v3 1/9] ARM: OMAP2+: gpmc: driver conversion
Jon Hunter
jon-hunter at ti.com
Thu Apr 5 16:21:41 EDT 2012
Hi Afzal,
On 04/05/2012 10:45 AM, Afzal Mohammed wrote:
[...]
> +struct gpmc_irq {
> + unsigned irq;
> + u32 regval;
Are you using regval to indicate the bit-mask? If so, maybe call it
"bitmask" instead.
[...]
> +static __devinit
> +int gpmc_setup_cs_irq(struct gpmc *gpmc, struct gpmc_device_pdata *gdp,
> + struct gpmc_cs_data *cs, struct resource *res)
> +{
> + int i, n, val;
> +
> + for (i = 0, n = 0; i< gpmc->num_irq; i++)
> + if (gpmc->irq[i].regval& cs->irq_flags) {
> + res[n].start = res[n].end = gpmc->irq[i].irq;
> + res[n].flags = IORESOURCE_IRQ;
> +
> + dev_info(gpmc->dev, "resource irq %u for %s "
> + "(on CS %d) [bit: %x]\n", res[n].start,
> + gdp->name, cs->cs, __ffs(gpmc->irq[i].regval));
> +
> + 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.
> + }
> + n++;
> + }
> +
> + return n;
> +}
> +
> +static __devinit int gpmc_setup_device(struct gpmc_device_pdata *gdp,
> + struct gpmc_device *dev, struct gpmc *gpmc)
> +{
> + int i, j, n;
> + struct gpmc_cs_data *cs;
> +
> + for (i = 0, n = 0, cs = gdp->cs_data; i< gdp->num_cs; i++, cs++)
> + n += hweight32(cs->irq_flags& GPMC_IRQ_MASK);
> +
> + n += gdp->num_cs;
> +
> + dev->gpmc_res = devm_kzalloc(gpmc->dev, sizeof(*dev->gpmc_res) * n,
> + GFP_KERNEL);
> + if (dev->gpmc_res == NULL) {
> + dev_err(gpmc->dev, "error: memory allocation\n");
> + return -ENOMEM;
> + }
> +
> + 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.
> + dev->num_gpmc_res = j;
>
> - ret = request_irq(gpmc_irq,
> - gpmc_handle_irq, IRQF_SHARED, "gpmc", gpmc_base);
> - if (ret)
> - pr_err("gpmc: irq-%d could not claim: err %d\n",
> - gpmc_irq, ret);
> - return ret;
> + dev->name = gdp->name;
> + dev->id = gdp->id;
> + dev->pdata = gdp->pdata;
> + dev->pdata_size = gdp->pdata_size;
> + dev->per_res = gdp->per_res;
> + dev->num_per_res = gdp->num_per_res;
> +
> + return 0;
> +}
> +
> +static __devinit
> +struct platform_device *gpmc_create_device(struct gpmc_device *p,
> + struct gpmc *gpmc)
> +{
> + int num = p->num_per_res + p->num_gpmc_res;
> + struct resource *res;
> + struct platform_device *pdev;
> +
> + res = devm_kzalloc(gpmc->dev, sizeof(struct resource) * num,
> + GFP_KERNEL);
> + if (!res) {
> + dev_err(gpmc->dev, "error: allocating memory\n");
> + return NULL;
> + }
> +
> + memcpy((char *)res, (const char *) p->gpmc_res,
> + sizeof(struct resource) * p->num_gpmc_res);
> + memcpy((char *)(res + p->num_gpmc_res), (const char *)p->per_res,
> + sizeof(struct resource) * p->num_per_res);
> +
> + pdev = platform_device_register_resndata(gpmc->dev, p->name, p->id,
> + res, num, p->pdata, p->pdata_size);
> +
> + devm_kfree(gpmc->dev, res);
> +
> + return pdev;
> }
> -postcore_initcall(gpmc_init);
>
> static irqreturn_t gpmc_handle_irq(int irq, void *dev)
> {
> - u8 cs;
> + int i;
> + u32 regval;
> + struct gpmc *gpmc = dev;
>
> - /* check cs to invoke the irq */
> - cs = ((gpmc_read_reg(GPMC_PREFETCH_CONFIG1))>> CS_NUM_SHIFT)& 0x7;
> - if (OMAP_GPMC_IRQ_BASE+cs<= OMAP_GPMC_IRQ_END)
> - generic_handle_irq(OMAP_GPMC_IRQ_BASE+cs);
> + regval = gpmc_read_reg(GPMC_IRQSTATUS);
> +
> +
> + for (i = 0; i< gpmc->num_irq; i++)
> + if (regval& gpmc->irq[i].regval)
> + generic_handle_irq(gpmc->irq[i].irq);
> + gpmc_write_reg(GPMC_IRQSTATUS, regval);
>
> return IRQ_HANDLED;
> }
>
> +static int gpmc_irq_endis(struct irq_data *p, bool endis)
> +{
> + struct gpmc *gpmc = irq_data_get_irq_chip_data(p);
> + int i;
> + u32 regval;
> +
> + for (i = 0; i< gpmc->num_irq; i++)
> + if (p->irq == gpmc->irq[i].irq) {
> + regval = gpmc_read_reg(GPMC_IRQENABLE);
> + if (endis)
> + regval |= gpmc->irq[i].regval;
> + else
> + regval&= ~gpmc->irq[i].regval;
> + gpmc_write_reg(GPMC_IRQENABLE, regval);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void gpmc_irq_disable(struct irq_data *p)
> +{
> + gpmc_irq_endis(p, false);
> +}
> +
> +static void gpmc_irq_enable(struct irq_data *p)
> +{
> + gpmc_irq_endis(p, true);
> +}
> +
> +static void gpmc_irq_noop(struct irq_data *data) { }
> +
> +static unsigned int gpmc_irq_noop_ret(struct irq_data *data) { return 0; }
> +
> +static __devinit int gpmc_setup_irq(struct gpmc *gpmc)
> +{
> + int i;
> + u32 regval;
> +
> + if (!gpmc->master_irq)
> + return -EINVAL;
> +
> + 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 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?
> + gpmc->irq[0].regval = GPMC_IRQ_FIFOEVENT;
> + gpmc->irq[1].regval = GPMC_IRQ_TERMINALCOUNT;
> + gpmc->irq[2].regval = GPMC_IRQ_WAIT0EDGEDETECTION;
> + gpmc->irq[3].regval = GPMC_IRQ_WAIT1EDGEDETECTION;
> + gpmc->irq[4].regval = GPMC_IRQ_WAIT2EDGEDETECTION;
> + gpmc->irq[5].regval = GPMC_IRQ_WAIT3EDGEDETECTION;
> +
> + for (i = 0; i< gpmc->num_irq; i++)
> + gpmc->irq[i].irq = gpmc->irq_start + i;
> +
> + gpmc->irq_chip.name = "gpmc";
> + gpmc->irq_chip.irq_startup = gpmc_irq_noop_ret;
> + gpmc->irq_chip.irq_enable = gpmc_irq_enable;
> + gpmc->irq_chip.irq_disable = gpmc_irq_disable;
> + gpmc->irq_chip.irq_shutdown = gpmc_irq_noop;
> + gpmc->irq_chip.irq_ack = gpmc_irq_noop;
> + gpmc->irq_chip.irq_mask = gpmc_irq_noop;
> + gpmc->irq_chip.irq_unmask = gpmc_irq_noop;
> +
> + for (i = 0; i< gpmc->num_irq; i++) {
> + irq_set_chip_and_handler(gpmc->irq[i].irq,
> + &gpmc->irq_chip, handle_simple_irq);
> + irq_set_chip_data(gpmc->irq[i].irq, gpmc);
> + set_irq_flags(gpmc->irq[i].irq, IRQF_VALID | IRQF_NOAUTOEN);
> + }
> +
> + /* clear interrupts */
> + regval = gpmc_read_reg(GPMC_IRQSTATUS);
> + gpmc_write_reg(GPMC_IRQSTATUS, regval);
> +
> + return request_irq(gpmc->master_irq, gpmc_handle_irq, IRQF_SHARED,
> + "gpmc", gpmc);
> +}
> +
> +static __devinit int gpmc_probe(struct platform_device *pdev)
> +{
> + u32 l;
> + int i;
> + int ret = 0;
> + struct resource *res = NULL;
> + struct gpmc_pdata *gp = dev_get_platdata(&pdev->dev);
> + struct gpmc_device_pdata **gdq = NULL;
> + struct gpmc_device *gd = NULL;
> +
> + gpmc = devm_kzalloc(&pdev->dev, sizeof(struct gpmc), GFP_KERNEL);
> + if (gpmc == NULL)
> + return -ENOMEM;
> +
> + gpmc->dev =&pdev->dev;
> + gpmc->fclk_period = gp->fclk_period;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL)
> + return -ENOENT;
> +
> + gpmc->phys_base = res->start;
> + gpmc->memsize = resource_size(res);
> +
> + gpmc->io_base = devm_request_and_ioremap(gpmc->dev, res);
> + if (!gpmc->io_base)
> + return -EADDRNOTAVAIL;
> +
> + 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.
> + gpmc->irq_start = gp->irq_start;
> + gpmc->num_irq = gp->num_irq;
> + gpmc_setup_irq(gpmc);
> +
> + gpmc->ecc_used = -EINVAL;
> + spin_lock_init(&gpmc->mem_lock);
> + platform_set_drvdata(pdev, gpmc);
> +
> + l = gpmc_read_reg(GPMC_REVISION);
> + dev_info(gpmc->dev, "GPMC revision %d.%d\n", (l>> 4)& 0x0f, l& 0x0f);
> +
> + gpmc_mem_init();
> +
> + 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?
The above loop appears to terminate when *gdq == 0. Is this always
guaranteed? In other words, safe?
I see that "i" is not being used in the above loop either.
> + gpmc->num_device = gd - gpmc->device;
> +
> + for (i = 0, gd = gpmc->device; i< gpmc->num_device; i++, gd++)
> + if (IS_ERR(gpmc_create_device(gd, gpmc)))
> + dev_err(gpmc->dev, "device creation on %s failed\n",
> + gd->name);
> +
> + return 0;
> +}
> +
> +static __devexit int gpmc_free_irq(struct gpmc *gpmc)
> +{
> + /* TODO: free gpmc irq chip */
> +
> + if (gpmc->master_irq)
> + free_irq(gpmc->master_irq, gpmc);
> +
> + return 0;
> +}
> +
> +static __devexit int gpmc_remove(struct platform_device *pdev)
> +{
> + struct gpmc *gpmc = platform_get_drvdata(pdev);
> +
> + platform_set_drvdata(pdev, NULL);
> + gpmc_free_irq(gpmc);
> +
> + return 0;
> +}
> +
> +static struct platform_driver gpmc_driver = {
> + .probe = gpmc_probe,
> + .remove = __devexit_p(gpmc_remove),
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(gpmc_driver);
> +
> #ifdef CONFIG_ARCH_OMAP3
> static struct omap3_gpmc_regs gpmc_context;
>
> @@ -855,10 +1193,10 @@ int gpmc_enable_hwecc(int cs, int mode, int dev_width, int ecc_size)
> unsigned int val;
>
> /* check if ecc module is in used */
> - if (gpmc_ecc_used != -EINVAL)
> + if (gpmc->ecc_used != -EINVAL)
> return -EINVAL;
>
> - gpmc_ecc_used = cs;
> + gpmc->ecc_used = cs;
>
> /* clear ecc and enable bits */
> val = ((0x00000001<<8) | 0x00000001);
> @@ -906,7 +1244,7 @@ int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
> {
> unsigned int val = 0x0;
>
> - if (gpmc_ecc_used != cs)
> + if (gpmc->ecc_used != cs)
> return -EINVAL;
>
> /* read ecc result */
> @@ -916,7 +1254,7 @@ int gpmc_calculate_ecc(int cs, const u_char *dat, u_char *ecc_code)
> /* P2048o, P1024o, P512o, P256o, P2048e, P1024e, P512e, P256e */
> *ecc_code++ = ((val>> 8)& 0x0f) | ((val>> 20)& 0xf0);
>
> - gpmc_ecc_used = -EINVAL;
> + gpmc->ecc_used = -EINVAL;
> return 0;
> }
> EXPORT_SYMBOL_GPL(gpmc_calculate_ecc);
> diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
> index 1527929..fa62cdf 100644
> --- a/arch/arm/plat-omap/include/plat/gpmc.h
> +++ b/arch/arm/plat-omap/include/plat/gpmc.h
> @@ -36,6 +36,7 @@
> #define GPMC_PREFETCH_FIFO_CNT 0x00000007 /* bytes available in FIFO for r/w */
> #define GPMC_PREFETCH_COUNT 0x00000008 /* remaining bytes to be read/write*/
> #define GPMC_STATUS_BUFFER 0x00000009 /* 1: buffer is available to write */
> +#define GPMC_CONFIG_WAITPIN 0x0000000A
>
> #define GPMC_NAND_COMMAND 0x0000000a
> #define GPMC_NAND_ADDRESS 0x0000000b
> @@ -83,6 +84,17 @@
> #define GPMC_IRQ_FIFOEVENTENABLE 0x01
> #define GPMC_IRQ_COUNT_EVENT 0x02
>
> +#define GPMC_IRQ_FIFOEVENT BIT(0)
> +#define GPMC_IRQ_TERMINALCOUNT BIT(1)
> +#define GPMC_IRQ_WAIT0EDGEDETECTION BIT(8)
> +#define GPMC_IRQ_WAIT1EDGEDETECTION BIT(9)
> +#define GPMC_IRQ_WAIT2EDGEDETECTION BIT(10)
> +#define GPMC_IRQ_WAIT3EDGEDETECTION BIT(11)
> +#define GPMC_IRQ_MASK \
> + (GPMC_IRQ_FIFOEVENT | GPMC_IRQ_TERMINALCOUNT | \
> + GPMC_IRQ_WAIT0EDGEDETECTION | GPMC_IRQ_WAIT1EDGEDETECTION | \
> + GPMC_IRQ_WAIT2EDGEDETECTION | GPMC_IRQ_WAIT3EDGEDETECTION)
> +
> #define PREFETCH_FIFOTHRESHOLD_MAX 0x40
> #define PREFETCH_FIFOTHRESHOLD(val) ((val)<< 8)
>
> @@ -131,6 +143,42 @@ struct gpmc_timings {
> u16 wr_data_mux_bus; /* WRDATAONADMUXBUS */
> };
>
> +struct gpmc_config {
> + int cmd;
> + int val;
> +};
> +
> +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".
> +};
> +
> +struct gpmc_device_pdata {
> + char *name;
> + int id;
> + void *pdata;
> + unsigned pdata_size;
> + /* resources configured via GPMC will be created by GPMC driver */
> + struct resource *per_res;
> + unsigned num_per_res;
> + struct gpmc_cs_data *cs_data;
> + unsigned num_cs;
> +};
> +
> +struct gpmc_pdata {
> + /* GPMC_FCLK period in picoseconds */
> + unsigned long fclk_period;
> + struct gpmc_device_pdata **device_pdata;
> + unsigned irq_start;
> + unsigned num_irq;
Again, not sure why we need to pass num_irq.
Cheers
Jon
More information about the linux-mtd
mailing list