[PATCH] adp1653: make ->power() method optional

[PATCH] adp1653: make ->power() method optional

[Andy Shevchenko]

The ->power() could be absent or not used on some platforms. This patch makes
its presence optional.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/adp1653.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
index 0fd9579..65f6f3f 100644
--- a/drivers/media/video/adp1653.c
+++ b/drivers/media/video/adp1653.c
@@ -309,6 +309,9 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
 	int ret;
 
+	if (flash->platform_data->power == NULL)
+		return 0;
+
 	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
-- 
1.7.5.4

Re: [PATCH] adp1653: make ->power() method optional

[Sakari Ailus]

On Thu, Aug 18, 2011 at 11:53:03AM +0300, Andy Shevchenko wrote:
> The ->power() could be absent or not used on some platforms. This patch makes
> its presence optional.

Hi Andy,

Thanks for the patch!

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/adp1653.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> index 0fd9579..65f6f3f 100644
> --- a/drivers/media/video/adp1653.c
> +++ b/drivers/media/video/adp1653.c
> @@ -309,6 +309,9 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
>  	int ret;
>  
> +	if (flash->platform_data->power == NULL)
> +		return 0;
> +
>  	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
> -- 
> 1.7.5.4
> 

How about doing this in adp1653_set_power() instead of
__adp1653_set_power()? At least I don't see any ill effects from this.
There's no need to keep track of the power state (flash->power_count) if
there isn't one. :-)

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] adp1653: make ->power() method optional

[Andy Shevchenko]

On Thu, 2011-08-18 at 12:21 +0300, Sakari Ailus wrote: 
> On Thu, Aug 18, 2011 at 11:53:03AM +0300, Andy Shevchenko wrote:
> > The ->power() could be absent or not used on some platforms. This patch makes
> > its presence optional.
> 
> Hi Andy,
> 
> Thanks for the patch!
> 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/video/adp1653.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > index 0fd9579..65f6f3f 100644
> > --- a/drivers/media/video/adp1653.c
> > +++ b/drivers/media/video/adp1653.c
> > @@ -309,6 +309,9 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
> >  {
> >  	int ret;
> >  
> > +	if (flash->platform_data->power == NULL)
> > +		return 0;
> > +
> >  	ret = flash->platform_data->power(&flash->subdev, on);
> >  	if (ret < 0)
> >  		return ret;

> How about doing this in adp1653_set_power() instead of
> __adp1653_set_power()? At least I don't see any ill effects from this.
> There's no need to keep track of the power state (flash->power_count) if
> there isn't one. :-)
It was my first assumption. However, the __adp1653_set_power() is used
directly from suspend/resume methods.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] adp1653: make ->power() method optional

[Sakari Ailus]

On Thu, Aug 18, 2011 at 01:30:50PM +0300, Andy Shevchenko wrote:
> On Thu, 2011-08-18 at 12:21 +0300, Sakari Ailus wrote: 
> > On Thu, Aug 18, 2011 at 11:53:03AM +0300, Andy Shevchenko wrote:
> > > The ->power() could be absent or not used on some platforms. This patch makes
> > > its presence optional.
> > 
> > Hi Andy,
> > 
> > Thanks for the patch!
> > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > >  drivers/media/video/adp1653.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > > index 0fd9579..65f6f3f 100644
> > > --- a/drivers/media/video/adp1653.c
> > > +++ b/drivers/media/video/adp1653.c
> > > @@ -309,6 +309,9 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
> > >  {
> > >  	int ret;
> > >  
> > > +	if (flash->platform_data->power == NULL)
> > > +		return 0;
> > > +
> > >  	ret = flash->platform_data->power(&flash->subdev, on);
> > >  	if (ret < 0)
> > >  		return ret;
> 
> > How about doing this in adp1653_set_power() instead of
> > __adp1653_set_power()? At least I don't see any ill effects from this.
> > There's no need to keep track of the power state (flash->power_count) if
> > there isn't one. :-)
> It was my first assumption. However, the __adp1653_set_power() is used
> directly from suspend/resume methods.

It is but it won't get called: power_count will be always zero when the
power() callback doesn't exist.

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] adp1653: make ->power() method optional

[Andy Shevchenko]

