[PATCH v2 0/2] media: ov9650: support device tree probing

[PATCH v2 0/2] media: ov9650: support device tree probing

[Akinobu Mita]

This patchset adds device tree probing for ov9650 driver. This contains
an actual driver change and a newly added binding documentation part.

* Changelog v2
- Split binding documentation, suggested by Rob Herring and Jacopo Mondi
- Improve the wording for compatible property in the binding documentation,
  suggested by Jacopo Mondi
- Improve the description for the device node in the binding documentation,
  suggested by Sakari Ailus
- Remove ov965x_gpio_set() helper and open-code it, suggested by Jacopo Mondi
  and Sakari Ailus
- Call clk_prepare_enable() in s_power callback instead of probe, suggested
  by Sakari Ailus
- Unify clk and gpio configuration in a single if-else block and, also add
  a check either platform data or fwnode is actually specified, suggested
  by Jacopo Mondi
- Add CONFIG_OF guards, suggested by Jacopo Mondi

Akinobu Mita (2):
  media: ov9650: support device tree probing
  media: ov9650: add device tree binding

 .../devicetree/bindings/media/i2c/ov9650.txt       |  36 ++++++
 drivers/media/i2c/ov9650.c                         | 130 +++++++++++++++------
 2 files changed, 128 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov9650.txt

Cc: Jacopo Mondi <jacopo@jmondi.org>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Hugues Fruchet <hugues.fruchet@st.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh@kernel.org>
-- 
2.7.4

[PATCH v2 1/2] media: ov9650: support device tree probing

[Akinobu Mita]

The ov9650 driver currently only supports legacy platform data probe.
This change adds device tree probing.

There has been an attempt to add device tree support for ov9650 driver
by Hugues Fruchet as a part of the patchset that adds support of OV9655
camera (http://www.spinics.net/lists/linux-media/msg117903.html), but
it wasn't merged into mainline because creating a separate driver for
OV9655 is preferred.

This is very similar to Hugues's patch, but not supporting new device.

Cc: Jacopo Mondi <jacopo@jmondi.org>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Hugues Fruchet <hugues.fruchet@st.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* Changelog v2
- Split binding documentation, suggested by Rob Herring and Jacopo Mondi
- Remove ov965x_gpio_set() helper and open-code it, suggested by Jacopo Mondi
  and Sakari Ailus
- Call clk_prepare_enable() in s_power callback instead of probe, suggested
  by Sakari Ailus
- Unify clk and gpio configuration in a single if-else block and, also add
  a check either platform data or fwnode is actually specified, suggested
  by Jacopo Mondi
- Add CONFIG_OF guards, suggested by Jacopo Mondi

 drivers/media/i2c/ov9650.c | 130 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 38 deletions(-)

diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index 69433e1..99a3eab 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -11,8 +11,10 @@
  * 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/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/media.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;
@@ -513,24 +516,27 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
 	return 0;
 }
 
-static void ov965x_gpio_set(int gpio, int val)
-{
-	if (gpio_is_valid(gpio))
-		gpio_set_value(gpio, val);
-}
-
-static void __ov965x_set_power(struct ov965x *ov965x, int on)
+static int __ov965x_set_power(struct ov965x *ov965x, int on)
 {
 	if (on) {
-		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
-		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
+		int ret = clk_prepare_enable(ov965x->clk);
+
+		if (ret)
+			return ret;
+
+		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 0);
+		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 0);
 		msleep(25);
 	} else {
-		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
-		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
+		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 1);
+		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 1);
+
+		clk_disable_unprepare(ov965x->clk);
 	}
 
 	ov965x->streaming = 0;
+
+	return 0;
 }
 
 static int ov965x_s_power(struct v4l2_subdev *sd, int on)
