Re: [PATCH v5 00/24] media: i2c: add Maxim GMSL2/3 serializer and deserializer drivers

[Cosmin Tanislav <demonsingur@gmail.com>]

On 7/3/25 4:54 PM, Julien Massot wrote:
> On Thu, 2025-07-03 at 15:17 +0300, Cosmin Tanislav wrote:
>>
>>
>> On 7/3/25 3:07 PM, Julien Massot wrote:
>>> Hi Cosmin,
>>>
>>> On Wed, 2025-07-02 at 16:20 +0300, Cosmin Tanislav wrote:
>>>> This series adds new drivers for multiple Maxim GMSL2 and GMSL3 devices,
>>>> replacing the few GMSL2 drivers already in upstream, and introducing a
>>>> common framework that can be used to implement such GMSL chips, which
>>>> avoids code duplication while also adding support for previously
>>>> unsupported features.
>>>>
>>>> While the normally acceptable and polite way would be to extend the
>>>> current mainline drivers, the choice was made here to add a totally new
>>>> set of drivers. The current drivers support only a small subset of the
>>>> possible features, and only a few devices, so the end result after
>>>> extending them would in any case be essentially fully rewritten, new
>>>> drivers.
>>>>
>>> Thanks for your work,
>>> The common framework will help a lot to drive new GMSL chips, and most of the
>>> features are covered.
>>>
>>>> This series depends on support for internal pads, for which a patch has
>>>> been added.
>>>>
>>>> The previous version is at:
>>>> https://lore.kernel.org/lkml/20250618095858.2145209-1-demonsingur@gmail.com
>>>>
>>>> The following deserializers are supported:
>>>>    * MAX96712 (already exists in staging)
>>>>    * MAX96714 (already exists)
>>>>    * MAX96714F (already exists)
>>>>    * MAX96714R (GMSL2)
>>>>    * MAX96716 (GMSL2)
>>>>    * MAX96724 (already exists as part of existing MAX96712 driver)
>>>>    * MAX96724F (GMSL2)
>>>>    * MAX96724R (GMSL2)
>>>>    * MAX9296A (GMSL2)
>>>>    * MAX96792A (GMSL3)
>>>>
>>>> The following serializers are supported:
>>>>    * MAX96717 (already exists)
>>>>    * MAX9295A (GMSL2)
>>>>    * MAX96793 (GMSL3)
>>>>
>>>> Known backward compatibility breakages:
>>>>    * No default routing. Default routing has been intentionally ommitted
>>>>      as the devices support quite complex routing and it would be
>>>>      unfeasible to provide sane defaults for multi-link deserialziers.
>>>>      It is expected that userspace programs would set appropritate
>>>>      routing.
>>>>
>>> This part is the most annoying one: at the moment, there is no way to set the routing except by
>>> manually enabling a boolean within the kernel source.
>>> You can't guess what routing the user really wants, but please at least provide a default
>>> routing
>>> table that allows using your drivers — for example, the device's default routing.
>>>
>>
>> It's a very delicate issue... I'll try to see if I can do that.
>> It would be great if we could enable the streams API globally since it's
>> been merged since Jan 15 2023. It's been over two years.
>>
>>
>> Thanks,
>>
>>>
>>>
>>>> The following list enumerates new features that are supported by the
>>>> common framework and their respective chip-specific drivers:
>>>>    * Full Streams API support. Most deserializers have support for more
>>>>      than one link, and more than one PHY. Streams support allows
>>>>      configuration of routing between these links and PHYs.
>>>>
>>>>    * .get_frame_desc() support. Both the serializers and deserializers
>>>>      implement this to query and provide frame descriptor data. This is
>>>>      used in features explained in-depth below.
>>>
>>> So are almost all the sensor drivers incompatible?
>>>
>>
>> Yes, sensor drivers need to have .get_frame_desc() implemented... It's
>> not a huge feat and it's the only way this type of bridge could work
>> properly.
>>
>> Alternatively, we could add a fallback that bases its decision on the
>> stream format, but I'd prefer if we didn't and we would just implement
>> .get_frame_desc(). After this series is merged I can submit my patches
>> for imx219.
> There is already one pending on the mailing list
> "media: i2c: imx219: Report streams using frame descriptors"
> I guess it's fine if we require the sensor to implement this function.
> 
> But I had to do it for vgxy61.
> 
> Btw I tested:
> TI AM62x + max96716 + 1 x max96717f + stvg5661 (tunnel mode)
> With special lanes mapping and polarities.
> 
> And I had to:
> 
> - Apply pending patches for j721e to support the enable_stream API instead of s_stream
> - Enable the experimental v4l2_subdev_enable_streams_api
> - Add get_frame_desc to the sensor driver

Did it work without issues with those changes?

If that's the case then I think all I can do to make it is easier is to
add default routing. I'm not a fan of each driver having its own
get_frame_desc() fallback, especially when it's a very small change to
do to the sensor drivers.