mbox series

[v2,00/12] Extensions to ov8865 driver

Message ID 20210809225845.916430-1-djrscally@gmail.com
Headers show
Series Extensions to ov8865 driver | expand

Message

Daniel Scally Aug. 9, 2021, 10:58 p.m. UTC 
Hello all

This series extends the ov8865 driver with:

* Support for binding to ACPI enumerated devices.
* Support for a 19.2MHz clock in addition to existing 24MHz clock support
* Another v4l2_subdev_pad_ops callback
* 4 more V4L2 controls
* makes the driver supported by the cio2-bridge

There's also a little bit of tidying that I did along the way.

The series is tested on an MS Surface Go 2.

Thanks
Dan

Series changes since V2:

	- Dropped a patch removing unused Macros from the driver.

Daniel Scally (12):
  media: i2c: Add ACPI support to ov8865
  media: i2c: Fix incorrect value in comment
  media: i2c: Defer probe if not endpoint found
  media: i2c: Support 19.2MHz input clock in ov8865
  media: i2c: Add .get_selection() support to ov8865
  media: i2c: Switch control to V4L2_CID_ANALOGUE_GAIN
  media: i2c: Add vblank control to ov8865
  media: i2c: Add hblank control to ov8865
  media: i2c: cap exposure at height + vblank in ov8865
  media: i2c: Add controls from fwnode to ov8865
  media: i2c: Switch exposure control unit to lines
  media: ipu3-cio2: Add INT347A to cio2-bridge

 drivers/media/i2c/ov8865.c                 | 334 +++++++++++++++++----
 drivers/media/pci/intel/ipu3/cio2-bridge.c |   2 +
 2 files changed, 282 insertions(+), 54 deletions(-)

Comments

Andy Shevchenko Aug. 10, 2021, 12:57 p.m. UTC | #1 
On Tue, Aug 10, 2021 at 1:58 AM Daniel Scally <djrscally@gmail.com> wrote:
>

> The ov8865 sensor is sometimes found on x86 platforms enumerated via ACPI.

> Add an ACPI match table to the driver so that it's probed on those

> platforms.


...

> +static const struct acpi_device_id ov8865_acpi_match[] = {

> +       {"INT347A"},

> +       {},


No comma for terminator entry.

> +};


-- 
With Best Regards,
Andy Shevchenko
Sakari Ailus Aug. 10, 2021, 1:38 p.m. UTC | #2 
Hi Daniel,

On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:
> The ov8865 driver's v4l2_subdev_pad_ops currently does not include

> .get_selection() - add support for that callback.


Could you use the same for .set_selection()? Even if it doesn't change
anything.

-- 
Sakari Ailus
Sakari Ailus Aug. 10, 2021, 2:30 p.m. UTC | #3 
Hi Daniel,

On Mon, Aug 09, 2021 at 11:58:42PM +0100, Daniel Scally wrote:
> Exposure limits depend on the total height; when vblank is altered (and

> thus the total height is altered), change the exposure limits to reflect

> the new cap.

> 

> Signed-off-by: Daniel Scally <djrscally@gmail.com>

> ---

> Changes in v2:

> 

> 	- None

> 

>  drivers/media/i2c/ov8865.c | 24 ++++++++++++++++++++++--

>  1 file changed, 22 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c

> index db84294b7a03..70747552e32a 100644

> --- a/drivers/media/i2c/ov8865.c

> +++ b/drivers/media/i2c/ov8865.c

> @@ -675,6 +675,7 @@ struct ov8865_ctrls {

>  	struct v4l2_ctrl *pixel_rate;

>  	struct v4l2_ctrl *hblank;

>  	struct v4l2_ctrl *vblank;

> +	struct v4l2_ctrl *exposure;

>  

>  	struct v4l2_ctrl_handler handler;

>  };

> @@ -2461,6 +2462,18 @@ static int ov8865_s_ctrl(struct v4l2_ctrl *ctrl)

>  	unsigned int index;

>  	int ret;

>  

> +	/* If VBLANK is altered we need to update exposure to compensate */

> +	if (ctrl->id == V4L2_CID_VBLANK) {

> +		int exposure_max;

> +

> +		exposure_max = sensor->state.mode->output_size_y + ctrl->val;

> +		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,

> +					 sensor->ctrls.exposure->minimum,

> +					 exposure_max,

> +					 sensor->ctrls.exposure->step,

> +					 min(sensor->ctrls.exposure->val, exposure_max));

> +	}

> +

>  	/* Wait for the sensor to be on before setting controls. */

