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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Cc: "Sakari Ailus" <sakari.ailus@iki.fi>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	linux-renesas-soc@vger.kernel.org,
	"Hans Verkuil" <hverkuil-cisco@xs4all.nl>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Tomi Valkeinen" <tomi.valkeinen@ideasonboard.com>
Subject: Re: [RFC PATCH 2/8] media: i2c: ov6650: Drop implementation of .set_mbus_config()
Date: Wed, 5 Jan 2022 23:42:38 +0200	[thread overview]
Message-ID: <YdYQztLGsEAqL6l+@pendragon.ideasonboard.com> (raw)
In-Reply-To: <CAGfqbt5ZyVAjCggqmQxp+2028Yaz+e=O6RqkfWH6LpDBm_MsSA@mail.gmail.com>

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

-- 
Regards,

Laurent Pinchart