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

[Julien Massot]

On Thu, 2025-07-03 at 17:46 +0300, Cosmin Tanislav wrote:
> 
> 
> 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@xxxxxxxxx
> > > > > 
> > > > > 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?
> 
Yes :)
btw you can add:
Tested-by: Julien Massot <julien.massot@xxxxxxxxxxxxx>
> 
> 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.
Ok for me.

Ragards,
-- 
Julien