On Thu, 2011-08-18 at 13:53 +0300, Sakari Ailus wrote: 
> On Thu, Aug 18, 2011 at 01:30:50PM +0300, Andy Shevchenko wrote:
> > On Thu, 2011-08-18 at 12:21 +0300, Sakari Ailus wrote: 
> > > On Thu, Aug 18, 2011 at 11:53:03AM +0300, Andy Shevchenko wrote:
> > > > The ->power() could be absent or not used on some platforms. This patch makes
> > > > its presence optional.
> > > 
> > > Hi Andy,
> > > 
> > > Thanks for the patch!
> > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > >  drivers/media/video/adp1653.c |    3 +++
> > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > > > index 0fd9579..65f6f3f 100644
> > > > --- a/drivers/media/video/adp1653.c
> > > > +++ b/drivers/media/video/adp1653.c
> > > > @@ -309,6 +309,9 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
> > > >  {
> > > >  	int ret;
> > > >  
> > > > +	if (flash->platform_data->power == NULL)
> > > > +		return 0;
> > > > +
> > > >  	ret = flash->platform_data->power(&flash->subdev, on);
> > > >  	if (ret < 0)
> > > >  		return ret;
> > 
> > > How about doing this in adp1653_set_power() instead of
> > > __adp1653_set_power()? At least I don't see any ill effects from this.
> > > There's no need to keep track of the power state (flash->power_count) if
> > > there isn't one. :-)
> > It was my first assumption. However, the __adp1653_set_power() is used
> > directly from suspend/resume methods.
> 
> It is but it won't get called: power_count will be always zero when the
> power() callback doesn't exist.

Ah, now I got the full picture. Yes, if we leave adp1653_set_power()
immediately, then power_count stays 0.

I will send patch v2 soon.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCHv2] adp1653: make ->power() method optional

[Andy Shevchenko]

The ->power() could be absent or not used on some platforms. This patch makes
its presence optional.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/adp1653.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
index 0fd9579..f830313 100644
--- a/drivers/media/video/adp1653.c
+++ b/drivers/media/video/adp1653.c
@@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
 	struct adp1653_flash *flash = to_adp1653_flash(subdev);
 	int ret = 0;
 
+	/* There is no need to switch power in case of absence ->power()
+	 * method. */
+	if (flash->platform_data->power == NULL)
+		return 0;
+
 	mutex_lock(&flash->power_lock);
 
 	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
-- 
1.7.5.4

Re: [PATCHv2] adp1653: make ->power() method optional

[Andy Shevchenko]

On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote: 
> The ->power() could be absent or not used on some platforms. This patch makes
> its presence optional.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/video/adp1653.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> index 0fd9579..f830313 100644
> --- a/drivers/media/video/adp1653.c
> +++ b/drivers/media/video/adp1653.c
> @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
>  	struct adp1653_flash *flash = to_adp1653_flash(subdev);
>  	int ret = 0;
>  
> +	/* There is no need to switch power in case of absence ->power()
> +	 * method. */
> +	if (flash->platform_data->power == NULL)
> +		return 0;
> +
>  	mutex_lock(&flash->power_lock);
>  
>  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,

He-h, I guess you are not going to apply this one.
The patch breaks init logic of the device. If we have no ->power(), we
still need to bring the device to the known state. I have no good idea
how to do this.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
> On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote: 
> > The ->power() could be absent or not used on some platforms. This patch makes
> > its presence optional.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > ---
> >  drivers/media/video/adp1653.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > index 0fd9579..f830313 100644
> > --- a/drivers/media/video/adp1653.c
> > +++ b/drivers/media/video/adp1653.c
> > @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
> >  	struct adp1653_flash *flash = to_adp1653_flash(subdev);
> >  	int ret = 0;
> >  
> > +	/* There is no need to switch power in case of absence ->power()
> > +	 * method. */
> > +	if (flash->platform_data->power == NULL)
> > +		return 0;
> > +
> >  	mutex_lock(&flash->power_lock);
> >  
> >  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> 
> He-h, I guess you are not going to apply this one.
> The patch breaks init logic of the device. If we have no ->power(), we
> still need to bring the device to the known state. I have no good idea
> how to do this.

I don't think it breaks anything actually. Albeit in practice one is still
likely to put the adp1653 reset line to the board since that lowers its power
consumption significantly.

Instead of being in power-up state after opening the flash subdev, it will
reach this state already when the system is powered up. At subdev open all
the relevant registers are written to anyway, so I don't see an issue here.

I think either this one, or one should check in probe() that the power()
callback is non-NULL.

The board code is going away in the near future so this callback will
disappear eventually anyway. The gpio code in the board file should likely
be moved to the driver itself. That assumes that there will be a gpio which
can be used to enable and disable the device and I'm not fully certain this
is generic enough. Hopefully it is, but I don't know where else the adp1653
would be used than on the N900.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Andy Shevchenko]

