Hi Eugen, On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote: > On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: > > Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus escreveu: > > > >> As part of removing mbus config flags, remove VC flag use in the > >> microchip-csi2dc driver. The support can be reintroduced later on as part > >> of the streams patches. > >> > >> Cc: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> > > > > Hmm... that sounds a regression to me. What effects this will cause at > > the driver? > > > > Eugen, any comments? > > Hi , > > I am not happy with this change. It looks like I wasn't even CC-ed on > the original patch e-mail. > > The effect on the driver will be that everything will be treated as > virtual channel=0 . I don't think there's any risk of regression, as we have no driver setting any of the V4L2_MBUS_CSI2_CHANNEL_[123] flags in the kernel. > I do not yet understand why we are about to remove > V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. Those flags were added in 2011. If you think of that as "just introduced" then I understand why you would be unhappy about "sudden changes" mentioned below ;-) > Is there any alternative in place ? Virtual channels have never been properly supported in V4L2. This is going to change with "[PATCH v10 00/38] v4l: subdev internal routing and streams" ([1]). [1] https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@xxxxxxxxxxxxxxxx > My opinion is that if we want to replace something existing with a new > API or something else, we should first add the new support, block any > new adopters for the old API such that everyone uses the new API, and > only after that convert the old API clients to the new API. > So 'can be reintroduced later on' is not okay. We can't remove things in > the hope that it would be reintroduced later. Just my personal take on > this, feel free to have a different opinion. When regressions are introduced this makes sense, but here we're dropping a feature that isn't used as no kernel driver selects a VC different than 0. > In the end you guys are the maintainers for the subsystem and can have > this change if you like, I am more unhappy about the fact that changes > happen suddenly and without notice. > > >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > >> --- > >> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- > >> 1 file changed, 2 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >> index 6bc549c28e05..6a7f5b4b0e3b 100644 > >> --- a/drivers/media/platform/atmel/microchip-csi2dc.c > >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >> if (ret == -ENOIOCTLCMD) { > >> dev_dbg(csi2dc->dev, > >> "no remote mbus configuration available\n"); > >> - goto csi2dc_get_mbus_config_defaults; > >> + return 0; > >> } > >> > >> if (ret) { > >> dev_err(csi2dc->dev, > >> "failed to get remote mbus configuration\n"); > >> - goto csi2dc_get_mbus_config_defaults; > >> + return 0; > >> } > >> > >> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >> - csi2dc->vc = 0; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >> - csi2dc->vc = 1; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >> - csi2dc->vc = 2; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >> - csi2dc->vc = 3; > >> - > >> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >> > >> csi2dc->clk_gated = mbus_config.flags & > >> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >> csi2dc->clk_gated ? "gated" : "free running"); > >> > >> return 0; > >> - > >> -csi2dc_get_mbus_config_defaults: > >> - csi2dc->vc = 0; /* Virtual ID 0 by default */ > >> - > >> - return 0; > >> } > >> > >> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) -- Regards, Laurent Pinchart