@@ -543,8 +549,8 @@ static int ov965x_s_power(struct v4l2_subdev *sd, int on)
 
 	mutex_lock(&ov965x->lock);
 	if (ov965x->power == !on) {
-		__ov965x_set_power(ov965x, on);
-		if (on) {
+		ret = __ov965x_set_power(ov965x, on);
+		if (!ret && on) {
 			ret = ov965x_write_array(client,
 						 ov965x_init_regs);
 			ov965x->apply_frame_fmt = 1;
@@ -1408,16 +1414,17 @@ static const struct v4l2_subdev_ops ov965x_subdev_ops = {
 /*
  * Reset and power down GPIOs configuration
  */
-static int ov965x_configure_gpios(struct ov965x *ov965x,
-				  const struct ov9650_platform_data *pdata)
+static int ov965x_configure_gpios_pdata(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];
+		int gpio = gpios[i];
 
 		if (!gpio_is_valid(gpio))
 			continue;
@@ -1427,9 +1434,30 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
 			return ret;
 		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
 
-		gpio_set_value(gpio, 1);
+		gpio_set_value_cansleep(gpio, 1);
 		gpio_export(gpio, 0);
-		ov965x->gpios[i] = gpio;
+		ov965x->gpios[i] = gpio_to_desc(gpio);
+	}
+
+	return 0;
+}
+
+static int ov965x_configure_gpios(struct ov965x *ov965x)
+{
+	struct device *dev = &ov965x->client->dev;
+
+	ov965x->gpios[GPIO_PWDN] = devm_gpiod_get_optional(dev, "powerdown",
+							GPIOD_OUT_HIGH);
+	if (IS_ERR(ov965x->gpios[GPIO_PWDN])) {
+		dev_info(dev, "can't get %s GPIO\n", "powerdown");
+		return PTR_ERR(ov965x->gpios[GPIO_PWDN]);
+	}
+
+	ov965x->gpios[GPIO_RST] = devm_gpiod_get_optional(dev, "reset",
+							GPIOD_OUT_HIGH);
+	if (IS_ERR(ov965x->gpios[GPIO_RST])) {
+		dev_info(dev, "can't get %s GPIO\n", "reset");
+		return PTR_ERR(ov965x->gpios[GPIO_RST]);
 	}
 
 	return 0;
@@ -1443,7 +1471,10 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
 	int ret;
 
 	mutex_lock(&ov965x->lock);
-	__ov965x_set_power(ov965x, 1);
+	ret = __ov965x_set_power(ov965x, 1);
+	if (ret)
+		goto out;
+
 	msleep(25);
 
 	/* Check sensor revision */
@@ -1463,6 +1494,7 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
 			ret = -ENODEV;
 		}
 	}
+out:
 	mutex_unlock(&ov965x->lock);
 
 	return ret;
@@ -1476,23 +1508,39 @@ static int ov965x_probe(struct i2c_client *client,
 	struct ov965x *ov965x;
 	int ret;
 
-	if (!pdata) {
-		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");
-		return -EINVAL;
-	}
-
 	ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
 	if (!ov965x)
 		return -ENOMEM;
 
-	mutex_init(&ov965x->lock);
 	ov965x->client = client;
-	ov965x->mclk_frequency = pdata->mclk_frequency;
+
+	if (pdata) {
+		if (pdata->mclk_frequency == 0) {
+			dev_err(&client->dev, "MCLK frequency not specified\n");
+			return -EINVAL;
+		}
+		ov965x->mclk_frequency = pdata->mclk_frequency;
+
+		ret = ov965x_configure_gpios_pdata(ov965x, pdata);
+		if (ret < 0)
+			return ret;
+	} else if (dev_fwnode(&client->dev)) {
+		ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL);
+		if (IS_ERR(ov965x->clk))
+			return PTR_ERR(ov965x->clk);
+		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
+
+		ret = ov965x_configure_gpios(ov965x);
+		if (ret < 0)
+			return ret;
+	} else {
+		dev_err(&client->dev,
+			"Neither platform data nor device property specified\n");
+
+		return -EINVAL;
+	}
+
+	mutex_init(&ov965x->lock);
 
 	sd = &ov965x->sd;
 	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
@@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client,
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 		     V4L2_SUBDEV_FL_HAS_EVENTS;
 
-	ret = ov965x_configure_gpios(ov965x, pdata);
-	if (ret < 0)
-		goto err_mutex;
-
 	ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
 	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad);
