media: i2c/adp1653: devicetree support for adp1653

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 <[email protected]>

---

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

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +10 -0

new file mode 100644
index 0000000..0fc28a9
@@ -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.
+

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +17 -0

+
+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>;
+		};
+	};

drivers/media/i2c/adp1653.c +3 -0

index 873fe19..0341009 100644
@@ -8,6 +8,7 @@
  * Contributors:
  *	Sakari Ailus <[email protected]>
  *	Tuukka Toivonen <[email protected]>
+ *	Pavel Machek <[email protected]>
  *
  * 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>
 

drivers/media/i2c/adp1653.c +67 -2

+	} 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;
 

drivers/media/i2c/adp1653.c +1 -0

 	return 0;
 
 free_and_quit:
+	dev_err(&client->dev, "adp1653: failed to register device\n");
 	v4l2_ctrl_handler_free(&flash->ctrls);
 	return ret;
 }

Hi Pawel,

My apologies for the very late reply.

Quoted 8 lines · 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 <[email protected]>

Quoted 4 lines

---

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

Quoted 3 lines

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.

Documentation/devicetree/bindings/media/i2c/adp1653.txt line 20

Quoted 4 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +4 -0

+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.

drivers/media/i2c/adp1653.c adp1653_init_device(struct adp1653_flash *flash) · line 315

Quoted 7 lines

__adp1653_set_power(struct adp1653_flash *flash, int on) {

drivers/media/i2c/adp1653.c +4 -1

-	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.

drivers/media/i2c/adp1653.c static int adp1653_resume(struct device *dev) · line 492

Quoted 6 lines

drivers/media/i2c/adp1653.c +1 -0

 	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.

drivers/media/i2c/adp1653.c static int adp1653_probe(struct i2c_client *client, · line 441

Quoted 5 lines

drivers/media/i2c/adp1653.c +0 -1

@@ -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. :-)

Quoted 4 lines

drivers/media/i2c/adp1653.c +0 -0

@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 

drivers/media/i2c/adp1653.c static const struct i2c_device_id adp1653_id_table[] = { · line 542

Quoted 7 lines

drivers/media/i2c/adp1653.c +1 -1

-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.

Hi!

Quoted 15 lines

Hi Pawel,

My apologies for the very late reply.

Quoted 10 lines · 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 <[email protected]>

---

Quoted 5 lines

Quoted 2 lines

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.

Quoted 11 lines

Quoted 4 lines

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.

Quoted 7 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt line 20

Quoted 4 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +4 -0

+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...

Quoted 10 lines

drivers/media/i2c/adp1653.c adp1653_init_device(struct adp1653_flash *flash) · line 315

Quoted 7 lines

__adp1653_set_power(struct adp1653_flash *flash, int on) {

drivers/media/i2c/adp1653.c +4 -1

-	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.

Quoted 8 lines

drivers/media/i2c/adp1653.c static int adp1653_resume(struct device *dev) · line 492

Quoted 6 lines

drivers/media/i2c/adp1653.c +1 -0

 	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.

Quoted 7 lines

drivers/media/i2c/adp1653.c static int adp1653_probe(struct i2c_client *client, · line 441

Quoted 5 lines

drivers/media/i2c/adp1653.c +0 -1

@@ -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.

Quoted 4 lines

Quoted 4 lines

drivers/media/i2c/adp1653.c +0 -0

@@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
 

Quoted 9 lines

drivers/media/i2c/adp1653.c static const struct i2c_device_id adp1653_id_table[] = { · line 542

Quoted 7 lines

drivers/media/i2c/adp1653.c +1 -1

-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?

Quoted 1 line

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.

Hi Pawel,

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

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?

Hi!

Quoted 1 line

Hi Pawel,

I'm still Pavel. v, not w.

Quoted 18 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 2 lines

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...

Hi Pavel,

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

Hi Pawel,

I'm still Pavel. v, not w.

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

Quoted 3 lines

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.

Quoted 20 lines · Sakari Ailus

Hi Pavel,

Quoted 5 lines · Pavel Machek

Hi!

Quoted 1 line

Hi Pawel,

I'm still Pavel. v, not w.

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

Quoted 3 lines

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.

Quoted 2 lines

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.

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 <[email protected]>

---

Fixed feedback by Sakari.

Please apply,

new file mode 100644
index 0000000..0fc28a9

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +28 -0

+
+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>;
+               };
+       };

drivers/media/i2c/adp1653.c +54 -3

index 873fe19..6d57b16 100644
@@ -8,6 +8,7 @@
  * Contributors:
  *     Sakari Ailus <[email protected]>
  *     Tuukka Toivonen <[email protected]>
+ *     Pavel Machek <[email protected]>
  *
  * 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;

drivers/media/i2c/adp1653.c +15 -4

+       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;
 }

Quoted 3 lines

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.

Documentation/devicetree/bindings/media/i2c/adp1653.txt

Quoted 1 line

Documentation/devicetree/bindings/media/i2c/adp1653.txt +0 -0

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.

Quoted 5 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +5 -0

+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address

Documentation/devicetree/bindings/media/i2c/adp1653.txt

Quoted 2 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +2 -0

+
+  - 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.

Documentation/devicetree/bindings/media/i2c/adp1653.txt

Quoted 6 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +6 -0

+       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.

Hi!

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt

Quoted 1 line

Documentation/devicetree/bindings/media/i2c/adp1653.txt +0 -0

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.

Quoted 5 lines

Quoted 5 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +5 -0

+Required Properties:
+
+  - compatible: Must contain be "adi,adp1653"
+
+  - reg: I2C slave address

Quoted 8 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt

Quoted 2 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +2 -0

+
+  - 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?

Quoted 7 lines

Quoted 6 lines

Documentation/devicetree/bindings/media/i2c/adp1653.txt +6 -0

+       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);

Quoted 4 lines

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
--

Quoted 9 lines

Quoted 5 lines

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.

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 <[email protected]>

---

Switched to gpiod_, as requested by Javier.

Please apply,

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +28 -0

+
+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>;
+		};
+	};

