Messages in this thread | | | Date | Wed, 7 Nov 2012 10:39:21 -0500 | From | Murali Karicheri <> | Subject | Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver |
| |
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 > >
| |