Re: [PATCH v2 10/11] arm64: dts: qcom: x1e80100-lenovo-yoga-slim7x: Add ov02c10 RGB sensor on CSIPHY4

[Neil Armstrong <neil.armstrong@linaro.org>]

On 3/16/26 12:46, Bryan O'Donoghue wrote:
> On 16/03/2026 08:28, Neil Armstrong wrote:
>>> +    ports {
>>> +        /*
>>> +         * port0 => csiphy0
>>> +         * port1 => csiphy1
>>> +         * port2 => csiphy2
>>> +         * port3 => csiphy4
>>> +         */
> 
> Hi.
> 
> Thanks for the review.
> 
> I think the above comment probably isn't making this very clear.
> 
> port0 => csiphy0 => msm_csiphy0 in the media-graph.
> 
>>> +        port@3 {
>>> +            camss_csiphy4_inep0: endpoint@0 {
>>> +                clock-lanes = <7>;
>>> +                data-lanes = <0 1>;
>>> +                remote-endpoint = <&ov02c10_ep>;
>>
>> This is quite wrong, with the PHY in a separate node, the lanes layout has nothing
>> to do in the "controller" ports since the sensor is connected to the the PHY which
>> configures the lanes functions.
>>
>> The PHY should be a media element in a port/endpoint chain to properly describe the
>> data flow from the sensor to the controller.
> 
> If I am reading your comment right, we are already defining the data-lanes where you've said they should be msm_csiphyX below port@X here maps to msm_csiphyX in the media graph.
> 
> So for example here is how we configure this before and after the changes in this series
> 
> media-ctl -v -d /dev/media0 -V '"ov08x40 '2-0036'":0[fmt:SGRBG10/3856x2416 field:none]'
> media-ctl -V '"msm_csiphy4":0[fmt:SGRBG10/3856x2416]'
> media-ctl -V '"msm_csid0":0[fmt:SGRBG10/3856x2416]'
> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SGRBG10/3856x2416]'
> media-ctl -l '"msm_csiphy4":1->"msm_csid0":0[1]'
> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
> media-ctl -d /dev/media0 -p

So a csiphy is a media element here, so why implement it as a PHY ? and there's a data link with CSID with should represented with a port/endpoint relationship...

> 
> Its a NOP change from user-space's perspective. CAMSS has historically used port@ to map to msm_csiphyX - taking logical elements and configuring pieces of the CSIPHY and CSID based on that mapping.
> 
> Other examples of this upstream:
> 
> /* Cadence CSI2RX */
> /* Sensor */
> camera@10 {
>      port {
>          csi2_cam0: endpoint {
>              remote-endpoint = <&csi2rx0_in_sensor>;
>              data-lanes = <1 2>;
>          };
>      };
> };
> 
> /* CSI bridge — NOT the PHY */
> &cdns_csi2rx0 {
>      phys = <&dphy0>;
>      port@0 {
>          csi2rx0_in_sensor: endpoint {
>              remote-endpoint = <&csi2_cam0>;
>              data-lanes = <1 2>;
>          };
>      };
> };

Yeah this is totally unrelated...

Neil

> 
> ---
> bod