[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

I'm not sure if it is device tree or media framework, either everyone
waits for someone else, or noone really cares.

Andrew, can you just merge it?

Please apply,
							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..0341009 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,77 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
 		goto free_and_quit;
 
 	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
-
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

I'm not sure if it is device tree or media framework, either everyone
waits for someone else, or noone really cares.

Andrew, can you just merge it?

Please apply,
							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..0341009 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,77 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
 		goto free_and_quit;
 
 	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
-
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

My apologies for the very late reply.

On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> 
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> I'm not sure if it is device tree or media framework, either everyone
> waits for someone else, or noone really cares.

Neither. Some people are unfortuantely very busy with many other things as
well. :-P

> Andrew, can you just merge it?
> 
> Please apply,

Please wait just a while.

I think we can merge this eventually through the linux-media tree, but
please first see the comments below.

> 							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - gpios: References to the GPIO that controls the power for the chip.
> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt

The documentation says that the maximum value is used if these values are
not specified. I think I'd make these optional.

> +
> +Example:
> +
> +	adp1653: led-controller@30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..0341009 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);

The power() callback should be dropped. It's controlling a GPIO. But that
can be done later on. The alternative is a patch before this one.

> +	} else {
> +		gpio_set_value(flash->platform_data->power_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -316,8 +327,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 +423,77 @@ 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;
> +	struct device_node *child;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +		return -EINVAL;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_torch_intensity = val/1000;
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_indicator_intensity = val;
> +
> +	if (!of_find_property(node, "gpios", NULL)) {
> +		dev_err(&client->dev, "No gpio node\n");
> +		return -EINVAL;
> +	}
> +
> +	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> +	if (pd->power_gpio < 0) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;
> +	if (!flash->platform_data) {

I'd check whether dev->of_node is non-NULL instead.

> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
>  		goto free_and_quit;
>  
>  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> -

I rather liked the newline here. Please don't remove it. :-)

>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
>  
> 

A corresponding change to the N900 dts would be very nice.

I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

-- 
Kind regards,

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

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

My apologies for the very late reply.

On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> 
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> I'm not sure if it is device tree or media framework, either everyone
> waits for someone else, or noone really cares.

Neither. Some people are unfortuantely very busy with many other things as
well. :-P

> Andrew, can you just merge it?
> 
> Please apply,

Please wait just a while.

I think we can merge this eventually through the linux-media tree, but
please first see the comments below.

> 							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - gpios: References to the GPIO that controls the power for the chip.
> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt

The documentation says that the maximum value is used if these values are
not specified. I think I'd make these optional.

> +
> +Example:
> +
> +	adp1653: led-controller at 30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..0341009 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);

The power() callback should be dropped. It's controlling a GPIO. But that
can be done later on. The alternative is a patch before this one.

> +	} else {
> +		gpio_set_value(flash->platform_data->power_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -316,8 +327,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 +423,77 @@ 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;
> +	struct device_node *child;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +		return -EINVAL;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_torch_intensity = val/1000;
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_indicator_intensity = val;
> +
> +	if (!of_find_property(node, "gpios", NULL)) {
> +		dev_err(&client->dev, "No gpio node\n");
> +		return -EINVAL;
> +	}
> +
> +	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> +	if (pd->power_gpio < 0) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;
> +	if (!flash->platform_data) {

I'd check whether dev->of_node is non-NULL instead.

> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
>  		goto free_and_quit;
>  
>  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> -

I rather liked the newline here. Please don't remove it. :-)

>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
>  
> 

A corresponding change to the N900 dts would be very nice.

I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

-- 
Kind regards,

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

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

My apologies for the very late reply.

On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> 
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> ---
> 
> I'm not sure if it is device tree or media framework, either everyone
> waits for someone else, or noone really cares.

Neither. Some people are unfortuantely very busy with many other things as
well. :-P

> Andrew, can you just merge it?
> 
> Please apply,

Please wait just a while.

I think we can merge this eventually through the linux-media tree, but
please first see the comments below.

> 							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - gpios: References to the GPIO that controls the power for the chip.
> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt

The documentation says that the maximum value is used if these values are
not specified. I think I'd make these optional.

> +
> +Example:
> +
> +	adp1653: led-controller@30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..0341009 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -8,6 +8,7 @@
>   * Contributors:
>   *	Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
>   *	Tuukka Toivonen <tuukkat76-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *	Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License
> @@ -34,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);

The power() callback should be dropped. It's controlling a GPIO. But that
can be done later on. The alternative is a patch before this one.

> +	} else {
> +		gpio_set_value(flash->platform_data->power_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -316,8 +327,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 +423,77 @@ 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;
> +	struct device_node *child;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +		return -EINVAL;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_torch_intensity = val/1000;
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_indicator_intensity = val;
> +
> +	if (!of_find_property(node, "gpios", NULL)) {
> +		dev_err(&client->dev, "No gpio node\n");
> +		return -EINVAL;
> +	}
> +
> +	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> +	if (pd->power_gpio < 0) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;
> +	if (!flash->platform_data) {

I'd check whether dev->of_node is non-NULL instead.

> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
>  		goto free_and_quit;
>  
>  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> -

I rather liked the newline here. Please don't remove it. :-)

>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
>  
> 

A corresponding change to the N900 dts would be very nice.

I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> Hi Pawel,
> 
> My apologies for the very late reply.
> 
> On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > 
> > 
> > We are moving to device tree support on OMAP3, but that currently
> > breaks ADP1653 driver. This adds device tree support, plus required
> > documentation.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> > 
> > I'm not sure if it is device tree or media framework, either everyone
> > waits for someone else, or noone really cares.
> 
> Neither. Some people are unfortuantely very busy with many other things as
> well. :-P

Well.. Being busy is ok. Nitpicking is also ok. But both at the same
time... is not good. 

> > Andrew, can you just merge it?
> > 
> > Please apply,
> 
> Please wait just a while.
> 
> I think we can merge this eventually through the linux-media tree, but
> please first see the comments below.
> 

> > +Required properties of the flash LED child node:
> > +
> > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> 
> The documentation says that the maximum value is used if these values are
> not specified. I think I'd make these optional.

I'd rather not: when you make a typo in dts, it would supply maximum
available current, potentially damaging the LED. You will not be able
to tell brightness difference with naked eye...

> >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> >  {
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	if (flash->platform_data->power) {
> > +		ret = flash->platform_data->power(&flash->subdev, on);
> 
> The power() callback should be dropped. It's controlling a GPIO. But that
> can be done later on. The alternative is a patch before this one.

I'd prefer to do it later; we want to keep functionality on N900
without DTS, too.

> >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> >  	if (flash == NULL)
> >  		return -ENOMEM;
> >  
> >  	flash->platform_data = client->dev.platform_data;
> > +	if (!flash->platform_data) {
> 
> I'd check whether dev->of_node is non-NULL instead.

Ok.

> > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> >  		goto free_and_quit;
> >  
> >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > -
> 
> I rather liked the newline here. Please don't remove it. :-)

Ok.

> > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> >  
> > -static struct dev_pm_ops adp1653_pm_ops = {
> > +static const struct dev_pm_ops adp1653_pm_ops = {
> >  	.suspend	= adp1653_suspend,
> >  	.resume		= adp1653_resume,
> >  };
> >  
> > 
> 
> A corresponding change to the N900 dts would be very nice.

Corresponding change to the dts will come in separate patch. Or do you
have n900 for testing?

> I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

It actually worked for me, which means device tree somehow does it
magic.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> Hi Pawel,
> 
> My apologies for the very late reply.
> 
> On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > 
> > 
> > We are moving to device tree support on OMAP3, but that currently
> > breaks ADP1653 driver. This adds device tree support, plus required
> > documentation.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> > 
> > I'm not sure if it is device tree or media framework, either everyone
> > waits for someone else, or noone really cares.
> 
> Neither. Some people are unfortuantely very busy with many other things as
> well. :-P

Well.. Being busy is ok. Nitpicking is also ok. But both at the same
time... is not good. 

> > Andrew, can you just merge it?
> > 
> > Please apply,
> 
> Please wait just a while.
> 
> I think we can merge this eventually through the linux-media tree, but
> please first see the comments below.
> 

> > +Required properties of the flash LED child node:
> > +
> > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> 
> The documentation says that the maximum value is used if these values are
> not specified. I think I'd make these optional.

I'd rather not: when you make a typo in dts, it would supply maximum
available current, potentially damaging the LED. You will not be able
to tell brightness difference with naked eye...

> >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> >  {
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	if (flash->platform_data->power) {
> > +		ret = flash->platform_data->power(&flash->subdev, on);
> 
> The power() callback should be dropped. It's controlling a GPIO. But that
> can be done later on. The alternative is a patch before this one.

I'd prefer to do it later; we want to keep functionality on N900
without DTS, too.

> >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> >  	if (flash == NULL)
> >  		return -ENOMEM;
> >  
> >  	flash->platform_data = client->dev.platform_data;
> > +	if (!flash->platform_data) {
> 
> I'd check whether dev->of_node is non-NULL instead.

Ok.

> > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> >  		goto free_and_quit;
> >  
> >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > -
> 
> I rather liked the newline here. Please don't remove it. :-)

Ok.

> > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> >  
> > -static struct dev_pm_ops adp1653_pm_ops = {
> > +static const struct dev_pm_ops adp1653_pm_ops = {
> >  	.suspend	= adp1653_suspend,
> >  	.resume		= adp1653_resume,
> >  };
> >  
> > 
> 
> A corresponding change to the N900 dts would be very nice.

Corresponding change to the dts will come in separate patch. Or do you
have n900 for testing?

> I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

It actually worked for me, which means device tree somehow does it
magic.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> Hi Pawel,
> 
> My apologies for the very late reply.
> 
> On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > 
> > 
> > We are moving to device tree support on OMAP3, but that currently
> > breaks ADP1653 driver. This adds device tree support, plus required
> > documentation.
> > 
> > Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > 
> > ---
> > 
> > I'm not sure if it is device tree or media framework, either everyone
> > waits for someone else, or noone really cares.
> 
> Neither. Some people are unfortuantely very busy with many other things as
> well. :-P

Well.. Being busy is ok. Nitpicking is also ok. But both at the same
time... is not good. 

> > Andrew, can you just merge it?
> > 
> > Please apply,
> 
> Please wait just a while.
> 
> I think we can merge this eventually through the linux-media tree, but
> please first see the comments below.
> 

> > +Required properties of the flash LED child node:
> > +
> > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> 
> The documentation says that the maximum value is used if these values are
> not specified. I think I'd make these optional.

I'd rather not: when you make a typo in dts, it would supply maximum
available current, potentially damaging the LED. You will not be able
to tell brightness difference with naked eye...

> >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> >  {
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	if (flash->platform_data->power) {
> > +		ret = flash->platform_data->power(&flash->subdev, on);
> 
> The power() callback should be dropped. It's controlling a GPIO. But that
> can be done later on. The alternative is a patch before this one.

I'd prefer to do it later; we want to keep functionality on N900
without DTS, too.

> >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> >  	if (flash == NULL)
> >  		return -ENOMEM;
> >  
> >  	flash->platform_data = client->dev.platform_data;
> > +	if (!flash->platform_data) {
> 
> I'd check whether dev->of_node is non-NULL instead.

Ok.

> > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> >  		goto free_and_quit;
> >  
> >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > -
> 
> I rather liked the newline here. Please don't remove it. :-)

Ok.

> > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> >  
> > -static struct dev_pm_ops adp1653_pm_ops = {
> > +static const struct dev_pm_ops adp1653_pm_ops = {
> >  	.suspend	= adp1653_suspend,
> >  	.resume		= adp1653_resume,
> >  };
> >  
> > 
> 
> A corresponding change to the N900 dts would be very nice.

Corresponding change to the dts will come in separate patch. Or do you
have n900 for testing?

> I think you're missing change to adp1653_i2c_driver.driver.of_match_table.

It actually worked for me, which means device tree somehow does it
magic.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

On Thu, Apr 02, 2015 at 10:30:44PM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> > 
> > My apologies for the very late reply.
> > 
> > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > > 
> > > 
> > > We are moving to device tree support on OMAP3, but that currently
> > > breaks ADP1653 driver. This adds device tree support, plus required
> > > documentation.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > > 
> > > I'm not sure if it is device tree or media framework, either everyone
> > > waits for someone else, or noone really cares.
> > 
> > Neither. Some people are unfortuantely very busy with many other things as
> > well. :-P
> 
> Well.. Being busy is ok. Nitpicking is also ok. But both at the same
> time... is not good. 

Good. Then we should be fine. :-)

> 
> > > Andrew, can you just merge it?
> > > 
> > > Please apply,
> > 
> > Please wait just a while.
> > 
> > I think we can merge this eventually through the linux-media tree, but
> > please first see the comments below.
> > 
> 
> > > +Required properties of the flash LED child node:
> > > +
> > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> > 
> > The documentation says that the maximum value is used if these values are
> > not specified. I think I'd make these optional.
> 
> I'd rather not: when you make a typo in dts, it would supply maximum
> available current, potentially damaging the LED. You will not be able
> to tell brightness difference with naked eye...

Fine for me.

> > >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> > >  {
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	if (flash->platform_data->power) {
> > > +		ret = flash->platform_data->power(&flash->subdev, on);
> > 
> > The power() callback should be dropped. It's controlling a GPIO. But that
> > can be done later on. The alternative is a patch before this one.
> 
> I'd prefer to do it later; we want to keep functionality on N900
> without DTS, too.

Fine as well.

> > >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> > >  	if (flash == NULL)
> > >  		return -ENOMEM;
> > >  
> > >  	flash->platform_data = client->dev.platform_data;
> > > +	if (!flash->platform_data) {
> > 
> > I'd check whether dev->of_node is non-NULL instead.
> 
> Ok.
> 
> > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> > >  		goto free_and_quit;
> > >  
> > >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > > -
> > 
> > I rather liked the newline here. Please don't remove it. :-)
> 
> Ok.
> 
> > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> > >  
> > > -static struct dev_pm_ops adp1653_pm_ops = {
> > > +static const struct dev_pm_ops adp1653_pm_ops = {
> > >  	.suspend	= adp1653_suspend,
> > >  	.resume		= adp1653_resume,
> > >  };
> > >  
> > > 
> > 
> > A corresponding change to the N900 dts would be very nice.
> 
> Corresponding change to the dts will come in separate patch. Or do you
> have n900 for testing?

Yes, it should be a separate patch, I agree.

I do have one but I can't say when I'd have time to test it. I'm fine with
you having tested it though.

> > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> 
> It actually worked for me, which means device tree somehow does it
> magic.

By magic? :-) It probably just ends up comparing the device and the driver
names. How about adding the of_match_table?

-- 
Regards,

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

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

On Thu, Apr 02, 2015 at 10:30:44PM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> > 
> > My apologies for the very late reply.
> > 
> > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > > 
> > > 
> > > We are moving to device tree support on OMAP3, but that currently
> > > breaks ADP1653 driver. This adds device tree support, plus required
> > > documentation.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > > 
> > > I'm not sure if it is device tree or media framework, either everyone
> > > waits for someone else, or noone really cares.
> > 
> > Neither. Some people are unfortuantely very busy with many other things as
> > well. :-P
> 
> Well.. Being busy is ok. Nitpicking is also ok. But both at the same
> time... is not good. 

Good. Then we should be fine. :-)

> 
> > > Andrew, can you just merge it?
> > > 
> > > Please apply,
> > 
> > Please wait just a while.
> > 
> > I think we can merge this eventually through the linux-media tree, but
> > please first see the comments below.
> > 
> 
> > > +Required properties of the flash LED child node:
> > > +
> > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> > 
> > The documentation says that the maximum value is used if these values are
> > not specified. I think I'd make these optional.
> 
> I'd rather not: when you make a typo in dts, it would supply maximum
> available current, potentially damaging the LED. You will not be able
> to tell brightness difference with naked eye...

Fine for me.

> > >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> > >  {
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	if (flash->platform_data->power) {
> > > +		ret = flash->platform_data->power(&flash->subdev, on);
> > 
> > The power() callback should be dropped. It's controlling a GPIO. But that
> > can be done later on. The alternative is a patch before this one.
> 
> I'd prefer to do it later; we want to keep functionality on N900
> without DTS, too.

Fine as well.

> > >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> > >  	if (flash == NULL)
> > >  		return -ENOMEM;
> > >  
> > >  	flash->platform_data = client->dev.platform_data;
> > > +	if (!flash->platform_data) {
> > 
> > I'd check whether dev->of_node is non-NULL instead.
> 
> Ok.
> 
> > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> > >  		goto free_and_quit;
> > >  
> > >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > > -
> > 
> > I rather liked the newline here. Please don't remove it. :-)
> 
> Ok.
> 
> > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> > >  
> > > -static struct dev_pm_ops adp1653_pm_ops = {
> > > +static const struct dev_pm_ops adp1653_pm_ops = {
> > >  	.suspend	= adp1653_suspend,
> > >  	.resume		= adp1653_resume,
> > >  };
> > >  
> > > 
> > 
> > A corresponding change to the N900 dts would be very nice.
> 
> Corresponding change to the dts will come in separate patch. Or do you
> have n900 for testing?

Yes, it should be a separate patch, I agree.

I do have one but I can't say when I'd have time to test it. I'm fine with
you having tested it though.

> > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> 
> It actually worked for me, which means device tree somehow does it
> magic.

By magic? :-) It probably just ends up comparing the device and the driver
names. How about adding the of_match_table?

-- 
Regards,

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

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pawel,

On Thu, Apr 02, 2015 at 10:30:44PM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> > 
> > My apologies for the very late reply.
> > 
> > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > > 
> > > 
> > > We are moving to device tree support on OMAP3, but that currently
> > > breaks ADP1653 driver. This adds device tree support, plus required
> > > documentation.
> > > 
> > > Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > > 
> > > ---
> > > 
> > > I'm not sure if it is device tree or media framework, either everyone
> > > waits for someone else, or noone really cares.
> > 
> > Neither. Some people are unfortuantely very busy with many other things as
> > well. :-P
> 
> Well.. Being busy is ok. Nitpicking is also ok. But both at the same
> time... is not good. 

Good. Then we should be fine. :-)

> 
> > > Andrew, can you just merge it?
> > > 
> > > Please apply,
> > 
> > Please wait just a while.
> > 
> > I think we can merge this eventually through the linux-media tree, but
> > please first see the comments below.
> > 
> 
> > > +Required properties of the flash LED child node:
> > > +
> > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> > 
> > The documentation says that the maximum value is used if these values are
> > not specified. I think I'd make these optional.
> 
> I'd rather not: when you make a typo in dts, it would supply maximum
> available current, potentially damaging the LED. You will not be able
> to tell brightness difference with naked eye...

Fine for me.

> > >  __adp1653_set_power(struct adp1653_flash *flash, int on)
> > >  {
> > > -	int ret;
> > > +	int ret = 0;
> > > +
> > > +	if (flash->platform_data->power) {
> > > +		ret = flash->platform_data->power(&flash->subdev, on);
> > 
> > The power() callback should be dropped. It's controlling a GPIO. But that
> > can be done later on. The alternative is a patch before this one.
> 
> I'd prefer to do it later; we want to keep functionality on N900
> without DTS, too.

Fine as well.

> > >  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> > >  	if (flash == NULL)
> > >  		return -ENOMEM;
> > >  
> > >  	flash->platform_data = client->dev.platform_data;
> > > +	if (!flash->platform_data) {
> > 
> > I'd check whether dev->of_node is non-NULL instead.
> 
> Ok.
> 
> > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> > >  		goto free_and_quit;
> > >  
> > >  	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > > -
> > 
> > I rather liked the newline here. Please don't remove it. :-)
> 
> Ok.
> 
> > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> > >  
> > > -static struct dev_pm_ops adp1653_pm_ops = {
> > > +static const struct dev_pm_ops adp1653_pm_ops = {
> > >  	.suspend	= adp1653_suspend,
> > >  	.resume		= adp1653_resume,
> > >  };
> > >  
> > > 
> > 
> > A corresponding change to the N900 dts would be very nice.
> 
> Corresponding change to the dts will come in separate patch. Or do you
> have n900 for testing?

Yes, it should be a separate patch, I agree.

I do have one but I can't say when I'd have time to test it. I'm fine with
you having tested it though.

> > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> 
> It actually worked for me, which means device tree somehow does it
> magic.

By magic? :-) It probably just ends up comparing the device and the driver
names. How about adding the of_match_table?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> Hi Pawel,

I'm still Pavel. v, not w.

> > > Hi Pawel,

> > > A corresponding change to the N900 dts would be very nice.
> > 
> > Corresponding change to the dts will come in separate patch. Or do you
> > have n900 for testing?
> 
> Yes, it should be a separate patch, I agree.
> 
> I do have one but I can't say when I'd have time to test it. I'm fine with
> you having tested it though.
> 
> > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> > 
> > It actually worked for me, which means device tree somehow does it
> > magic.
> 
> By magic? :-) It probably just ends up comparing the device and the driver
> names. How about adding the of_match_table?

I guess it uses adp1653_id_table. I'd hade to add redundand
information, because if it would just mask the errors if the code
changed...

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> Hi Pawel,

I'm still Pavel. v, not w.

> > > Hi Pawel,

> > > A corresponding change to the N900 dts would be very nice.
> > 
> > Corresponding change to the dts will come in separate patch. Or do you
> > have n900 for testing?
> 
> Yes, it should be a separate patch, I agree.
> 
> I do have one but I can't say when I'd have time to test it. I'm fine with
> you having tested it though.
> 
> > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> > 
> > It actually worked for me, which means device tree somehow does it
> > magic.
> 
> By magic? :-) It probably just ends up comparing the device and the driver
> names. How about adding the of_match_table?

I guess it uses adp1653_id_table. I'd hade to add redundand
information, because if it would just mask the errors if the code
changed...

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> 
> I'm still Pavel. v, not w.

I know too many Pawels. Sorry about that. :-)

> 
> > > > Hi Pawel,
> 
> > > > A corresponding change to the N900 dts would be very nice.
> > > 
> > > Corresponding change to the dts will come in separate patch. Or do you
> > > have n900 for testing?
> > 
> > Yes, it should be a separate patch, I agree.
> > 
> > I do have one but I can't say when I'd have time to test it. I'm fine with
> > you having tested it though.
> > 
> > > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> > > 
> > > It actually worked for me, which means device tree somehow does it
> > > magic.
> > 
> > By magic? :-) It probably just ends up comparing the device and the driver
> > names. How about adding the of_match_table?
> 
> I guess it uses adp1653_id_table. I'd hade to add redundand
> information, because if it would just mask the errors if the code
> changed...

Indeed, that's true. This is comparing "adp1653" vs. comparing
"adi,adp1653". I think I still prefer the latter since it's got also the
vendor prefix included.

Suppose we change this later and someone misspelled the vendor prefix ---
their board would break.

-- 
Regards,

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

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> 
> I'm still Pavel. v, not w.

I know too many Pawels. Sorry about that. :-)

> 
> > > > Hi Pawel,
> 
> > > > A corresponding change to the N900 dts would be very nice.
> > > 
> > > Corresponding change to the dts will come in separate patch. Or do you
> > > have n900 for testing?
> > 
> > Yes, it should be a separate patch, I agree.
> > 
> > I do have one but I can't say when I'd have time to test it. I'm fine with
> > you having tested it though.
> > 
> > > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> > > 
> > > It actually worked for me, which means device tree somehow does it
> > > magic.
> > 
> > By magic? :-) It probably just ends up comparing the device and the driver
> > names. How about adding the of_match_table?
> 
> I guess it uses adp1653_id_table. I'd hade to add redundand
> information, because if it would just mask the errors if the code
> changed...

Indeed, that's true. This is comparing "adp1653" vs. comparing
"adi,adp1653". I think I still prefer the latter since it's got also the
vendor prefix included.

Suppose we change this later and someone misspelled the vendor prefix ---
their board would break.

-- 
Regards,

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

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hi Pawel,
> 
> I'm still Pavel. v, not w.

I know too many Pawels. Sorry about that. :-)

> 
> > > > Hi Pawel,
> 
> > > > A corresponding change to the N900 dts would be very nice.
> > > 
> > > Corresponding change to the dts will come in separate patch. Or do you
> > > have n900 for testing?
> > 
> > Yes, it should be a separate patch, I agree.
> > 
> > I do have one but I can't say when I'd have time to test it. I'm fine with
> > you having tested it though.
> > 
> > > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
> > > 
> > > It actually worked for me, which means device tree somehow does it
> > > magic.
> > 
> > By magic? :-) It probably just ends up comparing the device and the driver
> > names. How about adding the of_match_table?
> 
> I guess it uses adp1653_id_table. I'd hade to add redundand
> information, because if it would just mask the errors if the code
> changed...

Indeed, that's true. This is comparing "adp1653" vs. comparing
"adi,adp1653". I think I still prefer the latter since it's got also the
vendor prefix included.

Suppose we change this later and someone misspelled the vendor prefix ---
their board would break.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

On Fri 2015-04-03 14:23:56, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > Hi Pawel,
> > 
> > I'm still Pavel. v, not w.
> 
> I know too many Pawels. Sorry about that. :-)
> 

> > I guess it uses adp1653_id_table. I'd hade to add redundand
> > information, because if it would just mask the errors if the code
> > changed...
> 
> Indeed, that's true. This is comparing "adp1653" vs. comparing
> "adi,adp1653". I think I still prefer the latter since it's got also the
> vendor prefix included.
> 
> Suppose we change this later and someone misspelled the vendor prefix ---
> their board would break.

Suppose we do what you suggest. That does not fix the problem, since
code will still match the "adp1653" in case someone misspells it.

If you want to change how i2c device matching works, well, you can do
it, but my patch is not right place to do that.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

On Fri 2015-04-03 14:23:56, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > Hi Pawel,
> > 
> > I'm still Pavel. v, not w.
> 
> I know too many Pawels. Sorry about that. :-)
> 

> > I guess it uses adp1653_id_table. I'd hade to add redundand
> > information, because if it would just mask the errors if the code
> > changed...
> 
> Indeed, that's true. This is comparing "adp1653" vs. comparing
> "adi,adp1653". I think I still prefer the latter since it's got also the
> vendor prefix included.
> 
> Suppose we change this later and someone misspelled the vendor prefix ---
> their board would break.

Suppose we do what you suggest. That does not fix the problem, since
code will still match the "adp1653" in case someone misspells it.

If you want to change how i2c device matching works, well, you can do
it, but my patch is not right place to do that.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

On Fri, Apr 03, 2015 at 10:29:53PM +0200, Pavel Machek wrote:
> On Fri 2015-04-03 14:23:56, Sakari Ailus wrote:
> > Hi Pavel,
> > 
> > On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Hi Pawel,
> > > 
> > > I'm still Pavel. v, not w.
> > 
> > I know too many Pawels. Sorry about that. :-)
> > 
> 
> > > I guess it uses adp1653_id_table. I'd hade to add redundand
> > > information, because if it would just mask the errors if the code
> > > changed...
> > 
> > Indeed, that's true. This is comparing "adp1653" vs. comparing
> > "adi,adp1653". I think I still prefer the latter since it's got also the
> > vendor prefix included.
> > 
> > Suppose we change this later and someone misspelled the vendor prefix ---
> > their board would break.
> 
> Suppose we do what you suggest. That does not fix the problem, since
> code will still match the "adp1653" in case someone misspells it.
> 
> If you want to change how i2c device matching works, well, you can do
> it, but my patch is not right place to do that.

Good point. Let's leave it as-is.

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

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

On Fri, Apr 03, 2015 at 10:29:53PM +0200, Pavel Machek wrote:
> On Fri 2015-04-03 14:23:56, Sakari Ailus wrote:
> > Hi Pavel,
> > 
> > On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Hi Pawel,
> > > 
> > > I'm still Pavel. v, not w.
> > 
> > I know too many Pawels. Sorry about that. :-)
> > 
> 
> > > I guess it uses adp1653_id_table. I'd hade to add redundand
> > > information, because if it would just mask the errors if the code
> > > changed...
> > 
> > Indeed, that's true. This is comparing "adp1653" vs. comparing
> > "adi,adp1653". I think I still prefer the latter since it's got also the
> > vendor prefix included.
> > 
> > Suppose we change this later and someone misspelled the vendor prefix ---
> > their board would break.
> 
> Suppose we do what you suggest. That does not fix the problem, since
> code will still match the "adp1653" in case someone misspells it.
> 
> If you want to change how i2c device matching works, well, you can do
> it, but my patch is not right place to do that.

Good point. Let's leave it as-is.

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

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
 
---

Fixed feedback by Sakari.

Please apply,
 							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..6d57b16 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,76 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +513,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
 
---

Fixed feedback by Sakari.

Please apply,
 							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..6d57b16 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,76 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +513,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
 
---

Fixed feedback by Sakari.

Please apply,
 							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..6d57b16 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -8,6 +8,7 @@
  * Contributors:
  *	Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
  *	Tuukka Toivonen <tuukkat76-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *	Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -34,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,76 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +513,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Javier Martinez Canillas]

Hello Pavel,

I haven't reviewed the patch since I'm not familiar with the hardware
but just wanted to point out a couple of things that I spot:

On Thu, Apr 2, 2015 at 10:34 PM, Pavel Machek <pavel@ucw.cz> wrote:
>
>
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> Fixed feedback by Sakari.
>
> Please apply,

There is no need to ask for patches to be applied IMHO. It is expected
that people post patches wanting them to be applied unless there is an
RFC prefix in the subject or say explicitly that the patch is for
testing and should not be picked.

>                                                         Pavel
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt

When adding DT bindings, the Documentation portion should be in a
separate patch and should come in the series before the patch
implementing the binding. That makes the change easier to review,
please take a look to points 1 and 3 in
Documentation/devicetree/bindings/submitting-patches.txt.

> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - gpios: References to the GPIO that controls the power for the chip.

The convention nowadays is to not use unnamed DT properties for GPIOs
but instead use a prefix that explains what those GPIOs are used for.
So something like "power-gpios" or "power-gpio" (if there is only one
GPIO) will be more suitable. Please take a look to
Documentation/gpio/board.txt for more details.

> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +       adp1653: led-controller@30 {
> +               compatible = "adi,adp1653";
> +               reg = <0x30>;
> +               gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +               flash {
> +                       flash-timeout-us = <500000>;
> +                       flash-max-microamp = <320000>;
> +                       max-microamp = <50000>;
> +               };
> +               indicator {
> +                       max-microamp = <17500>;
> +               };
> +       };
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..6d57b16 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -       int ret;
> +       int ret = 0;
> +
> +       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);
> +       }
>
> -       ret = flash->platform_data->power(&flash->subdev, on);
>         if (ret < 0)
>                 return ret;
>
> @@ -316,8 +327,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 +423,76 @@ 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;
> +       struct device_node *child;
> +
> +       if (!node)
> +               return -EINVAL;
> +
> +       pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +       flash->platform_data = pd;
> +
> +       child = of_get_child_by_name(node, "flash");
> +       if (!child)
> +               return -EINVAL;
> +       if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +               return -EINVAL;
> +
> +       pd->max_flash_timeout = val;
> +       if (of_property_read_u32(child, "flash-max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_flash_intensity = val/1000;
> +
> +       if (of_property_read_u32(child, "max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_torch_intensity = val/1000;
> +
> +       child = of_get_child_by_name(node, "indicator");
> +       if (!child)
> +               return -EINVAL;
> +       if (of_property_read_u32(child, "max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_indicator_intensity = val;
> +
> +       if (!of_find_property(node, "gpios", NULL)) {
> +               dev_err(&client->dev, "No gpio node\n");
> +               return -EINVAL;
> +       }
> +
> +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);

The old integer-based GPIO interface is deprecated and we want to get
rid of it so please use the descriptor-based for new code. For example
you want to use gpiod_get() instead of of_get_gpio_flags().
Documentation/gpio/gpio.txt describes the new interface.

> +       if (pd->power_gpio < 0) {
> +               dev_err(&client->dev, "Error getting GPIO\n");
> +               return -EINVAL;
> +       }
> +
> +       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;
> -
>         flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>         if (flash == NULL)
>                 return -ENOMEM;
>
>         flash->platform_data = client->dev.platform_data;
> +       if (client->dev.of_node) {
> +               ret = adp1653_of_init(client, flash, client->dev.of_node);
> +               if (ret)
> +                       return ret;
> +       }
>
>         mutex_init(&flash->power_lock);
>
> @@ -442,6 +513,7 @@ static int adp1653_probe(struct i2c_client *client,
>         return 0;
>
>  free_and_quit:
> +       dev_err(&client->dev, "adp1653: failed to register device\n");
>         v4l2_ctrl_handler_free(&flash->ctrls);
>         return ret;
>  }
> @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>         .suspend        = adp1653_suspend,
>         .resume         = adp1653_resume,
>  };
>

Best regards,
Javier

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Javier Martinez Canillas]

Hello Pavel,

I haven't reviewed the patch since I'm not familiar with the hardware
but just wanted to point out a couple of things that I spot:

On Thu, Apr 2, 2015 at 10:34 PM, Pavel Machek <pavel@ucw.cz> wrote:
>
>
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> Fixed feedback by Sakari.
>
> Please apply,

There is no need to ask for patches to be applied IMHO. It is expected
that people post patches wanting them to be applied unless there is an
RFC prefix in the subject or say explicitly that the patch is for
testing and should not be picked.

>                                                         Pavel
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt

When adding DT bindings, the Documentation portion should be in a
separate patch and should come in the series before the patch
implementing the binding. That makes the change easier to review,
please take a look to points 1 and 3 in
Documentation/devicetree/bindings/submitting-patches.txt.

> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - gpios: References to the GPIO that controls the power for the chip.

The convention nowadays is to not use unnamed DT properties for GPIOs
but instead use a prefix that explains what those GPIOs are used for.
So something like "power-gpios" or "power-gpio" (if there is only one
GPIO) will be more suitable. Please take a look to
Documentation/gpio/board.txt for more details.

> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +       adp1653: led-controller at 30 {
> +               compatible = "adi,adp1653";
> +               reg = <0x30>;
> +               gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +               flash {
> +                       flash-timeout-us = <500000>;
> +                       flash-max-microamp = <320000>;
> +                       max-microamp = <50000>;
> +               };
> +               indicator {
> +                       max-microamp = <17500>;
> +               };
> +       };
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..6d57b16 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -       int ret;
> +       int ret = 0;
> +
> +       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);
> +       }
>
> -       ret = flash->platform_data->power(&flash->subdev, on);
>         if (ret < 0)
>                 return ret;
>
> @@ -316,8 +327,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 +423,76 @@ 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;
> +       struct device_node *child;
> +
> +       if (!node)
> +               return -EINVAL;
> +
> +       pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +       flash->platform_data = pd;
> +
> +       child = of_get_child_by_name(node, "flash");
> +       if (!child)
> +               return -EINVAL;
> +       if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +               return -EINVAL;
> +
> +       pd->max_flash_timeout = val;
> +       if (of_property_read_u32(child, "flash-max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_flash_intensity = val/1000;
> +
> +       if (of_property_read_u32(child, "max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_torch_intensity = val/1000;
> +
> +       child = of_get_child_by_name(node, "indicator");
> +       if (!child)
> +               return -EINVAL;
> +       if (of_property_read_u32(child, "max-microamp", &val))
> +               return -EINVAL;
> +       pd->max_indicator_intensity = val;
> +
> +       if (!of_find_property(node, "gpios", NULL)) {
> +               dev_err(&client->dev, "No gpio node\n");
> +               return -EINVAL;
> +       }
> +
> +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);

The old integer-based GPIO interface is deprecated and we want to get
rid of it so please use the descriptor-based for new code. For example
you want to use gpiod_get() instead of of_get_gpio_flags().
Documentation/gpio/gpio.txt describes the new interface.

> +       if (pd->power_gpio < 0) {
> +               dev_err(&client->dev, "Error getting GPIO\n");
> +               return -EINVAL;
> +       }
> +
> +       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;
> -
>         flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>         if (flash == NULL)
>                 return -ENOMEM;
>
>         flash->platform_data = client->dev.platform_data;
> +       if (client->dev.of_node) {
> +               ret = adp1653_of_init(client, flash, client->dev.of_node);
> +               if (ret)
> +                       return ret;
> +       }
>
>         mutex_init(&flash->power_lock);
>
> @@ -442,6 +513,7 @@ static int adp1653_probe(struct i2c_client *client,
>         return 0;
>
>  free_and_quit:
> +       dev_err(&client->dev, "adp1653: failed to register device\n");
>         v4l2_ctrl_handler_free(&flash->ctrls);
>         return ret;
>  }
> @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>         .suspend        = adp1653_suspend,
>         .resume         = adp1653_resume,
>  };
>

Best regards,
Javier

Re: [PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > Fixed feedback by Sakari.
> >
> > Please apply,
> 
> There is no need to ask for patches to be applied IMHO. It is expected
> that people post patches wanting them to be applied unless there is an
> RFC prefix in the subject or say explicitly that the patch is for
> testing and should not be picked.

See history of this patch.

> > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> 
> When adding DT bindings, the Documentation portion should be in a
> separate patch and should come in the series before the patch
> implementing the binding. That makes the change easier to review,
> please take a look to points 1 and 3 in
> Documentation/devicetree/bindings/submitting-patches.txt.

Because actual patch at the end of email is too much eye clutter for
the poor device tree people, I can prepare nice series... producing
more work for me and more noise on the lists? No, thanks.

> > +Required Properties:
> > +
> > +  - compatible: Must contain be "adi,adp1653"
> > +
> > +  - reg: I2C slave address
> > +
> > +  - gpios: References to the GPIO that controls the power for the chip.
> 
> The convention nowadays is to not use unnamed DT properties for GPIOs
> but instead use a prefix that explains what those GPIOs are used for.
> So something like "power-gpios" or "power-gpio" (if there is only one
> GPIO) will be more suitable. Please take a look to
> Documentation/gpio/board.txt for more details.

Ok. Actually, reading docs below, "power-gpio" will not work, and it
needs to be "power-gpios", right?

> > +       if (!of_find_property(node, "gpios", NULL)) {
> > +               dev_err(&client->dev, "No gpio node\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> 
> The old integer-based GPIO interface is deprecated and we want to get
> rid of it so please use the descriptor-based for new code. For example
> you want to use gpiod_get() instead of of_get_gpio_flags().
> Documentation/gpio/gpio.txt describes the new interface.

Ok.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > Fixed feedback by Sakari.
> >
> > Please apply,
> 
> There is no need to ask for patches to be applied IMHO. It is expected
> that people post patches wanting them to be applied unless there is an
> RFC prefix in the subject or say explicitly that the patch is for
> testing and should not be picked.

See history of this patch.

> > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> 
> When adding DT bindings, the Documentation portion should be in a
> separate patch and should come in the series before the patch
> implementing the binding. That makes the change easier to review,
> please take a look to points 1 and 3 in
> Documentation/devicetree/bindings/submitting-patches.txt.

Because actual patch at the end of email is too much eye clutter for
the poor device tree people, I can prepare nice series... producing
more work for me and more noise on the lists? No, thanks.

> > +Required Properties:
> > +
> > +  - compatible: Must contain be "adi,adp1653"
> > +
> > +  - reg: I2C slave address
> > +
> > +  - gpios: References to the GPIO that controls the power for the chip.
> 
> The convention nowadays is to not use unnamed DT properties for GPIOs
> but instead use a prefix that explains what those GPIOs are used for.
> So something like "power-gpios" or "power-gpio" (if there is only one
> GPIO) will be more suitable. Please take a look to
> Documentation/gpio/board.txt for more details.

Ok. Actually, reading docs below, "power-gpio" will not work, and it
needs to be "power-gpios", right?

> > +       if (!of_find_property(node, "gpios", NULL)) {
> > +               dev_err(&client->dev, "No gpio node\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> 
> The old integer-based GPIO interface is deprecated and we want to get
> rid of it so please use the descriptor-based for new code. For example
> you want to use gpiod_get() instead of of_get_gpio_flags().
> Documentation/gpio/gpio.txt describes the new interface.

Ok.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > Fixed feedback by Sakari.
> >
> > Please apply,
> 
> There is no need to ask for patches to be applied IMHO. It is expected
> that people post patches wanting them to be applied unless there is an
> RFC prefix in the subject or say explicitly that the patch is for
> testing and should not be picked.

See history of this patch.

> > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> 
> When adding DT bindings, the Documentation portion should be in a
> separate patch and should come in the series before the patch
> implementing the binding. That makes the change easier to review,
> please take a look to points 1 and 3 in
> Documentation/devicetree/bindings/submitting-patches.txt.

Because actual patch at the end of email is too much eye clutter for
the poor device tree people, I can prepare nice series... producing
more work for me and more noise on the lists? No, thanks.

> > +Required Properties:
> > +
> > +  - compatible: Must contain be "adi,adp1653"
> > +
> > +  - reg: I2C slave address
> > +
> > +  - gpios: References to the GPIO that controls the power for the chip.
> 
> The convention nowadays is to not use unnamed DT properties for GPIOs
> but instead use a prefix that explains what those GPIOs are used for.
> So something like "power-gpios" or "power-gpio" (if there is only one
> GPIO) will be more suitable. Please take a look to
> Documentation/gpio/board.txt for more details.

Ok. Actually, reading docs below, "power-gpio" will not work, and it
needs to be "power-gpios", right?

> > +       if (!of_find_property(node, "gpios", NULL)) {
> > +               dev_err(&client->dev, "No gpio node\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
> 
> The old integer-based GPIO interface is deprecated and we want to get
> rid of it so please use the descriptor-based for new code. For example
> you want to use gpiod_get() instead of of_get_gpio_flags().
> Documentation/gpio/gpio.txt describes the new interface.

Ok.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Javier Martinez Canillas]

Hello Pavel,

On Fri, Apr 3, 2015 at 10:21 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > Fixed feedback by Sakari.
>> >
>> > Please apply,
>>
>> There is no need to ask for patches to be applied IMHO. It is expected
>> that people post patches wanting them to be applied unless there is an
>> RFC prefix in the subject or say explicitly that the patch is for
>> testing and should not be picked.
>
> See history of this patch.
>
>> > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
>>
>> When adding DT bindings, the Documentation portion should be in a
>> separate patch and should come in the series before the patch
>> implementing the binding. That makes the change easier to review,
>> please take a look to points 1 and 3 in
>> Documentation/devicetree/bindings/submitting-patches.txt.
>
> Because actual patch at the end of email is too much eye clutter for
> the poor device tree people, I can prepare nice series... producing
> more work for me and more noise on the lists? No, thanks.
>

I wonder then what's the point of having written rules if people are
not going to follow...

>> > +Required Properties:
>> > +
>> > +  - compatible: Must contain be "adi,adp1653"
>> > +
>> > +  - reg: I2C slave address
>> > +
>> > +  - gpios: References to the GPIO that controls the power for the chip.
>>
>> The convention nowadays is to not use unnamed DT properties for GPIOs
>> but instead use a prefix that explains what those GPIOs are used for.
>> So something like "power-gpios" or "power-gpio" (if there is only one
>> GPIO) will be more suitable. Please take a look to
>> Documentation/gpio/board.txt for more details.
>
> Ok. Actually, reading docs below, "power-gpio" will not work, and it
> needs to be "power-gpios", right?
>

No, the documentation is not updated. People used <function>-gpio so
at the end it was added as another supported suffix, see commit:
dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").

But I guess it doesn't matter if -gpio or -gpios is used.

>> > +       if (!of_find_property(node, "gpios", NULL)) {
>> > +               dev_err(&client->dev, "No gpio node\n");
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
>>
>> The old integer-based GPIO interface is deprecated and we want to get
>> rid of it so please use the descriptor-based for new code. For example
>> you want to use gpiod_get() instead of of_get_gpio_flags().
>> Documentation/gpio/gpio.txt describes the new interface.
>
> Ok.
>                                                                         Pavel
> --

Best regards,
Javier

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Javier Martinez Canillas]

Hello Pavel,

On Fri, Apr 3, 2015 at 10:21 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > Fixed feedback by Sakari.
>> >
>> > Please apply,
>>
>> There is no need to ask for patches to be applied IMHO. It is expected
>> that people post patches wanting them to be applied unless there is an
>> RFC prefix in the subject or say explicitly that the patch is for
>> testing and should not be picked.
>
> See history of this patch.
>
>> > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
>>
>> When adding DT bindings, the Documentation portion should be in a
>> separate patch and should come in the series before the patch
>> implementing the binding. That makes the change easier to review,
>> please take a look to points 1 and 3 in
>> Documentation/devicetree/bindings/submitting-patches.txt.
>
> Because actual patch at the end of email is too much eye clutter for
> the poor device tree people, I can prepare nice series... producing
> more work for me and more noise on the lists? No, thanks.
>

I wonder then what's the point of having written rules if people are
not going to follow...

>> > +Required Properties:
>> > +
>> > +  - compatible: Must contain be "adi,adp1653"
>> > +
>> > +  - reg: I2C slave address
>> > +
>> > +  - gpios: References to the GPIO that controls the power for the chip.
>>
>> The convention nowadays is to not use unnamed DT properties for GPIOs
>> but instead use a prefix that explains what those GPIOs are used for.
>> So something like "power-gpios" or "power-gpio" (if there is only one
>> GPIO) will be more suitable. Please take a look to
>> Documentation/gpio/board.txt for more details.
>
> Ok. Actually, reading docs below, "power-gpio" will not work, and it
> needs to be "power-gpios", right?
>

No, the documentation is not updated. People used <function>-gpio so
at the end it was added as another supported suffix, see commit:
dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").

But I guess it doesn't matter if -gpio or -gpios is used.

>> > +       if (!of_find_property(node, "gpios", NULL)) {
>> > +               dev_err(&client->dev, "No gpio node\n");
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
>>
>> The old integer-based GPIO interface is deprecated and we want to get
>> rid of it so please use the descriptor-based for new code. For example
>> you want to use gpiod_get() instead of of_get_gpio_flags().
>> Documentation/gpio/gpio.txt describes the new interface.
>
> Ok.
>                                                                         Pavel
> --

Best regards,
Javier

Re: [PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Hi,

On Fri, Apr 03, 2015 at 10:49:12AM +0200, Javier Martinez Canillas wrote:
> >> The convention nowadays is to not use unnamed DT properties for GPIOs
> >> but instead use a prefix that explains what those GPIOs are used for.
> >> So something like "power-gpios" or "power-gpio" (if there is only one
> >> GPIO) will be more suitable. Please take a look to
> >> Documentation/gpio/board.txt for more details.
> >
> > Ok. Actually, reading docs below, "power-gpio" will not work, and it
> > needs to be "power-gpios", right?
> >
> 
> No, the documentation is not updated. People used <function>-gpio so
> at the end it was added as another supported suffix, see commit:
> dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").
> 
> But I guess it doesn't matter if -gpio or -gpios is used.

I suggest to use enable-gpio(s), since the pin is called "enable" in
the datasheet.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv4] media: i2c/adp1653: devicetree support for adp1653

[Sebastian Reichel]

Hi,

On Fri, Apr 03, 2015 at 10:49:12AM +0200, Javier Martinez Canillas wrote:
> >> The convention nowadays is to not use unnamed DT properties for GPIOs
> >> but instead use a prefix that explains what those GPIOs are used for.
> >> So something like "power-gpios" or "power-gpio" (if there is only one
> >> GPIO) will be more suitable. Please take a look to
> >> Documentation/gpio/board.txt for more details.
> >
> > Ok. Actually, reading docs below, "power-gpio" will not work, and it
> > needs to be "power-gpios", right?
> >
> 
> No, the documentation is not updated. People used <function>-gpio so
> at the end it was added as another supported suffix, see commit:
> dd34c37aa3e8 ("gpio: of: Allow -gpio suffix for property names").
> 
> But I guess it doesn't matter if -gpio or -gpios is used.

I suggest to use enable-gpio(s), since the pin is called "enable" in
the datasheet.

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150403/d41f5a1b/attachment.sig>

[PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
 
---

Switched to gpiod_, as requested by Javier.

Please apply,
  							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..da9934a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - power-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..ba7f43d 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	if (flash->platform_data->power) {
+		ret = flash->platform_data->power(&flash->subdev, on);
+	} else {
+		gpiod_set_value(flash->platform_data->power_gpio, on);
+		if (on)
+			/* Some delay is apparently required. */
+			udelay(20);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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
+		gpiod_set_value(flash->platform_data->power_gpio, 0);
 
 	return ret;
 }
@@ -407,21 +423,75 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = devm_gpiod_get(&client->dev, "power");
+	if (!pd->power_gpio) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +512,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +535,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
diff --git a/include/media/adp1653.h b/include/media/adp1653.h
index 1d9b48a..34b505e 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 */
+
+	struct gpio_desc *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

[PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
 
---

Switched to gpiod_, as requested by Javier.

Please apply,
  							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..da9934a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - power-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..ba7f43d 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	if (flash->platform_data->power) {
+		ret = flash->platform_data->power(&flash->subdev, on);
+	} else {
+		gpiod_set_value(flash->platform_data->power_gpio, on);
+		if (on)
+			/* Some delay is apparently required. */
+			udelay(20);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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
+		gpiod_set_value(flash->platform_data->power_gpio, 0);
 
 	return ret;
 }
@@ -407,21 +423,75 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = devm_gpiod_get(&client->dev, "power");
+	if (!pd->power_gpio) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +512,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +535,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
diff --git a/include/media/adp1653.h b/include/media/adp1653.h
index 1d9b48a..34b505e 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 */
+
+	struct gpio_desc *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

Re: [PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

Thanks for the update.

On Fri, Apr 03, 2015 at 10:33:53AM +0200, Pavel Machek wrote:
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>  
> ---
> 
> Switched to gpiod_, as requested by Javier.
> 
> Please apply,

Let's properly review this first.

>   							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt

Please split this as Javier suggested. I'd think both could go through
the media-tree unless someone objects.

> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - power-gpios: Reference to the GPIO that controls the power for the chip.

You're using power-gpios in documentation only.

The spec refers to this by "EN". How about "en-gpios" instead? This
definitely isn't about power, but about resetting the chip. It gets the
power through another pin.

> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +	adp1653: led-controller@30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..ba7f43d 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);
> +	} else {
> +		gpiod_set_value(flash->platform_data->power_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -316,8 +327,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
> +		gpiod_set_value(flash->platform_data->power_gpio, 0);
>  
>  	return ret;
>  }
> @@ -407,21 +423,75 @@ 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;
> +	struct device_node *child;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +		return -EINVAL;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_torch_intensity = val/1000;

I think you need to do of_node_put(child) here and after you're done with
indicator below.

> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_indicator_intensity = val;
> +
> +	if (!of_find_property(node, "gpios", NULL)) {
> +		dev_err(&client->dev, "No gpio node\n");
> +		return -EINVAL;
> +	}
> +
> +	pd->power_gpio = devm_gpiod_get(&client->dev, "power");
> +	if (!pd->power_gpio) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;
> +	if (client->dev.of_node) {
> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -442,6 +512,7 @@ static int adp1653_probe(struct i2c_client *client,
>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +535,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
> diff --git a/include/media/adp1653.h b/include/media/adp1653.h
> index 1d9b48a..34b505e 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 */
> +
> +	struct gpio_desc *power_gpio;	/* for device-tree based boot */
>  };
>  
>  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> 

-- 
Kind regards,

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

[PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

Thanks for the update.

On Fri, Apr 03, 2015 at 10:33:53AM +0200, Pavel Machek wrote:
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>  
> ---
> 
> Switched to gpiod_, as requested by Javier.
> 
> Please apply,

Let's properly review this first.

>   							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt

Please split this as Javier suggested. I'd think both could go through
the media-tree unless someone objects.

> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - power-gpios: Reference to the GPIO that controls the power for the chip.

You're using power-gpios in documentation only.

The spec refers to this by "EN". How about "en-gpios" instead? This
definitely isn't about power, but about resetting the chip. It gets the
power through another pin.

> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +	adp1653: led-controller at 30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..ba7f43d 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);
> +	} else {
> +		gpiod_set_value(flash->platform_data->power_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -316,8 +327,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
> +		gpiod_set_value(flash->platform_data->power_gpio, 0);
>  
>  	return ret;
>  }
> @@ -407,21 +423,75 @@ 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;
> +	struct device_node *child;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
> +		return -EINVAL;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_torch_intensity = val/1000;

I think you need to do of_node_put(child) here and after you're done with
indicator below.

> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		return -EINVAL;
> +	pd->max_indicator_intensity = val;
> +
> +	if (!of_find_property(node, "gpios", NULL)) {
> +		dev_err(&client->dev, "No gpio node\n");
> +		return -EINVAL;
> +	}
> +
> +	pd->power_gpio = devm_gpiod_get(&client->dev, "power");
> +	if (!pd->power_gpio) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;
> +	if (client->dev.of_node) {
> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -442,6 +512,7 @@ static int adp1653_probe(struct i2c_client *client,
>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +535,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
> diff --git a/include/media/adp1653.h b/include/media/adp1653.h
> index 1d9b48a..34b505e 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 */
> +
> +	struct gpio_desc *power_gpio;	/* for device-tree based boot */
>  };
>  
>  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> 

-- 
Kind regards,

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

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Documentation for adp1653 binding.

---

> Please split this as Javier suggested. I'd think both could go through
> the media-tree unless someone objects.

Please apply.

> > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> 
> You're using power-gpios in documentation only.

Which is ok, because generic code adds "-gpios" itself.

> The spec refers to this by "EN". How about "en-gpios" instead? This
> definitely isn't about power, but about resetting the chip. It gets the
> power through another pin.

It controls power of the chip. Noone gets _power_ through gpios,
hopefully. Yes, I can rename it. "en-gpios" is too ugly to
live. Sebastian suggested "enable". Hope that's okay with you.

									Pavel

new file mode 100644
index 0000000..da9934a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - power-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Documentation for adp1653 binding.

---

> Please split this as Javier suggested. I'd think both could go through
> the media-tree unless someone objects.

Please apply.

> > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> 
> You're using power-gpios in documentation only.

Which is ok, because generic code adds "-gpios" itself.

> The spec refers to this by "EN". How about "en-gpios" instead? This
> definitely isn't about power, but about resetting the chip. It gets the
> power through another pin.

It controls power of the chip. Noone gets _power_ through gpios,
hopefully. Yes, I can rename it. "en-gpios" is too ugly to
live. Sebastian suggested "enable". Hope that's okay with you.

									Pavel

new file mode 100644
index 0000000..da9934a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - power-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> 
> Documentation for adp1653 binding.
> 
> ---
> 
> > Please split this as Javier suggested. I'd think both could go through
> > the media-tree unless someone objects.
> 
> Please apply.
> 
> > > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> > 
> > You're using power-gpios in documentation only.
> 
> Which is ok, because generic code adds "-gpios" itself.
> 
> > The spec refers to this by "EN". How about "en-gpios" instead? This
> > definitely isn't about power, but about resetting the chip. It gets the
> > power through another pin.
> 
> It controls power of the chip. Noone gets _power_ through gpios,
> hopefully. Yes, I can rename it. "en-gpios" is too ugly to
> live. Sebastian suggested "enable". Hope that's okay with you.

"enable-gpios" sounds fine for me.

> 
> 									Pavel
> 
> new file mode 100644
> index 0000000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +	adp1653: led-controller@30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> 

-- 
Regards,

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

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> 
> Documentation for adp1653 binding.
> 
> ---
> 
> > Please split this as Javier suggested. I'd think both could go through
> > the media-tree unless someone objects.
> 
> Please apply.
> 
> > > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> > 
> > You're using power-gpios in documentation only.
> 
> Which is ok, because generic code adds "-gpios" itself.
> 
> > The spec refers to this by "EN". How about "en-gpios" instead? This
> > definitely isn't about power, but about resetting the chip. It gets the
> > power through another pin.
> 
> It controls power of the chip. Noone gets _power_ through gpios,
> hopefully. Yes, I can rename it. "en-gpios" is too ugly to
> live. Sebastian suggested "enable". Hope that's okay with you.

"enable-gpios" sounds fine for me.

> 
> 									Pavel
> 
> new file mode 100644
> index 0000000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"
> +
> +  - reg: I2C slave address
> +
> +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +	adp1653: led-controller at 30 {
> +		compatible = "adi,adp1653";
> +		reg = <0x30>;
> +		power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> +		flash {
> +			flash-timeout-us = <500000>;
> +			flash-max-microamp = <320000>;
> +			max-microamp = <50000>;
> +		};
> +		indicator {
> +			max-microamp = <17500>;
> +		};
> +	};
> 

-- 
Regards,

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

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Documentation for adp1653 binding.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Please apply.

Sorry, wrong version of patch was sent last time.
									Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..4607ce3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - enable-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		enable-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Documentation for adp1653 binding.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Please apply.

Sorry, wrong version of patch was sent last time.
									Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..4607ce3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - enable-gpios: Reference to the GPIO that controls the power for the chip.
+
+There are two LED outputs available - flash and indicator. One LED is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		enable-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Sat, Apr 04, 2015 at 09:43:37AM +0200, Pavel Machek wrote:
> 
> Documentation for adp1653 binding.

s/binding/bindings/

> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Please apply.
> 
> Sorry, wrong version of patch was sent last time.
> 									Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..4607ce3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"

s/be //

> +
> +  - reg: I2C slave address
> +
> +  - enable-gpios: Reference to the GPIO that controls the power for the chip.

How about:

enable-gpios: Specifier of the GPIO connected to EN pin

I can make the changes if you're ok with that, otherwise please send v7. Then
I'll apply that to my tree.

-- 
Kind regards,

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

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Sat, Apr 04, 2015 at 09:43:37AM +0200, Pavel Machek wrote:
> 
> Documentation for adp1653 binding.

s/binding/bindings/

> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Please apply.
> 
> Sorry, wrong version of patch was sent last time.
> 									Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..4607ce3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"

s/be //

> +
> +  - reg: I2C slave address
> +
> +  - enable-gpios: Reference to the GPIO that controls the power for the chip.

How about:

enable-gpios: Specifier of the GPIO connected to EN pin

I can make the changes if you're ok with that, otherwise please send v7. Then
I'll apply that to my tree.

-- 
Kind regards,

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

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

On Sat, Apr 04, 2015 at 09:43:37AM +0200, Pavel Machek wrote:
> 
> Documentation for adp1653 binding.

s/binding/bindings/

> 
> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> ---
> 
> Please apply.
> 
> Sorry, wrong version of patch was sent last time.
> 									Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..4607ce3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> +  - compatible: Must contain be "adi,adp1653"

s/be //

> +
> +  - reg: I2C slave address
> +
> +  - enable-gpios: Reference to the GPIO that controls the power for the chip.

How about:

enable-gpios: Specifier of the GPIO connected to EN pin

I can make the changes if you're ok with that, otherwise please send v7. Then
I'll apply that to my tree.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Hi!

> enable-gpios: Specifier of the GPIO connected to EN pin
> 
> I can make the changes if you're ok with that, otherwise please send v7. Then
> I'll apply that to my tree.

I'm ok with that.

Thanks,
						Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Hi!

> enable-gpios: Specifier of the GPIO connected to EN pin
> 
> I can make the changes if you're ok with that, otherwise please send v7. Then
> I'll apply that to my tree.

I'm ok with that.

Thanks,
						Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Pavel Machek]

Hi!

> enable-gpios: Specifier of the GPIO connected to EN pin
> 
> I can make the changes if you're ok with that, otherwise please send v7. Then
> I'll apply that to my tree.

I'm ok with that.

Thanks,
						Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Sat, Apr 04, 2015 at 07:11:16PM +0200, Pavel Machek wrote:
> Hi!
> 
> > enable-gpios: Specifier of the GPIO connected to EN pin
> > 
> > I can make the changes if you're ok with that, otherwise please send v7. Then
> > I'll apply that to my tree.
> 
> I'm ok with that.

Thanks. The patch is applied here:

	git://linuxtv.org/sailus/media_tree.git, branch adp1653

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

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Sat, Apr 04, 2015 at 07:11:16PM +0200, Pavel Machek wrote:
> Hi!
> 
> > enable-gpios: Specifier of the GPIO connected to EN pin
> > 
> > I can make the changes if you're ok with that, otherwise please send v7. Then
> > I'll apply that to my tree.
> 
> I'm ok with that.

Thanks. The patch is applied here:

	git://linuxtv.org/sailus/media_tree.git, branch adp1653

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

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Sat, Apr 04, 2015 at 07:11:16PM +0200, Pavel Machek wrote:
> Hi!
> 
> > enable-gpios: Specifier of the GPIO connected to EN pin
> > 
> > I can make the changes if you're ok with that, otherwise please send v7. Then
> > I'll apply that to my tree.
> 
> I'm ok with that.

Thanks. The patch is applied here:

	git://linuxtv.org/sailus/media_tree.git, branch adp1653

-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

Add device tree support for adp1653 flash LED driver.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Second part of a patch after documentation was merged.

Please apply,
							Pavel

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..d703636 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	if (flash->platform_data->power) {
+		ret = flash->platform_data->power(&flash->subdev, on);
+	} else {
+		gpiod_set_value(flash->platform_data->enable_gpio, on);
+		if (on)
+			/* Some delay is apparently required. */
+			udelay(20);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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
+		gpiod_set_value(flash->platform_data->enable_gpio, 0);
 
 	return ret;
 }
@@ -407,21 +423,78 @@ 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;
+	struct device_node *child = NULL;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+
+	if (of_property_read_u32(child, "flash-timeout-us", &val))
+		goto err;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		goto err;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		goto err;
+	pd->max_torch_intensity = val/1000;
+	of_node_put(child);
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		goto err;
+	pd->max_indicator_intensity = val;
+
+	of_node_put(child);
+
+	pd->enable_gpio = devm_gpiod_get(&client->dev, "enable");
+	if (!pd->enable_gpio) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	return 0;
+err:
+	dev_err(&client->dev, "Required property not found\n");
+	of_node_put(child);
+	return -EINVAL;
+}
+
+
 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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
diff --git a/include/media/adp1653.h b/include/media/adp1653.h
index 1d9b48a..34b505e 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 */
+
+	struct gpio_desc *enable_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

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

Add device tree support for adp1653 flash LED driver.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Second part of a patch after documentation was merged.

Please apply,
							Pavel

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..d703636 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	if (flash->platform_data->power) {
+		ret = flash->platform_data->power(&flash->subdev, on);
+	} else {
+		gpiod_set_value(flash->platform_data->enable_gpio, on);
+		if (on)
+			/* Some delay is apparently required. */
+			udelay(20);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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
+		gpiod_set_value(flash->platform_data->enable_gpio, 0);
 
 	return ret;
 }
@@ -407,21 +423,78 @@ 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;
+	struct device_node *child = NULL;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+
+	if (of_property_read_u32(child, "flash-timeout-us", &val))
+		goto err;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		goto err;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		goto err;
+	pd->max_torch_intensity = val/1000;
+	of_node_put(child);
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		goto err;
+	pd->max_indicator_intensity = val;
+
+	of_node_put(child);
+
+	pd->enable_gpio = devm_gpiod_get(&client->dev, "enable");
+	if (!pd->enable_gpio) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	return 0;
+err:
+	dev_err(&client->dev, "Required property not found\n");
+	of_node_put(child);
+	return -EINVAL;
+}
+
+
 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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (client->dev.of_node) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client,
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
diff --git a/include/media/adp1653.h b/include/media/adp1653.h
index 1d9b48a..34b505e 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 */
+
+	struct gpio_desc *enable_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

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

Hi,

On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> [...]
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> [...]

This should probably be

#include <linux/of.h>
#include <linux/gpio/consumer.h>

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

Hi,

On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> [...]
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> [...]

This should probably be

#include <linux/of.h>
#include <linux/gpio/consumer.h>

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150409/db10f0dd/attachment.sig>

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

Hi,

On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> [...]
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
> [...]

This should probably be

#include <linux/of.h>
#include <linux/gpio/consumer.h>

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > [...]
> > +#include <linux/of_gpio.h>
> > +#include <linux/gpio.h>
> > [...]
> 
> This should probably be
> 
> #include <linux/of.h>
> #include <linux/gpio/consumer.h>

And I thought people would only bikesched paint on the
Documentation. Sakari, feel free to change that, but a) I don't see
why Sebastian's version is better and b) am pretty sure there is about
infinite number of possibilities there.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > [...]
> > +#include <linux/of_gpio.h>
> > +#include <linux/gpio.h>
> > [...]
> 
> This should probably be
> 
> #include <linux/of.h>
> #include <linux/gpio/consumer.h>

And I thought people would only bikesched paint on the
Documentation. Sakari, feel free to change that, but a) I don't see
why Sebastian's version is better and b) am pretty sure there is about
infinite number of possibilities there.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > [...]
> > > +#include <linux/of_gpio.h>
> > > +#include <linux/gpio.h>
> > > [...]
> > 
> > This should probably be
> > 
> > #include <linux/of.h>
> > #include <linux/gpio/consumer.h>
> 
> And I thought people would only bikesched paint on the
> Documentation. Sakari, feel free to change that, but

> a) I don't see why Sebastian's version is better

You neither use <linux/of_gpio.h> nor <linux/gpio.h>.

Well "include/linux/gpio.h" describes the old gpio API. The new
gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
you use it, so the include should be included ;)

You don't use anything from "include/linux/of_gpio.h", but it
includes "include/linux/of.h", which you are using. So you should
include <linux/of.h> instead ;)

> b) am pretty sure there is about infinite number of
> possibilities there.

Yes, but most are wrong. You should include all headers,
that are used by you - nothing more and nothing less.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > [...]
> > > +#include <linux/of_gpio.h>
> > > +#include <linux/gpio.h>
> > > [...]
> > 
> > This should probably be
> > 
> > #include <linux/of.h>
> > #include <linux/gpio/consumer.h>
> 
> And I thought people would only bikesched paint on the
> Documentation. Sakari, feel free to change that, but

> a) I don't see why Sebastian's version is better

You neither use <linux/of_gpio.h> nor <linux/gpio.h>.

Well "include/linux/gpio.h" describes the old gpio API. The new
gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
you use it, so the include should be included ;)

You don't use anything from "include/linux/of_gpio.h", but it
includes "include/linux/of.h", which you are using. So you should
include <linux/of.h> instead ;)

> b) am pretty sure there is about infinite number of
> possibilities there.

Yes, but most are wrong. You should include all headers,
that are used by you - nothing more and nothing less.

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150409/8604f8c9/attachment.sig>

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > [...]
> > > +#include <linux/of_gpio.h>
> > > +#include <linux/gpio.h>
> > > [...]
> > 
> > This should probably be
> > 
> > #include <linux/of.h>
> > #include <linux/gpio/consumer.h>
> 
> And I thought people would only bikesched paint on the
> Documentation. Sakari, feel free to change that, but

> a) I don't see why Sebastian's version is better

You neither use <linux/of_gpio.h> nor <linux/gpio.h>.

Well "include/linux/gpio.h" describes the old gpio API. The new
gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
you use it, so the include should be included ;)

You don't use anything from "include/linux/of_gpio.h", but it
includes "include/linux/of.h", which you are using. So you should
include <linux/of.h> instead ;)

> b) am pretty sure there is about infinite number of
> possibilities there.

Yes, but most are wrong. You should include all headers,
that are used by you - nothing more and nothing less.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCHv7] media: i2c/adp1653: fix includes

[Pavel Machek]

Fix includes according to Sebastian.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote:
> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > > [...]
> > > > +#include <linux/of_gpio.h>
> > > > +#include <linux/gpio.h>
> > > > [...]
> > > 
> > > This should probably be
> > > 
> > > #include <linux/of.h>
> > > #include <linux/gpio/consumer.h>
> > 
> > And I thought people would only bikesched paint on the
> > Documentation. Sakari, feel free to change that, but
> 
> > a) I don't see why Sebastian's version is better
> 
> You neither use <linux/of_gpio.h> nor <linux/gpio.h>.
> 
> Well "include/linux/gpio.h" describes the old gpio API. The new
> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
> you use it, so the include should be included ;)
> 
> You don't use anything from "include/linux/of_gpio.h", but it
> includes "include/linux/of.h", which you are using. So you should
> include <linux/of.h> instead ;)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index d703636..7107ac2 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -35,8 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
-#include <linux/of_gpio.h>
-#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv7] media: i2c/adp1653: fix includes

[Pavel Machek]

Fix includes according to Sebastian.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote:
> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > > [...]
> > > > +#include <linux/of_gpio.h>
> > > > +#include <linux/gpio.h>
> > > > [...]
> > > 
> > > This should probably be
> > > 
> > > #include <linux/of.h>
> > > #include <linux/gpio/consumer.h>
> > 
> > And I thought people would only bikesched paint on the
> > Documentation. Sakari, feel free to change that, but
> 
> > a) I don't see why Sebastian's version is better
> 
> You neither use <linux/of_gpio.h> nor <linux/gpio.h>.
> 
> Well "include/linux/gpio.h" describes the old gpio API. The new
> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
> you use it, so the include should be included ;)
> 
> You don't use anything from "include/linux/of_gpio.h", but it
> includes "include/linux/of.h", which you are using. So you should
> include <linux/of.h> instead ;)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index d703636..7107ac2 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -35,8 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
-#include <linux/of_gpio.h>
-#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv7] media: i2c/adp1653: fix includes