On Thu, 2011-08-18 at 14:51 +0300, Sakari Ailus wrote: 
> On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
> > On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote: 
> > > The ->power() could be absent or not used on some platforms. This patch makes
> > > its presence optional.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > ---
> > >  drivers/media/video/adp1653.c |    5 +++++
> > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > > index 0fd9579..f830313 100644
> > > --- a/drivers/media/video/adp1653.c
> > > +++ b/drivers/media/video/adp1653.c
> > > @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
> > >  	struct adp1653_flash *flash = to_adp1653_flash(subdev);
> > >  	int ret = 0;
> > >  
> > > +	/* There is no need to switch power in case of absence ->power()
> > > +	 * method. */
> > > +	if (flash->platform_data->power == NULL)
> > > +		return 0;
> > > +
> > >  	mutex_lock(&flash->power_lock);
> > >  
> > >  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > 
> > He-h, I guess you are not going to apply this one.
> > The patch breaks init logic of the device. If we have no ->power(), we
> > still need to bring the device to the known state. I have no good idea
> > how to do this.
> 
> I don't think it breaks anything actually. Albeit in practice one is still
> likely to put the adp1653 reset line to the board since that lowers its power
> consumption significantly.
Yeah, even in practice we might see various ways of a chip connection.

> Instead of being in power-up state after opening the flash subdev, it will
> reach this state already when the system is powered up. At subdev open all
> the relevant registers are written to anyway, so I don't see an issue here.
You mean at first writing to the V4L2 value, do you? Because ->open()
uses set_power() which will be skipped in case of no ->power method
defined.

> I think either this one, or one should check in probe() that the power()
> callback is non-NULL.
> The board code is going away in the near future so this callback will
> disappear eventually anyway.
So, it's up to you to include or not my last patch.

> The gpio code in the board file should likely
> be moved to the driver itself.
The line could be different, the hw could be used in environment w/o
gpio, but with (for example) external gate, and so on. I think current
generic driver is pretty okay. 

And what to do with limits? Pass them as the module parameters?

> That assumes that there will be a gpio which
> can be used to enable and disable the device and I'm not fully certain this
> is generic enough. Hopefully it is, but I don't know where else the adp1653
> would be used than on the N900.
Don't narrow a chip application to the one device.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

On Thu, Aug 18, 2011 at 04:32:21PM +0300, Andy Shevchenko wrote:
> On Thu, 2011-08-18 at 14:51 +0300, Sakari Ailus wrote: 
> > On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
> > > On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote: 
> > > > The ->power() could be absent or not used on some platforms. This patch makes
> > > > its presence optional.
> > > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > >  drivers/media/video/adp1653.c |    5 +++++
> > > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > > > index 0fd9579..f830313 100644
> > > > --- a/drivers/media/video/adp1653.c
> > > > +++ b/drivers/media/video/adp1653.c
> > > > @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
> > > >  	struct adp1653_flash *flash = to_adp1653_flash(subdev);
> > > >  	int ret = 0;
> > > >  
> > > > +	/* There is no need to switch power in case of absence ->power()
> > > > +	 * method. */
> > > > +	if (flash->platform_data->power == NULL)
> > > > +		return 0;
> > > > +
> > > >  	mutex_lock(&flash->power_lock);
> > > >  
> > > >  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > > 
> > > He-h, I guess you are not going to apply this one.
> > > The patch breaks init logic of the device. If we have no ->power(), we
> > > still need to bring the device to the known state. I have no good idea
> > > how to do this.
> > 
> > I don't think it breaks anything actually. Albeit in practice one is still
> > likely to put the adp1653 reset line to the board since that lowers its power
> > consumption significantly.
> Yeah, even in practice we might see various ways of a chip connection.
> 
> > Instead of being in power-up state after opening the flash subdev, it will
> > reach this state already when the system is powered up. At subdev open all
> > the relevant registers are written to anyway, so I don't see an issue here.
> You mean at first writing to the V4L2 value, do you? Because ->open()
> uses set_power() which will be skipped in case of no ->power method
> defined.

Oh. I missed that adp1653_init_device() is being called in
__adp1653_set_power(). That's quite important. It still needs to be called,
at least once between probe() and first open().

I'm beginning to think we should require power() callback and fail in
probe() if it doesn't exist, or directly make it a gpio.

My plan is to get the N900 board code with the rest of the subdev drivers to
mainline at some point but that will likely take quite a bit of calendar
time, unfortunately. The adp1653 driver and the flash interface was just a
first part of it.

> > I think either this one, or one should check in probe() that the power()
> > callback is non-NULL.
> > The board code is going away in the near future so this callback will
> > disappear eventually anyway.
> So, it's up to you to include or not my last patch.
> 
> > The gpio code in the board file should likely
> > be moved to the driver itself.
> The line could be different, the hw could be used in environment w/o
> gpio, but with (for example) external gate, and so on. I think current
> generic driver is pretty okay. 
> 
> And what to do with limits? Pass them as the module parameters?

