On 03/08/2013 03:59 PM, Arun Kumar K wrote:
This driver is for the FIMC-IS IP available in Samsung Exynos5
SoC onwards. This patch adds the core files for the new driver.
Signed-off-by: Arun Kumar K<arun.kk@xxxxxxxxxxx>
Signed-off-by: Kilyeon Im<kilyeon.im@xxxxxxxxxxx>
---
drivers/media/platform/exynos5-is/fimc-is-core.c | 421 ++++++++++++++++++++++
drivers/media/platform/exynos5-is/fimc-is-core.h | 140 +++++++
2 files changed, 561 insertions(+)
[...]
+static int __devinit fimc_is_probe(struct platform_device *pdev)
You need to remove this attribute, that's not supported in recent kernels
any more.
+{
+ struct device *dev =&pdev->dev;
+ struct fimc_is_platdata *pdata;
+ struct resource *res;
+ struct fimc_is *is;
+ struct pinctrl *pctrl;
+ void __iomem *regs;
+ int irq, ret;
+
+ pr_debug("FIMC-IS Probe Enter\n");
+
+ pctrl = devm_pinctrl_get_select_default(dev);
+ if (IS_ERR(pctrl)) {
+ dev_err(dev, "Pinctrl configuration failed\n");
+ return -EINVAL;
+ }
This is not needed any more. If you work with not latest kernel I suggest
to cherry pick
commit ab78029ecc347debbd737f06688d788bd9d60c1d
drivers/pinctrl: grab default handles from device core
and remove those devm_pinctrl_get_select_default() calls from drivers.
+ if (!pdev->dev.of_node) {
+ dev_err(dev, "Null platform data\n");
Huh ? Since this driver is for dt-only platforms, is there a need to check
pdev->dev.of_node at all ? Probably you want to just return -ENODEV if it
is NULL.
+ return -EINVAL;
+ }
+
+ pdata = fimc_is_parse_dt(dev);
+ if (!pdata) {
+ dev_err(dev, "Parse DT failed\n");
+ return -EINVAL;
+ }
+
+ is = devm_kzalloc(&pdev->dev, sizeof(*is), GFP_KERNEL);
+ if (!is)
+ return -ENOMEM;
+
+ is->pdev = pdev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ regs = devm_request_and_ioremap(dev, res);
+ if (regs == NULL) {
+ dev_err(dev, "Failed to obtain io memory\n");
+ return -ENOENT;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq< 0) {
+ dev_err(dev, "Failed to get IRQ\n");
+ return irq;
+ }
+
+ ret = fimc_is_clk_cfg(is);
+ if (ret< 0) {
+ dev_err(dev, "Clock config failed\n");
+ goto err_clk;
+ }
+
+ platform_set_drvdata(pdev, is);
+ pm_runtime_enable(dev);
+
+ ret = pm_runtime_get_sync(dev);
+ if (ret< 0)
+ goto err_clk;
+
+ is->alloc_ctx = vb2_dma_contig_init_ctx(dev);
+ if (IS_ERR(is->alloc_ctx)) {
+ ret = PTR_ERR(is->alloc_ctx);
+ goto err_pm;
+ }
+
+ /* Create sensor subdevs */
+ is->pdata = pdata;
+ ret = fimc_is_create_sensor_subdevs(is);
+ if (ret< 0)
+ goto err_sensor_sd;
+
+ /* Init FIMC Pipeline */
+ ret = fimc_is_pipeline_init(&is->pipeline, 0, is);
+ if (ret< 0)
+ goto err_sd;
+
+ /* Init FIMC Interface */
+ ret = fimc_is_interface_init(&is->interface, regs, irq);
+ if (ret< 0)
+ goto err_sd;
+
+ dev_dbg(dev, "FIMC-IS registered successfully\n");
Shouldn't there be pm_runtime_put() ?
+
+ return 0;
+
+err_sd:
+ fimc_is_pipeline_destroy(&is->pipeline);
+err_sensor_sd:
+ fimc_is_unregister_sensor_subdevs(is);
+err_vb:
+ vb2_dma_contig_cleanup_ctx(is->alloc_ctx);
+err_pm:
+ pm_runtime_put(dev);
+err_clk:
+ fimc_is_clk_put(is);
+
+ return ret;
+}
+
+int fimc_is_clk_enable(struct fimc_is *is)
+{
+ clk_enable(is->clock[IS_CLK_GATE0]);
+ clk_enable(is->clock[IS_CLK_GATE1]);
No need to check return value ?
+ return 0;
+}
+
+void fimc_is_clk_disable(struct fimc_is *is)
+{
+ clk_disable(is->clock[IS_CLK_GATE0]);
+ clk_disable(is->clock[IS_CLK_GATE1]);
+}
+
+static int fimc_is_pm_resume(struct device *dev)
+{
+ struct fimc_is *is = dev_get_drvdata(dev);
+ int ret;
+
+ ret = fimc_is_clk_enable(is);
+ if (ret< 0)
+ dev_err(dev, "Could not enable clocks\n");
+
+ return 0;
+}
+
+static int fimc_is_pm_suspend(struct device *dev)
+{
+ struct fimc_is *is = dev_get_drvdata(dev);
+
+ fimc_is_clk_disable(is);
+ return 0;
+}
+
+static int fimc_is_runtime_resume(struct device *dev)
+{
+ return fimc_is_pm_resume(dev);
+}
+
+static int fimc_is_runtime_suspend(struct device *dev)
+{
+ return fimc_is_pm_suspend(dev);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int fimc_is_resume(struct device *dev)
+{
+ return fimc_is_pm_resume(dev);
+}
+
+static int fimc_is_suspend(struct device *dev)
+{
+ return fimc_is_pm_suspend(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static int fimc_is_remove(struct platform_device *pdev)
+{
+ struct fimc_is *is = platform_get_drvdata(pdev);
+ struct device *dev =&pdev->dev;
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ fimc_is_pipeline_destroy(&is->pipeline);
+ fimc_is_unregister_sensor_subdevs(is);
+ vb2_dma_contig_cleanup_ctx(is->alloc_ctx);
+ fimc_is_clk_put(is);
+
+ return 0;
+}
+
+static struct platform_device_id fimc_is_driver_ids[] = {
+ {
+ .name = "exynos5-fimc-is",
+ .driver_data = 0,
This line doesn't change anything, I would just remove it.
But is this fimc_is_driver_ids[] array needed at all ?
+ },
+};
+MODULE_DEVICE_TABLE(platform, fimc_is_driver_ids);
+
+static const struct dev_pm_ops fimc_is_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(fimc_is_suspend, fimc_is_resume)
+ SET_RUNTIME_PM_OPS(fimc_is_runtime_suspend, fimc_is_runtime_resume,
+ NULL)
+};
+
+static struct platform_driver fimc_is_driver = {
+ .probe = fimc_is_probe,
+ .remove = fimc_is_remove,
+ .id_table = fimc_is_driver_ids,
+ .driver = {
+ .name = FIMC_IS_DRV_NAME,
+ .owner = THIS_MODULE,
+ .pm =&fimc_is_pm_ops,
How is this driver instantiated from the device tree when there
is no of_match_table ? I didn't find any chunk adding it further
in this series.
+ }
+};
+module_platform_driver(fimc_is_driver);
I forgot to say that in general this patch series looks very clean
to me. I'm really happy to see this driver in such a good shape.
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