[Pavel Machek]

Fix includes according to Sebastian.

Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

---

On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote:
> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> > > > [...]
> > > > +#include <linux/of_gpio.h>
> > > > +#include <linux/gpio.h>
> > > > [...]
> > > 
> > > This should probably be
> > > 
> > > #include <linux/of.h>
> > > #include <linux/gpio/consumer.h>
> > 
> > And I thought people would only bikesched paint on the
> > Documentation. Sakari, feel free to change that, but
> 
> > a) I don't see why Sebastian's version is better
> 
> You neither use <linux/of_gpio.h> nor <linux/gpio.h>.
> 
> Well "include/linux/gpio.h" describes the old gpio API. The new
> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
> you use it, so the include should be included ;)
> 
> You don't use anything from "include/linux/of_gpio.h", but it
> includes "include/linux/of.h", which you are using. So you should
> include <linux/of.h> instead ;)

diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index d703636..7107ac2 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -35,8 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
-#include <linux/of_gpio.h>
-#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] media: i2c/adp1653: fix includes

[Javier Martinez Canillas]

Hello Pavel,

On Thu, Apr 9, 2015 at 2:31 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Fix includes according to Sebastian.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote:
>> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
>> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
>> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
>> > > > [...]
>> > > > +#include <linux/of_gpio.h>
>> > > > +#include <linux/gpio.h>
>> > > > [...]
>> > >
>> > > This should probably be
>> > >
>> > > #include <linux/of.h>
>> > > #include <linux/gpio/consumer.h>
>> >
>> > And I thought people would only bikesched paint on the
>> > Documentation. Sakari, feel free to change that, but
>>
>> > a) I don't see why Sebastian's version is better
>>
>> You neither use <linux/of_gpio.h> nor <linux/gpio.h>.
>>
>> Well "include/linux/gpio.h" describes the old gpio API. The new
>> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
>> you use it, so the include should be included ;)
>>
>> You don't use anything from "include/linux/of_gpio.h", but it
>> includes "include/linux/of.h", which you are using. So you should
>> include <linux/of.h> instead ;)
>
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index d703636..7107ac2 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -35,8 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> -#include <linux/of_gpio.h>
> -#include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>

