Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now

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

Hi Eugen,

On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@microchip.com 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@microchip.com>
> > 
> > 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@ideasonboard.com

> 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@linux.intel.com>
> >> ---
> >>   .../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