diff mbox series

[RFC,2/8] media: i2c: ov6650: Drop implementation of .set_mbus_config()

Message ID 20220103162414.27723-3-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: Drop .set_mbus_config(), improve .get_mbus_config() | expand

Commit Message

Laurent Pinchart Jan. 3, 2022, 4:24 p.m. UTC
The subdev .set_mbus_config() operation is deprecated. No code in the
kernel calls it, so drop its implementation from the ov6650 driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/ov6650.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

Comments

Sakari Ailus Jan. 5, 2022, 6:04 p.m. UTC | #1
Hi Laurent,

Thanks for the set.

On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> The subdev .set_mbus_config() operation is deprecated. No code in the
> kernel calls it, so drop its implementation from the ov6650 driver.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/ov6650.c | 37 -------------------------------------
>  1 file changed, 37 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index f67412150b16..455a627e35a0 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -/* Alter bus settings on camera side */
> -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 = 0;
> -
> -	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> -	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> -		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);

I think this configuration should come from the endpoint which the driver
currently does not parse. In fact, there are no even DT bindings for the
device.

I wonder what kind of environment it is used in --- assuming it works
somewhere.

> -	if (ret)
> -		return ret;
> -
> -	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> -		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> -	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> -	if (ret)
> -		return ret;
> -
> -	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> -		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> -	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * Update the configuration to report what is actually applied to
> -	 * the hardware.
> -	 */
> -	return ov6650_get_mbus_config(sd, pad, cfg);
> -}
> -
>  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
>  	.s_stream	= ov6650_s_stream,
>  	.g_frame_interval = ov6650_g_frame_interval,
> @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
>  	.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 = {
Laurent Pinchart Jan. 5, 2022, 8:19 p.m. UTC | #2
Hi Sakari,

On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > The subdev .set_mbus_config() operation is deprecated. No code in the
> > kernel calls it, so drop its implementation from the ov6650 driver.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> >  1 file changed, 37 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index f67412150b16..455a627e35a0 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -/* Alter bus settings on camera side */
> > -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 = 0;
> > -
> > -	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > -		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > -	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > -		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> 
> I think this configuration should come from the endpoint which the driver
> currently does not parse. In fact, there are no even DT bindings for the
> device.

There's also no OF match table. While this isn't strictly required, it
may indicate that the sensor hasn't been tested much on DT-based
systems.

I agree that the configuration should come from the device tree, but I
can't test that, so I'm tempted to let someone else implement it if the
driver is actually still in use (I can also write a patch if someone can
test it).

> I wonder what kind of environment it is used in --- assuming it works
> somewhere.
> 
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > -		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> > -	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > -		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > -		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> > -	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > -		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> > -	if (ret)
> > -		return ret;
> > -
> > -	/*
> > -	 * Update the configuration to report what is actually applied to
> > -	 * the hardware.
> > -	 */
> > -	return ov6650_get_mbus_config(sd, pad, cfg);
> > -}
> > -
> >  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
> >  	.s_stream	= ov6650_s_stream,
> >  	.g_frame_interval = ov6650_g_frame_interval,
> > @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
> >  	.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 = {
Janusz Krzysztofik Jan. 5, 2022, 9:31 p.m. UTC | #3
Hi Laurent,

On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> Hi Sakari,
>
> On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > kernel calls it, so drop its implementation from the ov6650 driver.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > >  1 file changed, 37 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > index f67412150b16..455a627e35a0 100644
> > > --- a/drivers/media/i2c/ov6650.c
> > > +++ b/drivers/media/i2c/ov6650.c
> > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > >   return 0;
> > >  }
> > >
> > > -/* Alter bus settings on camera side */
> > > -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 = 0;
> > > -
> > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> >
> > I think this configuration should come from the endpoint which the driver
> > currently does not parse. In fact, there are no even DT bindings for the
> > device.
>
> There's also no OF match table. While this isn't strictly required, it
> may indicate that the sensor hasn't been tested much on DT-based
> systems.
>
> I agree that the configuration should come from the device tree, but I
> can't test that, so I'm tempted to let someone else implement it if the
> driver is actually still in use (I can also write a patch if someone can
> test it).

This driver was used with omap1_camera, removed from the tree a few years
ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
updated but I gave up due to lack of response to my submissions.  That also
blocked my attempts to rework and reintroduce omap1_camera.