Please re-spin your previous patch and submit it properly.

Best regards,
Javier

[PATCHv7] media: i2c/adp1653: fix includes

[Javier Martinez Canillas]

Hello Pavel,

On Thu, Apr 9, 2015 at 2:31 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Fix includes according to Sebastian.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote:
>> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote:
>> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote:
>> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
>> > > > [...]
>> > > > +#include <linux/of_gpio.h>
>> > > > +#include <linux/gpio.h>
>> > > > [...]
>> > >
>> > > This should probably be
>> > >
>> > > #include <linux/of.h>
>> > > #include <linux/gpio/consumer.h>
>> >
>> > And I thought people would only bikesched paint on the
>> > Documentation. Sakari, feel free to change that, but
>>
>> > a) I don't see why Sebastian's version is better
>>
>> You neither use <linux/of_gpio.h> nor <linux/gpio.h>.
>>
>> Well "include/linux/gpio.h" describes the old gpio API. The new
>> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and
>> you use it, so the include should be included ;)
>>
>> You don't use anything from "include/linux/of_gpio.h", but it
>> includes "include/linux/of.h", which you are using. So you should
>> include <linux/of.h> instead ;)
>
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index d703636..7107ac2 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -35,8 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> -#include <linux/of_gpio.h>
> -#include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>

