| Message ID | 20200428092048.14939-1-ricardo.canuelo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] dt-bindings: display: ti,tfp410.txt: convert to yaml | expand |
On 28/04/2020 12:20, Ricardo Cañuelo wrote: > 2) The definition of ti,deskew in the original binding seems to be > tailored to the current driver and the way it's defined may not be very > DT-friendly. > > This parameter maps to a 3-bit field in a hardware register that takes > a value from 0 to 7, so the [-4, 3] range described for this would map > to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. > > Then, the driver parses the parameter (unsigned) and casts it to a > signed integer to get a number in the [-4, 3] range. Interestingly the current example has ti,deskew = <4>... > A vendor-specific property must have a type definition in json-schema, > so if I translate the original bindings semantics directly, I should > define ti,deskew as an int32, but this makes dt_binding_check fail if > the property has a negative value in the example because of the > internal representation of cells as unsigned integers: > > ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings? > So I can think of two solutions to this: > > a) Keep the ti,deskew property as an uint32 and document the valid > range ([-4, 3]) in the property description (this is what this patch > does currently). > > b) Redefine this property to be closer to the datasheet description > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > This would also let us define its range properly using minimum and > maximum properties for it. > > I think (b) is the right thing to do but I want to know your > opinion. Besides, I don't have this hardware at hand and if I updated > the driver I wouldn't be able to test it. I don't think anyone has used deskew property, so I guess changing it is not out of the question. Laurent, did you have a board that needs deskew when you added it to tfp410? Tomi
Hi Ricardo. Thanks for looking into this bridge binding. Some comments in the following. Sam On Tue, Apr 28, 2020 at 11:20:48AM +0200, Ricardo Cañuelo wrote: > Convert the DT binding documentation for the TI TFP410 DPI-to-DVI > encoder to json-schema. > > Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> > --- > Hi all, > > I found some issues while converting this binding and I'd like to know > your opinions on how to tackle them. > > 1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts > > This board uses the TFP410 encoder but it doesn't define any ports for > it. I can't find any suitable remote endpoints in its description > either. Maybe this board description should be reworked? The current > driver won't handle the device if it doesn't define any ports or > endpoints anyway. > It is expected that there are a few DT files that will need an update when they are checked more fomally. So post a sepearate patch to fix it - preferably before the conversion. > It also uses the 'powerdown-gpio' property instead of > 'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver > point of view, but the general standard in DT bindings is to use the > plural. This is trivial to fix. Use the plural form as everyone else. > 2) The definition of ti,deskew in the original binding seems to be > tailored to the current driver and the way it's defined may not be very > DT-friendly. > > This parameter maps to a 3-bit field in a hardware register that takes > a value from 0 to 7, so the [-4, 3] range described for this would map > to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. > > Then, the driver parses the parameter (unsigned) and casts it to a > signed integer to get a number in the [-4, 3] range. > > A vendor-specific property must have a type definition in json-schema, > so if I translate the original bindings semantics directly, I should > define ti,deskew as an int32, but this makes dt_binding_check fail if > the property has a negative value in the example because of the > internal representation of cells as unsigned integers: > > ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 Can you define it as an enum like this: enum: [-4, -3, -2, -1, 0, 1, 2, 3] And then maybe in the descrition describe how they map to 0..7. The problem is that the binding is an API so we cannot just change the interpretation of the value 0 etc. > > So I can think of two solutions to this: > > a) Keep the ti,deskew property as an uint32 and document the valid > range ([-4, 3]) in the property description (this is what this patch > does currently). > > b) Redefine this property to be closer to the datasheet description > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > This would also let us define its range properly using minimum and > maximum properties for it. > > I think (b) is the right thing to do but I want to know your > opinion. Besides, I don't have this hardware at hand and if I updated > the driver I wouldn't be able to test it. > > Thanks. > > Patch tested with: > > make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml> > make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml> > > .../bindings/display/bridge/ti,tfp410.txt | 66 ---------- > .../bindings/display/bridge/ti,tfp410.yaml | 121 ++++++++++++++++++ > 2 files changed, 121 insertions(+), 66 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt > create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt > deleted file mode 100644 > index 5ff4f64ef8e8..000000000000 > --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt > +++ /dev/null > @@ -1,66 +0,0 @@ > -TFP410 DPI to DVI encoder > -========================= > - > -Required properties: > -- compatible: "ti,tfp410" > - > -Optional properties: > -- powerdown-gpios: power-down gpio > -- reg: I2C address. If and only if present the device node should be placed > - into the I2C controller node where the TFP410 I2C is connected to. > -- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured > - through th DK[3:1] pins. This property shall be present only if the TFP410 > - is not connected through I2C. > - > -Required nodes: > - > -This device has two video ports. Their connections are modeled using the OF > -graph bindings specified in [1]. Each port node shall have a single endpoint. > - > -- Port 0 is the DPI input port. Its endpoint subnode shall contain a > - pclk-sample and bus-width property and a remote-endpoint property as specified > - in [1]. > - - If pclk-sample is not defined, pclk-sample = 0 should be assumed for > - backward compatibility. > - - If bus-width is not defined then bus-width = 24 should be assumed for > - backward compatibility. > - bus-width = 24: 24 data lines are connected and single-edge mode > - bus-width = 12: 12 data lines are connected and dual-edge mode These comments are missing in the new binding. > - > -- Port 1 is the DVI output port. Its endpoint subnode shall contain a > - remote-endpoint property is specified in [1]. > - > -[1] Documentation/devicetree/bindings/media/video-interfaces.txt > - > - > -Example > -------- > - > -tfp410: encoder@0 { > - compatible = "ti,tfp410"; > - powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>; > - ti,deskew = <4>; > - > - ports { > - #address-cells = <1>; > - #size-cells = <0>; > - > - port@0 { > - reg = <0>; > - > - tfp410_in: endpoint@0 { > - pclk-sample = <1>; > - bus-width = <24>; > - remote-endpoint = <&dpi_out>; > - }; > - }; > - > - port@1 { > - reg = <1>; > - > - tfp410_out: endpoint@0 { > - remote-endpoint = <&dvi_connector_in>; > - }; > - }; > - }; > -}; > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > new file mode 100644 > index 000000000000..79666ee540f9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml > @@ -0,0 +1,121 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) You need to obtain approval to change the license to include BSD. The current binfing is GPL-2-0-only as it is default in the tree. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TFP410 DPI to DVI encoder > + > +maintainers: > + - Tomi Valkeinen <tomi.valkeinen@ti.com> > + - Jyri Sarha <jsarha@ti.com> > + > +properties: > + compatible: > + const: "ti,tfp410" No "" around the compatible name > + > + reg: > + description: I2C address of the device. > + maxItems: 1 > + > + powerdown-gpios: > + maxItems: 1 > + > + ti,deskew: > + description: > + Data de-skew in 350ps increments, from -4 to +3, as configured > + through the DK[3:1] pins. This property shall be present only if > + the TFP410 is not connected through I2C. > + maxItems: 1 > + allOf: > + - $ref: /schemas/types.yaml#/definitions/uint32 No need for the allOf: - $ref. Just use a plain $ref > + > + ports: > + description: > + A node containing input and output port nodes with endpoint > + definitions as documented in > + Documentation/devicetree/bindings/media/video-interfaces.txt > + type: object > + > + properties: > + port@0: > + description: DPI input port. > + type: object > + > + properties: > + reg: > + const: 0 > + > + endpoint: > + type: object > + > + properties: > + pclk-sample: > + description: Endpoint sampling edge. > + enum: > + - 0 # Falling edge > + - 1 # Rising edge > + > + bus-width: > + description: Endpoint bus width. > + enum: [ 12, 24 ] > + > + required: > + - endpoint > + > + port@1: > + description: DVI output port. > + type: object > + > + properties: > + reg: > + const: 1 > + > + endpoint: > + type: object > + > + required: > + - endpoint > + > + required: > + - port@0 > + - port@1 > + > +required: > + - compatible > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + tfp410: encoder { > + compatible = "ti,tfp410"; > + powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>; > + ti,deskew = <3>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + tfp410_in: endpoint { > + pclk-sample = <1>; > + bus-width = <24>; > + remote-endpoint = <&dpi_out>; > + }; > + }; > + > + port@1 { > + reg = <1>; > + tfp410_out: endpoint { > + remote-endpoint = <&dvi_connector_in>; > + }; > + }; > + }; > + }; > + > +... > -- > 2.18.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On mar 28-04-2020 21:21:17, Sam Ravnborg wrote: > Hi Ricardo. > > Thanks for looking into this bridge binding. > Some comments in the following. Thanks for reviewing it, Sam. I agree with your suggestions, some comments below: > Can you define it as an enum like this: > > enum: [-4, -3, -2, -1, 0, 1, 2, 3] > > And then maybe in the descrition describe how they map to 0..7. > The problem is that the binding is an API so we cannot just change > the interpretation of the value 0 etc. This is similar to what I wanted to do at first, specifying minimum and maximum values, but it'd have the same problem with the yaml checker. See, problem is in the yaml checking itself, the actual parameter parsing in the driver already works with negative integers. The issue with this particular property is that: - it's vendor specific, so it must have a type definition. - the actual value is signed, so the proper type definition to use should be a signed integer. - but then, if you use a negative value for this it will be cast into a very large u32 (that's what cells are, after all) beyond the maximum value of a positive int32. - Range constraints can't be properly checked either, since any negative integer interpreted as unsigned will result in a positive number beyond the top end of the value range (3 in this case). So unless there's a way of making the yaml check process aware of negative integers, I think the only way to pass the checks is to use unsigned integers and avoid defining the range constraints. FWIW, the only other similar case I found (adi,ltc2983.yaml) uses uint64 and no range constraints. Cheers, Ricardo
Hi Tomi, thanks for reviewing the patch. On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote: > I don't quite understand this. We cannot have negative numbers in dts files? > Or we can, but dt_binding_check doesn't handle them correctly? Or that int32 > is not supported in yaml bindings? AFAICT, you can have negative numbers in dts files (see [1] and [2]) and the DT schema certainly supports signed integers, but dt_binding_check seems to interpret all cells as unsigned 32bit integers because that's what they are, really. In kernel code this is not a problem because you can cast the value back to a signed int before you run your own sanity checks on them. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159682.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159681.html Cheers, Ricardo
Hi Ricardo, On 06/05/2020 10:21, Ricardo Cañuelo wrote: > Hi Tomi, thanks for reviewing the patch. > > On mar 28-04-2020 12:49:28, Tomi Valkeinen wrote: >> I don't quite understand this. We cannot have negative numbers in dts files? >> Or we can, but dt_binding_check doesn't handle them correctly? Or that int32 >> is not supported in yaml bindings? > > AFAICT, you can have negative numbers in dts files (see [1] and [2]) and This is also my understanding after some googling. And there's even of_property_read_s32() in the kernel. > the DT schema certainly supports signed integers, but dt_binding_check > seems to interpret all cells as unsigned 32bit integers because that's > what they are, really. In kernel code this is not a problem because you Well, this is in the nitpick category, and maybe not even relevant, but I don't think that's correct. They're just bits. Some pieces of SW happen to use u32 containers to store the bits. But what the bits mean is not related to the container. > can cast the value back to a signed int before you run your own sanity > checks on them. Doesn't all this just point to a bug or missing feature in dt_binding_check? That's not a reason to change the ABI. Tomi
Hi Tomi, On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote: > Doesn't all this just point to a bug or missing feature in dt_binding_check? > That's not a reason to change the ABI. I agree and I'd vote for "missing feature", but seeing that there aren't any other examples of this use case in the whole kernel dts collection (at least I couldn't find any) I thought that maybe it's us who are going against the norm here. Maybe Rob can shed some light about this? Cheers, Ricardo
On 06/05/2020 11:28, Ricardo Cañuelo wrote: > Hi Tomi, > > On mié 06-05-2020 11:01:07, Tomi Valkeinen wrote: >> Doesn't all this just point to a bug or missing feature in dt_binding_check? >> That's not a reason to change the ABI. > > I agree and I'd vote for "missing feature", but seeing that there aren't > any other examples of this use case in the whole kernel dts collection > (at least I couldn't find any) I thought that maybe it's us who are > going against the norm here. A valid point. I'm not aware of anyone using the deskew property. My guess is that Laurent added it just because it was in the spec, not because he had a need for it. So I don't think changing the binding is totally out of the question. Tomi
Hi Tomi, On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote: > On 28/04/2020 12:20, Ricardo Cañuelo wrote: > > > 2) The definition of ti,deskew in the original binding seems to be > > tailored to the current driver and the way it's defined may not be very > > DT-friendly. > > > > This parameter maps to a 3-bit field in a hardware register that takes > > a value from 0 to 7, so the [-4, 3] range described for this would map > > to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. > > > > Then, the driver parses the parameter (unsigned) and casts it to a > > signed integer to get a number in the [-4, 3] range. > > Interestingly the current example has ti,deskew = <4>... > > > A vendor-specific property must have a type definition in json-schema, > > so if I translate the original bindings semantics directly, I should > > define ti,deskew as an int32, but this makes dt_binding_check fail if > > the property has a negative value in the example because of the > > internal representation of cells as unsigned integers: > > > > ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings? > > > So I can think of two solutions to this: > > > > a) Keep the ti,deskew property as an uint32 and document the valid > > range ([-4, 3]) in the property description (this is what this patch > > does currently). > > > > b) Redefine this property to be closer to the datasheet description > > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > > This would also let us define its range properly using minimum and > > maximum properties for it. > > > > I think (b) is the right thing to do but I want to know your > > opinion. Besides, I don't have this hardware at hand and if I updated > > the driver I wouldn't be able to test it. > > I don't think anyone has used deskew property, so I guess changing it is not out of the question. > > Laurent, did you have a board that needs deskew when you added it to tfp410? I didn't if I remember correctly, I just mapped it to the hardware features. The hardware register indeed takes a value between 0 and 7, and that is mapped to [-4,3] x t(STEP). I don't mind either option.
Hi Rob, What's your opinion on this? Some context: It's about bindings that define signed integer properties with range checks that go below and above zero. The schema checker fails because, apparently, it interprets every cell value as an uint32, which makes the range check fail for negative numbers. On mié 06-05-2020 18:53:20, Laurent Pinchart wrote: > Hi Tomi, > > On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote: > > On 28/04/2020 12:20, Ricardo Cañuelo wrote: > > > > > 2) The definition of ti,deskew in the original binding seems to be > > > tailored to the current driver and the way it's defined may not be very > > > DT-friendly. > > > > > > This parameter maps to a 3-bit field in a hardware register that takes > > > a value from 0 to 7, so the [-4, 3] range described for this would map > > > to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. > > > > > > Then, the driver parses the parameter (unsigned) and casts it to a > > > signed integer to get a number in the [-4, 3] range. > > > > Interestingly the current example has ti,deskew = <4>... > > > > > A vendor-specific property must have a type definition in json-schema, > > > so if I translate the original bindings semantics directly, I should > > > define ti,deskew as an int32, but this makes dt_binding_check fail if > > > the property has a negative value in the example because of the > > > internal representation of cells as unsigned integers: > > > > > > ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 > > > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but > > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings? > > > > > So I can think of two solutions to this: > > > > > > a) Keep the ti,deskew property as an uint32 and document the valid > > > range ([-4, 3]) in the property description (this is what this patch > > > does currently). > > > > > > b) Redefine this property to be closer to the datasheet description > > > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > > > This would also let us define its range properly using minimum and > > > maximum properties for it. > > > > > > I think (b) is the right thing to do but I want to know your > > > opinion. Besides, I don't have this hardware at hand and if I updated > > > the driver I wouldn't be able to test it. > > > > I don't think anyone has used deskew property, so I guess changing it is not out of the question. > > > > Laurent, did you have a board that needs deskew when you added it to tfp410? > > I didn't if I remember correctly, I just mapped it to the hardware > features. The hardware register indeed takes a value between 0 and 7, > and that is mapped to [-4,3] x t(STEP). I don't mind either option. > > -- > Regards, > > Laurent Pinchart I haven't found any examples of yaml bindings defining signed integer properties such as this, what's the norm in this case? Do you agree with any of the proposed solutions? Do you have a better suggestion? Thanks, Ricardo
Hi Ricardo. On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote: > Hi Rob, > > What's your opinion on this? I'm not Rob, but anyway. > > Some context: It's about bindings that define signed integer properties > with range checks that go below and above zero. The schema checker fails > because, apparently, it interprets every cell value as an uint32, which > makes the range check fail for negative numbers. > > > > > b) Redefine this property to be closer to the datasheet description > > > > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > > > > This would also let us define its range properly using minimum and > > > > maximum properties for it. > > > > > > > > I think (b) is the right thing to do but I want to know your > > > > opinion. Besides, I don't have this hardware at hand and if I updated > > > > the driver I wouldn't be able to test it. Based on the discussions option b) above seems like the best compromise. Sam
On Mon, May 11, 2020 at 04:59:11PM +0200, Ricardo Cañuelo wrote: > Hi Rob, > > What's your opinion on this? > > Some context: It's about bindings that define signed integer properties > with range checks that go below and above zero. The schema checker fails > because, apparently, it interprets every cell value as an uint32, which > makes the range check fail for negative numbers. The real fix is dtc needs to carry the sign thru to the yaml output format. In the mean time, perhaps the schema should have an unsigned range for signed types. Rob > > On mié 06-05-2020 18:53:20, Laurent Pinchart wrote: > > Hi Tomi, > > > > On Tue, Apr 28, 2020 at 12:49:28PM +0300, Tomi Valkeinen wrote: > > > On 28/04/2020 12:20, Ricardo Cañuelo wrote: > > > > > > > 2) The definition of ti,deskew in the original binding seems to be > > > > tailored to the current driver and the way it's defined may not be very > > > > DT-friendly. > > > > > > > > This parameter maps to a 3-bit field in a hardware register that takes > > > > a value from 0 to 7, so the [-4, 3] range described for this would map > > > > to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. > > > > > > > > Then, the driver parses the parameter (unsigned) and casts it to a > > > > signed integer to get a number in the [-4, 3] range. > > > > > > Interestingly the current example has ti,deskew = <4>... > > > > > > > A vendor-specific property must have a type definition in json-schema, > > > > so if I translate the original bindings semantics directly, I should > > > > define ti,deskew as an int32, but this makes dt_binding_check fail if > > > > the property has a negative value in the example because of the > > > > internal representation of cells as unsigned integers: > > > > > > > > ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 > > > > > > I don't quite understand this. We cannot have negative numbers in dts files? Or we can, but > > > dt_binding_check doesn't handle them correctly? Or that int32 is not supported in yaml bindings? > > > > > > > So I can think of two solutions to this: > > > > > > > > a) Keep the ti,deskew property as an uint32 and document the valid > > > > range ([-4, 3]) in the property description (this is what this patch > > > > does currently). > > > > > > > > b) Redefine this property to be closer to the datasheet description > > > > (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. > > > > This would also let us define its range properly using minimum and > > > > maximum properties for it. > > > > > > > > I think (b) is the right thing to do but I want to know your > > > > opinion. Besides, I don't have this hardware at hand and if I updated > > > > the driver I wouldn't be able to test it. > > > > > > I don't think anyone has used deskew property, so I guess changing it is not out of the question. > > > > > > Laurent, did you have a board that needs deskew when you added it to tfp410? > > > > I didn't if I remember correctly, I just mapped it to the hardware > > features. The hardware register indeed takes a value between 0 and 7, > > and that is mapped to [-4,3] x t(STEP). I don't mind either option. > > > > -- > > Regards, > > > > Laurent Pinchart > > I haven't found any examples of yaml bindings defining signed integer > properties such as this, what's the norm in this case? Do you agree with > any of the proposed solutions? Do you have a better suggestion? > > Thanks, > Ricardo
Hi Laurent, On mié 06-05-2020 18:53:20, Laurent Pinchart wrote: > I didn't if I remember correctly, I just mapped it to the hardware > features. The hardware register indeed takes a value between 0 and 7, > and that is mapped to [-4,3] x t(STEP). I don't mind either option. I was taking a look at the ti-tfp410.c driver to see if it needs any changes to support the updated deskew property ranges [0-7], but I don't fully understand what this does (line 276): /* Get the setup and hold time from vendor-specific properties. */ of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew); if (deskew < -4 || deskew > 3) return -EINVAL; timings->setup_time_ps = min(0, 1200 - 350 * deskew); timings->hold_time_ps = min(0, 1300 + 350 * deskew); It looks like that the driver doesn't really apply the deskew settings to the device and that this has not been really tested, so it's probably not a big deal. I guess what you wanted to do was to adjust the setup and hold times around 1200 and 1300 ps respectively in increments/decrements of 350 ps depending on the deskew value, as the datasheet describes. But this code would set timings->setup_time_ps to 0 regardless of the deskew value, and timings->hold_time_ps would be either 0 or a very big integer value if deskew is -4 (both setup_time_ps and hold_time_ps are u32). Am I missing something? Was this intentional? Thanks, Ricardo
Hi Ricardo, On Wed, May 13, 2020 at 01:09:57PM +0200, Ricardo Cañuelo wrote: > On mié 06-05-2020 18:53:20, Laurent Pinchart wrote: > > I didn't if I remember correctly, I just mapped it to the hardware > > features. The hardware register indeed takes a value between 0 and 7, > > and that is mapped to [-4,3] x t(STEP). I don't mind either option. > > I was taking a look at the ti-tfp410.c driver to see if it needs any > changes to support the updated deskew property ranges [0-7], but I don't > fully understand what this does (line 276): > > /* Get the setup and hold time from vendor-specific properties. */ > of_property_read_u32(dvi->dev->of_node, "ti,deskew", (u32 *)&deskew); > if (deskew < -4 || deskew > 3) > return -EINVAL; > > timings->setup_time_ps = min(0, 1200 - 350 * deskew); > timings->hold_time_ps = min(0, 1300 + 350 * deskew); > > It looks like that the driver doesn't really apply the deskew settings > to the device and that this has not been really tested, so it's probably > not a big deal. The driver doesn't apply any setting to the device :-) The ti,deskew property is meant to report the deskew settings selected by the chip's configuration pins, not to set a value to be programmed to the device. > I guess what you wanted to do was to adjust the setup and hold times > around 1200 and 1300 ps respectively in increments/decrements of 350 ps > depending on the deskew value, as the datasheet describes. But this code > would set timings->setup_time_ps to 0 regardless of the deskew value, > and timings->hold_time_ps would be either 0 or a very big integer value > if deskew is -4 (both setup_time_ps and hold_time_ps are u32). > > Am I missing something? Was this intentional? Oops. That's embarassing... It should clearly be a max(), not a min(). And only for hold_time_ps is this required. Would you like to send a patch, or should I do so ?
Hi Laurent, On mié 13-05-2020 17:08:32, Laurent Pinchart wrote: > The driver doesn't apply any setting to the device :-) The ti,deskew > property is meant to report the deskew settings selected by the chip's > configuration pins, not to set a value to be programmed to the device. Oh, I see, thanks for clarifying it. > Would you like to send a patch, or should I do so ? I can take care of it, I'll try to have a new series by tomorrow. Cheers, Ricardo
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt deleted file mode 100644 index 5ff4f64ef8e8..000000000000 --- a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt +++ /dev/null @@ -1,66 +0,0 @@ -TFP410 DPI to DVI encoder -========================= - -Required properties: -- compatible: "ti,tfp410" - -Optional properties: -- powerdown-gpios: power-down gpio -- reg: I2C address. If and only if present the device node should be placed - into the I2C controller node where the TFP410 I2C is connected to. -- ti,deskew: data de-skew in 350ps increments, from -4 to +3, as configured - through th DK[3:1] pins. This property shall be present only if the TFP410 - is not connected through I2C. - -Required nodes: - -This device has two video ports. Their connections are modeled using the OF -graph bindings specified in [1]. Each port node shall have a single endpoint. - -- Port 0 is the DPI input port. Its endpoint subnode shall contain a - pclk-sample and bus-width property and a remote-endpoint property as specified - in [1]. - - If pclk-sample is not defined, pclk-sample = 0 should be assumed for - backward compatibility. - - If bus-width is not defined then bus-width = 24 should be assumed for - backward compatibility. - bus-width = 24: 24 data lines are connected and single-edge mode - bus-width = 12: 12 data lines are connected and dual-edge mode - -- Port 1 is the DVI output port. Its endpoint subnode shall contain a - remote-endpoint property is specified in [1]. - -[1] Documentation/devicetree/bindings/media/video-interfaces.txt - - -Example -------- - -tfp410: encoder@0 { - compatible = "ti,tfp410"; - powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>; - ti,deskew = <4>; - - ports { - #address-cells = <1>; - #size-cells = <0>; - - port@0 { - reg = <0>; - - tfp410_in: endpoint@0 { - pclk-sample = <1>; - bus-width = <24>; - remote-endpoint = <&dpi_out>; - }; - }; - - port@1 { - reg = <1>; - - tfp410_out: endpoint@0 { - remote-endpoint = <&dvi_connector_in>; - }; - }; - }; -}; diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml new file mode 100644 index 000000000000..79666ee540f9 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml @@ -0,0 +1,121 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/ti,tfp410.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TFP410 DPI to DVI encoder + +maintainers: + - Tomi Valkeinen <tomi.valkeinen@ti.com> + - Jyri Sarha <jsarha@ti.com> + +properties: + compatible: + const: "ti,tfp410" + + reg: + description: I2C address of the device. + maxItems: 1 + + powerdown-gpios: + maxItems: 1 + + ti,deskew: + description: + Data de-skew in 350ps increments, from -4 to +3, as configured + through the DK[3:1] pins. This property shall be present only if + the TFP410 is not connected through I2C. + maxItems: 1 + allOf: + - $ref: /schemas/types.yaml#/definitions/uint32 + + ports: + description: + A node containing input and output port nodes with endpoint + definitions as documented in + Documentation/devicetree/bindings/media/video-interfaces.txt + type: object + + properties: + port@0: + description: DPI input port. + type: object + + properties: + reg: + const: 0 + + endpoint: + type: object + + properties: + pclk-sample: + description: Endpoint sampling edge. + enum: + - 0 # Falling edge + - 1 # Rising edge + + bus-width: + description: Endpoint bus width. + enum: [ 12, 24 ] + + required: + - endpoint + + port@1: + description: DVI output port. + type: object + + properties: + reg: + const: 1 + + endpoint: + type: object + + required: + - endpoint + + required: + - port@0 + - port@1 + +required: + - compatible + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + tfp410: encoder { + compatible = "ti,tfp410"; + powerdown-gpios = <&twl_gpio 2 GPIO_ACTIVE_LOW>; + ti,deskew = <3>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + tfp410_in: endpoint { + pclk-sample = <1>; + bus-width = <24>; + remote-endpoint = <&dpi_out>; + }; + }; + + port@1 { + reg = <1>; + tfp410_out: endpoint { + remote-endpoint = <&dvi_connector_in>; + }; + }; + }; + }; + +...
Convert the DT binding documentation for the TI TFP410 DPI-to-DVI encoder to json-schema. Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com> --- Hi all, I found some issues while converting this binding and I'd like to know your opinions on how to tackle them. 1) dtbs_check fails for arch/arm/boot/dts/dove-sbc-a510.dts This board uses the TFP410 encoder but it doesn't define any ports for it. I can't find any suitable remote endpoints in its description either. Maybe this board description should be reworked? The current driver won't handle the device if it doesn't define any ports or endpoints anyway. It also uses the 'powerdown-gpio' property instead of 'powerdown-gpios'. AFAICT this shouldn't be a problem from the driver point of view, but the general standard in DT bindings is to use the plural. This is trivial to fix. 2) The definition of ti,deskew in the original binding seems to be tailored to the current driver and the way it's defined may not be very DT-friendly. This parameter maps to a 3-bit field in a hardware register that takes a value from 0 to 7, so the [-4, 3] range described for this would map to [000, 111]: -4 -> 000, -3 -> 001, -2 -> 010, ... 3 -> 111. Then, the driver parses the parameter (unsigned) and casts it to a signed integer to get a number in the [-4, 3] range. A vendor-specific property must have a type definition in json-schema, so if I translate the original bindings semantics directly, I should define ti,deskew as an int32, but this makes dt_binding_check fail if the property has a negative value in the example because of the internal representation of cells as unsigned integers: ti,deskew:0:0: 4294967293 is greater than the maximum of 2147483647 So I can think of two solutions to this: a) Keep the ti,deskew property as an uint32 and document the valid range ([-4, 3]) in the property description (this is what this patch does currently). b) Redefine this property to be closer to the datasheet description (ie. unsigned integers from 0 to 7) and adapt the driver accordingly. This would also let us define its range properly using minimum and maximum properties for it. I think (b) is the right thing to do but I want to know your opinion. Besides, I don't have this hardware at hand and if I updated the driver I wouldn't be able to test it. Thanks. Patch tested with: make dt_binding_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml> make dtbs_check ARCH=arm DT_SCHEMA_FILES=<.../ti,tfp410.yaml> .../bindings/display/bridge/ti,tfp410.txt | 66 ---------- .../bindings/display/bridge/ti,tfp410.yaml | 121 ++++++++++++++++++ 2 files changed, 121 insertions(+), 66 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.txt create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,tfp410.yaml