Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Rui Miguel Silva]

Hi Sakari,
On Sat 24 Mar 2018 at 10:57, Sakari Ailus wrote:

Hi Rui,

I wanted to go through the code the last time and I'd have a few 
review

comments below...


Thanks for the review.



On Tue, Mar 13, 2018 at 11:33:11AM +, Rui Miguel Silva 
wrote:

...
+static int ov2680_gain_set(struct ov2680_dev *sensor, bool 
auto_gain)

+{
+   struct ov2680_ctrls *ctrls = >ctrls;
+   u32 gain;
+   int ret;
+
+   ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(1),
+auto_gain ? 0 : BIT(1));
+   if (ret < 0)
+   return ret;
+
+   if (auto_gain || !ctrls->gain->is_new)
+   return 0;
+
+   gain = ctrls->gain->val;
+
+	ret = ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, 
gain);

+
+   return 0;
+}
+
+static int ov2680_gain_get(struct ov2680_dev *sensor)
+{
+   u32 gain;
+   int ret;
+
+	ret = ov2680_read_reg16(sensor, OV2680_REG_GAIN_PK, 
);

+   if (ret)
+   return ret;
+
+   return gain;
+}
+
+static int ov2680_auto_gain_enable(struct ov2680_dev *sensor)
+{
+   return ov2680_gain_set(sensor, true);
+}
+
+static int ov2680_auto_gain_disable(struct ov2680_dev *sensor)
+{
+   return ov2680_gain_set(sensor, false);
+}
+
+static int ov2680_exposure_set(struct ov2680_dev *sensor, bool 
auto_exp)

+{
+   struct ov2680_ctrls *ctrls = >ctrls;
+   u32 exp;
+   int ret;
+
+   ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(0),
+auto_exp ? 0 : BIT(0));
+   if (ret < 0)
+   return ret;
+
+   if (auto_exp || !ctrls->exposure->is_new)
+   return 0;
+
+   exp = (u32)ctrls->exposure->val;
+   exp <<= 4;
+
+	return ov2680_write_reg24(sensor, 
OV2680_REG_EXPOSURE_PK_HIGH, exp);

+}
+
+static int ov2680_exposure_get(struct ov2680_dev *sensor)
+{
+   int ret;
+   u32 exp;
+
+	ret = ov2680_read_reg24(sensor, 
OV2680_REG_EXPOSURE_PK_HIGH, );

+   if (ret)
+   return ret;
+
+   return exp >> 4;
+}
+
+static int ov2680_auto_exposure_enable(struct ov2680_dev 
*sensor)

+{
+   return ov2680_exposure_set(sensor, true);
+}
+
+static int ov2680_auto_exposure_disable(struct ov2680_dev 
*sensor)

+{
+   return ov2680_exposure_set(sensor, false);
+}


I still think you could call the function actually doing the 
work, and pass
the bool parameter. That'd be much clearer. Please see the 
comments below,

too; they're related. Same for gain.


Ok, no problem, will change that in v4.



...


+static int ov2680_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+   struct ov2680_dev *sensor = to_ov2680_dev(sd);
+   int val;
+
+   if (!sensor->is_enabled)
+   return 0;
+
+   switch (ctrl->id) {
+   case V4L2_CID_AUTOGAIN:
+   if (!ctrl->val)
+   return 0;
+   val = ov2680_gain_get(sensor);
+   if (val < 0)
+   return val;
+   sensor->ctrls.gain->val = val;
+   break;
+   case V4L2_CID_EXPOSURE_AUTO:


I reckon the purpose of implementing volatile controls here 
would be to
provide the exposure and gain values to the user. But the 
controls here are
the ones enabling or disabling the automatic behaviour, not the 
value of

those settings themselves.


+   if (ctrl->val == V4L2_EXPOSURE_MANUAL)
+   return 0;
+   val = ov2680_exposure_get(sensor);
+   if (val < 0)
+   return val;
+   sensor->ctrls.exposure->val = val;
+   break;
+   }
+
+   return 0;
+}
+
+static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+   struct ov2680_dev *sensor = to_ov2680_dev(sd);
+
+   if (!sensor->is_enabled)
+   return 0;
+
+   switch (ctrl->id) {
+   case V4L2_CID_AUTOGAIN:
+   return ov2680_gain_set(sensor, !!ctrl->val);
+   case V4L2_CID_EXPOSURE_AUTO:
+   return ov2680_exposure_set(sensor, !!ctrl->val);


With this, you can enable or disable automatic exposure and 
gain, but you

cannot manually set the values. Are such controls useful?

Unless I'm mistaken, exposure or gain are not settable, so you 
should make
them read-only controls. Or better, allow setting them if 
automatic control

is disabled.


Yeah, I could definitely change the values of exposure and gain 
also,
but that may came how the ctrl group work internally. But I will 
change

and add them.




+   case V4L2_CID_VFLIP:
+   if (sensor->is_streaming)
+   return -EBUSY;
+   if (ctrl->val)
+   return ov2680_vflip_enable(sensor);
+   else
+   return ov2680_vflip_disable(sensor);
+   case V4L2_CID_HFLIP:
+   if (ctrl->val)
+   return 

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Sakari Ailus]