Limits still do belong to platform data. They are board dependent. Bad
limits may cause the all-important smoke to escape from the board. :-)

> > That assumes that there will be a gpio which
> > can be used to enable and disable the device and I'm not fully certain this
> > is generic enough. Hopefully it is, but I don't know where else the adp1653
> > would be used than on the N900.
> Don't narrow a chip application to the one device.

It's not useful to generalise this until it is useful for something. I
assume the chip will be connected to a gpio in practice. If there are
exceptions then we need to think again what to do.

Board code which contains functions is a problem in general since it cannot
be meaningfully converted to the device tree which can only contain static
data. This is an issue for complex systems such as digital cameras in
general, I hope a means to have board code will be left even after the
device tree conversion.

Regrads,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

On Thu, Aug 18, 2011 at 04:55:56PM +0300, Sakari Ailus wrote:
[clip]
> I'm beginning to think we should require power() callback and fail in
> probe() if it doesn't exist, or directly make it a gpio.
> 
> My plan is to get the N900 board code with the rest of the subdev drivers to
> mainline at some point but that will likely take quite a bit of calendar
> time, unfortunately. The adp1653 driver and the flash interface was just a
> first part of it.

FYI: the code is available here:

	git://gitorious.org/omap3camera/mainline.git rx51-005/002-devel

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Sylwester Nawrocki]

Hi,

On 08/18/2011 03:32 PM, Andy Shevchenko wrote:
> On Thu, 2011-08-18 at 14:51 +0300, Sakari Ailus wrote:
>> On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
>>> On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote:
>>>> The ->power() could be absent or not used on some platforms. This patch makes
>>>> its presence optional.
>>>>
>>>> Signed-off-by: Andy Shevchenko<andriy.shevchenko@linux.intel.com>
>>>> Cc: Sakari Ailus<sakari.ailus@iki.fi>
>>>> ---
>>>>   drivers/media/video/adp1653.c |    5 +++++
>>>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
>>>> index 0fd9579..f830313 100644
>>>> --- a/drivers/media/video/adp1653.c
>>>> +++ b/drivers/media/video/adp1653.c
>>>> @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
>>>>   	struct adp1653_flash *flash = to_adp1653_flash(subdev);
>>>>   	int ret = 0;
>>>>
>>>> +	/* There is no need to switch power in case of absence ->power()
>>>> +	 * method. */
>>>> +	if (flash->platform_data->power == NULL)
>>>> +		return 0;
>>>> +
>>>>   	mutex_lock(&flash->power_lock);
>>>>
>>>>   	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
>>>
>>> He-h, I guess you are not going to apply this one.
>>> The patch breaks init logic of the device. If we have no ->power(), we
>>> still need to bring the device to the known state. I have no good idea
>>> how to do this.
>>
>> I don't think it breaks anything actually. Albeit in practice one is still
>> likely to put the adp1653 reset line to the board since that lowers its power
>> consumption significantly.
> Yeah, even in practice we might see various ways of a chip connection.
> 
>> Instead of being in power-up state after opening the flash subdev, it will
>> reach this state already when the system is powered up. At subdev open all
>> the relevant registers are written to anyway, so I don't see an issue here.
> You mean at first writing to the V4L2 value, do you? Because ->open()
> uses set_power() which will be skipped in case of no ->power method
> defined.
> 
>> I think either this one, or one should check in probe() that the power()
>> callback is non-NULL.
>> The board code is going away in the near future so this callback will
>> disappear eventually anyway.
> So, it's up to you to include or not my last patch.
> 
>> The gpio code in the board file should likely
>> be moved to the driver itself.
> The line could be different, the hw could be used in environment w/o
> gpio, but with (for example) external gate, and so on. I think current
> generic driver is pretty okay.

Would it make sense to use the regulator API in place of the platform_data
callback? If there is only one GPIO then it's easy to create a 'fixed voltage
regulator' for this.

Does the 'platform_data->power' callback control power supply on pin 14 (VDD)
or does it do something else?

Also, what do you mean by an external gate?

> 
> And what to do with limits? Pass them as the module parameters?
> 
>> That assumes that there will be a gpio which
>> can be used to enable and disable the device and I'm not fully certain this
>> is generic enough. Hopefully it is, but I don't know where else the adp1653
>> would be used than on the N900.
> Don't narrow a chip application to the one device.

We don't want this, but on the other hand there is a need to replace custom
callbacks in driver's platform_data with something else.

--
Regards,
Sylwester

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

Sylwester Nawrocki wrote:
> Hi,

Hi Sylwester,

