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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <59e07c6a-6f1b-0cc7-dddc-96d2a4050843@linaro.org>
Date:   Wed, 6 Sep 2023 11:21:31 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Paul Elder <paul.elder@...asonboard.com>,
        linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 3/3] arm64: dts: mediatek: mt8365-pumpkin: Add overlays
 for thp7312 cameras

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