[PATCH] Move .power and .reset from soc_camera platform to sensor driver

[PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Stefan Herbrechtsmeier]

Move .power (enable_camera, disable_camera) and .reset from soc_camera
platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
driver (soc_camera_link) and add .init and .release to request and free
gpios.

Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>

diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/drivers/media/video/mt9m001.c
--- a/linux/drivers/media/video/mt9m001.c	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/drivers/media/video/mt9m001.c	Thu Jul 31 07:59:35 2008 +0200
@@ -117,14 +117,23 @@ static int reg_clear(struct soc_camera_d
 
 static int mt9m001_init(struct soc_camera_device *icd)
 {
-	int ret;
+	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
+	int ret = 0;
 
 	/* Disable chip, synchronous option update */
 	dev_dbg(icd->vdev->parent, "%s\n", __func__);
 
-	ret = reg_write(icd, MT9M001_RESET, 1);
-	if (ret >= 0)
-		ret = reg_write(icd, MT9M001_RESET, 0);
+	if (icl->power)
+		icl->power(&mt9m001->client->dev, 1);
+
+	if (icl->reset)
+		icl->reset(&mt9m001->client->dev);
+	else {
+		ret = reg_write(icd, MT9M001_RESET, 1);
+		if (ret >= 0)
+			ret = reg_write(icd, MT9M001_RESET, 0);
+	}
 	if (ret >= 0)
 		ret = reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
 
@@ -133,8 +142,15 @@ static int mt9m001_init(struct soc_camer
 
 static int mt9m001_release(struct soc_camera_device *icd)
 {
+	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
+
 	/* Disable the chip */
-	reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
+	if (icl->power)
+		icl->power(&mt9m001->client->dev, 0);
+	else
+		reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
+
 	return 0;
 }
 
@@ -670,6 +686,12 @@ static int mt9m001_probe(struct i2c_clie
 	 * ourselves in the driver based on vertical blanking and frame width */
 	mt9m001->autoexposure = 1;
 
+	if (icl->init) {
+		ret = icl->init(&mt9m001->client->dev);
+		if (ret)
+			goto einit;
+	}
+
 	ret = bus_switch_request(mt9m001, icl);
 	if (ret)
 		goto eswinit;
@@ -683,6 +705,9 @@ eisdr:
 eisdr:
 	bus_switch_release(mt9m001);
 eswinit:
+	if (icl->release)
+		icl->release(&mt9m001->client->dev);
+einit:
 	kfree(mt9m001);
 	return ret;
 }
@@ -690,9 +715,12 @@ static int mt9m001_remove(struct i2c_cli
 static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = i2c_get_clientdata(client);
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
 
 	soc_camera_device_unregister(&mt9m001->icd);
 	bus_switch_release(mt9m001);
+	if (icl->release)
+		icl->release(&mt9m001->client->dev);
 	kfree(mt9m001);
 
 	return 0;
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/drivers/media/video/mt9v022.c
--- a/linux/drivers/media/video/mt9v022.c	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/drivers/media/video/mt9v022.c	Thu Jul 31 07:59:35 2008 +0200
@@ -134,7 +134,11 @@ static int mt9v022_init(struct soc_camer
 static int mt9v022_init(struct soc_camera_device *icd)
 {
 	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
 	int ret;
+
+	if (icl->power)
+		icl->power(&mt9v022->client->dev, 1);
 
 	/* Almost the default mode: master, parallel, simultaneous, and an
 	 * undocumented bit 0x200, which is present in table 7, but not in 8,
@@ -161,7 +165,12 @@ static int mt9v022_init(struct soc_camer
 
 static int mt9v022_release(struct soc_camera_device *icd)
 {
-	/* Nothing? */
+	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
+
+	if (icl->power)
+		icl->power(&mt9v022->client->dev, 0);
+
 	return 0;
 }
 
@@ -668,6 +677,7 @@ static int mt9v022_video_probe(struct so
 static int mt9v022_video_probe(struct soc_camera_device *icd)
 {
 	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
 	s32 data;
 	int ret;
 
@@ -686,15 +696,19 @@ static int mt9v022_video_probe(struct so
 		goto ei2c;
 	}
 
-	/* Soft reset */
-	ret = reg_write(icd, MT9V022_RESET, 1);
-	if (ret < 0)
-		goto ei2c;
-	/* 15 clock cycles */
-	udelay(200);
-	if (reg_read(icd, MT9V022_RESET)) {
-		dev_err(&icd->dev, "Resetting MT9V022 failed!\n");
-		goto ei2c;
+	if (icl->reset)
+		icl->reset(&mt9v022->client->dev);
+	else {
+		/* Soft reset */
+		ret = reg_write(icd, MT9V022_RESET, 1);
+		if (ret < 0)
+			goto ei2c;
+		/* 15 clock cycles */
+		udelay(200);
+		if (reg_read(icd, MT9V022_RESET)) {
+			dev_err(&icd->dev, "Resetting MT9V022 failed!\n");
+			goto ei2c;
+		}
 	}
 
 	/* Set monochrome or colour sensor type */
@@ -788,6 +802,12 @@ static int mt9v022_probe(struct i2c_clie
 	 * other widths. Therefore it seems to be a sensible default. */
 	mt9v022->datawidth = 10;
 
+	if (icl->init) {
+		ret = icl->init(&mt9v022->client->dev);
+		if (ret)
+			goto einit;
+	}
+
 	ret = bus_switch_request(mt9v022, icl);
 	if (ret)
 		goto eswinit;
@@ -801,6 +821,9 @@ eisdr:
 eisdr:
 	bus_switch_release(mt9v022);
 eswinit:
+	if (icl->release)
+		icl->release(&mt9v022->client->dev);
+einit:
 	kfree(mt9v022);
 	return ret;
 }
@@ -808,9 +831,12 @@ static int mt9v022_remove(struct i2c_cli
 static int mt9v022_remove(struct i2c_client *client)
 {
 	struct mt9v022 *mt9v022 = i2c_get_clientdata(client);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
 
 	soc_camera_device_unregister(&mt9v022->icd);
 	bus_switch_release(mt9v022);
+	if (icl->release)
+		icl->release(&mt9v022->client->dev);
 	kfree(mt9v022);
 
 	return 0;
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/drivers/media/video/pxa_camera.c
--- a/linux/drivers/media/video/pxa_camera.c	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/drivers/media/video/pxa_camera.c	Thu Jul 31 07:59:35 2008 +0200
@@ -627,17 +627,6 @@ static void pxa_camera_activate(struct p
 		pdata->init(pcdev->dev);
 	}
 
-	if (pdata && pdata->power) {
-		dev_dbg(pcdev->dev, "%s: Power on camera\n", __func__);
-		pdata->power(pcdev->dev, 1);
-	}
-
-	if (pdata && pdata->reset) {
-		dev_dbg(pcdev->dev, "%s: Releasing camera reset\n",
-			__func__);
-		pdata->reset(pcdev->dev, 1);
-	}
-
 	CICR0 = 0x3FF;   /* disable all interrupts */
 
 	if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)
@@ -658,20 +647,7 @@ static void pxa_camera_activate(struct p
 
 static void pxa_camera_deactivate(struct pxa_camera_dev *pcdev)
 {
-	struct pxacamera_platform_data *board = pcdev->pdata;
-
 	clk_disable(pcdev->clk);
-
-	if (board && board->reset) {
-		dev_dbg(pcdev->dev, "%s: Asserting camera reset\n",
-			__func__);
-		board->reset(pcdev->dev, 0);
-	}
-
-	if (board && board->power) {
-		dev_dbg(pcdev->dev, "%s: Power off camera\n", __func__);
-		board->power(pcdev->dev, 0);
-	}
 }
 
 static irqreturn_t pxa_camera_irq(int irq, void *data)
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/drivers/media/video/sh_mobile_ceu_camera.c
--- a/linux/drivers/media/video/sh_mobile_ceu_camera.c	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/drivers/media/video/sh_mobile_ceu_camera.c	Thu Jul 31 07:59:35 2008 +0200
@@ -304,9 +304,6 @@ static int sh_mobile_ceu_add_device(stru
 		 "SuperH Mobile CEU driver attached to camera %d\n",
 		 icd->devnum);
 
-	if (pcdev->pdata->enable_camera)
-		pcdev->pdata->enable_camera();
-
 	ret = icd->ops->init(icd);
 	if (ret)
 		goto err;
@@ -333,8 +330,6 @@ static void sh_mobile_ceu_remove_device(
 	ceu_write(pcdev, CEIER, 0);
 	ceu_write(pcdev, CAPSR, 1 << 16); /* reset */
 	icd->ops->release(icd);
-	if (pcdev->pdata->disable_camera)
-		pcdev->pdata->disable_camera();
 
 	dev_info(&icd->dev,
 		 "SuperH Mobile CEU driver detached from camera %d\n",
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/include/asm-arm/arch-pxa/camera.h
--- a/linux/include/asm-arm/arch-pxa/camera.h	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/include/asm-arm/arch-pxa/camera.h	Thu Jul 31 07:59:35 2008 +0200
@@ -36,8 +36,6 @@
 
 struct pxacamera_platform_data {
 	int (*init)(struct device *);
-	int (*power)(struct device *, int);
-	int (*reset)(struct device *, int);
 
 	unsigned long flags;
 	unsigned long mclk_10khz;
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/include/media/sh_mobile_ceu.h
--- a/linux/include/media/sh_mobile_ceu.h	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/include/media/sh_mobile_ceu.h	Thu Jul 31 07:59:35 2008 +0200
@@ -5,8 +5,6 @@
 
 struct sh_mobile_ceu_info {
 	unsigned long flags; /* SOCAM_... */
-	void (*enable_camera)(void);
-	void (*disable_camera)(void);
 };
 
 #endif /* __ASM_SH_MOBILE_CEU_H__ */
diff -r 55e8c99c8aa8 -r 294f0a37c4fe linux/include/media/soc_camera.h
--- a/linux/include/media/soc_camera.h	Wed Jul 30 07:18:13 2008 -0300
+++ b/linux/include/media/soc_camera.h	Thu Jul 31 07:59:35 2008 +0200
@@ -76,6 +76,11 @@ struct soc_camera_host_ops {
 };
 
 struct soc_camera_link {
+	int (*init)(struct device *);
+	void (*release)(struct device *);
+	void (*power)(struct device *, int);
+	void (*reset)(struct device *);
+
 	/* Camera bus id, used to match a camera and a bus */
 	int bus_id;
 	/* GPIO number to switch between 8 and 10 bit modes */

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Paulius Zaleckas]

Stefan Herbrechtsmeier wrote:
> Move .power (enable_camera, disable_camera) and .reset from soc_camera
> platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
> driver (soc_camera_link) and add .init and .release to request and free
> gpios.
> 
> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>

While I agree that it is good to move .power and .reset to
soc_camera_link... IMHO controlling of these should be left in
host driver.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Stefan Herbrechtsmeier]

Paulius Zaleckas schrieb:
> Stefan Herbrechtsmeier wrote:
>> Move .power (enable_camera, disable_camera) and .reset from soc_camera
>> platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
>> driver (soc_camera_link) and add .init and .release to request and free
>> gpios.
>>
>> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
>
> While I agree that it is good to move .power and .reset to
> soc_camera_link... IMHO controlling of these should be left in
> host driver.
How should we deal with the register based version of this functions 
(soft reset)?
At the moment we reset the sensors twice, if we use a hardware reset 
(.reset).

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Guennadi Liakhovetski]

On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:

> Paulius Zaleckas schrieb:
> > Stefan Herbrechtsmeier wrote:
> > > Move .power (enable_camera, disable_camera) and .reset from soc_camera
> > > platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
> > > driver (soc_camera_link) and add .init and .release to request and free
> > > gpios.
> > > 
> > > Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
> > 
> > While I agree that it is good to move .power and .reset to
> > soc_camera_link... IMHO controlling of these should be left in
> > host driver.
> How should we deal with the register based version of this functions (soft
> reset)?
> At the moment we reset the sensors twice, if we use a hardware reset (.reset).

Paulius, can you give any specific reason why you think, calling those 
functions from the host driver would be better?

As for calling either platform-provided reset or internal one. Actually, 
whyt about making platform reset (and power too) return an error code, and 
if it failed call th internal one? And as a parameter wouldn't it make 
more sense to pass the soc_camera_link to the platform functions instead 
of the struct device from the i2c device?

I'll have a better look at your patch this WE, so, you don't have to be in 
a hurry with a new version:-) I probably will have some more comments. 
These are just a couple to think about.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Paulius Zaleckas schrieb:
>>     
>>> Stefan Herbrechtsmeier wrote:
>>>       
>>>> Move .power (enable_camera, disable_camera) and .reset from soc_camera
>>>> platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
>>>> driver (soc_camera_link) and add .init and .release to request and free
>>>> gpios.
>>>>
>>>> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
>>>>         
>>> While I agree that it is good to move .power and .reset to
>>> soc_camera_link... IMHO controlling of these should be left in
>>> host driver.
>>>       
>> How should we deal with the register based version of this functions (soft
>> reset)?
>> At the moment we reset the sensors twice, if we use a hardware reset (.reset).
>>     
>
> Paulius, can you give any specific reason why you think, calling those 
> functions from the host driver would be better?
>
> As for calling either platform-provided reset or internal one. Actually, 
> whyt about making platform reset (and power too) return an error code, and 
> if it failed call th internal one? 
At the moment I assume that reset and power will work, if they are 
defined, but we can change it.
> And as a parameter wouldn't it make 
> more sense to pass the soc_camera_link to the platform functions instead 
> of the struct device from the i2c device?
>   
I have simple make the function similar to other platform_data functions 
on my system.
At the moment I use the parameter only for printing messages via dev_err.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Guennadi Liakhovetski]

On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > As for calling either platform-provided reset or internal one. Actually,
> > whyt about making platform reset (and power too) return an error code, and
> > if it failed call th internal one? 
> At the moment I assume that reset and power will work, if they are defined,
> but we can change it.

I'd rather preserve the possibility to use "soft" reset / poweroff also 
when a platform function is defined. In fact, it might be even better to 
do a soft power-off first and then call platform-provided one. Don't think 
it would make much sense for reset though.

> > And as a parameter wouldn't it make more sense to pass the soc_camera_link
> > to the platform functions instead of the struct device from the i2c device?
> >   
> I have simple make the function similar to other platform_data functions on my
> system.
> At the moment I use the parameter only for printing messages via dev_err.

You have to be able to trace which camera has to be resetted / powered on 
or off in your platform code, and the camera_link structure is the object 
that identifies a specific camera, ot, at least, it can be. Whereas the 
device pointer doesn't easily tell you which camera you want to operate 
upon.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>> As for calling either platform-provided reset or internal one. Actually,
>>> whyt about making platform reset (and power too) return an error code, and
>>> if it failed call th internal one? 
>>>       
>> At the moment I assume that reset and power will work, if they are defined,
>> but we can change it.
>>     
>
> I'd rather preserve the possibility to use "soft" reset / poweroff also 
> when a platform function is defined. In fact, it might be even better to 
> do a soft power-off first and then call platform-provided one. Don't think 
> it would make much sense for reset though.
>   
You are right, I'll change it with the next version.
>   
>>> And as a parameter wouldn't it make more sense to pass the soc_camera_link
>>> to the platform functions instead of the struct device from the i2c device?
>>>   
>>>       
>> I have simple make the function similar to other platform_data functions on my
>> system.
>> At the moment I use the parameter only for printing messages via dev_err.
>>     
>
> You have to be able to trace which camera has to be resetted / powered on 
> or off in your platform code, and the camera_link structure is the object 
> that identifies a specific camera, ot, at least, it can be. Whereas the 
> device pointer doesn't easily tell you which camera you want to operate 
> upon.
>   
If you use the same function for different sensors (camera_links), you 
are right,
but you can get the camera_link from the dev pointer (.platform_data) if you
need it.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] Move .power and .reset from soc_camera platform to sensor driver

[Paulius Zaleckas]

Guennadi Liakhovetski wrote:
> On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:
> 
>> Paulius Zaleckas schrieb:
>>> Stefan Herbrechtsmeier wrote:
>>>> Move .power (enable_camera, disable_camera) and .reset from soc_camera
>>>> platform driver (pxa_camera_platform_data, sh_mobile_ceu_info) to sensor
>>>> driver (soc_camera_link) and add .init and .release to request and free
>>>> gpios.
>>>>
>>>> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
>>> While I agree that it is good to move .power and .reset to
>>> soc_camera_link... IMHO controlling of these should be left in
>>> host driver.
>> How should we deal with the register based version of this functions (soft
>> reset)?
>> At the moment we reset the sensors twice, if we use a hardware reset (.reset).
> 
> Paulius, can you give any specific reason why you think, calling those 
> functions from the host driver would be better?

I have changed my mind :)
But I think sensor driver should also control mclk activation.
This is because some sensors (in my case OV7670) needs mclk all the time
running to produce images with good white balance in open-capture-close
scenario. Currently I have a patch for soc_camera which leaves the
camera activated after probing...

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Guennadi Liakhovetski]

Make .power and .reset callbacks per camera instead of per host, also move 
their invocation to camera drivers.

Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

---

Robert and Stefan, please, read comments below, well, and the patch too:-)

This patch applies on the top of other my patches today.

Robert, please, have a look if you agree with the hunk for mt9m111. I so 
far added ->power calls to your enable and disable functions, not sure if 
this is the best place. BTW, why do you call enable from video_probe 
again? Is it really necessary? And you might want to call disable at 
suspend? Which would then also power the camera down, if supported by the 
platform.

Stefan,

On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > I'd rather preserve the possibility to use "soft" reset / poweroff also when
> > a platform function is defined. In fact, it might be even better to do a
> > soft power-off first and then call platform-provided one. Don't think it
> > would make much sense for reset though.
> >   
> You are right, I'll change it with the next version.

How about the version below? I didn't understand why you need extra .init 
and .release calls, so, I removed them for now. I think, .init per host 
and power-on / off per camera should be enough for all init / release 
needs, don't you think so?

> > > > And as a parameter wouldn't it make more sense to pass the
> > > > soc_camera_link
> > > > to the platform functions instead of the struct device from the i2c
> > > > device?
> > > >         
> > > I have simple make the function similar to other platform_data functions
> > > on my
> > > system.
> > > At the moment I use the parameter only for printing messages via dev_err.
> > >     
> > 
> > You have to be able to trace which camera has to be resetted / powered on or
> > off in your platform code, and the camera_link structure is the object that
> > identifies a specific camera, ot, at least, it can be. Whereas the device
> > pointer doesn't easily tell you which camera you want to operate upon.
> >   
> If you use the same function for different sensors (camera_links), you are
> right,
> but you can get the camera_link from the dev pointer (.platform_data) if you
> need it.

Agree, I kept your version with struct device pointer.

diff --git a/drivers/media/video/mt9m001.c b/drivers/media/video/mt9m001.c
index 3531f93..0c52437 100644
--- a/drivers/media/video/mt9m001.c
+++ b/drivers/media/video/mt9m001.c
@@ -117,13 +117,33 @@ static int reg_clear(struct soc_camera_device *icd, const u8 reg,
 
 static int mt9m001_init(struct soc_camera_device *icd)
 {
+	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
 	int ret;
 
 	dev_dbg(icd->vdev->parent, "%s\n", __func__);
 
-	ret = reg_write(icd, MT9M001_RESET, 1);
-	if (!ret)
-		ret = reg_write(icd, MT9M001_RESET, 0);
+	if (icl->power) {
+		ret = icl->power(&mt9m001->client->dev, 1);
+		if (ret < 0) {
+			dev_err(icd->vdev->parent,
+				"Platform failed to power-on the camera.\n");
+			return ret;
+		}
+	}
+
+	/* The camera could have been already on, we reset it additionally */
+	if (icl->reset)
+		ret = icl->reset(&mt9m001->client->dev);
+	else
+		ret = -ENODEV;
+
+	if (ret < 0) {
+		/* Either no platform reset, or platform reset failed */
+		ret = reg_write(icd, MT9M001_RESET, 1);
+		if (!ret)
+			ret = reg_write(icd, MT9M001_RESET, 0);
+	}
 	/* Disable chip, synchronous option update */
 	if (!ret)
 		ret = reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
@@ -133,8 +153,15 @@ static int mt9m001_init(struct soc_camera_device *icd)
 
 static int mt9m001_release(struct soc_camera_device *icd)
 {
+	struct mt9m001 *mt9m001 = container_of(icd, struct mt9m001, icd);
+	struct soc_camera_link *icl = mt9m001->client->dev.platform_data;
+
 	/* Disable the chip */
 	reg_write(icd, MT9M001_OUTPUT_CONTROL, 0);
+
+	if (icl->power)
+		icl->power(&mt9m001->client->dev, 0);
+
 	return 0;
 }
 
diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index 537cff0..8c532ac 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -351,8 +351,18 @@ static int mt9m111_setfmt_yuv(struct soc_camera_device *icd)
 static int mt9m111_enable(struct soc_camera_device *icd)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
+	struct soc_camera_link *icl = mt9m111->client->dev.platform_data;
 	int ret;
 
+	if (icl->power) {
+		ret = icl->power(&mt9m111->client->dev, 1);
+		if (ret < 0) {
+			dev_err(icd->vdev->parent,
+				"Platform failed to power-on the camera.\n");
+			return ret;
+		}
+	}
+
 	ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
 	if (!ret)
 		mt9m111->powered = 1;
@@ -362,11 +372,16 @@ static int mt9m111_enable(struct soc_camera_device *icd)
 static int mt9m111_disable(struct soc_camera_device *icd)
 {
 	struct mt9m111 *mt9m111 = container_of(icd, struct mt9m111, icd);
+	struct soc_camera_link *icl = mt9m111->client->dev.platform_data;
 	int ret;
 
 	ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
 	if (!ret)
 		mt9m111->powered = 0;
+
+	if (icl->power)
+		icl->power(&mt9m111->client->dev, 0);
+
 	return ret;
 }
 
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index 0f4b204..2584201 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -134,8 +134,25 @@ static int reg_clear(struct soc_camera_device *icd, const u8 reg,
 static int mt9v022_init(struct soc_camera_device *icd)
 {
 	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
 	int ret;
 
+	if (icl->power) {
+		ret = icl->power(&mt9v022->client->dev, 1);
+		if (ret < 0) {
+			dev_err(icd->vdev->parent,
+				"Platform failed to power-on the camera.\n");
+			return ret;
+		}
+	}
+
+	/*
+	 * The camera could have been already on, we hard-reset it additionally,
+	 * if available. Soft reset is done in video_probe().
+	 */
+	if (icl->reset)
+		icl->reset(&mt9v022->client->dev);
+
 	/* Almost the default mode: master, parallel, simultaneous, and an
 	 * undocumented bit 0x200, which is present in table 7, but not in 8,
 	 * plus snapshot mode to disable scan for now */
@@ -161,7 +178,12 @@ static int mt9v022_init(struct soc_camera_device *icd)
 
 static int mt9v022_release(struct soc_camera_device *icd)
 {
-	/* Nothing? */
+	struct mt9v022 *mt9v022 = container_of(icd, struct mt9v022, icd);
+	struct soc_camera_link *icl = mt9v022->client->dev.platform_data;
+
+	if (icl->power)
+		icl->power(&mt9v022->client->dev, 0);
+
 	return 0;
 }
 
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 85f545d..6df2aee 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -629,17 +629,6 @@ static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 		pdata->init(pcdev->dev);
 	}
 
-	if (pdata && pdata->power) {
-		dev_dbg(pcdev->dev, "%s: Power on camera\n", __func__);
-		pdata->power(pcdev->dev, 1);
-	}
-
-	if (pdata && pdata->reset) {
-		dev_dbg(pcdev->dev, "%s: Releasing camera reset\n",
-			__func__);
-		pdata->reset(pcdev->dev, 1);
-	}
-
 	CICR0 = 0x3FF;   /* disable all interrupts */
 
 	if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)
@@ -660,20 +649,7 @@ static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 
 static void pxa_camera_deactivate(struct pxa_camera_dev *pcdev)
 {
-	struct pxacamera_platform_data *board = pcdev->pdata;
-
 	clk_disable(pcdev->clk);
-
-	if (board && board->reset) {
-		dev_dbg(pcdev->dev, "%s: Asserting camera reset\n",
-			__func__);
-		board->reset(pcdev->dev, 0);
-	}
-
-	if (board && board->power) {
-		dev_dbg(pcdev->dev, "%s: Power off camera\n", __func__);
-		board->power(pcdev->dev, 0);
-	}
 }
 
 static irqreturn_t pxa_camera_irq(int irq, void *data)
diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index f7ca3cb..f6cec44 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -304,9 +304,6 @@ static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
 		 "SuperH Mobile CEU driver attached to camera %d\n",
 		 icd->devnum);
 
-	if (pcdev->pdata->enable_camera)
-		pcdev->pdata->enable_camera();
-
 	ret = icd->ops->init(icd);
 	if (ret)
 		goto err;
@@ -333,8 +330,6 @@ static void sh_mobile_ceu_remove_device(struct soc_camera_device *icd)
 	ceu_write(pcdev, CEIER, 0);
 	ceu_write(pcdev, CAPSR, 1 << 16); /* reset */
 	icd->ops->release(icd);
-	if (pcdev->pdata->disable_camera)
-		pcdev->pdata->disable_camera();
 
 	dev_info(&icd->dev,
 		 "SuperH Mobile CEU driver detached from camera %d\n",
diff --git a/include/asm-arm/arch-pxa/camera.h b/include/asm-arm/arch-pxa/camera.h
index 39516ce..31abe6d 100644
--- a/include/asm-arm/arch-pxa/camera.h
+++ b/include/asm-arm/arch-pxa/camera.h
@@ -36,8 +36,6 @@
 
 struct pxacamera_platform_data {
 	int (*init)(struct device *);
-	int (*power)(struct device *, int);
-	int (*reset)(struct device *, int);
 
 	unsigned long flags;
 	unsigned long mclk_10khz;
diff --git a/include/media/sh_mobile_ceu.h b/include/media/sh_mobile_ceu.h
index 234a471..b5dbefe 100644
--- a/include/media/sh_mobile_ceu.h
+++ b/include/media/sh_mobile_ceu.h
@@ -5,8 +5,6 @@
 
 struct sh_mobile_ceu_info {
 	unsigned long flags; /* SOCAM_... */
-	void (*enable_camera)(void);
-	void (*disable_camera)(void);
 };
 
 #endif /* __ASM_SH_MOBILE_CEU_H__ */
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index d548de3..c5de7bb 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -83,6 +83,9 @@ struct soc_camera_link {
 	int bus_id;
 	/* GPIO number to switch between 8 and 10 bit modes */
 	unsigned int gpio;
+	/* Optional callbacks to power on or off and reset the sensor */
+	int (*power)(struct device *, int);
+	int (*reset)(struct device *);
 };
 
 static inline struct soc_camera_device *to_soc_camera_dev(struct device *dev)

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> Make .power and .reset callbacks per camera instead of per host, also move 
> their invocation to camera drivers.
>
> Signed-off-by: Stefan Herbrechtsmeier <hbmeier@hni.uni-paderborn.de>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> On Fri, 1 Aug 2008, Stefan Herbrechtsmeier wrote:
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>> I'd rather preserve the possibility to use "soft" reset / poweroff also when
>>> a platform function is defined. In fact, it might be even better to do a
>>> soft power-off first and then call platform-provided one. Don't think it
>>> would make much sense for reset though.
>>>   
>>>       
>> You are right, I'll change it with the next version.
>>     
>
> How about the version below? I didn't understand why you need extra .init 
> and .release calls, so, I removed them for now. I think, .init per host 
> and power-on / off per camera should be enough for all init / release 
> needs, don't you think so
I use the .init call for gpio_request and gpio_direction_output and the 
.release call for gpio_free.
I do that this way, because I think they belongs more to the camera.

The patch looks ok for me.

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Guennadi Liakhovetski]

On Thu, 14 Aug 2008, Stefan Herbrechtsmeier wrote:

> Guennadi Liakhovetski schrieb:
> > 
> > How about the version below? I didn't understand why you need extra .init
> > and .release calls, so, I removed them for now. I think, .init per host and
> > power-on / off per camera should be enough for all init / release needs,
> > don't you think so
> I use the .init call for gpio_request and gpio_direction_output and the
> .release call for gpio_free.
> I do that this way, because I think they belongs more to the camera.

What are these GPIOs? Are they interfacing to a specific camera, or do 
they belong to the camera-host interface? If they belong to a specific 
camera, then yes, it is logical to control them from the camera driver 
platform callbacks. But cannot you do this in .power? Just do the .init 
part on power-on and the .release part on power-off?

> The patch looks ok for me.

Well, if you cannot use it in this form without .init and .release then it 
can hardly be called "ok" for you:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Thu, 14 Aug 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> Guennadi Liakhovetski schrieb:
>>     
>>> How about the version below? I didn't understand why you need extra .init
>>> and .release calls, so, I removed them for now. I think, .init per host and
>>> power-on / off per camera should be enough for all init / release needs,
>>> don't you think so
>>>       
>> I use the .init call for gpio_request and gpio_direction_output and the
>> .release call for gpio_free.
>> I do that this way, because I think they belongs more to the camera.
>>     
>
> What are these GPIOs? Are they interfacing to a specific camera, or do 
> they belong to the camera-host interface? If they belong to a specific 
> camera, then yes, it is logical to control them from the camera driver 
> platform callbacks.
The GPIOs belongs to the camera. One GPIO for the camera reset input and 
one for the power down
input of the camera.
> But cannot you do this in .power? Just do the .init 
> part on power-on and the .release part on power-off?
>   
The driver need to control the power down input after power-off to hold 
the camera in power down mode.
Maybe I can free the gpio after power-off without changing the gpio 
value, but I think the use of .init and
.release call is better and more clearly. Do you think it is ok to free 
the gpio even if it must stay high?
>   
>> The patch looks ok for me.
>>     
>
> Well, if you cannot use it in this form without .init and .release then it 
> can hardly be called "ok" for you:-)
>   
I mean it is ok except the .init and .release part. ;-)

Regards
    Stefan

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Guennadi Liakhovetski]

On Thu, 14 Aug 2008, Stefan Herbrechtsmeier wrote:

> The GPIOs belongs to the camera. One GPIO for the camera reset input and one
> for the power down
> input of the camera.

Ok, then don't you want to keep the camera off even if the driver is not 
configured or not loaded at all? Why don't you configure those GPIOs in 
your platform code permanently, you can make it dependent on 
defined(CONFIG_...) || defined(CONFIG_..._MODULE), and then just let power 
and reset callbacks toggle them without reconfiguring them ever again?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] soc-camera: Move .power and .reset from soc_camera host to sensor driver

[Stefan Herbrechtsmeier]

Guennadi Liakhovetski schrieb:
> On Thu, 14 Aug 2008, Stefan Herbrechtsmeier wrote:
>
>   
>> The GPIOs belongs to the camera. One GPIO for the camera reset input and one
>> for the power down
>> input of the camera.
>>     
>
> Ok, then don't you want to keep the camera off even if the driver is not 
> configured or not loaded at all? Why don't you configure those GPIOs in 
> your platform code permanently, you can make it dependent on 
> defined(CONFIG_...) || defined(CONFIG_..._MODULE), and then just let power 
> and reset callbacks toggle them without reconfiguring them ever again?
>   
I move the GPIOs init code to platform code. Thus your patch is complete 
ok for me. ;-)

-- 
Dipl.-Ing. Stefan Herbrechtsmeier

Heinz Nixdorf Institute
University of Paderborn 
System and Circuit Technology 
Fürstenallee 11
D-33102 Paderborn (Germany)

office : F0.415
phone  : + 49 5251 - 60 6342
fax    : + 49 5251 - 60 6351

mailto : hbmeier@hni.upb.de

www    : http://wwwhni.upb.de/sct/mitarbeiter/hbmeier


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list