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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 = {

-- 
Regards,

Laurent Pinchart