| Message ID | 20150404074337.GA31064@amd |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Pavel, On Sat, Apr 04, 2015 at 09:43:37AM +0200, Pavel Machek wrote: > > Documentation for adp1653 binding. s/binding/bindings/ > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > --- > > Please apply. > > Sorry, wrong version of patch was sent last time. > Pavel > > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > new file mode 100644 > index 0000000..4607ce3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > @@ -0,0 +1,37 @@ > +* Analog Devices ADP1653 flash LED driver > + > +Required Properties: > + > + - compatible: Must contain be "adi,adp1653" s/be // > + > + - reg: I2C slave address > + > + - enable-gpios: Reference to the GPIO that controls the power for the chip. How about: enable-gpios: Specifier of the GPIO connected to EN pin I can make the changes if you're ok with that, otherwise please send v7. Then I'll apply that to my tree.
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
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
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
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
Hello Pavel, On Thu, Apr 9, 2015 at 2:31 PM, Pavel Machek <pavel@ucw.cz> wrote: > Fix includes according to Sebastian. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > --- > > On Thu 2015-04-09 14:19:14, Sebastian Reichel wrote: >> On Thu, Apr 09, 2015 at 01:29:43PM +0200, Pavel Machek wrote: >> > On Thu 2015-04-09 11:10:17, Sebastian Reichel wrote: >> > > On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote: >> > > > [...] >> > > > +#include <linux/of_gpio.h> >> > > > +#include <linux/gpio.h> >> > > > [...] >> > > >> > > This should probably be >> > > >> > > #include <linux/of.h> >> > > #include <linux/gpio/consumer.h> >> > >> > And I thought people would only bikesched paint on the >> > Documentation. Sakari, feel free to change that, but >> >> > a) I don't see why Sebastian's version is better >> >> You neither use <linux/of_gpio.h> nor <linux/gpio.h>. >> >> Well "include/linux/gpio.h" describes the old gpio API. The new >> gpiod gpiod API is described in "include/linux/gpio/consumer.h" and >> you use it, so the include should be included ;) >> >> You don't use anything from "include/linux/of_gpio.h", but it >> includes "include/linux/of.h", which you are using. So you should >> include <linux/of.h> instead ;) > > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index d703636..7107ac2 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -35,8 +35,8 @@ > #include <linux/module.h> > #include <linux/i2c.h> > #include <linux/slab.h> > -#include <linux/of_gpio.h> > -#include <linux/gpio.h> > +#include <linux/of.h> > +#include <linux/gpio/consumer.h> > #include <media/adp1653.h> > #include <media/v4l2-device.h> > Please re-spin your previous patch and submit it properly. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
Hi Pavel, (Cc linux-media. Media related patches should be sent there.) On Thu, Apr 09, 2015 at 09:42:38AM +0200, Pavel Machek wrote: > > Add device tree support for adp1653 flash LED driver. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > --- > > Second part of a patch after documentation was merged. > > Please apply, > Pavel > > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 873fe19..d703636 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -8,6 +8,7 @@ > * Contributors: > * Sakari Ailus <sakari.ailus@iki.fi> > * Tuukka Toivonen <tuukkat76@gmail.com> > + * Pavel Machek <pavel@ucw.cz> > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -34,6 +35,8 @@ > #include <linux/module.h> > #include <linux/i2c.h> > #include <linux/slab.h> > +#include <linux/of_gpio.h> > +#include <linux/gpio.h> As Sebastian suggested, linux/of.h and linux/gpio/consumer.h should be used. > #include <media/adp1653.h> > #include <media/v4l2-device.h> > > @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash) > static int > __adp1653_set_power(struct adp1653_flash *flash, int on) > { > - int ret; > + int ret = 0; > + > + if (flash->platform_data->power) { > + ret = flash->platform_data->power(&flash->subdev, on); > + } else { > + gpiod_set_value(flash->platform_data->enable_gpio, on); > + if (on) > + /* Some delay is apparently required. */ > + udelay(20); > + } > > - ret = flash->platform_data->power(&flash->subdev, on); > if (ret < 0) > return ret; Please check ret after assigning it. The assignment in declaration is unnecessary. > > @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > return 0; > > ret = adp1653_init_device(flash); > - if (ret < 0) > + if (ret >= 0) > + return ret; > + > + if (flash->platform_data->power) > flash->platform_data->power(&flash->subdev, 0); > + else > + gpiod_set_value(flash->platform_data->enable_gpio, 0); > > return ret; > } > @@ -407,21 +423,78 @@ static int adp1653_resume(struct device *dev) > > #endif /* CONFIG_PM */ > > +static int adp1653_of_init(struct i2c_client *client, > + struct adp1653_flash *flash, > + struct device_node *node) > +{ > + u32 val; > + struct adp1653_platform_data *pd; > + struct device_node *child = NULL; The NULL assignment can be removed. > + > + if (!node) > + return -EINVAL; node is always non-NULL here; no need to check. > + > + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); > + if (!pd) > + return -ENOMEM; > + flash->platform_data = pd; > + > + child = of_get_child_by_name(node, "flash"); > + if (!child) > + return -EINVAL; > + > + if (of_property_read_u32(child, "flash-timeout-us", &val)) You could read the values directly to the appropriate struct adp1653_platform_data field. > + goto err; > + > + pd->max_flash_timeout = val; > + if (of_property_read_u32(child, "flash-max-microamp", &val)) > + goto err; > + pd->max_flash_intensity = val/1000; > + > + if (of_property_read_u32(child, "max-microamp", &val)) > + goto err; > + pd->max_torch_intensity = val/1000; > + of_node_put(child); > + > + child = of_get_child_by_name(node, "indicator"); > + if (!child) > + return -EINVAL; > + if (of_property_read_u32(child, "max-microamp", &val)) Let's wait a bit the resolution of the property name. I'm in principle fine with both. I can do the change once it's been decided, hopefully very soon. > + goto err; > + pd->max_indicator_intensity = val; > + > + of_node_put(child); > + > + pd->enable_gpio = devm_gpiod_get(&client->dev, "enable"); > + if (!pd->enable_gpio) { > + dev_err(&client->dev, "Error getting GPIO\n"); > + return -EINVAL; > + } > + > + return 0; > +err: > + dev_err(&client->dev, "Required property not found\n"); > + of_node_put(child); > + return -EINVAL; > +} > + > + > static int adp1653_probe(struct i2c_client *client, > const struct i2c_device_id *devid) > { > struct adp1653_flash *flash; > int ret; > > - /* we couldn't work without platform data */ > - if (client->dev.platform_data == NULL) > - return -ENODEV; > - > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); > if (flash == NULL) > return -ENOMEM; > > flash->platform_data = client->dev.platform_data; I think it'd be cleaner to make the assignment only if not using of, i.e. add else branch to the if below. > + if (client->dev.of_node) { > + ret = adp1653_of_init(client, flash, client->dev.of_node); > + if (ret) > + return ret; > + } > > mutex_init(&flash->power_lock); > > @@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client, > return 0; > > free_and_quit: > + dev_err(&client->dev, "adp1653: failed to register device\n"); > v4l2_ctrl_handler_free(&flash->ctrls); > return ret; > } > @@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = { > }; > MODULE_DEVICE_TABLE(i2c, adp1653_id_table); > > -static struct dev_pm_ops adp1653_pm_ops = { > +static const struct dev_pm_ops adp1653_pm_ops = { > .suspend = adp1653_suspend, > .resume = adp1653_resume, > }; > diff --git a/include/media/adp1653.h b/include/media/adp1653.h > index 1d9b48a..34b505e 100644 > --- a/include/media/adp1653.h > +++ b/include/media/adp1653.h > @@ -100,9 +100,11 @@ struct adp1653_platform_data { > int (*power)(struct v4l2_subdev *sd, int on); > > u32 max_flash_timeout; /* flash light timeout in us */ > - u32 max_flash_intensity; /* led intensity, flash mode */ > - u32 max_torch_intensity; /* led intensity, torch mode */ > - u32 max_indicator_intensity; /* indicator led intensity */ > + u32 max_flash_intensity; /* led intensity, flash mode, mA */ > + u32 max_torch_intensity; /* led intensity, torch mode, mA */ > + u32 max_indicator_intensity; /* indicator led intensity, uA */ > + > + struct gpio_desc *enable_gpio; /* for device-tree based boot */ > }; > > #define to_adp1653_flash(sd) container_of(sd, struct adp1653_flash, subdev) > Let me know if you're going to send v8 or if I can make the changes. I think we're pretty much done then.
Hello Pali, On Thu, Apr 9, 2015 at 2:59 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Thursday 09 April 2015 14:43:59 Javier Martinez Canillas wrote: >> >> Please re-spin your previous patch and submit it properly. >> >> Best regards, >> Javier > > Hi all! What about stopping this meaningless discussion about resending > full patch series when everybody know how to fix is quickly in editor > (e.g with sed under 5s) and not wasting another 10 minutes to generate > new unified diff sent via SMTP protocol? > No, there is a reason why we have written rules on how patches should be submitted. Everyone in the kernel community is expected to optimize their workflow according to these rules to make life easier for people reviewing and merging the patches. As you said now someone has to fix this using an editor and that is an error prone process. Besides, why it would take 10 minutes to generate a proper patch-set? git is very good on this regard (i.e: git commit ---fixup && git rebase -i && git format-patch && git send-email). I won't argue anymore but I find very sad that people who have been in the kernel community for years don't follow the basic rules we have documented it. So I wonder why we have the documentation in the first place and how can we expect newcomers to follow if even experienced kernel hackers don't. > -- > Pali Rohár > pali.rohar@gmail.com Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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>; + }; + };
Documentation for adp1653 binding. Signed-off-by: Pavel Machek <pavel@ucw.cz> --- Please apply. Sorry, wrong version of patch was sent last time. Pavel