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

[Murali Karicheri <m-karicheri2@ti.com>]

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@ti.com>");
>> +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
>
>