Hi Rui,

I wanted to go through the code the last time and I'd have a few review
comments below...

On Tue, Mar 13, 2018 at 11:33:11AM +, Rui Miguel Silva wrote:
...
> +static int ov2680_gain_set(struct ov2680_dev *sensor, bool auto_gain)
> +{
> + struct ov2680_ctrls *ctrls = >ctrls;
> + u32 gain;
> + int ret;
> +
> + ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(1),
> +  auto_gain ? 0 : BIT(1));
> + if (ret < 0)
> + return ret;
> +
> + if (auto_gain || !ctrls->gain->is_new)
> + return 0;
> +
> + gain = ctrls->gain->val;
> +
> + ret = ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
> +
> + return 0;
> +}
> +
> +static int ov2680_gain_get(struct ov2680_dev *sensor)
> +{
> + u32 gain;
> + int ret;
> +
> + ret = ov2680_read_reg16(sensor, OV2680_REG_GAIN_PK, );
> + if (ret)
> + return ret;
> +
> + return gain;
> +}
> +
> +static int ov2680_auto_gain_enable(struct ov2680_dev *sensor)
> +{
> + return ov2680_gain_set(sensor, true);
> +}
> +
> +static int ov2680_auto_gain_disable(struct ov2680_dev *sensor)
> +{
> + return ov2680_gain_set(sensor, false);
> +}
> +
> +static int ov2680_exposure_set(struct ov2680_dev *sensor, bool auto_exp)
> +{
> + struct ov2680_ctrls *ctrls = >ctrls;
> + u32 exp;
> + int ret;
> +
> + ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(0),
> +  auto_exp ? 0 : BIT(0));
> + if (ret < 0)
> + return ret;
> +
> + if (auto_exp || !ctrls->exposure->is_new)
> + return 0;
> +
> + exp = (u32)ctrls->exposure->val;
> + exp <<= 4;
> +
> + return ov2680_write_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, exp);
> +}
> +
> +static int ov2680_exposure_get(struct ov2680_dev *sensor)
> +{
> + int ret;
> + u32 exp;
> +
> + ret = ov2680_read_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, );
> + if (ret)
> + return ret;
> +
> + return exp >> 4;
> +}
> +
> +static int ov2680_auto_exposure_enable(struct ov2680_dev *sensor)
> +{
> + return ov2680_exposure_set(sensor, true);
> +}
> +
> +static int ov2680_auto_exposure_disable(struct ov2680_dev *sensor)
> +{
> + return ov2680_exposure_set(sensor, false);
> +}

I still think you could call the function actually doing the work, and pass
the bool parameter. That'd be much clearer. Please see the comments below,
too; they're related. Same for gain.

...

> +static int ov2680_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> + struct ov2680_dev *sensor = to_ov2680_dev(sd);
> + int val;
> +
> + if (!sensor->is_enabled)
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_AUTOGAIN:
> + if (!ctrl->val)
> + return 0;
> + val = ov2680_gain_get(sensor);
> + if (val < 0)
> + return val;
> + sensor->ctrls.gain->val = val;
> + break;
> + case V4L2_CID_EXPOSURE_AUTO:

I reckon the purpose of implementing volatile controls here would be to
provide the exposure and gain values to the user. But the controls here are
the ones enabling or disabling the automatic behaviour, not the value of
those settings themselves.

> + if (ctrl->val == V4L2_EXPOSURE_MANUAL)
> + return 0;
> + val = ov2680_exposure_get(sensor);
> + if (val < 0)
> + return val;
> + sensor->ctrls.exposure->val = val;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> + struct ov2680_dev *sensor = to_ov2680_dev(sd);
> +
> + if (!sensor->is_enabled)
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_AUTOGAIN:
> + return ov2680_gain_set(sensor, !!ctrl->val);
> + case V4L2_CID_EXPOSURE_AUTO:
> + return ov2680_exposure_set(sensor, !!ctrl->val);

