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

[<Eugen.Hristev@microchip.com>]

On 2/8/22 1:06 PM, Laurent Pinchart wrote:
> 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 ;-)

Hello Laurent,

I am sorry, I was referring to the get_mbus_config which was recently 
added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* .
Without the get_mbus_config, I wasn't supposed to use the 
V4L2_MBUS_CSI2_CHANNEL_* at all .
Hence my confusion in adding them into the same bucket.

My driver uses these flags because my initial implementation to get the 
VC from the devicetree was rejected and this was suggested instead.

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

In that case then I suppose the support in the csi2dc driver has to be 
rewritten

> 
>> 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.

I still disagree that these should be removed before the patch you 
mentioned earlier about adding 'subdev internal routing and streams' is 
applied. I see things in the way that after the new support is 
available, old flags , even if unused, can be removed.
You can disagree, of course.

Thank you for sharing your thoughts,
Eugen

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