public inbox for linux-omap@vger.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH] OMAP1: camera core : Use platform driver structure
@ 2006-02-24 14:28 Woodruff, Richard
  2006-03-03 19:49 ` Tony Lindgren
  0 siblings, 1 reply; 6+ messages in thread
From: Woodruff, Richard @ 2006-02-24 14:28 UTC (permalink / raw)
  To: Komal Shah, Zhang, Jian, linux-omap-open-source

? Just because the platform bus is convenient for PC's doesn't
necessarily mean its right for all.  One nice property of using the
platform bus is the community keeps it up to date.  Perhaps it makes it
also slightly easier to port code from device to device if they all use
the same bus code.

One reason we have kept the OMAP bus structure is that it does reflect
the hardware and it allows for grouping of devices.  For power calls
this provides some way to group devices with their true buses.

The current buses reflect real buses and clock domains for peripherals.
There are controllable aspects per bus and definite per clock domain
controllable attributes which effect devices.  How effectively these
have been utilized in open tree's is something else.

Regards,
Richard W.

> -----Original Message-----
> From: linux-omap-open-source-bounces+r-woodruff2=ti.com@linux.omap.com
>
[mailto:linux-omap-open-source-bounces+r-woodruff2=ti.com@linux.omap.com
]
> On Behalf Of Komal Shah
> Sent: Friday, February 24, 2006 4:36 AM
> To: Zhang, Jian; linux-omap-open-source@linux.omap.com
> Subject: RE: [PATCH] OMAP1: camera core : Use platform driver
structure
> 
> --- "Zhang, Jian" <jzhang@ti.com> wrote:
> 
> > Komal,
> >
> > Is there any guideline on the way a driver is registered with LDM?
> > e.g.
> > via driver_register() or platform_driver_register()? We are now
> > always
> > using omap_driver_register() which is a wrapper to
driver_register().
> >
> > Again, is this the trend to move the bulk of init code to probe()
> > function?
> 
> This one is very tricky question :) May be GregKH and RMK can do
better
> justice :)
> 
> But I don't understand the need for you to wrap-up the arch specific
> register function around driver_register, may be you were defining
your
> bus type? I also feel, that this wrapper came much before in your
> kernel (2.6.9 and .8 series) than the platform_driver addition.
> 
> platform_driver structure does wraps the all the members except the
> devid, bus and shared clocks, and devid can be assigned using the
> platform_device structure, and bus assignment may not be required as
we
> connect them under common platform bus.
> 
> And platform_driver_register does call driver_register similar to your
> omap_driver_register with few more wrapper functions inside. But as
> platform_driver interface wrappers was added recently (Nov'05) by RMK,
> which allows to platform device driver methods to be passed a platform
> device structure instead a plain device structure and which will
> introduce casting in every platform driver. (Last sentence is copied
> from RMK's patch comment :) ).
> 
> But using driver_register directly doesn't break anything yet.
> 
> >
> > If these are what other drivers do, I certainly don't want to see
> > camera
> > driver being different.
> >
> 
> I don't see this as 'must have' kind of things, but if I look back at
> what my camera patch gained having those modifications as of current
> state of usage for OMAP1610/1710 camera arch, it is 'nothing' except
> (void *) casting.
> 
> Just FYI.
> 
> [1] http://lwn.net/Articles/161236/
> [2] http://lwn.net/Articles/158747/
> [3] http://lwn.net/Articles/158781/
> 
> 
> ---Komal Shah
> http://komalshah.blogspot.com/
> 
> __________________________________________________
> Do You Yahoo!?
> Tired of spam?  Yahoo! Mail has the best spam protection around
> http://mail.yahoo.com
> _______________________________________________
> Linux-omap-open-source mailing list
> Linux-omap-open-source@linux.omap.com
> http://linux.omap.com/mailman/listinfo/linux-omap-open-source

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH] OMAP1: camera core : Use platform driver structure
@ 2006-02-24  0:49 Zhang, Jian
  2006-02-24 10:35 ` Komal Shah
  0 siblings, 1 reply; 6+ messages in thread