@@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ov965x_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov965x_of_match[] = {
+	{ .compatible = "ovti,ov9650", },
+	{ .compatible = "ovti,ov9652", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov965x_of_match);
+#endif
+
 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,
-- 
2.7.4

[PATCH v2 2/2] media: ov9650: add device tree binding

[Akinobu Mita]

Now the ov9650 driver supports device tree probing.  So this adds a
device tree binding documentation.

Cc: Jacopo Mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
Cc: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
Cc: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
Cc: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Mauro Carvalho Chehab <mchehab-JsYNTwtnfakRB7SZvlqPiA@public.gmane.org>
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
* Changelog v2
- Split binding documentation, suggested by Rob Herring and Jacopo Mondi
- Improve the wording for compatible property in the binding documentation,
  suggested by Jacopo Mondi
- Improve the description for the device node in the binding documentation,
  suggested by Sakari Ailus

 .../devicetree/bindings/media/i2c/ov9650.txt       | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov9650.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov9650.txt b/Documentation/devicetree/bindings/media/i2c/ov9650.txt
new file mode 100644
index 0000000..506dfc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov9650.txt
@@ -0,0 +1,36 @@
+* Omnivision OV9650/OV9652 CMOS sensor
+
+Required Properties:
+- compatible: shall be one of
+	"ovti,ov9650"
+	"ovti,ov9652"
+- clocks: reference to the xvclk input clock.
+
+Optional Properties:
+- reset-gpios: reference to the GPIO connected to the resetb pin, if any.
+  Active is high.
+- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
+  Active is high.
+
+The device node shall contain one 'port' child node with one child 'endpoint'
+subnode for its digital output video port, in accordance with the video
+interface bindings defined in Documentation/devicetree/bindings/media/
+video-interfaces.txt.
+
+Example:
+
+&i2c0 {
+	ov9650: camera@30 {
+		compatible = "ovti,ov9650";
+		reg = <0x30>;
+		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_HIGH>;
+		powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_HIGH>;
+		clocks = <&xclk>;
+
+		port {
+			ov9650_0: endpoint {
+				remote-endpoint = <&vcap1_in0>;
+			};
+		};
+	};
+};
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[jacopo mondi]

Hi Akinobu,

   just one small comments below

On Mon, Jan 08, 2018 at 01:54:23AM +0900, Akinobu Mita wrote:
> The ov9650 driver currently only supports legacy platform data probe.
> This change adds device tree probing.
>
> There has been an attempt to add device tree support for ov9650 driver
> by Hugues Fruchet as a part of the patchset that adds support of OV9655
> camera (http://www.spinics.net/lists/linux-media/msg117903.html), but
> it wasn't merged into mainline because creating a separate driver for
> OV9655 is preferred.
>
> This is very similar to Hugues's patch, but not supporting new device.
>
> Cc: Jacopo Mondi <jacopo@jmondi.org>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: Hugues Fruchet <hugues.fruchet@st.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * Changelog v2
> - Split binding documentation, suggested by Rob Herring and Jacopo Mondi
> - Remove ov965x_gpio_set() helper and open-code it, suggested by Jacopo Mondi
>   and Sakari Ailus
> - Call clk_prepare_enable() in s_power callback instead of probe, suggested
>   by Sakari Ailus
> - Unify clk and gpio configuration in a single if-else block and, also add
>   a check either platform data or fwnode is actually specified, suggested
>   by Jacopo Mondi
> - Add CONFIG_OF guards, suggested by Jacopo Mondi
>
>  drivers/media/i2c/ov9650.c | 130 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 92 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 69433e1..99a3eab 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c
> @@ -11,8 +11,10 @@
>   * 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/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/media.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;
> @@ -513,24 +516,27 @@ static int ov965x_set_color_matrix(struct ov965x *ov965x)
>  	return 0;
>  }
>
> -static void ov965x_gpio_set(int gpio, int val)
> -{
> -	if (gpio_is_valid(gpio))
> -		gpio_set_value(gpio, val);
> -}
> -
> -static void __ov965x_set_power(struct ov965x *ov965x, int on)
> +static int __ov965x_set_power(struct ov965x *ov965x, int on)
>  {
>  	if (on) {
> -		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
> -		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
> +		int ret = clk_prepare_enable(ov965x->clk);
> +
> +		if (ret)
> +			return ret;
> +
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 0);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 0);
>  		msleep(25);
>  	} else {
> -		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
> -		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 1);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 1);
> +
> +		clk_disable_unprepare(ov965x->clk);
>  	}
>
>  	ov965x->streaming = 0;
> +
> +	return 0;
>  }
>
>  static int ov965x_s_power(struct v4l2_subdev *sd, int on)
> @@ -543,8 +549,8 @@ static int ov965x_s_power(struct v4l2_subdev *sd, int on)
>
>  	mutex_lock(&ov965x->lock);
>  	if (ov965x->power == !on) {
> -		__ov965x_set_power(ov965x, on);
> -		if (on) {
> +		ret = __ov965x_set_power(ov965x, on);
> +		if (!ret && on) {
>  			ret = ov965x_write_array(client,
>  						 ov965x_init_regs);
>  			ov965x->apply_frame_fmt = 1;
> @@ -1408,16 +1414,17 @@ static const struct v4l2_subdev_ops ov965x_subdev_ops = {
>  /*
>   * Reset and power down GPIOs configuration
>   */
> -static int ov965x_configure_gpios(struct ov965x *ov965x,
> -				  const struct ov9650_platform_data *pdata)
> +static int ov965x_configure_gpios_pdata(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];
> +		int gpio = gpios[i];
>
>  		if (!gpio_is_valid(gpio))
>  			continue;
> @@ -1427,9 +1434,30 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>  			return ret;
>  		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>
> -		gpio_set_value(gpio, 1);
> +		gpio_set_value_cansleep(gpio, 1);
>  		gpio_export(gpio, 0);
> -		ov965x->gpios[i] = gpio;
> +		ov965x->gpios[i] = gpio_to_desc(gpio);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov965x_configure_gpios(struct ov965x *ov965x)
> +{
> +	struct device *dev = &ov965x->client->dev;
> +
> +	ov965x->gpios[GPIO_PWDN] = devm_gpiod_get_optional(dev, "powerdown",
> +							GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov965x->gpios[GPIO_PWDN])) {
> +		dev_info(dev, "can't get %s GPIO\n", "powerdown");
> +		return PTR_ERR(ov965x->gpios[GPIO_PWDN]);
> +	}
> +
> +	ov965x->gpios[GPIO_RST] = devm_gpiod_get_optional(dev, "reset",
> +							GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov965x->gpios[GPIO_RST])) {
> +		dev_info(dev, "can't get %s GPIO\n", "reset");
> +		return PTR_ERR(ov965x->gpios[GPIO_RST]);
>  	}
>
>  	return 0;
> @@ -1443,7 +1471,10 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>  	int ret;
>
>  	mutex_lock(&ov965x->lock);
> -	__ov965x_set_power(ov965x, 1);
> +	ret = __ov965x_set_power(ov965x, 1);
> +	if (ret)
> +		goto out;
> +
>  	msleep(25);
>
>  	/* Check sensor revision */
> @@ -1463,6 +1494,7 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>  			ret = -ENODEV;
>  		}
>  	}
> +out:
>  	mutex_unlock(&ov965x->lock);
>
>  	return ret;
> @@ -1476,23 +1508,39 @@ static int ov965x_probe(struct i2c_client *client,
>  	struct ov965x *ov965x;
>  	int ret;
>
> -	if (!pdata) {
> -		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");
> -		return -EINVAL;
> -	}
> -
>  	ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
>  	if (!ov965x)
>  		return -ENOMEM;
>
> -	mutex_init(&ov965x->lock);
>  	ov965x->client = client;
> -	ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> +	if (pdata) {
> +		if (pdata->mclk_frequency == 0) {
> +			dev_err(&client->dev, "MCLK frequency not specified\n");
> +			return -EINVAL;
> +		}
> +		ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> +		ret = ov965x_configure_gpios_pdata(ov965x, pdata);
> +		if (ret < 0)
> +			return ret;
> +	} else if (dev_fwnode(&client->dev)) {
> +		ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL);
> +		if (IS_ERR(ov965x->clk))
> +			return PTR_ERR(ov965x->clk);
> +		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
> +
> +		ret = ov965x_configure_gpios(ov965x);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		dev_err(&client->dev,
> +			"Neither platform data nor device property specified\n");
> +
> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&ov965x->lock);
>
>  	sd = &ov965x->sd;
>  	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
> @@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client,
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  		     V4L2_SUBDEV_FL_HAS_EVENTS;
>
> -	ret = ov965x_configure_gpios(ov965x, pdata);
> -	if (ret < 0)
> -		goto err_mutex;
> -
>  	ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad);
> @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov965x_of_match[] = {
> +	{ .compatible = "ovti,ov9650", },
> +	{ .compatible = "ovti,ov9652", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov965x_of_match);
> +#endif
> +
>  static struct i2c_driver ov965x_i2c_driver = {
>  	.driver = {
>  		.name	= DRIVER_NAME,
> +		.of_match_table = of_match_ptr(ov965x_of_match),

If CONFIG_OF is not defined, this will break compilation.
Please guard this with #if IS_ENABLED(CONFIG_OF) as well.

Apart from this:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  	},
>  	.probe		= ov965x_probe,
>  	.remove		= ov965x_remove,
> --
> 2.7.4
>

Re: [PATCH v2 2/2] media: ov9650: add device tree binding

[Sakari Ailus]

Hi Akinobu,

Thanks for the update.

On Mon, Jan 08, 2018 at 01:54:24AM +0900, Akinobu Mita wrote:
> Now the ov9650 driver supports device tree probing.  So this adds a
> device tree binding documentation.
> 
> Cc: Jacopo Mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> Cc: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Cc: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
> Cc: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Mauro Carvalho Chehab <mchehab-JsYNTwtnfakRB7SZvlqPiA@public.gmane.org>
> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> * Changelog v2
> - Split binding documentation, suggested by Rob Herring and Jacopo Mondi
> - Improve the wording for compatible property in the binding documentation,
>   suggested by Jacopo Mondi
> - Improve the description for the device node in the binding documentation,
>   suggested by Sakari Ailus
> 
>  .../devicetree/bindings/media/i2c/ov9650.txt       | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov9650.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov9650.txt b/Documentation/devicetree/bindings/media/i2c/ov9650.txt
> new file mode 100644
> index 0000000..506dfc5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov9650.txt
> @@ -0,0 +1,36 @@
> +* Omnivision OV9650/OV9652 CMOS sensor
> +
> +Required Properties:
> +- compatible: shall be one of
> +	"ovti,ov9650"
> +	"ovti,ov9652"
> +- clocks: reference to the xvclk input clock.
> +
> +Optional Properties:
> +- reset-gpios: reference to the GPIO connected to the resetb pin, if any.
> +  Active is high.
> +- powerdown-gpios: reference to the GPIO connected to the pwdn pin, if any.
> +  Active is high.
> +
> +The device node shall contain one 'port' child node with one child 'endpoint'
> +subnode for its digital output video port, in accordance with the video
> +interface bindings defined in Documentation/devicetree/bindings/media/
> +video-interfaces.txt.
> +
> +Example:
> +
> +&i2c0 {
> +	ov9650: camera@30 {
> +		compatible = "ovti,ov9650";
> +		reg = <0x30>;
> +		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_HIGH>;
> +		powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_HIGH>;
> +		clocks = <&xclk>;
> +
> +		port {
> +			ov9650_0: endpoint {
> +				remote-endpoint = <&vcap1_in0>;
> +			};
> +		};
> +	};
> +};

I was going to say you're missing the MAINTAINERS entry for this newly
added file but then I noticed that the entire driver is missing an entry.
Still this file should have a MAINTAINERS entry added for it independently
of that, in the same patch.

Cc Sylwester.

-- 
Kind regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[Sakari Ailus]

Hi Jacopo,

On Mon, Jan 08, 2018 at 10:18:38AM +0100, jacopo mondi wrote:
> > @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, ov965x_id);
> >
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id ov965x_of_match[] = {
> > +	{ .compatible = "ovti,ov9650", },
> > +	{ .compatible = "ovti,ov9652", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov965x_of_match);
> > +#endif
> > +
> >  static struct i2c_driver ov965x_i2c_driver = {
> >  	.driver = {
> >  		.name	= DRIVER_NAME,
> > +		.of_match_table = of_match_ptr(ov965x_of_match),
> 
> If CONFIG_OF is not defined, this will break compilation.
> Please guard this with #if IS_ENABLED(CONFIG_OF) as well.

of_match_ptr() will be NULL if CONFIG_OF is not defined, so AFAICT this is
fine.

-- 
Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[jacopo mondi]

Hi Sakari,

On Mon, Jan 08, 2018 at 11:37:12AM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Jan 08, 2018 at 10:18:38AM +0100, jacopo mondi wrote:
> > > @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, ov965x_id);
> > >
> > > +#if IS_ENABLED(CONFIG_OF)
> > > +static const struct of_device_id ov965x_of_match[] = {
> > > +	{ .compatible = "ovti,ov9650", },
> > > +	{ .compatible = "ovti,ov9652", },
> > > +	{ /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, ov965x_of_match);
> > > +#endif
> > > +
> > >  static struct i2c_driver ov965x_i2c_driver = {
> > >  	.driver = {
> > >  		.name	= DRIVER_NAME,
> > > +		.of_match_table = of_match_ptr(ov965x_of_match),
> >
> > If CONFIG_OF is not defined, this will break compilation.
> > Please guard this with #if IS_ENABLED(CONFIG_OF) as well.
>
> of_match_ptr() will be NULL if CONFIG_OF is not defined, so AFAICT this is
> fine.
>

That's even better. Thanks for pointing this out!
Please ignore my comment then!

Cheers
   j

> --
> Sakari Ailus
> sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] media: ov9650: add device tree binding

[Rob Herring]

On Mon, Jan 08, 2018 at 01:54:24AM +0900, Akinobu Mita wrote:
> Now the ov9650 driver supports device tree probing.  So this adds a
> device tree binding documentation.
> 
> Cc: Jacopo Mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> Cc: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> Cc: Hugues Fruchet <hugues.fruchet-qxv4g6HH51o@public.gmane.org>
> Cc: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Mauro Carvalho Chehab <mchehab-JsYNTwtnfakRB7SZvlqPiA@public.gmane.org>
> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> * Changelog v2
> - Split binding documentation, suggested by Rob Herring and Jacopo Mondi
> - Improve the wording for compatible property in the binding documentation,
>   suggested by Jacopo Mondi
> - Improve the description for the device node in the binding documentation,
>   suggested by Sakari Ailus
> 
>  .../devicetree/bindings/media/i2c/ov9650.txt       | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov9650.txt

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[Sylwester Nawrocki]

On 01/07/2018 05:54 PM, Akinobu Mita wrote:
> The ov9650 driver currently only supports legacy platform data probe.
> This change adds device tree probing.

> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

>  drivers/media/i2c/ov9650.c | 130 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 92 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
> index 69433e1..99a3eab 100644
> --- a/drivers/media/i2c/ov9650.c
> +++ b/drivers/media/i2c/ov9650.c

> -static void __ov965x_set_power(struct ov965x *ov965x, int on)
> +static int __ov965x_set_power(struct ov965x *ov965x, int on)
>  {
>  	if (on) {
> -		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
> -		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
> +		int ret = clk_prepare_enable(ov965x->clk);

It seems you rely on the fact clk_prepare_enable() is a nop when passed
argument is NULL, which happens in non-DT case.

> +		if (ret)
> +			return ret;
> +
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 0);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 0);
>  		msleep(25);
>  	} else {
> -		ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
> -		ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 1);
> +		gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 1);
> +
> +		clk_disable_unprepare(ov965x->clk);
>  	}
>  
>  	ov965x->streaming = 0;
> +
> +	return 0;
>  }

> @@ -1408,16 +1414,17 @@ static const struct v4l2_subdev_ops ov965x_subdev_ops = {
>  /*
>   * Reset and power down GPIOs configuration
>   */
> -static int ov965x_configure_gpios(struct ov965x *ov965x,
> -				  const struct ov9650_platform_data *pdata)
> +static int ov965x_configure_gpios_pdata(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];
> +		int gpio = gpios[i];
>  
>  		if (!gpio_is_valid(gpio))
>  			continue;
> @@ -1427,9 +1434,30 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>  			return ret;
>  		v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>  
> -		gpio_set_value(gpio, 1);
> +		gpio_set_value_cansleep(gpio, 1);
>  		gpio_export(gpio, 0);
> -		ov965x->gpios[i] = gpio;
> +		ov965x->gpios[i] = gpio_to_desc(gpio);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov965x_configure_gpios(struct ov965x *ov965x)
> +{
> +	struct device *dev = &ov965x->client->dev;
> +
> +	ov965x->gpios[GPIO_PWDN] = devm_gpiod_get_optional(dev, "powerdown",
> +							GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov965x->gpios[GPIO_PWDN])) {
> +		dev_info(dev, "can't get %s GPIO\n", "powerdown");
> +		return PTR_ERR(ov965x->gpios[GPIO_PWDN]);
> +	}
> +
> +	ov965x->gpios[GPIO_RST] = devm_gpiod_get_optional(dev, "reset",
> +							GPIOD_OUT_HIGH);
> +	if (IS_ERR(ov965x->gpios[GPIO_RST])) {
> +		dev_info(dev, "can't get %s GPIO\n", "reset");
> +		return PTR_ERR(ov965x->gpios[GPIO_RST]);
>  	}
>  
>  	return 0;
> @@ -1443,7 +1471,10 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>  	int ret;
>  
>  	mutex_lock(&ov965x->lock);
> -	__ov965x_set_power(ov965x, 1);
> +	ret = __ov965x_set_power(ov965x, 1);
> +	if (ret)
> +		goto out;
> +
>  	msleep(25);
>  
>  	/* Check sensor revision */
> @@ -1463,6 +1494,7 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>  			ret = -ENODEV;
>  		}
>  	}
> +out:
>  	mutex_unlock(&ov965x->lock);
>  
>  	return ret;
> @@ -1476,23 +1508,39 @@ static int ov965x_probe(struct i2c_client *client,
>  	struct ov965x *ov965x;
>  	int ret;
>  
> -	if (!pdata) {
> -		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");
> -		return -EINVAL;
> -	}
> -
>  	ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
>  	if (!ov965x)
>  		return -ENOMEM;
>  
> -	mutex_init(&ov965x->lock);

I would leave mutex initialization as first thing after the private data
structure allocation, is there a need to move it further?

>  	ov965x->client = client;
> -	ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> +	if (pdata) {
> +		if (pdata->mclk_frequency == 0) {
> +			dev_err(&client->dev, "MCLK frequency not specified\n");
> +			return -EINVAL;
> +		}
> +		ov965x->mclk_frequency = pdata->mclk_frequency;
> +
> +		ret = ov965x_configure_gpios_pdata(ov965x, pdata);
> +		if (ret < 0)
> +			return ret;
> +	} else if (dev_fwnode(&client->dev)) {
> +		ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL);
> +		if (IS_ERR(ov965x->clk))
> +			return PTR_ERR(ov965x->clk);
> +		ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
> +
> +		ret = ov965x_configure_gpios(ov965x);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		dev_err(&client->dev,
> +			"Neither platform data nor device property specified\n");
> +
> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&ov965x->lock);
>  
>  	sd = &ov965x->sd;
>  	v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
> @@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client,
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  		     V4L2_SUBDEV_FL_HAS_EVENTS;
>  
> -	ret = ov965x_configure_gpios(ov965x, pdata);
> -	if (ret < 0)
> -		goto err_mutex;
> -
>  	ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad);
> @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>  
> +#if IS_ENABLED(CONFIG_OF)

