[PATCH v2 00/10] media: ov772x: support media controller, device tree probing, etc.

[PATCH v2 00/10] media: ov772x: support media controller, device tree probing, etc.

[Akinobu Mita]

This patchset includes support media controller, device tree probing and
other miscellanuous changes for ov772x driver.

* v2 (thanks to Jacopo Mondi)
- Replace the implementation of ov772x_read() instead of adding an
  alternative method
- Assign the ov772x_read() return value to pid and ver directly
- Do the same for MIDH and MIDL
- Move video_probe() before the entity initialization and remove the #ifdef
  around the media_entity_cleanup()
- Use generic names for reset and powerdown gpios (New)
- Add "dt-bindings:" in the subject
- Add a brief description of the sensor
- Update the GPIO names
- Indicate the GPIO active level
- Add missing NULL checks for priv->info
- Leave the check for the missing platform data if legacy platform data
  probe is used.
- Handle nested s_power() calls (New)
- Reconstruct s_frame_interval() (New)
- Avoid accessing registers under power saving mode (New)

Akinobu Mita (10):
  media: ov772x: allow i2c controllers without
    I2C_FUNC_PROTOCOL_MANGLING
  media: ov772x: add checks for register read errors
  media: ov772x: create subdevice device node
  media: ov772x: add media controller support
  media: ov772x: use generic names for reset and powerdown gpios
  media: dt-bindings: ov772x: add device tree binding
  media: ov772x: support device tree probing
  media: ov772x: handle nested s_power() calls
  media: ov772x: reconstruct s_frame_interval()
  media: ov772x: avoid accessing registers under power saving mode

 .../devicetree/bindings/media/i2c/ov772x.txt       |  42 ++++
 MAINTAINERS                                        |   1 +
 arch/sh/boards/mach-migor/setup.c                  |   5 +-
 drivers/media/i2c/ov772x.c                         | 275 ++++++++++++++++-----
 4 files changed, 255 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh+dt@kernel.org>
-- 
2.7.4

[PATCH v2 01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Akinobu Mita]

The ov772x driver only works when the i2c controller have
I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
support it.

The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
it doesn't support repeated starts.

This changes the reading ov772x register method so that it doesn't
require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Replace the implementation of ov772x_read() instead of adding an
  alternative method

 drivers/media/i2c/ov772x.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..7e79da0 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -542,9 +542,19 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov772x_priv, subdev);
 }
 
-static inline int ov772x_read(struct i2c_client *client, u8 addr)
+static int ov772x_read(struct i2c_client *client, u8 addr)
 {
-	return i2c_smbus_read_byte_data(client, addr);
+	int ret;
+	u8 val;
+
+	ret = i2c_master_send(client, &addr, 1);
+	if (ret < 0)
+		return ret;
+	ret = i2c_master_recv(client, &val, 1);
+	if (ret < 0)
+		return ret;
+
+	return val;
 }
 
 static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
@@ -1255,10 +1265,9 @@ static int ov772x_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
-					      I2C_FUNC_PROTOCOL_MANGLING)) {
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&adapter->dev,
-			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
+			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
 		return -EIO;
 	}
 	client->flags |= I2C_CLIENT_SCCB;
-- 
2.7.4

Re: [PATCH v2 01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:42AM +0900, Akinobu Mita wrote:
> The ov772x driver only works when the i2c controller have
> I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> support it.
>
> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> it doesn't support repeated starts.
>
> This changes the reading ov772x register method so that it doesn't
> require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - Replace the implementation of ov772x_read() instead of adding an
>   alternative method

I now wonder if my initial reply to this patch was wrong, and where
possible we should try to use smbus operations...

From Documentation/i2c/smbus-protocol
"If you write a driver for some I2C device, please try to use the SMBus
commands if at all possible... " and that's because, according to
documentation, most I2c adapters support smbus protocol but may not
support the full i2c command set.

The fact this driver then restricts the supported adapters to the ones
that support protocol mangling makes me think your change is fine,
but as it often happens, I would scale this to more knowledgable
people...

>
>  drivers/media/i2c/ov772x.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..7e79da0 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -542,9 +542,19 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ov772x_priv, subdev);
>  }
>
> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> +static int ov772x_read(struct i2c_client *client, u8 addr)
>  {
> -	return i2c_smbus_read_byte_data(client, addr);
> +	int ret;
> +	u8 val;
> +
> +	ret = i2c_master_send(client, &addr, 1);
> +	if (ret < 0)
> +		return ret;
> +	ret = i2c_master_recv(client, &val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return val;
>  }
>
>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> @@ -1255,10 +1265,9 @@ static int ov772x_probe(struct i2c_client *client,
>  		return -EINVAL;
>  	}
>
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> -					      I2C_FUNC_PROTOCOL_MANGLING)) {
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>  		dev_err(&adapter->dev,
> -			"I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
> +			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
>  		return -EIO;
>  	}
>  	client->flags |= I2C_CLIENT_SCCB;

I think you can remove this flag if we're using plain i2c
transactions. Sorry, I have missed this in v1.

Thanks
   j

> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[Sakari Ailus]

On Wed, Apr 18, 2018 at 12:05:49PM +0200, jacopo mondi wrote:
> Hi Akinobu,
> 
> On Mon, Apr 16, 2018 at 11:51:42AM +0900, Akinobu Mita wrote:
> > The ov772x driver only works when the i2c controller have
> > I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> > support it.
> >
> > The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> > it doesn't support repeated starts.
> >
> > This changes the reading ov772x register method so that it doesn't
> > require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
> >
> > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> > * v2
> > - Replace the implementation of ov772x_read() instead of adding an
> >   alternative method
> 
> I now wonder if my initial reply to this patch was wrong, and where
> possible we should try to use smbus operations...
> 
> From Documentation/i2c/smbus-protocol
> "If you write a driver for some I2C device, please try to use the SMBus
> commands if at all possible... " and that's because, according to
> documentation, most I2c adapters support smbus protocol but may not
> support the full i2c command set.
> 
> The fact this driver then restricts the supported adapters to the ones
> that support protocol mangling makes me think your change is fine,
> but as it often happens, I would scale this to more knowledgable
> people...

Do you actually need to use this on SMBus adapters? A lot of sensor drivers
just use I�C; if SMBus support is really needed it can be always added back
later on...

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v2 01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

Hi Sakari,

On Wed, Apr 18, 2018 at 01:41:54PM +0300, Sakari Ailus wrote:
> On Wed, Apr 18, 2018 at 12:05:49PM +0200, jacopo mondi wrote:
> > Hi Akinobu,
> >
> > On Mon, Apr 16, 2018 at 11:51:42AM +0900, Akinobu Mita wrote:
> > > The ov772x driver only works when the i2c controller have
> > > I2C_FUNC_PROTOCOL_MANGLING.  However, many i2c controller drivers don't
> > > support it.
> > >
> > > The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> > > it doesn't support repeated starts.
> > >
> > > This changes the reading ov772x register method so that it doesn't
> > > require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
> > >
> > > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > ---
> > > * v2
> > > - Replace the implementation of ov772x_read() instead of adding an
> > >   alternative method
> >
> > I now wonder if my initial reply to this patch was wrong, and where
> > possible we should try to use smbus operations...
> >
> > From Documentation/i2c/smbus-protocol
> > "If you write a driver for some I2C device, please try to use the SMBus
> > commands if at all possible... " and that's because, according to
> > documentation, most I2c adapters support smbus protocol but may not
> > support the full i2c command set.
> >
> > The fact this driver then restricts the supported adapters to the ones
> > that support protocol mangling makes me think your change is fine,
> > but as it often happens, I would scale this to more knowledgable
> > people...
>
> Do you actually need to use this on SMBus adapters? A lot of sensor drivers
> just use I²C; if SMBus support is really needed it can be always added back
> later on...

That was actually my question, sorry for not being clear.

As the documentation says that SMBus has to be preferred when
possible, I was wondering if ditching it completely in favour of plain
I2c was wrong or not... I assume from your answer it is fine.

>
> --
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 02/10] media: ov772x: add checks for register read errors

[Akinobu Mita]