From: Zhang, Jian @ 2006-02-24  0:49 UTC (permalink / raw)
  To: Komal Shah, linux-omap-open-source

Komal,

Is there any guideline on the way a driver is registered with LDM? e.g.
via driver_register() or platform_driver_register()? We are now always
using omap_driver_register() which is a wrapper to driver_register().

Again, is this the trend to move the bulk of init code to probe()
function?

If these are what other drivers do, I certainly don't want to see camera
driver being different.

Regards,
Jian
  
-----Original Message-----
From: Komal Shah [mailto:komal_shah802003@yahoo.com] 
Sent: Thursday, February 23, 2006 6:53 AM
To: linux-omap-open-source@linux.omap.com
Cc: david.cohen@indt.org.br; Zhang, Jian
Subject: [PATCH] OMAP1: camera core : Use platform driver structure

Jian/David,

Attached patch uses the platform driver structure and introduced probe
and remove function from init and cleanup. It builds for H2, but unable
to test as not having H2 board access.

Please review it and if possible test it.

Signed-off-by: Komal Shah <komal_shah802003@yahoo.com>

---Komal Shah
http://komalshah.blogspot.com/

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] OMAP1: camera core : Use platform driver structure
@ 2006-02-23 12:53 Komal Shah
  0 siblings, 0 replies; 6+ messages in thread
From: Komal Shah @ 2006-02-23 12:53 UTC (permalink / raw)
  To: linux-omap-open-source

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

Jian/David,

Attached patch uses the platform driver structure and introduced probe
and remove function from init and cleanup. It builds for H2, but unable
to test as not having H2 board access.

Please review it and if possible test it.

Signed-off-by: Komal Shah <komal_shah802003@yahoo.com>

---Komal Shah
http://komalshah.blogspot.com/

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 2437325919-camplatform.patch --]
[-- Type: text/x-patch; name="camplatform.patch", Size: 7796 bytes --]

diff --git a/drivers/input/keyboard/omap-keypad.c b/drivers/input/keyboard/omap-keypad.c
diff --git a/drivers/media/video/omap/camera_core.c b/drivers/media/video/omap/camera_core.c
index 12c1fd1..f5f916d 100644
--- a/drivers/media/video/omap/camera_core.c
+++ b/drivers/media/video/omap/camera_core.c
@@ -914,9 +914,9 @@ camera_core_open(struct inode *inode, st
 }
 
 #ifdef CONFIG_PM
-static int camera_core_suspend(struct device *dev, pm_message_t state)
+static int camera_core_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct camera_device *cam = dev_get_drvdata(dev);
+	struct camera_device *cam = platform_get_drvdata(pdev);
 	int ret = 0;
 
 	spin_lock(&cam->img_lock);
@@ -925,12 +925,13 @@ static int camera_core_suspend(struct de
 	}
 	cam->cam_sensor->power_off(cam->sensor_data);
 	spin_unlock(&cam->img_lock);
+
 	return ret;
 }
 
-static int camera_core_resume(struct device *dev)
+static int camera_core_resume(struct platform_device *pdev)
 {
-	struct camera_device *cam = dev_get_drvdata(dev);
+	struct camera_device *cam = platform_get_drvdata(pdev);
 	int ret = 0;
 
 	spin_lock(&cam->img_lock);
@@ -946,7 +947,7 @@ static int camera_core_resume(struct dev
 		camera_core_sgdma_process(cam);
 	}
 	spin_unlock(&cam->img_lock);
-
+	
 	return ret;
 }
 #endif	/* CONFIG_PM */
@@ -963,90 +964,29 @@ static struct file_operations camera_cor
 	.release		= camera_core_release,
 };
 