With this, you can enable or disable automatic exposure and gain, but you
cannot manually set the values. Are such controls useful?

Unless I'm mistaken, exposure or gain are not settable, so you should make
them read-only controls. Or better, allow setting them if automatic control
is disabled.

> + case V4L2_CID_VFLIP:
> + if (sensor->is_streaming)
> + return -EBUSY;
> + if (ctrl->val)
> + return ov2680_vflip_enable(sensor);
> + else
> + return ov2680_vflip_disable(sensor);
> + case V4L2_CID_HFLIP:
> + if (ctrl->val)
> + return ov2680_hflip_enable(sensor);
> + else
> + return ov2680_hflip_disable(sensor);
> + case V4L2_CID_TEST_PATTERN:
> + return ov2680_test_pattern_set(sensor, 

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Rui Miguel Silva]

Hi Sakari,
On Fri 16 Mar 2018 at 16:10, Sakari Ailus wrote:
On Thu, Mar 15, 2018 at 09:29:33AM +, Rui Miguel Silva 
wrote:

Hi,
On Wed 14 Mar 2018 at 19:39, kbuild test robot wrote:
> Hi Rui,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on v4.16-rc4]

> [cannot apply to next-20180314]
> [if your patch is applied to the wrong git tree, please drop 
> us a note

> to help improve the system]
> 
> url: 
> https://github.com/0day-ci/linux/commits/Rui-Miguel-Silva/media-Introduce-Omnivision-OV2680-driver/20180315-020617

> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sh
> 
> All errors (new ones prefixed by >>):
> 
>drivers/media/i2c/ov2680.c: In function 'ov2680_set_fmt':
> > > drivers/media/i2c/ov2680.c:713:9: error: implicit 
> > > declaration of

> > > function 'v4l2_find_nearest_size'; did you mean
> > > 'v4l2_find_nearest_format'?
> > > [-Werror=implicit-function-declaration]
>  mode = v4l2_find_nearest_size(ov2680_mode_data,
> ^~
> v4l2_find_nearest_format

As requested by maintainer this series depend on this patch 
[0], which
introduce this macro. I am not sure of the status of that patch 
though.


No need to worry about that, the sensor driver will just be 
merged after
the dependencies are in. Mauro said he'd handle the pull request 
early next

week.


Great, Many thanks for everything.

---
Cheers,
Rui

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Sakari Ailus]

On Thu, Mar 15, 2018 at 09:29:33AM +, Rui Miguel Silva wrote:
> Hi,
> On Wed 14 Mar 2018 at 19:39, kbuild test robot wrote:
> > Hi Rui,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on v4.16-rc4]
> > [cannot apply to next-20180314]
> > [if your patch is applied to the wrong git tree, please drop us a note
> > to help improve the system]
> > 
> > url: 
> > https://github.com/0day-ci/linux/commits/Rui-Miguel-Silva/media-Introduce-Omnivision-OV2680-driver/20180315-020617
> > config: sh-allmodconfig (attached as .config)
> > compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> > wget
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # save the attached .config to linux build tree
> > make.cross ARCH=sh
> > 
> > All errors (new ones prefixed by >>):
> > 
> >drivers/media/i2c/ov2680.c: In function 'ov2680_set_fmt':
> > > > drivers/media/i2c/ov2680.c:713:9: error: implicit declaration of
> > > > function 'v4l2_find_nearest_size'; did you mean
> > > > 'v4l2_find_nearest_format'?
> > > > [-Werror=implicit-function-declaration]
> >  mode = v4l2_find_nearest_size(ov2680_mode_data,
> > ^~
> > v4l2_find_nearest_format
> 
> As requested by maintainer this series depend on this patch [0], which
> introduce this macro. I am not sure of the status of that patch though.

No need to worry about that, the sensor driver will just be merged after
the dependencies are in. Mauro said he'd handle the pull request early next
week.

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

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Rui Miguel Silva]

Hi,
On Wed 14 Mar 2018 at 19:39, kbuild test robot wrote:

Hi Rui,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[cannot apply to next-20180314]
[if your patch is applied to the wrong git tree, please drop us 
a note to help improve the system]