I think I'm still able to update my local (v4l2, non-mc) version of
omap1_camera to the extent required to test any changes to ov6650.
However, the OMAP1 platform does not support DT, and will probably never
do.  Then,  I think that it makes sense to spend my time on that only if
you (media maintainers) are not going to depreciate non-DT support any
soon.  Are you?

Thanks,
Janusz


>
> > I wonder what kind of environment it is used in --- assuming it works
> > somewhere.
> >
> > > - if (ret)
> > > -         return ret;
> > > -
> > > - if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > > -         ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> > > - else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > -         ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> > > - if (ret)
> > > -         return ret;
> > > -
> > > - if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> > > - else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> > > - if (ret)
> > > -         return ret;
> > > -
> > > - /*
> > > -  * Update the configuration to report what is actually applied to
> > > -  * the hardware.
> > > -  */
> > > - return ov6650_get_mbus_config(sd, pad, cfg);
> > > -}
> > > -
> > >  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
> > >   .s_stream       = ov6650_s_stream,
> > >   .g_frame_interval = ov6650_g_frame_interval,
> > > @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
> > >   .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 = {
>
>
Laurent Pinchart Jan. 5, 2022, 9:42 p.m. UTC | #4
Hi Janusz,

On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > >  1 file changed, 37 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > > index f67412150b16..455a627e35a0 100644
> > > > --- a/drivers/media/i2c/ov6650.c
> > > > +++ b/drivers/media/i2c/ov6650.c
> > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > >   return 0;
> > > >  }
> > > >
> > > > -/* Alter bus settings on camera side */
> > > > -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 = 0;
> > > > -
> > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > >
> > > I think this configuration should come from the endpoint which the driver
> > > currently does not parse. In fact, there are no even DT bindings for the
> > > device.
> >
> > There's also no OF match table. While this isn't strictly required, it
> > may indicate that the sensor hasn't been tested much on DT-based
> > systems.
> >
> > I agree that the configuration should come from the device tree, but I
> > can't test that, so I'm tempted to let someone else implement it if the
> > driver is actually still in use (I can also write a patch if someone can
> > test it).
> 
> This driver was used with omap1_camera, removed from the tree a few years
> ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> updated but I gave up due to lack of response to my submissions.  That also
> blocked my attempts to rework and reintroduce omap1_camera.
> 
> I think I'm still able to update my local (v4l2, non-mc) version of
> omap1_camera to the extent required to test any changes to ov6650.
> However, the OMAP1 platform does not support DT, and will probably never
> do.  Then,  I think that it makes sense to spend my time on that only if
> you (media maintainers) are not going to depreciate non-DT support any
> soon.  Are you?

Thank you for the quick reply, and for the offer to test this. I
understand your frustration, and the impossibility (with reasonable
effort) to move OMAP1 to DT. This means that we would need to add
platform data support to the ov6650 driver, and specify the platform
data in the corresponding board file. On the ov6650 driver side I have
no issue with that, and while platform data is deprecated for new
platforms, it can be kept around for older ones as long as needed. I
however don't know if changes to board files in arch/arm/mach-omap1
would be accepted. I also don't see any mention of ov6650 there, leading
me to believe nobody can use this driver with the mainline kernel
without resurecting the omap1_camera driver. I'm thus wondering if this
would be a good use of your time, or if we should just merge this patch
as-is.

