RE: [PATCH v0 1/2] V4L - vpfe capture - convert ccdc drivers to platform drivers
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- sou...@linux.davincidsp.com/msg14973.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v0 1/2] V4L - vpfe capture - convert ccdc drivers to platform drivers
+#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
+++
RE: [PATCH v0 1/2] V4L - vpfe capture - convert ccdc drivers to platform drivers
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-sou...@linux.davincidsp.com/msg14973.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v0 1/2] V4L - vpfe capture - convert ccdc drivers to platform drivers
-Original Message-
From: Karicheri, Muralidharan
Sent: Tuesday, December 01, 2009 11:46 PM
To: linux-media@vger.kernel.org; hverk...@xs4all.nl;
khil...@deeprootsystems.com
Cc: davinci-linux-open-sou...@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-kariche...@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-kariche...@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)
{