url: 
https://github.com/0day-ci/linux/commits/Rui-Miguel-Silva/media-Introduce-Omnivision-OV2680-driver/20180315-020617

config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
-O ~/bin/make.cross

chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sh 


All errors (new ones prefixed by >>):

   drivers/media/i2c/ov2680.c: In function 'ov2680_set_fmt':
drivers/media/i2c/ov2680.c:713:9: error: implicit declaration 
of function 'v4l2_find_nearest_size'; did you mean 
'v4l2_find_nearest_format'? 
[-Werror=implicit-function-declaration]

 mode = v4l2_find_nearest_size(ov2680_mode_data,
^~
v4l2_find_nearest_format


As requested by maintainer this series depend on this patch [0], 
which
introduce this macro. I am not sure of the status of that patch 
though.


---
Cheers,
Rui

[0] https://patchwork.kernel.org/patch/10207087/

drivers/media/i2c/ov2680.c:714:41: error: 'width' undeclared 
(first use in this function)

  ARRAY_SIZE(ov2680_mode_data), width,
^
   drivers/media/i2c/ov2680.c:714:41: note: each undeclared 
   identifier is reported only once for each function it appears 
   in
drivers/media/i2c/ov2680.c:715:11: error: 'height' undeclared 
(first use in this function); did you mean 'hweight8'?

  height, fmt->width, fmt->height);
  ^~
  hweight8
   cc1: some warnings being treated as errors

vim +713 drivers/media/i2c/ov2680.c

   693  
   694  static int ov2680_set_fmt(struct v4l2_subdev *sd,
   695  struct 
   v4l2_subdev_pad_config *cfg,
   696  struct 
   v4l2_subdev_format *format)

   697  {
   698		struct ov2680_dev *sensor = 
   to_ov2680_dev(sd);
   699		struct v4l2_mbus_framefmt *fmt = 
   >format;

   700  const struct ov2680_mode_info *mode;
   701  int ret = 0;
   702  
   703  if (format->pad != 0)
   704  return -EINVAL;
   705  
   706  mutex_lock(>lock);
   707  
   708  if (sensor->is_streaming) {
   709  ret = -EBUSY;
   710  goto unlock;
   711  }
   712  
 > 713		mode = 
 > v4l2_find_nearest_size(ov2680_mode_data,
 > 714 
 > ARRAY_SIZE(ov2680_mode_data), width,
 > 715	  height, 
 > fmt->width, fmt->height);

   716  if (!mode) {
   717  ret = -EINVAL;
   718  goto unlock;
   719  }
   720  
   721		if (format->which == 
   V4L2_SUBDEV_FORMAT_TRY) {
   722			fmt = 
   v4l2_subdev_get_try_format(sd, cfg, 0);

   723  
   724  *fmt = format->format;
   725  goto unlock;
   726  }
   727  
   728  fmt->width = mode->width;
   729  fmt->height = mode->height;
   730  fmt->code = sensor->fmt.code;
   731  fmt->colorspace = sensor->fmt.colorspace;
   732  
   733  sensor->current_mode = mode;
   734  sensor->fmt = format->format;
   735  sensor->mode_pending_changes = true;
   736  
   737  unlock:
   738  mutex_unlock(>lock);
   739  
   740  return ret;
   741  }
   742  

---
0-DAY kernel test infrastructureOpen Source 
Technology Center
https://lists.01.org/pipermail/kbuild-all 
Intel Corporation

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[kbuild test robot]

Hi Rui,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[cannot apply to next-20180314]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rui-Miguel-Silva/media-Introduce-Omnivision-OV2680-driver/20180315-020617
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/media/i2c/ov2680.c: In function 'ov2680_set_fmt':
   drivers/media/i2c/ov2680.c:713:9: error: implicit declaration of function 
'v4l2_find_nearest_size'; did you mean 'v4l2_find_nearest_format'? 
[-Werror=implicit-function-declaration]
 mode = v4l2_find_nearest_size(ov2680_mode_data,
^~
v4l2_find_nearest_format
   drivers/media/i2c/ov2680.c:714:195: error: 'width' undeclared (first use in 
this function)
  ARRAY_SIZE(ov2680_mode_data), width,


  ^
   drivers/media/i2c/ov2680.c:714:195: note: each undeclared identifier is 
reported only once for each function it appears in
>> drivers/media/i2c/ov2680.c:715:11: error: 'height' undeclared (first use in 
>> this function); did you mean '__iget'?
  height, fmt->width, fmt->height);
  ^~
  __iget
   cc1: some warnings being treated as errors