>  	if (pm_runtime_suspended(sensor->dev))

>  		return 0;

> @@ -2517,8 +2530,8 @@ static int ov8865_ctrls_init(struct ov8865_sensor *sensor)

>  

>  	/* Exposure */

>  

> -	v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16, 1048575, 16,

> -			  512);

> +	ctrls->exposure = v4l2_ctrl_new_std(handler, ops, V4L2_CID_EXPOSURE, 16,

> +					    1048575, 16, 512);

>  

>  	/* Gain */

>  

> @@ -2699,6 +2712,7 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,

>  	u32 mbus_code = 0;

>  	unsigned int hblank;

>  	unsigned int index;

> +	int exposure_max;

>  	int ret = 0;

>  

>  	mutex_lock(&sensor->mutex);

> @@ -2746,6 +2760,12 @@ static int ov8865_set_fmt(struct v4l2_subdev *subdev,

>  	__v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank, 1,

>  				 hblank);

>  

> +	exposure_max = mode->vts;

> +	__v4l2_ctrl_modify_range(sensor->ctrls.exposure,

> +				 sensor->ctrls.exposure->minimum, exposure_max,

> +				 sensor->ctrls.exposure->step,

> +				 min(sensor->ctrls.exposure->val, exposure_max));


Please wrap lines over 80 (unless there's a sound reason not to).

> +

>  complete:

>  	mutex_unlock(&sensor->mutex);

>  


-- 
Sakari Ailus
Daniel Scally Aug. 24, 2021, 11:17 p.m. UTC | #4 
Hi Sakari - sorry delayed reply

On 10/08/2021 14:38, Sakari Ailus wrote:
> Hi Daniel,

>

> On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:

>> The ov8865 driver's v4l2_subdev_pad_ops currently does not include

>> .get_selection() - add support for that callback.

> Could you use the same for .set_selection()? Even if it doesn't change

> anything.



You mean do the same? Or use the same function?
Sakari Ailus Aug. 25, 2021, 7:16 a.m. UTC | #5 
On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:
> Hi Sakari - sorry delayed reply

> 

> On 10/08/2021 14:38, Sakari Ailus wrote:

> > Hi Daniel,

> >

> > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:

> >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include

> >> .get_selection() - add support for that callback.

> > Could you use the same for .set_selection()? Even if it doesn't change

> > anything.

> 

> 

> You mean do the same? Or use the same function?


The same function. If the selection isn't changeable anyway, the
functionality is the same for both.

-- 
Sakari Ailus
Laurent Pinchart Aug. 25, 2021, 8:04 a.m. UTC | #6 
On Wed, Aug 25, 2021 at 10:16:02AM +0300, Sakari Ailus wrote:
> On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:

> > Hi Sakari - sorry delayed reply

> > 

> > On 10/08/2021 14:38, Sakari Ailus wrote:

> > > Hi Daniel,

> > >

> > > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:

> > >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include

> > >> .get_selection() - add support for that callback.

> > > Could you use the same for .set_selection()? Even if it doesn't change

> > > anything.

> > 

> > You mean do the same? Or use the same function?

> 

> The same function. If the selection isn't changeable anyway, the

> functionality is the same for both.


Except that .s_selection() should return an error if you try to set the
bounds or default rectangles.

-- 
Regards,

Laurent Pinchart
Sakari Ailus Aug. 25, 2021, 8:29 a.m. UTC | #7 
On Wed, Aug 25, 2021 at 11:04:02AM +0300, Laurent Pinchart wrote:
> On Wed, Aug 25, 2021 at 10:16:02AM +0300, Sakari Ailus wrote:

> > On Wed, Aug 25, 2021 at 12:17:15AM +0100, Daniel Scally wrote:

> > > Hi Sakari - sorry delayed reply

> > > 

> > > On 10/08/2021 14:38, Sakari Ailus wrote:

> > > > Hi Daniel,

> > > >

> > > > On Mon, Aug 09, 2021 at 11:58:38PM +0100, Daniel Scally wrote:

> > > >> The ov8865 driver's v4l2_subdev_pad_ops currently does not include

> > > >> .get_selection() - add support for that callback.

> > > > Could you use the same for .set_selection()? Even if it doesn't change

> > > > anything.

> > > 

> > > You mean do the same? Or use the same function?

> > 

> > The same function. If the selection isn't changeable anyway, the

> > functionality is the same for both.

> 

> Except that .s_selection() should return an error if you try to set the

> bounds or default rectangles.


That would make sense. But it's not documented. And in any case it should
be implemented in the framework. :-)

-- 
Sakari Ailus