| 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
| ||
|
Message-ID: <6c254d5f-9fa1-b06a-4edb-7e58e4b33101@ideasonboard.com>
Date: Thu, 3 Nov 2022 13:50:43 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Rob Herring <robh@...nel.org>
Cc: devicetree@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Jacopo Mondi <jacopo@...ndi.org>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Luca Ceresoli <luca@...aceresoli.net>,
Mark Rutland <mark.rutland@....com>,
Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Peter Rosin <peda@...ntia.se>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Vladimir Zapolskiy <vz@...ia.com>,
Wolfram Sang <wsa@...-dreams.de>,
satish.nagireddy@...cruise.com
Subject: Re: [PATCH v4 3/8] dt-bindings: media: add bindings for TI DS90UB960
Hi Rob,
On 02/11/2022 19:26, Rob Herring wrote:
> On Tue, Nov 01, 2022 at 03:20:27PM +0200, Tomi Valkeinen wrote:
>> Add DT bindings for TI DS90UB960 FPDLink-3 Deserializer.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>> .../bindings/media/i2c/ti,ds90ub960.yaml | 392 ++++++++++++++++++
>> 1 file changed, 392 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> new file mode 100644
>> index 000000000000..4456d9b3e2c7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml
>> @@ -0,0 +1,392 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ti,ds90ub960.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs
>> +
>> +maintainers:
>> + - Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> +
>> +description: |
>
> Don't need '|'
Hmm, ok... But why does that work? I can only find yaml examples for
multi-line with either | or >.
>> + The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO
>> + forwarding.
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - ti,ds90ub960-q1
>> + - ti,ds90ub9702-q1
>> +
>> + reg:
>> + maxItems: 1
>> + description:
>> + i2c addresses for the deserializer and the serializers
>> +
>> + reg-names:
>> + items:
>> + - const: main
>
> 'reg-names' is not all that useful with only 1 entry.
True.
>> +
>> + clocks:
>> + maxItems: 1
>> + description:
>> + Reference clock connected to the REFCLK pin.
>> +
>> + clock-names:
>> + items:
>> + - const: refclk
>> +
>> + powerdown-gpios:
>> + maxItems: 1
>> + description:
>> + Specifier for the GPIO connected to the PDB pin.
>> +
>> + i2c-alias-pool:
>
> Something common or could be? If not, then needs a vendor prefix.
I'll have to think about this. It is related to the i2c-atr, so I think
it might be a common thing.
>> + $ref: /schemas/types.yaml#/definitions/uint16-array
>> + description:
>> + i2c alias pool for remote devices.
>
> Needs a better description. What's an 'alias pool'?
Right.
"i2c alias pool is a pool of i2c addresses on the main i2c bus that can
be used to access the remote peripherals. Each remote peripheral is
assigned an alias from the pool, and transactions to that address will
be forwarded to the remote peripheral, with the address translated to
the remote peripheral's real address."
> 0-0xffff are valid values?
They are i2c addresses, and linux i2c uses u16 for addresses. Then
again, the fpdlink devices only support 7-bit addresses, so maybe this
could be an uint8 array. I am not sure what's the best way to define this.
>> +
>> + links:
>> + type: object
>> + additionalProperties: false
>> +
>> + properties:
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> + manual-strobe:
>> + type: boolean
>> + description:
>> + Enable manual strobe position and EQ level
>> +
>> + patternProperties:
>> + '^link@[0-9a-f]+$':
>> + type: object
>> + additionalProperties: false
>> + properties:
>> + reg:
>> + description: The link number
>> + maxItems: 1
>> +
>> + i2c-alias:
>
> Vendor prefix.
>
>> + description: |
>> + The i2c address used for the serializer. Transactions to this
>> + address on the i2c bus where the deserializer resides are
>> + forwarded to the serializer.
>> +
>> + rx-mode:
>
> Vendor prefix. And so on...
Yes, I totally missed these.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum:
>> + - 0 # RAW10
>> + - 1 # RAW12 HF
>> + - 2 # RAW12 LF
>> + - 3 # CSI2 SYNC
>> + - 4 # CSI2 NON-SYNC
>> + description: FPD-Link Input Mode
>> +
>> + cdr-mode:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum:
>> + - 0 # FPD3
>> + - 1 # FPD4
>> + description: FPD-Link CDR Mode
>> +
>> + strobe-pos:
>> + $ref: /schemas/types.yaml#/definitions/int32
>> + minimum: -13
>> + maximum: 13
>> + description: Manual strobe position, from -13 to 13
>
> No need to put constraints in free form text.
Ok.
>> +
>> + eq-level:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + maximum: 14
>> + description: Manual EQ level, from 0 to 14
>> +
>> + serializer:
>> + type: object
>> + description: FPD-Link Serializer node
>> +
>> + required:
>> + - reg
>> + - i2c-alias
>> + - rx-mode
>> + - serializer
>> +
>> + ports:
>> + $ref: /schemas/graph.yaml#/properties/ports
>> +
>> + properties:
>> + port@0:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>
> additionalProperties: false
This gives me check errors about the port's 'reg' property. Using
'unevaluatedProperties' works fine. Is 'unevaluatedProperties' correct,
or am I missing something here?
>> + description: FPD-Link input 0
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>
> unevaluatedProperties: false
>
> Same for the other port nodes
Yep.
>> +
>> + port@1:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + description: FPD-Link input 1
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> +
>> + port@2:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + description: FPD-Link input 2
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> +
>> + port@3:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + description: FPD-Link input 3
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> +
>> + port@4:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + description: CSI-2 Output 0
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> +
>> + properties:
>> + clock-lanes:
>> + maxItems: 1
>> +
>> + data-lanes:
>> + minItems: 1
>> + maxItems: 4
>
> Why the constraints on this endpoint? Are the other ones actually using
> properties from video-interfaces.yaml? If not, then just reference
> /properties/port and drop 'endpoint' instead.
The ports 0-3 do not use any properties from video-interfaces.yaml, so
I'll drop the endpoint.
Ports 4,5 are CSI-2 ports and need the clock-lanes and data-lanes to be
defined.
Is there something wrong with the constraints, or were you just
wondering about the difference between ports 0-3 and 4,5
>> +
>> + port@5:
>> + $ref: /schemas/graph.yaml#/$defs/port-base
>> + description: CSI-2 Output 1
>> +
>> + properties:
>> + endpoint:
>> + $ref: /schemas/media/video-interfaces.yaml#
>> +
>> + properties:
>> + clock-lanes:
>> + maxItems: 1
>> +
>> + data-lanes:
>> + minItems: 1
>> + maxItems: 4
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/gpio/gpio.h>
>> +
>> + i2c {
>> + clock-frequency = <400000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + deser@3d {
>> + compatible = "ti,ds90ub960-q1";
>> +
>> + reg-names = "main";
>> + reg = <0x3d>;
>> +
>> + clock-names = "refclk";
>> + clocks = <&fixed_clock>;
>> +
>> + powerdown-gpios = <&pca9555 7 GPIO_ACTIVE_LOW>;
>> +
>> + i2c-alias-pool = /bits/ 16 <0x4a 0x4b 0x4c 0x4d 0x4e 0x4f>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + /* Port 0, Camera 0 */
>> + port@0 {
>> + reg = <0>;
>> +
>> + ub960_fpd3_1_in: endpoint {
>> + remote-endpoint = <&ub953_1_out>;
>> +
>> + rx-mode = <0>;
>
> Looks like this is not defined under 'endpoint'.
Indeed, and after adding the 'unevaluatedProperties' I do get a warning
here.
>> + };
>> + };
>> +
>> + /* Port 0, Camera 1 */
>> + port@1 {
>> + reg = <1>;
>> +
>> + ub960_fpd3_2_in: endpoint {
>> + remote-endpoint = <&ub913_2_out>;
>> +
>> + rx-mode = <0>;
>> + };
>> + };
>> +
>> + /* Port 4, CSI-2 TX */
>> + port@4 {
>> + reg = <4>;
>> + ds90ub960_0_csi_out: endpoint {
>> + clock-lanes = <0>;
>> + data-lanes = <1 2 3 4>;
>> + link-frequencies = /bits/ 64 <800000000>;
>> + remote-endpoint = <&csi2_phy0>;
>> + };
>> + };
>> + };
>> +
>> + links {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + /* Link 0 has DS90UB953 serializer and IMX390 sensor */
>> +
>> + link@0 {
>> + reg = <0>;
>> + i2c-alias = <68>;
>> +
>> + rx-mode = <3>;
>> +
>> + serializer1: serializer {
>> + compatible = "ti,ds90ub953-q1";
>> +
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> +
>> + #clock-cells = <0>;
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>> + ub953_1_in: endpoint {
>> + clock-lanes = <0>;
>> + data-lanes = <1 2 3 4>;
>> + remote-endpoint = <&sensor_1_out>;
>> + };
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> +
>> + ub953_1_out: endpoint {
>> + remote-endpoint = <&ub960_fpd3_1_in>;
>> + };
>> + };
>> + };
>> +
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + sensor@21 {
>> + compatible = "sony,imx390";
DT_CHECKER_FLAGS=-m gives a warning here, as sony,imx390 is not in
upstream. The sensor details are not really relevant here, but I used
the data for the setup I have.
Should I instead use some sensor here that is in upstream, which I think
should work with the fpdlink ICs?
Tomi
Powered by blists - more mailing lists