vim +715 drivers/media/i2c/ov2680.c

   693  
   694  static int ov2680_set_fmt(struct v4l2_subdev *sd,
   695struct v4l2_subdev_pad_config *cfg,
   696struct v4l2_subdev_format *format)
   697  {
   698  struct ov2680_dev *sensor = to_ov2680_dev(sd);
   699  struct v4l2_mbus_framefmt *fmt = >format;
   700  const struct ov2680_mode_info *mode;
   701  int ret = 0;
   702  
   703  if (format->pad != 0)
   704  return -EINVAL;
   705  
   706  mutex_lock(>lock);
   707  
   708  if (sensor->is_streaming) {
   709  ret = -EBUSY;
   710  goto unlock;
   711  }
   712  
 > 713  mode = v4l2_find_nearest_size(ov2680_mode_data,
   714ARRAY_SIZE(ov2680_mode_data), 
width,
 > 715height, fmt->width, fmt->height);
   716  if (!mode) {
   717  ret = -EINVAL;
   718  goto unlock;
   719  }
   720  
   721  if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
   722  fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
   723  
   724  *fmt = format->format;
   725  goto unlock;
   726  }
   727  
   728  fmt->width = mode->width;
   729  fmt->height = mode->height;
   730  fmt->code = sensor->fmt.code;
   731  fmt->colorspace = sensor->fmt.colorspace;
   732  
   733  sensor->current_mode = mode;
   734  sensor->fmt = format->format;
   735  sensor->mode_pending_changes = true;
   736  
   737  unlock:
   738  mutex_unlock(>lock);
   739  
   740  return ret;
   741  }
   742  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[kbuild test robot]

Hi Rui,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[cannot apply to next-20180314]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Rui-Miguel-Silva/media-Introduce-Omnivision-OV2680-driver/20180315-020617
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   drivers/media/i2c/ov2680.c: In function 'ov2680_set_fmt':
>> drivers/media/i2c/ov2680.c:713:9: error: implicit declaration of function 
>> 'v4l2_find_nearest_size'; did you mean 'v4l2_find_nearest_format'? 
>> [-Werror=implicit-function-declaration]
 mode = v4l2_find_nearest_size(ov2680_mode_data,
^~
v4l2_find_nearest_format
>> drivers/media/i2c/ov2680.c:714:41: error: 'width' undeclared (first use in 
>> this function)
  ARRAY_SIZE(ov2680_mode_data), width,
^
   drivers/media/i2c/ov2680.c:714:41: note: each undeclared identifier is 
reported only once for each function it appears in
>> drivers/media/i2c/ov2680.c:715:11: error: 'height' undeclared (first use in 
>> this function); did you mean 'hweight8'?
  height, fmt->width, fmt->height);
  ^~
  hweight8
   cc1: some warnings being treated as errors

vim +713 drivers/media/i2c/ov2680.c

   693  
   694  static int ov2680_set_fmt(struct v4l2_subdev *sd,
   695struct v4l2_subdev_pad_config *cfg,
   696struct v4l2_subdev_format *format)
   697  {
   698  struct ov2680_dev *sensor = to_ov2680_dev(sd);
   699  struct v4l2_mbus_framefmt *fmt = >format;
   700  const struct ov2680_mode_info *mode;
   701  int ret = 0;
   702  
   703  if (format->pad != 0)
   704  return -EINVAL;
   705  
   706  mutex_lock(>lock);
   707  
   708  if (sensor->is_streaming) {
   709  ret = -EBUSY;
   710  goto unlock;
   711  }
   712  
 > 713  mode = v4l2_find_nearest_size(ov2680_mode_data,
 > 714ARRAY_SIZE(ov2680_mode_data), 
 > width,
 > 715height, fmt->width, fmt->height);
   716  if (!mode) {
   717  ret = -EINVAL;
   718  goto unlock;
   719  }
   720  
   721  if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
   722  fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
   723  
   724  *fmt = format->format;
   725  goto unlock;
   726  }
   727  
   728  fmt->width = mode->width;
   729  fmt->height = mode->height;
   730  fmt->code = sensor->fmt.code;
   731  fmt->colorspace = sensor->fmt.colorspace;
   732  
   733  sensor->current_mode = mode;
   734  sensor->fmt = format->format;
   735  sensor->mode_pending_changes = true;
   736  
   737  unlock:
   738  mutex_unlock(>lock);
   739  
   740  return ret;
   741  }
   742  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