Please re-spin your previous patch and submit it properly.

Best regards,
Javier

Re: [PATCHv7] media: i2c/adp1653: fix includes

[Pali Rohár]

On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:
> Hello Pavel,
> 
> >
> > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> > index d703636..7107ac2 100644
> > --- a/drivers/media/i2c/adp1653.c
> > +++ b/drivers/media/i2c/adp1653.c
> > @@ -35,8 +35,8 @@
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > -#include <linux/of_gpio.h>
> > -#include <linux/gpio.h>
> > +#include <linux/of.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <media/adp1653.h>
> >  #include <media/v4l2-device.h>
> >
> 
> Please re-spin your previous patch and submit it properly.
> 
> Best regards,
> Javier

Hi all! What about stopping this meaningless discussion about resending
full patch series when everybody know how to fix is quickly in editor
(e.g with sed under 5s) and not wasting another 10 minutes to generate
new unified diff sent via SMTP protocol?

-- 
Pali Rohár
pali.rohar@gmail.com

[PATCHv7] media: i2c/adp1653: fix includes

[Pali Rohár]

On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:
> Hello Pavel,
> 
> >
> > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> > index d703636..7107ac2 100644
> > --- a/drivers/media/i2c/adp1653.c
> > +++ b/drivers/media/i2c/adp1653.c
> > @@ -35,8 +35,8 @@
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > -#include <linux/of_gpio.h>
> > -#include <linux/gpio.h>
> > +#include <linux/of.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <media/adp1653.h>
> >  #include <media/v4l2-device.h>
> >
> 
> Please re-spin your previous patch and submit it properly.
> 
> Best regards,
> Javier

