| Message ID | 20141116075928.GA9763@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/16/2014 08:59 AM, Pavel Machek wrote: >[...] > + adp1653: adp1653@30 { > + compatible = "ad,adp1653"; The Analog Devices vendor prefix is adi.
On Sun 2014-11-16 09:11:04, Lars-Peter Clausen wrote: > On 11/16/2014 08:59 AM, Pavel Machek wrote: > >[...] > >+ adp1653: adp1653@30 { > >+ compatible = "ad,adp1653"; > > The Analog Devices vendor prefix is adi. Thanks, will be fixed in next version.
Hi Pavel, Am 16.11.2014 um 08:59 schrieb Pavel Machek: > For device tree people: Yes, I know I'll have to create file in > documentation, but does the binding below look acceptable? [...] > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > index 739fcf2..ed0bfc1 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -553,6 +561,18 @@ > > ti,usb-charger-detection = <&isp1704>; > }; > + > + adp1653: adp1653@30 { This should probably be led-controller@30 (a generic description not specific to the model). The label name is fine. > + compatible = "ad,adp1653"; > + reg = <0x30>; > + > + max-flash-timeout-usec = <500000>; > + max-flash-intensity-uA = <320000>; > + max-torch-intensity-uA = <50000>; > + max-indicator-intensity-uA = <17500>; > + > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* Want 88 */ At least to me, the meaning of "Want 88" is not clear - drop or clarify? > + }; > }; > > &i2c3 { [snip] Regards, Andreas
On Sun 2014-11-16 11:09:51, Andreas Färber wrote: > Hi Pavel, > > Am 16.11.2014 um 08:59 schrieb Pavel Machek: > > For device tree people: Yes, I know I'll have to create file in > > documentation, but does the binding below look acceptable? > [...] > > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > > index 739fcf2..ed0bfc1 100644 > > --- a/arch/arm/boot/dts/omap3-n900.dts > > +++ b/arch/arm/boot/dts/omap3-n900.dts > > @@ -553,6 +561,18 @@ > > > > ti,usb-charger-detection = <&isp1704>; > > }; > > + > > + adp1653: adp1653@30 { > > This should probably be led-controller@30 (a generic description not > specific to the model). The label name is fine. Thanks. > > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* Want 88 */ > > At least to me, the meaning of "Want 88" is not clear - drop or clarify? I changed it to /* 88 */ to match rest of code. Best regards, Pavel
On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > For device tree people: Yes, I know I'll have to create file > in documentation, but does the binding below look acceptable? > > I'll clean up driver code a bit more, remove the printks. > Anything else obviously wrong? > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > Thanks, > Pavel > > Hello, I think that this patch is probably not good and specially not for n900. adp1653 should be registered throw omap3 isp camera subsystem which does not have DT support yet. See n900 legacy board camera code in file board-rx51-camera.c.
Hi! On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > For device tree people: Yes, I know I'll have to create file > > in documentation, but does the binding below look acceptable? > > > > I'll clean up driver code a bit more, remove the printks. > > Anything else obviously wrong? > I think that this patch is probably not good and specially not > for n900. adp1653 should be registered throw omap3 isp camera > subsystem which does not have DT support yet. Can you explain? adp1653 is independend device on i2c bus, and we have kernel driver for it (unlike rest of n900 camera system). Just now it is unusable due to lack of DT binding. It has two functions, LED light and a camera flash; yes, the second one should be integrated to the rest of camera system, but that is not yet merged. That should not prevent us from merging DT support for the flash, so that this part can be tested/maintained. > See n900 legacy board camera code in file board-rx51-camera.c. I have seen that. Pavel
On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > Hi! > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > For device tree people: Yes, I know I'll have to create > > > file in documentation, but does the binding below look > > > acceptable? > > > > > > I'll clean up driver code a bit more, remove the printks. > > > Anything else obviously wrong? > > > > I think that this patch is probably not good and specially > > not for n900. adp1653 should be registered throw omap3 isp > > camera subsystem which does not have DT support yet. > > Can you explain? > > adp1653 is independend device on i2c bus, and we have kernel > driver for it (unlike rest of n900 camera system). Just now > it is unusable due to lack of DT binding. It has two > functions, LED light and a camera flash; yes, the second one > should be integrated to the rest of camera system, but that > is not yet merged. That should not prevent us from merging DT > support for the flash, so that this part can be > tested/maintained. > Ok. When ISP camera subsystem has DT support somebody will modify n900 DT to add camera flash from adp1653 to ISP... I believe it will not be hard. > > See n900 legacy board camera code in file > > board-rx51-camera.c. > > I have seen that. > Pavel
On Mon 2014-11-17 11:09:45, Pali Rohár wrote: > On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > > Hi! > > > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > > For device tree people: Yes, I know I'll have to create > > > > file in documentation, but does the binding below look > > > > acceptable? > > > > > > > > I'll clean up driver code a bit more, remove the printks. > > > > Anything else obviously wrong? > > > > > > I think that this patch is probably not good and specially > > > not for n900. adp1653 should be registered throw omap3 isp > > > camera subsystem which does not have DT support yet. > > > > Can you explain? > > > > adp1653 is independend device on i2c bus, and we have kernel > > driver for it (unlike rest of n900 camera system). Just now > > it is unusable due to lack of DT binding. It has two > > functions, LED light and a camera flash; yes, the second one > > should be integrated to the rest of camera system, but that > > is not yet merged. That should not prevent us from merging DT > > support for the flash, so that this part can be > > tested/maintained. > > > > Ok. When ISP camera subsystem has DT support somebody will modify > n900 DT to add camera flash from adp1653 to ISP... I believe it > will not be hard. Exactly. And yes, I'd like to get complete camera support for n900 merged. But first step is "make sure existing support does not break". Best regards, Pavel
* Pavel Machek <pavel@ucw.cz> [141117 02:17]: > On Mon 2014-11-17 11:09:45, Pali Rohár wrote: > > On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > > > Hi! > > > > > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > > > For device tree people: Yes, I know I'll have to create > > > > > file in documentation, but does the binding below look > > > > > acceptable? > > > > > > > > > > I'll clean up driver code a bit more, remove the printks. > > > > > Anything else obviously wrong? > > > > > > > > I think that this patch is probably not good and specially > > > > not for n900. adp1653 should be registered throw omap3 isp > > > > camera subsystem which does not have DT support yet. > > > > > > Can you explain? > > > > > > adp1653 is independend device on i2c bus, and we have kernel > > > driver for it (unlike rest of n900 camera system). Just now > > > it is unusable due to lack of DT binding. It has two > > > functions, LED light and a camera flash; yes, the second one > > > should be integrated to the rest of camera system, but that > > > is not yet merged. That should not prevent us from merging DT > > > support for the flash, so that this part can be > > > tested/maintained. > > > > > > > Ok. When ISP camera subsystem has DT support somebody will modify > > n900 DT to add camera flash from adp1653 to ISP... I believe it > > will not be hard. > > Exactly. And yes, I'd like to get complete camera support for n900 > merged. But first step is "make sure existing support does not break". There's nothing stopping us from initializing the camera code from pdata-quirks.c for now to keep it working. Certainly the binding should be added to the driver, but that removes a dependency to the legacy booting mode if things are otherwise working. Regards, Tony
Hi Pavel, On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote: > For device tree people: Yes, I know I'll have to create file in > documentation, but does the binding below look acceptable? > > I'll clean up driver code a bit more, remove the printks. Anything > else obviously wrong? Jacek Anaszewski is working on flash support for LED devices. I think it'd be good to sync the DT bindings for the two, as the types of devices supported by the LED API and the V4L2 flash API are quite similar. Cc Jacek. > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > Thanks, > Pavel > > > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > index 739fcf2..ed0bfc1 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -553,6 +561,18 @@ > > ti,usb-charger-detection = <&isp1704>; > }; > + > + adp1653: adp1653@30 { > + compatible = "ad,adp1653"; > + reg = <0x30>; > + > + max-flash-timeout-usec = <500000>; > + max-flash-intensity-uA = <320000>; > + max-torch-intensity-uA = <50000>; > + max-indicator-intensity-uA = <17500>; > + > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* Want 88 */ > + }; > }; > > &i2c3 { > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 873fe19..e21ed02 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -8,6 +8,7 @@ > * Contributors: > * Sakari Ailus <sakari.ailus@iki.fi> > * Tuukka Toivonen <tuukkat76@gmail.com> > + * Pavel Machek <pavel@ucw.cz> > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -34,9 +35,12 @@ > #include <linux/module.h> > #include <linux/i2c.h> > #include <linux/slab.h> > +#include <linux/of_gpio.h> > #include <media/adp1653.h> > #include <media/v4l2-device.h> > > +#include <linux/gpio.h> > + > #define TIMEOUT_MAX 820000 > #define TIMEOUT_STEP 54600 > #define TIMEOUT_MIN (TIMEOUT_MAX - ADP1653_REG_CONFIG_TMR_SET_MAX \ > @@ -308,7 +316,16 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > { > int ret; > > - ret = flash->platform_data->power(&flash->subdev, on); > + if (flash->platform_data->power) > + ret = flash->platform_data->power(&flash->subdev, on); > + else { > + gpio_set_value(flash->platform_data->power_gpio, on); > + if (on) { > + /* Some delay is apparently required. */ > + udelay(20); > + } > + } > + > if (ret < 0) > return ret; > > @@ -316,8 +333,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > return 0; > > ret = adp1653_init_device(flash); > - if (ret < 0) > + if (ret >= 0) > + return ret; > + > + if (flash->platform_data->power) > flash->platform_data->power(&flash->subdev, 0); > + else > + gpio_set_value(flash->platform_data->power_gpio, 0); > > return ret; > } > @@ -407,21 +429,87 @@ static int adp1653_resume(struct device *dev) > > #endif /* CONFIG_PM */ > > + > + > + > + > + > + > + > + > + > + > + > + > + > +static int adp1653_of_init(struct i2c_client *client, struct adp1653_flash *flash, > + struct device_node *node) > +{ > + u32 val; > + struct adp1653_platform_data *pd; > + enum of_gpio_flags flags; > + int gpio; > + > + if (!node) > + return -EINVAL; > + > + printk("adp1653: no platform data\n"); > + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); > + if (!pd) > + return -ENOMEM; > + flash->platform_data = pd; > + > + > + > + > + > + > + > + if (of_property_read_u32(node, "max-flash-timeout-usec", &val)) return -EINVAL; > + pd->max_flash_timeout = val; > + if (of_property_read_u32(node, "max-flash-intensity-uA", &val)) return -EINVAL; > + pd->max_flash_intensity = val/1000; > + if (of_property_read_u32(node, "max-torch-intensity-uA", &val)) return -EINVAL; > + pd->max_torch_intensity = val/1000; > + if (of_property_read_u32(node, "max-indicator-intensity-uA", &val)) return -EINVAL; > + pd->max_indicator_intensity = val; > + > + if (!of_find_property(node, "gpios", NULL)) { > + printk("No gpio node\n"); > + return -EINVAL; > + } > + > + gpio = of_get_gpio_flags(node, 0, &flags); > + if (gpio < 0) { > + printk("Error getting GPIO\n"); > + return -EINVAL; > + } > + > + pd->power_gpio = gpio; > + > + return 0; > +} > + > + > static int adp1653_probe(struct i2c_client *client, > const struct i2c_device_id *devid) > { > struct adp1653_flash *flash; > int ret; > > - /* we couldn't work without platform data */ > - if (client->dev.platform_data == NULL) > - return -ENODEV; > + printk("adp1653: probe\n"); > > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > if (flash == NULL) > return -ENOMEM; > > - flash->platform_data = client->dev.platform_data; > + /* we couldn't work without platform data */ > + if (client->dev.platform_data == NULL) { > + ret = adp1653_of_init(client, flash, client->dev.of_node); > + if (ret) > + return ret; > + } else > + flash->platform_data = client->dev.platform_data; > > mutex_init(&flash->power_lock); > > @@ -439,9 +527,15 @@ static int adp1653_probe(struct i2c_client *client, > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > > + __adp1653_set_power(flash, 1); > + > + __adp1653_set_power(flash, 0); > + printk("Flash should have blinked\n"); > + > return 0; > > free_and_quit: > + printk("adp1653: something failed registering\n"); > v4l2_ctrl_handler_free(&flash->ctrls); > return ret; > } > diff --git a/include/media/adp1653.h b/include/media/adp1653.h > index 1d9b48a..b556580 100644 > --- a/include/media/adp1653.h > +++ b/include/media/adp1653.h > @@ -100,9 +100,11 @@ struct adp1653_platform_data { > int (*power)(struct v4l2_subdev *sd, int on); > > u32 max_flash_timeout; /* flash light timeout in us */ > - u32 max_flash_intensity; /* led intensity, flash mode */ > - u32 max_torch_intensity; /* led intensity, torch mode */ > - u32 max_indicator_intensity; /* indicator led intensity */ > + u32 max_flash_intensity; /* led intensity, flash mode, mA */ > + u32 max_torch_intensity; /* led intensity, torch mode, mA */ > + u32 max_indicator_intensity; /* indicator led intensity, uA */ > + > + int power_gpio; /* for device-tree based boot */ > }; > > #define to_adp1653_flash(sd) container_of(sd, struct adp1653_flash, subdev) > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > * Pavel Machek <pavel@ucw.cz> [141117 02:17]: > > On Mon 2014-11-17 11:09:45, Pali Rohár wrote: > > > On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > > > > Hi! > > > > > > > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > > > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > > > > For device tree people: Yes, I know I'll have to > > > > > > create file in documentation, but does the binding > > > > > > below look acceptable? > > > > > > > > > > > > I'll clean up driver code a bit more, remove the > > > > > > printks. Anything else obviously wrong? > > > > > > > > > > I think that this patch is probably not good and > > > > > specially not for n900. adp1653 should be registered > > > > > throw omap3 isp camera subsystem which does not have > > > > > DT support yet. > > > > > > > > Can you explain? > > > > > > > > adp1653 is independend device on i2c bus, and we have > > > > kernel driver for it (unlike rest of n900 camera > > > > system). Just now it is unusable due to lack of DT > > > > binding. It has two functions, LED light and a camera > > > > flash; yes, the second one should be integrated to the > > > > rest of camera system, but that is not yet merged. That > > > > should not prevent us from merging DT support for the > > > > flash, so that this part can be tested/maintained. > > > > > > Ok. When ISP camera subsystem has DT support somebody will > > > modify n900 DT to add camera flash from adp1653 to ISP... > > > I believe it will not be hard. > > > > Exactly. And yes, I'd like to get complete camera support > > for n900 merged. But first step is "make sure existing > > support does not break". > > There's nothing stopping us from initializing the camera code > from pdata-quirks.c for now to keep it working. Certainly the > binding should be added to the driver, but that removes a > dependency to the legacy booting mode if things are otherwise > working. > > Regards, > > Tony Tony, legacy board code for n900 is not in mainline tree. And that omap3 camera subsystem for n900 is broken since 3.5 kernel... (both Front and Back camera on n900 show only green picture).
Hi Pali, On Mon, Nov 17, 2014 at 04:01:31PM +0100, Pali Rohár wrote: > On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > > * Pavel Machek <pavel@ucw.cz> [141117 02:17]: > > > On Mon 2014-11-17 11:09:45, Pali Rohár wrote: > > > > On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > > > > > Hi! > > > > > > > > > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > > > > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > > > > > For device tree people: Yes, I know I'll have to > > > > > > > create file in documentation, but does the binding > > > > > > > below look acceptable? > > > > > > > > > > > > > > I'll clean up driver code a bit more, remove the > > > > > > > printks. Anything else obviously wrong? > > > > > > > > > > > > I think that this patch is probably not good and > > > > > > specially not for n900. adp1653 should be registered > > > > > > throw omap3 isp camera subsystem which does not have > > > > > > DT support yet. > > > > > > > > > > Can you explain? > > > > > > > > > > adp1653 is independend device on i2c bus, and we have > > > > > kernel driver for it (unlike rest of n900 camera > > > > > system). Just now it is unusable due to lack of DT > > > > > binding. It has two functions, LED light and a camera > > > > > flash; yes, the second one should be integrated to the > > > > > rest of camera system, but that is not yet merged. That > > > > > should not prevent us from merging DT support for the > > > > > flash, so that this part can be tested/maintained. > > > > > > > > Ok. When ISP camera subsystem has DT support somebody will > > > > modify n900 DT to add camera flash from adp1653 to ISP... > > > > I believe it will not be hard. > > > > > > Exactly. And yes, I'd like to get complete camera support > > > for n900 merged. But first step is "make sure existing > > > support does not break". > > > > There's nothing stopping us from initializing the camera code > > from pdata-quirks.c for now to keep it working. Certainly the > > binding should be added to the driver, but that removes a > > dependency to the legacy booting mode if things are otherwise > > working. > > > > Regards, > > > > Tony > > Tony, legacy board code for n900 is not in mainline tree. And > that omap3 camera subsystem for n900 is broken since 3.5 > kernel... (both Front and Back camera on n900 show only green > picture). Can you capture raw bayer images correctly? I assume green means YUV buffers that are all zero. Do you know more specifically which patch breaks it?
* Pali Rohár <pali.rohar@gmail.com> [141117 07:03]: > On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > > > > There's nothing stopping us from initializing the camera code > > from pdata-quirks.c for now to keep it working. Certainly the > > binding should be added to the driver, but that removes a > > dependency to the legacy booting mode if things are otherwise > > working. > > Tony, legacy board code for n900 is not in mainline tree. And > that omap3 camera subsystem for n900 is broken since 3.5 > kernel... (both Front and Back camera on n900 show only green > picture). I'm still seeing the legacy board code for n900 in mainline tree :) It's deprecated, but still there. Are you maybe talking about some other piece of platform_data that's no longer in the mainline kernel? No idea what might be wrong with the camera though. Regards, Tony
On Monday 17 November 2014 16:04:07 Sakari Ailus wrote: > Hi Pali, > > On Mon, Nov 17, 2014 at 04:01:31PM +0100, Pali Rohár wrote: > > On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > > > * Pavel Machek <pavel@ucw.cz> [141117 02:17]: > > > > On Mon 2014-11-17 11:09:45, Pali Rohár wrote: > > > > > On Monday 17 November 2014 11:05:19 Pavel Machek wrote: > > > > > > Hi! > > > > > > > > > > > > On Mon 2014-11-17 09:43:19, Pali Rohár wrote: > > > > > > > On Sunday 16 November 2014 08:59:28 Pavel Machek wrote: > > > > > > > > For device tree people: Yes, I know I'll have to > > > > > > > > create file in documentation, but does the > > > > > > > > binding below look acceptable? > > > > > > > > > > > > > > > > I'll clean up driver code a bit more, remove the > > > > > > > > printks. Anything else obviously wrong? > > > > > > > > > > > > > > I think that this patch is probably not good and > > > > > > > specially not for n900. adp1653 should be > > > > > > > registered throw omap3 isp camera subsystem which > > > > > > > does not have DT support yet. > > > > > > > > > > > > Can you explain? > > > > > > > > > > > > adp1653 is independend device on i2c bus, and we > > > > > > have kernel driver for it (unlike rest of n900 > > > > > > camera system). Just now it is unusable due to lack > > > > > > of DT binding. It has two functions, LED light and > > > > > > a camera flash; yes, the second one should be > > > > > > integrated to the rest of camera system, but that > > > > > > is not yet merged. That should not prevent us from > > > > > > merging DT support for the flash, so that this part > > > > > > can be tested/maintained. > > > > > > > > > > Ok. When ISP camera subsystem has DT support somebody > > > > > will modify n900 DT to add camera flash from adp1653 > > > > > to ISP... I believe it will not be hard. > > > > > > > > Exactly. And yes, I'd like to get complete camera > > > > support for n900 merged. But first step is "make sure > > > > existing support does not break". > > > > > > There's nothing stopping us from initializing the camera > > > code from pdata-quirks.c for now to keep it working. > > > Certainly the binding should be added to the driver, but > > > that removes a dependency to the legacy booting mode if > > > things are otherwise working. > > > > > > Regards, > > > > > > Tony > > > > Tony, legacy board code for n900 is not in mainline tree. > > And that omap3 camera subsystem for n900 is broken since > > 3.5 kernel... (both Front and Back camera on n900 show only > > green picture). > > Can you capture raw bayer images correctly? I assume green > means YUV buffers that are all zero. > > Do you know more specifically which patch breaks it? CCing freemangordon (Ivaylo Dimitrov). He tried to debug it months ago but without success. Should know more info about this problem. I think that commit which broke it was not bisected...
On Monday 17 November 2014 16:06:17 Tony Lindgren wrote: > * Pali Rohár <pali.rohar@gmail.com> [141117 07:03]: > > On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > > > There's nothing stopping us from initializing the camera > > > code from pdata-quirks.c for now to keep it working. > > > Certainly the binding should be added to the driver, but > > > that removes a dependency to the legacy booting mode if > > > things are otherwise working. > > > > Tony, legacy board code for n900 is not in mainline tree. > > And that omap3 camera subsystem for n900 is broken since > > 3.5 kernel... (both Front and Back camera on n900 show only > > green picture). > > I'm still seeing the legacy board code for n900 in mainline > tree :) It's deprecated, but still there. > Yes, it is there because conversion from board code to DT is not complete yet... It is slow progress but you can watch it on page http://elinux.org/N900 (last two columns). > Are you maybe talking about some other piece of platform_data > that's no longer in the mainline kernel? > Yes, about platform_data which were never in mainline kernel. Just only in other trees. That code is: camera subsystem (with all other devices), cellular modem, bluetooth, radio. > No idea what might be wrong with the camera though. > > Regards, > > Tony
Hi Pavel, Sakari, On 11/17/2014 03:58 PM, Sakari Ailus wrote: > Hi Pavel, > > On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote: >> For device tree people: Yes, I know I'll have to create file in >> documentation, but does the binding below look acceptable? >> >> I'll clean up driver code a bit more, remove the printks. Anything >> else obviously wrong? > > Jacek Anaszewski is working on flash support for LED devices. I think it'd > be good to sync the DT bindings for the two, as the types of devices > supported by the LED API and the V4L2 flash API are quite similar. > > Cc Jacek. I've already submitted a patch [1] that updates leds common bindings. I hasn't been merged yet, as the related LED Flash class patch [2] still needs some indicator leds related discussion [3]. I think this is a good moment to discuss the flash related led common bindings. [1] http://www.spinics.net/lists/linux-leds/msg02121.html [2] http://www.spinics.net/lists/linux-media/msg83100.html [3] http://www.spinics.net/lists/linux-leds/msg02472.html Best Regards, Jacek Anaszewski
On Tue 2014-11-18 09:09:09, Jacek Anaszewski wrote: > Hi Pavel, Sakari, > > On 11/17/2014 03:58 PM, Sakari Ailus wrote: > >Hi Pavel, > > > >On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote: > >>For device tree people: Yes, I know I'll have to create file in > >>documentation, but does the binding below look acceptable? > >> > >>I'll clean up driver code a bit more, remove the printks. Anything > >>else obviously wrong? > > > >Jacek Anaszewski is working on flash support for LED devices. I think it'd > >be good to sync the DT bindings for the two, as the types of devices > >supported by the LED API and the V4L2 flash API are quite similar. > > > >Cc Jacek. > > I've already submitted a patch [1] that updates leds common bindings. > I hasn't been merged yet, as the related LED Flash class patch [2] > still needs some indicator leds related discussion [3]. > > I think this is a good moment to discuss the flash related led common > bindings. Part of problem is that adp1653 is not regarded as "LED" device by current kernel driver. > [1] http://www.spinics.net/lists/linux-leds/msg02121.html @@ -3,6 +3,17 @@ Common leds properties. Optional properties for child nodes: - label : The label for this LED. If omitted, the label is taken from the node name (excluding the unit address). +- iout-torch : Array of maximum intensities in microamperes of the torch + led currents in order from sub-led 0 to N-1, where N is the number + of torch sub-leds exposed by the device +- iout-flash : Array of maximum intensities in microamperes of the flash + led currents in order from sub-led 0 to N-1, where N is the number + of flash sub-leds exposed by the device +- iout-indicator : Array of maximum intensities in microamperes of + the indicator led currents in order from sub-led 0 to N-1, + where N is the number of indicator sub-leds exposed by the device +- flash-timeout : timeout in microseconds after which flash led + is turned off - linux,default-trigger : This parameter, if present, is a string defining the trigger assigned to the LED. Current triggers are: @@ -19,5 +30,10 @@ Examples: system-status { label = "Status"; linux,default-trigger = "heartbeat"; + iout-torch = <500 500>; + iout-flash = <1000 1000>; + iout-indicator = <100 100>; + flash-timeout = <1000>; + ... }; I don't get it; system-status describes single LED, why are iout-torch (and friends) arrays of two? Also, at least on adp1653, these are actually two leds -- white and red. Torch and flash is white led, indicator is red led. > [2] http://www.spinics.net/lists/linux-media/msg83100.html > [3] http://www.spinics.net/lists/linux-leds/msg02472.html What device are you using for testing? Can you cc me on future patches? Why do we need complex "flash LED class" support, and where is the V4L2 glue? Best regards, Pavel
On Tue 2014-11-18 09:09:09, Jacek Anaszewski wrote: > Hi Pavel, Sakari, > > On 11/17/2014 03:58 PM, Sakari Ailus wrote: > >Hi Pavel, > > > >On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote: > >>For device tree people: Yes, I know I'll have to create file in > >>documentation, but does the binding below look acceptable? > >> > >>I'll clean up driver code a bit more, remove the printks. Anything > >>else obviously wrong? > > > >Jacek Anaszewski is working on flash support for LED devices. I think it'd > >be good to sync the DT bindings for the two, as the types of devices > >supported by the LED API and the V4L2 flash API are quite similar. > > > >Cc Jacek. > > I've already submitted a patch [1] that updates leds common bindings. > I hasn't been merged yet, as the related LED Flash class patch [2] > still needs some indicator leds related discussion [3]. > > I think this is a good moment to discuss the flash related led common > bindings. > > [2] http://www.spinics.net/lists/linux-media/msg83100.html Actually, would it be possible to do the patches the other way around? V4L2 api already knows about flash, torch and indicators. Provide glue, so that V4L2 torch automatically appears as two leds in the led subystem? (white and red in my case?) [Well, I kind of like the LED class API more, but it appears both you and me really have two LEDs in one package, and V4L2 already knows about that so it is better match....?] Pavel
On 11/18/2014 09:46 AM, Pavel Machek wrote: > On Tue 2014-11-18 09:09:09, Jacek Anaszewski wrote: >> Hi Pavel, Sakari, >> >> On 11/17/2014 03:58 PM, Sakari Ailus wrote: >>> Hi Pavel, >>> >>> On Sun, Nov 16, 2014 at 08:59:28AM +0100, Pavel Machek wrote: >>>> For device tree people: Yes, I know I'll have to create file in >>>> documentation, but does the binding below look acceptable? >>>> >>>> I'll clean up driver code a bit more, remove the printks. Anything >>>> else obviously wrong? >>> >>> Jacek Anaszewski is working on flash support for LED devices. I think it'd >>> be good to sync the DT bindings for the two, as the types of devices >>> supported by the LED API and the V4L2 flash API are quite similar. >>> >>> Cc Jacek. >> >> I've already submitted a patch [1] that updates leds common bindings. >> I hasn't been merged yet, as the related LED Flash class patch [2] >> still needs some indicator leds related discussion [3]. >> >> I think this is a good moment to discuss the flash related led common >> bindings. > > Part of problem is that adp1653 is not regarded as "LED" device by > current kernel driver. It doesn't prevent us from keeping the flash devices related DT bindings unified across kernel subsystems. The DT bindings docs for the adp1653 could just provide a reference to the led/common.txt bindings. In the future, when LED Flash class will be merged, all the V4L2 Flash drivers might be moved to the LED subsystem to gain the LED subsystem support. >> [1] http://www.spinics.net/lists/linux-leds/msg02121.html > > @@ -3,6 +3,17 @@ Common leds properties. > Optional properties for child nodes: > - label : The label for this LED. If omitted, the label is > taken from the node name (excluding the unit address). > +- iout-torch : Array of maximum intensities in microamperes of the > torch > + led currents in order from sub-led 0 to N-1, where N is the > number > + of torch sub-leds exposed by the device > +- iout-flash : Array of maximum intensities in microamperes of the > flash > + led currents in order from sub-led 0 to N-1, where N is the > number > + of flash sub-leds exposed by the device > +- iout-indicator : Array of maximum intensities in microamperes of > + the indicator led currents in order from sub-led 0 to N-1, > + where N is the number of indicator sub-leds exposed by the device > +- flash-timeout : timeout in microseconds after which flash led > + is turned off > > - linux,default-trigger : This parameter, if present, is a > string defining the trigger assigned to the LED. Current > triggers are: > @@ -19,5 +30,10 @@ Examples: > system-status { > label = "Status"; > linux,default-trigger = "heartbeat"; > + iout-torch = <500 500>; > + iout-flash = <1000 1000>; > + iout-indicator = <100 100>; > + flash-timeout = <1000>; > + > ... > }; > > I don't get it; system-status describes single LED, why are iout-torch > (and friends) arrays of two? Some devices can control more than one led. The array is for such purposes. The system-status should be probably renamed to something more generic for both common leds and flash leds, e.g. system-led. > Also, at least on adp1653, these are actually two leds -- white and > red. Torch and flash is white led, indicator is red led. Then you should define three properties: iout-torch = <[uA]>; iout-flash = <[uA]>; iout-indicator = <[uA]>; iout-torch and iout-flash properties would determine the current for the white led in the torch and flash modes respectively and the iout-indicator property would determine the current for the indicator led. >> [2] http://www.spinics.net/lists/linux-media/msg83100.html >> [3] http://www.spinics.net/lists/linux-leds/msg02472.html > > What device are you using for testing? Can you cc me on future > patches? I am using max77693 [1] and aat1290 [2]. OK, I will add you on cc. > Why do we need complex "flash LED class" support, and where is the > V4L2 glue? The rationale for unification of the LED and V4L2 Flash API can be found in the discussion [3]. The glue is the v4l2-flash [4] module which exposes a sub-device, that controls a LED Flash class device with use of LED Flash class API. The v4l2-flash sub-device registers with v4l2-async API in a media device. Exemplary support for v4l2-flash sub-devices is added to the exynos4-is driver in the patch [5]. Best Regards, Jacek Anaszewski [1] http://www.spinics.net/lists/linux-leds/msg02326.html [2] http://www.spinics.net/lists/linux-media/msg81079.html [3] http://www.spinics.net/lists/linux-media/msg69012.html [4] http://www.spinics.net/lists/linux-leds/msg02322.html [5] http://www.spinics.net/lists/linux-leds/msg02323.html
> >>I've already submitted a patch [1] that updates leds common bindings. > >>I hasn't been merged yet, as the related LED Flash class patch [2] > >>still needs some indicator leds related discussion [3]. > >> > >>I think this is a good moment to discuss the flash related led common > >>bindings. > > > >Part of problem is that adp1653 is not regarded as "LED" device by > >current kernel driver. > > It doesn't prevent us from keeping the flash devices related > DT bindings unified across kernel subsystems. The DT bindings > docs for the adp1653 could just provide a reference to the > led/common.txt bindings. In the future, when LED Flash > class will be merged, all the V4L2 Flash drivers might be > moved to the LED subsystem to gain the LED subsystem support. Yeah, that makses sense. > >@@ -19,5 +30,10 @@ Examples: > > system-status { > > label = "Status"; > > linux,default-trigger = "heartbeat"; > >+ iout-torch = <500 500>; > >+ iout-flash = <1000 1000>; > >+ iout-indicator = <100 100>; > >+ flash-timeout = <1000>; > >+ > > ... > > }; > > > >I don't get it; system-status describes single LED, why are iout-torch > >(and friends) arrays of two? > > Some devices can control more than one led. The array is for such > purposes. The system-status should be probably renamed to > something more generic for both common leds and flash leds, > e.g. system-led. No, sorry. The Documentation/devicetree/bindings/leds/common.txt describes binding for _one LED_. Yes, your device can have two leds, so your devices should have two such blocks in the device tree... Each led should have its own label and default trigger, for example. And I guess flash-timeout be per-LED, too. Would it make sense to include "-uA" and "-usec" suffixes in the dt property names? > >Also, at least on adp1653, these are actually two leds -- white and > >red. Torch and flash is white led, indicator is red led. > > Then you should define three properties: > > iout-torch = <[uA]>; > iout-flash = <[uA]>; > iout-indicator = <[uA]>; > > iout-torch and iout-flash properties would determine the current > for the white led in the torch and flash modes respectively and > the iout-indicator property would determine the current for > the indicator led. Yes, that would work. I have used + max-flash-timeout-usec = <500000>; + max-flash-intensity-uA = <320000>; + max-torch-intensity-uA = <50000>; + max-indicator-intensity-uA = <17500>; . Which is pretty similar. (Actually, maybe the longer property names are easier to understand for more people?) (And yes, I should probably separate red and white led into separate groups). > >>[2] http://www.spinics.net/lists/linux-media/msg83100.html > >>[3] http://www.spinics.net/lists/linux-leds/msg02472.html > > > >What device are you using for testing? Can you cc me on future > >patches? > > I am using max77693 [1] and aat1290 [2]. OK, I will add you on cc. Thanks for cc and thanks for links! I see max77693 has two different "white" leds, aat1290 has one "white" led, and neither of them has secondary red led for indication? > The v4l2-flash sub-device registers with v4l2-async API > in a media device. Exemplary support for v4l2-flash > sub-devices is added to the exynos4-is driver in the patch [5]. Thanks for the links. It seems that aside from moving adp1653 driver to device tree, it should be moved to the LED framework, but that's a topic for another patch. Best regards, Pavel
On 11/18/2014 12:32 PM, Pavel Machek wrote: > >>>> I've already submitted a patch [1] that updates leds common bindings. >>>> I hasn't been merged yet, as the related LED Flash class patch [2] >>>> still needs some indicator leds related discussion [3]. >>>> >>>> I think this is a good moment to discuss the flash related led common >>>> bindings. >>> >>> Part of problem is that adp1653 is not regarded as "LED" device by >>> current kernel driver. >> >> It doesn't prevent us from keeping the flash devices related >> DT bindings unified across kernel subsystems. The DT bindings >> docs for the adp1653 could just provide a reference to the >> led/common.txt bindings. In the future, when LED Flash >> class will be merged, all the V4L2 Flash drivers might be >> moved to the LED subsystem to gain the LED subsystem support. > > Yeah, that makses sense. > >>> @@ -19,5 +30,10 @@ Examples: >>> system-status { >>> label = "Status"; >>> linux,default-trigger = "heartbeat"; >>> + iout-torch = <500 500>; >>> + iout-flash = <1000 1000>; >>> + iout-indicator = <100 100>; >>> + flash-timeout = <1000>; >>> + >>> ... >>> }; >>> >>> I don't get it; system-status describes single LED, why are iout-torch >>> (and friends) arrays of two? >> >> Some devices can control more than one led. The array is for such >> purposes. The system-status should be probably renamed to >> something more generic for both common leds and flash leds, >> e.g. system-led. > > No, sorry. The Documentation/devicetree/bindings/leds/common.txt > describes binding for _one LED_. Yes, your device can have two leds, > so your devices should have two such blocks in the device tree... Each > led should have its own label and default trigger, for example. And I > guess flash-timeout be per-LED, too. I think that a device tree binding describes a single physical device. No matter how many sub-leds a device controls, it is still one piece of hardware. default-trigger property should also be an array of strings. I agree that flash-timeout should be per led - an array, similarly as in case of iout's. > Would it make sense to include "-uA" and "-usec" suffixes in the dt > property names? I don't see this type of naming anywhere. The documentation should contain the information about units. Nonetheless I am not a device tree specialist, so an opinion of the relevant maintainer will be welcome. >>> Also, at least on adp1653, these are actually two leds -- white and >>> red. Torch and flash is white led, indicator is red led. >> >> Then you should define three properties: >> >> iout-torch = <[uA]>; >> iout-flash = <[uA]>; >> iout-indicator = <[uA]>; >> >> iout-torch and iout-flash properties would determine the current >> for the white led in the torch and flash modes respectively and >> the iout-indicator property would determine the current for >> the indicator led. > > Yes, that would work. I have used > > + max-flash-timeout-usec = <500000>; > + max-flash-intensity-uA = <320000>; > + max-torch-intensity-uA = <50000>; > + max-indicator-intensity-uA = <17500>; > > . Which is pretty similar. (Actually, maybe the longer property names > are easier to understand for more people?) (And yes, I should probably > separate red and white led into separate groups). Documentation of a binding should provide sufficient explanation for the people to understand what a property is for. And I would stay by arrays as argued above. >>>> [2] http://www.spinics.net/lists/linux-media/msg83100.html >>>> [3] http://www.spinics.net/lists/linux-leds/msg02472.html >>> >>> What device are you using for testing? Can you cc me on future >>> patches? >> >> I am using max77693 [1] and aat1290 [2]. OK, I will add you on cc. > > Thanks for cc and thanks for links! > > I see max77693 has two different "white" leds, aat1290 has one "white" > led, and neither of them has secondary red led for indication? You're right. >> The v4l2-flash sub-device registers with v4l2-async API >> in a media device. Exemplary support for v4l2-flash >> sub-devices is added to the exynos4-is driver in the patch [5]. > > Thanks for the links. It seems that aside from moving adp1653 driver > to device tree, it should be moved to the LED framework, but that's a > topic for another patch. Like I mentioned in the previous message the LED Flash class patch isn't in its final shape yet. Nonetheless I think that we should agree on the leds/common.txt documentation improvements and define DT documentation for adp1653 accordingly. Regards, Jacek
Hi! > >>>@@ -19,5 +30,10 @@ Examples: > >>> system-status { > >>> label = "Status"; > >>> linux,default-trigger = "heartbeat"; > >>>+ iout-torch = <500 500>; > >>>+ iout-flash = <1000 1000>; > >>>+ iout-indicator = <100 100>; > >>>+ flash-timeout = <1000>; > >>>+ > >>> ... > >>> }; > >>> > >>>I don't get it; system-status describes single LED, why are iout-torch > >>>(and friends) arrays of two? > >> > >>Some devices can control more than one led. The array is for such > >>purposes. The system-status should be probably renamed to > >>something more generic for both common leds and flash leds, > >>e.g. system-led. > > > >No, sorry. The Documentation/devicetree/bindings/leds/common.txt > >describes binding for _one LED_. Yes, your device can have two leds, > >so your devices should have two such blocks in the device tree... Each > >led should have its own label and default trigger, for example. And I > >guess flash-timeout be per-LED, too. > > I think that a device tree binding describes a single physical device. > No matter how many sub-leds a device controls, it is still one > piece of hardware. You got this wrong, sorry. In my case, there are three physical devices: adp1653 white LED red LED Each LED should have an label, and probably default trigger -- default trigger for red one should be "we are recording video" and for white should be "this is flash for default camera". If the hardware LED changes with one that needs different current, the block for the adp1653 stays the same, but white LED block should be updated with different value. > default-trigger property should also be an array of strings. That is not how it currently works. > I agree that flash-timeout should be per led - an array, similarly > as in case of iout's. Agreed about per-led, disagreed about the array. As all the fields would need arrays, and as LED system currently does not use arrays for label and linux,default-trigger, I believe we should follow existing design and model it as three devices. (It _is_ physically three devices.) > >>The v4l2-flash sub-device registers with v4l2-async API > >>in a media device. Exemplary support for v4l2-flash > >>sub-devices is added to the exynos4-is driver in the patch [5]. > > > >Thanks for the links. It seems that aside from moving adp1653 driver > >to device tree, it should be moved to the LED framework, but that's a > >topic for another patch. > > Like I mentioned in the previous message the LED Flash class patch > isn't in its final shape yet. Nonetheless I think that we should > agree on the leds/common.txt documentation improvements and > define DT documentation for adp1653 accordingly. Agreed. Pavel
Hi Pavel, On 11/18/2014 02:21 PM, Pavel Machek wrote: > Hi! > >>>>> @@ -19,5 +30,10 @@ Examples: >>>>> system-status { >>>>> label = "Status"; >>>>> linux,default-trigger = "heartbeat"; >>>>> + iout-torch = <500 500>; >>>>> + iout-flash = <1000 1000>; >>>>> + iout-indicator = <100 100>; >>>>> + flash-timeout = <1000>; >>>>> + >>>>> ... >>>>> }; >>>>> >>>>> I don't get it; system-status describes single LED, why are iout-torch >>>>> (and friends) arrays of two? >>>> >>>> Some devices can control more than one led. The array is for such >>>> purposes. The system-status should be probably renamed to >>>> something more generic for both common leds and flash leds, >>>> e.g. system-led. >>> >>> No, sorry. The Documentation/devicetree/bindings/leds/common.txt >>> describes binding for _one LED_. Yes, your device can have two leds, >>> so your devices should have two such blocks in the device tree... Each >>> led should have its own label and default trigger, for example. And I >>> guess flash-timeout be per-LED, too. >> >> I think that a device tree binding describes a single physical device. >> No matter how many sub-leds a device controls, it is still one >> piece of hardware. > > You got this wrong, sorry. > > In my case, there are three physical devices: > > adp1653 > white LED > red LED You've mentioned that your white led is torch/flash and indicator is the red led. They are probably connected to the HPLED and ILED pins of the ADP1653 device respectively. The device is just a regulator, that delivers electric current to the leds connected to it. Kernel cannot directly activate leds, but has to talk to the device through I2C bus. One I2C device can have only one related device tree binding. > Each LED should have an label, and probably default trigger -- default > trigger for red one should be "we are recording video" and for white > should be "this is flash for default camera". default-trigger is not mandatory, the device doesn't have to have associated led-trigger. I think that you should look at Documentation/leds/leds-class.txt and drivers/leds/triggers for more detailed information. In a nutshell triggers are kernel sources of led events. You can set e.g. "heartbeat", "timer" trigger etc. As for now the driver belongs to the V4L2 subsystem it doesn't support triggers. Moreover your event "we are recording a video" should be activated by setting V4L2_CID_FLASH_INDICATOR_INTENSITY v4l2 control followed by V4L2_FLASH_LED_MODE_TORCH. Your event "this is flash for default camera" seems to be flash strobe, that can be activated by setting V4L2_CID_FLASH_STROBE control. The driver by default sets the indicator current for both actions to the value previously set with V4L2_CID_FLASH_INDICATOR_INTENSITY. > If the hardware LED changes with one that needs different current, the > block for the adp1653 stays the same, but white LED block should be > updated with different value. I think that you are talking about sub nodes. Indeed I am leaning towards this type of design. >> default-trigger property should also be an array of strings. > > That is not how it currently works. OK, I agree. > >> I agree that flash-timeout should be per led - an array, similarly >> as in case of iout's. > > Agreed about per-led, disagreed about the array. As all the fields > would need arrays, and as LED system currently does not use arrays for > label and linux,default-trigger, I believe we should follow existing > design and model it as three devices. (It _is_ physically three devices.) Right, I missed that the leds/common.txt describes child node. I propose following modifications to the binding: Optional properties for child nodes: - iout-mode-led : maximum intensity in microamperes of the LED (torch LED for flash devices) - iout-mode-flash : initial intensity in microamperes of the flash LED; it is required to enable support for the flash led - iout-mode-indicator : initial intensity in microamperes of the indicator LED; it is required to enable support for the indicator led - max-iout-mode-led : maximum intensity in microamperes of the LED (torch LED for flash devices) - max-iout-mode-flash : maximum intensity in microamperes of the flash LED - max-iout-mode-indicator : maximum intensity in microamperes of the indicator LED - flash-timeout : timeout in microseconds after which flash led is turned off system-status { label = "max77693_1"; iout-mode-led = <500>; max-iout-mode-led = <500>; ... }; camera-flash1 { label = "max77693_2"; iout-mode-led = <500>; iout-mode-flash = <1000>; iout-mode-indicator = <100>; max-iout-mode-led = <500>; max-iout-mode-flash = <1000>; max-iout-mode-indicator = <100>; flash-timeout = <1000>; ... }; I propose to avoid name "torch", as for ordinary leds it would be misleading. Regards, Jacek
Hi! > >If the hardware LED changes with one that needs different current, the > >block for the adp1653 stays the same, but white LED block should be > >updated with different value. > > I think that you are talking about sub nodes. Indeed I am leaning > towards this type of design. I think I am :-). > >>I agree that flash-timeout should be per led - an array, similarly > >>as in case of iout's. > > > >Agreed about per-led, disagreed about the array. As all the fields > >would need arrays, and as LED system currently does not use arrays for > >label and linux,default-trigger, I believe we should follow existing > >design and model it as three devices. (It _is_ physically three devices.) > > Right, I missed that the leds/common.txt describes child node. > > I propose following modifications to the binding: > > Optional properties for child nodes: > - iout-mode-led : maximum intensity in microamperes of the LED > (torch LED for flash devices) > - iout-mode-flash : initial intensity in microamperes of the > flash LED; it is required to enable support > for the flash led > - iout-mode-indicator : initial intensity in microamperes of the > indicator LED; it is required to enable support > for the indicator led > - max-iout-mode-led : maximum intensity in microamperes of the LED > (torch LED for flash devices) > - max-iout-mode-flash : maximum intensity in microamperes of the > flash LED > - max-iout-mode-indicator : maximum intensity in microamperes of the > indicator LED > - flash-timeout : timeout in microseconds after which flash > led is turned off Ok, I took a look, and "iout" is notation I understand, but people may have trouble with and I don't see it used anywhere else. Also... do we need both "current" and "max-current" properties? But regulators already have "regulator-max-microamp" property. So what about: max-microamp : maximum intensity in microamperes of the LED (torch LED for flash devices) max-flash-microamp : initial intensity in microamperes of the flash LED; it is required to enable support for the flash led flash-timeout-microseconds : timeout in microseconds after which flash led is turned off If you had indicator on the same led, I guess indicator-microamp : recommended intensity in microamperes of the LED for indication ...would do? > I propose to avoid name "torch", as for ordinary leds it would > be misleading. Agreed. Thanks, Pavel
On Mon 2014-11-17 07:06:17, Tony Lindgren wrote: > * Pali Rohár <pali.rohar@gmail.com> [141117 07:03]: > > On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: > > > > > > There's nothing stopping us from initializing the camera code > > > from pdata-quirks.c for now to keep it working. Certainly the > > > binding should be added to the driver, but that removes a > > > dependency to the legacy booting mode if things are otherwise > > > working. > > > > Tony, legacy board code for n900 is not in mainline tree. And > > that omap3 camera subsystem for n900 is broken since 3.5 > > kernel... (both Front and Back camera on n900 show only green > > picture). > > I'm still seeing the legacy board code for n900 in mainline tree :) > It's deprecated, but still there. > > Are you maybe talking about some other piece of platform_data that's > no longer in the mainline kernel? > > No idea what might be wrong with the camera though. Camera support for main and secondary cameras was never mainline, AFAICT. Merging it will not be easy, as it lacks DT support... and was broken for long time. Anyway, flash is kind of important for me, since it makes phone useful as backup light; and it is simple piece of hw, so I intend to keep it useful. Pavel
Hi Pavel, Sakari, On 11/18/2014 05:51 PM, Pavel Machek wrote: > Hi! > >>> If the hardware LED changes with one that needs different current, the >>> block for the adp1653 stays the same, but white LED block should be >>> updated with different value. >> >> I think that you are talking about sub nodes. Indeed I am leaning >> towards this type of design. > > I think I am :-). > >>>> I agree that flash-timeout should be per led - an array, similarly >>>> as in case of iout's. >>> >>> Agreed about per-led, disagreed about the array. As all the fields >>> would need arrays, and as LED system currently does not use arrays for >>> label and linux,default-trigger, I believe we should follow existing >>> design and model it as three devices. (It _is_ physically three devices.) >> >> Right, I missed that the leds/common.txt describes child node. >> >> I propose following modifications to the binding: >> >> Optional properties for child nodes: >> - iout-mode-led : maximum intensity in microamperes of the LED >> (torch LED for flash devices) >> - iout-mode-flash : initial intensity in microamperes of the >> flash LED; it is required to enable support >> for the flash led >> - iout-mode-indicator : initial intensity in microamperes of the >> indicator LED; it is required to enable support >> for the indicator led >> - max-iout-mode-led : maximum intensity in microamperes of the LED >> (torch LED for flash devices) >> - max-iout-mode-flash : maximum intensity in microamperes of the >> flash LED >> - max-iout-mode-indicator : maximum intensity in microamperes of the >> indicator LED >> - flash-timeout : timeout in microseconds after which flash >> led is turned off > > Ok, I took a look, and "iout" is notation I understand, but people may > have trouble with and I don't see it used anywhere else. > > Also... do we need both "current" and "max-current" properties? > > But regulators already have "regulator-max-microamp" property. So what > about: > > max-microamp : maximum intensity in microamperes of the LED > (torch LED for flash devices) > max-flash-microamp : initial intensity in microamperes of the > flash LED; it is required to enable support > for the flash led > flash-timeout-microseconds : timeout in microseconds after which flash > led is turned off > > If you had indicator on the same led, I guess > > indicator-microamp : recommended intensity in microamperes of the LED > for indication > > ...would do? Ongoing discussion allowed me for taking a look at the indicator issue from different perspective. This is also vital for the issue of whether a v4l2-flash sub-device should be created per device or per sub-led [1]. Currently each sub-led is represented as a separate device tree sub node and the led drivers create separate LED class device for the sub nodes. What this implies is that indicator led also must be represented by the separate LED class device. This is contrary to the way how V4L2 Flash API approaches this issue, as it considers a flash device as a regulator chip driven through a bus. The API allows to set the led in torch or flash mode and implicitly assumes that there can be additional indicator led supported, which can't be turned on separately, but the drivers apply the indicator current to the indicator led when the torch or flash led is activated. I propose to create separate v4l2-flash device for the indicator led, and treat it as a regular sub-led similarly like it is done in the LED subsystem. LED Flash class driver would only add a flag LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it. There could ba also additional control added: V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature supported by some LED class drivers. From the media device perspective such an approach would be harmful, as the indicator led could be turned on right before strobing the flash or turning the torch on, by separate calls to different v4l2-flash sub-devices. The design described above would allow for avoiding issues I touched in the message [1]. Regarding DT documentation: I would also swap the segments of a property name to follow the convention as in case of "regulator-max-microamp". Updated version: ========================================================== Optional properties for child nodes: - max-microamp : maximum intensity in microamperes of the LED (torch LED for flash devices) - flash-max-microamp : maximum intensity in microamperes of the flash LED; it is mandatory if the led should support the flash mode - flash-timeout-microsec : timeout in microseconds after which the flash led is turned off - indicator-pattern : identifier of the blinking pattern for the indicator led ========================================================== Regards, Jacek [1] http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/84643
Hi Jacek and Pavel, Jacek Anaszewski wrote: > Hi Pavel, Sakari, > > On 11/18/2014 05:51 PM, Pavel Machek wrote: >> Hi! >> >>>> If the hardware LED changes with one that needs different current, the >>>> block for the adp1653 stays the same, but white LED block should be >>>> updated with different value. >>> >>> I think that you are talking about sub nodes. Indeed I am leaning >>> towards this type of design. >> >> I think I am :-). >> >>>>> I agree that flash-timeout should be per led - an array, similarly >>>>> as in case of iout's. >>>> >>>> Agreed about per-led, disagreed about the array. As all the fields >>>> would need arrays, and as LED system currently does not use arrays for >>>> label and linux,default-trigger, I believe we should follow existing >>>> design and model it as three devices. (It _is_ physically three >>>> devices.) >>> >>> Right, I missed that the leds/common.txt describes child node. >>> >>> I propose following modifications to the binding: >>> >>> Optional properties for child nodes: >>> - iout-mode-led : maximum intensity in microamperes of the LED >>> (torch LED for flash devices) >>> - iout-mode-flash : initial intensity in microamperes of the >>> flash LED; it is required to enable support >>> for the flash led >>> - iout-mode-indicator : initial intensity in microamperes of the >>> indicator LED; it is required to enable support >>> for the indicator led >>> - max-iout-mode-led : maximum intensity in microamperes of the LED >>> (torch LED for flash devices) >>> - max-iout-mode-flash : maximum intensity in microamperes of the >>> flash LED >>> - max-iout-mode-indicator : maximum intensity in microamperes of the >>> indicator LED >>> - flash-timeout : timeout in microseconds after which flash >>> led is turned off >> >> Ok, I took a look, and "iout" is notation I understand, but people may >> have trouble with and I don't see it used anywhere else. >> >> Also... do we need both "current" and "max-current" properties? >> >> But regulators already have "regulator-max-microamp" property. So what >> about: >> >> max-microamp : maximum intensity in microamperes of the LED >> (torch LED for flash devices) >> max-flash-microamp : initial intensity in microamperes of the >> flash LED; it is required to enable support >> for the flash led >> flash-timeout-microseconds : timeout in microseconds after which flash >> led is turned off >> >> If you had indicator on the same led, I guess >> >> indicator-microamp : recommended intensity in microamperes of the LED >> for indication The value for the indicator is maximum as well, not just a recommendation. >> >> ...would do? > > > Ongoing discussion allowed me for taking a look at the indicator issue > from different perspective. This is also vital for the issue of > whether a v4l2-flash sub-device should be created per device or > per sub-led [1]. > > Currently each sub-led is represented as a separate device tree > sub node and the led drivers create separate LED class device for the > sub nodes. What this implies is that indicator led also must be > represented by the separate LED class device. > > This is contrary to the way how V4L2 Flash API approaches this issue, > as it considers a flash device as a regulator chip driven through > a bus. The API allows to set the led in torch or flash mode and > implicitly assumes that there can be additional indicator led > supported, which can't be turned on separately, but the drivers apply > the indicator current to the indicator led when the torch or flash led > is activated. The indicator is independent of the flash LED in V4L2 flash API. At least that's how it should be, and in adp1653 the two are independent, but the as3645a can't use indicator with the flash AFAIR. > I propose to create separate v4l2-flash device for the indicator led, > and treat it as a regular sub-led similarly like it is done in the > LED subsystem. LED Flash class driver would only add a flag > LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device > would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it. > There could ba also additional control added: > V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature > supported by some LED class drivers. Interesting idea. The flash controller is still a single I2C device with common set of faults, for instance. Some devices refuse to work again in case of faults until they are cleared (= read). Could the indicator pattern control be present in the same sub-device? Are there any flash LED controllers that support such functionality? > From the media device perspective such an approach would > be harmful, as the indicator led could be turned on right > before strobing the flash or turning the torch on, by > separate calls to different v4l2-flash sub-devices. > > The design described above would allow for avoiding issues I touched > in the message [1]. Let me reply this separately. Feel free to ping me if I obviously appear to miss something.
Hi Pavel, Pavel Machek wrote: > On Mon 2014-11-17 07:06:17, Tony Lindgren wrote: >> * Pali Rohár <pali.rohar@gmail.com> [141117 07:03]: >>> On Monday 17 November 2014 15:55:46 Tony Lindgren wrote: >>>> >>>> There's nothing stopping us from initializing the camera code >>>> from pdata-quirks.c for now to keep it working. Certainly the >>>> binding should be added to the driver, but that removes a >>>> dependency to the legacy booting mode if things are otherwise >>>> working. >>> >>> Tony, legacy board code for n900 is not in mainline tree. And >>> that omap3 camera subsystem for n900 is broken since 3.5 >>> kernel... (both Front and Back camera on n900 show only green >>> picture). >> >> I'm still seeing the legacy board code for n900 in mainline tree :) >> It's deprecated, but still there. >> >> Are you maybe talking about some other piece of platform_data that's >> no longer in the mainline kernel? >> >> No idea what might be wrong with the camera though. > > Camera support for main and secondary cameras was never mainline, AFAICT. > > Merging it will not be easy, as it lacks DT support... and was broken > for long time. I have a smiapp patchset for DT support that I posted a while ago, here: <URL:http://www.spinics.net/lists/linux-media/msg83285.html> What's missing on top of that is the omap3isp support, plus something to toggle the sysctl registers based on the chosen receiver. I have a preliminary, not RFC yet but functional set here: <URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/rm696-045-dt> The main camera support requires et8ek8 driver as well, and resolving the breakage with the image capture on 3430. N9/N950 will be first, though. Lens controllers are another matter, but nothing too difficult on that side either. > Anyway, flash is kind of important for me, since it makes phone useful > as backup light; and it is simple piece of hw, so I intend to keep it > useful. Me, too. :-)
Hi Pavel, Sakari, On 11/19/2014 06:53 PM, Sakari Ailus wrote: > Hi Jacek and Pavel, > > Jacek Anaszewski wrote: >> Hi Pavel, Sakari, >> >> On 11/18/2014 05:51 PM, Pavel Machek wrote: >>> Hi! >>> >>>>> If the hardware LED changes with one that needs different current, the >>>>> block for the adp1653 stays the same, but white LED block should be >>>>> updated with different value. >>>> >>>> I think that you are talking about sub nodes. Indeed I am leaning >>>> towards this type of design. >>> >>> I think I am :-). >>> >>>>>> I agree that flash-timeout should be per led - an array, similarly >>>>>> as in case of iout's. >>>>> >>>>> Agreed about per-led, disagreed about the array. As all the fields >>>>> would need arrays, and as LED system currently does not use arrays for >>>>> label and linux,default-trigger, I believe we should follow existing >>>>> design and model it as three devices. (It _is_ physically three >>>>> devices.) >>>> >>>> Right, I missed that the leds/common.txt describes child node. >>>> >>>> I propose following modifications to the binding: >>>> >>>> Optional properties for child nodes: >>>> - iout-mode-led : maximum intensity in microamperes of the LED >>>> (torch LED for flash devices) >>>> - iout-mode-flash : initial intensity in microamperes of the >>>> flash LED; it is required to enable support >>>> for the flash led >>>> - iout-mode-indicator : initial intensity in microamperes of the >>>> indicator LED; it is required to enable support >>>> for the indicator led >>>> - max-iout-mode-led : maximum intensity in microamperes of the LED >>>> (torch LED for flash devices) >>>> - max-iout-mode-flash : maximum intensity in microamperes of the >>>> flash LED >>>> - max-iout-mode-indicator : maximum intensity in microamperes of the >>>> indicator LED >>>> - flash-timeout : timeout in microseconds after which flash >>>> led is turned off >>> >>> Ok, I took a look, and "iout" is notation I understand, but people may >>> have trouble with and I don't see it used anywhere else. >>> >>> Also... do we need both "current" and "max-current" properties? >>> >>> But regulators already have "regulator-max-microamp" property. So what >>> about: >>> >>> max-microamp : maximum intensity in microamperes of the LED >>> (torch LED for flash devices) >>> max-flash-microamp : initial intensity in microamperes of the >>> flash LED; it is required to enable support >>> for the flash led >>> flash-timeout-microseconds : timeout in microseconds after which flash >>> led is turned off >>> >>> If you had indicator on the same led, I guess >>> >>> indicator-microamp : recommended intensity in microamperes of the LED >>> for indication > > The value for the indicator is maximum as well, not just a recommendation. > >>> >>> ...would do? >> >> >> Ongoing discussion allowed me for taking a look at the indicator issue >> from different perspective. This is also vital for the issue of >> whether a v4l2-flash sub-device should be created per device or >> per sub-led [1]. >> >> Currently each sub-led is represented as a separate device tree >> sub node and the led drivers create separate LED class device for the >> sub nodes. What this implies is that indicator led also must be >> represented by the separate LED class device. >> >> This is contrary to the way how V4L2 Flash API approaches this issue, >> as it considers a flash device as a regulator chip driven through >> a bus. The API allows to set the led in torch or flash mode and >> implicitly assumes that there can be additional indicator led >> supported, which can't be turned on separately, but the drivers apply >> the indicator current to the indicator led when the torch or flash led >> is activated. > > The indicator is independent of the flash LED in V4L2 flash API. At > least that's how it should be, and in adp1653 the two are independent, > but the as3645a can't use indicator with the flash AFAIR. Right. >> I propose to create separate v4l2-flash device for the indicator led, >> and treat it as a regular sub-led similarly like it is done in the >> LED subsystem. LED Flash class driver would only add a flag >> LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device >> would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it. >> There could ba also additional control added: >> V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature >> supported by some LED class drivers. > > Interesting idea. > > The flash controller is still a single I2C device with common set of > faults, for instance. Some devices refuse to work again in case of > faults until they are cleared (= read). The V4L2_CID_FLASH_FAULT control should be also supported by the indicator v4l2-flash sub-device then. > Could the indicator pattern control be present in the same sub-device? Yes, this was my intention. To conclude - the indicator v4l2-flash sub-device should support up to three controls: - V4L2_CID_FLASH_TORCH_INTENSITY - V4L2_CID_FLASH_FAULT - V4L2_CID_FLASH_INDICATOR_PATTERN (if supported by the LED Flash class driver) V4L2_CID_FLASH_INDICATOR_PATTERN would be a menu control with custom menu items. > Are there any flash LED controllers that support such functionality? There is: lm3556. LED subsystem leds-lm355x.c driver supports it. The driver exposes dedicated sysfs attribute for setting the blinking pattern (four possible options). >> From the media device perspective such an approach would >> be harmful, as the indicator led could be turned on right >> before strobing the flash or turning the torch on, by >> separate calls to different v4l2-flash sub-devices. >> >> The design described above would allow for avoiding issues I touched >> in the message [1]. > > Let me reply this separately. Feel free to ping me if I obviously appear > to miss something. > Best Regards, Jacek Anaszewski
Hi! > I would also swap the segments of a property name to follow the convention > as in case of "regulator-max-microamp". > > Updated version: > > ========================================================== > > Optional properties for child nodes: > - max-microamp : maximum intensity in microamperes of the LED > (torch LED for flash devices) > - flash-max-microamp : maximum intensity in microamperes of the > flash LED; it is mandatory if the led should > support the flash mode > - flash-timeout-microsec : timeout in microseconds after which the flash > led is turned off Works for me. Do you want to submit a patch or should I do it? > - indicator-pattern : identifier of the blinking pattern for the > indicator led > This would need a bit more documentation, no? Pavel
Hi! > >> But regulators already have "regulator-max-microamp" property. So what > >> about: > >> > >> max-microamp : maximum intensity in microamperes of the LED > >> (torch LED for flash devices) > >> max-flash-microamp : initial intensity in microamperes of the > >> flash LED; it is required to enable support > >> for the flash led > >> flash-timeout-microseconds : timeout in microseconds after which flash > >> led is turned off > >> > >> If you had indicator on the same led, I guess > >> > >> indicator-microamp : recommended intensity in microamperes of the LED > >> for indication > > The value for the indicator is maximum as well, not just a > recommendation. Actually, no. This is all for one LED, if you want to use it as a flash, torch and indicator. You already know the maximum value with max-microamp. Best regards, Pavel
On 11/19/2014 10:45 AM, Jacek Anaszewski wrote: > Hi Pavel, Sakari, > > On 11/18/2014 05:51 PM, Pavel Machek wrote: >> Hi! >> >>>> If the hardware LED changes with one that needs different current, the >>>> block for the adp1653 stays the same, but white LED block should be >>>> updated with different value. >>> >>> I think that you are talking about sub nodes. Indeed I am leaning >>> towards this type of design. >> >> I think I am :-). >> >>>>> I agree that flash-timeout should be per led - an array, similarly >>>>> as in case of iout's. >>>> >>>> Agreed about per-led, disagreed about the array. As all the fields >>>> would need arrays, and as LED system currently does not use arrays for >>>> label and linux,default-trigger, I believe we should follow existing >>>> design and model it as three devices. (It _is_ physically three >>>> devices.) >>> >>> Right, I missed that the leds/common.txt describes child node. >>> >>> I propose following modifications to the binding: >>> >>> Optional properties for child nodes: >>> - iout-mode-led : maximum intensity in microamperes of the LED >>> (torch LED for flash devices) >>> - iout-mode-flash : initial intensity in microamperes of the >>> flash LED; it is required to enable support >>> for the flash led >>> - iout-mode-indicator : initial intensity in microamperes of the >>> indicator LED; it is required to enable support >>> for the indicator led >>> - max-iout-mode-led : maximum intensity in microamperes of the LED >>> (torch LED for flash devices) >>> - max-iout-mode-flash : maximum intensity in microamperes of the >>> flash LED >>> - max-iout-mode-indicator : maximum intensity in microamperes of the >>> indicator LED >>> - flash-timeout : timeout in microseconds after which flash >>> led is turned off >> >> Ok, I took a look, and "iout" is notation I understand, but people may >> have trouble with and I don't see it used anywhere else. >> >> Also... do we need both "current" and "max-current" properties? >> >> But regulators already have "regulator-max-microamp" property. So what >> about: >> >> max-microamp : maximum intensity in microamperes of the LED >> (torch LED for flash devices) >> max-flash-microamp : initial intensity in microamperes of the >> flash LED; it is required to enable support >> for the flash led >> flash-timeout-microseconds : timeout in microseconds after which flash >> led is turned off >> >> If you had indicator on the same led, I guess >> >> indicator-microamp : recommended intensity in microamperes of the LED >> for indication >> >> ...would do? > > > Ongoing discussion allowed me for taking a look at the indicator issue > from different perspective. This is also vital for the issue of > whether a v4l2-flash sub-device should be created per device or > per sub-led [1]. > > Currently each sub-led is represented as a separate device tree > sub node and the led drivers create separate LED class device for the > sub nodes. What this implies is that indicator led also must be > represented by the separate LED class device. > > This is contrary to the way how V4L2 Flash API approaches this issue, > as it considers a flash device as a regulator chip driven through > a bus. The API allows to set the led in torch or flash mode and > implicitly assumes that there can be additional indicator led > supported, which can't be turned on separately, but the drivers apply > the indicator current to the indicator led when the torch or flash led > is activated. > > I propose to create separate v4l2-flash device for the indicator led, > and treat it as a regular sub-led similarly like it is done in the > LED subsystem. LED Flash class driver would only add a flag > LED_DEV_CAP_INDICATOR and basing on it the v4l2-flash sub-device > would create only V4L2_CID_FLASH_INDICATOR_INTENSITY control for it. > There could ba also additional control added: > V4L2_CID_FLASH_INDICATOR_PATTERN to support the feature > supported by some LED class drivers. > > From the media device perspective such an approach would > be harmful, as the indicator led could be turned on right I intended here "wouldn't be harmful". > before strobing the flash or turning the torch on, by > separate calls to different v4l2-flash sub-devices. > > The design described above would allow for avoiding issues I touched > in the message [1]. > > Regarding DT documentation: > > I would also swap the segments of a property name to follow the > convention as in case of "regulator-max-microamp". > > Updated version: > > ========================================================== > > Optional properties for child nodes: > - max-microamp : maximum intensity in microamperes of the LED > (torch LED for flash devices) > - flash-max-microamp : maximum intensity in microamperes of the > flash LED; it is mandatory if the led should > support the flash mode > - flash-timeout-microsec : timeout in microseconds after which the flash > led is turned off > - indicator-pattern : identifier of the blinking pattern for the > indicator led > > ========================================================== > > Regards, > Jacek > > [1] > http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/84643 >
Hi Pavel, On 11/20/2014 01:12 PM, Pavel Machek wrote: > Hi! > >> I would also swap the segments of a property name to follow the convention >> as in case of "regulator-max-microamp". >> >> Updated version: >> >> ========================================================== >> >> Optional properties for child nodes: >> - max-microamp : maximum intensity in microamperes of the LED >> (torch LED for flash devices) >> - flash-max-microamp : maximum intensity in microamperes of the >> flash LED; it is mandatory if the led should >> support the flash mode >> - flash-timeout-microsec : timeout in microseconds after which the flash >> led is turned off > > Works for me. Do you want to submit a patch or should I do it? You can submit a patch for leds/common.txt and a separate patch for the adp1653 with a reference to the leds/common.txt for the child nodes. > >> - indicator-pattern : identifier of the blinking pattern for the >> indicator led >> > > This would need a bit more documentation, no? - indicator-pattern : identifier of the blinking pattern for the indicator led; valid identifiers should be defined in the documentation of the parent node. I wouldn't go for pre-defined identifiers as the pattern can be a combination of various settings like ramp-up, ramp-down, pulse time etc. Drivers should expose only few combinations of these settings in my opinion, like e.g. leds-lm355x.c does. Regards, Jacek
Hi, >> Can you capture raw bayer images correctly? I assume green >> means YUV buffers that are all zero. >> >> Do you know more specifically which patch breaks it? > > CCing freemangordon (Ivaylo Dimitrov). He tried to debug it > months ago but without success. Should know more info about this > problem. > > I think that commit which broke it was not bisected... > According to my vague memories, the green captured image was a result from the ISP IRQ never got triggered. I tried to find why, but never succeeded. Sakari, we discussed that on #maemo-ssu when I was playing with cameras, and there is nothing new in that regard: http://mg.pov.lt/maemo-ssu-irclog/%23maemo-ssu.2013-09-18.log.html#t2013-09-18T18:46:38
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 739fcf2..ed0bfc1 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -553,6 +561,18 @@ ti,usb-charger-detection = <&isp1704>; }; + + adp1653: adp1653@30 { + compatible = "ad,adp1653"; + reg = <0x30>; + + max-flash-timeout-usec = <500000>; + max-flash-intensity-uA = <320000>; + max-torch-intensity-uA = <50000>; + max-indicator-intensity-uA = <17500>; + + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* Want 88 */ + }; }; &i2c3 { diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 873fe19..e21ed02 100644 --- a/drivers/media/i2c/adp1653.c +++ b/drivers/media/i2c/adp1653.c @@ -8,6 +8,7 @@ * Contributors: * Sakari Ailus <sakari.ailus@iki.fi> * Tuukka Toivonen <tuukkat76@gmail.com> + * Pavel Machek <pavel@ucw.cz> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -34,9 +35,12 @@ #include <linux/module.h> #include <linux/i2c.h> #include <linux/slab.h> +#include <linux/of_gpio.h> #include <media/adp1653.h> #include <media/v4l2-device.h> +#include <linux/gpio.h> + #define TIMEOUT_MAX 820000 #define TIMEOUT_STEP 54600 #define TIMEOUT_MIN (TIMEOUT_MAX - ADP1653_REG_CONFIG_TMR_SET_MAX \ @@ -308,7 +316,16 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) { int ret; - ret = flash->platform_data->power(&flash->subdev, on); + if (flash->platform_data->power) + ret = flash->platform_data->power(&flash->subdev, on); + else { + gpio_set_value(flash->platform_data->power_gpio, on); + if (on) { + /* Some delay is apparently required. */ + udelay(20); + } + } + if (ret < 0) return ret; @@ -316,8 +333,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) return 0; ret = adp1653_init_device(flash); - if (ret < 0) + if (ret >= 0) + return ret; + + if (flash->platform_data->power) flash->platform_data->power(&flash->subdev, 0); + else + gpio_set_value(flash->platform_data->power_gpio, 0); return ret; } @@ -407,21 +429,87 @@ static int adp1653_resume(struct device *dev) #endif /* CONFIG_PM */ + + + + + + + + + + + + + +static int adp1653_of_init(struct i2c_client *client, struct adp1653_flash *flash, + struct device_node *node) +{ + u32 val; + struct adp1653_platform_data *pd; + enum of_gpio_flags flags; + int gpio; + + if (!node) + return -EINVAL; + + printk("adp1653: no platform data\n"); + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); + if (!pd) + return -ENOMEM; + flash->platform_data = pd; + + + + + + + + if (of_property_read_u32(node, "max-flash-timeout-usec", &val)) return -EINVAL; + pd->max_flash_timeout = val; + if (of_property_read_u32(node, "max-flash-intensity-uA", &val)) return -EINVAL; + pd->max_flash_intensity = val/1000; + if (of_property_read_u32(node, "max-torch-intensity-uA", &val)) return -EINVAL; + pd->max_torch_intensity = val/1000; + if (of_property_read_u32(node, "max-indicator-intensity-uA", &val)) return -EINVAL; + pd->max_indicator_intensity = val; + + if (!of_find_property(node, "gpios", NULL)) { + printk("No gpio node\n"); + return -EINVAL; + } + + gpio = of_get_gpio_flags(node, 0, &flags); + if (gpio < 0) { + printk("Error getting GPIO\n"); + return -EINVAL; + } + + pd->power_gpio = gpio; + + return 0; +} + + static int adp1653_probe(struct i2c_client *client, const struct i2c_device_id *devid) { struct adp1653_flash *flash; int ret; - /* we couldn't work without platform data */ - if (client->dev.platform_data == NULL) - return -ENODEV; + printk("adp1653: probe\n"); flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); if (flash == NULL) return -ENOMEM; - flash->platform_data = client->dev.platform_data; + /* we couldn't work without platform data */ + if (client->dev.platform_data == NULL) { + ret = adp1653_of_init(client, flash, client->dev.of_node); + if (ret) + return ret; + } else + flash->platform_data = client->dev.platform_data; mutex_init(&flash->power_lock); @@ -439,9 +527,15 @@ static int adp1653_probe(struct i2c_client *client, flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; + __adp1653_set_power(flash, 1); + + __adp1653_set_power(flash, 0); + printk("Flash should have blinked\n"); + return 0; free_and_quit: + printk("adp1653: something failed registering\n"); v4l2_ctrl_handler_free(&flash->ctrls); return ret; } diff --git a/include/media/adp1653.h b/include/media/adp1653.h index 1d9b48a..b556580 100644 --- a/include/media/adp1653.h +++ b/include/media/adp1653.h @@ -100,9 +100,11 @@ struct adp1653_platform_data { int (*power)(struct v4l2_subdev *sd, int on); u32 max_flash_timeout; /* flash light timeout in us */ - u32 max_flash_intensity; /* led intensity, flash mode */ - u32 max_torch_intensity; /* led intensity, torch mode */ - u32 max_indicator_intensity; /* indicator led intensity */ + u32 max_flash_intensity; /* led intensity, flash mode, mA */ + u32 max_torch_intensity; /* led intensity, torch mode, mA */ + u32 max_indicator_intensity; /* indicator led intensity, uA */ + + int power_gpio; /* for device-tree based boot */ }; #define to_adp1653_flash(sd) container_of(sd, struct adp1653_flash, subdev)
For device tree people: Yes, I know I'll have to create file in documentation, but does the binding below look acceptable? I'll clean up driver code a bit more, remove the printks. Anything else obviously wrong? Signed-off-by: Pavel Machek <pavel@ucw.cz> Thanks, Pavel