Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-18 Thread Hugues FRUCHET 
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 
>> Signed-off-by: Hugues Fruchet 
>> ---
>>   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 
>>   #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);
> 
> 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(>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(>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;
> 
> It'd be nice to declare this next to pdata, rather than after ret and other
> short declarations.

done

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

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-18 Thread Hugues FRUCHET 
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 
>> Signed-off-by: Hugues Fruchet 
>> ---
>>   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 
>>   #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);
> 
> 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(>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(>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;
> 
> It'd be nice to declare this next to pdata, rather than after ret and other
> short declarations.

done

> 
>>   
>> -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

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-12 Thread Sylwester Nawrocki 
On 07/03/2017 11:16 AM, 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 
> Signed-off-by: Hugues Fruchet 
> ---
>   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 
>   #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,35 @@ static int ov965x_probe(struct i2c_client *client,
>   
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> + mutex_init(>lock);

Are you initializing the mutex twice?

> + if (np) {
> + /* Device tree */
> + ov965x->gpios[GPIO_RST] =
> + devm_gpiod_get_optional(>dev, "resetb",
> +

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-12 Thread Sylwester Nawrocki 
On 07/03/2017 11:16 AM, 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 
> Signed-off-by: Hugues Fruchet 
> ---
>   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 
>   #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,35 @@ static int ov965x_probe(struct i2c_client *client,
>   
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> + mutex_init(>lock);

Are you initializing the mutex twice?

> + if (np) {
> + /* Device tree */
> + ov965x->gpios[GPIO_RST] =
> + devm_gpiod_get_optional(>dev, "resetb",
> +

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-08 Thread Sakari Ailus 
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 
> Signed-off-by: Hugues Fruchet 
> ---
>  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 
>  #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);

gpiod_set_value_cansleep() can manage with NULL gpio parameter, no need to
check it.

>  }
>  
>  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);

DRIVER_NAME is different from "OV965X". Is this an intended change?

> + 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;

It'd be nice to declare this next to pdata, rather than after ret and other
short declarations.

>  
> - 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,35 @@ static int ov965x_probe(struct i2c_client *client,
>  
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency =

Re: [PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-08 Thread Sakari Ailus 
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 
> Signed-off-by: Hugues Fruchet 
> ---
>  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 
>  #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);

gpiod_set_value_cansleep() can manage with NULL gpio parameter, no need to
check it.

>  }
>  
>  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);

DRIVER_NAME is different from "OV965X". Is this an intended change?

> + 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;

It'd be nice to declare this next to pdata, rather than after ret and other
short declarations.

>  
> - 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,35 @@ static int ov965x_probe(struct i2c_client *client,
>  
>   mutex_init(>lock);
>   ov965x->client = client;
> - ov965x->mclk_frequency = pdata->mclk_frequency;
> +

[PATCH v2 3/7] [media] ov9650: add device tree support

2017-07-03 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 | 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 
 #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,35 @@ 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 v2 3/7] [media] ov9650: add device tree support

2017-07-03 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 | 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 
 #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,35 @@ 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);
+