> On 08/18/2011 03:32 PM, Andy Shevchenko wrote:
>> On Thu, 2011-08-18 at 14:51 +0300, Sakari Ailus wrote:
>>> On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
>>>> On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote:
>>>>> The ->power() could be absent or not used on some platforms. This patch makes
>>>>> its presence optional.
>>>>>
>>>>> Signed-off-by: Andy Shevchenko<andriy.shevchenko@linux.intel.com>
>>>>> Cc: Sakari Ailus<sakari.ailus@iki.fi>
>>>>> ---
>>>>>    drivers/media/video/adp1653.c |    5 +++++
>>>>>    1 files changed, 5 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
>>>>> index 0fd9579..f830313 100644
>>>>> --- a/drivers/media/video/adp1653.c
>>>>> +++ b/drivers/media/video/adp1653.c
>>>>> @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
>>>>>    	struct adp1653_flash *flash = to_adp1653_flash(subdev);
>>>>>    	int ret = 0;
>>>>>
>>>>> +	/* There is no need to switch power in case of absence ->power()
>>>>> +	 * method. */
>>>>> +	if (flash->platform_data->power == NULL)
>>>>> +		return 0;
>>>>> +
>>>>>    	mutex_lock(&flash->power_lock);
>>>>>
>>>>>    	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
>>>>
>>>> He-h, I guess you are not going to apply this one.
>>>> The patch breaks init logic of the device. If we have no ->power(), we
>>>> still need to bring the device to the known state. I have no good idea
>>>> how to do this.
>>>
>>> I don't think it breaks anything actually. Albeit in practice one is still
>>> likely to put the adp1653 reset line to the board since that lowers its power
>>> consumption significantly.
>> Yeah, even in practice we might see various ways of a chip connection.
>>
>>> Instead of being in power-up state after opening the flash subdev, it will
>>> reach this state already when the system is powered up. At subdev open all
>>> the relevant registers are written to anyway, so I don't see an issue here.
>> You mean at first writing to the V4L2 value, do you? Because ->open()
>> uses set_power() which will be skipped in case of no ->power method
>> defined.
>>
>>> I think either this one, or one should check in probe() that the power()
>>> callback is non-NULL.
>>> The board code is going away in the near future so this callback will
>>> disappear eventually anyway.
>> So, it's up to you to include or not my last patch.
>>
>>> The gpio code in the board file should likely
>>> be moved to the driver itself.
>> The line could be different, the hw could be used in environment w/o
>> gpio, but with (for example) external gate, and so on. I think current
>> generic driver is pretty okay.
>
> Would it make sense to use the regulator API in place of the platform_data
> callback? If there is only one GPIO then it's easy to create a 'fixed voltage
> regulator' for this.

I don't know the regulator framework very well, but do you mean creating 
a new regulator which just controls a gpio? It would be preferrable that 
this wouldn't create a new driver nor add any board core.

> Does the 'platform_data->power' callback control power supply on pin 14 (VDD)
> or does it do something else?

No. The chip is always powered on the N900 but pulling down (or up, I 
don't remember) its reset pin puts the chip to reset and causes the 
current draw to reach almost zero. I think it's in the class of some or 
few tens of µA. Someone still might implement a board containing the 
adp1653 which would require enabling a regulator for it.

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Sylwester Nawrocki]

On 08/18/2011 09:02 PM, Sakari Ailus wrote:
>>>>>
>>>>> He-h, I guess you are not going to apply this one.
>>>>> The patch breaks init logic of the device. If we have no ->power(), we
>>>>> still need to bring the device to the known state. I have no good idea
>>>>> how to do this.
>>>>
>>>> I don't think it breaks anything actually. Albeit in practice one is still
>>>> likely to put the adp1653 reset line to the board since that lowers its power
>>>> consumption significantly.
>>> Yeah, even in practice we might see various ways of a chip connection.
>>>
>>>> Instead of being in power-up state after opening the flash subdev, it will
>>>> reach this state already when the system is powered up. At subdev open all
>>>> the relevant registers are written to anyway, so I don't see an issue here.
>>> You mean at first writing to the V4L2 value, do you? Because ->open()
>>> uses set_power() which will be skipped in case of no ->power method
>>> defined.
>>>
>>>> I think either this one, or one should check in probe() that the power()
>>>> callback is non-NULL.
>>>> The board code is going away in the near future so this callback will
>>>> disappear eventually anyway.
>>> So, it's up to you to include or not my last patch.
>>>
>>>> The gpio code in the board file should likely
>>>> be moved to the driver itself.
>>> The line could be different, the hw could be used in environment w/o
>>> gpio, but with (for example) external gate, and so on. I think current
>>> generic driver is pretty okay.
>>
>> Would it make sense to use the regulator API in place of the platform_data
>> callback? If there is only one GPIO then it's easy to create a 'fixed voltage
>> regulator' for this.
> 
> I don't know the regulator framework very well, but do you mean creating a new
> regulator which just controls a gpio? It would be preferable that this wouldn't
> create a new driver nor add any board core.

