| Message ID | 20150402143846.GA11687@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pawel, My apologies for the very late reply. On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote: > > > We are moving to device tree support on OMAP3, but that currently > breaks ADP1653 driver. This adds device tree support, plus required > documentation. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > --- > > I'm not sure if it is device tree or media framework, either everyone > waits for someone else, or noone really cares. Neither. Some people are unfortuantely very busy with many other things as well. :-P > Andrew, can you just merge it? > > Please apply, Please wait just a while. I think we can merge this eventually through the linux-media tree, but please first see the comments below. > Pavel > > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > new file mode 100644 > index 0000000..0fc28a9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > @@ -0,0 +1,37 @@ > +* Analog Devices ADP1653 flash LED driver > + > +Required Properties: > + > + - compatible: Must contain be "adi,adp1653" > + > + - reg: I2C slave address > + > + - gpios: References to the GPIO that controls the power for the chip. > + > +There are two led outputs available - flash and indicator. One led is > +represented by one child node, nodes need to be named "flash" and "indicator". > + > +Required properties of the LED child node: > +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt > + > +Required properties of the flash LED child node: > + > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt The documentation says that the maximum value is used if these values are not specified. I think I'd make these optional. > + > +Example: > + > + adp1653: led-controller@30 { > + compatible = "adi,adp1653"; > + reg = <0x30>; > + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ > + > + flash { > + flash-timeout-us = <500000>; > + flash-max-microamp = <320000>; > + max-microamp = <50000>; > + }; > + indicator { > + max-microamp = <17500>; > + }; > + }; > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 873fe19..0341009 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -8,6 +8,7 @@ > * Contributors: > * Sakari Ailus <sakari.ailus@iki.fi> > * Tuukka Toivonen <tuukkat76@gmail.com> > + * Pavel Machek <pavel@ucw.cz> > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -34,6 +35,8 @@ > #include <linux/module.h> > #include <linux/i2c.h> > #include <linux/slab.h> > +#include <linux/of_gpio.h> > +#include <linux/gpio.h> > #include <media/adp1653.h> > #include <media/v4l2-device.h> > > @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash) > static int > __adp1653_set_power(struct adp1653_flash *flash, int on) > { > - int ret; > + int ret = 0; > + > + if (flash->platform_data->power) { > + ret = flash->platform_data->power(&flash->subdev, on); The power() callback should be dropped. It's controlling a GPIO. But that can be done later on. The alternative is a patch before this one. > + } else { > + gpio_set_value(flash->platform_data->power_gpio, on); > + if (on) > + /* Some delay is apparently required. */ > + udelay(20); > + } > > - ret = flash->platform_data->power(&flash->subdev, on); > if (ret < 0) > return ret; > > @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > return 0; > > ret = adp1653_init_device(flash); > - if (ret < 0) > + if (ret >= 0) > + return ret; > + > + if (flash->platform_data->power) > flash->platform_data->power(&flash->subdev, 0); > + else > + gpio_set_value(flash->platform_data->power_gpio, 0); > > return ret; > } > @@ -407,21 +423,77 @@ static int adp1653_resume(struct device *dev) > > #endif /* CONFIG_PM */ > > +static int adp1653_of_init(struct i2c_client *client, > + struct adp1653_flash *flash, > + struct device_node *node) > +{ > + u32 val; > + struct adp1653_platform_data *pd; > + enum of_gpio_flags flags; > + int gpio; > + struct device_node *child; > + > + if (!node) > + return -EINVAL; > + > + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); > + if (!pd) > + return -ENOMEM; > + flash->platform_data = pd; > + > + child = of_get_child_by_name(node, "flash"); > + if (!child) > + return -EINVAL; > + if (of_property_read_u32(child, "flash-timeout-microsec", &val)) > + return -EINVAL; > + > + pd->max_flash_timeout = val; > + if (of_property_read_u32(child, "flash-max-microamp", &val)) > + return -EINVAL; > + pd->max_flash_intensity = val/1000; > + > + if (of_property_read_u32(child, "max-microamp", &val)) > + return -EINVAL; > + pd->max_torch_intensity = val/1000; > + > + child = of_get_child_by_name(node, "indicator"); > + if (!child) > + return -EINVAL; > + if (of_property_read_u32(child, "max-microamp", &val)) > + return -EINVAL; > + pd->max_indicator_intensity = val; > + > + if (!of_find_property(node, "gpios", NULL)) { > + dev_err(&client->dev, "No gpio node\n"); > + return -EINVAL; > + } > + > + pd->power_gpio = of_get_gpio_flags(node, 0, &flags); > + if (pd->power_gpio < 0) { > + dev_err(&client->dev, "Error getting GPIO\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > + > static int adp1653_probe(struct i2c_client *client, > const struct i2c_device_id *devid) > { > struct adp1653_flash *flash; > int ret; > > - /* we couldn't work without platform data */ > - if (client->dev.platform_data == NULL) > - return -ENODEV; > - > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > if (flash == NULL) > return -ENOMEM; > > flash->platform_data = client->dev.platform_data; > + if (!flash->platform_data) { I'd check whether dev->of_node is non-NULL instead. > + ret = adp1653_of_init(client, flash, client->dev.of_node); > + if (ret) > + return ret; > + } > > mutex_init(&flash->power_lock); > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client, > goto free_and_quit; > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > - I rather liked the newline here. Please don't remove it. :-) > return 0; > > free_and_quit: > + dev_err(&client->dev, "adp1653: failed to register device\n"); > v4l2_ctrl_handler_free(&flash->ctrls); > return ret; > } > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = { > }; > MODULE_DEVICE_TABLE(i2c, adp1653_id_table); > > -static struct dev_pm_ops adp1653_pm_ops = { > +static const struct dev_pm_ops adp1653_pm_ops = { > .suspend = adp1653_suspend, > .resume = adp1653_resume, > }; > > A corresponding change to the N900 dts would be very nice. I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
Hi! > Hi Pawel, > > My apologies for the very late reply. > > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote: > > > > > > We are moving to device tree support on OMAP3, but that currently > > breaks ADP1653 driver. This adds device tree support, plus required > > documentation. > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > --- > > > > I'm not sure if it is device tree or media framework, either everyone > > waits for someone else, or noone really cares. > > Neither. Some people are unfortuantely very busy with many other things as > well. :-P Well.. Being busy is ok. Nitpicking is also ok. But both at the same time... is not good. > > Andrew, can you just merge it? > > > > Please apply, > > Please wait just a while. > > I think we can merge this eventually through the linux-media tree, but > please first see the comments below. > > > +Required properties of the flash LED child node: > > + > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > > The documentation says that the maximum value is used if these values are > not specified. I think I'd make these optional. I'd rather not: when you make a typo in dts, it would supply maximum available current, potentially damaging the LED. You will not be able to tell brightness difference with naked eye... > > __adp1653_set_power(struct adp1653_flash *flash, int on) > > { > > - int ret; > > + int ret = 0; > > + > > + if (flash->platform_data->power) { > > + ret = flash->platform_data->power(&flash->subdev, on); > > The power() callback should be dropped. It's controlling a GPIO. But that > can be done later on. The alternative is a patch before this one. I'd prefer to do it later; we want to keep functionality on N900 without DTS, too. > > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > > if (flash == NULL) > > return -ENOMEM; > > > > flash->platform_data = client->dev.platform_data; > > + if (!flash->platform_data) { > > I'd check whether dev->of_node is non-NULL instead. Ok. > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client, > > goto free_and_quit; > > > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > > - > > I rather liked the newline here. Please don't remove it. :-) Ok. > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = { > > }; > > MODULE_DEVICE_TABLE(i2c, adp1653_id_table); > > > > -static struct dev_pm_ops adp1653_pm_ops = { > > +static const struct dev_pm_ops adp1653_pm_ops = { > > .suspend = adp1653_suspend, > > .resume = adp1653_resume, > > }; > > > > > > A corresponding change to the N900 dts would be very nice. Corresponding change to the dts will come in separate patch. Or do you have n900 for testing? > I think you're missing change to adp1653_i2c_driver.driver.of_match_table. It actually worked for me, which means device tree somehow does it magic. Pavel
Hi Pawel, On Thu, Apr 02, 2015 at 10:30:44PM +0200, Pavel Machek wrote: > Hi! > > > Hi Pawel, > > > > My apologies for the very late reply. > > > > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote: > > > > > > > > > We are moving to device tree support on OMAP3, but that currently > > > breaks ADP1653 driver. This adds device tree support, plus required > > > documentation. > > > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > > > > --- > > > > > > I'm not sure if it is device tree or media framework, either everyone > > > waits for someone else, or noone really cares. > > > > Neither. Some people are unfortuantely very busy with many other things as > > well. :-P > > Well.. Being busy is ok. Nitpicking is also ok. But both at the same > time... is not good. Good. Then we should be fine. :-) > > > > Andrew, can you just merge it? > > > > > > Please apply, > > > > Please wait just a while. > > > > I think we can merge this eventually through the linux-media tree, but > > please first see the comments below. > > > > > > +Required properties of the flash LED child node: > > > + > > > +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt > > > +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > > > > The documentation says that the maximum value is used if these values are > > not specified. I think I'd make these optional. > > I'd rather not: when you make a typo in dts, it would supply maximum > available current, potentially damaging the LED. You will not be able > to tell brightness difference with naked eye... Fine for me. > > > __adp1653_set_power(struct adp1653_flash *flash, int on) > > > { > > > - int ret; > > > + int ret = 0; > > > + > > > + if (flash->platform_data->power) { > > > + ret = flash->platform_data->power(&flash->subdev, on); > > > > The power() callback should be dropped. It's controlling a GPIO. But that > > can be done later on. The alternative is a patch before this one. > > I'd prefer to do it later; we want to keep functionality on N900 > without DTS, too. Fine as well. > > > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > > > if (flash == NULL) > > > return -ENOMEM; > > > > > > flash->platform_data = client->dev.platform_data; > > > + if (!flash->platform_data) { > > > > I'd check whether dev->of_node is non-NULL instead. > > Ok. > > > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client, > > > goto free_and_quit; > > > > > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > > > - > > > > I rather liked the newline here. Please don't remove it. :-) > > Ok. > > > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = { > > > }; > > > MODULE_DEVICE_TABLE(i2c, adp1653_id_table); > > > > > > -static struct dev_pm_ops adp1653_pm_ops = { > > > +static const struct dev_pm_ops adp1653_pm_ops = { > > > .suspend = adp1653_suspend, > > > .resume = adp1653_resume, > > > }; > > > > > > > > > > A corresponding change to the N900 dts would be very nice. > > Corresponding change to the dts will come in separate patch. Or do you > have n900 for testing? Yes, it should be a separate patch, I agree. I do have one but I can't say when I'd have time to test it. I'm fine with you having tested it though. > > I think you're missing change to adp1653_i2c_driver.driver.of_match_table. > > It actually worked for me, which means device tree somehow does it > magic. By magic? :-) It probably just ends up comparing the device and the driver names. How about adding the of_match_table?
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
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.
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
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.
diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt new file mode 100644 index 0000000..0fc28a9 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt @@ -0,0 +1,37 @@ +* Analog Devices ADP1653 flash LED driver + +Required Properties: + + - compatible: Must contain be "adi,adp1653" + + - reg: I2C slave address + + - gpios: References to the GPIO that controls the power for the chip. + +There are two led outputs available - flash and indicator. One led is +represented by one child node, nodes need to be named "flash" and "indicator". + +Required properties of the LED child node: +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt + +Required properties of the flash LED child node: + +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt + +Example: + + adp1653: led-controller@30 { + compatible = "adi,adp1653"; + reg = <0x30>; + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */ + + flash { + flash-timeout-us = <500000>; + flash-max-microamp = <320000>; + max-microamp = <50000>; + }; + indicator { + max-microamp = <17500>; + }; + }; diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 873fe19..0341009 100644 --- a/drivers/media/i2c/adp1653.c +++ b/drivers/media/i2c/adp1653.c @@ -8,6 +8,7 @@ * Contributors: * Sakari Ailus <sakari.ailus@iki.fi> * Tuukka Toivonen <tuukkat76@gmail.com> + * Pavel Machek <pavel@ucw.cz> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -34,6 +35,8 @@ #include <linux/module.h> #include <linux/i2c.h> #include <linux/slab.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> #include <media/adp1653.h> #include <media/v4l2-device.h> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash) static int __adp1653_set_power(struct adp1653_flash *flash, int on) { - int ret; + int ret = 0; + + if (flash->platform_data->power) { + ret = flash->platform_data->power(&flash->subdev, on); + } else { + gpio_set_value(flash->platform_data->power_gpio, on); + if (on) + /* Some delay is apparently required. */ + udelay(20); + } - ret = flash->platform_data->power(&flash->subdev, on); if (ret < 0) return ret; @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) return 0; ret = adp1653_init_device(flash); - if (ret < 0) + if (ret >= 0) + return ret; + + if (flash->platform_data->power) flash->platform_data->power(&flash->subdev, 0); + else + gpio_set_value(flash->platform_data->power_gpio, 0); return ret; } @@ -407,21 +423,77 @@ static int adp1653_resume(struct device *dev) #endif /* CONFIG_PM */ +static int adp1653_of_init(struct i2c_client *client, + struct adp1653_flash *flash, + struct device_node *node) +{ + u32 val; + struct adp1653_platform_data *pd; + enum of_gpio_flags flags; + int gpio; + struct device_node *child; + + if (!node) + return -EINVAL; + + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); + if (!pd) + return -ENOMEM; + flash->platform_data = pd; + + child = of_get_child_by_name(node, "flash"); + if (!child) + return -EINVAL; + if (of_property_read_u32(child, "flash-timeout-microsec", &val)) + return -EINVAL; + + pd->max_flash_timeout = val; + if (of_property_read_u32(child, "flash-max-microamp", &val)) + return -EINVAL; + pd->max_flash_intensity = val/1000; + + if (of_property_read_u32(child, "max-microamp", &val)) + return -EINVAL; + pd->max_torch_intensity = val/1000; + + child = of_get_child_by_name(node, "indicator"); + if (!child) + return -EINVAL; + if (of_property_read_u32(child, "max-microamp", &val)) + return -EINVAL; + pd->max_indicator_intensity = val; + + if (!of_find_property(node, "gpios", NULL)) { + dev_err(&client->dev, "No gpio node\n"); + return -EINVAL; + } + + pd->power_gpio = of_get_gpio_flags(node, 0, &flags); + if (pd->power_gpio < 0) { + dev_err(&client->dev, "Error getting GPIO\n"); + return -EINVAL; + } + + return 0; +} + + static int adp1653_probe(struct i2c_client *client, const struct i2c_device_id *devid) { struct adp1653_flash *flash; int ret; - /* we couldn't work without platform data */ - if (client->dev.platform_data == NULL) - return -ENODEV; - flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); if (flash == NULL) return -ENOMEM; flash->platform_data = client->dev.platform_data; + if (!flash->platform_data) { + ret = adp1653_of_init(client, flash, client->dev.of_node); + if (ret) + return ret; + } mutex_init(&flash->power_lock); @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client, goto free_and_quit; flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; - return 0; free_and_quit: + dev_err(&client->dev, "adp1653: failed to register device\n"); v4l2_ctrl_handler_free(&flash->ctrls); return ret; } @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[] = { }; MODULE_DEVICE_TABLE(i2c, adp1653_id_table); -static struct dev_pm_ops adp1653_pm_ops = { +static const struct dev_pm_ops adp1653_pm_ops = { .suspend = adp1653_suspend, .resume = adp1653_resume, };
We are moving to device tree support on OMAP3, but that currently breaks ADP1653 driver. This adds device tree support, plus required documentation. Signed-off-by: Pavel Machek <pavel@ucw.cz> --- I'm not sure if it is device tree or media framework, either everyone waits for someone else, or noone really cares. Andrew, can you just merge it? Please apply, Pavel