Hi all! What about stopping this meaningless discussion about resending
full patch series when everybody know how to fix is quickly in editor
(e.g with sed under 5s) and not wasting another 10 minutes to generate
new unified diff sent via SMTP protocol?

-- 
Pali Roh?r
pali.rohar at gmail.com

Re: [PATCHv7] media: i2c/adp1653: fix includes

[Pali Rohár]

On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:
> Hello Pavel,
> 
> >
> > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> > index d703636..7107ac2 100644
> > --- a/drivers/media/i2c/adp1653.c
> > +++ b/drivers/media/i2c/adp1653.c
> > @@ -35,8 +35,8 @@
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > -#include <linux/of_gpio.h>
> > -#include <linux/gpio.h>
> > +#include <linux/of.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <media/adp1653.h>
> >  #include <media/v4l2-device.h>
> >
> 
> Please re-spin your previous patch and submit it properly.
> 
> Best regards,
> Javier

Hi all! What about stopping this meaningless discussion about resending
full patch series when everybody know how to fix is quickly in editor
(e.g with sed under 5s) and not wasting another 10 minutes to generate
new unified diff sent via SMTP protocol?

-- 
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv7] media: i2c/adp1653: fix includes

[Javier Martinez Canillas]

Hello Pali,

On Thu, Apr 9, 2015 at 2:59 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:
>>
>> Please re-spin your previous patch and submit it properly.
>>
>> Best regards,
>> Javier
>
> Hi all! What about stopping this meaningless discussion about resending
> full patch series when everybody know how to fix is quickly in editor
> (e.g with sed under 5s) and not wasting another 10 minutes to generate
> new unified diff sent via SMTP protocol?
>