> > > I wonder what kind of environment it is used in --- assuming it works
> > > somewhere.
> > >
> > > > - if (ret)
> > > > -         return ret;
> > > > -
> > > > - if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> > > > - else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> > > > - if (ret)
> > > > -         return ret;
> > > > -
> > > > - if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> > > > - else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> > > > - if (ret)
> > > > -         return ret;
> > > > -
> > > > - /*
> > > > -  * Update the configuration to report what is actually applied to
> > > > -  * the hardware.
> > > > -  */
> > > > - return ov6650_get_mbus_config(sd, pad, cfg);
> > > > -}
> > > > -
> > > >  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
> > > >   .s_stream       = ov6650_s_stream,
> > > >   .g_frame_interval = ov6650_g_frame_interval,
> > > > @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
> > > >   .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 = {
Janusz Krzysztofik Jan. 6, 2022, 12:13 a.m. UTC | #5
On Wednesday, 5 January 2022 22:42:38 CET Laurent Pinchart wrote:
> Hi Janusz,
>
> On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> > On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > > The subdev .set_mbus_config() operation is deprecated. No code in
the
> > > > > kernel calls it, so drop its implementation from the ov6650
driver.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov6650.c | 37
-------------------------------------
> > > > >  1 file changed, 37 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/
ov6650.c
> > > > > index f67412150b16..455a627e35a0 100644
> > > > > --- a/drivers/media/i2c/ov6650.c
> > > > > +++ b/drivers/media/i2c/ov6650.c
> > > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct
v4l2_subdev *sd,
> > > > >   return 0;
> > > > >  }
> > > > >
> > > > > -/* Alter bus settings on camera side */
> > > > > -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 = 0;
> > > > > -
> > > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ,
COMJ_PCLK_RISING, 0);
> > > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0,
COMJ_PCLK_RISING);
> > > >
> > > > I think this configuration should come from the endpoint which the
driver
> > > > currently does not parse. In fact, there are no even DT bindings
for the
> > > > device.
> > >
> > > There's also no OF match table. While this isn't strictly required,
it
> > > may indicate that the sensor hasn't been tested much on DT-based
> > > systems.
> > >
> > > I agree that the configuration should come from the device tree, but I
> > > can't test that, so I'm tempted to let someone else implement it if
the
> > > driver is actually still in use (I can also write a patch if someone
can
> > > test it).
> >
> > This driver was used with omap1_camera, removed from the tree a few
years
> > ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> > updated but I gave up due to lack of response to my submissions.  That
also
> > blocked my attempts to rework and reintroduce omap1_camera.
> >
> > I think I'm still able to update my local (v4l2, non-mc) version of
> > omap1_camera to the extent required to test any changes to ov6650.
> > However, the OMAP1 platform does not support DT, and will probably
never
> > do.  Then,  I think that it makes sense to spend my time on that only
if
> > you (media maintainers) are not going to depreciate non-DT support any
> > soon.  Are you?
>
> Thank you for the quick reply, and for the offer to test this. I
> understand your frustration, and the impossibility (with reasonable
> effort) to move OMAP1 to DT.

I forgot to mention one more limitation of OMAP1 platform: it does not
support CCF.  With removal of v4l2-clk support from ov6650 a year ago, the
driver is probably no longer usable with OMAP1 boards.

> This means that we would need to add
> platform data support to the ov6650 driver, and specify the platform
> data in the corresponding board file. On the ov6650 driver side I have
> no issue with that, and while platform data is deprecated for new
> platforms, it can be kept around for older ones as long as needed. I
> however don't know if changes to board files in arch/arm/mach-omap1
> would be accepted.

I think that shouldn't be a problem, I could take care.

> I also don't see any mention of ov6650 there,

Respective i2c_board_info was removed from arch/arm/mach-omap1/board-ams-
delta.c together with removal of soc_camera support.  I had a patch in my
queue that was registering the sensor info with I2C sybsystem at boot time
for v4l2_async use but that alone wouldn't help much.

> leading
> me to believe nobody can use this driver with the mainline kernel
> without resurecting the omap1_camera driver. I'm thus wondering if this
> would be a good use of your time, or if we should just merge this patch
> as-is.

Yes, please feel free to merge it.

Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Thanks,
Janusz


