Re: [PATCH v13 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema

[Vladimir Zapolskiy]

On 7/21/26 16:19, Bryan O'Donoghue wrote:
> On 21/07/2026 12:24, Bryan O'Donoghue wrote:
> > On 21/07/2026 12:05, Vladimir Zapolskiy wrote:
> > > On 7/21/26 13:47, Bryan O'Donoghue wrote:
> > > > On 21/07/2026 10:56, Vladimir Zapolskiy wrote:
> > > > > On 7/21/26 12:39, Bryan O'Donoghue wrote:
> > > > > > On 21/07/2026 09:17, Vladimir Zapolskiy wrote:
> > > > > > > > +  "#phy-cells":
> > > > > > > > +    const: 1
> > > > > > > > +    description:
> > > > > > > > +      The single cell specifies the PHY operating mode.
> > > > > > > This has been reported before, #phy-cells shall be 0, the operation
> > > > > > > mode is selected by bus-type of endpoints as it is described in
> > > > > > > video-interfaces.yaml
> > > > > > >
> > > > > > > Two competing values are redundant and it opens a door to a wrong
> > > > > > > hardware description, the technical discussion has not been started
> > > > > > > yet.
> > > > > > So as I said before, I'm following Rob Herring's input on this and
> > > > > > Krzysztof has given RB for this binding. At this stage, I don't see
> > > > > > scope to change it.
> > > > > >
> > > > > > Adding endpoints was already a compromise to address your feedback,
> > > > > > which I hoped would bring you on board with the design.
> > > > > That's my comment dated March 27, 2026, and it says moving phy type
> > > > > from phy cell to the endpoint property excludes the need to set any
> > > > > phy cells:
> > > > >
> > > > > https://lore.kernel.org/linux-arm-msm/e37ce438-12c7-462d-
> > > > > b2bc-1351be62b806@xxxxxxxxxx/
> > > > >
> > > > > The technical comment is left unresponded so far since then.
> > > > >
> > > > > Since there is no expected phy cells to get on consumer's side, and
> > > > > since there is another link between CSIPHY and CSID described in
> > > > > video-interfaces.yaml, any kind of dependency of this IP on phy can
> > > > > be just removed, which serves the purpose of hardware description
> > > > > simplification.
> > > >
> > > > Rob's v1 feedback was consumer-decides-mode via the cell; Krzysztof has
> > > > R-b'd the binding with cells=1.
> > >
> > > Unfortunately this is not a technical discussion to move forward
> > > a better hardware description.
> > >
> > > > I'm happy to lock down this binding as-is on that basis.
> > > >
> > > > Are you arguing about phy-cells or are you actually arguing the entire
> > > > instantiation of Qualcomm MIPI CSI2 as a standalone generic driver in
> > > > drivers/phy ? Its not the same debate at all.
> > >
> > > So far I don't review the driver, only CSIPHY IP hardware description.
> > >
> > > In the current version of the hardware description one may find that
> > > the exploited 'video-intefaces' interface completely covers the 'phys'
> > > interface between CSIPHY and CSID, quite probably both are not needed
> > > at the same time, and the simpler 'phys' can be removed without any
> > > losses. But if it is kept, then its cell value is anyway redundant.
> > >
> > > > As the CAMSS maintainer, I'm fully against adding more inline monolithic
> > > > CSIPHY init sequences. That could be represented as a separate
> > > > linux-media driver but, then that implies that _all_ upstream CSIPHY
> > > > drivers in drivers/phy are wrong, and I just don't accept that.
> > >
> > > Other two drivers are different and should be excluded as good
> > > references, both of them links exactly one sensor to exactly one
> > > CSI-2 decoder, it's not the case here. Also one of these two drivers
> > > has no upstream users.
> > >
> > > > The bindings have DT maintainer RB, and the driver has Qcom engineer
> > > > tested-by, review-by.
> > > >
> > > > Vinod, Kishon - Again, I'd request this merges as-is.
> >
> > The graph and phys are different layers, endpoints describe the
> > media topology, phys is how CSID controls the PHY through the PHY
> > framework (phy_get/configure/power_on).
> >
> > Removing phys doesn't simplify the description, it removes the control
> > interface which is the monolith again.
> >
> > We have multiple examples of this model upstream, cdns-dphy-rx +
> > ti-csi2rx in-tree, with users.
> >
> > I'm still kicking this up to Vinod, Kishon, request stands.
> >
> > Multiple DT and driver users are RB/Tested-by on the schema and driver,
> > lots of other work is gated.
> >
> > It is time to merge.
> >
> > Please adjudicate.
> >
> > ---
> > bod
>
> Had a brief discussion with Vlad, Neil and Vinod
>
> Resolution:
>
> - phys = <> retained

It should suffice as an optional property, moreover any kind of
phy interface usage is not supposed to be done from CSID driver.

> - phy-cells = <0>
> - bus-type from the media-controller
>     Already specified with camss still aligns with Rob's declaration
>     "the consumer defines the mode" so I'm comfortable with this
>     compromise position.
>     This will still facilitate CPHY work/mode-selection via
>     phy_ops->configure() with how to define the CPHY part TBD.

As it was discussed there should be no phy specific interfaces
between this phy driver and CAMSS driver.

> - data-lanes = <0 1 2 3> valid

Would be nice to get an ack from linux-media maintainers on it.

> - MAX_LANES 8 instead of MAX_DATA_LANES 7

Since 'data-lanes' value follows some register bits and not
the actual PCB lanes as in video-intefaces.yaml, this shall be
explicitly described in the CSIPHY dt binding documentation.

> - v14 with these changes shortly.

--
Best wishes,
Vladimir