No, there is a reason why we have written rules on how patches should
be submitted. Everyone in the kernel community is expected to optimize
their workflow according to these rules to make life easier for people
reviewing and merging the patches.

As you said now someone has to fix this using an editor and that is an
error prone process. Besides, why it would take 10 minutes to generate
a proper patch-set? git is very good on this regard (i.e: git commit
---fixup && git rebase -i && git format-patch && git send-email).

I won't argue anymore but I find very sad that people who have been in
the kernel community for years don't follow the basic rules we have
documented it. So I wonder why we have the documentation in the first
place and how can we expect newcomers to follow if even experienced
kernel hackers don't.

> --
> Pali Rohár
> pali.rohar@gmail.com

Best regards,
Javier

[PATCHv7] media: i2c/adp1653: fix includes

[Javier Martinez Canillas]

Hello Pali,

On Thu, Apr 9, 2015 at 2:59 PM, Pali Roh?r <pali.rohar@gmail.com> wrote:
> On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:
>>
>> Please re-spin your previous patch and submit it properly.
>>
>> Best regards,
>> Javier
>
> Hi all! What about stopping this meaningless discussion about resending
> full patch series when everybody know how to fix is quickly in editor
> (e.g with sed under 5s) and not wasting another 10 minutes to generate
> new unified diff sent via SMTP protocol?
>

