[PATCH 1/1] as3645a: Remove set_power() from platform data

[PATCH 1/1] as3645a: Remove set_power() from platform data

[Sakari Ailus]

The chip is typically powered constantly and no board uses the set_power()
callback. Remove it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/as3645a.c |   39 +++++++++------------------------------
 include/media/as3645a.h       |    1 -
 2 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/media/video/as3645a.c b/drivers/media/video/as3645a.c
index c4b0357..7454660 100644
--- a/drivers/media/video/as3645a.c
+++ b/drivers/media/video/as3645a.c
@@ -512,31 +512,6 @@ static int as3645a_setup(struct as3645a *flash)
 	return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
 }
 
-static int __as3645a_set_power(struct as3645a *flash, int on)
-{
-	int ret;
-
-	if (!on)
-		as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
-
-	if (flash->pdata->set_power) {
-		ret = flash->pdata->set_power(&flash->subdev, on);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (!on)
-		return 0;
-
-	ret = as3645a_setup(flash);
-	if (ret < 0) {
-		if (flash->pdata->set_power)
-			flash->pdata->set_power(&flash->subdev, 0);
-	}
-
-	return ret;
-}
-
 static int as3645a_set_power(struct v4l2_subdev *sd, int on)
 {
 	struct as3645a *flash = to_as3645a(sd);
@@ -545,9 +520,13 @@ static int as3645a_set_power(struct v4l2_subdev *sd, int on)
 	mutex_lock(&flash->power_lock);
 
 	if (flash->power_count == !on) {
-		ret = __as3645a_set_power(flash, !!on);
-		if (ret < 0)
-			goto done;
+		if (!on) {
+			as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
+		} else {
+			ret = as3645a_setup(flash);
+			if (ret < 0)
+				goto done;
+		}
 	}
 
 	flash->power_count += on ? 1 : -1;
@@ -675,7 +654,7 @@ static int as3645a_suspend(struct device *dev)
 	if (flash->power_count == 0)
 		return 0;
 
-	rval = __as3645a_set_power(flash, 0);
+	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
 
 	dev_dbg(&client->dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
 
@@ -692,7 +671,7 @@ static int as3645a_resume(struct device *dev)
 	if (flash->power_count == 0)
 		return 0;
 
-	rval = __as3645a_set_power(flash, 1);
+	rval = as3645a_setup(flash);
 
 	dev_dbg(&client->dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
 
diff --git a/include/media/as3645a.h b/include/media/as3645a.h
index 5075496..a83ab3a 100644
--- a/include/media/as3645a.h
+++ b/include/media/as3645a.h
@@ -57,7 +57,6 @@
  * @timeout_max:	Max flash timeout (us, <= AS3645A_FLASH_TIMEOUT_MAX)
  */
 struct as3645a_platform_data {
-	int (*set_power)(struct v4l2_subdev *subdev, int on);
 	unsigned int vref;
 	unsigned int peak;
 	bool ext_strobe;
-- 
1.7.2.5

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Wednesday 16 May 2012 06:12:49 Sakari Ailus wrote:
> The chip is typically powered constantly and no board uses the set_power()
> callback. Remove it.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/as3645a.c |   39 +++++++++----------------------------
>  include/media/as3645a.h       |    1 -
>  2 files changed, 9 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/video/as3645a.c b/drivers/media/video/as3645a.c
> index c4b0357..7454660 100644
> --- a/drivers/media/video/as3645a.c
> +++ b/drivers/media/video/as3645a.c
> @@ -512,31 +512,6 @@ static int as3645a_setup(struct as3645a *flash)
>  	return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
>  }
> 
> -static int __as3645a_set_power(struct as3645a *flash, int on)
> -{
> -	int ret;
> -
> -	if (!on)
> -		as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> -
> -	if (flash->pdata->set_power) {
> -		ret = flash->pdata->set_power(&flash->subdev, on);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (!on)
> -		return 0;
> -
> -	ret = as3645a_setup(flash);
> -	if (ret < 0) {
> -		if (flash->pdata->set_power)
> -			flash->pdata->set_power(&flash->subdev, 0);
> -	}
> -
> -	return ret;
> -}
> -
>  static int as3645a_set_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct as3645a *flash = to_as3645a(sd);
> @@ -545,9 +520,13 @@ static int as3645a_set_power(struct v4l2_subdev *sd,
> int on) mutex_lock(&flash->power_lock);
> 
>  	if (flash->power_count == !on) {
> -		ret = __as3645a_set_power(flash, !!on);
> -		if (ret < 0)
> -			goto done;
> +		if (!on) {
> +			as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> +		} else {
> +			ret = as3645a_setup(flash);
> +			if (ret < 0)
> +				goto done;
> +		}
>  	}
> 

If the chip is powered on constantly, why do we need a .s_power() subdev 
operation at all ?

>  	flash->power_count += on ? 1 : -1;
> @@ -675,7 +654,7 @@ static int as3645a_suspend(struct device *dev)
>  	if (flash->power_count == 0)
>  		return 0;
> 
> -	rval = __as3645a_set_power(flash, 0);
> +	rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> 
>  	dev_dbg(&client->dev, "Suspend %s\n", rval < 0 ? "failed" : "ok");
> 
> @@ -692,7 +671,7 @@ static int as3645a_resume(struct device *dev)
>  	if (flash->power_count == 0)
>  		return 0;
> 
> -	rval = __as3645a_set_power(flash, 1);
> +	rval = as3645a_setup(flash);
> 
>  	dev_dbg(&client->dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
> 
> diff --git a/include/media/as3645a.h b/include/media/as3645a.h
> index 5075496..a83ab3a 100644
> --- a/include/media/as3645a.h
> +++ b/include/media/as3645a.h
> @@ -57,7 +57,6 @@
>   * @timeout_max:	Max flash timeout (us, <= AS3645A_FLASH_TIMEOUT_MAX)
>   */
>  struct as3645a_platform_data {
> -	int (*set_power)(struct v4l2_subdev *subdev, int on);
>  	unsigned int vref;
>  	unsigned int peak;
>  	bool ext_strobe;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Sakari Ailus]

Hi Laurent,

On Wed, May 23, 2012 at 01:00:08PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Wednesday 16 May 2012 06:12:49 Sakari Ailus wrote:
> > The chip is typically powered constantly and no board uses the set_power()
> > callback. Remove it.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/video/as3645a.c |   39 +++++++++----------------------------
> >  include/media/as3645a.h       |    1 -
> >  2 files changed, 9 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/media/video/as3645a.c b/drivers/media/video/as3645a.c
> > index c4b0357..7454660 100644
> > --- a/drivers/media/video/as3645a.c
> > +++ b/drivers/media/video/as3645a.c
> > @@ -512,31 +512,6 @@ static int as3645a_setup(struct as3645a *flash)
> >  	return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
> >  }
> > 
> > -static int __as3645a_set_power(struct as3645a *flash, int on)
> > -{
> > -	int ret;
> > -
> > -	if (!on)
> > -		as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> > -
> > -	if (flash->pdata->set_power) {
> > -		ret = flash->pdata->set_power(&flash->subdev, on);
> > -		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> > -	if (!on)
> > -		return 0;
> > -
> > -	ret = as3645a_setup(flash);
> > -	if (ret < 0) {
> > -		if (flash->pdata->set_power)
> > -			flash->pdata->set_power(&flash->subdev, 0);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> >  static int as3645a_set_power(struct v4l2_subdev *sd, int on)
> >  {
> >  	struct as3645a *flash = to_as3645a(sd);
> > @@ -545,9 +520,13 @@ static int as3645a_set_power(struct v4l2_subdev *sd,
> > int on) mutex_lock(&flash->power_lock);
> > 
> >  	if (flash->power_count == !on) {
> > -		ret = __as3645a_set_power(flash, !!on);
> > -		if (ret < 0)
> > -			goto done;
> > +		if (!on) {
> > +			as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> > +		} else {
> > +			ret = as3645a_setup(flash);
> > +			if (ret < 0)
> > +				goto done;
> > +		}
> >  	}
> > 
> 
> If the chip is powered on constantly, why do we need a .s_power() subdev 
> operation at all ?

I don't know why was it there in the first place. Probably to make it easier
to use the driver on boards that required e.g. a regulator for the chip.

But typically they're connected to battery directly. The idle power
consumption is just some tens of µA.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Laurent Pinchart]

Hi Sakari,

On Wednesday 23 May 2012 14:19:51 Sakari Ailus wrote:
> On Wed, May 23, 2012 at 01:00:08PM +0200, Laurent Pinchart wrote:
> > On Wednesday 16 May 2012 06:12:49 Sakari Ailus wrote:
> > > The chip is typically powered constantly and no board uses the
> > > set_power() callback. Remove it.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > > 
> > >  drivers/media/video/as3645a.c |   39
> > >  +++++++++----------------------------
> > >  include/media/as3645a.h       |    1 -
> > >  2 files changed, 9 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/as3645a.c
> > > b/drivers/media/video/as3645a.c
> > > index c4b0357..7454660 100644
> > > --- a/drivers/media/video/as3645a.c
> > > +++ b/drivers/media/video/as3645a.c
> > > @@ -512,31 +512,6 @@ static int as3645a_setup(struct as3645a *flash)
> > > 
> > >  	return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0;
> > >  
> > >  }
> > > 
> > > -static int __as3645a_set_power(struct as3645a *flash, int on)
> > > -{
> > > -	int ret;
> > > -
> > > -	if (!on)
> > > -		as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> > > -
> > > -	if (flash->pdata->set_power) {
> > > -		ret = flash->pdata->set_power(&flash->subdev, on);
> > > -		if (ret < 0)
> > > -			return ret;
> > > -	}
> > > -
> > > -	if (!on)
> > > -		return 0;
> > > -
> > > -	ret = as3645a_setup(flash);
> > > -	if (ret < 0) {
> > > -		if (flash->pdata->set_power)
> > > -			flash->pdata->set_power(&flash->subdev, 0);
> > > -	}
> > > -
> > > -	return ret;
> > > -}
> > > -
> > > 
> > >  static int as3645a_set_power(struct v4l2_subdev *sd, int on)
> > >  {
> > >  
> > >  	struct as3645a *flash = to_as3645a(sd);
> > > 
> > > @@ -545,9 +520,13 @@ static int as3645a_set_power(struct v4l2_subdev
> > > *sd,
> > > int on) mutex_lock(&flash->power_lock);
> > > 
> > >  	if (flash->power_count == !on) {
> > > 
> > > -		ret = __as3645a_set_power(flash, !!on);
> > > -		if (ret < 0)
> > > -			goto done;
> > > +		if (!on) {
> > > +			as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
> > > +		} else {
> > > +			ret = as3645a_setup(flash);
> > > +			if (ret < 0)
> > > +				goto done;
> > > +		}
> > > 
> > >  	}
> > 
> > If the chip is powered on constantly, why do we need a .s_power() subdev
> > operation at all ?
> 
> I don't know why was it there in the first place. Probably to make it easier
> to use the driver on boards that required e.g. a regulator for the chip.
> 
> But typically they're connected to battery directly. The idle power
> consumption is just some tens of µA.

What about on the N9 ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Sakari Ailus]

On Wed, May 23, 2012 at 01:31:26PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
...
> > > If the chip is powered on constantly, why do we need a .s_power() subdev
> > > operation at all ?
> > 
> > I don't know why was it there in the first place. Probably to make it easier
> > to use the driver on boards that required e.g. a regulator for the chip.
> > 
> > But typically they're connected to battery directly. The idle power
> > consumption is just some tens of µA.
> 
> What about on the N9 ?

That function pointer is NULL for N9. I used to configure the GPIOs but that
was wrong in the first place.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Sakari Ailus]

On Wed, May 23, 2012 at 03:06:41PM +0300, Sakari Ailus wrote:
> On Wed, May 23, 2012 at 01:31:26PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> ...
> > > > If the chip is powered on constantly, why do we need a .s_power() subdev
> > > > operation at all ?
> > > 
> > > I don't know why was it there in the first place. Probably to make it easier
> > > to use the driver on boards that required e.g. a regulator for the chip.
> > > 
> > > But typically they're connected to battery directly. The idle power
> > > consumption is just some tens of µA.
> > 
> > What about on the N9 ?
> 
> That function pointer is NULL for N9. I used to configure the GPIOs but that
> was wrong in the first place.

Ping.

Should we either remove the s_power() callback altogether or just the
platform data callback function (which is unused)?

It is indeed possible that the device was powered from a regulator which
isn't always on but we don't have such use cases right now.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/1] as3645a: Remove set_power() from platform data

[Laurent Pinchart]

Hi Sakari,

On Saturday 02 November 2013 23:43:02 Sakari Ailus wrote:
> On Wed, May 23, 2012 at 03:06:41PM +0300, Sakari Ailus wrote:
> > On Wed, May 23, 2012 at 01:31:26PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > 
> > ...
> > 
> > > > > If the chip is powered on constantly, why do we need a .s_power()
> > > > > subdev
> > > > > operation at all ?
> > > > 
> > > > I don't know why was it there in the first place. Probably to make it
> > > > easier to use the driver on boards that required e.g. a regulator for
> > > > the chip.
> > > > 
> > > > But typically they're connected to battery directly. The idle power
> > > > consumption is just some tens of µA.
> > > 
> > > What about on the N9 ?
> > 
> > That function pointer is NULL for N9. I used to configure the GPIOs but
> > that was wrong in the first place.
> 
> Ping.
> 
> Should we either remove the s_power() callback altogether or just the
> platform data callback function (which is unused)?
> 
> It is indeed possible that the device was powered from a regulator which
> isn't always on but we don't have such use cases right now.

I would remove the platform callback only. The s_power() function currently 
turns the torch when called to disable power, which is a sane thing to do. 
Your patch moves that to the call sites, but I believe it would be easier to 
keep the current __as3645a_set_power() function, especially if we later need 
to add support for regulators. Would that be fine with you ?

-- 
Regards,

Laurent Pinchart