I'm afraid your requirements are too demanding :)
Yes, I meant creating a new regulator. In case the ADP1635 voltage regulator
is inhibited through a GPIO at a host processor such regulator would in fact
be only flipping a GPIO (and its driver would request the GPIO and set it into
a default inactive state during its initialization). But the LDO for ADP1635
could be also a part of larger PMIC device, which are often configured through
I2C and have their generic drivers (in drivers/regulator). So the regulator API
in some extent abstracts the power supply details. There is a common API at the
client driver side regardless of the details how the power is actually enabled/
disabled.

I've seen some patches for the device tree bindings for the regulator API
but I guess this is not in the mainline yet.

Currently the 'fixed voltage regulator' is instantiated by creating a platform 
device, with an appropriate 'id', in the board code. And you have to create... 
a platform data for it :) The driver is common for all such devices
(drivers/regulator/fixed.c). 

> 
>> Does the 'platform_data->power' callback control power supply on pin 14 (VDD)
>> or does it do something else?
> 
> No. The chip is always powered on the N900 but pulling down (or up, I don't remember)
> its reset pin puts the chip to reset and causes the current draw to reach almost zero.
> I think it's in the class of some or few tens of µA. Someone still might implement

According to the datasheet it's even less, below 1 uA :)

"Shutdown Current (EN = GND, TJ = −40°C to +85°C) = 0.1μA (Typ.), 1μA (Max)"

So the reset (EN) pin is basically a GPIO, but it could be as well some signal
provided by a glue FPGA, driven by a memory mapped register(s).
Then the callback is most flexible, but it's a little bit ugly at the same time:)
If I were you I, would probably originally put a GPIO number in platform_data,
instead of a function callback. But that depends on priorities.

> a board containing the adp1653 which would require enabling a regulator for it.
 
Indeed, I guess there is no point in adding support for the power supply control
over the regulator API now. When someone needs it on some other board, it can
be added in the driver then.

--
Regards,
Sylwester

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

Sylwester Nawrocki wrote:
> On 08/18/2011 09:02 PM, Sakari Ailus wrote:
>>>>>>
>>>>>> He-h, I guess you are not going to apply this one.
>>>>>> The patch breaks init logic of the device. If we have no ->power(), we
>>>>>> still need to bring the device to the known state. I have no good idea
>>>>>> how to do this.
>>>>>
>>>>> I don't think it breaks anything actually. Albeit in practice one is still
>>>>> likely to put the adp1653 reset line to the board since that lowers its power
>>>>> consumption significantly.
>>>> Yeah, even in practice we might see various ways of a chip connection.
>>>>
>>>>> Instead of being in power-up state after opening the flash subdev, it will
>>>>> reach this state already when the system is powered up. At subdev open all
>>>>> the relevant registers are written to anyway, so I don't see an issue here.
>>>> You mean at first writing to the V4L2 value, do you? Because ->open()
>>>> uses set_power() which will be skipped in case of no ->power method
>>>> defined.
>>>>
>>>>> I think either this one, or one should check in probe() that the power()
>>>>> callback is non-NULL.
>>>>> The board code is going away in the near future so this callback will
>>>>> disappear eventually anyway.
>>>> So, it's up to you to include or not my last patch.
>>>>
>>>>> The gpio code in the board file should likely
>>>>> be moved to the driver itself.
>>>> The line could be different, the hw could be used in environment w/o
>>>> gpio, but with (for example) external gate, and so on. I think current
>>>> generic driver is pretty okay.
>>>
>>> Would it make sense to use the regulator API in place of the platform_data
>>> callback? If there is only one GPIO then it's easy to create a 'fixed voltage
>>> regulator' for this.
>>
>> I don't know the regulator framework very well, but do you mean creating a new
>> regulator which just controls a gpio? It would be preferable that this wouldn't
>> create a new driver nor add any board core.
> 
> I'm afraid your requirements are too demanding :)
> Yes, I meant creating a new regulator. In case the ADP1635 voltage regulator
> is inhibited through a GPIO at a host processor such regulator would in fact
> be only flipping a GPIO (and its driver would request the GPIO and set it into
> a default inactive state during its initialization). But the LDO for ADP1635

Thinking about this again, I think we'd need a regulator and reset gpio.
The reset line probably can't be really modelled as a power supply, as
the voltage provided to the chip is different from the reset line. Both
may exist on some boards.

The regulator might be a dummy one, too, as well as the reset line.

> could be also a part of larger PMIC device, which are often configured through
> I2C and have their generic drivers (in drivers/regulator). So the regulator API
> in some extent abstracts the power supply details. There is a common API at the
> client driver side regardless of the details how the power is actually enabled/
> disabled.
> 
> I've seen some patches for the device tree bindings for the regulator API
> but I guess this is not in the mainline yet.
> 
> Currently the 'fixed voltage regulator' is instantiated by creating a platform 
> device, with an appropriate 'id', in the board code. And you have to create... 
> a platform data for it :) The driver is common for all such devices
> (drivers/regulator/fixed.c). 

