Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-27 Thread Hugues FRUCHET 


On 06/27/2017 07:36 AM, Sakari Ailus wrote:
> On Mon, Jun 26, 2017 at 07:46:34PM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>> Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
>>>
>>> Hi Hugues,
>>>
>>> On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
 @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
 }

 static const struct i2c_device_id ov965x_id[] = {
 -  { "OV9650", 0 },
 -  { "OV9652", 0 },
 +  { "OV9650", 0x9650 },
 +  { "OV9652", 0x9652 },
>>>
>>> This change does not appear to match with the patch description nor it the
>>> information is used. How about not changing it, unless there's a reason to?
>>> The same for the data field of the of_device_id array below.
>>
>> I think it could/should be used to check if the camera chip that is found
>> by reading the product-id and version registers does match what the device
>> tree expects and abort probing on a mismatch.
> 
> Makes sense. But it should be a separate patch, shouldn't it?
> 
> You could also put the id to the ops struct, and choose the ops struct that
> way. Entirely up to you.
> 

I'll suggest to skip the id check between DT compatible string and real 
device id read from sensor, this is not something I see in other drivers 
currently.
But I would suggest to keep in a separate patch the switch of device id 
names to lower case in order to align with other omnivision cameras and 
not introduce upper/lower case potential bugs in DT later on (as the one 
encountered by Nikolaus):

  [media] ov9650: switch i2c device id to lower case

  static const struct i2c_device_id ov965x_id[] = {
-   { "OV9650", 0 },
-   { "OV9652", 0 },
+   { "ov9650", 0 },
+   { "ov9652", 0 },


  [media] ov9650: add device tree support

+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),

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-27 Thread Hugues FRUCHET 


On 06/27/2017 07:36 AM, Sakari Ailus wrote:
> On Mon, Jun 26, 2017 at 07:46:34PM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>> Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
>>>
>>> Hi Hugues,
>>>
>>> On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
 @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
 }

 static const struct i2c_device_id ov965x_id[] = {
 -  { "OV9650", 0 },
 -  { "OV9652", 0 },
 +  { "OV9650", 0x9650 },
 +  { "OV9652", 0x9652 },
>>>
>>> This change does not appear to match with the patch description nor it the
>>> information is used. How about not changing it, unless there's a reason to?
>>> The same for the data field of the of_device_id array below.
>>
>> I think it could/should be used to check if the camera chip that is found
>> by reading the product-id and version registers does match what the device
>> tree expects and abort probing on a mismatch.
> 
> Makes sense. But it should be a separate patch, shouldn't it?
> 
> You could also put the id to the ops struct, and choose the ops struct that
> way. Entirely up to you.
> 

I'll suggest to skip the id check between DT compatible string and real 
device id read from sensor, this is not something I see in other drivers 
currently.
But I would suggest to keep in a separate patch the switch of device id 
names to lower case in order to align with other omnivision cameras and 
not introduce upper/lower case potential bugs in DT later on (as the one 
encountered by Nikolaus):

  [media] ov9650: switch i2c device id to lower case

  static const struct i2c_device_id ov965x_id[] = {
-   { "OV9650", 0 },
-   { "OV9652", 0 },
+   { "ov9650", 0 },
+   { "ov9652", 0 },


  [media] ov9650: add device tree support

+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),

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread Sakari Ailus 
On Mon, Jun 26, 2017 at 07:46:34PM +0200, H. Nikolaus Schaller wrote:
> Hi,
> 
> > Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
> > 
> > Hi Hugues,
> > 
> > On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
> >> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
> >> }
> >> 
> >> static const struct i2c_device_id ov965x_id[] = {
> >> -  { "OV9650", 0 },
> >> -  { "OV9652", 0 },
> >> +  { "OV9650", 0x9650 },
> >> +  { "OV9652", 0x9652 },
> > 
> > This change does not appear to match with the patch description nor it the
> > information is used. How about not changing it, unless there's a reason to?
> > The same for the data field of the of_device_id array below.
> 
> I think it could/should be used to check if the camera chip that is found
> by reading the product-id and version registers does match what the device
> tree expects and abort probing on a mismatch.

Makes sense. But it should be a separate patch, shouldn't it?

You could also put the id to the ops struct, and choose the ops struct that
way. Entirely up to you.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread Sakari Ailus 
On Mon, Jun 26, 2017 at 07:46:34PM +0200, H. Nikolaus Schaller wrote:
> Hi,
> 
> > Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
> > 
> > Hi Hugues,
> > 
> > On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
> >> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
> >> }
> >> 
> >> static const struct i2c_device_id ov965x_id[] = {
> >> -  { "OV9650", 0 },
> >> -  { "OV9652", 0 },
> >> +  { "OV9650", 0x9650 },
> >> +  { "OV9652", 0x9652 },
> > 
> > This change does not appear to match with the patch description nor it the
> > information is used. How about not changing it, unless there's a reason to?
> > The same for the data field of the of_device_id array below.
> 
> I think it could/should be used to check if the camera chip that is found
> by reading the product-id and version registers does match what the device
> tree expects and abort probing on a mismatch.

Makes sense. But it should be a separate patch, shouldn't it?

You could also put the id to the ops struct, and choose the ops struct that
way. Entirely up to you.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread H. Nikolaus Schaller 
Hi,

> Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
> 
> Hi Hugues,
> 
> On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
>> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
>> }
>> 
>> static const struct i2c_device_id ov965x_id[] = {
>> -{ "OV9650", 0 },
>> -{ "OV9652", 0 },
>> +{ "OV9650", 0x9650 },
>> +{ "OV9652", 0x9652 },
> 
> This change does not appear to match with the patch description nor it the
> information is used. How about not changing it, unless there's a reason to?
> The same for the data field of the of_device_id array below.

I think it could/should be used to check if the camera chip that is found
by reading the product-id and version registers does match what the device
tree expects and abort probing on a mismatch.

BR,
Nikolaus

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread H. Nikolaus Schaller 
Hi,

> Am 26.06.2017 um 18:31 schrieb Sakari Ailus :
> 
> Hi Hugues,
> 
> On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
>> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
>> }
>> 
>> static const struct i2c_device_id ov965x_id[] = {
>> -{ "OV9650", 0 },
>> -{ "OV9652", 0 },
>> +{ "OV9650", 0x9650 },
>> +{ "OV9652", 0x9652 },
> 
> This change does not appear to match with the patch description nor it the
> information is used. How about not changing it, unless there's a reason to?
> The same for the data field of the of_device_id array below.

I think it could/should be used to check if the camera chip that is found
by reading the product-id and version registers does match what the device
tree expects and abort probing on a mismatch.

BR,
Nikolaus

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread Sakari Ailus 
Hi Hugues,

On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id ov965x_id[] = {
> - { "OV9650", 0 },
> - { "OV9652", 0 },
> + { "OV9650", 0x9650 },
> + { "OV9652", 0x9652 },

This change does not appear to match with the patch description nor it the
information is used. How about not changing it, unless there's a reason to?
The same for the data field of the of_device_id array below.

>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>  
> +static const struct of_device_id ov965x_of_match[] = {
> + { .compatible = "ovti,ov9650", .data = (void *)0x9650 },
> + { .compatible = "ovti,ov9652", .data = (void *)0x9652 },
> + { /* 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,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-26 Thread Sakari Ailus 
Hi Hugues,

On Thu, Jun 22, 2017 at 05:05:38PM +0200, Hugues Fruchet wrote:
> @@ -1545,15 +1577,22 @@ static int ov965x_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id ov965x_id[] = {
> - { "OV9650", 0 },
> - { "OV9652", 0 },
> + { "OV9650", 0x9650 },
> + { "OV9652", 0x9652 },

This change does not appear to match with the patch description nor it the
information is used. How about not changing it, unless there's a reason to?
The same for the data field of the of_device_id array below.

>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ov965x_id);
>  
> +static const struct of_device_id ov965x_of_match[] = {
> + { .compatible = "ovti,ov9650", .data = (void *)0x9650 },
> + { .compatible = "ovti,ov9652", .data = (void *)0x9652 },
> + { /* 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,

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

[PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-22 Thread Hugues Fruchet 
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 
Signed-off-by: Hugues Fruchet 
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ov9650.c | 81 ++
 2 files changed, 61 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c380e24..efea14d 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -595,7 +595,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 2de2fbb..8340a45 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 
 #include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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);
 }
 
 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(>client->dev, gpio,
-   GPIOF_OUT_INIT_HIGH, "OV965X");
-   if (ret < 0)
+   GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
+   if (ret < 0) {
+   dev_err(>client->dev,
+   "Failed to request gpio%d (%d)\n", gpio, ret);
return ret;
+   }
v4l2_dbg(1, debug, >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;
 
-   if (pdata == NULL) {
-   dev_err(>dev, "platform data not specified\n");
-   return -EINVAL;
-   }
-
-   if (pdata->mclk_frequency == 0) {
-   dev_err(>dev, "MCLK frequency not specified\n");
+   if (!pdata && !np) {
+   dev_err(>dev, "Platform data or device tree data must 
be provided\n");
return -EINVAL;
}
 
@@ -1486,7 +1489,36 @@ static int ov965x_probe(struct i2c_client *client,
 
mutex_init(>lock);
ov965x->client = client;
-   ov965x->mclk_frequency = pdata->mclk_frequency;
+   mutex_init(>lock);
+
+   if (np) {
+   /* Device tree */
+   ov965x->gpios[GPIO_RST] =
+   devm_gpiod_get_optional(>dev, "resetb",
+   GPIOD_OUT_LOW);
+   ov965x->gpios[GPIO_PWDN] =
+   devm_gpiod_get_optional(>dev, "pwdn",
+   GPIOD_OUT_HIGH);
+
+

[PATCH v1 2/6] [media] ov9650: add device tree support

2017-06-22 Thread Hugues Fruchet 
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 
Signed-off-by: Hugues Fruchet 
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ov9650.c | 81 ++
 2 files changed, 61 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index c380e24..efea14d 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -595,7 +595,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 2de2fbb..8340a45 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 
 #include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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);
 }
 
 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(>client->dev, gpio,
-   GPIOF_OUT_INIT_HIGH, "OV965X");
-   if (ret < 0)
+   GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
+   if (ret < 0) {
+   dev_err(>client->dev,
+   "Failed to request gpio%d (%d)\n", gpio, ret);
return ret;
+   }
v4l2_dbg(1, debug, >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;
 
-   if (pdata == NULL) {
-   dev_err(>dev, "platform data not specified\n");
-   return -EINVAL;
-   }
-
-   if (pdata->mclk_frequency == 0) {
-   dev_err(>dev, "MCLK frequency not specified\n");
+   if (!pdata && !np) {
+   dev_err(>dev, "Platform data or device tree data must 
be provided\n");
return -EINVAL;
}
 
@@ -1486,7 +1489,36 @@ static int ov965x_probe(struct i2c_client *client,
 
mutex_init(>lock);
ov965x->client = client;
-   ov965x->mclk_frequency = pdata->mclk_frequency;
+   mutex_init(>lock);
+
+   if (np) {
+   /* Device tree */
+   ov965x->gpios[GPIO_RST] =
+   devm_gpiod_get_optional(>dev, "resetb",
+   GPIOD_OUT_LOW);
+   ov965x->gpios[GPIO_PWDN] =
+   devm_gpiod_get_optional(>dev, "pwdn",
+   GPIOD_OUT_HIGH);
+
+   ov965x->clk = devm_clk_get(>dev, NULL);
+