| Message ID | 20150409074238.GA22603@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 873fe19..d703636 100644 --- a/drivers/media/i2c/adp1653.c +++ b/drivers/media/i2c/adp1653.c @@ -8,6 +8,7 @@ * Contributors: * Sakari Ailus <sakari.ailus@iki.fi> * Tuukka Toivonen <tuukkat76@gmail.com> + * Pavel Machek <pavel@ucw.cz> * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -34,6 +35,8 @@ #include <linux/module.h> #include <linux/i2c.h> #include <linux/slab.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> #include <media/adp1653.h> #include <media/v4l2-device.h> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash) static int __adp1653_set_power(struct adp1653_flash *flash, int on) { - int ret; + int ret = 0; + + if (flash->platform_data->power) { + ret = flash->platform_data->power(&flash->subdev, on); + } else { + gpiod_set_value(flash->platform_data->enable_gpio, on); + if (on) + /* Some delay is apparently required. */ + udelay(20); + } - ret = flash->platform_data->power(&flash->subdev, on); if (ret < 0) return ret; @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) return 0; ret = adp1653_init_device(flash); - if (ret < 0) + if (ret >= 0) + return ret; + + if (flash->platform_data->power) flash->platform_data->power(&flash->subdev, 0); + else + gpiod_set_value(flash->platform_data->enable_gpio, 0); return ret; } @@ -407,21 +423,78 @@ static int adp1653_resume(struct device *dev) #endif /* CONFIG_PM */ +static int adp1653_of_init(struct i2c_client *client, + struct adp1653_flash *flash, + struct device_node *node) +{ + u32 val; + struct adp1653_platform_data *pd; + struct device_node *child = NULL; + + if (!node) + return -EINVAL; + + pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); + if (!pd) + return -ENOMEM; + flash->platform_data = pd; + + child = of_get_child_by_name(node, "flash"); + if (!child) + return -EINVAL; + + if (of_property_read_u32(child, "flash-timeout-us", &val)) + goto err; + + pd->max_flash_timeout = val; + if (of_property_read_u32(child, "flash-max-microamp", &val)) + goto err; + pd->max_flash_intensity = val/1000; + + if (of_property_read_u32(child, "max-microamp", &val)) + goto err; + pd->max_torch_intensity = val/1000; + of_node_put(child); + + child = of_get_child_by_name(node, "indicator"); + if (!child) + return -EINVAL; + if (of_property_read_u32(child, "max-microamp", &val)) + goto err; + pd->max_indicator_intensity = val; + + of_node_put(child); + + pd->enable_gpio = devm_gpiod_get(&client->dev, "enable"); + if (!pd->enable_gpio) { + dev_err(&client->dev, "Error getting GPIO\n"); + return -EINVAL; + } + + return 0; +err: + dev_err(&client->dev, "Required property not found\n"); + of_node_put(child); + return -EINVAL; +} + + static int adp1653_probe(struct i2c_client *client, const struct i2c_device_id *devid) { struct adp1653_flash *flash; int ret; - /* we couldn't work without platform data */ - if (client->dev.platform_data == NULL) - return -ENODEV; - flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL); if (flash == NULL) return -ENOMEM; flash->platform_data = client->dev.platform_data; + if (client->dev.of_node) { + ret = adp1653_of_init(client, flash, client->dev.of_node); + if (ret) + return ret; + } mutex_init(&flash->power_lock); @@ -442,6 +515,7 @@ static int adp1653_probe(struct i2c_client *client, return 0; free_and_quit: + dev_err(&client->dev, "adp1653: failed to register device\n"); v4l2_ctrl_handler_free(&flash->ctrls); return ret; } @@ -464,7 +538,7 @@ static const struct i2c_device_id adp1653_id_table[] = { }; MODULE_DEVICE_TABLE(i2c, adp1653_id_table); -static struct dev_pm_ops adp1653_pm_ops = { +static const struct dev_pm_ops adp1653_pm_ops = { .suspend = adp1653_suspend, .resume = adp1653_resume, }; diff --git a/include/media/adp1653.h b/include/media/adp1653.h index 1d9b48a..34b505e 100644 --- a/include/media/adp1653.h +++ b/include/media/adp1653.h @@ -100,9 +100,11 @@ struct adp1653_platform_data { int (*power)(struct v4l2_subdev *sd, int on); u32 max_flash_timeout; /* flash light timeout in us */ - u32 max_flash_intensity; /* led intensity, flash mode */ - u32 max_torch_intensity; /* led intensity, torch mode */ - u32 max_indicator_intensity; /* indicator led intensity */ + u32 max_flash_intensity; /* led intensity, flash mode, mA */ + u32 max_torch_intensity; /* led intensity, torch mode, mA */ + u32 max_indicator_intensity; /* indicator led intensity, uA */ + + struct gpio_desc *enable_gpio; /* for device-tree based boot */ }; #define to_adp1653_flash(sd) container_of(sd, struct adp1653_flash, subdev)
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