I'll take a look at that.

>>> Does the 'platform_data->power' callback control power supply on pin 14 (VDD)
>>> or does it do something else?
>>
>> No. The chip is always powered on the N900 but pulling down (or up, I don't remember)
>> its reset pin puts the chip to reset and causes the current draw to reach almost zero.
>> I think it's in the class of some or few tens of µA. Someone still might implement
> 
> According to the datasheet it's even less, below 1 uA :)

Right. That indeed was probably the reason there was no need for a
controllable regulator.

> "Shutdown Current (EN = GND, TJ = −40°C to +85°C) = 0.1μA (Typ.), 1μA (Max)"
> 
> So the reset (EN) pin is basically a GPIO, but it could be as well some signal
> provided by a glue FPGA, driven by a memory mapped register(s).
> Then the callback is most flexible, but it's a little bit ugly at the same time:)
> If I were you I, would probably originally put a GPIO number in platform_data,
> instead of a function callback. But that depends on priorities.
> 
>> a board containing the adp1653 which would require enabling a regulator for it.
>  
> Indeed, I guess there is no point in adding support for the power supply control
> over the regulator API now. When someone needs it on some other board, it can
> be added in the driver then.

I fully agree.

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

On Fri, Aug 19, 2011 at 07:16:59PM +0300, Sakari Ailus wrote:
[clip]
> > I'm afraid your requirements are too demanding :)
> > Yes, I meant creating a new regulator. In case the ADP1635 voltage regulator
> > is inhibited through a GPIO at a host processor such regulator would in fact
> > be only flipping a GPIO (and its driver would request the GPIO and set it into
> > a default inactive state during its initialization). But the LDO for ADP1635
> 
> Thinking about this again, I think we'd need a regulator and reset gpio.

And as noted below, the regulator isn't needed at this point.

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCHv2] adp1653: make ->power() method optional

[Sylwester Nawrocki]

On 08/19/2011 06:16 PM, Sakari Ailus wrote:
> Sylwester Nawrocki wrote:
>> On 08/18/2011 09:02 PM, Sakari Ailus wrote:
>>>>>>>
>>>>>>> He-h, I guess you are not going to apply this one.
>>>>>>> The patch breaks init logic of the device. If we have no ->power(), we
>>>>>>> still need to bring the device to the known state. I have no good idea
>>>>>>> how to do this.
>>>>>>
>>>>>> I don't think it breaks anything actually. Albeit in practice one is still
>>>>>> likely to put the adp1653 reset line to the board since that lowers its power
>>>>>> consumption significantly.
>>>>> Yeah, even in practice we might see various ways of a chip connection.
>>>>>
>>>>>> Instead of being in power-up state after opening the flash subdev, it will
>>>>>> reach this state already when the system is powered up. At subdev open all
>>>>>> the relevant registers are written to anyway, so I don't see an issue here.
>>>>> You mean at first writing to the V4L2 value, do you? Because ->open()
>>>>> uses set_power() which will be skipped in case of no ->power method
>>>>> defined.
>>>>>
>>>>>> I think either this one, or one should check in probe() that the power()
>>>>>> callback is non-NULL.
>>>>>> The board code is going away in the near future so this callback will
>>>>>> disappear eventually anyway.
>>>>> So, it's up to you to include or not my last patch.
>>>>>
>>>>>> The gpio code in the board file should likely
>>>>>> be moved to the driver itself.
>>>>> The line could be different, the hw could be used in environment w/o
>>>>> gpio, but with (for example) external gate, and so on. I think current
>>>>> generic driver is pretty okay.
>>>>
>>>> Would it make sense to use the regulator API in place of the platform_data
>>>> callback? If there is only one GPIO then it's easy to create a 'fixed voltage
>>>> regulator' for this.
>>>
>>> I don't know the regulator framework very well, but do you mean creating a new
>>> regulator which just controls a gpio? It would be preferable that this wouldn't
>>> create a new driver nor add any board core.
>>
>> I'm afraid your requirements are too demanding :)
>> Yes, I meant creating a new regulator. In case the ADP1635 voltage regulator
>> is inhibited through a GPIO at a host processor such regulator would in fact
>> be only flipping a GPIO (and its driver would request the GPIO and set it into
>> a default inactive state during its initialization). But the LDO for ADP1635
> 
> Thinking about this again, I think we'd need a regulator and reset gpio.
> The reset line probably can't be really modelled as a power supply, as
> the voltage provided to the chip is different from the reset line. Both
> may exist on some boards.
> 
> The regulator might be a dummy one, too, as well as the reset line. 

