Re: [PATCH v4 5/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

[Richard Acayan <mailingradian@gmail.com>]

On Tue, Dec 16, 2025 at 05:23:53PM +0200, Vladimir Zapolskiy wrote:
> On 12/16/25 16:41, Konrad Dybcio wrote:
> > On 12/16/25 3:31 PM, Vladimir Zapolskiy wrote:
> > > On 12/16/25 15:56, Konrad Dybcio wrote:
> > > > On 12/12/25 8:22 PM, Dmitry Baryshkov wrote:
> > > > > On Thu, Dec 11, 2025 at 07:41:37PM -0500, Richard Acayan wrote:
> > > > > > On Thu, Dec 11, 2025 at 07:16:30AM +0200, Dmitry Baryshkov wrote:
> > > > > > > On Wed, Dec 10, 2025 at 08:48:46PM -0500, Richard Acayan wrote:
> > > > > > > > The Sony IMX355 is the front camera on the Pixel 3a, mounted in portrait
> > > > > > > > mode. It is connected to CSIPHY1 and CCI I2C1, and uses MCLK2. Add
> > > > > > > > support for it.
> > > > > > > > 
> > > > > > > > Co-developed-by: Robert Mader <robert.mader@collabora.com>
> > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com>
> > > > > > > > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > > > > > > > ---
> > > > > > > >    .../boot/dts/qcom/sdm670-google-sargo.dts     | 107 ++++++++++++++++++
> > > > > > > >    1 file changed, 107 insertions(+)
> > > > > > > > 
> > > > > > > > @@ -392,6 +420,64 @@ vreg_bob: bob {
> > > > > > > >        };
> > > > > > > >    };
> > > > > > > >    +&camss {
> > > > > > > > +    vdda-phy-supply = <&vreg_l1a_1p225>;
> > > > > > > > +    vdda-pll-supply = <&vreg_s6a_0p87>;
> > > > > > > > +
> > > > > > > > +    status = "okay";
> > > > > > > > +
> > > > > > > > +    ports {
> > > > > > > > +        port@1 {
> > > > > > > > +            camss_endpoint1: endpoint {
> > > > > > > > +                clock-lanes = <7>;
> > > > > > > > +                data-lanes = <0 1 2 3>;
> > > > > > > > +                remote-endpoint = <&cam_front_endpoint>;
> > > > > > > > +            };
> > > > > > > > +        };
> > > > > > > > +    };
> > > > > > > 
> > > > > > > This would be much better:
> > > > > > > 
> > > > > > >     &camss_endpoint1: {
> > > > > > >         clock-lanes, data-lanes, remote-endpoint here
> > > > > > >     };
> > > > > > 
> > > > > > I'm not sure what you mean, there might be some typo.
> > > > > 
> > > > > My point is that you are duplicating `ports { port@1 {.... }; };` from
> > > > > the base DTSI here.  We usually try to avoid this kind of path
> > > > > duplication. If you can't have an empty endpoint in the base DTSI, I
> > > > > suggest adding necessary labels to port@N nodes and then extending those
> > > > > nodes in the board DTSI.
> > > > > 
> > > > > > If this is about using the commonly-defined endpoints, Vladimir broke it
> > > > > > in commit dcf6fb89e6f7 ("media: qcom: camss: remove a check for
> > > > > > unavailable CAMSS endpoint"). If I do this again and go full circle, I'm
> > > > > > afraid this could break a second time before even making it to
> > > > > > linux-next.
> > > > 
> > > > Quite frankly I don't think that commit was valid, given it's conceivable
> > > > that an endpoint could be unconnected..
> > > > 
> > > 
> > > Endpoint is not a device, status property is the property of devices and
> > > not a property of anything else as the Devicetree Specification v0.4 and
> > > earlier ones define. Dangling endpoints are fine, there is no need to
> > > add another property to determine, if an endpoint is connected or not.
> > > 
> > > There should be no status properties inside endpoint device tree nodes.
> > 
> > The spec doesn't actually define what a "device" is. Funnily enough, it refers
> > to "endpoint" as a device:
> > 
> > 2.2.2 Generic Names Recommendation
> > The name of a node should be somewhat generic, reflecting the function of the
> > _device_ and not its precise programming model. If appropriate, the name should
> > be one of the following choices:
> > 
> > [...]
> > 
> > * endpoint
> > 
> > 
> > Plus an OF node is opaque in its purpose.. The top node, a firmware node, a
> > node representing a physical IP block and a config.ini-style blurb are all
> > "device nodes"
> 
> It sounds like somebody of DT maintainers should clarify the matter and update
> the spec to be less ambiguous, if it happens that "device" term is undefined.
> 
> > 
> > But coming back to the real world, the ports/endpoints represent the physical
> > connections to CAMSS and it makes sense to have them defined in one place,
> > especially since there's a predictable number of them that should not be left
> > up to each board to define.. That quite obviously implies that not all boards
> > are going to utilize all interfaces and the commit of yours that was mentioned
> > above seems to only be valid on the basis of semantics, which I above mentioned
> > are not *really* a valid point..
> 
> For whatever reason CAMSS on SDM670 is very special, because IIRC there is no
> other platform with preset status poperties of endpoints. This exclusive SDM670
> specifics shall be analysed and eliminated, since it hasn't been done during
> patch review time, it's time to do it right now then.

An SoC-common endpoint node is how panels are hooked up to DSI. This
seems to be the case for SDM670, SDM845, SM8[123456]50, etc.
It's not special or unpopular for DSI, but CAMSS seems to be the
subsystem where an endpoint node pre-defined by the SoC is only in
SDM670.

At least from the context retained in this reply and my memory, the most
compelling argument I've seen from you so far about removing the
pre-defined endpoints is (in multiple steps):

	1. Suggesting common endpoint nodes instead of common port nodes
	https://lore.kernel.org/all/e9dc1a6f-156b-40aa-9209-2010464d54ed@linaro.org/

	2. Breaking common endpoint nodes in CAMSS
	https://lore.kernel.org/all/20250903002255.346026-4-vladimir.zapolskiy@linaro.org/

This is why I try to remove the common endpoint nodes now.