No, there is a reason why we have written rules on how patches should
be submitted. Everyone in the kernel community is expected to optimize
their workflow according to these rules to make life easier for people
reviewing and merging the patches.

As you said now someone has to fix this using an editor and that is an
error prone process. Besides, why it would take 10 minutes to generate
a proper patch-set? git is very good on this regard (i.e: git commit
---fixup && git rebase -i && git format-patch && git send-email).

I won't argue anymore but I find very sad that people who have been in
the kernel community for years don't follow the basic rules we have
documented it. So I wonder why we have the documentation in the first
place and how can we expect newcomers to follow if even experienced
kernel hackers don't.

> --
> Pali Roh?r
> pali.rohar at gmail.com

Best regards,
Javier

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

(Cc linux-media. Media related patches should be sent there.)

On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> 
> Add device tree support for adp1653 flash LED driver.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Second part of a patch after documentation was merged.
> 
> Please apply,
> 							Pavel
> 
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..d703636 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>

As Sebastian suggested, linux/of.h and linux/gpio/consumer.h should be used.

>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);
> +	} else {
> +		gpiod_set_value(flash->platform_data->enable_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;

Please check ret after assigning it. The assignment in declaration is
unnecessary.

>  
> @@ -316,8 +327,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
> +		gpiod_set_value(flash->platform_data->enable_gpio, 0);
>  
>  	return ret;
>  }
> @@ -407,21 +423,78 @@ 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;
> +	struct device_node *child = NULL;

The NULL assignment can be removed.

> +
> +	if (!node)
> +		return -EINVAL;

node is always non-NULL here; no need to check.

> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +
> +	if (of_property_read_u32(child, "flash-timeout-us", &val))

You could read the values directly to the appropriate struct
adp1653_platform_data field.

> +		goto err;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		goto err;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		goto err;
> +	pd->max_torch_intensity = val/1000;
> +	of_node_put(child);
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))

Let's wait a bit the resolution of the property name. I'm in principle fine
with both. I can do the change once it's been decided, hopefully very soon.

> +		goto err;
> +	pd->max_indicator_intensity = val;
> +
> +	of_node_put(child);
> +
> +	pd->enable_gpio = devm_gpiod_get(&client->dev, "enable");
> +	if (!pd->enable_gpio) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +err:
> +	dev_err(&client->dev, "Required property not found\n");
> +	of_node_put(child);
> +	return -EINVAL;
> +}
> +
> +
>  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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;

I think it'd be cleaner to make the assignment only if not using of, i.e.
add else branch to the if below.

> +	if (client->dev.of_node) {
> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client,
>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
> diff --git a/include/media/adp1653.h b/include/media/adp1653.h
> index 1d9b48a..34b505e 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 */
> +
> +	struct gpio_desc *enable_gpio;	/* for device-tree based boot */
>  };
>  
>  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> 

Let me know if you're going to send v8 or if I can make the changes. I think
we're pretty much done then.

-- 
Kind regards,

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

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Sakari Ailus]

Hi Pavel,

(Cc linux-media. Media related patches should be sent there.)

On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote:
> 
> Add device tree support for adp1653 flash LED driver.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Second part of a patch after documentation was merged.
> 
> Please apply,
> 							Pavel
> 
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..d703636 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,6 +35,8 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>

As Sebastian suggested, linux/of.h and linux/gpio/consumer.h should be used.

>  #include <media/adp1653.h>
>  #include <media/v4l2-device.h>
>  
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
>  static int
>  __adp1653_set_power(struct adp1653_flash *flash, int on)
>  {
> -	int ret;
> +	int ret = 0;
> +
> +	if (flash->platform_data->power) {
> +		ret = flash->platform_data->power(&flash->subdev, on);
> +	} else {
> +		gpiod_set_value(flash->platform_data->enable_gpio, on);
> +		if (on)
> +			/* Some delay is apparently required. */
> +			udelay(20);
> +	}
>  
> -	ret = flash->platform_data->power(&flash->subdev, on);
>  	if (ret < 0)
>  		return ret;

Please check ret after assigning it. The assignment in declaration is
unnecessary.

>  
> @@ -316,8 +327,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
> +		gpiod_set_value(flash->platform_data->enable_gpio, 0);
>  
>  	return ret;
>  }
> @@ -407,21 +423,78 @@ 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;
> +	struct device_node *child = NULL;

The NULL assignment can be removed.

> +
> +	if (!node)
> +		return -EINVAL;

node is always non-NULL here; no need to check.

> +
> +	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +	flash->platform_data = pd;
> +
> +	child = of_get_child_by_name(node, "flash");
> +	if (!child)
> +		return -EINVAL;
> +
> +	if (of_property_read_u32(child, "flash-timeout-us", &val))

You could read the values directly to the appropriate struct
adp1653_platform_data field.