>
> > > > I wonder what kind of environment it is used in --- assuming it
works
> > > > somewhere.
> > > >
> > > > > - if (ret)
> > > > > -         return ret;
> > > > > -
> > > > > - if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW,
0);
> > > > > - else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMF, 0,
COMF_HREF_LOW);
> > > > > - if (ret)
> > > > > -         return ret;
> > > > > -
> > > > > - if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH,
0);
> > > > > - else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0,
COMJ_VSYNC_HIGH);
> > > > > - if (ret)
> > > > > -         return ret;
> > > > > -
> > > > > - /*
> > > > > -  * Update the configuration to report what is actually applied
to
> > > > > -  * the hardware.
> > > > > -  */
> > > > > - return ov6650_get_mbus_config(sd, pad, cfg);
> > > > > -}
> > > > > -
> > > > >  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
> > > > >   .s_stream       = ov6650_s_stream,
> > > > >   .g_frame_interval = ov6650_g_frame_interval,
> > > > > @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops
ov6650_pad_ops = {
> > > > >   .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 = {
>
>
Sakari Ailus Jan. 7, 2022, 1:41 p.m. UTC | #6
Hi Janusz,

On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> Hi Laurent,
> 
> On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > >  1 file changed, 37 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > > index f67412150b16..455a627e35a0 100644
> > > > --- a/drivers/media/i2c/ov6650.c
> > > > +++ b/drivers/media/i2c/ov6650.c
> > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > >   return 0;
> > > >  }
> > > >
> > > > -/* Alter bus settings on camera side */
> > > > -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 = 0;
> > > > -
> > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > >
> > > I think this configuration should come from the endpoint which the driver
> > > currently does not parse. In fact, there are no even DT bindings for the
> > > device.
> >
> > There's also no OF match table. While this isn't strictly required, it
> > may indicate that the sensor hasn't been tested much on DT-based
> > systems.
> >
> > I agree that the configuration should come from the device tree, but I
> > can't test that, so I'm tempted to let someone else implement it if the
> > driver is actually still in use (I can also write a patch if someone can
> > test it).
> 
> This driver was used with omap1_camera, removed from the tree a few years
> ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> updated but I gave up due to lack of response to my submissions.  That also
> blocked my attempts to rework and reintroduce omap1_camera.

My apologies for this --- I indeed to see a set of unreviewed ov6650 patches
from you. Please do ping me if you expect an answer but do not get one.

> 
> I think I'm still able to update my local (v4l2, non-mc) version of
> omap1_camera to the extent required to test any changes to ov6650.
> However, the OMAP1 platform does not support DT, and will probably never
> do.  Then,  I think that it makes sense to spend my time on that only if
> you (media maintainers) are not going to depreciate non-DT support any
> soon.  Are you?

Commenting just this and not the discussion later in this thread --- it is
possible to support such sensor drivers without DT or ACPI nowadays,
through software nodes. See e.g. drivers/media/pci/intel/ipu3/cio2-bridge.c
.
Laurent Pinchart Jan. 9, 2022, 2:37 a.m. UTC | #7
Hi Janusz,

On Thu, Jan 06, 2022 at 01:13:56AM +0100, Janusz Krzysztofik wrote:
> On Wednesday, 5 January 2022 22:42:38 CET Laurent Pinchart wrote:
> > On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> > > On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > > > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > > > >  1 file changed, 37 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ ov6650.c
> > > > > > index f67412150b16..455a627e35a0 100644
> > > > > > --- a/drivers/media/i2c/ov6650.c
> > > > > > +++ b/drivers/media/i2c/ov6650.c
> > > > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > > > >   return 0;
> > > > > >  }
> > > > > >
> > > > > > -/* Alter bus settings on camera side */
> > > > > > -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 = 0;
> > > > > > -
> > > > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > > > >
> > > > > I think this configuration should come from the endpoint which the driver
> > > > > currently does not parse. In fact, there are no even DT bindings for the
> > > > > device.
> > > >
> > > > There's also no OF match table. While this isn't strictly required, it
> > > > may indicate that the sensor hasn't been tested much on DT-based
> > > > systems.
> > > >
> > > > I agree that the configuration should come from the device tree, but I
> > > > can't test that, so I'm tempted to let someone else implement it if the
> > > > driver is actually still in use (I can also write a patch if someone can
> > > > test it).
> > >
> > > This driver was used with omap1_camera, removed from the tree a few years
> > > ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> > > updated but I gave up due to lack of response to my submissions.  That also
> > > blocked my attempts to rework and reintroduce omap1_camera.
> > >
> > > I think I'm still able to update my local (v4l2, non-mc) version of
> > > omap1_camera to the extent required to test any changes to ov6650.
> > > However, the OMAP1 platform does not support DT, and will probably never
> > > do.  Then,  I think that it makes sense to spend my time on that only if
> > > you (media maintainers) are not going to depreciate non-DT support any
> > > soon.  Are you?
> >
> > Thank you for the quick reply, and for the offer to test this. I
> > understand your frustration, and the impossibility (with reasonable
> > effort) to move OMAP1 to DT.
> 
> I forgot to mention one more limitation of OMAP1 platform: it does not
> support CCF.  With removal of v4l2-clk support from ov6650 a year ago, the
> driver is probably no longer usable with OMAP1 boards.

