Re: [PATCH 3/3] arm64: dts: mediatek: mt8365-pumpkin: Add overlays for thp7312 cameras

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 06/09/2023 11:00, Laurent Pinchart wrote:
>>> has a regulator@0. There are similar instances for clocks.
>>>
>>> I understand why it may not be a good idea, and how the root node is
>>> indeed not a bus. In some cases, those regulators and clocks are grouped
>>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
>>> not sure if that's a good idea, but at least it should validate.
>>>
>>> What's the best practice for discrete board-level clocks and regulators
>>> in overlays ? How do we ensure that their node name will not conflict
>>> with the board to which the overlay is attached ?
>>
>> Top-level nodes (so under /) do not have unit addresses. If they have -
>> it's an error, because it is not a bus. Also, unit address requires reg.
>> No reg? No unit address. DTC reports this as warnings as well.
> 
> I agree with all that, but what's the recommended practice to add
> top-level clocks and regulators in overlays, in a way that avoids
> namespace clashes with the base board ?

Whether you use regulator@0 or regulator-0, you have the same chances of
clash.

> 
>>>>> +		orientation = <0>;
>>>>> +		rotation = <0>;
>>>>> +
>>>>> +		thine,rx,data-lanes = <4 1 3 2>;
>>>>
>>>> NAK for this property.
>>>
>>> Please explain why. You commented very briefly in the bindings review,
>>> and it wasn't clear to me if you were happy or not with the property,
>>> and if not, why.
>>
>> Because it is duplicating endpoint. At least from the description.
> 
> The THP7312 is an external ISP. At the hardware level, it has an input
> side, with a CSI-2 receiver and an I2C master controller, and an output
> side, with a CSI-2 transmitter and an I2C slave controller. A raw camera
> sensor is connected on the input side, transmitting image data to the
> THP7312, and being controlled over I2C by the firmware running on the
> THP7312. From a Linux point of view, only the output side of the THP7312
> is visible, and the combination of the raw camera sensor and the THP7312
> acts as a smart camera sensor, producing YUV images.

None of this was explained in the device description or property field.
I probably judged to fast but it just looked like duplicated property.
Then shouldn't it have two ports, even if camera side is not visible for
the Linux?

> 
> As there are two CSI-2 buses, the data lanes configuration needs to be
> specified for both sides. On the output side, connected to the SoC and
> visible to Linux, the bindings use a port node with an endpoint and the
> standard data-lanes property. On the input side, which is invisible to
> Linux, the bindings use the vendor-specific thine,rx,data-lanes
> property. Its semantics is identical to the standard data-lanes
> property, but it's not located in an endpoint as there's no port for the
> input side.

And how does the property support multiple sensors? What if they data
lanes are also different between each other?

Best regards,
Krzysztof