> +		goto err;
> +
> +	pd->max_flash_timeout = val;
> +	if (of_property_read_u32(child, "flash-max-microamp", &val))
> +		goto err;
> +	pd->max_flash_intensity = val/1000;
> +
> +	if (of_property_read_u32(child, "max-microamp", &val))
> +		goto err;
> +	pd->max_torch_intensity = val/1000;
> +	of_node_put(child);
> +
> +	child = of_get_child_by_name(node, "indicator");
> +	if (!child)
> +		return -EINVAL;
> +	if (of_property_read_u32(child, "max-microamp", &val))

Let's wait a bit the resolution of the property name. I'm in principle fine
with both. I can do the change once it's been decided, hopefully very soon.

> +		goto err;
> +	pd->max_indicator_intensity = val;
> +
> +	of_node_put(child);
> +
> +	pd->enable_gpio = devm_gpiod_get(&client->dev, "enable");
> +	if (!pd->enable_gpio) {
> +		dev_err(&client->dev, "Error getting GPIO\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +err:
> +	dev_err(&client->dev, "Required property not found\n");
> +	of_node_put(child);
> +	return -EINVAL;
> +}
> +
> +
>  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;
> -
>  	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
>  	if (flash == NULL)
>  		return -ENOMEM;
>  
>  	flash->platform_data = client->dev.platform_data;

I think it'd be cleaner to make the assignment only if not using of, i.e.
add else branch to the if below.

> +	if (client->dev.of_node) {
> +		ret = adp1653_of_init(client, flash, client->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mutex_init(&flash->power_lock);
>  
> @@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client,
>  	return 0;
>  
>  free_and_quit:
> +	dev_err(&client->dev, "adp1653: failed to register device\n");
>  	v4l2_ctrl_handler_free(&flash->ctrls);
>  	return ret;
>  }
> @@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
>  
> -static struct dev_pm_ops adp1653_pm_ops = {
> +static const struct dev_pm_ops adp1653_pm_ops = {
>  	.suspend	= adp1653_suspend,
>  	.resume		= adp1653_resume,
>  };
> diff --git a/include/media/adp1653.h b/include/media/adp1653.h
> index 1d9b48a..34b505e 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 */
> +
> +	struct gpio_desc *enable_gpio;	/* for device-tree based boot */
>  };
>  
>  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> 

Let me know if you're going to send v8 or if I can make the changes. I think
we're pretty much done then.

-- 
Kind regards,

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

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

Hi!

> >  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> > 
> 
> Let me know if you're going to send v8 or if I can make the changes. I think
> we're pretty much done then.

You are free to make the changes.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

Hi!

> >  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> > 
> 
> Let me know if you're going to send v8 or if I can make the changes. I think
> we're pretty much done then.

You are free to make the changes.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv7] media: i2c/adp1653: Devicetree support for adp1653

[Pavel Machek]

Hi!

> >  #define to_adp1653_flash(sd)	container_of(sd, struct adp1653_flash, subdev)
> > 
> 
> Let me know if you're going to send v8 or if I can make the changes. I think
> we're pretty much done then.

You are free to make the changes.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> > > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> > 
> > You're using power-gpios in documentation only.
> 
> Which is ok, because generic code adds "-gpios" itself.

Do you think you need this part:

+       if (!of_find_property(node, "gpios", NULL)) {                           
+               dev_err(&client->dev, "No gpio node\n");                        
+               return -EINVAL;                                                 
+       }                                                                       


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

[PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> > > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> > 
> > You're using power-gpios in documentation only.
> 
> Which is ok, because generic code adds "-gpios" itself.

Do you think you need this part:

+       if (!of_find_property(node, "gpios", NULL)) {                           
+               dev_err(&client->dev, "No gpio node\n");                        
+               return -EINVAL;                                                 
+       }                                                                       


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

Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653

[Sakari Ailus]

On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> > > +  - power-gpios: Reference to the GPIO that controls the power for the chip.
> > 
> > You're using power-gpios in documentation only.
> 
> Which is ok, because generic code adds "-gpios" itself.

Do you think you need this part:

+       if (!of_find_property(node, "gpios", NULL)) {                           
+               dev_err(&client->dev, "No gpio node\n");                        
+               return -EINVAL;                                                 
+       }                                                                       


-- 
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > +	pd->max_flash_intensity = val/1000;
> > +
> > +	if (of_property_read_u32(child, "max-microamp", &val))
> > +		return -EINVAL;
> > +	pd->max_torch_intensity = val/1000;
> 
> I think you need to do of_node_put(child) here and after you're done with
> indicator below.

...and in most of the error paths. Ok. Will submit the updated patch
when the documentation one is accepted.

Best regards,

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > +	pd->max_flash_intensity = val/1000;
> > +
> > +	if (of_property_read_u32(child, "max-microamp", &val))
> > +		return -EINVAL;
> > +	pd->max_torch_intensity = val/1000;
> 
> I think you need to do of_node_put(child) here and after you're done with
> indicator below.

...and in most of the error paths. Ok. Will submit the updated patch
when the documentation one is accepted.

Best regards,

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv5] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

Hi!

> > +	pd->max_flash_intensity = val/1000;
> > +
> > +	if (of_property_read_u32(child, "max-microamp", &val))
> > +		return -EINVAL;
> > +	pd->max_torch_intensity = val/1000;
> 
> I think you need to do of_node_put(child) here and after you're done with
> indicator below.

...and in most of the error paths. Ok. Will submit the updated patch
when the documentation one is accepted.

Best regards,

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,38 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain one of the following
+    - "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-microsec : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+        adp1653: led-controller@30 {
+                compatible = "adi,adp1653";
+		reg = <0x30>;
+                gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+                        flash-timeout-microsec = <500000>;
+                        flash-max-microamp = <320000>;
+                        max-microamp = <50000>;
+		};
+                indicator {
+                        max-microamp = <17500>;
+		};
+        };
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index cfddc3d..11d8afd 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -560,6 +567,22 @@
 
 		ti,usb-charger-detection = <&isp1704>;
 	};
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-microsec = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
 };
 
 &i2c3 {
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..62601b2 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,11 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
 #define TIMEOUT_MAX		820000
 #define TIMEOUT_STEP		54600
 #define TIMEOUT_MIN		(TIMEOUT_MAX - ADP1653_REG_CONFIG_TMR_SET_MAX \
@@ -306,9 +318,18 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
 
-	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 +337,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 +433,69 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child) return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val)) return -EINVAL;
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val)) return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+	if (of_property_read_u32(child, "max-microamp", &val)) return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child) return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val)) return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	gpio = of_get_gpio_flags(node, 0, &flags);
+	if (gpio < 0) {
+		dev_err(&client->dev, "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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
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

[PATCHv2] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Changed -microsec to -us, as requested by devicetree people.

Fixed checkpatch issues.

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 2d88816..2c6c7c5 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -14,6 +14,15 @@ Optional properties for child nodes:
      "ide-disk" - LED indicates disk activity
      "timer" - LED flashes at a fixed, configurable rate
 
+- 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
+
+
 Examples:
 
 system-status {
@@ -21,3 +30,10 @@ system-status {
 	linux,default-trigger = "heartbeat";
 	...
 };
+
+camera-flash {
+	label = "Flash";
+	max-microamp = <50000>;
+	flash-max-microamp = <320000>;
+	flash-timeout-microsec = <500000>;
+}
diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..3c7065f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,38 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain one of the following
+    - "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+        adp1653: led-controller@30 {
+                compatible = "adi,adp1653";
+		reg = <0x30>;
+                gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+                        flash-timeout-us = <500000>;
+                        flash-max-microamp = <320000>;
+                        max-microamp = <50000>;
+		};
+                indicator {
+                        max-microamp = <17500>;
+		};
+        };
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index bc82a12..d04e7cc 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -553,6 +558,22 @@
 
 		ti,usb-charger-detection = <&isp1704>;
 	};
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
 };
 
 &i2c3 {
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..78341d0 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 \
@@ -306,9 +318,18 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+		}
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +337,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 +433,78 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	gpio = of_get_gpio_flags(node, 0, &flags);
+	if (gpio < 0) {
+		dev_err(&client->dev, "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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -438,10 +521,11 @@ static int adp1653_probe(struct i2c_client *client,
 		goto free_and_quit;
 
 	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
-
+
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +551,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Please apply,
							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller@30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..0341009 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,77 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
 		goto free_and_quit;
 
 	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
-
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

We are moving to device tree support on OMAP3, but that currently
breaks ADP1653 driver. This adds device tree support, plus required
documentation.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Please apply,
							Pavel

diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
new file mode 100644
index 0000000..0fc28a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
@@ -0,0 +1,37 @@
+* Analog Devices ADP1653 flash LED driver
+
+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address
+
+  - gpios: References to the GPIO that controls the power for the chip.
+
+There are two led outputs available - flash and indicator. One led is
+represented by one child node, nodes need to be named "flash" and "indicator".
+
+Required properties of the LED child node:
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+
+Required properties of the flash LED child node:
+
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	adp1653: led-controller at 30 {
+		compatible = "adi,adp1653";
+		reg = <0x30>;
+		gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
+
+		flash {
+			flash-timeout-us = <500000>;
+			flash-max-microamp = <320000>;
+			max-microamp = <50000>;
+		};
+		indicator {
+			max-microamp = <17500>;
+		};
+	};
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..0341009 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,6 +35,8 @@
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <media/adp1653.h>
 #include <media/v4l2-device.h>
 
@@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
 static int
 __adp1653_set_power(struct adp1653_flash *flash, int on)
 {
-	int ret;
+	int ret = 0;
+
+	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);
+	}
 
-	ret = flash->platform_data->power(&flash->subdev, on);
 	if (ret < 0)
 		return ret;
 
@@ -316,8 +327,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 +423,77 @@ 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;
+	struct device_node *child;
+
+	if (!node)
+		return -EINVAL;
+
+	pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+	flash->platform_data = pd;
+
+	child = of_get_child_by_name(node, "flash");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "flash-timeout-microsec", &val))
+		return -EINVAL;
+
+	pd->max_flash_timeout = val;
+	if (of_property_read_u32(child, "flash-max-microamp", &val))
+		return -EINVAL;
+	pd->max_flash_intensity = val/1000;
+
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_torch_intensity = val/1000;
+
+	child = of_get_child_by_name(node, "indicator");
+	if (!child)
+		return -EINVAL;
+	if (of_property_read_u32(child, "max-microamp", &val))
+		return -EINVAL;
+	pd->max_indicator_intensity = val;
+
+	if (!of_find_property(node, "gpios", NULL)) {
+		dev_err(&client->dev, "No gpio node\n");
+		return -EINVAL;
+	}
+
+	pd->power_gpio = of_get_gpio_flags(node, 0, &flags);
+	if (pd->power_gpio < 0) {
+		dev_err(&client->dev, "Error getting GPIO\n");
+		return -EINVAL;
+	}
+
+	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;
-
 	flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
 	if (flash == NULL)
 		return -ENOMEM;
 
 	flash->platform_data = client->dev.platform_data;
+	if (!flash->platform_data) {
+		ret = adp1653_of_init(client, flash, client->dev.of_node);
+		if (ret)
+			return ret;
+	}
 
 	mutex_init(&flash->power_lock);
 
@@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
 		goto free_and_quit;
 
 	flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
-
 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }
@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 
-static struct dev_pm_ops adp1653_pm_ops = {
+static const struct dev_pm_ops adp1653_pm_ops = {
 	.suspend	= adp1653_suspend,
 	.resume		= adp1653_resume,
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

On Sun 2015-01-04 10:43:52, Pavel Machek wrote:
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

Sakari? You are listed as adp1653 maintainer. Did you apply the patch?
Is it going to be in 3.20?

Thanks,
								Pavel
								
> ---
> 
> Please apply,
> 							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv3] media: i2c/adp1653: devicetree support for adp1653

[Pavel Machek]

On Sun 2015-01-04 10:43:52, Pavel Machek wrote:
> 
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

Sakari? You are listed as adp1653 maintainer. Did you apply the patch?
Is it going to be in 3.20?

Thanks,
								Pavel
								
> ---
> 
> Please apply,
> 							Pavel
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 0000000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html