Is there any advantage in using IS_ENABLED() rather than just
#ifdef CONFIG_OF ? of_match_ptr() is defined with just #idef CONFIG_OF/
#else/#endif. I would use simply #ifdef CONFIG_OF here.

Otherwise looks good.

Reviewed-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

-- 
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] media: ov9650: add device tree binding

[Sylwester Nawrocki]

On 01/08/2018 10:35 AM, Sakari Ailus wrote:
 
> I was going to say you're missing the MAINTAINERS entry for this newly
> added file but then I noticed that the entire driver is missing an entry.
> Still this file should have a MAINTAINERS entry added for it independently
> of that, in the same patch.
> 
> Cc Sylwester.

I don't the hardware and I can't test the patches so Mita-san if you wish
so please add yourself as a maintainer of whole driver.

-- 
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] media: ov9650: support device tree probing

[Akinobu Mita]

2018-01-12 23:14 GMT+09:00 Sylwester Nawrocki <s.nawrocki@samsung.com>:
> On 01/07/2018 05:54 PM, Akinobu Mita wrote:
>> The ov9650 driver currently only supports legacy platform data probe.
>> This change adds device tree probing.
>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>
>>  drivers/media/i2c/ov9650.c | 130 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 92 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
>> index 69433e1..99a3eab 100644
>> --- a/drivers/media/i2c/ov9650.c
>> +++ b/drivers/media/i2c/ov9650.c
>
>> -static void __ov965x_set_power(struct ov965x *ov965x, int on)
>> +static int __ov965x_set_power(struct ov965x *ov965x, int on)
>>  {
>>       if (on) {
>> -             ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 0);
>> -             ov965x_gpio_set(ov965x->gpios[GPIO_RST], 0);
>> +             int ret = clk_prepare_enable(ov965x->clk);
>
> It seems you rely on the fact clk_prepare_enable() is a nop when passed
> argument is NULL, which happens in non-DT case.

So this works fine as before in non-DT case, doesn't it?

>> +             if (ret)
>> +                     return ret;
>> +
>> +             gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 0);
>> +             gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 0);
>>               msleep(25);
>>       } else {
>> -             ov965x_gpio_set(ov965x->gpios[GPIO_RST], 1);
>> -             ov965x_gpio_set(ov965x->gpios[GPIO_PWDN], 1);
>> +             gpiod_set_value_cansleep(ov965x->gpios[GPIO_RST], 1);
>> +             gpiod_set_value_cansleep(ov965x->gpios[GPIO_PWDN], 1);
>> +
>> +             clk_disable_unprepare(ov965x->clk);
>>       }
>>
>>       ov965x->streaming = 0;
>> +
>> +     return 0;
>>  }
>
>> @@ -1408,16 +1414,17 @@ static const struct v4l2_subdev_ops ov965x_subdev_ops = {
>>  /*
>>   * Reset and power down GPIOs configuration
>>   */
>> -static int ov965x_configure_gpios(struct ov965x *ov965x,
>> -                               const struct ov9650_platform_data *pdata)
>> +static int ov965x_configure_gpios_pdata(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];
>> +             int gpio = gpios[i];
>>
>>               if (!gpio_is_valid(gpio))
>>                       continue;
>> @@ -1427,9 +1434,30 @@ static int ov965x_configure_gpios(struct ov965x *ov965x,
>>                       return ret;
>>               v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio);
>>
>> -             gpio_set_value(gpio, 1);
>> +             gpio_set_value_cansleep(gpio, 1);
>>               gpio_export(gpio, 0);
>> -             ov965x->gpios[i] = gpio;
>> +             ov965x->gpios[i] = gpio_to_desc(gpio);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int ov965x_configure_gpios(struct ov965x *ov965x)
>> +{
>> +     struct device *dev = &ov965x->client->dev;
>> +
>> +     ov965x->gpios[GPIO_PWDN] = devm_gpiod_get_optional(dev, "powerdown",
>> +                                                     GPIOD_OUT_HIGH);
>> +     if (IS_ERR(ov965x->gpios[GPIO_PWDN])) {
>> +             dev_info(dev, "can't get %s GPIO\n", "powerdown");
>> +             return PTR_ERR(ov965x->gpios[GPIO_PWDN]);
>> +     }
>> +
>> +     ov965x->gpios[GPIO_RST] = devm_gpiod_get_optional(dev, "reset",
>> +                                                     GPIOD_OUT_HIGH);
>> +     if (IS_ERR(ov965x->gpios[GPIO_RST])) {
>> +             dev_info(dev, "can't get %s GPIO\n", "reset");
>> +             return PTR_ERR(ov965x->gpios[GPIO_RST]);
>>       }
>>
>>       return 0;
>> @@ -1443,7 +1471,10 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>>       int ret;
>>
>>       mutex_lock(&ov965x->lock);
>> -     __ov965x_set_power(ov965x, 1);
>> +     ret = __ov965x_set_power(ov965x, 1);
>> +     if (ret)
>> +             goto out;
>> +
>>       msleep(25);
>>
>>       /* Check sensor revision */
>> @@ -1463,6 +1494,7 @@ static int ov965x_detect_sensor(struct v4l2_subdev *sd)
>>                       ret = -ENODEV;
>>               }
>>       }
>> +out:
>>       mutex_unlock(&ov965x->lock);
>>
>>       return ret;
>> @@ -1476,23 +1508,39 @@ static int ov965x_probe(struct i2c_client *client,
>>       struct ov965x *ov965x;
>>       int ret;
>>
>> -     if (!pdata) {
>> -             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");
>> -             return -EINVAL;
>> -     }
>> -
>>       ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL);
>>       if (!ov965x)
>>               return -ENOMEM;
>>
>> -     mutex_init(&ov965x->lock);
>
> I would leave mutex initialization as first thing after the private data
> structure allocation, is there a need to move it further?

