* [PATCH v6 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge @ 2018-03-16 15:16 Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Jacopo Mondi @ 2018-03-16 15:16 UTC (permalink / raw) To: architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: Jacopo Mondi, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi, (hopefully) last iteration, with Niklas' Reviewed-by tags. Simply drop an un-necessary #ifdef guard for CONFIG_OF in driver's code as suggested by Niklas. Time to talk how this series will go in? I assume bindings and driver through DRM tree, while Simon is to pick up the Eagle DTS update once the first two patches will get in. Is this ok for both DRM and Renesas side? Also I assume before bindings gets accepted someone from devicetree list should ack them, correct? Simon: as Niklas suggested, I have now listed the series dependencies below the commit message. It is my understanding that is not necessary to resend his series with that commit squashed on top (which I also prefer not to). Thanks j v5 -> v6: - Drop check for CONFIG_OF as it is a Kconfig dependency - Add Niklas Reviewed-by tags - List [3/3] depenencies below commit message to ease integration v4 -> v5: - Fix punctuation in bindings documentation - Add small statement to bindings document to clarify the chip has no control bus - Print regulator name in enable/disable routines error path - Add Andrzej Reviewed-by tag v3 -> v4: - Rename permutations of "pdwn" to just "pdwn" everywhere in the series - Improve power enable/disable routines as suggested by Andrzej and Sergei - Change "pdwn" gpio initialization to use the logical output level - Change Kconfig description v2 -> v3: - Drop support for "lvds-decoder" and make the driver THC63LVD1024 specific -- Rework bindings to describe multiple input/output ports -- Rename driver and remove "lvds-decoder" references -- Rework Eagle DTS to use new bindings v1 -> v2: - Drop support for THC63LVD1024 Jacopo Mondi (3): dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder drm: bridge: Add thc63lvd1024 LVDS decoder driver arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 ++++++ arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 ++- drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/thc63lvd1024.c | 255 +++++++++++++++++++++ 5 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c -- 2.7.4 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-16 15:16 [PATCH v6 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge Jacopo Mondi @ 2018-03-16 15:16 ` Jacopo Mondi 2018-03-20 12:43 ` Laurent Pinchart 2018-03-16 15:16 ` [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle Jacopo Mondi 2 siblings, 1 reply; 32+ messages in thread From: Jacopo Mondi @ 2018-03-16 15:16 UTC (permalink / raw) To: architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: Jacopo Mondi, dri-devel, linux-renesas-soc, devicetree, linux-kernel Document Thine THC63LVD1024 LVDS decoder device tree bindings. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt new file mode 100644 index 0000000..8225c6a --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt @@ -0,0 +1,66 @@ +Thine Electronics THC63LVD1024 LVDS decoder +------------------------------------------- + +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams +to parallel data outputs. The chip supports single/dual input/output modes, +handling up to two two input LVDS stream and up to two digital CMOS/TTL outputs. + +Single or dual operation modes, output data mapping and DDR output modes are +configured through input signals and the chip does not expose any control bus. + +Required properties: +- compatible: Shall be "thine,thc63lvd1024" + +Optional properties: +- vcc-supply: Power supply for TTL output and digital circuitry +- cvcc-supply: Power supply for TTL CLOCKOUT signal +- lvcc-supply: Power supply for LVDS inputs +- pvcc-supply: Power supply for PLL circuitry +- pdwn-gpios: Power down GPIO signal. Active low +- oe-gpios: Output enable GPIO signal. Active high + +The THC63LVD1024 video port connections are modeled according +to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt + +Required video port nodes: +- Port@0: First LVDS input port +- Port@2: First digital CMOS/TTL parallel output + +Optional video port nodes: +- Port@1: Second LVDS input port +- Port@3: Second digital CMOS/TTL parallel output + +Example: +-------- + + thc63lvd1024: lvds-decoder { + compatible = "thine,thc63lvd1024"; + + vcc-supply = <®_lvds_vcc>; + lvcc-supply = <®_lvds_lvcc>; + + pdwn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + + lvds_dec_in_0: endpoint { + remote-endpoint = <&lvds_out>; + }; + }; + + port@2{ + reg = <2>; + + lvds_dec_out_2: endpoint { + remote-endpoint = <&adv7511_in>; + }; + + }; + + }; + }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-16 15:16 ` [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi @ 2018-03-20 12:43 ` Laurent Pinchart 2018-03-26 12:08 ` jacopo mondi 2018-03-26 22:22 ` Rob Herring 0 siblings, 2 replies; 32+ messages in thread From: Laurent Pinchart @ 2018-03-20 12:43 UTC (permalink / raw) To: Jacopo Mondi Cc: architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel, robh+dt Hi Jacopo, (CC'ing Rob) Thank you for the patch. On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: > Document Thine THC63LVD1024 LVDS decoder device tree bindings. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > 1 file changed, 66 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > diff --git > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > new file mode 100644 > index 0000000..8225c6a > --- /dev/null > +++ > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > @@ -0,0 +1,66 @@ > +Thine Electronics THC63LVD1024 LVDS decoder > +------------------------------------------- > + > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > streams > +to parallel data outputs. The chip supports single/dual input/output modes, > +handling up to two two input LVDS stream and up to two digital CMOS/TTL > outputs. > + > +Single or dual operation modes, output data mapping and DDR output modes > are > +configured through input signals and the chip does not expose any control > bus. > + > +Required properties: > +- compatible: Shall be "thine,thc63lvd1024" > + > +Optional properties: > +- vcc-supply: Power supply for TTL output and digital circuitry > +- cvcc-supply: Power supply for TTL CLOCKOUT signal > +- lvcc-supply: Power supply for LVDS inputs > +- pvcc-supply: Power supply for PLL circuitry As explained in a comment to one of the previous versions of this series, I'm tempted to make vcc-supply mandatory and drop the three other power supplies for now, as I believe there's very little chance they will be connected to separately controllable regulators (all supplies use the same voltage). In the very unlikely event that this occurs in design we need to support in the future, the cvcc, lvcc and pvcc supplies can be added later as optional without breaking backward compatibility. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +- pdwn-gpios: Power down GPIO signal. Active low > +- oe-gpios: Output enable GPIO signal. Active high > + > +The THC63LVD1024 video port connections are modeled according > +to OF graph bindings specified by > Documentation/devicetree/bindings/graph.txt > + > +Required video port nodes: > +- Port@0: First LVDS input port > +- Port@2: First digital CMOS/TTL parallel output > + > +Optional video port nodes: > +- Port@1: Second LVDS input port > +- Port@3: Second digital CMOS/TTL parallel output > + > +Example: > +-------- > + > + thc63lvd1024: lvds-decoder { > + compatible = "thine,thc63lvd1024"; > + > + vcc-supply = <®_lvds_vcc>; > + lvcc-supply = <®_lvds_lvcc>; > + > + pdwn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + > + lvds_dec_in_0: endpoint { > + remote-endpoint = <&lvds_out>; > + }; > + }; > + > + port@2{ > + reg = <2>; > + > + lvds_dec_out_2: endpoint { > + remote-endpoint = <&adv7511_in>; > + }; > + > + }; > + > + }; > + }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-20 12:43 ` Laurent Pinchart @ 2018-03-26 12:08 ` jacopo mondi 2018-03-26 22:22 ` Rob Herring 1 sibling, 0 replies; 32+ messages in thread From: jacopo mondi @ 2018-03-26 12:08 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Laurent, On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > (CC'ing Rob) > > Thank you for the patch. > > On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: > > Document Thine THC63LVD1024 LVDS decoder device tree bindings. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > new file mode 100644 > > index 0000000..8225c6a > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > @@ -0,0 +1,66 @@ > > +Thine Electronics THC63LVD1024 LVDS decoder > > +------------------------------------------- > > + > > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > > streams > > +to parallel data outputs. The chip supports single/dual input/output modes, > > +handling up to two two input LVDS stream and up to two digital CMOS/TTL > > outputs. > > + > > +Single or dual operation modes, output data mapping and DDR output modes > > are > > +configured through input signals and the chip does not expose any control > > bus. > > + > > +Required properties: > > +- compatible: Shall be "thine,thc63lvd1024" > > + > > +Optional properties: > > +- vcc-supply: Power supply for TTL output and digital circuitry > > +- cvcc-supply: Power supply for TTL CLOCKOUT signal > > +- lvcc-supply: Power supply for LVDS inputs > > +- pvcc-supply: Power supply for PLL circuitry > > As explained in a comment to one of the previous versions of this series, I'm > tempted to make vcc-supply mandatory and drop the three other power supplies > for now, as I believe there's very little chance they will be connected to > separately controllable regulators (all supplies use the same voltage). In the > very unlikely event that this occurs in design we need to support in the > future, the cvcc, lvcc and pvcc supplies can be added later as optional > without breaking backward compatibility. Fine, you and Andrzej both agree on this, and I actually do, as all the supplies have the same voltage. I'll make vcc the only and mandatory supply. I'll keep Andrzej Reviwed-by as he suggested it, and add yours. Thanks j > > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +- pdwn-gpios: Power down GPIO signal. Active low > > +- oe-gpios: Output enable GPIO signal. Active high > > + > > +The THC63LVD1024 video port connections are modeled according > > +to OF graph bindings specified by > > Documentation/devicetree/bindings/graph.txt > > + > > +Required video port nodes: > > +- Port@0: First LVDS input port > > +- Port@2: First digital CMOS/TTL parallel output > > + > > +Optional video port nodes: > > +- Port@1: Second LVDS input port > > +- Port@3: Second digital CMOS/TTL parallel output > > + > > +Example: > > +-------- > > + > > + thc63lvd1024: lvds-decoder { > > + compatible = "thine,thc63lvd1024"; > > + > > + vcc-supply = <®_lvds_vcc>; > > + lvcc-supply = <®_lvds_lvcc>; > > + > > + pdwn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + > > + lvds_dec_in_0: endpoint { > > + remote-endpoint = <&lvds_out>; > > + }; > > + }; > > + > > + port@2{ > > + reg = <2>; > > + > > + lvds_dec_out_2: endpoint { > > + remote-endpoint = <&adv7511_in>; > > + }; > > + > > + }; > > + > > + }; > > + }; > > -- > Regards, > > Laurent Pinchart > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-20 12:43 ` Laurent Pinchart 2018-03-26 12:08 ` jacopo mondi @ 2018-03-26 22:22 ` Rob Herring 2018-03-27 6:15 ` Vladimir Zapolskiy 1 sibling, 1 reply; 32+ messages in thread From: Rob Herring @ 2018-03-26 22:22 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > (CC'ing Rob) > > Thank you for the patch. > > On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: > > Document Thine THC63LVD1024 LVDS decoder device tree bindings. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > new file mode 100644 > > index 0000000..8225c6a > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > > @@ -0,0 +1,66 @@ > > +Thine Electronics THC63LVD1024 LVDS decoder > > +------------------------------------------- > > + > > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > > streams > > +to parallel data outputs. The chip supports single/dual input/output modes, > > +handling up to two two input LVDS stream and up to two digital CMOS/TTL > > outputs. > > + > > +Single or dual operation modes, output data mapping and DDR output modes > > are > > +configured through input signals and the chip does not expose any control > > bus. > > + > > +Required properties: > > +- compatible: Shall be "thine,thc63lvd1024" > > + > > +Optional properties: > > +- vcc-supply: Power supply for TTL output and digital circuitry > > +- cvcc-supply: Power supply for TTL CLOCKOUT signal > > +- lvcc-supply: Power supply for LVDS inputs > > +- pvcc-supply: Power supply for PLL circuitry > > As explained in a comment to one of the previous versions of this series, I'm > tempted to make vcc-supply mandatory and drop the three other power supplies > for now, as I believe there's very little chance they will be connected to > separately controllable regulators (all supplies use the same voltage). In the > very unlikely event that this occurs in design we need to support in the > future, the cvcc, lvcc and pvcc supplies can be added later as optional > without breaking backward compatibility. I'm okay with that. > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +- pdwn-gpios: Power down GPIO signal. Active low powerdown-gpios is the semi-standard name. > > +- oe-gpios: Output enable GPIO signal. Active high > > + > > +The THC63LVD1024 video port connections are modeled according > > +to OF graph bindings specified by > > Documentation/devicetree/bindings/graph.txt > > + > > +Required video port nodes: > > +- Port@0: First LVDS input port > > +- Port@2: First digital CMOS/TTL parallel output s/Port/port/ > > + > > +Optional video port nodes: > > +- Port@1: Second LVDS input port > > +- Port@3: Second digital CMOS/TTL parallel output > > + > > +Example: > > +-------- > > + > > + thc63lvd1024: lvds-decoder { > > + compatible = "thine,thc63lvd1024"; > > + > > + vcc-supply = <®_lvds_vcc>; > > + lvcc-supply = <®_lvds_lvcc>; > > + > > + pdwn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + port@0 { > > + reg = <0>; > > + > > + lvds_dec_in_0: endpoint { > > + remote-endpoint = <&lvds_out>; > > + }; > > + }; > > + > > + port@2{ > > + reg = <2>; > > + > > + lvds_dec_out_2: endpoint { > > + remote-endpoint = <&adv7511_in>; > > + }; > > + > > + }; > > + > > + }; > > + }; > > -- > Regards, > > Laurent Pinchart > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-26 22:22 ` Rob Herring @ 2018-03-27 6:15 ` Vladimir Zapolskiy 2018-03-27 7:12 ` Andrzej Hajda 0 siblings, 1 reply; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 6:15 UTC (permalink / raw) To: Jacopo Mondi Cc: Rob Herring, Laurent Pinchart, architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, On 03/27/2018 01:22 AM, Rob Herring wrote: > On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote: >> Hi Jacopo, >> >> (CC'ing Rob) >> >> Thank you for the patch. >> >> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: >>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> --- >>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>> 1 file changed, 66 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>> >>> diff --git >>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>> new file mode 100644 >>> index 0000000..8225c6a >>> --- /dev/null >>> +++ >>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>> @@ -0,0 +1,66 @@ >>> +Thine Electronics THC63LVD1024 LVDS decoder >>> +------------------------------------------- >>> + >>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>> streams >>> +to parallel data outputs. The chip supports single/dual input/output modes, >>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>> outputs. >>> + >>> +Single or dual operation modes, output data mapping and DDR output modes >>> are >>> +configured through input signals and the chip does not expose any control >>> bus. >>> + >>> +Required properties: >>> +- compatible: Shall be "thine,thc63lvd1024" >>> + >>> +Optional properties: >>> +- vcc-supply: Power supply for TTL output and digital circuitry >>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>> +- lvcc-supply: Power supply for LVDS inputs >>> +- pvcc-supply: Power supply for PLL circuitry >> >> As explained in a comment to one of the previous versions of this series, I'm >> tempted to make vcc-supply mandatory and drop the three other power supplies >> for now, as I believe there's very little chance they will be connected to >> separately controllable regulators (all supplies use the same voltage). In the >> very unlikely event that this occurs in design we need to support in the >> future, the cvcc, lvcc and pvcc supplies can be added later as optional >> without breaking backward compatibility. > > I'm okay with that. > >> Apart from that, >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >>> +- pdwn-gpios: Power down GPIO signal. Active low > > powerdown-gpios is the semi-standard name. > right, I've also noticed it. If possible please avoid shortenings in property names. >>> +- oe-gpios: Output enable GPIO signal. Active high >>> + And this one is also a not ever met property name, please consider to rename it to 'enable-gpios', for instance display panels define it. >>> +The THC63LVD1024 video port connections are modeled according >>> +to OF graph bindings specified by >>> Documentation/devicetree/bindings/graph.txt [snip] >>> + >>> + port@2{ >>> + reg = <2>; >>> + >>> + lvds_dec_out_2: endpoint { >>> + remote-endpoint = <&adv7511_in>; >>> + }; >>> + Drop a surplus empty line above. >>> + }; >>> + Drop a surplus empty line above. >>> + }; >>> + }; -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 6:15 ` Vladimir Zapolskiy @ 2018-03-27 7:12 ` Andrzej Hajda 2018-03-27 7:33 ` jacopo mondi 0 siblings, 1 reply; 32+ messages in thread From: Andrzej Hajda @ 2018-03-27 7:12 UTC (permalink / raw) To: Vladimir Zapolskiy, Jacopo Mondi Cc: Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel On 27.03.2018 08:15, Vladimir Zapolskiy wrote: > Hi Jacopo, > > On 03/27/2018 01:22 AM, Rob Herring wrote: >> On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote: >>> Hi Jacopo, >>> >>> (CC'ing Rob) >>> >>> Thank you for the patch. >>> >>> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: >>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>>> >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>> --- >>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>>> 1 file changed, 66 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>> new file mode 100644 >>>> index 0000000..8225c6a >>>> --- /dev/null >>>> +++ >>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>> @@ -0,0 +1,66 @@ >>>> +Thine Electronics THC63LVD1024 LVDS decoder >>>> +------------------------------------------- >>>> + >>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>>> streams >>>> +to parallel data outputs. The chip supports single/dual input/output modes, >>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>>> outputs. >>>> + >>>> +Single or dual operation modes, output data mapping and DDR output modes >>>> are >>>> +configured through input signals and the chip does not expose any control >>>> bus. >>>> + >>>> +Required properties: >>>> +- compatible: Shall be "thine,thc63lvd1024" >>>> + >>>> +Optional properties: >>>> +- vcc-supply: Power supply for TTL output and digital circuitry >>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>>> +- lvcc-supply: Power supply for LVDS inputs >>>> +- pvcc-supply: Power supply for PLL circuitry >>> As explained in a comment to one of the previous versions of this series, I'm >>> tempted to make vcc-supply mandatory and drop the three other power supplies >>> for now, as I believe there's very little chance they will be connected to >>> separately controllable regulators (all supplies use the same voltage). In the >>> very unlikely event that this occurs in design we need to support in the >>> future, the cvcc, lvcc and pvcc supplies can be added later as optional >>> without breaking backward compatibility. >> I'm okay with that. >> >>> Apart from that, >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> +- pdwn-gpios: Power down GPIO signal. Active low >> powerdown-gpios is the semi-standard name. >> > right, I've also noticed it. If possible please avoid shortenings in > property names. It is not shortening, it just follow pin name from decoder's datasheet. > >>>> +- oe-gpios: Output enable GPIO signal. Active high >>>> + > And this one is also a not ever met property name, please consider to > rename it to 'enable-gpios', for instance display panels define it. Again, it follows datasheet naming scheme. Has something changed in DT conventions? Regards Andrzej > >>>> +The THC63LVD1024 video port connections are modeled according >>>> +to OF graph bindings specified by >>>> Documentation/devicetree/bindings/graph.txt > [snip] > >>>> + >>>> + port@2{ >>>> + reg = <2>; >>>> + >>>> + lvds_dec_out_2: endpoint { >>>> + remote-endpoint = <&adv7511_in>; >>>> + }; >>>> + > Drop a surplus empty line above. > >>>> + }; >>>> + > Drop a surplus empty line above. > >>>> + }; >>>> + }; > -- > With best wishes, > Vladimir > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 7:12 ` Andrzej Hajda @ 2018-03-27 7:33 ` jacopo mondi 2018-03-27 8:27 ` Sergei Shtylyov 0 siblings, 1 reply; 32+ messages in thread From: jacopo mondi @ 2018-03-27 7:33 UTC (permalink / raw) To: Andrzej Hajda Cc: Vladimir Zapolskiy, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Andrzej, On Tue, Mar 27, 2018 at 09:12:46AM +0200, Andrzej Hajda wrote: > On 27.03.2018 08:15, Vladimir Zapolskiy wrote: > > Hi Jacopo, > > > > On 03/27/2018 01:22 AM, Rob Herring wrote: > >> On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote: > >>> Hi Jacopo, > >>> > >>> (CC'ing Rob) > >>> > >>> Thank you for the patch. > >>> > >>> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote: > >>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >>>> --- > >>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > >>>> 1 file changed, 66 insertions(+) > >>>> create mode 100644 > >>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>> > >>>> diff --git > >>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>> new file mode 100644 > >>>> index 0000000..8225c6a > >>>> --- /dev/null > >>>> +++ > >>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>> @@ -0,0 +1,66 @@ > >>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>> +------------------------------------------- > >>>> + > >>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > >>>> streams > >>>> +to parallel data outputs. The chip supports single/dual input/output modes, > >>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL > >>>> outputs. > >>>> + > >>>> +Single or dual operation modes, output data mapping and DDR output modes > >>>> are > >>>> +configured through input signals and the chip does not expose any control > >>>> bus. > >>>> + > >>>> +Required properties: > >>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>> + > >>>> +Optional properties: > >>>> +- vcc-supply: Power supply for TTL output and digital circuitry > >>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>> +- lvcc-supply: Power supply for LVDS inputs > >>>> +- pvcc-supply: Power supply for PLL circuitry > >>> As explained in a comment to one of the previous versions of this series, I'm > >>> tempted to make vcc-supply mandatory and drop the three other power supplies > >>> for now, as I believe there's very little chance they will be connected to > >>> separately controllable regulators (all supplies use the same voltage). In the > >>> very unlikely event that this occurs in design we need to support in the > >>> future, the cvcc, lvcc and pvcc supplies can be added later as optional > >>> without breaking backward compatibility. > >> I'm okay with that. > >> > >>> Apart from that, > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>>> +- pdwn-gpios: Power down GPIO signal. Active low > >> powerdown-gpios is the semi-standard name. > >> > > right, I've also noticed it. If possible please avoid shortenings in > > property names. > > It is not shortening, it just follow pin name from decoder's datasheet. > > > > >>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>> + > > And this one is also a not ever met property name, please consider to > > rename it to 'enable-gpios', for instance display panels define it. > > > Again, it follows datasheet naming scheme. Has something changed in DT > conventions? Seconded. My understanding is that the property name should reflect what reported in the the chip manual. For THC63LVD1024 the enable and power down pins are named 'OE' and 'PDWN' respectively. Thanks j > > Regards > Andrzej > > > > >>>> +The THC63LVD1024 video port connections are modeled according > >>>> +to OF graph bindings specified by > >>>> Documentation/devicetree/bindings/graph.txt > > [snip] > > > >>>> + > >>>> + port@2{ > >>>> + reg = <2>; > >>>> + > >>>> + lvds_dec_out_2: endpoint { > >>>> + remote-endpoint = <&adv7511_in>; > >>>> + }; > >>>> + > > Drop a surplus empty line above. > > > >>>> + }; > >>>> + > > Drop a surplus empty line above. > > > >>>> + }; > >>>> + }; > > -- > > With best wishes, > > Vladimir > > > > > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 7:33 ` jacopo mondi @ 2018-03-27 8:27 ` Sergei Shtylyov 2018-03-27 8:30 ` Vladimir Zapolskiy 0 siblings, 1 reply; 32+ messages in thread From: Sergei Shtylyov @ 2018-03-27 8:27 UTC (permalink / raw) To: jacopo mondi, Andrzej Hajda Cc: Vladimir Zapolskiy, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hello! On 3/27/2018 10:33 AM, jacopo mondi wrote: [...] >>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>>>>> >>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>>> --- >>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>>>>> 1 file changed, 66 insertions(+) >>>>>> create mode 100644 >>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>> >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>> new file mode 100644 >>>>>> index 0000000..8225c6a >>>>>> --- /dev/null >>>>>> +++ >>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>> @@ -0,0 +1,66 @@ >>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >>>>>> +------------------------------------------- >>>>>> + >>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>>>>> streams >>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, >>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>>>>> outputs. >>>>>> + >>>>>> +Single or dual operation modes, output data mapping and DDR output modes >>>>>> are >>>>>> +configured through input signals and the chip does not expose any control >>>>>> bus. >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible: Shall be "thine,thc63lvd1024" >>>>>> + >>>>>> +Optional properties: >>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry >>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>>>>> +- lvcc-supply: Power supply for LVDS inputs >>>>>> +- pvcc-supply: Power supply for PLL circuitry >>>>> As explained in a comment to one of the previous versions of this series, I'm >>>>> tempted to make vcc-supply mandatory and drop the three other power supplies >>>>> for now, as I believe there's very little chance they will be connected to >>>>> separately controllable regulators (all supplies use the same voltage). In the >>>>> very unlikely event that this occurs in design we need to support in the >>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional >>>>> without breaking backward compatibility. >>>> I'm okay with that. >>>> >>>>> Apart from that, >>>>> >>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>> >>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >>>> powerdown-gpios is the semi-standard name. >>>> >>> right, I've also noticed it. If possible please avoid shortenings in >>> property names. >> >> It is not shortening, it just follow pin name from decoder's datasheet. >> >>> >>>>>> +- oe-gpios: Output enable GPIO signal. Active high >>>>>> + >>> And this one is also a not ever met property name, please consider to >>> rename it to 'enable-gpios', for instance display panels define it. >> >> >> Again, it follows datasheet naming scheme. Has something changed in DT >> conventions? > > Seconded. My understanding is that the property name should reflect > what reported in the the chip manual. For THC63LVD1024 the enable and > power down pins are named 'OE' and 'PDWN' respectively. But don't we need the vendor prefix in the prop names then, like "renesas,oe-gpios" then? > Thanks > j MBR, Sergei ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 8:27 ` Sergei Shtylyov @ 2018-03-27 8:30 ` Vladimir Zapolskiy 2018-03-27 8:57 ` jacopo mondi 0 siblings, 1 reply; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 8:30 UTC (permalink / raw) To: Sergei Shtylyov, jacopo mondi, Andrzej Hajda Cc: Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Sergei, On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > Hello! > > On 3/27/2018 10:33 AM, jacopo mondi wrote: > [...] >>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>>>>>> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>>>> --- >>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>>>>>> 1 file changed, 66 insertions(+) >>>>>>> create mode 100644 >>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>> new file mode 100644 >>>>>>> index 0000000..8225c6a >>>>>>> --- /dev/null >>>>>>> +++ >>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>> @@ -0,0 +1,66 @@ >>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >>>>>>> +------------------------------------------- >>>>>>> + >>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>>>>>> streams >>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, >>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>>>>>> outputs. >>>>>>> + >>>>>>> +Single or dual operation modes, output data mapping and DDR output modes >>>>>>> are >>>>>>> +configured through input signals and the chip does not expose any control >>>>>>> bus. >>>>>>> + >>>>>>> +Required properties: >>>>>>> +- compatible: Shall be "thine,thc63lvd1024" >>>>>>> + >>>>>>> +Optional properties: >>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry >>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>>>>>> +- lvcc-supply: Power supply for LVDS inputs >>>>>>> +- pvcc-supply: Power supply for PLL circuitry >>>>>> As explained in a comment to one of the previous versions of this series, I'm >>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies >>>>>> for now, as I believe there's very little chance they will be connected to >>>>>> separately controllable regulators (all supplies use the same voltage). In the >>>>>> very unlikely event that this occurs in design we need to support in the >>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional >>>>>> without breaking backward compatibility. >>>>> I'm okay with that. >>>>> >>>>>> Apart from that, >>>>>> >>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>> >>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >>>>> powerdown-gpios is the semi-standard name. >>>>> >>>> right, I've also noticed it. If possible please avoid shortenings in >>>> property names. >>> >>> It is not shortening, it just follow pin name from decoder's datasheet. >>> >>>> >>>>>>> +- oe-gpios: Output enable GPIO signal. Active high >>>>>>> + >>>> And this one is also a not ever met property name, please consider to >>>> rename it to 'enable-gpios', for instance display panels define it. >>> >>> >>> Again, it follows datasheet naming scheme. Has something changed in DT >>> conventions? >> >> Seconded. My understanding is that the property name should reflect >> what reported in the the chip manual. For THC63LVD1024 the enable and >> power down pins are named 'OE' and 'PDWN' respectively. > > But don't we need the vendor prefix in the prop names then, like > "renesas,oe-gpios" then? > Seconded, with a correction to "thine,oe-gpios". If vendor agnostic properties are supposed to be used, then please follow the referenced by Rob semi-standard notations. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 8:30 ` Vladimir Zapolskiy @ 2018-03-27 8:57 ` jacopo mondi 2018-03-27 9:37 ` Vladimir Zapolskiy 0 siblings, 1 reply; 32+ messages in thread From: jacopo mondi @ 2018-03-27 8:57 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4559 bytes --] Hi Vladimir, On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: > Hi Sergei, > > On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > > Hello! > > > > On 3/27/2018 10:33 AM, jacopo mondi wrote: > > [...] > >>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. > >>>>>>> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >>>>>>> --- > >>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > >>>>>>> 1 file changed, 66 insertions(+) > >>>>>>> create mode 100644 > >>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>> > >>>>>>> diff --git > >>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>> new file mode 100644 > >>>>>>> index 0000000..8225c6a > >>>>>>> --- /dev/null > >>>>>>> +++ > >>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>> @@ -0,0 +1,66 @@ > >>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>>>>> +------------------------------------------- > >>>>>>> + > >>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > >>>>>>> streams > >>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, > >>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL > >>>>>>> outputs. > >>>>>>> + > >>>>>>> +Single or dual operation modes, output data mapping and DDR output modes > >>>>>>> are > >>>>>>> +configured through input signals and the chip does not expose any control > >>>>>>> bus. > >>>>>>> + > >>>>>>> +Required properties: > >>>>>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>>>>> + > >>>>>>> +Optional properties: > >>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry > >>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>>>>> +- lvcc-supply: Power supply for LVDS inputs > >>>>>>> +- pvcc-supply: Power supply for PLL circuitry > >>>>>> As explained in a comment to one of the previous versions of this series, I'm > >>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies > >>>>>> for now, as I believe there's very little chance they will be connected to > >>>>>> separately controllable regulators (all supplies use the same voltage). In the > >>>>>> very unlikely event that this occurs in design we need to support in the > >>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional > >>>>>> without breaking backward compatibility. > >>>>> I'm okay with that. > >>>>> > >>>>>> Apart from that, > >>>>>> > >>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>> > >>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > >>>>> powerdown-gpios is the semi-standard name. > >>>>> > >>>> right, I've also noticed it. If possible please avoid shortenings in > >>>> property names. > >>> > >>> It is not shortening, it just follow pin name from decoder's datasheet. > >>> > >>>> > >>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>>>>> + > >>>> And this one is also a not ever met property name, please consider to > >>>> rename it to 'enable-gpios', for instance display panels define it. > >>> > >>> > >>> Again, it follows datasheet naming scheme. Has something changed in DT > >>> conventions? > >> > >> Seconded. My understanding is that the property name should reflect > >> what reported in the the chip manual. For THC63LVD1024 the enable and > >> power down pins are named 'OE' and 'PDWN' respectively. > > > > But don't we need the vendor prefix in the prop names then, like > > "renesas,oe-gpios" then? > > > > Seconded, with a correction to "thine,oe-gpios". > mmm, okay then... A grep for that semi-standard properties names in Documentation/ returns only usage examples and no actual definitions, so I assume this is why they are semi-standard. Seems like there is some tribal knowledge involved in defining what is semi-standard and what's not, or are those properties documented somewhere? Thanks j > If vendor agnostic properties are supposed to be used, then please follow > the referenced by Rob semi-standard notations. > > -- > With best wishes, > Vladimir [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 8:57 ` jacopo mondi @ 2018-03-27 9:37 ` Vladimir Zapolskiy 2018-03-27 10:10 ` jacopo mondi 0 siblings, 1 reply; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 9:37 UTC (permalink / raw) To: jacopo mondi Cc: Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, On 03/27/2018 11:57 AM, jacopo mondi wrote: > Hi Vladimir, > > On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: >> Hi Sergei, >> >> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: >>> Hello! >>> >>> On 3/27/2018 10:33 AM, jacopo mondi wrote: >>> [...] >>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>>>>>>>> >>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>>>>>> --- >>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>>>>>>>> 1 file changed, 66 insertions(+) >>>>>>>>> create mode 100644 >>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>> new file mode 100644 >>>>>>>>> index 0000000..8225c6a >>>>>>>>> --- /dev/null >>>>>>>>> +++ >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>> @@ -0,0 +1,66 @@ >>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >>>>>>>>> +------------------------------------------- >>>>>>>>> + >>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>>>>>>>> streams >>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, >>>>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>>>>>>>> outputs. >>>>>>>>> + >>>>>>>>> +Single or dual operation modes, output data mapping and DDR output modes >>>>>>>>> are >>>>>>>>> +configured through input signals and the chip does not expose any control >>>>>>>>> bus. >>>>>>>>> + >>>>>>>>> +Required properties: >>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" >>>>>>>>> + >>>>>>>>> +Optional properties: >>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry >>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs >>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry >>>>>>>> As explained in a comment to one of the previous versions of this series, I'm >>>>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies >>>>>>>> for now, as I believe there's very little chance they will be connected to >>>>>>>> separately controllable regulators (all supplies use the same voltage). In the >>>>>>>> very unlikely event that this occurs in design we need to support in the >>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional >>>>>>>> without breaking backward compatibility. >>>>>>> I'm okay with that. >>>>>>> >>>>>>>> Apart from that, >>>>>>>> >>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>>> >>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >>>>>>> powerdown-gpios is the semi-standard name. >>>>>>> >>>>>> right, I've also noticed it. If possible please avoid shortenings in >>>>>> property names. >>>>> >>>>> It is not shortening, it just follow pin name from decoder's datasheet. >>>>> >>>>>> >>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high >>>>>>>>> + >>>>>> And this one is also a not ever met property name, please consider to >>>>>> rename it to 'enable-gpios', for instance display panels define it. >>>>> >>>>> >>>>> Again, it follows datasheet naming scheme. Has something changed in DT >>>>> conventions? >>>> >>>> Seconded. My understanding is that the property name should reflect >>>> what reported in the the chip manual. For THC63LVD1024 the enable and >>>> power down pins are named 'OE' and 'PDWN' respectively. >>> >>> But don't we need the vendor prefix in the prop names then, like >>> "renesas,oe-gpios" then? >>> >> >> Seconded, with a correction to "thine,oe-gpios". >> > > mmm, okay then... > > A grep for that semi-standard properties names in Documentation/ > returns only usage examples and no actual definitions, so I assume this > is why they are semi-standard. Here we have to be specific about a particular property, let it be 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l 0 $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l 86 While 'thine,oe-gpios' would be correct, I see no reason to introduce a vendor specific property to define a pin with a common and well understood purpose. If you go forward with the vendor specific prefix, apparently you can set the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I guess no. Standards do not define '-gpios' suffix, but partially the description is found in Documentation/bindings/gpio/gpio.txt, still it is not a section in any standard as far as I know. > Seems like there is some tribal knowledge involved in defining what > is semi-standard and what's not, or are those properties documented somewhere? > The point is that there is no formal standard which describes every IP, every IC and every single their property, some device node names and property names are recommended in ePAPR and Devicetree Specification though. Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. 'powerdown-gpios'. >> If vendor agnostic properties are supposed to be used, then please follow >> the referenced by Rob semi-standard notations. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 9:37 ` Vladimir Zapolskiy @ 2018-03-27 10:10 ` jacopo mondi 2018-03-27 11:03 ` Vladimir Zapolskiy 0 siblings, 1 reply; 32+ messages in thread From: jacopo mondi @ 2018-03-27 10:10 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 6939 bytes --] Hi Vladimir, On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: > Hi Jacopo, > > On 03/27/2018 11:57 AM, jacopo mondi wrote: > > Hi Vladimir, > > > > On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: > >> Hi Sergei, > >> > >> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > >>> Hello! > >>> > >>> On 3/27/2018 10:33 AM, jacopo mondi wrote: > >>> [...] > >>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >>>>>>>>> --- > >>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > >>>>>>>>> 1 file changed, 66 insertions(+) > >>>>>>>>> create mode 100644 > >>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>> > >>>>>>>>> diff --git > >>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>> new file mode 100644 > >>>>>>>>> index 0000000..8225c6a > >>>>>>>>> --- /dev/null > >>>>>>>>> +++ > >>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>> @@ -0,0 +1,66 @@ > >>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>>>>>>> +------------------------------------------- > >>>>>>>>> + > >>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > >>>>>>>>> streams > >>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, > >>>>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL > >>>>>>>>> outputs. > >>>>>>>>> + > >>>>>>>>> +Single or dual operation modes, output data mapping and DDR output modes > >>>>>>>>> are > >>>>>>>>> +configured through input signals and the chip does not expose any control > >>>>>>>>> bus. > >>>>>>>>> + > >>>>>>>>> +Required properties: > >>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>>>>>>> + > >>>>>>>>> +Optional properties: > >>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry > >>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs > >>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry > >>>>>>>> As explained in a comment to one of the previous versions of this series, I'm > >>>>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies > >>>>>>>> for now, as I believe there's very little chance they will be connected to > >>>>>>>> separately controllable regulators (all supplies use the same voltage). In the > >>>>>>>> very unlikely event that this occurs in design we need to support in the > >>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional > >>>>>>>> without breaking backward compatibility. > >>>>>>> I'm okay with that. > >>>>>>> > >>>>>>>> Apart from that, > >>>>>>>> > >>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>>> > >>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > >>>>>>> powerdown-gpios is the semi-standard name. > >>>>>>> > >>>>>> right, I've also noticed it. If possible please avoid shortenings in > >>>>>> property names. > >>>>> > >>>>> It is not shortening, it just follow pin name from decoder's datasheet. > >>>>> > >>>>>> > >>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>>>>>>> + > >>>>>> And this one is also a not ever met property name, please consider to > >>>>>> rename it to 'enable-gpios', for instance display panels define it. > >>>>> > >>>>> > >>>>> Again, it follows datasheet naming scheme. Has something changed in DT > >>>>> conventions? > >>>> > >>>> Seconded. My understanding is that the property name should reflect > >>>> what reported in the the chip manual. For THC63LVD1024 the enable and > >>>> power down pins are named 'OE' and 'PDWN' respectively. > >>> > >>> But don't we need the vendor prefix in the prop names then, like > >>> "renesas,oe-gpios" then? > >>> > >> > >> Seconded, with a correction to "thine,oe-gpios". > >> > > > > mmm, okay then... > > > > A grep for that semi-standard properties names in Documentation/ > > returns only usage examples and no actual definitions, so I assume this > > is why they are semi-standard. > > Here we have to be specific about a particular property, let it be 'oe-gpios' > vs. 'enable-gpios' and let's collect some statistics: > > % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l > 0 > > $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l > 86 > > While 'thine,oe-gpios' would be correct, I see no reason to introduce a vendor > specific property to define a pin with a common and well understood purpose. > > If you go forward with the vendor specific prefix, apparently you can set the name > to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the datasheet names > the pin as "OE GPIO" or "OE connected to a GPIO"? I guess no. > Let me clarify I don't want to push for a vendor specific name or similar, I'm fine with using 'semi-standard' names, I'm just confused by the 'semi-standard' definition. I guess from your examples, the usage count makes a difference here. > Standards do not define '-gpios' suffix, but partially the description is found > in Documentation/bindings/gpio/gpio.txt, still it is not a section in any > standard as far as I know. > > > Seems like there is some tribal knowledge involved in defining what > > is semi-standard and what's not, or are those properties documented somewhere? > > > > The point is that there is no formal standard which describes every IP, > every IC and every single their property, some device node names and property > names are recommended in ePAPR and Devicetree Specification though. > > Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST pin?) and > 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. 'powerdown-gpios'. I see all your points and I agree with most of them. Anyway, if the chip manual describes a pin as 'RST' I would not find it confusing to have a 'rst-gpio' defined in bindings :) Let me be a bit pesky here: what if a chip defines a reset GPIO, which is definitely a reset, but names it, say "XYZ" ? Would you prefer to see it defined as "reset-gpios" for consistency with other bindings, or "xyz-gpios" for consistency with documentation? Thanks j > > >> If vendor agnostic properties are supposed to be used, then please follow > >> the referenced by Rob semi-standard notations. > > -- > With best wishes, > Vladimir [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 10:10 ` jacopo mondi @ 2018-03-27 11:03 ` Vladimir Zapolskiy 2018-03-29 10:02 ` jacopo mondi 2018-04-02 13:36 ` Laurent Pinchart 0 siblings, 2 replies; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 11:03 UTC (permalink / raw) To: jacopo mondi Cc: Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, On 03/27/2018 01:10 PM, jacopo mondi wrote: > Hi Vladimir, > > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: >> Hi Jacopo, >> >> On 03/27/2018 11:57 AM, jacopo mondi wrote: >>> Hi Vladimir, >>> >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: >>>> Hi Sergei, >>>> >>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: >>>>> Hello! >>>>> >>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: >>>>> [...] >>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>>>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>>>>>>>> --- >>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ >>>>>>>>>>> 1 file changed, 66 insertions(+) >>>>>>>>>>> create mode 100644 >>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>>>> >>>>>>>>>>> diff --git >>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>>>> new file mode 100644 >>>>>>>>>>> index 0000000..8225c6a >>>>>>>>>>> --- /dev/null >>>>>>>>>>> +++ >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt >>>>>>>>>>> @@ -0,0 +1,66 @@ >>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >>>>>>>>>>> +------------------------------------------- >>>>>>>>>>> + >>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS >>>>>>>>>>> streams >>>>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, >>>>>>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL >>>>>>>>>>> outputs. >>>>>>>>>>> + >>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output modes >>>>>>>>>>> are >>>>>>>>>>> +configured through input signals and the chip does not expose any control >>>>>>>>>>> bus. >>>>>>>>>>> + >>>>>>>>>>> +Required properties: >>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" >>>>>>>>>>> + >>>>>>>>>>> +Optional properties: >>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry >>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs >>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry >>>>>>>>>> As explained in a comment to one of the previous versions of this series, I'm >>>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies >>>>>>>>>> for now, as I believe there's very little chance they will be connected to >>>>>>>>>> separately controllable regulators (all supplies use the same voltage). In the >>>>>>>>>> very unlikely event that this occurs in design we need to support in the >>>>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional >>>>>>>>>> without breaking backward compatibility. >>>>>>>>> I'm okay with that. >>>>>>>>> >>>>>>>>>> Apart from that, >>>>>>>>>> >>>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>>>>> >>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >>>>>>>>> powerdown-gpios is the semi-standard name. >>>>>>>>> >>>>>>>> right, I've also noticed it. If possible please avoid shortenings in >>>>>>>> property names. >>>>>>> >>>>>>> It is not shortening, it just follow pin name from decoder's datasheet. >>>>>>> >>>>>>>> >>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high >>>>>>>>>>> + >>>>>>>> And this one is also a not ever met property name, please consider to >>>>>>>> rename it to 'enable-gpios', for instance display panels define it. >>>>>>> >>>>>>> >>>>>>> Again, it follows datasheet naming scheme. Has something changed in DT >>>>>>> conventions? >>>>>> >>>>>> Seconded. My understanding is that the property name should reflect >>>>>> what reported in the the chip manual. For THC63LVD1024 the enable and >>>>>> power down pins are named 'OE' and 'PDWN' respectively. >>>>> >>>>> But don't we need the vendor prefix in the prop names then, like >>>>> "renesas,oe-gpios" then? >>>>> >>>> >>>> Seconded, with a correction to "thine,oe-gpios". >>>> >>> >>> mmm, okay then... >>> >>> A grep for that semi-standard properties names in Documentation/ >>> returns only usage examples and no actual definitions, so I assume this >>> is why they are semi-standard. >> >> Here we have to be specific about a particular property, let it be 'oe-gpios' >> vs. 'enable-gpios' and let's collect some statistics: >> >> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l >> 0 >> >> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l >> 86 >> >> While 'thine,oe-gpios' would be correct, I see no reason to introduce a vendor >> specific property to define a pin with a common and well understood purpose. >> >> If you go forward with the vendor specific prefix, apparently you can set the name >> to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the datasheet names >> the pin as "OE GPIO" or "OE connected to a GPIO"? I guess no. >> > > Let me clarify I don't want to push for a vendor specific name or > similar, I'm fine with using 'semi-standard' names, I'm just confused > by the 'semi-standard' definition. I guess from your examples, the > usage count makes a difference here. yes, in gneneral you can read "semi-standard" as "widely used", thus collecting statistics is a good enough method to make a reasoning. Hopefully the next evolutionary step of "widely used" is "described in standard". >> Standards do not define '-gpios' suffix, but partially the description is found >> in Documentation/bindings/gpio/gpio.txt, still it is not a section in any >> standard as far as I know. > >> >>> Seems like there is some tribal knowledge involved in defining what >>> is semi-standard and what's not, or are those properties documented somewhere? >>> >> >> The point is that there is no formal standard which describes every IP, >> every IC and every single their property, some device node names and property >> names are recommended in ePAPR and Devicetree Specification though. >> >> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST pin?) and >> 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. 'powerdown-gpios'. > > I see all your points and I agree with most of them. Anyway, if the > chip manual describes a pin as 'RST' I would not find it confusing to > have a 'rst-gpio' defined in bindings :) > > Let me be a bit pesky here: what if a chip defines a reset GPIO, which > is definitely a reset, but names it, say "XYZ" ? Would you prefer to > see it defined as "reset-gpios" for consistency with other bindings, > or "xyz-gpios" for consistency with documentation? If a pin is definitely an IC reset as you said, then my preference is to see it described under 'reset-gpios' property name, plus a comment in the IC device tree documentation document about it. I can provide two reasons to advocate my position: 1) developers spend significantly more time reading and editing the actual DTSI/DTS board files rather than reading and editing documentation, it makes sense to use common property names to save time and reduce amount of "what does 'oe' stand for?" type of questions; I suppose that the recommendation to avoid not "widely used" abbreviations in device node and property names arises from the same reasoning, 2) "widely used" and "standard" properties are excellent candidates for developing (or re-using) generalization wrappers, it happened so many times in the past, and this process shall be supported in my opinion; due to compatibility restrictions it might be problematic to change property names, and every new exception to "widely used" properties makes problematic to develop and maintain these kinds of wrappers, and of course it postpones a desired "described in standard" recognition. If my point of view is accepted, I do admit that a developer who translates a board schematics to board DTS file may experience a minor discomfort, which is mitigated if relevant pin names are found in device tree binding documentation in comments to properties, still the overall gain is noticeably higher in my personal opinion. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 11:03 ` Vladimir Zapolskiy @ 2018-03-29 10:02 ` jacopo mondi 2018-04-02 13:36 ` Laurent Pinchart 1 sibling, 0 replies; 32+ messages in thread From: jacopo mondi @ 2018-03-29 10:02 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, Laurent Pinchart, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 9258 bytes --] Hi Vladimir, On Tue, Mar 27, 2018 at 02:03:25PM +0300, Vladimir Zapolskiy wrote: > Hi Jacopo, > > On 03/27/2018 01:10 PM, jacopo mondi wrote: > > Hi Vladimir, > > > > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: > >> Hi Jacopo, > >> > >> On 03/27/2018 11:57 AM, jacopo mondi wrote: > >>> Hi Vladimir, > >>> > >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: > >>>> Hi Sergei, > >>>> > >>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > >>>>> Hello! > >>>>> > >>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: > >>>>> [...] > >>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >>>>>>>>>>> --- > >>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++++++++++++++++++ > >>>>>>>>>>> 1 file changed, 66 insertions(+) > >>>>>>>>>>> create mode 100644 > >>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>>>> > >>>>>>>>>>> diff --git > >>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>>>> new file mode 100644 > >>>>>>>>>>> index 0000000..8225c6a > >>>>>>>>>>> --- /dev/null > >>>>>>>>>>> +++ > >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt > >>>>>>>>>>> @@ -0,0 +1,66 @@ > >>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>>>>>>>>> +------------------------------------------- > >>>>>>>>>>> + > >>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS > >>>>>>>>>>> streams > >>>>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output modes, > >>>>>>>>>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL > >>>>>>>>>>> outputs. > >>>>>>>>>>> + > >>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output modes > >>>>>>>>>>> are > >>>>>>>>>>> +configured through input signals and the chip does not expose any control > >>>>>>>>>>> bus. > >>>>>>>>>>> + > >>>>>>>>>>> +Required properties: > >>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>>>>>>>>> + > >>>>>>>>>>> +Optional properties: > >>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry > >>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs > >>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry > >>>>>>>>>> As explained in a comment to one of the previous versions of this series, I'm > >>>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power supplies > >>>>>>>>>> for now, as I believe there's very little chance they will be connected to > >>>>>>>>>> separately controllable regulators (all supplies use the same voltage). In the > >>>>>>>>>> very unlikely event that this occurs in design we need to support in the > >>>>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional > >>>>>>>>>> without breaking backward compatibility. > >>>>>>>>> I'm okay with that. > >>>>>>>>> > >>>>>>>>>> Apart from that, > >>>>>>>>>> > >>>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>>>>> > >>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > >>>>>>>>> powerdown-gpios is the semi-standard name. > >>>>>>>>> > >>>>>>>> right, I've also noticed it. If possible please avoid shortenings in > >>>>>>>> property names. > >>>>>>> > >>>>>>> It is not shortening, it just follow pin name from decoder's datasheet. > >>>>>>> > >>>>>>>> > >>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>>>>>>>>> + > >>>>>>>> And this one is also a not ever met property name, please consider to > >>>>>>>> rename it to 'enable-gpios', for instance display panels define it. > >>>>>>> > >>>>>>> > >>>>>>> Again, it follows datasheet naming scheme. Has something changed in DT > >>>>>>> conventions? > >>>>>> > >>>>>> Seconded. My understanding is that the property name should reflect > >>>>>> what reported in the the chip manual. For THC63LVD1024 the enable and > >>>>>> power down pins are named 'OE' and 'PDWN' respectively. > >>>>> > >>>>> But don't we need the vendor prefix in the prop names then, like > >>>>> "renesas,oe-gpios" then? > >>>>> > >>>> > >>>> Seconded, with a correction to "thine,oe-gpios". > >>>> > >>> > >>> mmm, okay then... > >>> > >>> A grep for that semi-standard properties names in Documentation/ > >>> returns only usage examples and no actual definitions, so I assume this > >>> is why they are semi-standard. > >> > >> Here we have to be specific about a particular property, let it be 'oe-gpios' > >> vs. 'enable-gpios' and let's collect some statistics: > >> > >> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l > >> 0 > >> > >> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l > >> 86 > >> > >> While 'thine,oe-gpios' would be correct, I see no reason to introduce a vendor > >> specific property to define a pin with a common and well understood purpose. > >> > >> If you go forward with the vendor specific prefix, apparently you can set the name > >> to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the datasheet names > >> the pin as "OE GPIO" or "OE connected to a GPIO"? I guess no. > >> > > > > Let me clarify I don't want to push for a vendor specific name or > > similar, I'm fine with using 'semi-standard' names, I'm just confused > > by the 'semi-standard' definition. I guess from your examples, the > > usage count makes a difference here. > > yes, in gneneral you can read "semi-standard" as "widely used", thus collecting > statistics is a good enough method to make a reasoning. > > Hopefully the next evolutionary step of "widely used" is "described in standard". > > >> Standards do not define '-gpios' suffix, but partially the description is found > >> in Documentation/bindings/gpio/gpio.txt, still it is not a section in any > >> standard as far as I know. > > > >> > >>> Seems like there is some tribal knowledge involved in defining what > >>> is semi-standard and what's not, or are those properties documented somewhere? > >>> > >> > >> The point is that there is no formal standard which describes every IP, > >> every IC and every single their property, some device node names and property > >> names are recommended in ePAPR and Devicetree Specification though. > >> > >> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST pin?) and > >> 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. 'powerdown-gpios'. > > > > I see all your points and I agree with most of them. Anyway, if the > > chip manual describes a pin as 'RST' I would not find it confusing to > > have a 'rst-gpio' defined in bindings :) > > > > Let me be a bit pesky here: what if a chip defines a reset GPIO, which > > is definitely a reset, but names it, say "XYZ" ? Would you prefer to > > see it defined as "reset-gpios" for consistency with other bindings, > > or "xyz-gpios" for consistency with documentation? > > If a pin is definitely an IC reset as you said, then my preference is to see > it described under 'reset-gpios' property name, plus a comment in the IC > device tree documentation document about it. I can provide two reasons to > advocate my position: > > 1) developers spend significantly more time reading and editing the actual > DTSI/DTS board files rather than reading and editing documentation, > it makes sense to use common property names to save time and reduce > amount of "what does 'oe' stand for?" type of questions; I suppose > that the recommendation to avoid not "widely used" abbreviations in > device node and property names arises from the same reasoning, > > 2) "widely used" and "standard" properties are excellent candidates for > developing (or re-using) generalization wrappers, it happened so many > times in the past, and this process shall be supported in my opinion; > due to compatibility restrictions it might be problematic to change > property names, and every new exception to "widely used" properties > makes problematic to develop and maintain these kinds of wrappers, and > of course it postpones a desired "described in standard" recognition. > > If my point of view is accepted, I do admit that a developer who > translates a board schematics to board DTS file may experience a minor > discomfort, which is mitigated if relevant pin names are found in device > tree binding documentation in comments to properties, still the overall > gain is noticeably higher in my personal opinion. > Thank you for sharing your point of view. Makes much sense actually. I will use semi-standard names in v7 bindings. Thanks j > -- > With best wishes, > Vladimir [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-03-27 11:03 ` Vladimir Zapolskiy 2018-03-29 10:02 ` jacopo mondi @ 2018-04-02 13:36 ` Laurent Pinchart 2018-04-03 12:33 ` jacopo mondi 2018-04-05 16:33 ` Rob Herring 1 sibling, 2 replies; 32+ messages in thread From: Laurent Pinchart @ 2018-04-02 13:36 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: jacopo mondi, Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Rob Herring, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Vladimir, On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote: > On 03/27/2018 01:10 PM, jacopo mondi wrote: > > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: > >> On 03/27/2018 11:57 AM, jacopo mondi wrote: > >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: > >>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > >>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: > >>>>> [...] > >>>>> > >>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. > >>>>>>>>>>> > >>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>>>>>>>>> Reviewed-by: Niklas Söderlund > >>>>>>>>>>> <niklas.soderlund+renesas@ragnatech.se> > >>>>>>>>>>> --- > >>>>>>>>>>> > >>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 > >>>>>>>>>>> +++++++++++++++++++ > >>>>>>>>>>> 1 file changed, 66 insertions(+) > >>>>>>>>>>> create mode 100644 > >>>>>>>>>>> > >>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1 > >>>>>>>>>>> 024.txt > >>>>>>>>>>> > >>>>>>>>>>> diff --git > >>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lv > >>>>>>>>>>> d1024.txt > >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv > >>>>>>>>>>> d1024.txt > >>>>>>>>>>> new file mode 100644 > >>>>>>>>>>> index 0000000..8225c6a > >>>>>>>>>>> --- /dev/null > >>>>>>>>>>> +++ > >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv > >>>>>>>>>>> d1024.txt > >>>>>>>>>>> @@ -0,0 +1,66 @@ > >>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>>>>>>>>> +------------------------------------------- > >>>>>>>>>>> + > >>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to > >>>>>>>>>>> convert LVDS streams > >>>>>>>>>>> +to parallel data outputs. The chip supports single/dual > >>>>>>>>>>> input/output modes, +handling up to two two input LVDS stream > >>>>>>>>>>> and up to two digital CMOS/TTL outputs. > >>>>>>>>>>> + > >>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR > >>>>>>>>>>> output modes are > >>>>>>>>>>> +configured through input signals and the chip does not expose > >>>>>>>>>>> any control bus. > >>>>>>>>>>> + > >>>>>>>>>>> +Required properties: > >>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>>>>>>>>> + > >>>>>>>>>>> +Optional properties: > >>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry > >>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs > >>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry > >>>>>>>>>> > >>>>>>>>>> As explained in a comment to one of the previous versions of this > >>>>>>>>>> series, I'm tempted to make vcc-supply mandatory and drop the > >>>>>>>>>> three other power supplies for now, as I believe there's very > >>>>>>>>>> little chance they will be connected to separately controllable > >>>>>>>>>> regulators (all supplies use the same voltage). In the very > >>>>>>>>>> unlikely event that this occurs in design we need to support in > >>>>>>>>>> the future, the cvcc, lvcc and pvcc supplies can be added later > >>>>>>>>>> as optional without breaking backward compatibility. > >>>>>>>>> > >>>>>>>>> I'm okay with that. > >>>>>>>>> > >>>>>>>>>> Apart from that, > >>>>>>>>>> > >>>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>>>>>> > >>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > >>>>>>>>> > >>>>>>>>> powerdown-gpios is the semi-standard name. > >>>>>>>> > >>>>>>>> right, I've also noticed it. If possible please avoid shortenings > >>>>>>>> in property names. > >>>>>>> > >>>>>>> It is not shortening, it just follow pin name from decoder's > >>>>>>> datasheet. > >>>>>>> > >>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>>>>>>>>> + > >>>>>>>> > >>>>>>>> And this one is also a not ever met property name, please consider > >>>>>>>> to rename it to 'enable-gpios', for instance display panels define > >>>>>>>> it. > >>>>>>> > >>>>>>> Again, it follows datasheet naming scheme. Has something changed in > >>>>>>> DT conventions? > >>>>>> > >>>>>> Seconded. My understanding is that the property name should reflect > >>>>>> what reported in the the chip manual. For THC63LVD1024 the enable and > >>>>>> power down pins are named 'OE' and 'PDWN' respectively. > >>>>>> > >>>>> But don't we need the vendor prefix in the prop names then, like > >>>>> "renesas,oe-gpios" then? > >>>> > >>>> Seconded, with a correction to "thine,oe-gpios". > >>> > >>> mmm, okay then... > >>> > >>> A grep for that semi-standard properties names in Documentation/ > >>> returns only usage examples and no actual definitions, so I assume this > >>> is why they are semi-standard. > >> > >> Here we have to be specific about a particular property, let it be > >> 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: > >> > >> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l > >> 0 > >> > >> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l > >> 86 > >> > >> While 'thine,oe-gpios' would be correct, I see no reason to introduce a > >> vendor specific property to define a pin with a common and well > >> understood purpose. > >> > >> If you go forward with the vendor specific prefix, apparently you can set > >> the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the > >> datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I > >> guess no. > > > > Let me clarify I don't want to push for a vendor specific name or > > similar, I'm fine with using 'semi-standard' names, I'm just confused > > by the 'semi-standard' definition. I guess from your examples, the > > usage count makes a difference here. > > yes, in gneneral you can read "semi-standard" as "widely used", thus > collecting statistics is a good enough method to make a reasoning. > > Hopefully the next evolutionary step of "widely used" is "described in > standard". > > >> Standards do not define '-gpios' suffix, but partially the description is > >> found in Documentation/bindings/gpio/gpio.txt, still it is not a section > >> in any standard as far as I know. > >> > >>> Seems like there is some tribal knowledge involved in defining what > >>> is semi-standard and what's not, or are those properties documented > >>> somewhere? > >> > >> The point is that there is no formal standard which describes every IP, > >> every IC and every single their property, some device node names and > >> property names are recommended in ePAPR and Devicetree Specification > >> though. > >> > >> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST > >> pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. > >> 'powerdown-gpios'. > > > > I see all your points and I agree with most of them. Anyway, if the > > chip manual describes a pin as 'RST' I would not find it confusing to > > have a 'rst-gpio' defined in bindings :) > > > > Let me be a bit pesky here: what if a chip defines a reset GPIO, which > > is definitely a reset, but names it, say "XYZ" ? Would you prefer to > > see it defined as "reset-gpios" for consistency with other bindings, > > or "xyz-gpios" for consistency with documentation? > > If a pin is definitely an IC reset as you said, then my preference is to see > it described under 'reset-gpios' property name, plus a comment in the IC > device tree documentation document about it. I can provide two reasons to > advocate my position: > > 1) developers spend significantly more time reading and editing the actual > DTSI/DTS board files rather than reading and editing documentation, > it makes sense to use common property names to save time and reduce > amount of "what does 'oe' stand for?" type of questions; I suppose > that the recommendation to avoid not "widely used" abbreviations in > device node and property names arises from the same reasoning, > > 2) "widely used" and "standard" properties are excellent candidates for > developing (or re-using) generalization wrappers, it happened so many > times in the past, and this process shall be supported in my opinion; > due to compatibility restrictions it might be problematic to change > property names, and every new exception to "widely used" properties > makes problematic to develop and maintain these kinds of wrappers, and > of course it postpones a desired "described in standard" recognition. > > If my point of view is accepted, I do admit that a developer who > translates a board schematics to board DTS file may experience a minor > discomfort, which is mitigated if relevant pin names are found in device > tree binding documentation in comments to properties, still the overall > gain is noticeably higher in my personal opinion. I have to disagree with this. When using a property name that doesn't correspond to the hardware documentation, developers will need to refer to the DT bindings documentation to confirm the property name. "Widely used" property names will not save time, they will use more time. This is of course marginal and I don't think it would have any noticeable impact, but I don't think your argument holds. I'm all for standardizing properties across DT bindings for multiple components, but doing so in a semi-random fashion will in my opinion not result in any gain. We can decide that power-down or output-enable GPIOS should have common property names (and I'm not even sure that would be useful, but we can certainly discuss it), but in that case someone should make a proposal and get the names standardized. Unless we do so, no matter what property name gets picked for a particular binding, it won't become universally used by magic. I'd like to hear the DT bindings maintainers position on this matter. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-04-02 13:36 ` Laurent Pinchart @ 2018-04-03 12:33 ` jacopo mondi 2018-04-05 16:33 ` Rob Herring 1 sibling, 0 replies; 32+ messages in thread From: jacopo mondi @ 2018-04-03 12:33 UTC (permalink / raw) To: robh Cc: Laurent Pinchart, Vladimir Zapolskiy, Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, architt, airlied, horms, magnus.damm, geert, niklas.soderlund, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7562 bytes --] Hi Rob, sorry for pointing to you directly :) On Mon, Apr 02, 2018 at 04:36:55PM +0300, Laurent Pinchart wrote: > Hi Vladimir, > > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote: > > On 03/27/2018 01:10 PM, jacopo mondi wrote: > > > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: > > >> On 03/27/2018 11:57 AM, jacopo mondi wrote: > > >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: [snip] > > >>>>>>>>>> > > >>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > > >>>>>>>>> > > >>>>>>>>> powerdown-gpios is the semi-standard name. > > >>>>>>>> > > >>>>>>>> right, I've also noticed it. If possible please avoid shortenings > > >>>>>>>> in property names. > > >>>>>>> > > >>>>>>> It is not shortening, it just follow pin name from decoder's > > >>>>>>> datasheet. > > >>>>>>> > > >>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > > >>>>>>>>>>> + > > >>>>>>>> > > >>>>>>>> And this one is also a not ever met property name, please consider > > >>>>>>>> to rename it to 'enable-gpios', for instance display panels define > > >>>>>>>> it. > > >>>>>>> > > >>>>>>> Again, it follows datasheet naming scheme. Has something changed in > > >>>>>>> DT conventions? > > >>>>>> > > >>>>>> Seconded. My understanding is that the property name should reflect > > >>>>>> what reported in the the chip manual. For THC63LVD1024 the enable and > > >>>>>> power down pins are named 'OE' and 'PDWN' respectively. > > >>>>>> > > >>>>> But don't we need the vendor prefix in the prop names then, like > > >>>>> "renesas,oe-gpios" then? > > >>>> > > >>>> Seconded, with a correction to "thine,oe-gpios". > > >>> > > >>> mmm, okay then... > > >>> > > >>> A grep for that semi-standard properties names in Documentation/ > > >>> returns only usage examples and no actual definitions, so I assume this > > >>> is why they are semi-standard. > > >> > > >> Here we have to be specific about a particular property, let it be > > >> 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: > > >> > > >> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l > > >> 0 > > >> > > >> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l > > >> 86 > > >> > > >> While 'thine,oe-gpios' would be correct, I see no reason to introduce a > > >> vendor specific property to define a pin with a common and well > > >> understood purpose. > > >> > > >> If you go forward with the vendor specific prefix, apparently you can set > > >> the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the > > >> datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I > > >> guess no. > > > > > > Let me clarify I don't want to push for a vendor specific name or > > > similar, I'm fine with using 'semi-standard' names, I'm just confused > > > by the 'semi-standard' definition. I guess from your examples, the > > > usage count makes a difference here. > > > > yes, in gneneral you can read "semi-standard" as "widely used", thus > > collecting statistics is a good enough method to make a reasoning. > > > > Hopefully the next evolutionary step of "widely used" is "described in > > standard". > > > > >> Standards do not define '-gpios' suffix, but partially the description is > > >> found in Documentation/bindings/gpio/gpio.txt, still it is not a section > > >> in any standard as far as I know. > > >> > > >>> Seems like there is some tribal knowledge involved in defining what > > >>> is semi-standard and what's not, or are those properties documented > > >>> somewhere? > > >> > > >> The point is that there is no formal standard which describes every IP, > > >> every IC and every single their property, some device node names and > > >> property names are recommended in ePAPR and Devicetree Specification > > >> though. > > >> > > >> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST > > >> pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. > > >> 'powerdown-gpios'. > > > > > > I see all your points and I agree with most of them. Anyway, if the > > > chip manual describes a pin as 'RST' I would not find it confusing to > > > have a 'rst-gpio' defined in bindings :) > > > > > > Let me be a bit pesky here: what if a chip defines a reset GPIO, which > > > is definitely a reset, but names it, say "XYZ" ? Would you prefer to > > > see it defined as "reset-gpios" for consistency with other bindings, > > > or "xyz-gpios" for consistency with documentation? > > > > If a pin is definitely an IC reset as you said, then my preference is to see > > it described under 'reset-gpios' property name, plus a comment in the IC > > device tree documentation document about it. I can provide two reasons to > > advocate my position: > > > > 1) developers spend significantly more time reading and editing the actual > > DTSI/DTS board files rather than reading and editing documentation, > > it makes sense to use common property names to save time and reduce > > amount of "what does 'oe' stand for?" type of questions; I suppose > > that the recommendation to avoid not "widely used" abbreviations in > > device node and property names arises from the same reasoning, > > > > 2) "widely used" and "standard" properties are excellent candidates for > > developing (or re-using) generalization wrappers, it happened so many > > times in the past, and this process shall be supported in my opinion; > > due to compatibility restrictions it might be problematic to change > > property names, and every new exception to "widely used" properties > > makes problematic to develop and maintain these kinds of wrappers, and > > of course it postpones a desired "described in standard" recognition. > > > > If my point of view is accepted, I do admit that a developer who > > translates a board schematics to board DTS file may experience a minor > > discomfort, which is mitigated if relevant pin names are found in device > > tree binding documentation in comments to properties, still the overall > > gain is noticeably higher in my personal opinion. > > I have to disagree with this. When using a property name that doesn't > correspond to the hardware documentation, developers will need to refer to the > DT bindings documentation to confirm the property name. "Widely used" property > names will not save time, they will use more time. This is of course marginal > and I don't think it would have any noticeable impact, but I don't think your > argument holds. > > I'm all for standardizing properties across DT bindings for multiple > components, but doing so in a semi-random fashion will in my opinion not > result in any gain. We can decide that power-down or output-enable GPIOS > should have common property names (and I'm not even sure that would be useful, > but we can certainly discuss it), but in that case someone should make a > proposal and get the names standardized. Unless we do so, no matter what > property name gets picked for a particular binding, it won't become > universally used by magic. > > I'd like to hear the DT bindings maintainers position on this matter. > Me too :) As driver developer I see both Vladimir's and Laurent's points. I still prefer to reflect in bindings the pin name assigned in the chip manual, over semi-standard names, but that's a personal preference. In order to send next version I would like to know which direction do the dt custodians should be taken on this. Thanks j > -- > Regards, > > Laurent Pinchart > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-04-02 13:36 ` Laurent Pinchart 2018-04-03 12:33 ` jacopo mondi @ 2018-04-05 16:33 ` Rob Herring 2018-04-05 18:53 ` Laurent Pinchart 1 sibling, 1 reply; 32+ messages in thread From: Rob Herring @ 2018-04-05 16:33 UTC (permalink / raw) To: Laurent Pinchart Cc: Vladimir Zapolskiy, jacopo mondi, Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Archit Taneja, David Airlie, Simon Horman, Magnus Damm, Geert Uytterhoeven, Niklas Söderlund, Mark Rutland, dri-devel, open list:MEDIA DRIVERS FOR RENESAS - FCP, devicetree, linux-kernel On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Vladimir, > > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote: >> On 03/27/2018 01:10 PM, jacopo mondi wrote: >> > On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: >> >> On 03/27/2018 11:57 AM, jacopo mondi wrote: >> >>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: >> >>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: >> >>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: >> >>>>> [...] >> >>>>> >> >>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings. >> >>>>>>>>>>> >> >>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> >>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >> >>>>>>>>>>> Reviewed-by: Niklas Söderlund >> >>>>>>>>>>> <niklas.soderlund+renesas@ragnatech.se> >> >>>>>>>>>>> --- >> >>>>>>>>>>> >> >>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 >> >>>>>>>>>>> +++++++++++++++++++ >> >>>>>>>>>>> 1 file changed, 66 insertions(+) >> >>>>>>>>>>> create mode 100644 >> >>>>>>>>>>> >> >>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1 >> >>>>>>>>>>> 024.txt >> >>>>>>>>>>> >> >>>>>>>>>>> diff --git >> >>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lv >> >>>>>>>>>>> d1024.txt >> >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv >> >>>>>>>>>>> d1024.txt >> >>>>>>>>>>> new file mode 100644 >> >>>>>>>>>>> index 0000000..8225c6a >> >>>>>>>>>>> --- /dev/null >> >>>>>>>>>>> +++ >> >>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lv >> >>>>>>>>>>> d1024.txt >> >>>>>>>>>>> @@ -0,0 +1,66 @@ >> >>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >> >>>>>>>>>>> +------------------------------------------- >> >>>>>>>>>>> + >> >>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to >> >>>>>>>>>>> convert LVDS streams >> >>>>>>>>>>> +to parallel data outputs. The chip supports single/dual >> >>>>>>>>>>> input/output modes, +handling up to two two input LVDS stream >> >>>>>>>>>>> and up to two digital CMOS/TTL outputs. >> >>>>>>>>>>> + >> >>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR >> >>>>>>>>>>> output modes are >> >>>>>>>>>>> +configured through input signals and the chip does not expose >> >>>>>>>>>>> any control bus. >> >>>>>>>>>>> + >> >>>>>>>>>>> +Required properties: >> >>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" >> >>>>>>>>>>> + >> >>>>>>>>>>> +Optional properties: >> >>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry >> >>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >> >>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs >> >>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry >> >>>>>>>>>> >> >>>>>>>>>> As explained in a comment to one of the previous versions of this >> >>>>>>>>>> series, I'm tempted to make vcc-supply mandatory and drop the >> >>>>>>>>>> three other power supplies for now, as I believe there's very >> >>>>>>>>>> little chance they will be connected to separately controllable >> >>>>>>>>>> regulators (all supplies use the same voltage). In the very >> >>>>>>>>>> unlikely event that this occurs in design we need to support in >> >>>>>>>>>> the future, the cvcc, lvcc and pvcc supplies can be added later >> >>>>>>>>>> as optional without breaking backward compatibility. >> >>>>>>>>> >> >>>>>>>>> I'm okay with that. >> >>>>>>>>> >> >>>>>>>>>> Apart from that, >> >>>>>>>>>> >> >>>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >>>>>>>>>> >> >>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >> >>>>>>>>> >> >>>>>>>>> powerdown-gpios is the semi-standard name. >> >>>>>>>> >> >>>>>>>> right, I've also noticed it. If possible please avoid shortenings >> >>>>>>>> in property names. >> >>>>>>> >> >>>>>>> It is not shortening, it just follow pin name from decoder's >> >>>>>>> datasheet. >> >>>>>>> >> >>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high >> >>>>>>>>>>> + >> >>>>>>>> >> >>>>>>>> And this one is also a not ever met property name, please consider >> >>>>>>>> to rename it to 'enable-gpios', for instance display panels define >> >>>>>>>> it. >> >>>>>>> >> >>>>>>> Again, it follows datasheet naming scheme. Has something changed in >> >>>>>>> DT conventions? >> >>>>>> >> >>>>>> Seconded. My understanding is that the property name should reflect >> >>>>>> what reported in the the chip manual. For THC63LVD1024 the enable and >> >>>>>> power down pins are named 'OE' and 'PDWN' respectively. >> >>>>>> >> >>>>> But don't we need the vendor prefix in the prop names then, like >> >>>>> "renesas,oe-gpios" then? >> >>>> >> >>>> Seconded, with a correction to "thine,oe-gpios". >> >>> >> >>> mmm, okay then... >> >>> >> >>> A grep for that semi-standard properties names in Documentation/ >> >>> returns only usage examples and no actual definitions, so I assume this >> >>> is why they are semi-standard. >> >> >> >> Here we have to be specific about a particular property, let it be >> >> 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: >> >> >> >> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l >> >> 0 >> >> >> >> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l >> >> 86 >> >> >> >> While 'thine,oe-gpios' would be correct, I see no reason to introduce a >> >> vendor specific property to define a pin with a common and well >> >> understood purpose. >> >> >> >> If you go forward with the vendor specific prefix, apparently you can set >> >> the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does the >> >> datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I >> >> guess no. >> > >> > Let me clarify I don't want to push for a vendor specific name or >> > similar, I'm fine with using 'semi-standard' names, I'm just confused >> > by the 'semi-standard' definition. I guess from your examples, the >> > usage count makes a difference here. >> >> yes, in gneneral you can read "semi-standard" as "widely used", thus >> collecting statistics is a good enough method to make a reasoning. >> >> Hopefully the next evolutionary step of "widely used" is "described in >> standard". >> >> >> Standards do not define '-gpios' suffix, but partially the description is >> >> found in Documentation/bindings/gpio/gpio.txt, still it is not a section >> >> in any standard as far as I know. >> >> >> >>> Seems like there is some tribal knowledge involved in defining what >> >>> is semi-standard and what's not, or are those properties documented >> >>> somewhere? >> >> >> >> The point is that there is no formal standard which describes every IP, >> >> every IC and every single their property, some device node names and >> >> property names are recommended in ePAPR and Devicetree Specification >> >> though. >> >> >> >> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST >> >> pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' vs. >> >> 'powerdown-gpios'. >> > >> > I see all your points and I agree with most of them. Anyway, if the >> > chip manual describes a pin as 'RST' I would not find it confusing to >> > have a 'rst-gpio' defined in bindings :) >> > >> > Let me be a bit pesky here: what if a chip defines a reset GPIO, which >> > is definitely a reset, but names it, say "XYZ" ? Would you prefer to >> > see it defined as "reset-gpios" for consistency with other bindings, >> > or "xyz-gpios" for consistency with documentation? >> >> If a pin is definitely an IC reset as you said, then my preference is to see >> it described under 'reset-gpios' property name, plus a comment in the IC >> device tree documentation document about it. I can provide two reasons to >> advocate my position: >> >> 1) developers spend significantly more time reading and editing the actual >> DTSI/DTS board files rather than reading and editing documentation, >> it makes sense to use common property names to save time and reduce >> amount of "what does 'oe' stand for?" type of questions; I suppose >> that the recommendation to avoid not "widely used" abbreviations in >> device node and property names arises from the same reasoning, >> >> 2) "widely used" and "standard" properties are excellent candidates for >> developing (or re-using) generalization wrappers, it happened so many >> times in the past, and this process shall be supported in my opinion; >> due to compatibility restrictions it might be problematic to change >> property names, and every new exception to "widely used" properties >> makes problematic to develop and maintain these kinds of wrappers, and >> of course it postpones a desired "described in standard" recognition. >> >> If my point of view is accepted, I do admit that a developer who >> translates a board schematics to board DTS file may experience a minor >> discomfort, which is mitigated if relevant pin names are found in device >> tree binding documentation in comments to properties, still the overall >> gain is noticeably higher in my personal opinion. > > I have to disagree with this. When using a property name that doesn't > correspond to the hardware documentation, developers will need to refer to the > DT bindings documentation to confirm the property name. "Widely used" property > names will not save time, they will use more time. This is of course marginal > and I don't think it would have any noticeable impact, but I don't think your > argument holds. We can have it both ways. The name should follow the documented name/function. For example, we have enable-gpios which is simply the invert of powerdown-gpios (for software's purposes). Pick the one closest to the documentation. We're not trying to make bindings use "enable" if a signal is called "powerdown". What we don't want is gratuitous variation in the names based on the whims of hw designers: resetb-gpios resetn-gpios rst-gpios rstn-gpios nRESET-gpios ...you get the idea (and I left out vendor prefixes). > I'm all for standardizing properties across DT bindings for multiple > components, but doing so in a semi-random fashion will in my opinion not > result in any gain. We can decide that power-down or output-enable GPIOS > should have common property names (and I'm not even sure that would be useful, > but we can certainly discuss it), but in that case someone should make a > proposal and get the names standardized. Unless we do so, no matter what > property name gets picked for a particular binding, it won't become > universally used by magic. For "output enable", I suspect that is a common signal/function and should have a standardized name. Generally, the way this works is we get several variations and then we try to standardize things. I think we can all agree standardizing first is better. If you want to put it in a common place, please do. Maybe people will read that. Regardless, the only way to enforce following standard names is with review. Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar with h/w design should recognize OE. The reason to try and standardize names is so we can have common drivers or library functions. In particular, for things like GPIOs that need to be configured first for devices on otherwise discoverable buses, this is very useful. Rob ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-04-05 16:33 ` Rob Herring @ 2018-04-05 18:53 ` Laurent Pinchart 2018-04-05 21:27 ` Rob Herring 0 siblings, 1 reply; 32+ messages in thread From: Laurent Pinchart @ 2018-04-05 18:53 UTC (permalink / raw) To: Rob Herring Cc: Vladimir Zapolskiy, jacopo mondi, Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Archit Taneja, David Airlie, Simon Horman, Magnus Damm, Geert Uytterhoeven, Niklas Söderlund, Mark Rutland, dri-devel, open list:MEDIA DRIVERS FOR RENESAS - FCP, devicetree, linux-kernel Hi Rob, On Thursday, 5 April 2018 19:33:33 EEST Rob Herring wrote: > On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart wrote: > > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote: > >> On 03/27/2018 01:10 PM, jacopo mondi wrote: > >>> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: > >>>> On 03/27/2018 11:57 AM, jacopo mondi wrote: > >>>>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: > >>>>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: > >>>>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: > >>>>>>> [...] > >>>>>>> > >>>>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree > >>>>>>>>>>>>> bindings. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>>>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > >>>>>>>>>>>>> Reviewed-by: Niklas Söderlund > >>>>>>>>>>>>> <niklas.soderlund+renesas@ragnatech.se> > >>>>>>>>>>>>> --- > >>>>>>>>>>>>> > >>>>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++ > >>>>>>>>>>>>> 1 file changed, 66 insertions(+) > >>>>>>>>>>>>> create mode 100644 > >>>>>>>>>>>>> > >>>>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63l > >>>>>>>>>>>>> vd1024.txt > >>>>>>>>>>>>> diff --git > >>>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc6 > >>>>>>>>>>>>> 3lvd1024.txt > >>>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc6 > >>>>>>>>>>>>> 3lvd1024.txt > >>>>>>>>>>>>> new file mode 100644 > >>>>>>>>>>>>> index 0000000..8225c6a > >>>>>>>>>>>>> --- /dev/null > >>>>>>>>>>>>> +++ > >>>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc6 > >>>>>>>>>>>>> 3lvd1024.txt > >>>>>>>>>>>>> @@ -0,0 +1,66 @@ > >>>>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder > >>>>>>>>>>>>> +------------------------------------------- > >>>>>>>>>>>>> + > >>>>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to > >>>>>>>>>>>>> convert LVDS streams > >>>>>>>>>>>>> +to parallel data outputs. The chip supports single/dual > >>>>>>>>>>>>> input/output modes, > >>>>>>>>>>>>> +handling up to two two input LVDS stream and up to two > >>>>>>>>>>>>> digital CMOS/TTL outputs. > >>>>>>>>>>>>> + > >>>>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR > >>>>>>>>>>>>> output modes are > >>>>>>>>>>>>> +configured through input signals and the chip does not > >>>>>>>>>>>>> expose any control bus. > >>>>>>>>>>>>> + > >>>>>>>>>>>>> +Required properties: > >>>>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" > >>>>>>>>>>>>> + > >>>>>>>>>>>>> +Optional properties: > >>>>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital > >>>>>>>>>>>>> circuitry > >>>>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal > >>>>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs > >>>>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry > >>>>>>>>>>>> > >>>>>>>>>>>> As explained in a comment to one of the previous versions of > >>>>>>>>>>>> this series, I'm tempted to make vcc-supply mandatory and drop > >>>>>>>>>>>> the three other power supplies for now, as I believe there's > >>>>>>>>>>>> very little chance they will be connected to separately > >>>>>>>>>>>> controllable regulators (all supplies use the same voltage). In > >>>>>>>>>>>> the very unlikely event that this occurs in design we need to > >>>>>>>>>>>> support in the future, the cvcc, lvcc and pvcc supplies can be > >>>>>>>>>>>> added later as optional without breaking backward > >>>>>>>>>>>> compatibility. > >>>>>>>>>>> > >>>>>>>>>>> I'm okay with that. > >>>>>>>>>>> > >>>>>>>>>>>> Apart from that, > >>>>>>>>>>>> > >>>>>>>>>>>> Reviewed-by: Laurent Pinchart > >>>>>>>>>>>> <laurent.pinchart@ideasonboard.com> > >>>>>>>>>>>> > >>>>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low > >>>>>>>>>>> > >>>>>>>>>>> powerdown-gpios is the semi-standard name. > >>>>>>>>>> > >>>>>>>>>> right, I've also noticed it. If possible please avoid > >>>>>>>>>> shortenings in property names. > >>>>>>>>> > >>>>>>>>> It is not shortening, it just follow pin name from decoder's > >>>>>>>>> datasheet. > >>>>>>>>> > >>>>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high > >>>>>>>>>>>>> + > >>>>>>>>>> > >>>>>>>>>> And this one is also a not ever met property name, please > >>>>>>>>>> consider to rename it to 'enable-gpios', for instance display > >>>>>>>>>> panels define it. > >>>>>>>>> > >>>>>>>>> Again, it follows datasheet naming scheme. Has something changed > >>>>>>>>> in DT conventions? > >>>>>>>> > >>>>>>>> Seconded. My understanding is that the property name should > >>>>>>>> reflect what reported in the the chip manual. For THC63LVD1024 the > >>>>>>>> enable and power down pins are named 'OE' and 'PDWN' respectively. > >>>>>>> > >>>>>>> But don't we need the vendor prefix in the prop names then, like > >>>>>>> "renesas,oe-gpios" then? > >>>>>> > >>>>>> Seconded, with a correction to "thine,oe-gpios". > >>>>> > >>>>> mmm, okay then... > >>>>> > >>>>> A grep for that semi-standard properties names in Documentation/ > >>>>> returns only usage examples and no actual definitions, so I assume > >>>>> this is why they are semi-standard. > >>>> > >>>> Here we have to be specific about a particular property, let it be > >>>> 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: > >>>> > >>>> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l > >>>> 0 > >>>> > >>>> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l > >>>> 86 > >>>> > >>>> While 'thine,oe-gpios' would be correct, I see no reason to introduce > >>>> a vendor specific property to define a pin with a common and well > >>>> understood purpose. > >>>> > >>>> If you go forward with the vendor specific prefix, apparently you can > >>>> set the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does > >>>> the datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I > >>>> guess no. > >>> > >>> Let me clarify I don't want to push for a vendor specific name or > >>> similar, I'm fine with using 'semi-standard' names, I'm just confused > >>> by the 'semi-standard' definition. I guess from your examples, the > >>> usage count makes a difference here. > >> > >> yes, in gneneral you can read "semi-standard" as "widely used", thus > >> collecting statistics is a good enough method to make a reasoning. > >> > >> Hopefully the next evolutionary step of "widely used" is "described in > >> standard". > >> > >>>> Standards do not define '-gpios' suffix, but partially the description > >>>> is found in Documentation/bindings/gpio/gpio.txt, still it is not a > >>>> section in any standard as far as I know. > >>>> > >>>>> Seems like there is some tribal knowledge involved in defining what > >>>>> is semi-standard and what's not, or are those properties documented > >>>>> somewhere? > >>>> > >>>> The point is that there is no formal standard which describes every > >>>> IP, every IC and every single their property, some device node names > >>>> and property names are recommended in ePAPR and Devicetree > >>>> Specification though. > >>>> > >>>> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST > >>>> pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' > >>>> vs. 'powerdown-gpios'. > >>> > >>> I see all your points and I agree with most of them. Anyway, if the > >>> chip manual describes a pin as 'RST' I would not find it confusing to > >>> have a 'rst-gpio' defined in bindings :) > >>> > >>> Let me be a bit pesky here: what if a chip defines a reset GPIO, which > >>> is definitely a reset, but names it, say "XYZ" ? Would you prefer to > >>> see it defined as "reset-gpios" for consistency with other bindings, > >>> or "xyz-gpios" for consistency with documentation? > >> > >> If a pin is definitely an IC reset as you said, then my preference is to > >> see it described under 'reset-gpios' property name, plus a comment in > >> the IC device tree documentation document about it. I can provide two > >> reasons to advocate my position: > >> > >> 1) developers spend significantly more time reading and editing the > >> actual > >> > >> DTSI/DTS board files rather than reading and editing documentation, > >> it makes sense to use common property names to save time and reduce > >> amount of "what does 'oe' stand for?" type of questions; I suppose > >> that the recommendation to avoid not "widely used" abbreviations in > >> device node and property names arises from the same reasoning, > >> > >> 2) "widely used" and "standard" properties are excellent candidates for > >> > >> developing (or re-using) generalization wrappers, it happened so many > >> times in the past, and this process shall be supported in my opinion; > >> due to compatibility restrictions it might be problematic to change > >> property names, and every new exception to "widely used" properties > >> makes problematic to develop and maintain these kinds of wrappers, and > >> of course it postpones a desired "described in standard" recognition. > >> > >> If my point of view is accepted, I do admit that a developer who > >> translates a board schematics to board DTS file may experience a minor > >> discomfort, which is mitigated if relevant pin names are found in device > >> tree binding documentation in comments to properties, still the overall > >> gain is noticeably higher in my personal opinion. > > > > I have to disagree with this. When using a property name that doesn't > > correspond to the hardware documentation, developers will need to refer to > > the DT bindings documentation to confirm the property name. "Widely used" > > property names will not save time, they will use more time. This is of > > course marginal and I don't think it would have any noticeable impact, > > but I don't think your argument holds. > > We can have it both ways. The name should follow the documented > name/function. For example, we have enable-gpios which is simply the > invert of powerdown-gpios (for software's purposes). Pick the one > closest to the documentation. We're not trying to make bindings use > "enable" if a signal is called "powerdown". > > What we don't want is gratuitous variation in the names based on the > whims of hw designers: > > resetb-gpios > resetn-gpios > rst-gpios > rstn-gpios > nRESET-gpios > > ...you get the idea (and I left out vendor prefixes). Do we have a list of standardized names that should be used preferentially ? If not, should we create one ? > > I'm all for standardizing properties across DT bindings for multiple > > components, but doing so in a semi-random fashion will in my opinion not > > result in any gain. We can decide that power-down or output-enable GPIOS > > should have common property names (and I'm not even sure that would be > > useful, but we can certainly discuss it), but in that case someone should > > make a proposal and get the names standardized. Unless we do so, no > > matter what property name gets picked for a particular binding, it won't > > become universally used by magic. > > For "output enable", I suspect that is a common signal/function and > should have a standardized name. Generally, the way this works is we > get several variations and then we try to standardize things. I think > we can all agree standardizing first is better. If you want to put it > in a common place, please do. Maybe people will read that. Regardless, > the only way to enforce following standard names is with review. > > Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar > with h/w design should recognize OE. > > The reason to try and standardize names is so we can have common > drivers or library functions. In particular, for things like GPIOs > that need to be configured first for devices on otherwise discoverable > buses, this is very useful. I'm not sure we will ever implement that for the OE or power-down GPIOs, but I'm also not sure we will never do it, so I suppose it makes sense, just in case. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder 2018-04-05 18:53 ` Laurent Pinchart @ 2018-04-05 21:27 ` Rob Herring 0 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2018-04-05 21:27 UTC (permalink / raw) To: Laurent Pinchart Cc: Vladimir Zapolskiy, jacopo mondi, Sergei Shtylyov, Andrzej Hajda, Jacopo Mondi, Archit Taneja, David Airlie, Simon Horman, Magnus Damm, Geert Uytterhoeven, Niklas Söderlund, Mark Rutland, dri-devel, open list:MEDIA DRIVERS FOR RENESAS - FCP, devicetree, linux-kernel On Thu, Apr 5, 2018 at 1:53 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > On Thursday, 5 April 2018 19:33:33 EEST Rob Herring wrote: >> On Mon, Apr 2, 2018 at 8:36 AM, Laurent Pinchart wrote: >> > On Tuesday, 27 March 2018 14:03:25 EEST Vladimir Zapolskiy wrote: >> >> On 03/27/2018 01:10 PM, jacopo mondi wrote: >> >>> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote: >> >>>> On 03/27/2018 11:57 AM, jacopo mondi wrote: >> >>>>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote: >> >>>>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote: >> >>>>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote: >> >>>>>>> [...] >> >>>>>>> >> >>>>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree >> >>>>>>>>>>>>> bindings. >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> >>>>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >> >>>>>>>>>>>>> Reviewed-by: Niklas Söderlund >> >>>>>>>>>>>>> <niklas.soderlund+renesas@ragnatech.se> >> >>>>>>>>>>>>> --- >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++ >> >>>>>>>>>>>>> 1 file changed, 66 insertions(+) >> >>>>>>>>>>>>> create mode 100644 >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63l >> >>>>>>>>>>>>> vd1024.txt >> >>>>>>>>>>>>> diff --git >> >>>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc6 >> >>>>>>>>>>>>> 3lvd1024.txt >> >>>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc6 >> >>>>>>>>>>>>> 3lvd1024.txt >> >>>>>>>>>>>>> new file mode 100644 >> >>>>>>>>>>>>> index 0000000..8225c6a >> >>>>>>>>>>>>> --- /dev/null >> >>>>>>>>>>>>> +++ >> >>>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc6 >> >>>>>>>>>>>>> 3lvd1024.txt >> >>>>>>>>>>>>> @@ -0,0 +1,66 @@ >> >>>>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder >> >>>>>>>>>>>>> +------------------------------------------- >> >>>>>>>>>>>>> + >> >>>>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to >> >>>>>>>>>>>>> convert LVDS streams >> >>>>>>>>>>>>> +to parallel data outputs. The chip supports single/dual >> >>>>>>>>>>>>> input/output modes, >> >>>>>>>>>>>>> +handling up to two two input LVDS stream and up to two >> >>>>>>>>>>>>> digital CMOS/TTL outputs. >> >>>>>>>>>>>>> + >> >>>>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR >> >>>>>>>>>>>>> output modes are >> >>>>>>>>>>>>> +configured through input signals and the chip does not >> >>>>>>>>>>>>> expose any control bus. >> >>>>>>>>>>>>> + >> >>>>>>>>>>>>> +Required properties: >> >>>>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024" >> >>>>>>>>>>>>> + >> >>>>>>>>>>>>> +Optional properties: >> >>>>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital >> >>>>>>>>>>>>> circuitry >> >>>>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal >> >>>>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs >> >>>>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry >> >>>>>>>>>>>> >> >>>>>>>>>>>> As explained in a comment to one of the previous versions of >> >>>>>>>>>>>> this series, I'm tempted to make vcc-supply mandatory and drop >> >>>>>>>>>>>> the three other power supplies for now, as I believe there's >> >>>>>>>>>>>> very little chance they will be connected to separately >> >>>>>>>>>>>> controllable regulators (all supplies use the same voltage). In >> >>>>>>>>>>>> the very unlikely event that this occurs in design we need to >> >>>>>>>>>>>> support in the future, the cvcc, lvcc and pvcc supplies can be >> >>>>>>>>>>>> added later as optional without breaking backward >> >>>>>>>>>>>> compatibility. >> >>>>>>>>>>> >> >>>>>>>>>>> I'm okay with that. >> >>>>>>>>>>> >> >>>>>>>>>>>> Apart from that, >> >>>>>>>>>>>> >> >>>>>>>>>>>> Reviewed-by: Laurent Pinchart >> >>>>>>>>>>>> <laurent.pinchart@ideasonboard.com> >> >>>>>>>>>>>> >> >>>>>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low >> >>>>>>>>>>> >> >>>>>>>>>>> powerdown-gpios is the semi-standard name. >> >>>>>>>>>> >> >>>>>>>>>> right, I've also noticed it. If possible please avoid >> >>>>>>>>>> shortenings in property names. >> >>>>>>>>> >> >>>>>>>>> It is not shortening, it just follow pin name from decoder's >> >>>>>>>>> datasheet. >> >>>>>>>>> >> >>>>>>>>>>>>> +- oe-gpios: Output enable GPIO signal. Active high >> >>>>>>>>>>>>> + >> >>>>>>>>>> >> >>>>>>>>>> And this one is also a not ever met property name, please >> >>>>>>>>>> consider to rename it to 'enable-gpios', for instance display >> >>>>>>>>>> panels define it. >> >>>>>>>>> >> >>>>>>>>> Again, it follows datasheet naming scheme. Has something changed >> >>>>>>>>> in DT conventions? >> >>>>>>>> >> >>>>>>>> Seconded. My understanding is that the property name should >> >>>>>>>> reflect what reported in the the chip manual. For THC63LVD1024 the >> >>>>>>>> enable and power down pins are named 'OE' and 'PDWN' respectively. >> >>>>>>> >> >>>>>>> But don't we need the vendor prefix in the prop names then, like >> >>>>>>> "renesas,oe-gpios" then? >> >>>>>> >> >>>>>> Seconded, with a correction to "thine,oe-gpios". >> >>>>> >> >>>>> mmm, okay then... >> >>>>> >> >>>>> A grep for that semi-standard properties names in Documentation/ >> >>>>> returns only usage examples and no actual definitions, so I assume >> >>>>> this is why they are semi-standard. >> >>>> >> >>>> Here we have to be specific about a particular property, let it be >> >>>> 'oe-gpios' vs. 'enable-gpios' and let's collect some statistics: >> >>>> >> >>>> % grep -Hr oe-gpios Documentation/devicetree/bindings/* | wc -l >> >>>> 0 >> >>>> >> >>>> $ grep -Hr enable-gpios Documentation/devicetree/bindings/* | wc -l >> >>>> 86 >> >>>> >> >>>> While 'thine,oe-gpios' would be correct, I see no reason to introduce >> >>>> a vendor specific property to define a pin with a common and well >> >>>> understood purpose. >> >>>> >> >>>> If you go forward with the vendor specific prefix, apparently you can >> >>>> set the name to 'thine,oe-gpio' (single) or even to 'thine,oe', or does >> >>>> the datasheet names the pin as "OE GPIO" or "OE connected to a GPIO"? I >> >>>> guess no. >> >>> >> >>> Let me clarify I don't want to push for a vendor specific name or >> >>> similar, I'm fine with using 'semi-standard' names, I'm just confused >> >>> by the 'semi-standard' definition. I guess from your examples, the >> >>> usage count makes a difference here. >> >> >> >> yes, in gneneral you can read "semi-standard" as "widely used", thus >> >> collecting statistics is a good enough method to make a reasoning. >> >> >> >> Hopefully the next evolutionary step of "widely used" is "described in >> >> standard". >> >> >> >>>> Standards do not define '-gpios' suffix, but partially the description >> >>>> is found in Documentation/bindings/gpio/gpio.txt, still it is not a >> >>>> section in any standard as far as I know. >> >>>> >> >>>>> Seems like there is some tribal knowledge involved in defining what >> >>>>> is semi-standard and what's not, or are those properties documented >> >>>>> somewhere? >> >>>> >> >>>> The point is that there is no formal standard which describes every >> >>>> IP, every IC and every single their property, some device node names >> >>>> and property names are recommended in ePAPR and Devicetree >> >>>> Specification though. >> >>>> >> >>>> Think of a confusion if 'rst-gpios' (have you seen any ICs with an RST >> >>>> pin?) and 'reset-gpios' are different. Same applies to 'pdwn-gpios' >> >>>> vs. 'powerdown-gpios'. >> >>> >> >>> I see all your points and I agree with most of them. Anyway, if the >> >>> chip manual describes a pin as 'RST' I would not find it confusing to >> >>> have a 'rst-gpio' defined in bindings :) >> >>> >> >>> Let me be a bit pesky here: what if a chip defines a reset GPIO, which >> >>> is definitely a reset, but names it, say "XYZ" ? Would you prefer to >> >>> see it defined as "reset-gpios" for consistency with other bindings, >> >>> or "xyz-gpios" for consistency with documentation? >> >> >> >> If a pin is definitely an IC reset as you said, then my preference is to >> >> see it described under 'reset-gpios' property name, plus a comment in >> >> the IC device tree documentation document about it. I can provide two >> >> reasons to advocate my position: >> >> >> >> 1) developers spend significantly more time reading and editing the >> >> actual >> >> >> >> DTSI/DTS board files rather than reading and editing documentation, >> >> it makes sense to use common property names to save time and reduce >> >> amount of "what does 'oe' stand for?" type of questions; I suppose >> >> that the recommendation to avoid not "widely used" abbreviations in >> >> device node and property names arises from the same reasoning, >> >> >> >> 2) "widely used" and "standard" properties are excellent candidates for >> >> >> >> developing (or re-using) generalization wrappers, it happened so many >> >> times in the past, and this process shall be supported in my opinion; >> >> due to compatibility restrictions it might be problematic to change >> >> property names, and every new exception to "widely used" properties >> >> makes problematic to develop and maintain these kinds of wrappers, and >> >> of course it postpones a desired "described in standard" recognition. >> >> >> >> If my point of view is accepted, I do admit that a developer who >> >> translates a board schematics to board DTS file may experience a minor >> >> discomfort, which is mitigated if relevant pin names are found in device >> >> tree binding documentation in comments to properties, still the overall >> >> gain is noticeably higher in my personal opinion. >> > >> > I have to disagree with this. When using a property name that doesn't >> > correspond to the hardware documentation, developers will need to refer to >> > the DT bindings documentation to confirm the property name. "Widely used" >> > property names will not save time, they will use more time. This is of >> > course marginal and I don't think it would have any noticeable impact, >> > but I don't think your argument holds. >> >> We can have it both ways. The name should follow the documented >> name/function. For example, we have enable-gpios which is simply the >> invert of powerdown-gpios (for software's purposes). Pick the one >> closest to the documentation. We're not trying to make bindings use >> "enable" if a signal is called "powerdown". >> >> What we don't want is gratuitous variation in the names based on the >> whims of hw designers: >> >> resetb-gpios >> resetn-gpios >> rst-gpios >> rstn-gpios >> nRESET-gpios >> >> ...you get the idea (and I left out vendor prefixes). > > Do we have a list of standardized names that should be used preferentially ? No. I think the list is pretty much in this thread. I didn't find any others grepping the tree. There was an effort with USB devices to do "generic" power/reset control, but I think that never landed. It was using standard property names like reset-gpios within the device nodes rather than the approach used by mmc pwrseq binding (which I'm not a fan of). > If not, should we create one ? Sure. You can put it in the root. Then maybe people will find it. >> > I'm all for standardizing properties across DT bindings for multiple >> > components, but doing so in a semi-random fashion will in my opinion not >> > result in any gain. We can decide that power-down or output-enable GPIOS >> > should have common property names (and I'm not even sure that would be >> > useful, but we can certainly discuss it), but in that case someone should >> > make a proposal and get the names standardized. Unless we do so, no >> > matter what property name gets picked for a particular binding, it won't >> > become universally used by magic. >> >> For "output enable", I suspect that is a common signal/function and >> should have a standardized name. Generally, the way this works is we >> get several variations and then we try to standardize things. I think >> we can all agree standardizing first is better. If you want to put it >> in a common place, please do. Maybe people will read that. Regardless, >> the only way to enforce following standard names is with review. >> >> Debating "oe" vs. "output-enable" is bikeshedding IMO. Anyone familiar >> with h/w design should recognize OE. >> >> The reason to try and standardize names is so we can have common >> drivers or library functions. In particular, for things like GPIOs >> that need to be configured first for devices on otherwise discoverable >> buses, this is very useful. > > I'm not sure we will ever implement that for the OE or power-down GPIOs, but > I'm also not sure we will never do it, so I suppose it makes sense, just in > case. It's no different than "power-supply" in the simple panel binding which we support in the common driver. Rob ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-16 15:16 [PATCH v6 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi @ 2018-03-16 15:16 ` Jacopo Mondi 2018-03-20 13:00 ` Laurent Pinchart 2018-03-27 6:24 ` Vladimir Zapolskiy 2018-03-16 15:16 ` [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle Jacopo Mondi 2 siblings, 2 replies; 32+ messages in thread From: Jacopo Mondi @ 2018-03-16 15:16 UTC (permalink / raw) To: architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: Jacopo Mondi, dri-devel, linux-renesas-soc, devicetree, linux-kernel Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel output converter. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++ 3 files changed, 262 insertions(+) create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 3b99d5a..5815a20 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -92,6 +92,12 @@ config DRM_SII9234 It is an I2C driver, that detects connection of MHL bridge and starts encapsulation of HDMI signal. +config DRM_THINE_THC63LVD1024 + tristate "Thine THC63LVD1024 LVDS decoder bridge" + depends on OF + ---help--- + Thine THC63LVD1024 LVDS/parallel converter driver. + config DRM_TOSHIBA_TC358767 tristate "Toshiba TC358767 eDP bridge" depends on OF diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o obj-$(CONFIG_DRM_SII902X) += sii902x.o obj-$(CONFIG_DRM_SII9234) += sii9234.o +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644 index 0000000..02a54c2 --- /dev/null +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c @@ -0,0 +1,255 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * THC63LVD1024 LVDS to parallel data DRM bridge driver. + * + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> + */ + +#include <drm/drmP.h> +#include <drm/drm_bridge.h> +#include <drm/drm_panel.h> + +#include <linux/gpio/consumer.h> +#include <linux/of_graph.h> +#include <linux/regulator/consumer.h> + +enum { + THC63_LVDS_IN0, + THC63_LVDS_IN1, + THC63_DIGITAL_OUT0, + THC63_DIGITAL_OUT1, +}; + +static const char * const thc63_reg_names[] = { + "vcc", "lvcc", "pvcc", "cvcc", +}; + +struct thc63_dev { + struct device *dev; + + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; + + struct gpio_desc *pdwn; + struct gpio_desc *oe; + + struct drm_bridge bridge; + struct drm_bridge *next; +}; + +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) +{ + return container_of(bridge, struct thc63_dev, bridge); +} + +static int thc63_attach(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); +} + +static void thc63_enable(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + struct regulator *vcc; + int i; + + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { + vcc = thc63->vccs[i]; + if (!vcc) + continue; + + if (regulator_enable(vcc)) + goto error_vcc_enable; + } + + if (thc63->pdwn) + gpiod_set_value(thc63->pdwn, 0); + + if (thc63->oe) + gpiod_set_value(thc63->oe, 1); + + return; + +error_vcc_enable: + dev_err(thc63->dev, "Failed to enable regulator %s\n", + thc63_reg_names[i]); + + for (i = i - 1; i >= 0; i--) { + vcc = thc63->vccs[i]; + if (!vcc) + continue; + + regulator_disable(vcc); + } +} + +static void thc63_disable(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + struct regulator *vcc; + int i; + + if (thc63->oe) + gpiod_set_value(thc63->oe, 0); + + if (thc63->pdwn) + gpiod_set_value(thc63->pdwn, 1); + + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) { + vcc = thc63->vccs[i]; + if (!vcc) + continue; + + if (regulator_disable(vcc)) + dev_dbg(thc63->dev, + "Failed to disable regulator %s\n", + thc63_reg_names[i]); + } +} + +struct drm_bridge_funcs thc63_bridge_func = { + .attach = thc63_attach, + .enable = thc63_enable, + .disable = thc63_disable, +}; + +static int thc63_parse_dt(struct thc63_dev *thc63) +{ + struct device_node *thc63_out; + struct device_node *remote; + int ret = 0; + + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, + THC63_DIGITAL_OUT0, -1); + if (!thc63_out) { + dev_err(thc63->dev, "Missing endpoint in Port@%u\n", + THC63_DIGITAL_OUT0); + return -ENODEV; + } + + remote = of_graph_get_remote_port_parent(thc63_out); + if (!remote) { + dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n", + THC63_DIGITAL_OUT0); + ret = -ENODEV; + goto error_put_out_node; + } + + if (!of_device_is_available(remote)) { + dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n", + THC63_DIGITAL_OUT0); + ret = -ENODEV; + goto error_put_remote_node; + } + + thc63->next = of_drm_find_bridge(remote); + if (!thc63->next) + ret = -EPROBE_DEFER; + +error_put_remote_node: + of_node_put(remote); +error_put_out_node: + of_node_put(thc63_out); + + return ret; +} + +static int thc63_gpio_init(struct thc63_dev *thc63) +{ + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); + if (IS_ERR(thc63->oe)) { + dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n"); + return PTR_ERR(thc63->oe); + } + + thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn", + GPIOD_OUT_HIGH); + if (IS_ERR(thc63->pdwn)) { + dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n"); + return PTR_ERR(thc63->pdwn); + } + + return 0; +} + +static int thc63_regulator_init(struct thc63_dev *thc63) +{ + struct regulator **reg; + int i; + + reg = &thc63->vccs[0]; + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) { + *reg = devm_regulator_get_optional(thc63->dev, + thc63_reg_names[i]); + if (IS_ERR(*reg)) { + if (PTR_ERR(*reg) == -EPROBE_DEFER) + return -EPROBE_DEFER; + *reg = NULL; + } + } + + return 0; +} + +static int thc63_probe(struct platform_device *pdev) +{ + struct thc63_dev *thc63; + int ret; + + thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL); + if (!thc63) + return -ENOMEM; + + thc63->dev = &pdev->dev; + platform_set_drvdata(pdev, thc63); + + ret = thc63_regulator_init(thc63); + if (ret) + return ret; + + ret = thc63_gpio_init(thc63); + if (ret) + return ret; + + ret = thc63_parse_dt(thc63); + if (ret) + return ret; + + thc63->bridge.driver_private = thc63; + thc63->bridge.of_node = pdev->dev.of_node; + thc63->bridge.funcs = &thc63_bridge_func; + + drm_bridge_add(&thc63->bridge); + + return 0; +} + +static int thc63_remove(struct platform_device *pdev) +{ + struct thc63_dev *thc63 = platform_get_drvdata(pdev); + + drm_bridge_remove(&thc63->bridge); + + return 0; +} + +static const struct of_device_id thc63_match[] = { + { .compatible = "thine,thc63lvd1024", }, + { }, +}; +MODULE_DEVICE_TABLE(of, thc63_match); + +static struct platform_driver thc63_driver = { + .probe = thc63_probe, + .remove = thc63_remove, + .driver = { + .name = "thc63lvd1024", + .of_match_table = thc63_match, + }, +}; +module_platform_driver(thc63_driver); + +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-16 15:16 ` [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi @ 2018-03-20 13:00 ` Laurent Pinchart 2018-03-27 6:24 ` Vladimir Zapolskiy 1 sibling, 0 replies; 32+ messages in thread From: Laurent Pinchart @ 2018-03-20 13:00 UTC (permalink / raw) To: Jacopo Mondi Cc: architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, Thank you for the patch. On Friday, 16 March 2018 17:16:38 EET Jacopo Mondi wrote: > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > output converter. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/gpu/drm/bridge/Kconfig | 6 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/thc63lvd1024.c | 255 +++++++++++++++++++++++++++++++ > 3 files changed, 262 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c I'm aware that you started with a generic driver and then moved to a chip- specific driver and that it caused issues. I however believe it was a good design, but we can make the driver generic later when a second similar chip will need to be supported. > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 3b99d5a..5815a20 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -92,6 +92,12 @@ config DRM_SII9234 > It is an I2C driver, that detects connection of MHL bridge > and starts encapsulation of HDMI signal. > > +config DRM_THINE_THC63LVD1024 > + tristate "Thine THC63LVD1024 LVDS decoder bridge" > + depends on OF > + ---help--- > + Thine THC63LVD1024 LVDS/parallel converter driver. > + > config DRM_TOSHIBA_TC358767 > tristate "Toshiba TC358767 eDP bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile > b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > obj-$(CONFIG_DRM_SII902X) += sii902x.o > obj-$(CONFIG_DRM_SII9234) += sii9234.o > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c > b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644 > index 0000000..02a54c2 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > @@ -0,0 +1,255 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > + * > + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_panel.h> > + > +#include <linux/gpio/consumer.h> > +#include <linux/of_graph.h> > +#include <linux/regulator/consumer.h> > + > +enum { Please name the enumeration. > + THC63_LVDS_IN0, > + THC63_LVDS_IN1, > + THC63_DIGITAL_OUT0, > + THC63_DIGITAL_OUT1, Strictly speaking LVDS is digital too. Maybe THC63_RGB_OUT* ? > +}; > + > +static const char * const thc63_reg_names[] = { > + "vcc", "lvcc", "pvcc", "cvcc", > +}; > + > +struct thc63_dev { > + struct device *dev; > + > + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; > + > + struct gpio_desc *pdwn; > + struct gpio_desc *oe; > + > + struct drm_bridge bridge; > + struct drm_bridge *next; > +}; > + > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct thc63_dev, bridge); > +} > + > +static int thc63_attach(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + > + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); > +} > + > +static void thc63_enable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + if (regulator_enable(vcc)) You can operate on thc63->vccs[i] directly without a local variable, here and below in the functions that handle regulators. > + goto error_vcc_enable; > + } I thought about proposing usage of the regulators bulk API but it doesn't support optional regulators :-( If you agree with my proposal to make all regulators mandatory, please consider the bulk API to simplify power supply handling. If on the other hand you agree with my proposal to keep the vcc supply only, it won't matter. > + if (thc63->pdwn) > + gpiod_set_value(thc63->pdwn, 0); > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 1); > + > + return; > + > +error_vcc_enable: > + dev_err(thc63->dev, "Failed to enable regulator %s\n", > + thc63_reg_names[i]); I'd move the error message right before the goto. > + for (i = i - 1; i >= 0; i--) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + regulator_disable(vcc); > + } > +} > + > +static void thc63_disable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + int i; > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 0); > + > + if (thc63->pdwn) > + gpiod_set_value(thc63->pdwn, 1); > + > + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + if (regulator_disable(vcc)) > + dev_dbg(thc63->dev, > + "Failed to disable regulator %s\n", > + thc63_reg_names[i]); > + } > +} > + > +struct drm_bridge_funcs thc63_bridge_func = { static const > + .attach = thc63_attach, > + .enable = thc63_enable, > + .disable = thc63_disable, > +}; > + > +static int thc63_parse_dt(struct thc63_dev *thc63) > +{ > + struct device_node *thc63_out; > + struct device_node *remote; > + int ret = 0; > + > + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, > + THC63_DIGITAL_OUT0, -1); > + if (!thc63_out) { > + dev_err(thc63->dev, "Missing endpoint in Port@%u\n", > + THC63_DIGITAL_OUT0); > + return -ENODEV; > + } > + > + remote = of_graph_get_remote_port_parent(thc63_out); of_node_put(thc63_out); here, it will simplify error handling. > + if (!remote) { > + dev_err(thc63->dev, "Endpoint in Port@%u unconnected\n", > + THC63_DIGITAL_OUT0); > + ret = -ENODEV; > + goto error_put_out_node; > + } > + > + if (!of_device_is_available(remote)) { > + dev_err(thc63->dev, "Port@%u remote endpoint is disabled\n", > + THC63_DIGITAL_OUT0); > + ret = -ENODEV; > + goto error_put_remote_node; > + } > + > + thc63->next = of_drm_find_bridge(remote); > + if (!thc63->next) > + ret = -EPROBE_DEFER; You can add a return 0 here (and a goto in the previous check. Going through error labels in the non-error case is confusing. As a result you can avoid initializing ret to 0. > +error_put_remote_node: > + of_node_put(remote); > +error_put_out_node: > + of_node_put(thc63_out); > + > + return ret; > +} > + > +static int thc63_gpio_init(struct thc63_dev *thc63) > +{ > + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); > + if (IS_ERR(thc63->oe)) { > + dev_err(thc63->dev, "Unable to get \"oe-gpios\"\n"); You can add the error code to the message, it could be helpful. > + return PTR_ERR(thc63->oe); > + } > + > + thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "pdwn", > + GPIOD_OUT_HIGH); > + if (IS_ERR(thc63->pdwn)) { > + dev_err(thc63->dev, "Unable to get \"pdwn-gpios\"\n"); Same here. > + return PTR_ERR(thc63->pdwn); > + } > + > + return 0; > +} > + > +static int thc63_regulator_init(struct thc63_dev *thc63) > +{ > + struct regulator **reg; > + int i; i is never negative, you can make it an unsigned int. > + reg = &thc63->vccs[0]; > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) { > + *reg = devm_regulator_get_optional(thc63->dev, > + thc63_reg_names[i]); > + if (IS_ERR(*reg)) { > + if (PTR_ERR(*reg) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + *reg = NULL; > + } > + } Wouldn't the following be simpler ? for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { struct regulator *reg; reg = devm_regulator_get_optional(thc63->dev, thc63_reg_names[i]); if (IS_ERR(reg)) { if (PTR_ERR(reg) == -EPROBE_DEFER) return -EPROBE_DEFER; reg = NULL; } thc63->vccs[i] = reg; } > + return 0; > +} > + > +static int thc63_probe(struct platform_device *pdev) > +{ > + struct thc63_dev *thc63; > + int ret; > + > + thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL); > + if (!thc63) > + return -ENOMEM; > + > + thc63->dev = &pdev->dev; > + platform_set_drvdata(pdev, thc63); > + > + ret = thc63_regulator_init(thc63); > + if (ret) > + return ret; > + > + ret = thc63_gpio_init(thc63); > + if (ret) > + return ret; > + > + ret = thc63_parse_dt(thc63); > + if (ret) > + return ret; > + > + thc63->bridge.driver_private = thc63; > + thc63->bridge.of_node = pdev->dev.of_node; > + thc63->bridge.funcs = &thc63_bridge_func; > + > + drm_bridge_add(&thc63->bridge); > + > + return 0; > +} > + > +static int thc63_remove(struct platform_device *pdev) > +{ > + struct thc63_dev *thc63 = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&thc63->bridge); > + > + return 0; > +} > + > +static const struct of_device_id thc63_match[] = { > + { .compatible = "thine,thc63lvd1024", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, thc63_match); > + > +static struct platform_driver thc63_driver = { > + .probe = thc63_probe, > + .remove = thc63_remove, > + .driver = { > + .name = "thc63lvd1024", > + .of_match_table = thc63_match, > + }, > +}; > +module_platform_driver(thc63_driver); > + > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>"); > +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver"); > +MODULE_LICENSE("GPL v2"); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-16 15:16 ` [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi 2018-03-20 13:00 ` Laurent Pinchart @ 2018-03-27 6:24 ` Vladimir Zapolskiy 2018-03-27 7:28 ` Andrzej Hajda 2018-03-27 7:30 ` jacopo mondi 1 sibling, 2 replies; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 6:24 UTC (permalink / raw) To: Jacopo Mondi, architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, On 03/16/2018 05:16 PM, Jacopo Mondi wrote: > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > output converter. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/gpu/drm/bridge/Kconfig | 6 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++ > 3 files changed, 262 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 3b99d5a..5815a20 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -92,6 +92,12 @@ config DRM_SII9234 > It is an I2C driver, that detects connection of MHL bridge > and starts encapsulation of HDMI signal. > > +config DRM_THINE_THC63LVD1024 > + tristate "Thine THC63LVD1024 LVDS decoder bridge" > + depends on OF > + ---help--- > + Thine THC63LVD1024 LVDS/parallel converter driver. > + > config DRM_TOSHIBA_TC358767 > tristate "Toshiba TC358767 eDP bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 373eb28..fd90b16 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > obj-$(CONFIG_DRM_SII902X) += sii902x.o > obj-$(CONFIG_DRM_SII9234) += sii9234.o > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c > new file mode 100644 > index 0000000..02a54c2 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > @@ -0,0 +1,255 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > + * > + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_panel.h> > + > +#include <linux/gpio/consumer.h> > +#include <linux/of_graph.h> > +#include <linux/regulator/consumer.h> > + > +enum { > + THC63_LVDS_IN0, > + THC63_LVDS_IN1, > + THC63_DIGITAL_OUT0, > + THC63_DIGITAL_OUT1, > +}; > + > +static const char * const thc63_reg_names[] = { > + "vcc", "lvcc", "pvcc", "cvcc", > +}; > + > +struct thc63_dev { > + struct device *dev; > + > + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; > + > + struct gpio_desc *pdwn; > + struct gpio_desc *oe; > + > + struct drm_bridge bridge; > + struct drm_bridge *next; > +}; > + > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct thc63_dev, bridge); > +} > + > +static int thc63_attach(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + > + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); > +} > + > +static void thc63_enable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + int i; unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + if (regulator_enable(vcc)) > + goto error_vcc_enable; > + } > + > + if (thc63->pdwn) > + gpiod_set_value(thc63->pdwn, 0); > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 1); > + > + return; > + > +error_vcc_enable: > + dev_err(thc63->dev, "Failed to enable regulator %s\n", > + thc63_reg_names[i]); > + > + for (i = i - 1; i >= 0; i--) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + regulator_disable(vcc); > + } > +} > + > +static void thc63_disable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + int i; > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 0); > + > + if (thc63->pdwn) > + gpiod_set_value(thc63->pdwn, 1); > + > + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) { > + vcc = thc63->vccs[i]; > + if (!vcc) > + continue; > + > + if (regulator_disable(vcc)) > + dev_dbg(thc63->dev, > + "Failed to disable regulator %s\n", > + thc63_reg_names[i]); > + } > +} > + > +struct drm_bridge_funcs thc63_bridge_func = { Apparently you missed all my comments from v2 review. If you like to avoid non-NULL function pointers to the void, please use static const storage qualifier. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 6:24 ` Vladimir Zapolskiy @ 2018-03-27 7:28 ` Andrzej Hajda 2018-03-27 7:36 ` Geert Uytterhoeven 2018-03-27 9:57 ` Vladimir Zapolskiy 2018-03-27 7:30 ` jacopo mondi 1 sibling, 2 replies; 32+ messages in thread From: Andrzej Hajda @ 2018-03-27 7:28 UTC (permalink / raw) To: Vladimir Zapolskiy, Jacopo Mondi, architt, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: dri-devel, linux-renesas-soc, devicetree, linux-kernel On 27.03.2018 08:24, Vladimir Zapolskiy wrote: > Hi Jacopo, > > On 03/16/2018 05:16 PM, Jacopo Mondi wrote: >> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel >> output converter. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> --- >> drivers/gpu/drm/bridge/Kconfig | 6 + >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 262 insertions(+) >> create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c >> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index 3b99d5a..5815a20 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -92,6 +92,12 @@ config DRM_SII9234 >> It is an I2C driver, that detects connection of MHL bridge >> and starts encapsulation of HDMI signal. >> >> +config DRM_THINE_THC63LVD1024 >> + tristate "Thine THC63LVD1024 LVDS decoder bridge" >> + depends on OF >> + ---help--- >> + Thine THC63LVD1024 LVDS/parallel converter driver. >> + >> config DRM_TOSHIBA_TC358767 >> tristate "Toshiba TC358767 eDP bridge" >> depends on OF >> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >> index 373eb28..fd90b16 100644 >> --- a/drivers/gpu/drm/bridge/Makefile >> +++ b/drivers/gpu/drm/bridge/Makefile >> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o >> obj-$(CONFIG_DRM_SII902X) += sii902x.o >> obj-$(CONFIG_DRM_SII9234) += sii9234.o >> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o >> obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o >> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ >> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c >> new file mode 100644 >> index 0000000..02a54c2 >> --- /dev/null >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >> @@ -0,0 +1,255 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * THC63LVD1024 LVDS to parallel data DRM bridge driver. >> + * >> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> >> + */ >> + >> +#include <drm/drmP.h> >> +#include <drm/drm_bridge.h> >> +#include <drm/drm_panel.h> >> + >> +#include <linux/gpio/consumer.h> >> +#include <linux/of_graph.h> >> +#include <linux/regulator/consumer.h> >> + >> +enum { >> + THC63_LVDS_IN0, >> + THC63_LVDS_IN1, >> + THC63_DIGITAL_OUT0, >> + THC63_DIGITAL_OUT1, >> +}; >> + >> +static const char * const thc63_reg_names[] = { >> + "vcc", "lvcc", "pvcc", "cvcc", >> +}; >> + >> +struct thc63_dev { >> + struct device *dev; >> + >> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; >> + >> + struct gpio_desc *pdwn; >> + struct gpio_desc *oe; >> + >> + struct drm_bridge bridge; >> + struct drm_bridge *next; >> +}; >> + >> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) >> +{ >> + return container_of(bridge, struct thc63_dev, bridge); >> +} >> + >> +static int thc63_attach(struct drm_bridge *bridge) >> +{ >> + struct thc63_dev *thc63 = to_thc63(bridge); >> + >> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); >> +} >> + >> +static void thc63_enable(struct drm_bridge *bridge) >> +{ >> + struct thc63_dev *thc63 = to_thc63(bridge); >> + struct regulator *vcc; >> + int i; > unsigned int i; Why? You are introducing silly bug this way, see few lines below. > >> + >> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >> + vcc = thc63->vccs[i]; >> + if (!vcc) >> + continue; >> + >> + if (regulator_enable(vcc)) >> + goto error_vcc_enable; >> + } >> + >> + if (thc63->pdwn) >> + gpiod_set_value(thc63->pdwn, 0); >> + >> + if (thc63->oe) >> + gpiod_set_value(thc63->oe, 1); >> + >> + return; >> + >> +error_vcc_enable: >> + dev_err(thc63->dev, "Failed to enable regulator %s\n", >> + thc63_reg_names[i]); >> + >> + for (i = i - 1; i >= 0; i--) { Here, the loop will not end if you define i unsigned. I know one can change the loop, to make it working with unsigned. But this clearly shows that using unsigned is more risky. What are advantages of unsigned vs int in this case. Are there some guidelines/discussions about it? Regards Andrzej >> + vcc = thc63->vccs[i]; >> + if (!vcc) >> + continue; >> + >> + regulator_disable(vcc); >> + } >> +} >> + >> +static void thc63_disable(struct drm_bridge *bridge) >> +{ >> + struct thc63_dev *thc63 = to_thc63(bridge); >> + struct regulator *vcc; >> + int i; >> + >> + if (thc63->oe) >> + gpiod_set_value(thc63->oe, 0); >> + >> + if (thc63->pdwn) >> + gpiod_set_value(thc63->pdwn, 1); >> + >> + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) { >> + vcc = thc63->vccs[i]; >> + if (!vcc) >> + continue; >> + >> + if (regulator_disable(vcc)) >> + dev_dbg(thc63->dev, >> + "Failed to disable regulator %s\n", >> + thc63_reg_names[i]); >> + } >> +} >> + >> +struct drm_bridge_funcs thc63_bridge_func = { > Apparently you missed all my comments from v2 review. > > If you like to avoid non-NULL function pointers to the void, please > use static const storage qualifier. > > -- > With best wishes, > Vladimir > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 7:28 ` Andrzej Hajda @ 2018-03-27 7:36 ` Geert Uytterhoeven 2018-03-27 8:36 ` Andrzej Hajda 2018-03-27 9:57 ` Vladimir Zapolskiy 1 sibling, 1 reply; 32+ messages in thread From: Geert Uytterhoeven @ 2018-03-27 7:36 UTC (permalink / raw) To: Andrzej Hajda Cc: Vladimir Zapolskiy, Jacopo Mondi, Archit Taneja, Laurent Pinchart, David Airlie, Simon Horman, Magnus Damm, Niklas Söderlund, Sergei Shtylyov, Rob Herring, Mark Rutland, DRI Development, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List Hi Andrzej, On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> +static void thc63_enable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + int i; >> unsigned int i; > > Why? You are introducing silly bug this way, see few lines below. > >> >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (!vcc) >>> + continue; >>> + >>> + if (regulator_enable(vcc)) >>> + goto error_vcc_enable; >>> + } >>> + >>> + if (thc63->pdwn) >>> + gpiod_set_value(thc63->pdwn, 0); >>> + >>> + if (thc63->oe) >>> + gpiod_set_value(thc63->oe, 1); >>> + >>> + return; >>> + >>> +error_vcc_enable: >>> + dev_err(thc63->dev, "Failed to enable regulator %s\n", >>> + thc63_reg_names[i]); >>> + >>> + for (i = i - 1; i >= 0; i--) { > > Here, the loop will not end if you define i unsigned. True. > I know one can change the loop, to make it working with unsigned. But > this clearly shows that using unsigned is more risky. > What are advantages of unsigned vs int in this case. Are there some > guidelines/discussions about it? Some people consider signed integers harmful, as they may be converted silently by the compiler to the "larger" unsigned type when needed. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 7:36 ` Geert Uytterhoeven @ 2018-03-27 8:36 ` Andrzej Hajda 2018-03-27 9:06 ` Geert Uytterhoeven 0 siblings, 1 reply; 32+ messages in thread From: Andrzej Hajda @ 2018-03-27 8:36 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Vladimir Zapolskiy, Jacopo Mondi, Archit Taneja, Laurent Pinchart, David Airlie, Simon Horman, Magnus Damm, Niklas Söderlund, Sergei Shtylyov, Rob Herring, Mark Rutland, DRI Development, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List On 27.03.2018 09:36, Geert Uytterhoeven wrote: > Hi Andrzej, > > On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>>> +static void thc63_enable(struct drm_bridge *bridge) >>>> +{ >>>> + struct thc63_dev *thc63 = to_thc63(bridge); >>>> + struct regulator *vcc; >>>> + int i; >>> unsigned int i; >> Why? You are introducing silly bug this way, see few lines below. >> >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>>> + vcc = thc63->vccs[i]; >>>> + if (!vcc) >>>> + continue; >>>> + >>>> + if (regulator_enable(vcc)) >>>> + goto error_vcc_enable; >>>> + } >>>> + >>>> + if (thc63->pdwn) >>>> + gpiod_set_value(thc63->pdwn, 0); >>>> + >>>> + if (thc63->oe) >>>> + gpiod_set_value(thc63->oe, 1); >>>> + >>>> + return; >>>> + >>>> +error_vcc_enable: >>>> + dev_err(thc63->dev, "Failed to enable regulator %s\n", >>>> + thc63_reg_names[i]); >>>> + >>>> + for (i = i - 1; i >= 0; i--) { >> Here, the loop will not end if you define i unsigned. > True. > >> I know one can change the loop, to make it working with unsigned. But >> this clearly shows that using unsigned is more risky. >> What are advantages of unsigned vs int in this case. Are there some >> guidelines/discussions about it? > Some people consider signed integers harmful, as they may be converted > silently by the compiler to the "larger" unsigned type when needed. Wow, it sounds crazy, shall we expect gigantic patchsets, converting all occurrences of int to "unsigned int" ? :) I know both types have their pros and cons and can behave unexpectedly in corner cases, but I do not see why unsigned should be preferred over signed in general, or in this particular case. I guess there were somewhere discussion about it, could you point me to it if possible, to avoid unnecessary noise in this thread. Regards Andrzej > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 8:36 ` Andrzej Hajda @ 2018-03-27 9:06 ` Geert Uytterhoeven 0 siblings, 0 replies; 32+ messages in thread From: Geert Uytterhoeven @ 2018-03-27 9:06 UTC (permalink / raw) To: Andrzej Hajda Cc: Vladimir Zapolskiy, Jacopo Mondi, Archit Taneja, Laurent Pinchart, David Airlie, Simon Horman, Magnus Damm, Niklas Söderlund, Sergei Shtylyov, Rob Herring, Mark Rutland, DRI Development, Linux-Renesas, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux Kernel Mailing List Hi Andrezj, On Tue, Mar 27, 2018 at 10:36 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: > On 27.03.2018 09:36, Geert Uytterhoeven wrote: >> On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote: >>>>> --- /dev/null >>>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>>>> +static void thc63_enable(struct drm_bridge *bridge) >>>>> +{ >>>>> + struct thc63_dev *thc63 = to_thc63(bridge); >>>>> + struct regulator *vcc; >>>>> + int i; >>>> unsigned int i; >>> Why? You are introducing silly bug this way, see few lines below. >>> >>>>> + >>>>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>>>> + vcc = thc63->vccs[i]; >>>>> + if (!vcc) >>>>> + continue; >>>>> + >>>>> + if (regulator_enable(vcc)) >>>>> + goto error_vcc_enable; >>>>> + } >>>>> + >>>>> + if (thc63->pdwn) >>>>> + gpiod_set_value(thc63->pdwn, 0); >>>>> + >>>>> + if (thc63->oe) >>>>> + gpiod_set_value(thc63->oe, 1); >>>>> + >>>>> + return; >>>>> + >>>>> +error_vcc_enable: >>>>> + dev_err(thc63->dev, "Failed to enable regulator %s\n", >>>>> + thc63_reg_names[i]); >>>>> + >>>>> + for (i = i - 1; i >= 0; i--) { >>> Here, the loop will not end if you define i unsigned. >> True. >> >>> I know one can change the loop, to make it working with unsigned. But >>> this clearly shows that using unsigned is more risky. >>> What are advantages of unsigned vs int in this case. Are there some >>> guidelines/discussions about it? >> Some people consider signed integers harmful, as they may be converted >> silently by the compiler to the "larger" unsigned type when needed. > > Wow, it sounds crazy, shall we expect gigantic patchsets, converting all > occurrences of int to "unsigned int" ? :) No we shall not. > I know both types have their pros and cons and can behave unexpectedly > in corner cases, but I do not see why unsigned should be preferred over > signed in general, or in this particular case. When looping over array indices, and comparing with ARRAY_SIZE (which is unsigned), using "unsigned int" is preferred. However, in this case the error code relies on the index becoming negative, so a signed integer should be used. > I guess there were somewhere discussion about it, could you point me to > it if possible, to avoid unnecessary noise in this thread. Not here, but Google pointed me to http://blog.robertelder.org/signed-or-unsigned/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 7:28 ` Andrzej Hajda 2018-03-27 7:36 ` Geert Uytterhoeven @ 2018-03-27 9:57 ` Vladimir Zapolskiy 1 sibling, 0 replies; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 9:57 UTC (permalink / raw) To: Andrzej Hajda, Jacopo Mondi, architt, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Andrzej, On 03/27/2018 10:28 AM, Andrzej Hajda wrote: > On 27.03.2018 08:24, Vladimir Zapolskiy wrote: >> Hi Jacopo, >> >> On 03/16/2018 05:16 PM, Jacopo Mondi wrote: >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel >>> output converter. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> --- >>> drivers/gpu/drm/bridge/Kconfig | 6 + >>> drivers/gpu/drm/bridge/Makefile | 1 + >>> drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 262 insertions(+) >>> create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c >>> >>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >>> index 3b99d5a..5815a20 100644 >>> --- a/drivers/gpu/drm/bridge/Kconfig >>> +++ b/drivers/gpu/drm/bridge/Kconfig >>> @@ -92,6 +92,12 @@ config DRM_SII9234 >>> It is an I2C driver, that detects connection of MHL bridge >>> and starts encapsulation of HDMI signal. >>> >>> +config DRM_THINE_THC63LVD1024 >>> + tristate "Thine THC63LVD1024 LVDS decoder bridge" >>> + depends on OF >>> + ---help--- >>> + Thine THC63LVD1024 LVDS/parallel converter driver. >>> + >>> config DRM_TOSHIBA_TC358767 >>> tristate "Toshiba TC358767 eDP bridge" >>> depends on OF >>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >>> index 373eb28..fd90b16 100644 >>> --- a/drivers/gpu/drm/bridge/Makefile >>> +++ b/drivers/gpu/drm/bridge/Makefile >>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >>> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o >>> obj-$(CONFIG_DRM_SII902X) += sii902x.o >>> obj-$(CONFIG_DRM_SII9234) += sii9234.o >>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o >>> obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o >>> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ >>> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ >>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> new file mode 100644 >>> index 0000000..02a54c2 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> @@ -0,0 +1,255 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver. >>> + * >>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> >>> + */ >>> + >>> +#include <drm/drmP.h> >>> +#include <drm/drm_bridge.h> >>> +#include <drm/drm_panel.h> >>> + >>> +#include <linux/gpio/consumer.h> >>> +#include <linux/of_graph.h> >>> +#include <linux/regulator/consumer.h> >>> + >>> +enum { >>> + THC63_LVDS_IN0, >>> + THC63_LVDS_IN1, >>> + THC63_DIGITAL_OUT0, >>> + THC63_DIGITAL_OUT1, >>> +}; >>> + >>> +static const char * const thc63_reg_names[] = { >>> + "vcc", "lvcc", "pvcc", "cvcc", >>> +}; >>> + >>> +struct thc63_dev { >>> + struct device *dev; >>> + >>> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; >>> + >>> + struct gpio_desc *pdwn; >>> + struct gpio_desc *oe; >>> + >>> + struct drm_bridge bridge; >>> + struct drm_bridge *next; >>> +}; >>> + >>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) >>> +{ >>> + return container_of(bridge, struct thc63_dev, bridge); >>> +} >>> + >>> +static int thc63_attach(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + >>> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); >>> +} >>> + >>> +static void thc63_enable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + int i; >> unsigned int i; > > Why? You are introducing silly bug this way, see few lines below. You see that the comment was too terse to describe the context in full. Thank you for exposing a flaw. >> >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (!vcc) >>> + continue; >>> + >>> + if (regulator_enable(vcc)) >>> + goto error_vcc_enable; >>> + } >>> + >>> + if (thc63->pdwn) >>> + gpiod_set_value(thc63->pdwn, 0); >>> + >>> + if (thc63->oe) >>> + gpiod_set_value(thc63->oe, 1); >>> + >>> + return; >>> + >>> +error_vcc_enable: >>> + dev_err(thc63->dev, "Failed to enable regulator %s\n", >>> + thc63_reg_names[i]); >>> + >>> + for (i = i - 1; i >= 0; i--) { > > Here, the loop will not end if you define i unsigned. > > I know one can change the loop, to make it working with unsigned. But > this clearly shows that using unsigned is more risky. > What are advantages of unsigned vs int in this case. Are there some > guidelines/discussions about it? > The reason is pretty simple, like Geert said it might be a personal reason, but a code pattern int i; ... for (i = 0; i < ARRAY_SIZE(...); i++) is my own red rag, because I've seen the pattern so many times, and every time here a compiler produces a W=12 (or -Wsign-compare) warning like the next one: drivers/gpu/drm/bridge/thc63lvd1024.c:182:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) { ^ Usually a conversion from 'int i' to 'unsigned int i' is trivial, and lowering of a noise level in compiler's output is seen as beneficial, because it gives more chances to people to capture a real problem. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver 2018-03-27 6:24 ` Vladimir Zapolskiy 2018-03-27 7:28 ` Andrzej Hajda @ 2018-03-27 7:30 ` jacopo mondi 1 sibling, 0 replies; 32+ messages in thread From: jacopo mondi @ 2018-03-27 7:30 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Jacopo Mondi, architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel HI Vladimir, On Tue, Mar 27, 2018 at 09:24:28AM +0300, Vladimir Zapolskiy wrote: > Hi Jacopo, > > On 03/16/2018 05:16 PM, Jacopo Mondi wrote: > > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > > output converter. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/gpu/drm/bridge/Kconfig | 6 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/thc63lvd1024.c | 255 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 262 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index 3b99d5a..5815a20 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -92,6 +92,12 @@ config DRM_SII9234 > > It is an I2C driver, that detects connection of MHL bridge > > and starts encapsulation of HDMI signal. > > > > +config DRM_THINE_THC63LVD1024 > > + tristate "Thine THC63LVD1024 LVDS decoder bridge" > > + depends on OF > > + ---help--- > > + Thine THC63LVD1024 LVDS/parallel converter driver. > > + > > config DRM_TOSHIBA_TC358767 > > tristate "Toshiba TC358767 eDP bridge" > > depends on OF > > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > > index 373eb28..fd90b16 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > > obj-$(CONFIG_DRM_SII902X) += sii902x.o > > obj-$(CONFIG_DRM_SII9234) += sii9234.o > > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c > > new file mode 100644 > > index 0000000..02a54c2 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > > @@ -0,0 +1,255 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > > + * > > + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org> > > + */ > > + > > +#include <drm/drmP.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_panel.h> > > + > > +#include <linux/gpio/consumer.h> > > +#include <linux/of_graph.h> > > +#include <linux/regulator/consumer.h> > > + > > +enum { > > + THC63_LVDS_IN0, > > + THC63_LVDS_IN1, > > + THC63_DIGITAL_OUT0, > > + THC63_DIGITAL_OUT1, > > +}; > > + > > +static const char * const thc63_reg_names[] = { > > + "vcc", "lvcc", "pvcc", "cvcc", > > +}; > > + > > +struct thc63_dev { > > + struct device *dev; > > + > > + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; > > + > > + struct gpio_desc *pdwn; > > + struct gpio_desc *oe; > > + > > + struct drm_bridge bridge; > > + struct drm_bridge *next; > > +}; > > + > > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct thc63_dev, bridge); > > +} > > + > > +static int thc63_attach(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + > > + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); > > +} > > + > > +static void thc63_enable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + int i; > > unsigned int i; > > > + > > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > > + vcc = thc63->vccs[i]; > > + if (!vcc) > > + continue; > > + > > + if (regulator_enable(vcc)) > > + goto error_vcc_enable; > > + } > > + > > + if (thc63->pdwn) > > + gpiod_set_value(thc63->pdwn, 0); > > + > > + if (thc63->oe) > > + gpiod_set_value(thc63->oe, 1); > > + > > + return; > > + > > +error_vcc_enable: > > + dev_err(thc63->dev, "Failed to enable regulator %s\n", > > + thc63_reg_names[i]); > > + > > + for (i = i - 1; i >= 0; i--) { > > + vcc = thc63->vccs[i]; > > + if (!vcc) > > + continue; > > + > > + regulator_disable(vcc); > > + } > > +} > > + > > +static void thc63_disable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + int i; > > + > > + if (thc63->oe) > > + gpiod_set_value(thc63->oe, 0); > > + > > + if (thc63->pdwn) > > + gpiod_set_value(thc63->pdwn, 1); > > + > > + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) { > > + vcc = thc63->vccs[i]; > > + if (!vcc) > > + continue; > > + > > + if (regulator_disable(vcc)) > > + dev_dbg(thc63->dev, > > + "Failed to disable regulator %s\n", > > + thc63_reg_names[i]); > > + } > > +} > > + > > +struct drm_bridge_funcs thc63_bridge_func = { > > Apparently you missed all my comments from v2 review. > I did not :) v6 was already out when you commented on v2.. > If you like to avoid non-NULL function pointers to the void, please > use static const storage qualifier. > > -- > With best wishes, > Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle 2018-03-16 15:16 [PATCH v6 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi @ 2018-03-16 15:16 ` Jacopo Mondi 2018-03-20 13:01 ` Laurent Pinchart 2 siblings, 1 reply; 32+ messages in thread From: Jacopo Mondi @ 2018-03-16 15:16 UTC (permalink / raw) To: architt, a.hajda, Laurent.pinchart, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland Cc: Jacopo Mondi, dri-devel, linux-renesas-soc, devicetree, linux-kernel The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS decoder, connected to the on-chip LVDS encoder output on one side and to HDMI encoder ADV7511w on the other one. As the decoder does not need any configuration it has been so-far omitted from DTS. Now that a driver is available, describe it in DT as well. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> --- List of patch dependencies, as of renesas-drivers-2018-03-13-v4.16-rc5: - [PATCH v2 0/5] arm64: dts: renesas: r8a77970: enable HDMI output which includes DU, LVDS and FCPD enablement from: [PATCH v2 0/5] Add R8A77970/V3MSK LVDS/HDMI support - [PATCH v4] v4l: vsp1: Fix video output on R8A77970 Patches to be applied on top of "arm64: dts: renesas: eagle: add HDMI output using the ADV7511W" Thanks j --- arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 +++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index c0fd144..69f43b8 100644 --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts @@ -42,6 +42,33 @@ }; }; }; + + thc63lvd1024: lvds-decoder { + compatible = "thine,thc63lvd1024"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + + thc63lvd1024_in_0: endpoint { + remote-endpoint = <&lvds0_out>; + }; + }; + + port@2{ + reg = <2>; + + thc63lvd1024_out_2: endpoint { + remote-endpoint = <&adv7511_in>; + }; + + }; + + }; + }; }; &avb { @@ -98,7 +125,7 @@ port@0 { reg = <0>; adv7511_in: endpoint { - remote-endpoint = <&lvds0_out>; + remote-endpoint = <&thc63lvd1024_out_2>; }; }; @@ -152,8 +179,8 @@ ports { port@1 { - endpoint { - remote-endpoint = <&adv7511_in>; + lvds0_out: endpoint { + remote-endpoint = <&thc63lvd1024_in_0>; }; }; }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle 2018-03-16 15:16 ` [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle Jacopo Mondi @ 2018-03-20 13:01 ` Laurent Pinchart 2018-03-27 6:27 ` Vladimir Zapolskiy 0 siblings, 1 reply; 32+ messages in thread From: Laurent Pinchart @ 2018-03-20 13:01 UTC (permalink / raw) To: Jacopo Mondi Cc: architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, Thank you for the patch. On Friday, 16 March 2018 17:16:39 EET Jacopo Mondi wrote: > The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS > decoder, connected to the on-chip LVDS encoder output on one side > and to HDMI encoder ADV7511w on the other one. > > As the decoder does not need any configuration it has been so-far > omitted from DTS. Now that a driver is available, describe it in DT > as well. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> The patch looks OK to me, but I think it should be squashed with Niklas' patch that added display HDMI output support to the V3M Eagle DT. > --- > > List of patch dependencies, as of renesas-drivers-2018-03-13-v4.16-rc5: > > - [PATCH v2 0/5] arm64: dts: renesas: r8a77970: enable HDMI output > which includes DU, LVDS and FCPD enablement from: > [PATCH v2 0/5] Add R8A77970/V3MSK LVDS/HDMI support > - [PATCH v4] v4l: vsp1: Fix video output on R8A77970 > > Patches to be applied on top of > "arm64: dts: renesas: eagle: add HDMI output using the ADV7511W" > > Thanks > j > --- > arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 ++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts > b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index c0fd144..69f43b8 > 100644 > --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts > @@ -42,6 +42,33 @@ > }; > }; > }; > + > + thc63lvd1024: lvds-decoder { > + compatible = "thine,thc63lvd1024"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + > + thc63lvd1024_in_0: endpoint { > + remote-endpoint = <&lvds0_out>; > + }; > + }; > + > + port@2{ > + reg = <2>; > + > + thc63lvd1024_out_2: endpoint { > + remote-endpoint = <&adv7511_in>; > + }; > + > + }; > + > + }; > + }; > }; > > &avb { > @@ -98,7 +125,7 @@ > port@0 { > reg = <0>; > adv7511_in: endpoint { > - remote-endpoint = <&lvds0_out>; > + remote-endpoint = <&thc63lvd1024_out_2>; > }; > }; > > @@ -152,8 +179,8 @@ > > ports { > port@1 { > - endpoint { > - remote-endpoint = <&adv7511_in>; > + lvds0_out: endpoint { > + remote-endpoint = <&thc63lvd1024_in_0>; > }; > }; > }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle 2018-03-20 13:01 ` Laurent Pinchart @ 2018-03-27 6:27 ` Vladimir Zapolskiy 0 siblings, 0 replies; 32+ messages in thread From: Vladimir Zapolskiy @ 2018-03-27 6:27 UTC (permalink / raw) To: Laurent Pinchart, Jacopo Mondi Cc: architt, a.hajda, airlied, horms, magnus.damm, geert, niklas.soderlund, sergei.shtylyov, robh+dt, mark.rutland, dri-devel, linux-renesas-soc, devicetree, linux-kernel Hi Jacopo, On 03/20/2018 03:01 PM, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Friday, 16 March 2018 17:16:39 EET Jacopo Mondi wrote: >> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS >> decoder, connected to the on-chip LVDS encoder output on one side >> and to HDMI encoder ADV7511w on the other one. >> >> As the decoder does not need any configuration it has been so-far >> omitted from DTS. Now that a driver is available, describe it in DT >> as well. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> > > The patch looks OK to me, but I think it should be squashed with Niklas' patch > that added display HDMI output support to the V3M Eagle DT. > >> --- >> >> List of patch dependencies, as of renesas-drivers-2018-03-13-v4.16-rc5: >> >> - [PATCH v2 0/5] arm64: dts: renesas: r8a77970: enable HDMI output >> which includes DU, LVDS and FCPD enablement from: >> [PATCH v2 0/5] Add R8A77970/V3MSK LVDS/HDMI support >> - [PATCH v4] v4l: vsp1: Fix video output on R8A77970 >> >> Patches to be applied on top of >> "arm64: dts: renesas: eagle: add HDMI output using the ADV7511W" >> >> Thanks >> j >> --- >> arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 ++++++++++++++++++++--- >> 1 file changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts >> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index c0fd144..69f43b8 >> 100644 >> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts >> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts >> @@ -42,6 +42,33 @@ >> }; >> }; >> }; >> + >> + thc63lvd1024: lvds-decoder { >> + compatible = "thine,thc63lvd1024"; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + reg = <0>; >> + >> + thc63lvd1024_in_0: endpoint { >> + remote-endpoint = <&lvds0_out>; >> + }; >> + }; >> + >> + port@2{ >> + reg = <2>; >> + >> + thc63lvd1024_out_2: endpoint { >> + remote-endpoint = <&adv7511_in>; >> + }; >> + Remove the empty line above. >> + }; >> + Remove the empty line above. >> + }; >> + }; >> }; >> -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-04-05 21:27 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-16 15:16 [PATCH v6 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge Jacopo Mondi 2018-03-16 15:16 ` [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder Jacopo Mondi 2018-03-20 12:43 ` Laurent Pinchart 2018-03-26 12:08 ` jacopo mondi 2018-03-26 22:22 ` Rob Herring 2018-03-27 6:15 ` Vladimir Zapolskiy 2018-03-27 7:12 ` Andrzej Hajda 2018-03-27 7:33 ` jacopo mondi 2018-03-27 8:27 ` Sergei Shtylyov 2018-03-27 8:30 ` Vladimir Zapolskiy 2018-03-27 8:57 ` jacopo mondi 2018-03-27 9:37 ` Vladimir Zapolskiy 2018-03-27 10:10 ` jacopo mondi 2018-03-27 11:03 ` Vladimir Zapolskiy 2018-03-29 10:02 ` jacopo mondi 2018-04-02 13:36 ` Laurent Pinchart 2018-04-03 12:33 ` jacopo mondi 2018-04-05 16:33 ` Rob Herring 2018-04-05 18:53 ` Laurent Pinchart 2018-04-05 21:27 ` Rob Herring 2018-03-16 15:16 ` [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver Jacopo Mondi 2018-03-20 13:00 ` Laurent Pinchart 2018-03-27 6:24 ` Vladimir Zapolskiy 2018-03-27 7:28 ` Andrzej Hajda 2018-03-27 7:36 ` Geert Uytterhoeven 2018-03-27 8:36 ` Andrzej Hajda 2018-03-27 9:06 ` Geert Uytterhoeven 2018-03-27 9:57 ` Vladimir Zapolskiy 2018-03-27 7:30 ` jacopo mondi 2018-03-16 15:16 ` [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle Jacopo Mondi 2018-03-20 13:01 ` Laurent Pinchart 2018-03-27 6:27 ` Vladimir Zapolskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).