-static struct device_driver camera_core_driver = {
-	.name			= CAM_NAME,
-	.bus			= &platform_bus_type,
-	.probe			= NULL,
-	.remove			= NULL,
-#ifdef CONFIG_PM
-	.suspend		= camera_core_suspend,
-	.resume			= camera_core_resume,
-#endif
-	.shutdown		= NULL,
-};
-
-static struct platform_device camera_core_device = {
-	.name	= CAM_NAME,
-	.dev	= {
-			.release 	= NULL,
-		  },
-	.id	= 0,
-};
-
-void
-camera_core_cleanup(void)
-{
-	struct camera_device *cam = camera_dev;
-	struct video_device *vfd;
-
-	if (!cam)
-		return;
-
-	vfd = cam->vfd;
-	if (vfd) {
-		if (vfd->minor == -1) {
-			/* The device never got registered, so release the 
-			** video_device struct directly
-			*/
-			video_device_release(vfd);
-		} else {
-			/* The unregister function will release the video_device
-			** struct as well as unregistering it.
-			*/
-			video_unregister_device(vfd);
-			driver_unregister(&camera_core_driver);
-			platform_device_unregister(&camera_core_device);
-		}
-		cam->vfd = NULL;
-	}
-	if (cam->overlay_base) {
-		dma_free_coherent(NULL, cam->overlay_size,
-					(void *)cam->overlay_base, 
-					cam->overlay_base_phys);
-		cam->overlay_base = 0;
-	}	
-	cam->overlay_base_phys = 0;
-
-	cam->cam_sensor->cleanup(cam->sensor_data);
-	cam->cam_hardware->cleanup(cam->hardware_data);
-	kfree(cam);
-	camera_dev = NULL;
-
-	return;
-}
-
-
-int __init 
-camera_core_init(void)
+static int __init camera_core_probe(struct platform_device *pdev)
 {
 	struct camera_device *cam;
 	struct video_device *vfd;
+	int	status;
 
 	cam = kzalloc(sizeof(struct camera_device), GFP_KERNEL);
 	if (!cam) {
 		printk(KERN_ERR CAM_NAME ": could not allocate memory\n");
-		goto init_error;
+		status = -ENOMEM;
+		goto err0;
 	}
-	
+
 	/* Save the pointer to camera device in a global variable */
 	camera_dev = cam;
-
+	
 	/* initialize the video_device struct */
 	vfd = cam->vfd = video_device_alloc();
 	if (!vfd) {
 		printk(KERN_ERR CAM_NAME 
 			": could not allocate video device struct\n");
-		goto init_error;
+		status = -ENOMEM;
+		goto err1;
 	}
 	
  	vfd->release = video_device_release;
@@ -1077,7 +1017,8 @@ camera_core_init(void)
 		if (!cam->overlay_base) {
 			printk(KERN_ERR CAM_NAME
 				": cannot allocate overlay framebuffer\n");
-			goto init_error;
+			status = -ENOMEM;
+			goto err2;
 		}
 	}
 	memset((void*)cam->overlay_base, 0, cam->overlay_size);
@@ -1092,7 +1033,8 @@ camera_core_init(void)
 	cam->hardware_data = cam->cam_hardware->init();
 	if (!cam->hardware_data) {
 		printk(KERN_ERR CAM_NAME ": cannot initialize interface hardware\n");
-		goto init_error;
+		status = -ENODEV;
+		goto err3;
 	}
  	 
 	/* initialize the spinlock used to serialize access to the image 
@@ -1116,7 +1058,8 @@ camera_core_init(void)
 	if (!cam->sensor_data) {
 		cam->cam_hardware->disable(cam->hardware_data);
 		printk(KERN_ERR CAM_NAME ": cannot initialize sensor\n");
-		goto init_error;
+		status = -ENODEV;
+		goto err4;
 	}
 
 	printk(KERN_INFO CAM_NAME ": %s interface with %s sensor\n",
@@ -1141,35 +1084,116 @@ camera_core_init(void)
 
 	/* Disable the Camera after detection */
 	cam->cam_hardware->disable(cam->hardware_data);