This change adds checks for register read errors and returns correct
error code.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Assign the ov772x_read() return value to pid and ver directly
- Do the same for MIDH and MIDL

 drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 7e79da0..8badd6f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1146,7 +1146,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 static int ov772x_video_probe(struct ov772x_priv *priv)
 {
 	struct i2c_client  *client = v4l2_get_subdevdata(&priv->subdev);
-	u8                  pid, ver;
+	int		    pid, ver, midh, midl;
 	const char         *devname;
 	int		    ret;
 
@@ -1156,7 +1156,11 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 
 	/* Check and show product ID and manufacturer ID. */
 	pid = ov772x_read(client, PID);
+	if (pid < 0)
+		return pid;
 	ver = ov772x_read(client, VER);
+	if (ver < 0)
+		return ver;
 
 	switch (VERSION(pid, ver)) {
 	case OV7720:
@@ -1172,13 +1176,17 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 		goto done;
 	}
 
+	midh = ov772x_read(client, MIDH);
+	if (midh < 0)
+		return midh;
+	midl = ov772x_read(client, MIDL);
+	if (midl < 0)
+		return midl;
+
 	dev_info(&client->dev,
 		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
-		 devname,
-		 pid,
-		 ver,
-		 ov772x_read(client, MIDH),
-		 ov772x_read(client, MIDL));
+		 devname, pid, ver, midh, midl);
+
 	ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
 done:
-- 
2.7.4

Re: [PATCH v2 02/10] media: ov772x: add checks for register read errors

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:43AM +0900, Akinobu Mita wrote:
> This change adds checks for register read errors and returns correct
> error code.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

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

> ---
> * v2
> - Assign the ov772x_read() return value to pid and ver directly
> - Do the same for MIDH and MIDL
>
>  drivers/media/i2c/ov772x.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 7e79da0..8badd6f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1146,7 +1146,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  static int ov772x_video_probe(struct ov772x_priv *priv)
>  {
>  	struct i2c_client  *client = v4l2_get_subdevdata(&priv->subdev);
> -	u8                  pid, ver;
> +	int		    pid, ver, midh, midl;
>  	const char         *devname;
>  	int		    ret;
>
> @@ -1156,7 +1156,11 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>
>  	/* Check and show product ID and manufacturer ID. */
>  	pid = ov772x_read(client, PID);
> +	if (pid < 0)
> +		return pid;
>  	ver = ov772x_read(client, VER);
> +	if (ver < 0)
> +		return ver;
>
>  	switch (VERSION(pid, ver)) {
>  	case OV7720:
> @@ -1172,13 +1176,17 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  		goto done;
>  	}
>
> +	midh = ov772x_read(client, MIDH);
> +	if (midh < 0)
> +		return midh;
> +	midl = ov772x_read(client, MIDL);
> +	if (midl < 0)
> +		return midl;
> +
>  	dev_info(&client->dev,
>  		 "%s Product ID %0x:%0x Manufacturer ID %x:%x\n",
> -		 devname,
> -		 pid,
> -		 ver,
> -		 ov772x_read(client, MIDH),
> -		 ov772x_read(client, MIDL));
> +		 devname, pid, ver, midh, midl);
> +
>  	ret = v4l2_ctrl_handler_setup(&priv->hdl);
>
>  done:
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 03/10] media: ov772x: create subdevice device node

[Akinobu Mita]

Set the V4L2_SUBDEV_FL_HAS_DEVNODE flag for the subdevice so that the
subdevice device node is created.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- No changes

 drivers/media/i2c/ov772x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 8badd6f..188f2f1 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -1287,6 +1287,7 @@ static int ov772x_probe(struct i2c_client *client,
 	priv->info = client->dev.platform_data;
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
+	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
-- 
2.7.4

Re: [PATCH v2 03/10] media: ov772x: create subdevice device node

[Sakari Ailus]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:44AM +0900, Akinobu Mita wrote:
> Set the V4L2_SUBDEV_FL_HAS_DEVNODE flag for the subdevice so that the
> subdevice device node is created.
> 
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - No changes
> 
>  drivers/media/i2c/ov772x.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 8badd6f..188f2f1 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -1287,6 +1287,7 @@ static int ov772x_probe(struct i2c_client *client,
>  	priv->info = client->dev.platform_data;
>  
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> +	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

Could you move this patch as the last in the set, so that you're enabling
the sub-device interface only when all the changes to support it are
actually there?

>  	v4l2_ctrl_handler_init(&priv->hdl, 3);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  			  V4L2_CID_VFLIP, 0, 1, 1, 0);

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 04/10] media: ov772x: add media controller support

[Akinobu Mita]

Create a source pad and set the media controller type to the sensor.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Move video_probe() before the entity initialization and remove the #ifdef
  around the media_entity_cleanup()

 drivers/media/i2c/ov772x.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 188f2f1..0ae2a4f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_pad pad;
+#endif
 };
 
 /*
@@ -1318,16 +1321,26 @@ static int ov772x_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto error_gpio_put;
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
+	priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&priv->subdev.entity, 1, &priv->pad);
+	if (ret < 0)
+		goto error_gpio_put;
+#endif
+
 	priv->cfmt = &ov772x_cfmts[0];
 	priv->win = &ov772x_win_sizes[0];
 	priv->fps = 15;
 
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret)
-		goto error_gpio_put;
+		goto error_entity_cleanup;
 
 	return 0;
 
+error_entity_cleanup:
+	media_entity_cleanup(&priv->subdev.entity);
 error_gpio_put:
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
@@ -1343,6 +1356,7 @@ static int ov772x_remove(struct i2c_client *client)
 {
 	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
 
+	media_entity_cleanup(&priv->subdev.entity);
 	clk_put(priv->clk);
 	if (priv->pwdn_gpio)
 		gpiod_put(priv->pwdn_gpio);
-- 
2.7.4

Re: [PATCH v2 04/10] media: ov772x: add media controller support

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:45AM +0900, Akinobu Mita wrote:
> Create a source pad and set the media controller type to the sensor.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

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

Not strictly on this patch, but I'm a bit confused on the difference
between CONFIG_MEDIA_CONTROLLER and CONFIG_VIDEO_V4L2_SUBDEV_API...
Doesn't media controller support mandate implementing subdev APIs as
well?

Thanks
   j
> ---
> * v2
> - Move video_probe() before the entity initialization and remove the #ifdef
>   around the media_entity_cleanup()
>
>  drivers/media/i2c/ov772x.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 188f2f1..0ae2a4f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -424,6 +424,9 @@ struct ov772x_priv {
>  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>  	unsigned short                    band_filter;
>  	unsigned int			  fps;
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct media_pad pad;
> +#endif
>  };
>
>  /*
> @@ -1318,16 +1321,26 @@ static int ov772x_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		goto error_gpio_put;
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	priv->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	ret = media_entity_pads_init(&priv->subdev.entity, 1, &priv->pad);
> +	if (ret < 0)
> +		goto error_gpio_put;
> +#endif
> +
>  	priv->cfmt = &ov772x_cfmts[0];
>  	priv->win = &ov772x_win_sizes[0];
>  	priv->fps = 15;
>
>  	ret = v4l2_async_register_subdev(&priv->subdev);
>  	if (ret)
> -		goto error_gpio_put;
> +		goto error_entity_cleanup;
>
>  	return 0;
>
> +error_entity_cleanup:
> +	media_entity_cleanup(&priv->subdev.entity);
>  error_gpio_put:
>  	if (priv->pwdn_gpio)
>  		gpiod_put(priv->pwdn_gpio);
> @@ -1343,6 +1356,7 @@ static int ov772x_remove(struct i2c_client *client)
>  {
>  	struct ov772x_priv *priv = to_ov772x(i2c_get_clientdata(client));
>
> +	media_entity_cleanup(&priv->subdev.entity);
>  	clk_put(priv->clk);
>  	if (priv->pwdn_gpio)
>  		gpiod_put(priv->pwdn_gpio);
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 04/10] media: ov772x: add media controller support

[Sakari Ailus]

On Wed, Apr 18, 2018 at 01:28:14PM +0200, jacopo mondi wrote:
> Hi Akinobu,
> 
> On Mon, Apr 16, 2018 at 11:51:45AM +0900, Akinobu Mita wrote:
> > Create a source pad and set the media controller type to the sensor.
> >
> > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Not strictly on this patch, but I'm a bit confused on the difference
> between CONFIG_MEDIA_CONTROLLER and CONFIG_VIDEO_V4L2_SUBDEV_API...
> Doesn't media controller support mandate implementing subdev APIs as
> well?

The subdev uAPI depends on MC.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 04/10] media: ov772x: add media controller support

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

Hi Sakari,

On Wed, Apr 18, 2018 at 02:58:16PM +0300, Sakari Ailus wrote:
> On Wed, Apr 18, 2018 at 01:28:14PM +0200, jacopo mondi wrote:
> > Hi Akinobu,
> >
> > On Mon, Apr 16, 2018 at 11:51:45AM +0900, Akinobu Mita wrote:
> > > Create a source pad and set the media controller type to the sensor.
> > >
> > > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Not strictly on this patch, but I'm a bit confused on the difference
> > between CONFIG_MEDIA_CONTROLLER and CONFIG_VIDEO_V4L2_SUBDEV_API...
> > Doesn't media controller support mandate implementing subdev APIs as
> > well?
>
> The subdev uAPI depends on MC.

Again, sorry for not being clear. Can an mc-compliant device not
implement sudev uAPIs ?

Thanks
  j

>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 04/10] media: ov772x: add media controller support

[Sakari Ailus]

On Wed, Apr 18, 2018 at 02:13:17PM +0200, jacopo mondi wrote:
> Hi Sakari,
> 
> On Wed, Apr 18, 2018 at 02:58:16PM +0300, Sakari Ailus wrote:
> > On Wed, Apr 18, 2018 at 01:28:14PM +0200, jacopo mondi wrote:
> > > Hi Akinobu,
> > >
> > > On Mon, Apr 16, 2018 at 11:51:45AM +0900, Akinobu Mita wrote:
> > > > Create a source pad and set the media controller type to the sensor.
> > > >
> > > > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Not strictly on this patch, but I'm a bit confused on the difference
> > > between CONFIG_MEDIA_CONTROLLER and CONFIG_VIDEO_V4L2_SUBDEV_API...
> > > Doesn't media controller support mandate implementing subdev APIs as
> > > well?
> >
> > The subdev uAPI depends on MC.
> 
> Again, sorry for not being clear. Can an mc-compliant device not
> implement sudev uAPIs ?

In principle, yes. Still, for a sensor driver supporting MC it only makes
sense if it also supports sub-device uAPI.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 05/10] media: ov772x: use generic names for reset and powerdown gpios

[Akinobu Mita]

The ov772x driver uses "rstb-gpios" and "pwdn-gpios" for reset and
powerdown pins.  However, using generic names for thse gpios is preferred.
("reset-gpios" and "powerdown-gpios" respectively)

There is only one mainline user for these gpios, so rename to generic
names.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 arch/sh/boards/mach-migor/setup.c | 5 +++--
 drivers/media/i2c/ov772x.c        | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 271dfc2..73b9ee4 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -351,8 +351,9 @@ static struct platform_device migor_ceu_device = {
 static struct gpiod_lookup_table ov7725_gpios = {
 	.dev_id		= "0-0021",
 	.table		= {
-		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "pwdn", GPIO_ACTIVE_HIGH),
-		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "powerdown",
+			    GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "reset", GPIO_ACTIVE_LOW),
 	},
 };
 
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 0ae2a4f..88d1418a 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -837,10 +837,10 @@ static int ov772x_power_on(struct ov772x_priv *priv)
 	 * available to handle this cleanly, request the GPIO temporarily
 	 * to avoid conflicts.
 	 */