Yes, there is.  This enables to reduce several lines in clk and gpio
initialization.

For example,

        if (IS_ERR(ov965x->clk)) {
                ret = PTR_ERR(ov965x->clk);
                goto err_mutex;
        }

vs.
        if (IS_ERR(ov965x->clk))
                return PTR_ERR(ov965x->clk);

>>       ov965x->client = client;
>> -     ov965x->mclk_frequency = pdata->mclk_frequency;
>> +
>> +     if (pdata) {
>> +             if (pdata->mclk_frequency == 0) {
>> +                     dev_err(&client->dev, "MCLK frequency not specified\n");
>> +                     return -EINVAL;
>> +             }
>> +             ov965x->mclk_frequency = pdata->mclk_frequency;
>> +
>> +             ret = ov965x_configure_gpios_pdata(ov965x, pdata);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else if (dev_fwnode(&client->dev)) {
>> +             ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL);
>> +             if (IS_ERR(ov965x->clk))
>> +                     return PTR_ERR(ov965x->clk);
>> +             ov965x->mclk_frequency = clk_get_rate(ov965x->clk);
>> +
>> +             ret = ov965x_configure_gpios(ov965x);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>> +             dev_err(&client->dev,
>> +                     "Neither platform data nor device property specified\n");
>> +
>> +             return -EINVAL;
>> +     }
>> +
>> +     mutex_init(&ov965x->lock);
>>
>>       sd = &ov965x->sd;
>>       v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops);
>> @@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client,
>>       sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>>                    V4L2_SUBDEV_FL_HAS_EVENTS;
>>
>> -     ret = ov965x_configure_gpios(ov965x, pdata);
>> -     if (ret < 0)
>> -             goto err_mutex;
>> -
>>       ov965x->pad.flags = MEDIA_PAD_FL_SOURCE;
>>       sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>>       ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad);
>> @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>>
>> +#if IS_ENABLED(CONFIG_OF)
>
> Is there any advantage in using IS_ENABLED() rather than just
> #ifdef CONFIG_OF ? of_match_ptr() is defined with just #idef CONFIG_OF/
> #else/#endif. I would use simply #ifdef CONFIG_OF here.

There is no difference between these in this context.

I think it doesn't look too strange because ov2659, ov5647, and ov7670
also do like this.

> Otherwise looks good.
>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Thanks for your review.

Re: [PATCH v2 2/2] media: ov9650: add device tree binding

[Akinobu Mita]

2018-01-12 23:15 GMT+09:00 Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
> On 01/08/2018 10:35 AM, Sakari Ailus wrote:
>
>> I was going to say you're missing the MAINTAINERS entry for this newly
>> added file but then I noticed that the entire driver is missing an entry.
>> Still this file should have a MAINTAINERS entry added for it independently
>> of that, in the same patch.
>>
>> Cc Sylwester.
>
> I don't the hardware and I can't test the patches so Mita-san if you wish
> so please add yourself as a maintainer of whole driver.

Even if you don't have the hardware, you can help reviwing.  So if you
don't mind, I would like to add the following maintainer entry for this
driver.

OMNIVISION OV9650 SENSOR DRIVER
M:      Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
R:      Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
R:      Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
L:      linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
T:      git git://linuxtv.org/media_tree.git
S:      Maintained
F:      drivers/media/i2c/ov9650.c
F:      Documentation/devicetree/bindings/media/i2c/ov9650.txt
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html