| Message ID | 1429141034-29237-1-git-send-email-sakari.ailus@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sakari, Since this driver won't make it into 4.1 anyways, I have one more comment: On Thu, Apr 16, 2015 at 02:37:13AM +0300, Sakari Ailus wrote: > From: Pavel Machek <pavel@ucw.cz> > > Add device tree support for adp1653 flash LED driver. > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > --- > Hi folks, > > Here's an updated adp1653 DT patch, with changes since v7: > > - Include of.h and gpio/consumer.h instead of of_gpio.h and gpio.h. > > - Don't initialise ret as zero in __adp1653_set_power(), check ret only when > it's been set. > > - Don't check for node non-NULL in adp1653_of_init(). It never is NULL. > > - Remove temporary variable val in adp1653_of_init(). > > - If the device has no of_node, check that platform data is non-NULL; > otherwise return an error. > > - Assign flash->platform_data only if dev->of_node is NULL. > > drivers/media/i2c/adp1653.c | 100 ++++++++++++++++++++++++++++++++++++++----- > include/media/adp1653.h | 8 ++-- > 2 files changed, 95 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 873fe19..c70abab 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.h> > +#include <linux/gpio/consumer.h> > #include <media/adp1653.h> > #include <media/v4l2-device.h> > > @@ -308,16 +311,28 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > { > int ret; > > - ret = flash->platform_data->power(&flash->subdev, on); > - if (ret < 0) > - return ret; > + if (flash->platform_data->power) { > + ret = flash->platform_data->power(&flash->subdev, on); > + if (ret < 0) > + return ret; > + } else { > + gpiod_set_value(flash->platform_data->enable_gpio, on); > + if (on) > + /* Some delay is apparently required. */ > + udelay(20); > + } I suggest to remove the power callback from platform data. Instead you can require to setup a gpiod lookup table in the boardcode, if platform data based initialization is used (see for example si4713 initialization in board-rx51-periphals.c). This will reduce complexity in the driver and should be fairly easy to implement, since there is no adp1653 platform code user in the mainline kernel anyways. > if (!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 +422,85 @@ 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) > +{ > + struct adp1653_platform_data *pd; > + struct device_node *child; > + > + 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", > + &pd->max_flash_timeout)) > + goto err; > + > + if (of_property_read_u32(child, "flash-max-microamp", > + &pd->max_flash_intensity)) > + goto err; > + > + pd->max_flash_intensity /= 1000; > + > + if (of_property_read_u32(child, "led-max-microamp", > + &pd->max_torch_intensity)) > + goto err; > + > + pd->max_torch_intensity /= 1000; > + of_node_put(child); > + > + child = of_get_child_by_name(node, "indicator"); > + if (!child) > + return -EINVAL; > + > + if (of_property_read_u32(child, "led-max-microamp", > + &pd->max_indicator_intensity)) > + goto err; > + > + 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; > + } else { > + if (!client->dev.platform_data) { > + dev_err(&client->dev, > + "Neither DT not platform data provided\n"); > + return EINVAL; > + } > + flash->platform_data = client->dev.platform_data; > + } > > mutex_init(&flash->power_lock); > > @@ -442,6 +521,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 +544,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..9779c85 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 */ IMHO this should become part of "struct adp1653_flash", so that adp1653_platform_data only contains variables, which should be filled by boardcode / manual DT parsing code. > }; > > #define to_adp1653_flash(sd) container_of(sd, struct adp1653_flash, subdev) > -- > 1.7.10.4 > -- Sebastian
On Thu 2015-04-16 07:24:42, Sebastian Reichel wrote: > Hi Sakari, > > Since this driver won't make it into 4.1 anyways, I have one more > comment: Like this driver did not receive enough bikesheding. > > + } else { > > + gpiod_set_value(flash->platform_data->enable_gpio, on); > > + if (on) > > + /* Some delay is apparently required. */ > > + udelay(20); > > + } > > I suggest to remove the power callback from platform data. Instead > you can require to setup a gpiod lookup table in the boardcode, if > platform data based initialization is used (see for example si4713 > initialization in board-rx51-periphals.c). > > This will reduce complexity in the driver and should be fairly easy > to implement, since there is no adp1653 platform code user in the > mainline kernel anyways. I'd hate to break out of tree users for very little gain. Pavel
Hi Pavel, On Thu, Apr 16, 2015 at 07:58:18AM +0200, Pavel Machek wrote: > On Thu 2015-04-16 07:24:42, Sebastian Reichel wrote: > > Hi Sakari, > > > > Since this driver won't make it into 4.1 anyways, I have one more > > comment: > > Like this driver did not receive enough bikesheding. You should become more restrained with this argument if you want it to be taken seriously by me in the future... > > > + } else { > > > + gpiod_set_value(flash->platform_data->enable_gpio, on); > > > + if (on) > > > + /* Some delay is apparently required. */ > > > + udelay(20); > > > + } > > > > I suggest to remove the power callback from platform data. Instead > > you can require to setup a gpiod lookup table in the boardcode, if > > platform data based initialization is used (see for example si4713 > > initialization in board-rx51-periphals.c). > > > > This will reduce complexity in the driver and should be fairly easy > > to implement, since there is no adp1653 platform code user in the > > mainline kernel anyways. > > I'd hate to break out of tree users for very little gain. This normally happens as the kernel progresses. If you want your driver not to break, you should sent it upstream and maintain it. Also the only out-of-tree user I know is the Nokia N900 (which has already broken camera subsystem). Note that the required out of tree changes to use the platform code with gpiod interface are actually quite small and if you really care about it, they could actually be done *in kernel*. Note that many drivers are updated to use new kernel APIs together with the DT changes - especially those, which haven't been updated for quite some time. So let's have a look at the advantages of removing the power gpio: + gpio handling is always done in the driver, making code easier to read + less loc in the driver, making it easier to read + less loc in the boardcode (no gpio requesting/releasing) + less branching in driver code - easier testing coverage - out of tree users will break So basically its code quality vs minor out-of-tree breakage. -- Sebastian
Hi! > > > This will reduce complexity in the driver and should be fairly easy > > > to implement, since there is no adp1653 platform code user in the > > > mainline kernel anyways. > > > > I'd hate to break out of tree users for very little gain. ... > So let's have a look at the advantages of removing the power gpio: One change per patch. My change did what it said, "add a device tree support", if you want to do second change "break existing interface", feel free doing it as a separate patch. Pavel
Hi Sebastian, On Thu, Apr 16, 2015 at 07:24:42AM +0200, Sebastian Reichel wrote: > Hi Sakari, > > Since this driver won't make it into 4.1 anyways, I have one more > comment: > > On Thu, Apr 16, 2015 at 02:37:13AM +0300, Sakari Ailus wrote: ... > > @@ -308,16 +311,28 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) > > { > > int ret; > > > > - ret = flash->platform_data->power(&flash->subdev, on); > > - if (ret < 0) > > - return ret; > > + if (flash->platform_data->power) { > > + ret = flash->platform_data->power(&flash->subdev, on); > > + if (ret < 0) > > + return ret; > > + } else { > > + gpiod_set_value(flash->platform_data->enable_gpio, on); > > + if (on) > > + /* Some delay is apparently required. */ > > + udelay(20); > > + } > > I suggest to remove the power callback from platform data. Instead > you can require to setup a gpiod lookup table in the boardcode, if > platform data based initialization is used (see for example si4713 > initialization in board-rx51-periphals.c). > > This will reduce complexity in the driver and should be fairly easy > to implement, since there is no adp1653 platform code user in the > mainline kernel anyways. There are a couple of out-of-tree users perhaps. I think that I'd rather remove platform data support altogether than trying to polish it. The timeline could be about the same than with the omap3isp driver, that shouldn't be too many minor kernel versions either. What do you think? ... > > diff --git a/include/media/adp1653.h b/include/media/adp1653.h > > index 1d9b48a..9779c85 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 */ > > IMHO this should become part of "struct adp1653_flash", so that > adp1653_platform_data only contains variables, which should be > filled by boardcode / manual DT parsing code. We'll get rid of the whole header with platform data support removal. :-)
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 873fe19..c70abab 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.h> +#include <linux/gpio/consumer.h> #include <media/adp1653.h> #include <media/v4l2-device.h> @@ -308,16 +311,28 @@ __adp1653_set_power(struct adp1653_flash *flash, int on) { int ret; - ret = flash->platform_data->power(&flash->subdev, on); - if (ret < 0) - return ret; + if (flash->platform_data->power) { + ret = flash->platform_data->power(&flash->subdev, on); + if (ret < 0) + return ret; + } else { + gpiod_set_value(flash->platform_data->enable_gpio, on); + if (on) + /* Some delay is apparently required. */ + udelay(20); + } if (!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 +422,85 @@ 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) +{ + struct adp1653_platform_data *pd; + struct device_node *child; + + 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", + &pd->max_flash_timeout)) + goto err; + + if (of_property_read_u32(child, "flash-max-microamp", + &pd->max_flash_intensity)) + goto err; + + pd->max_flash_intensity /= 1000; + + if (of_property_read_u32(child, "led-max-microamp", + &pd->max_torch_intensity)) + goto err; + + pd->max_torch_intensity /= 1000; + of_node_put(child); + + child = of_get_child_by_name(node, "indicator"); + if (!child) + return -EINVAL; + + if (of_property_read_u32(child, "led-max-microamp", + &pd->max_indicator_intensity)) + goto err; + + 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; + } else { + if (!client->dev.platform_data) { + dev_err(&client->dev, + "Neither DT not platform data provided\n"); + return EINVAL; + } + flash->platform_data = client->dev.platform_data; + } mutex_init(&flash->power_lock); @@ -442,6 +521,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 +544,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..9779c85 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)