drivers/media/i2c/adp1653.c +39 -3

index 873fe19..ba7f43d 100644
@@ -8,6 +8,7 @@
  * Contributors:
  *	Sakari Ailus <[email protected]>
  *	Tuukka Toivonen <[email protected]>
+ *	Pavel Machek <[email protected]>
  *
  * 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))

drivers/media/i2c/adp1653.c +30 -5

+
+	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,
 };

include/media/adp1653.h +5 -3

index 1d9b48a..34b505e 100644
@@ -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)

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

Quoted 1 line

+ - 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.

Documentation for adp1653 binding.

---

Quoted 2 lines

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

Please apply.

Quoted 3 lines

Quoted 1 line

+ - 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.

Quoted 3 lines

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

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +37 -0

@@ -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>;
+		};
+	};

Documentation for adp1653 binding.

Signed-off-by Pavel Machek <[email protected]>

---

Please apply.

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

Documentation/devicetree/bindings/media/i2c/adp1653.txt new file +0 -0

new file mode 100644
index 0000000..4607ce3

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.

Hi!

Quoted 4 lines

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.

Hi,

drivers/media/i2c/adp1653.c line 39

Quoted 4 lines · Pavel Machek

[...]

drivers/media/i2c/adp1653.c +2 -0

+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
[...]

This should probably be

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

Quoted 8 lines · Sebastian Reichel

drivers/media/i2c/adp1653.c line 39

Quoted 4 lines · Pavel Machek

[...]

drivers/media/i2c/adp1653.c +2 -0

+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
[...]

This should probably be

Quoted 2 lines

#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

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

Quoted 9 lines · Pavel Machek

Quoted 8 lines · Sebastian Reichel

Review

Quoted 4 lines · Pavel Machek

[...]

(quoted diff) +2 -0

+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
[...]

This should probably be

Quoted 7 lines

Quoted 2 lines

#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 ;)

Fix includes according to Sebastian.

Signed-off-by Pavel Machek <[email protected]>

---

Quoted 27 lines · Sebastian Reichel

Quoted 9 lines · Pavel Machek

Quoted 8 lines · Sebastian Reichel

Review

Quoted 4 lines · Pavel Machek

[...]

(quoted diff) +2 -0

+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
[...]

This should probably be

Quoted 7 lines

Quoted 2 lines

#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 ;)

drivers/media/i2c/adp1653.c +0 -0

index d703636..7107ac2 100644

Hello Pavel,

Quoted 6 lines

drivers/media/i2c/adp1653.c +0 -0

index d703636..7107ac2 100644

drivers/media/i2c/adp1653.c line 42

Quoted 10 lines

drivers/media/i2c/adp1653.c +2 -2

@@ -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

Quoted 3 lines · On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote:

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

Quoted 2 lines

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

Add device tree support for adp1653 flash LED driver.

Signed-off-by Pavel Machek <[email protected]>

---

Second part of a patch after documentation was merged.

Please apply,
Pavel

drivers/media/i2c/adp1653.c +1 -0

index 873fe19..d703636 100644
@@ -8,6 +8,7 @@
  * Contributors:
  *	Sakari Ailus <[email protected]>
  *	Tuukka Toivonen <[email protected]>
+ *	Pavel Machek <[email protected]>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License

drivers/media/i2c/adp1653.c +4 -1

 #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);

drivers/media/i2c/adp1653.c +6 -1

@@ -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;
 }

drivers/media/i2c/adp1653.c +6 -0

+		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;

drivers/media/i2c/adp1653.c +19 -0

+		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;

drivers/media/i2c/adp1653.c +7 -1

+	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,
 };

include/media/adp1653.h +0 -1

index 1d9b48a..34b505e 100644
@@ -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 */

Quoted 2 lines

#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.

Quoted 3 lines

Quoted 1 line

+ - 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.

drivers/media/i2c/adp1653.c static int adp1653_resume(struct device *dev) · line 455

Quoted 5 lines

drivers/media/i2c/adp1653.c +5 -0

+	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.