diff mbox series

[v5,03/10] media: i2c: ov6650: Use new [get|set]_mbus_config ops

Message ID 20200616141244.49407-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 16, 2020, 2:12 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

Jacopo Mondi June 18, 2020, 1:21 p.m. UTC | #1
Hi Janusz,
On Tue, Jun 16, 2020 at 04:12:38PM +0200, Jacopo Mondi wrote:
> 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.

As the original author of this driver, are you still looking after it ?
Do you have any opinion on this patch/series? I should have cc-ed you
from the beginning probably, but get_maintainer didn't point to you :)

Thanks
   j

>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index 91906b94f978..d2e7a8556ed7 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 = {
> --
> 2.27.0
>
Janusz Krzysztofik June 21, 2020, 11:38 a.m. UTC | #2
Hi Jacopo,

Thanks for bringing my attention to this patch.

On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote:
> 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.

Assuming that was in line with officially supported semantics of the old API, 
not a misinterpretation, I would really like to see that limitation of the new 
API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it.

> 
> Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY
> 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(-)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index 91906b94f978..d2e7a8556ed7 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);

You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH.

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

Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for 
only one of mutually exclusive flags specified by user?

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

Populating cfg->flags by ov6650_get_mbus_config() without checking for a 
potential error it may return can result in invalid data silently returned to 
user.  Maybe it would be better to fetch current hardware status first, fail on 
error, then update the result with successfully performed hardware state 
modifications.

Thanks,
Janusz

> +
>  	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 = {
>
Jacopo Mondi June 24, 2020, 8:11 a.m. UTC | #3
Hello Janusz,
   thanks for your quick reply

On Sun, Jun 21, 2020 at 01:38:46PM +0200, Janusz Krzysztofik wrote:
> Hi Jacopo,
>
> Thanks for bringing my attention to this patch.
>
> On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote:
> > 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.
>
> Assuming that was in line with officially supported semantics of the old API,
> not a misinterpretation, I would really like to see that limitation of the new
> API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it.
>

I'm not sure this is a limitation, it's more by design that the new
get_mbus_config() only reports the current configuration.

To be honest, compared to the other users of the old g_mbus_config()
this driver was the only one implementing the operation in this way,
maybe as it's the sole user of s_mbus_config() left out of staging ?

I would however consider supporting FORMAT_TRY even if I'm not
actually sure if fully makes sense. For the format operations
(get/set_format()) FORMAT_TRY is used for concurrent applications to
test a format without stepping on each other toes.
get|set_mbus_config() are kAPI only, and I'm not sure we need to stay
safe against concurrent configuration attempts... I'll think about
this a bit more. Seems a development that could go on top, right ?

> >
> > Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY
> > 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(-)
> >
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index 91906b94f978..d2e7a8556ed7 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);
>
> You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH.
>

Indeed I did :/

Thanks for spotting

> >  	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)
>
> Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for
> only one of mutually exclusive flags specified by user?
>

Good question, but I wonder if this shouldn't be an accepted
behaviour. The caller can provide all settings it want to allow the
callee to chose which one to apply. The operation returns what has
been actually applied by the callee, so that the caller can adjust
itself to what the callee chose.

Alternatively, it's up to the caller to specify its preference without
mutually exclusive options, and the callee tries to adjust to what has
been requested. Also in this case the operation returns what has
actually been applied, so the caller can later adjust if it could.

Seems like a small difference, but it might be good to exapnd the
operations description to describe this to avoid each single
implementer going in slightly different directions ?

> >  		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);
>
> Populating cfg->flags by ov6650_get_mbus_config() without checking for a
> potential error it may return can result in invalid data silently returned to
> user.  Maybe it would be better to fetch current hardware status first, fail on
> error, then update the result with successfully performed hardware state
> modifications.

I'm not sure I got what you mean 8)

Would if be enough to check for the return value of
ov6650_get_mbus_config() (or actually returning it directly at the end
of this function).

Thanks
   j

>
> Thanks,
> Janusz
>
> > +
> >  	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 = {
> >
>
>
>
>
Hans Verkuil June 24, 2020, 9:55 a.m. UTC | #4
On 24/06/2020 10:11, Jacopo Mondi wrote:
> Hello Janusz,
>    thanks for your quick reply
> 
> On Sun, Jun 21, 2020 at 01:38:46PM +0200, Janusz Krzysztofik wrote:
>> Hi Jacopo,
>>
>> Thanks for bringing my attention to this patch.
>>
>> On Tuesday, June 16, 2020 4:12:38 P.M. CEST Jacopo Mondi wrote:
>>> 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.
>>
>> Assuming that was in line with officially supported semantics of the old API,
>> not a misinterpretation, I would really like to see that limitation of the new
>> API actually compensated with V4L2_SUBDEV_FORMAT_TRY support added to it.
>>
> 
> I'm not sure this is a limitation, it's more by design that the new
> get_mbus_config() only reports the current configuration.

Right. The old behavior was a left-over from the soc-camera driver that
required that. But soc-camera is from pre-device-tree times, and today this
information should come from the device tree.

> 
> To be honest, compared to the other users of the old g_mbus_config()
> this driver was the only one implementing the operation in this way,
> maybe as it's the sole user of s_mbus_config() left out of staging ?
> 
> I would however consider supporting FORMAT_TRY even if I'm not
> actually sure if fully makes sense. For the format operations
> (get/set_format()) FORMAT_TRY is used for concurrent applications to
> test a format without stepping on each other toes.
> get|set_mbus_config() are kAPI only, and I'm not sure we need to stay
> safe against concurrent configuration attempts... I'll think about
> this a bit more. Seems a development that could go on top, right ?

I wouldn't do this unless it turns out to be actually needed.

Regards,

	Hans

> 
>>>
>>> Adapt the driver to support the semantic of the two newly introducedV4L2_SUBDEV_FORMAT_TRY
>>> 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(-)
>>>
>>> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
>>> index 91906b94f978..d2e7a8556ed7 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);
>>
>> You probably missed hardware default V4L2_MBUS_DATA_ACTIVE_HIGH.
>>
> 
> Indeed I did :/
> 
> Thanks for spotting
> 
>>>  	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)
>>
>> Have you thought of extending v4l2_subdev_call_pad_wrappers with a check for
>> only one of mutually exclusive flags specified by user?
>>
> 
> Good question, but I wonder if this shouldn't be an accepted
> behaviour. The caller can provide all settings it want to allow the
> callee to chose which one to apply. The operation returns what has
> been actually applied by the callee, so that the caller can adjust
> itself to what the callee chose.
> 
> Alternatively, it's up to the caller to specify its preference without
> mutually exclusive options, and the callee tries to adjust to what has
> been requested. Also in this case the operation returns what has
> actually been applied, so the caller can later adjust if it could.
> 
> Seems like a small difference, but it might be good to exapnd the
> operations description to describe this to avoid each single
> implementer going in slightly different directions ?
> 
>>>  		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);
>>
>> Populating cfg->flags by ov6650_get_mbus_config() without checking for a
>> potential error it may return can result in invalid data silently returned to
>> user.  Maybe it would be better to fetch current hardware status first, fail on
>> error, then update the result with successfully performed hardware state
>> modifications.
> 
> I'm not sure I got what you mean 8)
> 
> Would if be enough to check for the return value of
> ov6650_get_mbus_config() (or actually returning it directly at the end
> of this function).
> 
> Thanks
>    j
> 
>>
>> Thanks,
>> Janusz
>>
>>> +
>>>  	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 = {
>>>
>>
>>
>>
>>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 91906b94f978..d2e7a8556ed7 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 = {