Ouch, that's even worse indeed :-(

> > This means that we would need to add
> > platform data support to the ov6650 driver, and specify the platform
> > data in the corresponding board file. On the ov6650 driver side I have
> > no issue with that, and while platform data is deprecated for new
> > platforms, it can be kept around for older ones as long as needed. I
> > however don't know if changes to board files in arch/arm/mach-omap1
> > would be accepted.
> 
> I think that shouldn't be a problem, I could take care.
> 
> > I also don't see any mention of ov6650 there,
> 
> Respective i2c_board_info was removed from arch/arm/mach-omap1/board-ams-
> delta.c together with removal of soc_camera support.  I had a patch in my
> queue that was registering the sensor info with I2C sybsystem at boot time
> for v4l2_async use but that alone wouldn't help much.
> 
> > leading
> > me to believe nobody can use this driver with the mainline kernel
> > without resurecting the omap1_camera driver. I'm thus wondering if this
> > would be a good use of your time, or if we should just merge this patch
> > as-is.
> 
> Yes, please feel free to merge it.
> 
> Acked-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Thank you.

> > > > > I wonder what kind of environment it is used in --- assuming it works
> > > > > somewhere.
> > > > >
> > > > > > - if (ret)
> > > > > > -         return ret;
> > > > > > -
> > > > > > - if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> > > > > > - else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> > > > > > - if (ret)
> > > > > > -         return ret;
> > > > > > -
> > > > > > - if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> > > > > > - else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> > > > > > - if (ret)
> > > > > > -         return ret;
> > > > > > -
> > > > > > - /*
> > > > > > -  * Update the configuration to report what is actually applied to
> > > > > > -  * the hardware.
> > > > > > -  */
> > > > > > - return ov6650_get_mbus_config(sd, pad, cfg);
> > > > > > -}
> > > > > > -
> > > > > >  static const struct v4l2_subdev_video_ops ov6650_video_ops = {
> > > > > >   .s_stream       = ov6650_s_stream,
> > > > > >   .g_frame_interval = ov6650_g_frame_interval,
> > > > > > @@ -993,7 +957,6 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
> > > > > >   .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 = {
Janusz Krzysztofik Jan. 10, 2022, 6:11 p.m. UTC | #8
Hi Sakari,

On Friday, 7 January 2022 14:41:51 CET Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> > Hi Laurent,
> > 
> > On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > > >  1 file changed, 37 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > > > index f67412150b16..455a627e35a0 100644
> > > > > --- a/drivers/media/i2c/ov6650.c
> > > > > +++ b/drivers/media/i2c/ov6650.c
> > > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > > >   return 0;
> > > > >  }
> > > > >
> > > > > -/* Alter bus settings on camera side */
> > > > > -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 = 0;
> > > > > -
> > > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > > >
> > > > I think this configuration should come from the endpoint which the driver
> > > > currently does not parse. In fact, there are no even DT bindings for the
> > > > device.
> > >
> > > There's also no OF match table. While this isn't strictly required, it
> > > may indicate that the sensor hasn't been tested much on DT-based
> > > systems.
> > >
> > > I agree that the configuration should come from the device tree, but I
> > > can't test that, so I'm tempted to let someone else implement it if the
> > > driver is actually still in use (I can also write a patch if someone can
> > > test it).
> > 
> > This driver was used with omap1_camera, removed from the tree a few years
> > ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> > updated but I gave up due to lack of response to my submissions.  That also
> > blocked my attempts to rework and reintroduce omap1_camera.
> 
> My apologies for this --- I indeed to see a set of unreviewed ov6650 patches
> from you. Please do ping me if you expect an answer but do not get one.

OK, thanks.

> > I think I'm still able to update my local (v4l2, non-mc) version of
> > omap1_camera to the extent required to test any changes to ov6650.
> > However, the OMAP1 platform does not support DT, and will probably never
> > do.  Then,  I think that it makes sense to spend my time on that only if
> > you (media maintainers) are not going to depreciate non-DT support any
> > soon.  Are you?
> 
> Commenting just this and not the discussion later in this thread --- it is
> possible to support such sensor drivers without DT or ACPI nowadays,
> through software nodes. See e.g. drivers/media/pci/intel/ipu3/cio2-bridge.c

Thanks for bringing this possibility to my awareness, I didn't know it 
existed.  AFAICS, I2C sensor drivers like ov6650 can now be provided by 
board files with device properties via i2c_board_info.swnode.

If I find a solution to implement a non-v4l2-clk clock device in 
omap1_camera driver, I'll try to get back to updating ov6650 as time 
permits.

Thanks,
Janusz
Sakari Ailus Jan. 11, 2022, 11:52 a.m. UTC | #9
Hi Janusz,

On Mon, Jan 10, 2022 at 07:11:45PM +0100, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Friday, 7 January 2022 14:41:51 CET Sakari Ailus wrote:
> > Hi Janusz,
> > 
> > On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> > > Hi Laurent,
> > > 
> > > On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > >
> > > > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > > > >  1 file changed, 37 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > > > > index f67412150b16..455a627e35a0 100644
> > > > > > --- a/drivers/media/i2c/ov6650.c
> > > > > > +++ b/drivers/media/i2c/ov6650.c
> > > > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > > > >   return 0;
> > > > > >  }
> > > > > >
> > > > > > -/* Alter bus settings on camera side */
> > > > > > -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 = 0;
> > > > > > -
> > > > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > > > >
> > > > > I think this configuration should come from the endpoint which the driver
> > > > > currently does not parse. In fact, there are no even DT bindings for the
> > > > > device.
> > > >
> > > > There's also no OF match table. While this isn't strictly required, it
> > > > may indicate that the sensor hasn't been tested much on DT-based
> > > > systems.
> > > >
> > > > I agree that the configuration should come from the device tree, but I
> > > > can't test that, so I'm tempted to let someone else implement it if the
> > > > driver is actually still in use (I can also write a patch if someone can
> > > > test it).
> > > 
> > > This driver was used with omap1_camera, removed from the tree a few years
> > > ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> > > updated but I gave up due to lack of response to my submissions.  That also
> > > blocked my attempts to rework and reintroduce omap1_camera.
> > 
> > My apologies for this --- I indeed to see a set of unreviewed ov6650 patches
> > from you. Please do ping me if you expect an answer but do not get one.
> 
> OK, thanks.
> 
> > > I think I'm still able to update my local (v4l2, non-mc) version of
> > > omap1_camera to the extent required to test any changes to ov6650.
> > > However, the OMAP1 platform does not support DT, and will probably never
> > > do.  Then,  I think that it makes sense to spend my time on that only if
> > > you (media maintainers) are not going to depreciate non-DT support any
> > > soon.  Are you?
> > 
> > Commenting just this and not the discussion later in this thread --- it is
> > possible to support such sensor drivers without DT or ACPI nowadays,
> > through software nodes. See e.g. drivers/media/pci/intel/ipu3/cio2-bridge.c
> 
> Thanks for bringing this possibility to my awareness, I didn't know it 
> existed.  AFAICS, I2C sensor drivers like ov6650 can now be provided by 
> board files with device properties via i2c_board_info.swnode.

