Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platformdriver

[Murali Karicheri]

Santhosh,

Thanks for the review. Some of the comments below applies to the 
original code as this driver is moved from mach-davinci to memory. I 
will fix them though. See below my response.
> > +
> > +    curr_cs_data = get_cs_data(chip_cs);
> > +    if (curr_cs_data) {
> > +        /* for configuration we use cs since it is used to index ACR */
> > +        ret = davinci_aemif_config_abus(chip_cs, aemif->base, data);
> > +        if (!ret) {
> > +            *curr_cs_data = *data;
> > +            return 0;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL(davinci_aemif_set_abus_params);
> > +
> Who is the user of this EXPORT ?
Mostly not needed, but there is a use case for connecting FPGA that 
needs to do the configuration dynamically I guess. Want to hear from 
others if this is needed? Same for below comment. May be we can remove 
this now and add it later if needed.
> > +/**
> > + * davinci_aemif_get_abus_params - Get bus configuration data for a 
> > given cs
> > + * @cs: chip-select, values 2-5
> > + * returns: ptr to a struct having the current configuration data
> > + */
> > +struct davinci_aemif_cs_data *davinci_aemif_get_abus_params(unsigned 
> > int cs)
> > +{
> > +    /* translate to chip CS which starts at 2 */
> > +    return get_cs_data(cs + 2);
> > +}
> > +EXPORT_SYMBOL(davinci_aemif_get_abus_params);
> > +
> This one too.
>
> [...]
>
> > +static int __devinit davinci_aemif_probe(struct platform_device *pdev)
> > +{
> > +    struct davinci_aemif_pdata *cfg;
> > +    int ret  = -ENODEV, i;
> > +    struct resource *res;
> > +
> > +    aemif = devm_kzalloc(&pdev->dev, sizeof(*aemif), GFP_KERNEL);
> > +
> > +    if (!aemif)
> > +        return -ENOMEM;
> > +
> > +    aemif->clk = clk_get(NULL, "aemif");
> Please use dev attributes. Above usage of clk_get isn't recommonded
> in drivers. You might have to add alias entries in your clock data for
> it to work though.
Need to investigate.
> > +    if (IS_ERR(aemif->clk))
> > +        return PTR_ERR(aemif->clk);
> > +
> > +    clk_prepare_enable(aemif->clk);
> > +    aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
> > +
> /1000 for what ? Converting it into KHz ?
Yes. WIll add a comment.
> > +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +    if (!res) {
> > +        pr_err("No IO memory address defined\n");
> > +        goto error;
> > +    }
> > +
> > +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +    aemif->base = devm_request_and_ioremap(&pdev->dev, res);
> > +    if (!aemif->base) {
> > +        ret = -EBUSY;
> > +        pr_err("ioremap failed\n");
> > +        goto error;
> > +    }
> > +
> > +    if (pdev->dev.platform_data == NULL) {
> > +        /* Not platform data, we get the cs data from the cs nodes */
> > +        cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL);
> > +        if (cfg == NULL)
> > +            return -ENOMEM;
> > +
> > +        aemif->cfg = cfg;
> > +        if (of_davinci_aemif_cs_init(pdev->dev.of_node) < 0) {
> > +            pr_err("No platform data or cs of node present\n");
> > +            goto error;
> > +        }
> > +    } else {
> > +        cfg = pdev->dev.platform_data;
> > +        aemif->cfg = cfg;
> > +    }
> > +
> > +    for (i = 0; i < cfg->num_cs; i++) {
> > +        /* cs is from 2-5. Internally we use cs-2 to access ACR */
> > +        ret = davinci_aemif_config_abus(cfg->cs_data[i].cs - 2,
> > +                aemif->base, &cfg->cs_data[i]);
> > +        if (ret < 0) {
> > +            pr_err("Error configuring chip select %d\n",
> > +                cfg->cs_data[i].cs);
> > +            goto error;
> > +        }
> > +    }
> > +    return 0;
> > +error:
> > +    clk_disable_unprepare(aemif->clk);
> > +    clk_put(aemif->clk);
> Alos unmap 'aemif->base' here..
I thought devm_ API do this automatically. I will check.
> > +    return ret;
> > +}
> > +
> > +static struct platform_driver davinci_aemif_driver = {
> > +    .probe = davinci_aemif_probe,
> > +    .driver = {
> > +        .name = DRV_NAME,
> > +        .owner = THIS_MODULE,
> > +        .of_match_table = davinci_aemif_of_match,
> > +    },
> > +};
> > +
> > +static int __init davinci_aemif_init(void)
> > +{
> > +    return platform_driver_register(&davinci_aemif_driver);
> > +}
> > +subsys_initcall(davinci_aemif_init);
> > +
> > +static void __exit davinci_aemif_exit(void)
> > +{
> > +    clk_disable_unprepare(aemif->clk);
> > +    clk_put(aemif->clk);
> > +    platform_driver_unregister(&davinci_aemif_driver);
> > +}
> > +module_exit(davinci_aemif_exit);
> > +
> > +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@xxxxxx>");
> > +MODULE_DESCRIPTION("Texas Instruments AEMIF driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" DRV_NAME);
> > diff --git a/include/linux/platform_data/davinci-aemif.h 
> > b/include/linux/platform_data/davinci-aemif.h
> > new file mode 100644
> > index 0000000..03f3ad0
> > --- /dev/null
> > +++ b/include/linux/platform_data/davinci-aemif.h
> > @@ -0,0 +1,47 @@
> > +/*
> > + * TI DaVinci AEMIF support
> > + *
> > + * Copyright 2010 (C) Texas Instruments, Inc. http://www.ti.com/
> > + *
> s/2010/2012
> > + * This file is licensed under the terms of the GNU General Public 
> > License
> > + * version 2. This program is licensed "as is" without any warranty 
> > of any
> > + * kind, whether express or implied.
> > + */
> > +#ifndef _MACH_DAVINCI_AEMIF_H
> > +#define _MACH_DAVINCI_AEMIF_H
> Fix the header guard as per new directory
Ok.
> Regards
> Santosh

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