-	priv->rstb_gpio = gpiod_get_optional(&client->dev, "rstb",
+	priv->rstb_gpio = gpiod_get_optional(&client->dev, "reset",
 					     GPIOD_OUT_LOW);
 	if (IS_ERR(priv->rstb_gpio)) {
-		dev_info(&client->dev, "Unable to get GPIO \"rstb\"");
+		dev_info(&client->dev, "Unable to get GPIO \"reset\"");
 		return PTR_ERR(priv->rstb_gpio);
 	}
 
@@ -1309,10 +1309,10 @@ static int ov772x_probe(struct i2c_client *client,
 		goto error_ctrl_free;
 	}
 
-	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "pwdn",
+	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "powerdown",
 					     GPIOD_OUT_LOW);
 	if (IS_ERR(priv->pwdn_gpio)) {
-		dev_info(&client->dev, "Unable to get GPIO \"pwdn\"");
+		dev_info(&client->dev, "Unable to get GPIO \"powerdown\"");
 		ret = PTR_ERR(priv->pwdn_gpio);
 		goto error_clk_put;
 	}
-- 
2.7.4

Re: [PATCH v2 05/10] media: ov772x: use generic names for reset and powerdown gpios

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:46AM +0900, Akinobu Mita wrote:
> The ov772x driver uses "rstb-gpios" and "pwdn-gpios" for reset and
> powerdown pins.  However, using generic names for thse gpios is preferred.

nit: 'these gpios'

> ("reset-gpios" and "powerdown-gpios" respectively)
>
> There is only one mainline user for these gpios, so rename to generic
> names.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Bindings update should come first.
Not a big deal.

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

> ---
> * v2
> - New patch
>
>  arch/sh/boards/mach-migor/setup.c | 5 +++--
>  drivers/media/i2c/ov772x.c        | 8 ++++----
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
> index 271dfc2..73b9ee4 100644
> --- a/arch/sh/boards/mach-migor/setup.c
> +++ b/arch/sh/boards/mach-migor/setup.c
> @@ -351,8 +351,9 @@ static struct platform_device migor_ceu_device = {
>  static struct gpiod_lookup_table ov7725_gpios = {
>  	.dev_id		= "0-0021",
>  	.table		= {
> -		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "pwdn", GPIO_ACTIVE_HIGH),
> -		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "rstb", GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT0, "powerdown",
> +			    GPIO_ACTIVE_HIGH),
> +		GPIO_LOOKUP("sh7722_pfc", GPIO_PTT3, "reset", GPIO_ACTIVE_LOW),
>  	},
>  };
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 0ae2a4f..88d1418a 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -837,10 +837,10 @@ static int ov772x_power_on(struct ov772x_priv *priv)
>  	 * available to handle this cleanly, request the GPIO temporarily
>  	 * to avoid conflicts.
>  	 */
> -	priv->rstb_gpio = gpiod_get_optional(&client->dev, "rstb",
> +	priv->rstb_gpio = gpiod_get_optional(&client->dev, "reset",
>  					     GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->rstb_gpio)) {
> -		dev_info(&client->dev, "Unable to get GPIO \"rstb\"");
> +		dev_info(&client->dev, "Unable to get GPIO \"reset\"");
>  		return PTR_ERR(priv->rstb_gpio);
>  	}
>
> @@ -1309,10 +1309,10 @@ static int ov772x_probe(struct i2c_client *client,
>  		goto error_ctrl_free;
>  	}
>
> -	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "pwdn",
> +	priv->pwdn_gpio = gpiod_get_optional(&client->dev, "powerdown",
>  					     GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->pwdn_gpio)) {
> -		dev_info(&client->dev, "Unable to get GPIO \"pwdn\"");
> +		dev_info(&client->dev, "Unable to get GPIO \"powerdown\"");
>  		ret = PTR_ERR(priv->pwdn_gpio);
>  		goto error_clk_put;
>  	}
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 06/10] media: dt-bindings: ov772x: add device tree binding

[Akinobu Mita]

This adds a device tree binding documentation for OV7720/OV7725 sensor.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Add "dt-bindings:" in the subject
- Add a brief description of the sensor
- Update the GPIO names
- Indicate the GPIO active level

 .../devicetree/bindings/media/i2c/ov772x.txt       | 42 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
new file mode 100644
index 0000000..b045503
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -0,0 +1,42 @@
+* Omnivision OV7720/OV7725 CMOS sensor
+
+The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
+such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
+support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
+
+Required Properties:
+- compatible: shall be one of
+	"ovti,ov7720"
+	"ovti,ov7725"
+- clocks: reference to the xclk input clock.
+- clock-names: shall be "xclk".
+
+Optional Properties:
+- reset-gpios: reference to the GPIO connected to the RSTB pin which is
+  active low, if any.
+- powerdown-gpios: reference to the GPIO connected to the PWDN pin which is
+  active high, if any.
+
+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 {
+	ov772x: camera@21 {
+		compatible = "ovti,ov7725";
+		reg = <0x21>;
+		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
+		powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
+		clocks = <&xclk>;
+		clock-names = "xclk";
+
+		port {
+			ov772x_0: endpoint {
+				remote-endpoint = <&vcap1_in0>;
+			};
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d..f500953 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10344,6 +10344,7 @@ T:	git git://linuxtv.org/media_tree.git
 S:	Odd fixes
 F:	drivers/media/i2c/ov772x.c
 F:	include/media/i2c/ov772x.h
+F:	Documentation/devicetree/bindings/media/i2c/ov772x.txt
 
 OMNIVISION OV7740 SENSOR DRIVER
 M:	Wenyou Yang <wenyou.yang@microchip.com>
-- 
2.7.4

Re: [PATCH v2 06/10] media: dt-bindings: ov772x: add device tree binding

[Rob Herring]

On Mon, Apr 16, 2018 at 11:51:47AM +0900, Akinobu Mita wrote:
> This adds a device tree binding documentation for OV7720/OV7725 sensor.
> 
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - Add "dt-bindings:" in the subject
> - Add a brief description of the sensor
> - Update the GPIO names
> - Indicate the GPIO active level
> 
>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 06/10] media: dt-bindings: ov772x: add device tree binding

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]

On Mon, Apr 16, 2018 at 11:51:47AM +0900, Akinobu Mita wrote:
> This adds a device tree binding documentation for OV7720/OV7725 sensor.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

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

> ---
> * v2
> - Add "dt-bindings:" in the subject
> - Add a brief description of the sensor
> - Update the GPIO names
> - Indicate the GPIO active level
>
>  .../devicetree/bindings/media/i2c/ov772x.txt       | 42 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> new file mode 100644
> index 0000000..b045503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -0,0 +1,42 @@
> +* Omnivision OV7720/OV7725 CMOS sensor
> +
> +The Omnivision OV7720/OV7725 sensor supports multiple resolutions output,
> +such as VGA, QVGA, and any size scaling down from CIF to 40x30. It also can
> +support the YUV422, RGB565/555/444, GRB422 or raw RGB output formats.
> +
> +Required Properties:
> +- compatible: shall be one of
> +	"ovti,ov7720"
> +	"ovti,ov7725"
> +- clocks: reference to the xclk input clock.
> +- clock-names: shall be "xclk".
> +
> +Optional Properties:
> +- reset-gpios: reference to the GPIO connected to the RSTB pin which is
> +  active low, if any.
> +- powerdown-gpios: reference to the GPIO connected to the PWDN pin which is
> +  active high, if any.
> +
> +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 {
> +	ov772x: camera@21 {
> +		compatible = "ovti,ov7725";
> +		reg = <0x21>;
> +		reset-gpios = <&axi_gpio_0 0 GPIO_ACTIVE_LOW>;
> +		powerdown-gpios = <&axi_gpio_0 1 GPIO_ACTIVE_LOW>;
> +		clocks = <&xclk>;
> +		clock-names = "xclk";
> +
> +		port {
> +			ov772x_0: endpoint {
> +				remote-endpoint = <&vcap1_in0>;
> +			};
> +		};
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0a1410d..f500953 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10344,6 +10344,7 @@ T:	git git://linuxtv.org/media_tree.git
>  S:	Odd fixes
>  F:	drivers/media/i2c/ov772x.c
>  F:	include/media/i2c/ov772x.h
> +F:	Documentation/devicetree/bindings/media/i2c/ov772x.txt
>
>  OMNIVISION OV7740 SENSOR DRIVER
>  M:	Wenyou Yang <wenyou.yang@microchip.com>
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 07/10] media: ov772x: support device tree probing

[Akinobu Mita]

The ov772x driver currently only supports legacy platform data probe.
This change enables device tree probing.

Note that the platform data probe can select auto or manual edge control
mode, but the device tree probling can only select auto edge control mode
for now.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Add missing NULL checks for priv->info
- Leave the check for the missing platform data if legacy platform data
  probe is used.

 drivers/media/i2c/ov772x.c | 61 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 88d1418a..4245a46 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -749,13 +749,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_VFLIP:
 		val = ctrl->val ? VFLIP_IMG : 0x00;
 		priv->flag_vflip = ctrl->val;
-		if (priv->info->flags & OV772X_FLAG_VFLIP)
+		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
 			val ^= VFLIP_IMG;
 		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
 	case V4L2_CID_HFLIP:
 		val = ctrl->val ? HFLIP_IMG : 0x00;
 		priv->flag_hflip = ctrl->val;
-		if (priv->info->flags & OV772X_FLAG_HFLIP)
+		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 			val ^= HFLIP_IMG;
 		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
 	case V4L2_CID_BAND_STOP_FILTER:
@@ -914,19 +914,14 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
 	*win = ov772x_select_win(mf->width, mf->height);
 }
 
-static int ov772x_set_params(struct ov772x_priv *priv,
-			     const struct ov772x_color_format *cfmt,
-			     const struct ov772x_win_size *win)
+static int ov772x_edgectrl(struct ov772x_priv *priv)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	struct v4l2_fract tpf;
 	int ret;
-	u8  val;
 
-	/* Reset hardware. */
-	ov772x_reset(client);
+	if (!priv->info)
+		return 0;
 
-	/* Edge Ctrl. */
 	if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
 		/*
 		 * Manual Edge Control Mode.
@@ -937,19 +932,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
 				      priv->info->edgectrl.threshold);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
 				      priv->info->edgectrl.strength);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 	} else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
 		/*
@@ -961,15 +956,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
 				      priv->info->edgectrl.upper);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 
 		ret = ov772x_mask_set(client,
 				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
 				      priv->info->edgectrl.lower);
 		if (ret < 0)
-			goto ov772x_set_fmt_error;
+			return ret;
 	}
 
+	return 0;
+}
+
+static int ov772x_set_params(struct ov772x_priv *priv,
+			     const struct ov772x_color_format *cfmt,
+			     const struct ov772x_win_size *win)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
+	struct v4l2_fract tpf;
+	int ret;
+	u8  val;
+
+	/* Reset hardware. */
+	ov772x_reset(client);
+
+	/* Edge Ctrl. */
+	ret =  ov772x_edgectrl(priv);
+	if (ret < 0)
+		goto ov772x_set_fmt_error;
+
 	/* Format and window size. */
 	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
 	if (ret < 0)
@@ -1020,9 +1035,9 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 
 	/* Set COM3. */
 	val = cfmt->com3;
-	if (priv->info->flags & OV772X_FLAG_VFLIP)
+	if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
 		val |= VFLIP_IMG;
-	if (priv->info->flags & OV772X_FLAG_HFLIP)
+	if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
 		val |= HFLIP_IMG;
 	if (priv->flag_vflip)
 		val ^= VFLIP_IMG;
@@ -1271,7 +1286,7 @@ static int ov772x_probe(struct i2c_client *client,
 	struct i2c_adapter	*adapter = client->adapter;
 	int			ret;
 
-	if (!client->dev.platform_data) {
+	if (!client->dev.of_node && !client->dev.platform_data) {
 		dev_err(&client->dev, "Missing ov772x platform data\n");
 		return -EINVAL;
 	}
@@ -1372,9 +1387,19 @@ static const struct i2c_device_id ov772x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ov772x_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov772x_of_match[] = {
+	{ .compatible = "ovti,ov7725", },
+	{ .compatible = "ovti,ov7720", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov772x_of_match);
+#endif
+
 static struct i2c_driver ov772x_i2c_driver = {
 	.driver = {
 		.name = "ov772x",
+		.of_match_table = of_match_ptr(ov772x_of_match),
 	},
 	.probe    = ov772x_probe,
 	.remove   = ov772x_remove,
-- 
2.7.4

Re: [PATCH v2 07/10] media: ov772x: support device tree probing

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5878 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:48AM +0900, Akinobu Mita wrote:
> The ov772x driver currently only supports legacy platform data probe.
> This change enables device tree probing.
>
> Note that the platform data probe can select auto or manual edge control
> mode, but the device tree probling can only select auto edge control mode
> for now.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - Add missing NULL checks for priv->info
> - Leave the check for the missing platform data if legacy platform data
>   probe is used.
>
>  drivers/media/i2c/ov772x.c | 61 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 88d1418a..4245a46 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -749,13 +749,13 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_VFLIP:
>  		val = ctrl->val ? VFLIP_IMG : 0x00;
>  		priv->flag_vflip = ctrl->val;
> -		if (priv->info->flags & OV772X_FLAG_VFLIP)
> +		if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
>  			val ^= VFLIP_IMG;
>  		return ov772x_mask_set(client, COM3, VFLIP_IMG, val);
>  	case V4L2_CID_HFLIP:
>  		val = ctrl->val ? HFLIP_IMG : 0x00;
>  		priv->flag_hflip = ctrl->val;
> -		if (priv->info->flags & OV772X_FLAG_HFLIP)
> +		if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
>  			val ^= HFLIP_IMG;
>  		return ov772x_mask_set(client, COM3, HFLIP_IMG, val);
>  	case V4L2_CID_BAND_STOP_FILTER:
> @@ -914,19 +914,14 @@ static void ov772x_select_params(const struct v4l2_mbus_framefmt *mf,
>  	*win = ov772x_select_win(mf->width, mf->height);
>  }
>
> -static int ov772x_set_params(struct ov772x_priv *priv,
> -			     const struct ov772x_color_format *cfmt,
> -			     const struct ov772x_win_size *win)
> +static int ov772x_edgectrl(struct ov772x_priv *priv)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> -	struct v4l2_fract tpf;
>  	int ret;
> -	u8  val;
>
> -	/* Reset hardware. */
> -	ov772x_reset(client);
> +	if (!priv->info)
> +		return 0;
>
> -	/* Edge Ctrl. */
>  	if (priv->info->edgectrl.strength & OV772X_MANUAL_EDGE_CTRL) {
>  		/*
>  		 * Manual Edge Control Mode.
> @@ -937,19 +932,19 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>  		ret = ov772x_mask_set(client, DSPAUTO, EDGE_ACTRL, 0x00);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_TRSHLD, OV772X_EDGE_THRESHOLD_MASK,
>  				      priv->info->edgectrl.threshold);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_STRNGT, OV772X_EDGE_STRENGTH_MASK,
>  				      priv->info->edgectrl.strength);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  	} else if (priv->info->edgectrl.upper > priv->info->edgectrl.lower) {
>  		/*
> @@ -961,15 +956,35 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  				      EDGE_UPPER, OV772X_EDGE_UPPER_MASK,
>  				      priv->info->edgectrl.upper);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>
>  		ret = ov772x_mask_set(client,
>  				      EDGE_LOWER, OV772X_EDGE_LOWER_MASK,
>  				      priv->info->edgectrl.lower);
>  		if (ret < 0)
> -			goto ov772x_set_fmt_error;
> +			return ret;
>  	}
>
> +	return 0;
> +}
> +
> +static int ov772x_set_params(struct ov772x_priv *priv,
> +			     const struct ov772x_color_format *cfmt,
> +			     const struct ov772x_win_size *win)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> +	struct v4l2_fract tpf;
> +	int ret;
> +	u8  val;
> +
> +	/* Reset hardware. */
> +	ov772x_reset(client);
> +
> +	/* Edge Ctrl. */
> +	ret =  ov772x_edgectrl(priv);
> +	if (ret < 0)
> +		goto ov772x_set_fmt_error;

You should return ret here, jumping to the ov772x_set_fmt_error label
will only reset the sensor twice and return.

> +
>  	/* Format and window size. */
>  	ret = ov772x_write(client, HSTART, win->rect.left >> 2);
>  	if (ret < 0)
> @@ -1020,9 +1035,9 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>
>  	/* Set COM3. */
>  	val = cfmt->com3;
> -	if (priv->info->flags & OV772X_FLAG_VFLIP)
> +	if (priv->info && (priv->info->flags & OV772X_FLAG_VFLIP))
>  		val |= VFLIP_IMG;
> -	if (priv->info->flags & OV772X_FLAG_HFLIP)
> +	if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
>  		val |= HFLIP_IMG;
>  	if (priv->flag_vflip)
>  		val ^= VFLIP_IMG;
> @@ -1271,7 +1286,7 @@ static int ov772x_probe(struct i2c_client *client,
>  	struct i2c_adapter	*adapter = client->adapter;
>  	int			ret;
>
> -	if (!client->dev.platform_data) {
> +	if (!client->dev.of_node && !client->dev.platform_data) {
>  		dev_err(&client->dev, "Missing ov772x platform data\n");

Update the error message as well.

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

Thanks
   j

>  		return -EINVAL;
>  	}
> @@ -1372,9 +1387,19 @@ static const struct i2c_device_id ov772x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ov772x_id);
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov772x_of_match[] = {
> +	{ .compatible = "ovti,ov7725", },
> +	{ .compatible = "ovti,ov7720", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov772x_of_match);
> +#endif
> +
>  static struct i2c_driver ov772x_i2c_driver = {
>  	.driver = {
>  		.name = "ov772x",
> +		.of_match_table = of_match_ptr(ov772x_of_match),
>  	},
>  	.probe    = ov772x_probe,
>  	.remove   = ov772x_remove,
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 08/10] media: ov772x: handle nested s_power() calls

[Akinobu Mita]

Depending on the v4l2 driver, calling s_power() could be nested.  So the
actual transitions between power saving mode and normal operation mode
should only happen at the first power on and the last power off.

This adds an s_power() nesting counter and updates the power state if the
counter is modified from 0 to != 0 or from != 0 to 0.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 4245a46..2cd6e85 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -424,6 +424,9 @@ struct ov772x_priv {
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	unsigned short                    band_filter;
 	unsigned int			  fps;
+	/* lock to protect power_count */
+	struct mutex			  power_lock;
+	int				  power_count;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
@@ -871,9 +874,25 @@ static int ov772x_power_off(struct ov772x_priv *priv)
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
+	int ret = 0;
+
+	mutex_lock(&priv->power_lock);
 
-	return on ? ov772x_power_on(priv) :
-		    ov772x_power_off(priv);
+	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (priv->power_count == !on)
+		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+
+	if (!ret) {
+		/* Update the power count. */
+		priv->power_count += on ? 1 : -1;
+		WARN_ON(priv->power_count < 0);
+	}
+
+	mutex_unlock(&priv->power_lock);
+
+	return ret;
 }
 
 static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
@@ -1303,6 +1322,7 @@ static int ov772x_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	priv->info = client->dev.platform_data;
+	mutex_init(&priv->power_lock);
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
 	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1314,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
 	priv->subdev.ctrl_handler = &priv->hdl;
-	if (priv->hdl.error)
-		return priv->hdl.error;
+	if (priv->hdl.error) {
+		ret = priv->hdl.error;
+		goto error_mutex_destroy;
+	}
 
 	priv->clk = clk_get(&client->dev, "xclk");
 	if (IS_ERR(priv->clk)) {
@@ -1363,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
 	clk_put(priv->clk);
 error_ctrl_free:
 	v4l2_ctrl_handler_free(&priv->hdl);
+error_mutex_destroy:
+	mutex_destroy(&priv->power_lock);
 
 	return ret;
 }
@@ -1377,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
 		gpiod_put(priv->pwdn_gpio);
 	v4l2_async_unregister_subdev(&priv->subdev);
 	v4l2_ctrl_handler_free(&priv->hdl);
+	mutex_destroy(&priv->power_lock);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2 08/10] media: ov772x: handle nested s_power() calls

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4531 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:49AM +0900, Akinobu Mita wrote:
> Depending on the v4l2 driver, calling s_power() could be nested.  So the
> actual transitions between power saving mode and normal operation mode
> should only happen at the first power on and the last power off.

What do you mean with 'nested' ?

My understanding is that:
- if a sensor driver is mc compliant, it's s_power is called from
  v4l2_mc.c pipeline_pm_power_one() only when the power state changes
- if a sensor driver is not mc compliant, the s_power routine is
  called from the platform driver, and it should not happen that it is
  called twice with the same power state
- if a sensor implements subdev's internal operations open and close
  it may call it's own s_power routines from there, and it can be
  opened more that once.

My understanding is that this driver s_power routines are only called
from platform drivers, and they -should- be safe.

Although, I'm not against this protection completely. Others might be,
though.

>
> This adds an s_power() nesting counter and updates the power state if the
> counter is modified from 0 to != 0 or from != 0 to 0.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - New patch
>
>  drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 4245a46..2cd6e85 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -424,6 +424,9 @@ struct ov772x_priv {
>  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>  	unsigned short                    band_filter;
>  	unsigned int			  fps;
> +	/* lock to protect power_count */
> +	struct mutex			  power_lock;
> +	int				  power_count;
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_pad pad;
>  #endif
> @@ -871,9 +874,25 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov772x_priv *priv = to_ov772x(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&priv->power_lock);
>
> -	return on ? ov772x_power_on(priv) :
> -		    ov772x_power_off(priv);
> +	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
> +	 * update the power state.
> +	 */
> +	if (priv->power_count == !on)
> +		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);

Just release the mutex and return 0 if (power_count == on)
The code will be more readable imho.

> +
> +	if (!ret) {
> +		/* Update the power count. */
> +		priv->power_count += on ? 1 : -1;
> +		WARN_ON(priv->power_count < 0);
> +	}
> +
> +	mutex_unlock(&priv->power_lock);
> +
> +	return ret;
>  }
>
>  static const struct ov772x_win_size *ov772x_select_win(u32 width, u32 height)
> @@ -1303,6 +1322,7 @@ static int ov772x_probe(struct i2c_client *client,
>  		return -ENOMEM;
>
>  	priv->info = client->dev.platform_data;
> +	mutex_init(&priv->power_lock);
>
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>  	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1314,8 +1334,10 @@ static int ov772x_probe(struct i2c_client *client,
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  			  V4L2_CID_BAND_STOP_FILTER, 0, 256, 1, 0);
>  	priv->subdev.ctrl_handler = &priv->hdl;
> -	if (priv->hdl.error)
> -		return priv->hdl.error;
> +	if (priv->hdl.error) {
> +		ret = priv->hdl.error;
> +		goto error_mutex_destroy;
> +	}
>
>  	priv->clk = clk_get(&client->dev, "xclk");
>  	if (IS_ERR(priv->clk)) {
> @@ -1363,6 +1385,8 @@ static int ov772x_probe(struct i2c_client *client,
>  	clk_put(priv->clk);
>  error_ctrl_free:
>  	v4l2_ctrl_handler_free(&priv->hdl);
> +error_mutex_destroy:
> +	mutex_destroy(&priv->power_lock);

mutex_destroy() does nothing most of the time. It won't hurt though.

Thanks
   j

>
>  	return ret;
>  }
> @@ -1377,6 +1401,7 @@ static int ov772x_remove(struct i2c_client *client)
>  		gpiod_put(priv->pwdn_gpio);
>  	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
> +	mutex_destroy(&priv->power_lock);
>
>  	return 0;
>  }
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 08/10] media: ov772x: handle nested s_power() calls

[Akinobu Mita]

2018-04-18 21:41 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
> Hi Akinobu,
>
> On Mon, Apr 16, 2018 at 11:51:49AM +0900, Akinobu Mita wrote:
>> Depending on the v4l2 driver, calling s_power() could be nested.  So the
>> actual transitions between power saving mode and normal operation mode
>> should only happen at the first power on and the last power off.
>
> What do you mean with 'nested' ?
>
> My understanding is that:
> - if a sensor driver is mc compliant, it's s_power is called from
>   v4l2_mc.c pipeline_pm_power_one() only when the power state changes
> - if a sensor driver is not mc compliant, the s_power routine is
>   called from the platform driver, and it should not happen that it is
>   called twice with the same power state
> - if a sensor implements subdev's internal operations open and close
>   it may call it's own s_power routines from there, and it can be
>   opened more that once.
>
> My understanding is that this driver s_power routines are only called
> from platform drivers, and they -should- be safe.

For pxa_camera driver, if there are more than two openers for a video
device at the same time, calling s_power() could be nested.  Because
there is nothing to prevent from calling s_power() in
pxac_fops_camera_open().

However, most of other V4L2 drivers use v4l2_fh_is_singular_file() to
prevent from nested s_power() in their open operation.  So we can do
the same for pxa_camera driver.

> Although, I'm not against this protection completely. Others might be,
> though.
>
>>
>> This adds an s_power() nesting counter and updates the power state if the
>> counter is modified from 0 to != 0 or from != 0 to 0.
>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>> * v2
>> - New patch
>>
>>  drivers/media/i2c/ov772x.c | 33 +++++++++++++++++++++++++++++----
>>  1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 4245a46..2cd6e85 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -424,6 +424,9 @@ struct ov772x_priv {
>>       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>>       unsigned short                    band_filter;
>>       unsigned int                      fps;
>> +     /* lock to protect power_count */
>> +     struct mutex                      power_lock;
>> +     int                               power_count;
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>       struct media_pad pad;
>>  #endif
>> @@ -871,9 +874,25 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>>  {
>>       struct ov772x_priv *priv = to_ov772x(sd);
>> +     int ret = 0;
>> +
>> +     mutex_lock(&priv->power_lock);
>>
>> -     return on ? ov772x_power_on(priv) :
>> -                 ov772x_power_off(priv);
>> +     /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>> +      * update the power state.
>> +      */
>> +     if (priv->power_count == !on)
>> +             ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
>
> Just release the mutex and return 0 if (power_count == on)
> The code will be more readable imho.

OK.  Actually, the change in this patch is used like boilerplate in
many subdev drivers.  Also, it's better to print warning when nested
s_power() call is detected as it should not be happened.

[PATCH v2 09/10] media: ov772x: reconstruct s_frame_interval()

[Akinobu Mita]

This splits the s_frame_interval() in subdev video ops into selecting the
frame interval and setting up the registers.

This is a preparatory change to avoid accessing registers under power
saving mode.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 56 +++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2cd6e85..1297a21 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -617,25 +617,16 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
-static int ov772x_set_frame_rate(struct ov772x_priv *priv,
-				 struct v4l2_fract *tpf,
-				 const struct ov772x_color_format *cfmt,
-				 const struct ov772x_win_size *win)
+static unsigned int ov772x_select_fps(struct ov772x_priv *priv,
+				 struct v4l2_fract *tpf)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	unsigned long fin = clk_get_rate(priv->clk);
 	unsigned int fps = tpf->numerator ?
 			   tpf->denominator / tpf->numerator :
 			   tpf->denominator;
 	unsigned int best_diff;
-	unsigned int fsize;
-	unsigned int pclk;
 	unsigned int diff;
 	unsigned int idx;
 	unsigned int i;
-	u8 clkrc = 0;
-	u8 com4 = 0;
-	int ret;
 
 	/* Approximate to the closest supported frame interval. */
 	best_diff = ~0L;
@@ -646,7 +637,25 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 			best_diff = diff;
 		}
 	}
-	fps = ov772x_frame_intervals[idx];
+
+	return ov772x_frame_intervals[idx];
+}
+
+static int ov772x_set_frame_rate(struct ov772x_priv *priv,
+				 unsigned int fps,
+				 const struct ov772x_color_format *cfmt,
+				 const struct ov772x_win_size *win)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
+	unsigned long fin = clk_get_rate(priv->clk);
+	unsigned int fsize;
+	unsigned int pclk;
+	unsigned int best_diff;
+	unsigned int diff;
+	unsigned int i;
+	u8 clkrc = 0;
+	u8 com4 = 0;
+	int ret;
 
 	/* Use image size (with blankings) to calculate desired pixel clock. */
 	switch (cfmt->com7 & OFMT_MASK) {
@@ -711,10 +720,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv *priv,
 	if (ret < 0)
 		return ret;
 
-	tpf->numerator = 1;
-	tpf->denominator = fps;
-	priv->fps = tpf->denominator;
-
 	return 0;
 }
 
@@ -735,8 +740,20 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
 	struct v4l2_fract *tpf = &ival->interval;
+	unsigned int fps;
+	int ret;
+
+	fps = ov772x_select_fps(priv, tpf);
+
+	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+	if (ret)
+		return ret;
 
-	return ov772x_set_frame_rate(priv, tpf, priv->cfmt, priv->win);
+	tpf->numerator = 1;
+	tpf->denominator = fps;
+	priv->fps = fps;
+
+	return 0;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -992,7 +1009,6 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 			     const struct ov772x_win_size *win)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
-	struct v4l2_fract tpf;
 	int ret;
 	u8  val;
 
@@ -1074,9 +1090,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
 		goto ov772x_set_fmt_error;
 
 	/* COM4, CLKRC: Set pixel clock and framerate. */
-	tpf.numerator = 1;
-	tpf.denominator = priv->fps;
-	ret = ov772x_set_frame_rate(priv, &tpf, cfmt, win);
+	ret = ov772x_set_frame_rate(priv, priv->fps, cfmt, win);
 	if (ret < 0)
 		goto ov772x_set_fmt_error;
 
-- 
2.7.4

[PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
and the s_frame_interval() in subdev video ops could be called when the
device is under power saving mode.  These callbacks for ov772x driver
cause updating H/W registers that will fail under power saving mode.

This avoids it by not apply any changes to H/W if the device is not powered
up.  Instead the changes will be restored right after power-up.

Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- New patch

 drivers/media/i2c/ov772x.c | 77 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 62 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 1297a21..c44728f 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
 	struct ov772x_priv *priv = to_ov772x(sd);
 	struct v4l2_fract *tpf = &ival->interval;
 	unsigned int fps;
-	int ret;
+	int ret = 0;
 
 	fps = ov772x_select_fps(priv, tpf);
 
-	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
-	if (ret)
-		return ret;
+	mutex_lock(&priv->power_lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any changes to H/W at this time. Instead
+	 * the frame rate will be restored right after power-up.
+	 */
+	if (priv->power_count > 0) {
+		ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
+		if (ret)
+			goto error;
+	}
 
 	tpf->numerator = 1;
 	tpf->denominator = fps;
 	priv->fps = fps;
+error:
+	mutex_unlock(&priv->power_lock);
 
-	return 0;
+	return ret;
 }
 
 static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 	int ret = 0;
 	u8 val;
 
+	/* v4l2_ctrl_lock() locks our own mutex */
+
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any controls to H/W at this time. Instead
+	 * the controls will be restored right after power-up.
+	 */
+	if (priv->power_count == 0)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		val = ctrl->val ? VFLIP_IMG : 0x00;
@@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
 	return 0;
 }
 
+static int ov772x_set_params(struct ov772x_priv *priv,
+			     const struct ov772x_color_format *cfmt,
+			     const struct ov772x_win_size *win);
+
 static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov772x_priv *priv = to_ov772x(sd);
@@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
 	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
 	 * update the power state.
 	 */
-	if (priv->power_count == !on)
-		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
+	if (priv->power_count == !on) {
+		if (on) {
+			ret = ov772x_power_on(priv);
+			/* Restore the controls */
+			if (!ret)
+				ret = ov772x_set_params(priv, priv->cfmt,
+							priv->win);
+			/* Restore the format and the frame rate */
+			if (!ret)
+				ret = __v4l2_ctrl_handler_setup(&priv->hdl);
+		} else {
+			ret = ov772x_power_off(priv);
+		}
+	}
 
 	if (!ret) {
 		/* Update the power count. */
@@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mf = &format->format;
 	const struct ov772x_color_format *cfmt;
 	const struct ov772x_win_size *win;
-	int ret;
+	int ret = 0;
 
 	if (format->pad)
 		return -EINVAL;
@@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-	ret = ov772x_set_params(priv, cfmt, win);
-	if (ret < 0)
-		return ret;
-
+	mutex_lock(&priv->power_lock);
+	/*
+	 * If the device is not powered up by the host driver do
+	 * not apply any changes to H/W at this time. Instead
+	 * the format will be restored right after power-up.
+	 */
+	if (priv->power_count > 0) {
+		ret = ov772x_set_params(priv, cfmt, win);
+		if (ret < 0)
+			goto error;
+	}
 	priv->win = win;
 	priv->cfmt = cfmt;
+error:
+	mutex_unlock(&priv->power_lock);
 
-	return 0;
+	return ret;
 }
 
 static int ov772x_video_probe(struct ov772x_priv *priv)
@@ -1201,7 +1246,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 	const char         *devname;
 	int		    ret;
 
-	ret = ov772x_s_power(&priv->subdev, 1);
+	ret = ov772x_power_on(priv);
 	if (ret < 0)
 		return ret;
 
@@ -1241,7 +1286,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
 	ret = v4l2_ctrl_handler_setup(&priv->hdl);
 
 done:
-	ov772x_s_power(&priv->subdev, 0);
+	ov772x_power_off(priv);
 
 	return ret;
 }
@@ -1341,6 +1386,8 @@ static int ov772x_probe(struct i2c_client *client,
 	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
 	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	v4l2_ctrl_handler_init(&priv->hdl, 3);
+	/* Use our mutex for the controls */
+	priv->hdl.lock = &priv->power_lock;
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 			  V4L2_CID_VFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
-- 
2.7.4

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Sakari Ailus]

Hi Akinobu,

As the driver now offers a V4L2 sub-device uAPI, it needs to serialise
access to its internal data structures. This appears to be fine, but there
are additional requirements; for instance ov772x_select_params() should
likely fail if you're streaming.

On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> and the s_frame_interval() in subdev video ops could be called when the
> device is under power saving mode.  These callbacks for ov772x driver
> cause updating H/W registers that will fail under power saving mode.
> 
> This avoids it by not apply any changes to H/W if the device is not powered
> up.  Instead the changes will be restored right after power-up.
> 
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - New patch
> 
>  drivers/media/i2c/ov772x.c | 77 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 62 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 1297a21..c44728f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
>  	struct ov772x_priv *priv = to_ov772x(sd);
>  	struct v4l2_fract *tpf = &ival->interval;
>  	unsigned int fps;
> -	int ret;
> +	int ret = 0;
>  
>  	fps = ov772x_select_fps(priv, tpf);
>  
> -	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> -	if (ret)
> -		return ret;
> +	mutex_lock(&priv->power_lock);
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any changes to H/W at this time. Instead
> +	 * the frame rate will be restored right after power-up.
> +	 */
> +	if (priv->power_count > 0) {
> +		ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> +		if (ret)
> +			goto error;
> +	}
>  
>  	tpf->numerator = 1;
>  	tpf->denominator = fps;
>  	priv->fps = fps;

Newline before a label would be nice.

> +error:
> +	mutex_unlock(&priv->power_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  	int ret = 0;
>  	u8 val;
>  
> +	/* v4l2_ctrl_lock() locks our own mutex */
> +
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any controls to H/W at this time. Instead
> +	 * the controls will be restored right after power-up.
> +	 */
> +	if (priv->power_count == 0)
> +		return 0;
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
>  		val = ctrl->val ? VFLIP_IMG : 0x00;
> @@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>  	return 0;
>  }
>  
> +static int ov772x_set_params(struct ov772x_priv *priv,
> +			     const struct ov772x_color_format *cfmt,
> +			     const struct ov772x_win_size *win);
> +
>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov772x_priv *priv = to_ov772x(sd);
> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
>  	 * update the power state.
>  	 */
> -	if (priv->power_count == !on)
> -		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
> +	if (priv->power_count == !on) {
> +		if (on) {
> +			ret = ov772x_power_on(priv);
> +			/* Restore the controls */
> +			if (!ret)
> +				ret = ov772x_set_params(priv, priv->cfmt,
> +							priv->win);
> +			/* Restore the format and the frame rate */
> +			if (!ret)
> +				ret = __v4l2_ctrl_handler_setup(&priv->hdl);
> +		} else {
> +			ret = ov772x_power_off(priv);
> +		}
> +	}
>  
>  	if (!ret) {
>  		/* Update the power count. */
> @@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *mf = &format->format;
>  	const struct ov772x_color_format *cfmt;
>  	const struct ov772x_win_size *win;
> -	int ret;
> +	int ret = 0;
>  
>  	if (format->pad)
>  		return -EINVAL;
> @@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>  
> -	ret = ov772x_set_params(priv, cfmt, win);
> -	if (ret < 0)
> -		return ret;
> -
> +	mutex_lock(&priv->power_lock);
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any changes to H/W at this time. Instead
> +	 * the format will be restored right after power-up.
> +	 */
> +	if (priv->power_count > 0) {
> +		ret = ov772x_set_params(priv, cfmt, win);
> +		if (ret < 0)
> +			goto error;
> +	}
>  	priv->win = win;
>  	priv->cfmt = cfmt;
> +error:
> +	mutex_unlock(&priv->power_lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int ov772x_video_probe(struct ov772x_priv *priv)
> @@ -1201,7 +1246,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  	const char         *devname;
>  	int		    ret;
>  
> -	ret = ov772x_s_power(&priv->subdev, 1);
> +	ret = ov772x_power_on(priv);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1241,7 +1286,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  	ret = v4l2_ctrl_handler_setup(&priv->hdl);
>  
>  done:
> -	ov772x_s_power(&priv->subdev, 0);
> +	ov772x_power_off(priv);
>  
>  	return ret;
>  }
> @@ -1341,6 +1386,8 @@ static int ov772x_probe(struct i2c_client *client,
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>  	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	v4l2_ctrl_handler_init(&priv->hdl, 3);
> +	/* Use our mutex for the controls */
> +	priv->hdl.lock = &priv->power_lock;
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  			  V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

2018-04-16 19:55 GMT+09:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
> Hi Akinobu,
>
> As the driver now offers a V4L2 sub-device uAPI, it needs to serialise
> access to its internal data structures. This appears to be fine, but there
> are additional requirements; for instance ov772x_select_params() should
> likely fail if you're streaming.

OK.  I can find many subdev drivers that have 'streaming' flag in the
private data to keep track of the streaming state and make set_fmt()
return -EBUSY if streaming is on.

I'll prepare another patch that does the same thing.

> On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
>> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
>> and the s_frame_interval() in subdev video ops could be called when the
>> device is under power saving mode.  These callbacks for ov772x driver
>> cause updating H/W registers that will fail under power saving mode.
>>
>> This avoids it by not apply any changes to H/W if the device is not powered
>> up.  Instead the changes will be restored right after power-up.
>>
>> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>> * v2
>> - New patch
>>
>>  drivers/media/i2c/ov772x.c | 77 +++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 62 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
>> index 1297a21..c44728f 100644
>> --- a/drivers/media/i2c/ov772x.c
>> +++ b/drivers/media/i2c/ov772x.c
>> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
>>       struct ov772x_priv *priv = to_ov772x(sd);
>>       struct v4l2_fract *tpf = &ival->interval;
>>       unsigned int fps;
>> -     int ret;
>> +     int ret = 0;
>>
>>       fps = ov772x_select_fps(priv, tpf);
>>
>> -     ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
>> -     if (ret)
>> -             return ret;
>> +     mutex_lock(&priv->power_lock);
>> +     /*
>> +      * If the device is not powered up by the host driver do
>> +      * not apply any changes to H/W at this time. Instead
>> +      * the frame rate will be restored right after power-up.
>> +      */
>> +     if (priv->power_count > 0) {
>> +             ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
>> +             if (ret)
>> +                     goto error;
>> +     }
>>
>>       tpf->numerator = 1;
>>       tpf->denominator = fps;
>>       priv->fps = fps;
>
> Newline before a label would be nice.

I see.

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 6119 bytes --]

Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> and the s_frame_interval() in subdev video ops could be called when the
> device is under power saving mode.  These callbacks for ov772x driver
> cause updating H/W registers that will fail under power saving mode.
>

I might be wrong, but if the sensor is powered off, you should not
receive any subdev_pad_ops function call if sensor is powered off.

For this driver that's up to the platform driver to handle this
correctly, have you found any case where it is necessary to handle
this in the sensor driver? Have I mis-interpreted the use case of this
patch?


> This avoids it by not apply any changes to H/W if the device is not powered
> up.  Instead the changes will be restored right after power-up.
>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - New patch
>
>  drivers/media/i2c/ov772x.c | 77 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 1297a21..c44728f 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -741,19 +741,29 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
>  	struct ov772x_priv *priv = to_ov772x(sd);
>  	struct v4l2_fract *tpf = &ival->interval;
>  	unsigned int fps;
> -	int ret;
> +	int ret = 0;
>
>  	fps = ov772x_select_fps(priv, tpf);
>
> -	ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> -	if (ret)
> -		return ret;
> +	mutex_lock(&priv->power_lock);
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any changes to H/W at this time. Instead
> +	 * the frame rate will be restored right after power-up.
> +	 */
> +	if (priv->power_count > 0) {
> +		ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> +		if (ret)
> +			goto error;
> +	}
>
>  	tpf->numerator = 1;
>  	tpf->denominator = fps;
>  	priv->fps = fps;
> +error:
> +	mutex_unlock(&priv->power_lock);
>
> -	return 0;
> +	return ret;
>  }
>
>  static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -765,6 +775,16 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  	int ret = 0;
>  	u8 val;
>
> +	/* v4l2_ctrl_lock() locks our own mutex */
> +
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any controls to H/W at this time. Instead
> +	 * the controls will be restored right after power-up.
> +	 */
> +	if (priv->power_count == 0)
> +		return 0;
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
>  		val = ctrl->val ? VFLIP_IMG : 0x00;
> @@ -888,6 +908,10 @@ static int ov772x_power_off(struct ov772x_priv *priv)
>  	return 0;
>  }
>
> +static int ov772x_set_params(struct ov772x_priv *priv,
> +			     const struct ov772x_color_format *cfmt,
> +			     const struct ov772x_win_size *win);
> +
>  static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov772x_priv *priv = to_ov772x(sd);
> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>  	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
>  	 * update the power state.
>  	 */
> -	if (priv->power_count == !on)
> -		ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
> +	if (priv->power_count == !on) {
> +		if (on) {
> +			ret = ov772x_power_on(priv);
> +			/* Restore the controls */
> +			if (!ret)
> +				ret = ov772x_set_params(priv, priv->cfmt,
> +							priv->win);
> +			/* Restore the format and the frame rate */
> +			if (!ret)
> +				ret = __v4l2_ctrl_handler_setup(&priv->hdl);

frame interval is not listed in the sensor control list, it won't be
restored if I'm not wrong...

Thanks
   j

> +		} else {
> +			ret = ov772x_power_off(priv);
> +		}
> +	}
>
>  	if (!ret) {
>  		/* Update the power count. */
> @@ -1163,7 +1199,7 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *mf = &format->format;
>  	const struct ov772x_color_format *cfmt;
>  	const struct ov772x_win_size *win;
> -	int ret;
> +	int ret = 0;
>
>  	if (format->pad)
>  		return -EINVAL;
> @@ -1184,14 +1220,23 @@ static int ov772x_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>
> -	ret = ov772x_set_params(priv, cfmt, win);
> -	if (ret < 0)
> -		return ret;
> -
> +	mutex_lock(&priv->power_lock);
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any changes to H/W at this time. Instead
> +	 * the format will be restored right after power-up.
> +	 */
> +	if (priv->power_count > 0) {
> +		ret = ov772x_set_params(priv, cfmt, win);
> +		if (ret < 0)
> +			goto error;
> +	}
>  	priv->win = win;
>  	priv->cfmt = cfmt;
> +error:
> +	mutex_unlock(&priv->power_lock);
>
> -	return 0;
> +	return ret;
>  }
>
>  static int ov772x_video_probe(struct ov772x_priv *priv)
> @@ -1201,7 +1246,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  	const char         *devname;
>  	int		    ret;
>
> -	ret = ov772x_s_power(&priv->subdev, 1);
> +	ret = ov772x_power_on(priv);
>  	if (ret < 0)
>  		return ret;
>
> @@ -1241,7 +1286,7 @@ static int ov772x_video_probe(struct ov772x_priv *priv)
>  	ret = v4l2_ctrl_handler_setup(&priv->hdl);
>
>  done:
> -	ov772x_s_power(&priv->subdev, 0);
> +	ov772x_power_off(priv);
>
>  	return ret;
>  }
> @@ -1341,6 +1386,8 @@ static int ov772x_probe(struct i2c_client *client,
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
>  	priv->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	v4l2_ctrl_handler_init(&priv->hdl, 3);
> +	/* Use our mutex for the controls */
> +	priv->hdl.lock = &priv->power_lock;
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  			  V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> --
> 2.7.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Sakari Ailus]

On Wed, Apr 18, 2018 at 02:55:36PM +0200, jacopo mondi wrote:
> Hi Akinobu,
> 
> On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> > The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> > and the s_frame_interval() in subdev video ops could be called when the
> > device is under power saving mode.  These callbacks for ov772x driver
> > cause updating H/W registers that will fail under power saving mode.
> >
> 
> I might be wrong, but if the sensor is powered off, you should not
> receive any subdev_pad_ops function call if sensor is powered off.

This happens (now that the driver supports sub-device uAPI) if the user
opens a sub-device node but the main driver has not powered the sensor on.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

Hi Sakari,

On Wed, Apr 18, 2018 at 04:17:02PM +0300, Sakari Ailus wrote:
> On Wed, Apr 18, 2018 at 02:55:36PM +0200, jacopo mondi wrote:
> > Hi Akinobu,
> >
> > On Mon, Apr 16, 2018 at 11:51:51AM +0900, Akinobu Mita wrote:
> > > The set_fmt() in subdev pad ops, the s_ctrl() for subdev control handler,
> > > and the s_frame_interval() in subdev video ops could be called when the
> > > device is under power saving mode.  These callbacks for ov772x driver
> > > cause updating H/W registers that will fail under power saving mode.
> > >
> >
> > I might be wrong, but if the sensor is powered off, you should not
> > receive any subdev_pad_ops function call if sensor is powered off.
>
> This happens (now that the driver supports sub-device uAPI) if the user
> opens a sub-device node but the main driver has not powered the sensor on.

Indeed. Sorry for the noise. Could you please check my reply to [08/10] as well?

Thanks
   j

>
> --
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 10/10] media: ov772x: avoid accessing registers under power saving mode

[Akinobu Mita]

2018-04-18 21:55 GMT+09:00 jacopo mondi <jacopo@jmondi.org>:
>> @@ -898,8 +922,20 @@ static int ov772x_s_power(struct v4l2_subdev *sd, int on)
>>       /* If the power count is modified from 0 to != 0 or from != 0 to 0,
>>        * update the power state.
>>        */
>> -     if (priv->power_count == !on)
>> -             ret = on ? ov772x_power_on(priv) : ov772x_power_off(priv);
>> +     if (priv->power_count == !on) {
>> +             if (on) {
>> +                     ret = ov772x_power_on(priv);
>> +                     /* Restore the controls */
>> +                     if (!ret)
>> +                             ret = ov772x_set_params(priv, priv->cfmt,
>> +                                                     priv->win);
>> +                     /* Restore the format and the frame rate */
>> +                     if (!ret)
>> +                             ret = __v4l2_ctrl_handler_setup(&priv->hdl);
>
> frame interval is not listed in the sensor control list, it won't be
> restored if I'm not wrong...

The above two comments were swapped wrongly.  The ov772x_set_params()
actually restores the format, the frame rate.  It restores COM3,
COM8, and BDBASE registers, too.  So calling __v4l2_ctrl_handler_setup()
here is not needed.