Looking at i2c_new_client_device(), it only takes a single software node.
For an endpoint at least three are required, meaning you'll need to do that
separately. At least that seems to be the case at the moment.

> 
> If I find a solution to implement a non-v4l2-clk clock device in 
> omap1_camera driver, I'll try to get back to updating ov6650 as time 
> permits.

Looking forward to that. Btw. I've taken the four patches you posted
earlier, rebased them a little and pushed them here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/>

Let me know if they're (not) fine.
Janusz Krzysztofik Jan. 11, 2022, 8:37 p.m. UTC | #10
Hi Sakari,

On Tuesday, 11 January 2022 12:52:38 CET Sakari Ailus wrote:
> Hi Janusz,
> 
> On Mon, Jan 10, 2022 at 07:11:45PM +0100, Janusz Krzysztofik wrote:
> > Hi Sakari,
> > 
> > On Friday, 7 January 2022 14:41:51 CET Sakari Ailus wrote:
> > > Hi Janusz,
> > > 
> > > On Wed, Jan 05, 2022 at 10:31:41PM +0100, Janusz Krzysztofik wrote:
> > > > Hi Laurent,
> > > > 
> > > > On Wednesday, 5 January 2022 21:19:49 CET Laurent Pinchart wrote:
> > > > > Hi Sakari,
> > > > >
> > > > > On Wed, Jan 05, 2022 at 08:04:24PM +0200, Sakari Ailus wrote:
> > > > > > On Mon, Jan 03, 2022 at 06:24:08PM +0200, Laurent Pinchart wrote:
> > > > > > > The subdev .set_mbus_config() operation is deprecated. No code in the
> > > > > > > kernel calls it, so drop its implementation from the ov6650 driver.
> > > > > > >
> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/ov6650.c | 37 -------------------------------------
> > > > > > >  1 file changed, 37 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > > > > > > index f67412150b16..455a627e35a0 100644
> > > > > > > --- a/drivers/media/i2c/ov6650.c
> > > > > > > +++ b/drivers/media/i2c/ov6650.c
> > > > > > > @@ -944,42 +944,6 @@ static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
> > > > > > >   return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > -/* Alter bus settings on camera side */
> > > > > > > -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 = 0;
> > > > > > > -
> > > > > > > - if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> > > > > > > - else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> > > > > > > -         ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> > > > > >
> > > > > > I think this configuration should come from the endpoint which the driver
> > > > > > currently does not parse. In fact, there are no even DT bindings for the
> > > > > > device.
> > > > >
> > > > > There's also no OF match table. While this isn't strictly required, it
> > > > > may indicate that the sensor hasn't been tested much on DT-based
> > > > > systems.
> > > > >
> > > > > I agree that the configuration should come from the device tree, but I
> > > > > can't test that, so I'm tempted to let someone else implement it if the
> > > > > driver is actually still in use (I can also write a patch if someone can
> > > > > test it).
> > > > 
> > > > This driver was used with omap1_camera, removed from the tree a few years
> > > > ago by Hans, despite my attempts to refresh it.  I tried to keep ov6650
> > > > updated but I gave up due to lack of response to my submissions.  That also
> > > > blocked my attempts to rework and reintroduce omap1_camera.
> > > 
> > > My apologies for this --- I indeed to see a set of unreviewed ov6650 patches
> > > from you. Please do ping me if you expect an answer but do not get one.
> > 
> > OK, thanks.
> > 
> > > > I think I'm still able to update my local (v4l2, non-mc) version of
> > > > omap1_camera to the extent required to test any changes to ov6650.
> > > > However, the OMAP1 platform does not support DT, and will probably never
> > > > do.  Then,  I think that it makes sense to spend my time on that only if
> > > > you (media maintainers) are not going to depreciate non-DT support any
> > > > soon.  Are you?
> > > 
> > > Commenting just this and not the discussion later in this thread --- it is
> > > possible to support such sensor drivers without DT or ACPI nowadays,
> > > through software nodes. See e.g. drivers/media/pci/intel/ipu3/cio2-bridge.c
> > 
> > Thanks for bringing this possibility to my awareness, I didn't know it 
> > existed.  AFAICS, I2C sensor drivers like ov6650 can now be provided by 
> > board files with device properties via i2c_board_info.swnode.
> 
> Looking at i2c_new_client_device(), it only takes a single software node.
> For an endpoint at least three are required, meaning you'll need to do that
> separately. At least that seems to be the case at the moment.

Thanks, I'll take a closer look.

> > 
> > If I find a solution to implement a non-v4l2-clk clock device in 
> > omap1_camera driver, I'll try to get back to updating ov6650 as time 
> > permits.
> 
> Looking forward to that. Btw. I've taken the four patches you posted
> earlier, rebased them a little and pushed them here:
> 
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
> 
> Let me know if they're (not) fine.

Should be fine, thank you.

Janusz

> 
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f67412150b16..455a627e35a0 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -944,42 +944,6 @@  static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
 	return 0;
 }
 
-/* Alter bus settings on camera side */
-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 = 0;
-
-	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
-	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
-		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
-	if (ret)
-		return ret;
-
-	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
-		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
-	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
-	if (ret)
-		return ret;
-
-	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
-		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
-	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
-	if (ret)
-		return ret;
-
-	/*
-	 * Update the configuration to report what is actually applied to
-	 * the hardware.
-	 */
-	return ov6650_get_mbus_config(sd, pad, cfg);
-}
-
 static const struct v4l2_subdev_video_ops ov6650_video_ops = {
 	.s_stream	= ov6650_s_stream,
 	.g_frame_interval = ov6650_g_frame_interval,
@@ -993,7 +957,6 @@  static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
 	.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 = {