Hi Sakari, thks for review. On 07/09/2017 01:06 AM, Sakari Ailus wrote: > Hi Hugues, > > On Mon, Jul 03, 2017 at 11:16:04AM +0200, Hugues Fruchet wrote: >> Allows use of device tree configuration data. >> If no device tree data is there, configuration is taken from platform data. >> In order to keep GPIOs configuration compatible between both way of doing, >> GPIOs are switched to descriptor-based interface. >> >> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx> >> --- >> drivers/media/i2c/Kconfig | 2 +- >> drivers/media/i2c/ov9650.c | 77 ++++++++++++++++++++++++++++++++++------------ >> 2 files changed, 59 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 121b3b5..168115c 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -615,7 +615,7 @@ config VIDEO_OV7670 >> >> config VIDEO_OV9650 >> tristate "OmniVision OV9650/OV9652 sensor support" >> - depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API >> ---help--- >> This is a V4L2 sensor-level driver for the Omnivision >> OV9650 and OV9652 camera sensors. >> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c >> index 1e4e99e..7e9a902 100644 >> --- a/drivers/media/i2c/ov9650.c >> +++ b/drivers/media/i2c/ov9650.c >> @@ -11,12 +11,14 @@ >> * it under the terms of the GNU General Public License version 2 as >> * published by the Free Software Foundation. >> */ >> +#include <linux/clk.h> >> #include <linux/delay.h> >> #include <linux/gpio.h> >> #include <linux/i2c.h> >> #include <linux/kernel.h> >> #include <linux/media.h> >> #include <linux/module.h> >> +#include <linux/of_gpio.h> >> #include <linux/ratelimit.h> >> #include <linux/slab.h> >> #include <linux/string.h> >> @@ -249,9 +251,10 @@ struct ov965x { >> struct v4l2_subdev sd; >> struct media_pad pad; >> enum v4l2_mbus_type bus_type; >> - int gpios[NUM_GPIOS]; >> + struct gpio_desc *gpios[NUM_GPIOS]; >> /* External master clock frequency */ >> unsigned long mclk_frequency; >> + struct clk *clk; >> >> /* Protects the struct fields below */ >> struct mutex lock; >> @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x) >> return 0; >> } >> >> -static void ov965x_gpio_set(int gpio, int val) >> +static void ov965x_gpio_set(struct gpio_desc *gpio, int val) >> { >> - if (gpio_is_valid(gpio)) >> - gpio_set_value(gpio, val); >> + if (gpio) >> + gpiod_set_value_cansleep(gpio, val); > > gpiod_set_value_cansleep() can manage with NULL gpio parameter, no need to > check it. done > >> } >> >> static void __ov965x_set_power(struct ov965x *ov965x, int on) >> @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x *ov965x, >> const struct ov9650_platform_data *pdata) >> { >> int ret, i; >> + int gpios[NUM_GPIOS]; >> >> - ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn; >> - ov965x->gpios[GPIO_RST] = pdata->gpio_reset; >> + gpios[GPIO_PWDN] = pdata->gpio_pwdn; >> + gpios[GPIO_RST] = pdata->gpio_reset; >> >> - for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) { >> - int gpio = ov965x->gpios[i]; >> + for (i = 0; i < ARRAY_SIZE(gpios); i++) { >> + int gpio = gpios[i]; >> >> if (!gpio_is_valid(gpio)) >> continue; >> ret = devm_gpio_request_one(&ov965x->client->dev, gpio, >> - GPIOF_OUT_INIT_HIGH, "OV965X"); >> - if (ret < 0) >> + GPIOF_OUT_INIT_HIGH, DRIVER_NAME); > > DRIVER_NAME is different from "OV965X". Is this an intended change? Yes it was to unify namings around a single DRIVER_NAME definition. > >> + if (ret < 0) { >> + dev_err(&ov965x->client->dev, >> + "Failed to request gpio%d (%d)\n", gpio, ret); >> return ret; >> + } >> v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio); >> >> gpio_set_value(gpio, 1); >> gpio_export(gpio, 0); >> - ov965x->gpios[i] = gpio; >> + ov965x->gpios[i] = gpio_to_desc(gpio); >> } >> >> return 0; >> @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client, >> struct v4l2_subdev *sd; >> struct ov965x *ov965x; >> int ret; >> + struct device_node *np = client->dev.of_node; > > It'd be nice to declare this next to pdata, rather than after ret and other > short declarations. done > >> >> - if (pdata == NULL) { >> - dev_err(&client->dev, "platform data not specified\n"); >> - return -EINVAL; >> - } >> - >> - if (pdata->mclk_frequency == 0) { >> - dev_err(&client->dev, "MCLK frequency not specified\n"); >> + if (!pdata && !np) { >> + dev_err(&client->dev, "Platform data or device tree data must be provided\n"); >> return -EINVAL; >> } >> >> @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client, >> >> mutex_init(&ov965x->lock); >> ov965x->client = client; >> - ov965x->mclk_frequency = pdata->mclk_frequency; >> + mutex_init(&ov965x->lock); >> + >> + if (np) { >> + /* Device tree */ >> + ov965x->gpios[GPIO_RST] = >> + devm_gpiod_get_optional(&client->dev, "resetb", >> + GPIOD_OUT_LOW); >> + ov965x->gpios[GPIO_PWDN] = >> + devm_gpiod_get_optional(&client->dev, "pwdn", >> + GPIOD_OUT_HIGH); >> + >> + ov965x->clk = devm_clk_get(&client->dev, NULL); >> + if (IS_ERR(ov965x->clk)) { >> + dev_err(&client->dev, "Could not get clock\n"); > > mutex_destroy() should called on an initialised mutex if probe is going to > fail. It's certainly not a problem introduced by this patch, but it'd be > nice to fix that (in a separate patch) now that it's found. The same goes > for remove below. Will do. > >> + return PTR_ERR(ov965x->clk); >> + } >> + ov965x->mclk_frequency = clk_get_rate(ov965x->clk); >> + } else { >> + /* Platform data */ >> + ret = ov965x_configure_gpios(ov965x, pdata); >> + if (ret < 0) >> + return ret; >> + >> + if (pdata->mclk_frequency == 0) { >> + dev_err(&client->dev, "MCLK frequency is mandatory\n"); >> + return -EINVAL; >> + } >> + ov965x->mclk_frequency = pdata->mclk_frequency; >> + } >> >> sd = &ov965x->sd; >> v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops); >> @@ -1551,9 +1582,17 @@ static int ov965x_remove(struct i2c_client *client) >> }; >> MODULE_DEVICE_TABLE(i2c, ov965x_id); >> >> +static const struct of_device_id ov965x_of_match[] = { >> + { .compatible = "ovti,ov9650", }, >> + { .compatible = "ovti,ov9652", }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, ov965x_of_match); >> + >> static struct i2c_driver ov965x_i2c_driver = { >> .driver = { >> .name = DRIVER_NAME, >> + .of_match_table = of_match_ptr(ov965x_of_match), >> }, >> .probe = ov965x_probe, >> .remove = ov965x_remove, > ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f