[Rui Miguel Silva]

This patch adds V4L2 sub-device driver for OV2680 image sensor.
The OV2680 is a 1/5" CMOS color sensor from Omnivision.
Supports output format: 10-bit Raw RGB.
The OV2680 has a single lane MIPI interface.

The driver exposes following V4L2 controls:
- auto/manual exposure,
- exposure,
- auto/manual gain,
- gain,
- horizontal/vertical flip,
- test pattern menu.
Supported resolution are only: QUXGA, 720P, UXGA.

Signed-off-by: Rui Miguel Silva 
---
 drivers/media/i2c/Kconfig  |   12 +
 drivers/media/i2c/Makefile |1 +
 drivers/media/i2c/ov2680.c | 1130 
 3 files changed, 1143 insertions(+)
 create mode 100644 drivers/media/i2c/ov2680.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9f18cd296841..39dc9f236ffa 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -586,6 +586,18 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
 
+config VIDEO_OV2680
+   tristate "OmniVision OV2680 sensor support"
+   depends on GPIOLIB && VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   select V4L2_FWNODE
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV2680 camera sensor with a MIPI CSI-2 interface.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov2680.
+
 config VIDEO_OV5640
tristate "OmniVision OV5640 sensor support"
depends on OF
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index c0f94cd8d56d..d0aba4d37b8d 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
 obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
 obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
 obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
+obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
 obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
 obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
 obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
new file mode 100644
index ..a17971ce71bc
--- /dev/null
+++ b/drivers/media/i2c/ov2680.c
@@ -0,0 +1,1130 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Omnivision OV2680 CMOS Image Sensor driver
+ *
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * Based on OV5640 Sensor Driver
+ * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright (C) 2014-2017 Mentor Graphics Inc.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#define OV2680_XVCLK_MIN   600
+#define OV2680_XVCLK_MAX   2400
+
+#define OV2680_CHIP_ID 0x2680
+
+#define OV2680_REG_STREAM_CTRL 0x0100
+#define OV2680_REG_SOFT_RESET  0x0103
+
+#define OV2680_REG_CHIP_ID_HIGH0x300a
+#define OV2680_REG_CHIP_ID_LOW 0x300b
+
+#define OV2680_REG_R_MANUAL0x3503
+#define OV2680_REG_GAIN_PK 0x350a
+#define OV2680_REG_EXPOSURE_PK_HIGH0x3500
+#define OV2680_REG_TIMING_HTS  0x380c
+#define OV2680_REG_TIMING_VTS  0x380e
+#define OV2680_REG_FORMAT1 0x3820
+#define OV2680_REG_FORMAT2 0x3821
+
+#define OV2680_REG_ISP_CTRL00  0x5080
+
+#define OV2680_FRAME_RATE  30
+
+#define OV2680_REG_VALUE_8BIT  1
+#define OV2680_REG_VALUE_16BIT 2
+#define OV2680_REG_VALUE_24BIT 3
+
+enum ov2680_mode_id {
+   OV2680_MODE_QUXGA_800_600,
+   OV2680_MODE_720P_1280_720,
+   OV2680_MODE_UXGA_1600_1200,
+   OV2680_MODE_MAX,
+};
+
+struct reg_value {
+   u16 reg_addr;
+   u8 val;
+};
+
+struct ov2680_mode_info {
+   const char *name;
+   enum ov2680_mode_id id;
+   u32 width;
+   u32 height;
+   const struct reg_value *reg_data;
+   u32 reg_data_size;
+};
+
+struct ov2680_ctrls {
+   struct v4l2_ctrl_handler handler;
+   struct {
+   struct v4l2_ctrl *auto_exp;
+   struct v4l2_ctrl *exposure;
+   };
+   struct {
+   struct v4l2_ctrl *auto_gain;
+   struct v4l2_ctrl *gain;
+   };
+
+   struct v4l2_ctrl *hflip;
+   struct v4l2_ctrl *vflip;
+   struct v4l2_ctrl *test_pattern;
+};
+
+struct ov2680_dev {
+   struct i2c_client   *i2c_client;
+   struct v4l2_subdev  sd;
+
+   struct media_padpad;
+   struct clk  *xvclk;
+   u32 xvclk_freq;
+
+   struct gpio_desc*pwdn_gpio;
+   struct mutexlock; /* protect members */
+
+   boolmode_pending_changes;
+   boolis_enabled;
+   bool