Yes, this would make the driver most complete I guess.
And the gpio API seems a natural choice for the reset signal. If there is
some 'non-standard' circuit to drive the ADP1635 pin possibly it can be
handled by some existing or dedicated gpio driver.


--
Regards,
Sylwester

Re: [PATCHv2] adp1653: make ->power() method optional

[Sakari Ailus]

Hi Andy,

On Thu, Aug 18, 2011 at 04:32:21PM +0300, Andy Shevchenko wrote:
> On Thu, 2011-08-18 at 14:51 +0300, Sakari Ailus wrote: 
> > On Thu, Aug 18, 2011 at 02:32:02PM +0300, Andy Shevchenko wrote:
> > > On Thu, 2011-08-18 at 14:22 +0300, Andy Shevchenko wrote: 
> > > > The ->power() could be absent or not used on some platforms. This patch makes
> > > > its presence optional.
> > > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > > ---
> > > >  drivers/media/video/adp1653.c |    5 +++++
> > > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/adp1653.c b/drivers/media/video/adp1653.c
> > > > index 0fd9579..f830313 100644
> > > > --- a/drivers/media/video/adp1653.c
> > > > +++ b/drivers/media/video/adp1653.c
> > > > @@ -329,6 +329,11 @@ adp1653_set_power(struct v4l2_subdev *subdev, int on)
> > > >  	struct adp1653_flash *flash = to_adp1653_flash(subdev);
> > > >  	int ret = 0;
> > > >  
> > > > +	/* There is no need to switch power in case of absence ->power()
> > > > +	 * method. */
> > > > +	if (flash->platform_data->power == NULL)
> > > > +		return 0;
> > > > +
> > > >  	mutex_lock(&flash->power_lock);
> > > >  
> > > >  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> > > 
> > > He-h, I guess you are not going to apply this one.
> > > The patch breaks init logic of the device. If we have no ->power(), we
> > > still need to bring the device to the known state. I have no good idea
> > > how to do this.
> > 
> > I don't think it breaks anything actually. Albeit in practice one is still
> > likely to put the adp1653 reset line to the board since that lowers its power
> > consumption significantly.
> Yeah, even in practice we might see various ways of a chip connection.

That's true. But I think the bottom line is that these should be modelled in
a generic way; the last resort is a board specific GPIO or regulator driver.

This is the way ARM platform is moving in Linux and the sooner we adapt to
that, the better. Otherwise we'll be in trouble. Of course we need to
actively make our concerns known for others.

Speaking of which, I don't think Intel (what comes to embedded CPUs) is
necessarily in a very different position than the ARM vendors; ARM is
exhibiting such symptoms not only because the vendors are very inventive
with the hardware but also simply because these systems are embedded
systems. Typically only few if any devices can be probed, for example, and
there are various means for interacting with things like flash controllers.
The same regulators and GPIOs are present there as well and the hardware
description must be somehow available to the drivers.

> > Instead of being in power-up state after opening the flash subdev, it will
> > reach this state already when the system is powered up. At subdev open all
> > the relevant registers are written to anyway, so I don't see an issue here.
> You mean at first writing to the V4L2 value, do you? Because ->open()
> uses set_power() which will be skipped in case of no ->power method
> defined.
> 
> > I think either this one, or one should check in probe() that the power()
> > callback is non-NULL.
> > The board code is going away in the near future so this callback will
> > disappear eventually anyway.
> So, it's up to you to include or not my last patch.

My opinion is that instead of checking the power callback, the platform data
needs to contain the GPIO number instead. The driver can then use the GPIO
framework to toggle it.

> 
> > The gpio code in the board file should likely
> > be moved to the driver itself.
> The line could be different, the hw could be used in environment w/o
> gpio, but with (for example) external gate, and so on. I think current
> generic driver is pretty okay. 
> 
> And what to do with limits? Pass them as the module parameters?
> 
> > That assumes that there will be a gpio which
> > can be used to enable and disable the device and I'm not fully certain this
> > is generic enough. Hopefully it is, but I don't know where else the adp1653
> > would be used than on the N900.
> Don't narrow a chip application to the one device.

We don't, but we also don't generalise something that has no use (yet).

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