-
-	dev_set_drvdata(&camera_core_device.dev, (void *)cam);
-	if (platform_device_register(&camera_core_device) < 0) {
-		printk(KERN_ERR CAM_NAME
-			": could not register platform_device\n");
-		goto init_error;
-	}
-
-	if (driver_register(&camera_core_driver) < 0) {
-		printk(KERN_ERR CAM_NAME
-			": could not register driver\n");
-		platform_device_unregister(&camera_core_device);
-		goto init_error;
-	}
+	
+	platform_set_drvdata(pdev, cam);
+	
 	if (video_register_device(vfd, VFL_TYPE_GRABBER, video_nr) < 0) {
 		printk(KERN_ERR CAM_NAME 
 			": could not register Video for Linux device\n");
-		platform_device_unregister(&camera_core_device);
-		driver_unregister(&camera_core_driver);
-		goto init_error;
+		status = -ENODEV;
+		goto err5;
 	}
 
 	printk(KERN_INFO CAM_NAME 
-		": registered device video%d [v4l2]\n", vfd->minor);
+	       ": registered device video%d [v4l2]\n", vfd->minor);
+
 	return 0;
 
-init_error:
-	camera_core_cleanup();
-	return -ENODEV;
+ err5:
+	cam->cam_sensor->cleanup(cam->sensor_data);
+ err4:
+	cam->cam_hardware->cleanup(cam->hardware_data);
+ err3:
+	dma_free_coherent(NULL, cam->overlay_size,
+				(void *)cam->overlay_base, 
+				cam->overlay_base_phys);
+	cam->overlay_base = 0;
+ err2:
+	video_device_release(vfd);
+ err1:
+	kfree(cam);
+	camera_dev = NULL;
+ err0:
+	return status;
+}
+
+static int camera_core_remove(struct platform_device *pdev)
+{
+	struct camera_device *cam = platform_get_drvdata(pdev);
+	struct video_device *vfd;
+
+	vfd = cam->vfd;
+	if (vfd) {
+		if (vfd->minor == -1) {
+			/* The device never got registered, so release the 
+			** video_device struct directly
+			*/
+			video_device_release(vfd);
+		} else {
+			/* The unregister function will release the video_device
+			** struct as well as unregistering it.
+			*/
+			video_unregister_device(vfd);
+		}
+		cam->vfd = NULL;
+	}
+	if (cam->overlay_base) {
+		dma_free_coherent(NULL, cam->overlay_size,
+					(void *)cam->overlay_base, 
+					cam->overlay_base_phys);
+		cam->overlay_base = 0;
+	}	
+	cam->overlay_base_phys = 0;
+
+	cam->cam_sensor->cleanup(cam->sensor_data);
+	cam->cam_hardware->cleanup(cam->hardware_data);
+	kfree(cam);
+	camera_dev = NULL;
+
+	return 0;
+}
+
+static struct platform_driver camera_core_driver = {
+	.driver = {
+		.name		= CAM_NAME,
+		.owner		= THIS_MODULE,
+	},
+	.probe			= camera_core_probe,
+	.remove			= camera_core_remove,
+#ifdef CONFIG_PM
+	.suspend		= camera_core_suspend,
+	.resume			= camera_core_resume,
+#endif
+};
+
+static struct platform_device camera_core_device = {
+	.name	= CAM_NAME,
+	.dev	= {
+			.release 	= NULL,
+		  },
+	.id	= 0,
+};
+
+void __exit
+camera_core_cleanup(void)
+{
+	platform_driver_unregister(&camera_core_driver);
+	platform_device_unregister(&camera_core_device);
+
+	return;
+}
+
+static char banner[] __initdata = KERN_INFO "OMAP Camera driver initialzing\n";
+
+int __init 
+camera_core_init(void)
+{
+
+	printk(banner);
+	platform_device_register(&camera_core_device);
+	platform_driver_register(&camera_core_driver);
+
+	return 0;
 }
 
 MODULE_AUTHOR("Texas Instruments.");
@@ -1185,4 +1209,3 @@ MODULE_PARM_DESC(capture_mem,
 module_init(camera_core_init);
 module_exit(camera_core_cleanup);
 
-

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-03-03 19:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-24 14:28 [PATCH] OMAP1: camera core : Use platform driver structure Woodruff, Richard
2006-03-03 19:49 ` Tony Lindgren
  -- strict thread matches above, loose matches on Subject: below --
2006-02-24  0:49 Zhang, Jian
2006-02-24 10:35 ` Komal Shah
2006-03-03 19:52   ` Tony Lindgren
2006-02-23 12:53 Komal Shah

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox