diff mbox series

[v4,3/9] media: i2c: ov6650: Use new [get|set]_mbus_config ops

Message ID 20200611161651.264633-4-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series v4l2-subdev: Introduce [g|s]et_mbus_format pad op | expand

Commit Message

Jacopo Mondi June 11, 2020, 4:16 p.m. UTC
Use the new get_mbus_config and set_mbus_config pad operations in place
of the video operations currently in use.

Compared to other drivers where the same conversion has been performed,
ov6650 proved to be a bit more tricky, as the existing g_mbus_config
implementation did not report the currently applied configuration but
the set of all possible configuration options.

Adapt the driver to support the semantic of the two newly introduced
operations:
- get_mbus_config reports the current media bus configuration
- set_mbus_config applies only changes explicitly requested and updates
  the provided cfg parameter to report what has actually been applied to
  the hardware.

Compile-tested only.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 17 deletions(-)

Comments

kernel test robot June 14, 2020, 4:31 a.m. UTC | #1
Hi Jacopo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/v4l2-subdev-Introduce-g-s-et_mbus_format-pad-op/20200612-001600
base:   git://linuxtv.org/media_tree.git master
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/media/i2c/ov6650.c:941:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
                                    ^
   drivers/media/i2c/ov6650.c:943:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
                                    ^
   drivers/media/i2c/ov6650.c:945:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
                                    ^
--
   In file included from fs/nfsd/trace.c:
>> fs/nfsd/trace.h:175:0: warning: There is an unknown macro here somewhere. Configuration is required. If DECLARE_EVENT_CLASS is a macro then please configure it. [unknownMacro]
   
   ^
>> fs/nfsd/nfsctl.c:1277:7: warning: Opposite inner 'if' condition leads to a dead code block. [oppositeInnerCondition]
     if (!files->name)
         ^
   fs/nfsd/nfsctl.c:1276:19: note: outer condition: files->name
    for (i = 0; files->name && files->name[0]; i++, files++) {
                     ^
   fs/nfsd/nfsctl.c:1277:7: note: opposite inner condition: !files->name
     if (!files->name)
         ^
>> fs/nfsd/nfsctl.c:524:5: warning: Variable 'rv' is reassigned a value before the old one has been used. [redundantAssignment]
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
   fs/nfsd/nfsctl.c:504:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = -ENOMEM;
       ^
   fs/nfsd/nfsctl.c:524:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
>> fs/nfsd/nfsctl.c:1201:6: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^
   fs/nfsd/nfsctl.c:1195:0: note: Variable 'ret' is reassigned a value before the old one has been used.
    int ret = -ENOMEM;
   ^
   fs/nfsd/nfsctl.c:1201:6: note: Variable 'ret' is reassigned a value before the old one has been used.
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^

vim +941 drivers/media/i2c/ov6650.c

   922	
   923	/* Request bus settings on camera side */
   924	static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
   925					  unsigned int pad,
   926					  struct v4l2_mbus_config *cfg)
   927	{
   928		struct i2c_client *client = v4l2_get_subdevdata(sd);
   929		u8 comj, comf;
   930		int ret;
   931	
   932		ret = ov6650_reg_read(client, REG_COMJ, &comj);
   933		if (ret)
   934			return ret;
   935	
   936		ret = ov6650_reg_read(client, REG_COMF, &comf);
   937		if (ret)
   938			return ret;
   939	
   940		cfg->flags = V4L2_MBUS_MASTER
 > 941			   |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
   942						       : V4L2_MBUS_VSYNC_ACTIVE_LOW)
   943			   |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
   944						       : V4L2_MBUS_HSYNC_ACTIVE_HIGH)
   945			   |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
   946						       : V4L2_MBUS_PCLK_SAMPLE_FALLING);
   947		cfg->type = V4L2_MBUS_PARALLEL;
   948	
   949		return 0;
   950	}
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 91906b94f978..efc798bac3a4 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -921,46 +921,68 @@  static const struct v4l2_subdev_core_ops ov6650_core_ops = {
 };
 
 /* Request bus settings on camera side */
-static int ov6650_g_mbus_config(struct v4l2_subdev *sd,
-				struct v4l2_mbus_config *cfg)
+static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
+				  unsigned int pad,
+				  struct v4l2_mbus_config *cfg)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	u8 comj, comf;
+	int ret;
+
+	ret = ov6650_reg_read(client, REG_COMJ, &comj);
+	if (ret)
+		return ret;
 
-	cfg->flags = V4L2_MBUS_MASTER |
-		V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING |
-		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW |
-		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW |
-		V4L2_MBUS_DATA_ACTIVE_HIGH;
+	ret = ov6650_reg_read(client, REG_COMF, &comf);
+	if (ret)
+		return ret;
+
+	cfg->flags = V4L2_MBUS_MASTER
+		   |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
+					       : V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		   |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
+					       : V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		   |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
+					       : V4L2_MBUS_PCLK_SAMPLE_FALLING);
 	cfg->type = V4L2_MBUS_PARALLEL;
 
 	return 0;
 }
 
 /* Alter bus settings on camera side */
-static int ov6650_s_mbus_config(struct v4l2_subdev *sd,
-				const struct v4l2_mbus_config *cfg)
+static int ov6650_set_mbus_config(struct v4l2_subdev *sd,
+				  unsigned int pad,
+				  struct v4l2_mbus_config *cfg)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret;
+	int ret = 0;
 
 	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
 	if (ret)
-		return ret;
+		goto error;
 
 	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
 		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
 		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
 	if (ret)
-		return ret;
+		goto error;
 
 	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
 
+error:
+	/*
+	 * Update the configuration to report what is actually applied to
+	 * the hardware.
+	 */
+	ov6650_get_mbus_config(sd, pad, cfg);
+
 	return ret;
 }
 
@@ -968,8 +990,6 @@  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
 	.s_stream	= ov6650_s_stream,
 	.g_frame_interval = ov6650_g_frame_interval,
 	.s_frame_interval = ov6650_s_frame_interval,
-	.g_mbus_config	= ov6650_g_mbus_config,
-	.s_mbus_config	= ov6650_s_mbus_config,
 };
 
 static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
@@ -978,6 +998,8 @@  static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
 	.set_selection	= ov6650_set_selection,
 	.get_fmt	= ov6650_get_fmt,
 	.set_fmt	= ov6650_set_fmt,
+	.get_mbus_config = ov6650_get_mbus_config,
+	.set_mbus_config = ov6650_set_mbus_config,
 };
 
 static const struct v4l2_subdev_ops ov6650_subdev_ops = {