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
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 <[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,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
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 <[email protected]>
>
> ---
>
> 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 <[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);
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: [email protected] XMPP: [email protected]
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 <[email protected]>
> >
> > ---
> >
> > 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
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,
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 <[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;
+ 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
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 <[email protected]> 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 <[email protected]>
>
> ---
>
> 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 <[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;
> + 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
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 <[email protected]>
> > >
> > > ---
> > >
> > > 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: [email protected] XMPP: [email protected]
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
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
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,
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 <[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))
+ 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
Hello Pavel,
On Fri, Apr 3, 2015 at 10:21 AM, Pavel Machek <[email protected]> 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
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: [email protected] XMPP: [email protected]
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 <[email protected]>
>
> ---
>
> 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 <[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))
> + 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: [email protected] XMPP: [email protected]
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
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
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
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
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: [email protected] XMPP: [email protected]
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: [email protected] XMPP: [email protected]
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: [email protected] XMPP: [email protected]
Documentation for adp1653 binding.
Signed-off-by: Pavel Machek <[email protected]>
---
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
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 <[email protected]>
>
> ---
>
> 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: [email protected] XMPP: [email protected]
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
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: [email protected] XMPP: [email protected]
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
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 <[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->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
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
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
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
Fix includes according to Sebastian.
Signed-off-by: Pavel Machek <[email protected]>
---
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
Hello Pavel,
On Thu, Apr 9, 2015 at 2:31 PM, Pavel Machek <[email protected]> wrote:
> Fix includes according to Sebastian.
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> ---
>
> 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
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
[email protected]
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 <[email protected]>
>
> ---
>
> 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 <[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>
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: [email protected] XMPP: [email protected]
Hello Pali,
On Thu, Apr 9, 2015 at 2:59 PM, Pali Rohár <[email protected]> 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
> [email protected]
Best regards,
Javier
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