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@xxxxxxxxxxxxxxx> >>>> Cc: Sakari Ailus<sakari.ailus@xxxxxx> >>>> --- >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
