[v0,1/2] V4L - vpfe capture - convert ccdc drivers to platform drivers
Commit Message
From: Muralidharan Karicheri <m-karicheri2@ti.com>
Current implementation defines ccdc memory map in vpfe capture platform
file and update the same in ccdc through a function call. This will not
scale well. For example for DM365 CCDC, there are are additional memory
map for Linear table. So it is cleaner to define memory map for ccdc
driver in the platform file and read it by the ccdc platform driver during
probe.
Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
---
Applies to V4L-DVB linux-next tree
drivers/media/video/davinci/dm355_ccdc.c | 89 ++++++++++++++++++++++++----
drivers/media/video/davinci/dm644x_ccdc.c | 78 ++++++++++++++++++++----
drivers/media/video/davinci/vpfe_capture.c | 49 +--------------
3 files changed, 145 insertions(+), 71 deletions(-)
Comments
> -----Original Message-----
> From: Karicheri, Muralidharan
> Sent: Tuesday, December 01, 2009 11:46 PM
> To: linux-media@vger.kernel.org; hverkuil@xs4all.nl;
> khilman@deeprootsystems.com
> Cc: davinci-linux-open-source@linux.davincidsp.com; Hiremath,
> Vaibhav; Karicheri, Muralidharan
> Subject: [PATCH v0 1/2] V4L - vpfe capture - convert ccdc drivers to
> platform drivers
>
> From: Muralidharan Karicheri <m-karicheri2@ti.com>
>
> Current implementation defines ccdc memory map in vpfe capture
> platform
> file and update the same in ccdc through a function call. This will
> not
> scale well. For example for DM365 CCDC, there are are additional
> memory
> map for Linear table. So it is cleaner to define memory map for ccdc
> driver in the platform file and read it by the ccdc platform driver
> during
> probe.
>
> Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
> ---
> Applies to V4L-DVB linux-next tree
> drivers/media/video/davinci/dm355_ccdc.c | 89
> ++++++++++++++++++++++++----
> drivers/media/video/davinci/dm644x_ccdc.c | 78
> ++++++++++++++++++++----
> drivers/media/video/davinci/vpfe_capture.c | 49 +--------------
> 3 files changed, 145 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/media/video/davinci/dm355_ccdc.c
> b/drivers/media/video/davinci/dm355_ccdc.c
> index 56fbefe..aacb95f 100644
> --- a/drivers/media/video/davinci/dm355_ccdc.c
> +++ b/drivers/media/video/davinci/dm355_ccdc.c
> @@ -37,6 +37,7 @@
> #include <linux/platform_device.h>
> #include <linux/uaccess.h>
> #include <linux/videodev2.h>
> +#include <mach/mux.h>
[Hiremath, Vaibhav] This should not be here, this should get handled in board file. The driver should be generic.
> #include <media/davinci/dm355_ccdc.h>
> #include <media/davinci/vpss.h>
> #include "dm355_ccdc_regs.h"
> @@ -105,7 +106,6 @@ static struct ccdc_params_ycbcr
> ccdc_hw_params_ycbcr = {
>
> static enum vpfe_hw_if_type ccdc_if_type;
> static void *__iomem ccdc_base_addr;
> -static int ccdc_addr_size;
>
> /* Raw Bayer formats */
> static u32 ccdc_raw_bayer_pix_formats[] =
> @@ -126,12 +126,6 @@ static inline void regw(u32 val, u32 offset)
> __raw_writel(val, ccdc_base_addr + offset);
> }
>
> -static void ccdc_set_ccdc_base(void *addr, int size)
> -{
> - ccdc_base_addr = addr;
> - ccdc_addr_size = size;
> -}
> -
> static void ccdc_enable(int en)
> {
> unsigned int temp;
> @@ -938,7 +932,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
> .hw_ops = {
> .open = ccdc_open,
> .close = ccdc_close,
> - .set_ccdc_base = ccdc_set_ccdc_base,
> .enable = ccdc_enable,
> .enable_out_to_sdram = ccdc_enable_output_to_sdram,
> .set_hw_if_params = ccdc_set_hw_if_params,
> @@ -959,19 +952,89 @@ static struct ccdc_hw_device ccdc_hw_dev = {
> },
> };
>
> -static int __init dm355_ccdc_init(void)
> +static int __init dm355_ccdc_probe(struct platform_device *pdev)
> {
> - printk(KERN_NOTICE "dm355_ccdc_init\n");
> - if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
> - return -1;
> + static resource_size_t res_len;
> + struct resource *res;
> + int status = 0;
> +
> + /**
> + * first try to register with vpfe. If not correct platform,
> then we
> + * don't have to iomap
> + */
> + status = vpfe_register_ccdc_device(&ccdc_hw_dev);
> + if (status < 0)
> + return status;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + status = -ENOENT;
[Hiremath, Vaibhav] I think right return value is -ENODEV.
> + goto fail_nores;
> + }
> + res_len = res->end - res->start + 1;
> +
> + res = request_mem_region(res->start, res_len, res->name);
[Hiremath, Vaibhav] You should use "resource_size" here for res_len here.
> + if (!res) {
> + status = -EBUSY;
> + goto fail_nores;
> + }
> +
> + ccdc_base_addr = ioremap_nocache(res->start, res_len);
> + if (!ccdc_base_addr) {
> + status = -EBUSY;
[Hiremath, Vaibhav] Is -EBUSY right return value, I think it should be -ENXIO or -ENOMEM.
> + goto fail;
> + }
> + /**
> + * setup Mux configuration for vpfe input and register
> + * vpfe capture platform device
> + */
> + davinci_cfg_reg(DM355_VIN_PCLK);
> + davinci_cfg_reg(DM355_VIN_CAM_WEN);
> + davinci_cfg_reg(DM355_VIN_CAM_VD);
> + davinci_cfg_reg(DM355_VIN_CAM_HD);
> + davinci_cfg_reg(DM355_VIN_YIN_EN);
> + davinci_cfg_reg(DM355_VIN_CINL_EN);
> + davinci_cfg_reg(DM355_VIN_CINH_EN);
> +
[Hiremath, Vaibhav] This should not be here, this code must be generic and might get used in another SoC.
> printk(KERN_NOTICE "%s is registered with vpfe.\n",
> ccdc_hw_dev.name);
> return 0;
> +fail:
> + release_mem_region(res->start, res_len);
> +fail_nores:
> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
> + return status;
> }
>
> -static void __exit dm355_ccdc_exit(void)
> +static int dm355_ccdc_remove(struct platform_device *pdev)
> {
> + struct resource *res;
> +
> + iounmap(ccdc_base_addr);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res)
> + release_mem_region(res->start, res->end - res->start +
> 1);
[Hiremath, Vaibhav] Please use "resource_size" here for size.
> vpfe_unregister_ccdc_device(&ccdc_hw_dev);
> + return 0;
> +}
> +
> +static struct platform_driver dm355_ccdc_driver = {
> + .driver = {
> + .name = "dm355_ccdc",
> + .owner = THIS_MODULE,
> + },
> + .remove = __devexit_p(dm355_ccdc_remove),
> + .probe = dm355_ccdc_probe,
> +};
> +
> +static int __init dm355_ccdc_init(void)
> +{
> + return platform_driver_register(&dm355_ccdc_driver);
> +}
> +
> +static void __exit dm355_ccdc_exit(void)
> +{
> + platform_driver_unregister(&dm355_ccdc_driver);
> }
>
> module_init(dm355_ccdc_init);
> diff --git a/drivers/media/video/davinci/dm644x_ccdc.c
> b/drivers/media/video/davinci/dm644x_ccdc.c
> index d5fa193..89ea6ae 100644
> --- a/drivers/media/video/davinci/dm644x_ccdc.c
> +++ b/drivers/media/video/davinci/dm644x_ccdc.c
> @@ -85,7 +85,6 @@ static u32 ccdc_raw_yuv_pix_formats[] =
> {V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_YUYV};
>
> static void *__iomem ccdc_base_addr;
> -static int ccdc_addr_size;
> static enum vpfe_hw_if_type ccdc_if_type;
>
> /* register access routines */
> @@ -99,12 +98,6 @@ static inline void regw(u32 val, u32 offset)
> __raw_writel(val, ccdc_base_addr + offset);
> }
>
> -static void ccdc_set_ccdc_base(void *addr, int size)
> -{
> - ccdc_base_addr = addr;
> - ccdc_addr_size = size;
> -}
> -
> static void ccdc_enable(int flag)
> {
> regw(flag, CCDC_PCR);
> @@ -838,7 +831,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
> .hw_ops = {
> .open = ccdc_open,
> .close = ccdc_close,
> - .set_ccdc_base = ccdc_set_ccdc_base,
> .reset = ccdc_sbl_reset,
> .enable = ccdc_enable,
> .set_hw_if_params = ccdc_set_hw_if_params,
> @@ -859,19 +851,79 @@ static struct ccdc_hw_device ccdc_hw_dev = {
> },
> };
>
> -static int __init dm644x_ccdc_init(void)
> +static int __init dm644x_ccdc_probe(struct platform_device *pdev)
> {
> - printk(KERN_NOTICE "dm644x_ccdc_init\n");
> - if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
> - return -1;
> + static resource_size_t res_len;
> + struct resource *res;
> + int status = 0;
> +
> + /**
> + * first try to register with vpfe. If not correct platform,
> then we
> + * don't have to iomap
> + */
> + status = vpfe_register_ccdc_device(&ccdc_hw_dev);
> + if (status < 0)
> + return status;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + status = -ENOENT;
> + goto fail_nores;
> + }
> +
> + res_len = res->end - res->start + 1;
> +
> + res = request_mem_region(res->start, res_len, res->name);
> + if (!res) {
> + status = -EBUSY;
> + goto fail_nores;
> + }
> +
> + ccdc_base_addr = ioremap_nocache(res->start, res_len);
> + if (!ccdc_base_addr) {
> + status = -EBUSY;
> + goto fail;
> + }
> +
> printk(KERN_NOTICE "%s is registered with vpfe.\n",
> ccdc_hw_dev.name);
> return 0;
> +fail:
> + release_mem_region(res->start, res_len);
> +fail_nores:
> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
> + return status;
> +}
> +
> +static int dm644x_ccdc_remove(struct platform_device *pdev)
> +{
> + struct resource *res;
> +
> + iounmap(ccdc_base_addr);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res)
> + release_mem_region(res->start, res->end - res->start +
> 1);
> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
> + return 0;
> +}
> +
> +static struct platform_driver dm644x_ccdc_driver = {
> + .driver = {
> + .name = "dm644x_ccdc",
> + .owner = THIS_MODULE,
> + },
> + .remove = __devexit_p(dm644x_ccdc_remove),
> + .probe = dm644x_ccdc_probe,
> +};
> +
> +static int __init dm644x_ccdc_init(void)
> +{
> + return platform_driver_register(&dm644x_ccdc_driver);
> }
>
> static void __exit dm644x_ccdc_exit(void)
> {
> - vpfe_unregister_ccdc_device(&ccdc_hw_dev);
> + platform_driver_unregister(&dm644x_ccdc_driver);
> }
[Hiremath, Vaibhav] All above comments mentioned for DM355 applicable here too.
Thanks,
Vaibhav
>
> module_init(dm644x_ccdc_init);
> diff --git a/drivers/media/video/davinci/vpfe_capture.c
> b/drivers/media/video/davinci/vpfe_capture.c
> index c3468ee..35bbb08 100644
> --- a/drivers/media/video/davinci/vpfe_capture.c
> +++ b/drivers/media/video/davinci/vpfe_capture.c
> @@ -108,9 +108,6 @@ struct ccdc_config {
> int vpfe_probed;
> /* name of ccdc device */
> char name[32];
> - /* for storing mem maps for CCDC */
> - int ccdc_addr_size;
> - void *__iomem ccdc_addr;
> };
>
> /* data structures */
> @@ -230,7 +227,6 @@ int vpfe_register_ccdc_device(struct
> ccdc_hw_device *dev)
> BUG_ON(!dev->hw_ops.set_image_window);
> BUG_ON(!dev->hw_ops.get_image_window);
> BUG_ON(!dev->hw_ops.get_line_length);
> - BUG_ON(!dev->hw_ops.setfbaddr);
> BUG_ON(!dev->hw_ops.getfid);
>
> mutex_lock(&ccdc_lock);
> @@ -241,25 +237,23 @@ int vpfe_register_ccdc_device(struct
> ccdc_hw_device *dev)
> * walk through it during vpfe probe
> */
> printk(KERN_ERR "vpfe capture not initialized\n");
> - ret = -1;
> + ret = -EFAULT;
> goto unlock;
> }
>
> if (strcmp(dev->name, ccdc_cfg->name)) {
> /* ignore this ccdc */
> - ret = -1;
> + ret = -EINVAL;
> goto unlock;
> }
>
> if (ccdc_dev) {
> printk(KERN_ERR "ccdc already registered\n");
> - ret = -1;
> + ret = -EINVAL;
> goto unlock;
> }
>
> ccdc_dev = dev;
> - dev->hw_ops.set_ccdc_base(ccdc_cfg->ccdc_addr,
> - ccdc_cfg->ccdc_addr_size);
> unlock:
> mutex_unlock(&ccdc_lock);
> return ret;
> @@ -1947,37 +1941,12 @@ static __init int vpfe_probe(struct
> platform_device *pdev)
> }
> vpfe_dev->ccdc_irq1 = res1->start;
>
> - /* Get address base of CCDC */
> - res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res1) {
> - v4l2_err(pdev->dev.driver,
> - "Unable to get register address map\n");
> - ret = -ENOENT;
> - goto probe_disable_clock;
> - }
> -
> - ccdc_cfg->ccdc_addr_size = res1->end - res1->start + 1;
> - if (!request_mem_region(res1->start, ccdc_cfg->ccdc_addr_size,
> - pdev->dev.driver->name)) {
> - v4l2_err(pdev->dev.driver,
> - "Failed request_mem_region for ccdc base\n");
> - ret = -ENXIO;
> - goto probe_disable_clock;
> - }
> - ccdc_cfg->ccdc_addr = ioremap_nocache(res1->start,
> - ccdc_cfg->ccdc_addr_size);
> - if (!ccdc_cfg->ccdc_addr) {
> - v4l2_err(pdev->dev.driver, "Unable to ioremap ccdc
> addr\n");
> - ret = -ENXIO;
> - goto probe_out_release_mem1;
> - }
> -
> ret = request_irq(vpfe_dev->ccdc_irq0, vpfe_isr,
> IRQF_DISABLED,
> "vpfe_capture0", vpfe_dev);
>
> if (0 != ret) {
> v4l2_err(pdev->dev.driver, "Unable to request
> interrupt\n");
> - goto probe_out_unmap1;
> + goto probe_disable_clock;
> }
>
> /* Allocate memory for video device */
> @@ -2101,10 +2070,6 @@ probe_out_video_release:
> video_device_release(vpfe_dev->video_dev);
> probe_out_release_irq:
> free_irq(vpfe_dev->ccdc_irq0, vpfe_dev);
> -probe_out_unmap1:
> - iounmap(ccdc_cfg->ccdc_addr);
> -probe_out_release_mem1:
> - release_mem_region(res1->start, res1->end - res1->start + 1);
> probe_disable_clock:
> vpfe_disable_clock(vpfe_dev);
> mutex_unlock(&ccdc_lock);
> @@ -2120,7 +2085,6 @@ probe_free_dev_mem:
> static int vpfe_remove(struct platform_device *pdev)
> {
> struct vpfe_device *vpfe_dev = platform_get_drvdata(pdev);
> - struct resource *res;
>
> v4l2_info(pdev->dev.driver, "vpfe_remove\n");
>
> @@ -2128,11 +2092,6 @@ static int vpfe_remove(struct platform_device
> *pdev)
> kfree(vpfe_dev->sd);
> v4l2_device_unregister(&vpfe_dev->v4l2_dev);
> video_unregister_device(vpfe_dev->video_dev);
> - mutex_lock(&ccdc_lock);
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - release_mem_region(res->start, res->end - res->start + 1);
> - iounmap(ccdc_cfg->ccdc_addr);
> - mutex_unlock(&ccdc_lock);
> vpfe_disable_clock(vpfe_dev);
> kfree(vpfe_dev);
> kfree(ccdc_cfg);
> --
> 1.6.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
>> +#include <mach/mux.h>
>[Hiremath, Vaibhav] This should not be here, this should get handled in
>board file. The driver should be generic.
>
See my comments against the platform part of this patch.
>> #include <media/davinci/dm355_ccdc.h>
>> #include <media/davinci/vpss.h>
>> #include "dm355_ccdc_regs.h"
>> @@ -105,7 +106,6 @@ static struct ccdc_params_ycbcr
>> ccdc_hw_params_ycbcr = {
>>
>> static enum vpfe_hw_if_type ccdc_if_type;
>> static void *__iomem ccdc_base_addr;
>> -static int ccdc_addr_size;
>>
>> /* Raw Bayer formats */
>> static u32 ccdc_raw_bayer_pix_formats[] =
>> @@ -126,12 +126,6 @@ static inline void regw(u32 val, u32 offset)
>> __raw_writel(val, ccdc_base_addr + offset);
>> }
>>
>> -static void ccdc_set_ccdc_base(void *addr, int size)
>> -{
>> - ccdc_base_addr = addr;
>> - ccdc_addr_size = size;
>> -}
>> -
>> static void ccdc_enable(int en)
>> {
>> unsigned int temp;
>> @@ -938,7 +932,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
>> .hw_ops = {
>> .open = ccdc_open,
>> .close = ccdc_close,
>> - .set_ccdc_base = ccdc_set_ccdc_base,
>> .enable = ccdc_enable,
>> .enable_out_to_sdram = ccdc_enable_output_to_sdram,
>> .set_hw_if_params = ccdc_set_hw_if_params,
>> @@ -959,19 +952,89 @@ static struct ccdc_hw_device ccdc_hw_dev = {
>> },
>> };
>>
>> -static int __init dm355_ccdc_init(void)
>> +static int __init dm355_ccdc_probe(struct platform_device *pdev)
>> {
>> - printk(KERN_NOTICE "dm355_ccdc_init\n");
>> - if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
>> - return -1;
>> + static resource_size_t res_len;
>> + struct resource *res;
>> + int status = 0;
>> +
>> + /**
>> + * first try to register with vpfe. If not correct platform,
>> then we
>> + * don't have to iomap
>> + */
>> + status = vpfe_register_ccdc_device(&ccdc_hw_dev);
>> + if (status < 0)
>> + return status;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + status = -ENOENT;
>[Hiremath, Vaibhav] I think right return value is -ENODEV.
>
Right. I will change it.
>> + goto fail_nores;
>> + }
>> + res_len = res->end - res->start + 1;
>> +
>> + res = request_mem_region(res->start, res_len, res->name);
>[Hiremath, Vaibhav] You should use "resource_size" here for res_len here.
Ok. I didn't know about such a function :(
Will change res_len -> resource_size(res)
>
>> + if (!res) {
>> + status = -EBUSY;
>> + goto fail_nores;
>> + }
>> +
>> + ccdc_base_addr = ioremap_nocache(res->start, res_len);
>> + if (!ccdc_base_addr) {
>> + status = -EBUSY;
>[Hiremath, Vaibhav] Is -EBUSY right return value, I think it should be -
>ENXIO or -ENOMEM.
>
I see -ENXIO & -ENOMEM being used by drivers. -ENXIO stands for "No such device or address". ENOMEM stands for "Out of memory" . Since we are trying to map the address here, -ENXIO looks reasonable to me. Same if request_mem_region() fails.
>> + goto fail;
>> + }
>> + /**
>> + * setup Mux configuration for vpfe input and register
>> + * vpfe capture platform device
>> + */
>> + davinci_cfg_reg(DM355_VIN_PCLK);
>> + davinci_cfg_reg(DM355_VIN_CAM_WEN);
>> + davinci_cfg_reg(DM355_VIN_CAM_VD);
>> + davinci_cfg_reg(DM355_VIN_CAM_HD);
>> + davinci_cfg_reg(DM355_VIN_YIN_EN);
>> + davinci_cfg_reg(DM355_VIN_CINL_EN);
>> + davinci_cfg_reg(DM355_VIN_CINH_EN);
>> +
>[Hiremath, Vaibhav] This should not be here, this code must be generic and
>might get used in another SoC.
yes. See my suggestion against the architecture part. will be replaced
with a setup_pinmux() fuction from platform_data.
>
>> printk(KERN_NOTICE "%s is registered with vpfe.\n",
>> ccdc_hw_dev.name);
>> return 0;
>> +fail:
>> + release_mem_region(res->start, res_len);
>> +fail_nores:
>> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
>> + return status;
>> }
>>
>> -static void __exit dm355_ccdc_exit(void)
>> +static int dm355_ccdc_remove(struct platform_device *pdev)
>> {
>> + struct resource *res;
>> +
>> + iounmap(ccdc_base_addr);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res)
>> + release_mem_region(res->start, res->end - res->start +
>> 1);
>[Hiremath, Vaibhav] Please use "resource_size" here for size.
Ok.
>
>> vpfe_unregister_ccdc_device(&ccdc_hw_dev);
>> + return 0;
>> +}
>> +
>> +static struct platform_driver dm355_ccdc_driver = {
>> + .driver = {
>> + .name = "dm355_ccdc",
>> + .owner = THIS_MODULE,
>> + },
>> + .remove = __devexit_p(dm355_ccdc_remove),
>> + .probe = dm355_ccdc_probe,
>> +};
>> +
>> +static int __init dm355_ccdc_init(void)
>> +{
>> + return platform_driver_register(&dm355_ccdc_driver);
>> +}
>> +
>> +static void __exit dm355_ccdc_exit(void)
>> +{
>> + platform_driver_unregister(&dm355_ccdc_driver);
>> }
>>
>> module_init(dm355_ccdc_init);
>> diff --git a/drivers/media/video/davinci/dm644x_ccdc.c
>> b/drivers/media/video/davinci/dm644x_ccdc.c
>> index d5fa193..89ea6ae 100644
>> --- a/drivers/media/video/davinci/dm644x_ccdc.c
>> +++ b/drivers/media/video/davinci/dm644x_ccdc.c
>> @@ -85,7 +85,6 @@ static u32 ccdc_raw_yuv_pix_formats[] =
>> {V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_YUYV};
>>
>> static void *__iomem ccdc_base_addr;
>> -static int ccdc_addr_size;
>> static enum vpfe_hw_if_type ccdc_if_type;
>>
>> /* register access routines */
>> @@ -99,12 +98,6 @@ static inline void regw(u32 val, u32 offset)
>> __raw_writel(val, ccdc_base_addr + offset);
>> }
>>
>> -static void ccdc_set_ccdc_base(void *addr, int size)
>> -{
>> - ccdc_base_addr = addr;
>> - ccdc_addr_size = size;
>> -}
>> -
>> static void ccdc_enable(int flag)
>> {
>> regw(flag, CCDC_PCR);
>> @@ -838,7 +831,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
>> .hw_ops = {
>> .open = ccdc_open,
>> .close = ccdc_close,
>> - .set_ccdc_base = ccdc_set_ccdc_base,
>> .reset = ccdc_sbl_reset,
>> .enable = ccdc_enable,
>> .set_hw_if_params = ccdc_set_hw_if_params,
>> @@ -859,19 +851,79 @@ static struct ccdc_hw_device ccdc_hw_dev = {
>> },
>> };
>>
>> -static int __init dm644x_ccdc_init(void)
>> +static int __init dm644x_ccdc_probe(struct platform_device *pdev)
>> {
>> - printk(KERN_NOTICE "dm644x_ccdc_init\n");
>> - if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
>> - return -1;
>> + static resource_size_t res_len;
>> + struct resource *res;
>> + int status = 0;
>> +
>> + /**
>> + * first try to register with vpfe. If not correct platform,
>> then we
>> + * don't have to iomap
>> + */
>> + status = vpfe_register_ccdc_device(&ccdc_hw_dev);
>> + if (status < 0)
>> + return status;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + status = -ENOENT;
>> + goto fail_nores;
>> + }
>> +
>> + res_len = res->end - res->start + 1;
>> +
>> + res = request_mem_region(res->start, res_len, res->name);
>> + if (!res) {
>> + status = -EBUSY;
>> + goto fail_nores;
>> + }
>> +
>> + ccdc_base_addr = ioremap_nocache(res->start, res_len);
>> + if (!ccdc_base_addr) {
>> + status = -EBUSY;
>> + goto fail;
>> + }
>> +
>> printk(KERN_NOTICE "%s is registered with vpfe.\n",
>> ccdc_hw_dev.name);
>> return 0;
>> +fail:
>> + release_mem_region(res->start, res_len);
>> +fail_nores:
>> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
>> + return status;
>> +}
>> +
>> +static int dm644x_ccdc_remove(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> +
>> + iounmap(ccdc_base_addr);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res)
>> + release_mem_region(res->start, res->end - res->start +
>> 1);
>> + vpfe_unregister_ccdc_device(&ccdc_hw_dev);
>> + return 0;
>> +}
>> +
>> +static struct platform_driver dm644x_ccdc_driver = {
>> + .driver = {
>> + .name = "dm644x_ccdc",
>> + .owner = THIS_MODULE,
>> + },
>> + .remove = __devexit_p(dm644x_ccdc_remove),
>> + .probe = dm644x_ccdc_probe,
>> +};
>> +
>> +static int __init dm644x_ccdc_init(void)
>> +{
>> + return platform_driver_register(&dm644x_ccdc_driver);
>> }
>>
>> static void __exit dm644x_ccdc_exit(void)
>> {
>> - vpfe_unregister_ccdc_device(&ccdc_hw_dev);
>> + platform_driver_unregister(&dm644x_ccdc_driver);
>> }
>[Hiremath, Vaibhav] All above comments mentioned for DM355 applicable here
>too.
Ok.
>
>Thanks,
>Vaibhav
>
>>
>> module_init(dm644x_ccdc_init);
>> diff --git a/drivers/media/video/davinci/vpfe_capture.c
>> b/drivers/media/video/davinci/vpfe_capture.c
>> index c3468ee..35bbb08 100644
>> --- a/drivers/media/video/davinci/vpfe_capture.c
>> +++ b/drivers/media/video/davinci/vpfe_capture.c
>> @@ -108,9 +108,6 @@ struct ccdc_config {
>> int vpfe_probed;
>> /* name of ccdc device */
>> char name[32];
>> - /* for storing mem maps for CCDC */
>> - int ccdc_addr_size;
>> - void *__iomem ccdc_addr;
>> };
>>
>> /* data structures */
>> @@ -230,7 +227,6 @@ int vpfe_register_ccdc_device(struct
>> ccdc_hw_device *dev)
>> BUG_ON(!dev->hw_ops.set_image_window);
>> BUG_ON(!dev->hw_ops.get_image_window);
>> BUG_ON(!dev->hw_ops.get_line_length);
>> - BUG_ON(!dev->hw_ops.setfbaddr);
>> BUG_ON(!dev->hw_ops.getfid);
>>
>> mutex_lock(&ccdc_lock);
>> @@ -241,25 +237,23 @@ int vpfe_register_ccdc_device(struct
>> ccdc_hw_device *dev)
>> * walk through it during vpfe probe
>> */
>> printk(KERN_ERR "vpfe capture not initialized\n");
>> - ret = -1;
>> + ret = -EFAULT;
>> goto unlock;
>> }
>>
>> if (strcmp(dev->name, ccdc_cfg->name)) {
>> /* ignore this ccdc */
>> - ret = -1;
>> + ret = -EINVAL;
>> goto unlock;
>> }
>>
>> if (ccdc_dev) {
>> printk(KERN_ERR "ccdc already registered\n");
>> - ret = -1;
>> + ret = -EINVAL;
>> goto unlock;
>> }
>>
>> ccdc_dev = dev;
>> - dev->hw_ops.set_ccdc_base(ccdc_cfg->ccdc_addr,
>> - ccdc_cfg->ccdc_addr_size);
>> unlock:
>> mutex_unlock(&ccdc_lock);
>> return ret;
>> @@ -1947,37 +1941,12 @@ static __init int vpfe_probe(struct
>> platform_device *pdev)
>> }
>> vpfe_dev->ccdc_irq1 = res1->start;
>>
>> - /* Get address base of CCDC */
>> - res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - if (!res1) {
>> - v4l2_err(pdev->dev.driver,
>> - "Unable to get register address map\n");
>> - ret = -ENOENT;
>> - goto probe_disable_clock;
>> - }
>> -
>> - ccdc_cfg->ccdc_addr_size = res1->end - res1->start + 1;
>> - if (!request_mem_region(res1->start, ccdc_cfg->ccdc_addr_size,
>> - pdev->dev.driver->name)) {
>> - v4l2_err(pdev->dev.driver,
>> - "Failed request_mem_region for ccdc base\n");
>> - ret = -ENXIO;
>> - goto probe_disable_clock;
>> - }
>> - ccdc_cfg->ccdc_addr = ioremap_nocache(res1->start,
>> - ccdc_cfg->ccdc_addr_size);
>> - if (!ccdc_cfg->ccdc_addr) {
>> - v4l2_err(pdev->dev.driver, "Unable to ioremap ccdc
>> addr\n");
>> - ret = -ENXIO;
>> - goto probe_out_release_mem1;
>> - }
>> -
>> ret = request_irq(vpfe_dev->ccdc_irq0, vpfe_isr,
>> IRQF_DISABLED,
>> "vpfe_capture0", vpfe_dev);
>>
>> if (0 != ret) {
>> v4l2_err(pdev->dev.driver, "Unable to request
>> interrupt\n");
>> - goto probe_out_unmap1;
>> + goto probe_disable_clock;
>> }
>>
>> /* Allocate memory for video device */
>> @@ -2101,10 +2070,6 @@ probe_out_video_release:
>> video_device_release(vpfe_dev->video_dev);
>> probe_out_release_irq:
>> free_irq(vpfe_dev->ccdc_irq0, vpfe_dev);
>> -probe_out_unmap1:
>> - iounmap(ccdc_cfg->ccdc_addr);
>> -probe_out_release_mem1:
>> - release_mem_region(res1->start, res1->end - res1->start + 1);
>> probe_disable_clock:
>> vpfe_disable_clock(vpfe_dev);
>> mutex_unlock(&ccdc_lock);
>> @@ -2120,7 +2085,6 @@ probe_free_dev_mem:
>> static int vpfe_remove(struct platform_device *pdev)
>> {
>> struct vpfe_device *vpfe_dev = platform_get_drvdata(pdev);
>> - struct resource *res;
>>
>> v4l2_info(pdev->dev.driver, "vpfe_remove\n");
>>
>> @@ -2128,11 +2092,6 @@ static int vpfe_remove(struct platform_device
>> *pdev)
>> kfree(vpfe_dev->sd);
>> v4l2_device_unregister(&vpfe_dev->v4l2_dev);
>> video_unregister_device(vpfe_dev->video_dev);
>> - mutex_lock(&ccdc_lock);
>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - release_mem_region(res->start, res->end - res->start + 1);
>> - iounmap(ccdc_cfg->ccdc_addr);
>> - mutex_unlock(&ccdc_lock);
>> vpfe_disable_clock(vpfe_dev);
>> kfree(vpfe_dev);
>> kfree(ccdc_cfg);
>> --
>> 1.6.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 04, 2009 at 01:21:43, Karicheri, Muralidharan wrote:
>
[...]
> >
> >> + if (!res) {
> >> + status = -EBUSY;
> >> + goto fail_nores;
> >> + }
> >> +
> >> + ccdc_base_addr = ioremap_nocache(res->start, res_len);
> >> + if (!ccdc_base_addr) {
> >> + status = -EBUSY;
> >[Hiremath, Vaibhav] Is -EBUSY right return value, I think it should be -
> >ENXIO or -ENOMEM.
> >
> I see -ENXIO & -ENOMEM being used by drivers. -ENXIO stands for "No such device or address". ENOMEM stands for "Out of memory" . Since we are trying to map the address here, -ENXIO looks reasonable to me. Same if request_mem_region() fails.
>
Sergei had posted on this earlier[1]. Quoting him here:
"
> What are the proper error codes when platform_get_resource,
-ENODEV.
> request_mem_region
-EBUSY.
> and ioremap functions fail?.
-ENOMEM.
"
Not sure if ioremap failure can relate to absence of a device.
Thanks,
Sekhar
[1] http://www.mail-archive.com/davinci-linux-open-source@linux.davincidsp.com/msg14973.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sekhar,
>> >> + status = -EBUSY;
>> >[Hiremath, Vaibhav] Is -EBUSY right return value, I think it should be -
>> >ENXIO or -ENOMEM.
>> >
>> I see -ENXIO & -ENOMEM being used by drivers. -ENXIO stands for "No such
>device or address". ENOMEM stands for "Out of memory" . Since we are trying
>to map the address here, -ENXIO looks reasonable to me. Same if
>request_mem_region() fails.
>>
>
>Sergei had posted on this earlier[1]. Quoting him here:
Was this his personal opinion or has he given any reference to support it?
I did a grep for this in the driver directory and the result I got is in inline with Sergie's suggestion. So I am going to update the patch with these and send it again.
-Murali
>
>"
>> What are the proper error codes when platform_get_resource,
>
> -ENODEV.
>
>> request_mem_region
>
> -EBUSY.
>
>> and ioremap functions fail?.
>
> -ENOMEM.
>"
>
>Not sure if ioremap failure can relate to absence of a device.
>
>Thanks,
>Sekhar
>
>[1] http://www.mail-archive.com/davinci-linux-open-
>source@linux.davincidsp.com/msg14973.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -37,6 +37,7 @@
#include <linux/platform_device.h>
#include <linux/uaccess.h>
#include <linux/videodev2.h>
+#include <mach/mux.h>
#include <media/davinci/dm355_ccdc.h>
#include <media/davinci/vpss.h>
#include "dm355_ccdc_regs.h"
@@ -105,7 +106,6 @@ static struct ccdc_params_ycbcr ccdc_hw_params_ycbcr = {
static enum vpfe_hw_if_type ccdc_if_type;
static void *__iomem ccdc_base_addr;
-static int ccdc_addr_size;
/* Raw Bayer formats */
static u32 ccdc_raw_bayer_pix_formats[] =
@@ -126,12 +126,6 @@ static inline void regw(u32 val, u32 offset)
__raw_writel(val, ccdc_base_addr + offset);
}
-static void ccdc_set_ccdc_base(void *addr, int size)
-{
- ccdc_base_addr = addr;
- ccdc_addr_size = size;
-}
-
static void ccdc_enable(int en)
{
unsigned int temp;
@@ -938,7 +932,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
.hw_ops = {
.open = ccdc_open,
.close = ccdc_close,
- .set_ccdc_base = ccdc_set_ccdc_base,
.enable = ccdc_enable,
.enable_out_to_sdram = ccdc_enable_output_to_sdram,
.set_hw_if_params = ccdc_set_hw_if_params,
@@ -959,19 +952,89 @@ static struct ccdc_hw_device ccdc_hw_dev = {
},
};
-static int __init dm355_ccdc_init(void)
+static int __init dm355_ccdc_probe(struct platform_device *pdev)
{
- printk(KERN_NOTICE "dm355_ccdc_init\n");
- if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
- return -1;
+ static resource_size_t res_len;
+ struct resource *res;
+ int status = 0;
+
+ /**
+ * first try to register with vpfe. If not correct platform, then we
+ * don't have to iomap
+ */
+ status = vpfe_register_ccdc_device(&ccdc_hw_dev);
+ if (status < 0)
+ return status;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ status = -ENOENT;
+ goto fail_nores;
+ }
+ res_len = res->end - res->start + 1;
+
+ res = request_mem_region(res->start, res_len, res->name);
+ if (!res) {
+ status = -EBUSY;
+ goto fail_nores;
+ }
+
+ ccdc_base_addr = ioremap_nocache(res->start, res_len);
+ if (!ccdc_base_addr) {
+ status = -EBUSY;
+ goto fail;
+ }
+ /**
+ * setup Mux configuration for vpfe input and register
+ * vpfe capture platform device
+ */
+ davinci_cfg_reg(DM355_VIN_PCLK);
+ davinci_cfg_reg(DM355_VIN_CAM_WEN);
+ davinci_cfg_reg(DM355_VIN_CAM_VD);
+ davinci_cfg_reg(DM355_VIN_CAM_HD);
+ davinci_cfg_reg(DM355_VIN_YIN_EN);
+ davinci_cfg_reg(DM355_VIN_CINL_EN);
+ davinci_cfg_reg(DM355_VIN_CINH_EN);
+
printk(KERN_NOTICE "%s is registered with vpfe.\n",
ccdc_hw_dev.name);
return 0;
+fail:
+ release_mem_region(res->start, res_len);
+fail_nores:
+ vpfe_unregister_ccdc_device(&ccdc_hw_dev);
+ return status;
}
-static void __exit dm355_ccdc_exit(void)
+static int dm355_ccdc_remove(struct platform_device *pdev)
{
+ struct resource *res;
+
+ iounmap(ccdc_base_addr);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res)
+ release_mem_region(res->start, res->end - res->start + 1);
vpfe_unregister_ccdc_device(&ccdc_hw_dev);
+ return 0;
+}
+
+static struct platform_driver dm355_ccdc_driver = {
+ .driver = {
+ .name = "dm355_ccdc",
+ .owner = THIS_MODULE,
+ },
+ .remove = __devexit_p(dm355_ccdc_remove),
+ .probe = dm355_ccdc_probe,
+};
+
+static int __init dm355_ccdc_init(void)
+{
+ return platform_driver_register(&dm355_ccdc_driver);
+}
+
+static void __exit dm355_ccdc_exit(void)
+{
+ platform_driver_unregister(&dm355_ccdc_driver);
}
module_init(dm355_ccdc_init);
@@ -85,7 +85,6 @@ static u32 ccdc_raw_yuv_pix_formats[] =
{V4L2_PIX_FMT_UYVY, V4L2_PIX_FMT_YUYV};
static void *__iomem ccdc_base_addr;
-static int ccdc_addr_size;
static enum vpfe_hw_if_type ccdc_if_type;
/* register access routines */
@@ -99,12 +98,6 @@ static inline void regw(u32 val, u32 offset)
__raw_writel(val, ccdc_base_addr + offset);
}
-static void ccdc_set_ccdc_base(void *addr, int size)
-{
- ccdc_base_addr = addr;
- ccdc_addr_size = size;
-}
-
static void ccdc_enable(int flag)
{
regw(flag, CCDC_PCR);
@@ -838,7 +831,6 @@ static struct ccdc_hw_device ccdc_hw_dev = {
.hw_ops = {
.open = ccdc_open,
.close = ccdc_close,
- .set_ccdc_base = ccdc_set_ccdc_base,
.reset = ccdc_sbl_reset,
.enable = ccdc_enable,
.set_hw_if_params = ccdc_set_hw_if_params,
@@ -859,19 +851,79 @@ static struct ccdc_hw_device ccdc_hw_dev = {
},
};
-static int __init dm644x_ccdc_init(void)
+static int __init dm644x_ccdc_probe(struct platform_device *pdev)
{
- printk(KERN_NOTICE "dm644x_ccdc_init\n");
- if (vpfe_register_ccdc_device(&ccdc_hw_dev) < 0)
- return -1;
+ static resource_size_t res_len;
+ struct resource *res;
+ int status = 0;
+
+ /**
+ * first try to register with vpfe. If not correct platform, then we
+ * don't have to iomap
+ */
+ status = vpfe_register_ccdc_device(&ccdc_hw_dev);
+ if (status < 0)
+ return status;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ status = -ENOENT;
+ goto fail_nores;
+ }
+
+ res_len = res->end - res->start + 1;
+
+ res = request_mem_region(res->start, res_len, res->name);
+ if (!res) {
+ status = -EBUSY;
+ goto fail_nores;
+ }
+
+ ccdc_base_addr = ioremap_nocache(res->start, res_len);
+ if (!ccdc_base_addr) {
+ status = -EBUSY;
+ goto fail;
+ }
+
printk(KERN_NOTICE "%s is registered with vpfe.\n",
ccdc_hw_dev.name);
return 0;
+fail:
+ release_mem_region(res->start, res_len);
+fail_nores:
+ vpfe_unregister_ccdc_device(&ccdc_hw_dev);
+ return status;
+}
+
+static int dm644x_ccdc_remove(struct platform_device *pdev)
+{
+ struct resource *res;
+
+ iounmap(ccdc_base_addr);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res)
+ release_mem_region(res->start, res->end - res->start + 1);
+ vpfe_unregister_ccdc_device(&ccdc_hw_dev);
+ return 0;
+}
+
+static struct platform_driver dm644x_ccdc_driver = {
+ .driver = {
+ .name = "dm644x_ccdc",
+ .owner = THIS_MODULE,
+ },
+ .remove = __devexit_p(dm644x_ccdc_remove),
+ .probe = dm644x_ccdc_probe,
+};
+
+static int __init dm644x_ccdc_init(void)
+{
+ return platform_driver_register(&dm644x_ccdc_driver);
}
static void __exit dm644x_ccdc_exit(void)
{
- vpfe_unregister_ccdc_device(&ccdc_hw_dev);
+ platform_driver_unregister(&dm644x_ccdc_driver);
}
module_init(dm644x_ccdc_init);
@@ -108,9 +108,6 @@ struct ccdc_config {
int vpfe_probed;
/* name of ccdc device */
char name[32];
- /* for storing mem maps for CCDC */
- int ccdc_addr_size;
- void *__iomem ccdc_addr;
};
/* data structures */
@@ -230,7 +227,6 @@ int vpfe_register_ccdc_device(struct ccdc_hw_device *dev)
BUG_ON(!dev->hw_ops.set_image_window);
BUG_ON(!dev->hw_ops.get_image_window);
BUG_ON(!dev->hw_ops.get_line_length);
- BUG_ON(!dev->hw_ops.setfbaddr);
BUG_ON(!dev->hw_ops.getfid);
mutex_lock(&ccdc_lock);
@@ -241,25 +237,23 @@ int vpfe_register_ccdc_device(struct ccdc_hw_device *dev)
* walk through it during vpfe probe
*/
printk(KERN_ERR "vpfe capture not initialized\n");
- ret = -1;
+ ret = -EFAULT;
goto unlock;
}
if (strcmp(dev->name, ccdc_cfg->name)) {
/* ignore this ccdc */
- ret = -1;
+ ret = -EINVAL;
goto unlock;
}
if (ccdc_dev) {
printk(KERN_ERR "ccdc already registered\n");
- ret = -1;
+ ret = -EINVAL;
goto unlock;
}
ccdc_dev = dev;
- dev->hw_ops.set_ccdc_base(ccdc_cfg->ccdc_addr,
- ccdc_cfg->ccdc_addr_size);
unlock:
mutex_unlock(&ccdc_lock);
return ret;
@@ -1947,37 +1941,12 @@ static __init int vpfe_probe(struct platform_device *pdev)
}
vpfe_dev->ccdc_irq1 = res1->start;
- /* Get address base of CCDC */
- res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res1) {
- v4l2_err(pdev->dev.driver,
- "Unable to get register address map\n");
- ret = -ENOENT;
- goto probe_disable_clock;
- }
-
- ccdc_cfg->ccdc_addr_size = res1->end - res1->start + 1;
- if (!request_mem_region(res1->start, ccdc_cfg->ccdc_addr_size,
- pdev->dev.driver->name)) {
- v4l2_err(pdev->dev.driver,
- "Failed request_mem_region for ccdc base\n");
- ret = -ENXIO;
- goto probe_disable_clock;
- }
- ccdc_cfg->ccdc_addr = ioremap_nocache(res1->start,
- ccdc_cfg->ccdc_addr_size);
- if (!ccdc_cfg->ccdc_addr) {
- v4l2_err(pdev->dev.driver, "Unable to ioremap ccdc addr\n");
- ret = -ENXIO;
- goto probe_out_release_mem1;
- }
-
ret = request_irq(vpfe_dev->ccdc_irq0, vpfe_isr, IRQF_DISABLED,
"vpfe_capture0", vpfe_dev);
if (0 != ret) {
v4l2_err(pdev->dev.driver, "Unable to request interrupt\n");
- goto probe_out_unmap1;
+ goto probe_disable_clock;
}
/* Allocate memory for video device */
@@ -2101,10 +2070,6 @@ probe_out_video_release:
video_device_release(vpfe_dev->video_dev);
probe_out_release_irq:
free_irq(vpfe_dev->ccdc_irq0, vpfe_dev);
-probe_out_unmap1:
- iounmap(ccdc_cfg->ccdc_addr);
-probe_out_release_mem1:
- release_mem_region(res1->start, res1->end - res1->start + 1);
probe_disable_clock:
vpfe_disable_clock(vpfe_dev);
mutex_unlock(&ccdc_lock);
@@ -2120,7 +2085,6 @@ probe_free_dev_mem:
static int vpfe_remove(struct platform_device *pdev)
{
struct vpfe_device *vpfe_dev = platform_get_drvdata(pdev);
- struct resource *res;
v4l2_info(pdev->dev.driver, "vpfe_remove\n");
@@ -2128,11 +2092,6 @@ static int vpfe_remove(struct platform_device *pdev)
kfree(vpfe_dev->sd);
v4l2_device_unregister(&vpfe_dev->v4l2_dev);
video_unregister_device(vpfe_dev->video_dev);
- mutex_lock(&ccdc_lock);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- release_mem_region(res->start, res->end - res->start + 1);
- iounmap(ccdc_cfg->ccdc_addr);
- mutex_unlock(&ccdc_lock);
vpfe_disable_clock(vpfe_dev);
kfree(vpfe_dev);
kfree(ccdc_cfg);