| Message ID | 1527606359-19261-9-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 29, 2018 at 05:05:59PM +0200, Jacopo Mondi wrote: > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN > driver and only confuse users. Remove them in all Gen2 SoC that use > them. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > v3: > - remove bus-width from dt-bindings example > --- > Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - > arch/arm/boot/dts/r8a7790-lager.dts | 3 --- > arch/arm/boot/dts/r8a7791-koelsch.dts | 3 --- > arch/arm/boot/dts/r8a7791-porter.dts | 1 - > arch/arm/boot/dts/r8a7793-gose.dts | 3 --- > arch/arm/boot/dts/r8a7794-alt.dts | 1 - > arch/arm/boot/dts/r8a7794-silk.dts | 1 - > 7 files changed, 13 deletions(-) Reviewed-by: Rob Herring <robh@kernel.org>
Hi Jacopo, Thanks for your work. On 2018-05-29 17:05:59 +0200, Jacopo Mondi wrote: > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN > driver and only confuse users. Remove them in all Gen2 SoC that use > them. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> The more I think about this the more I lean towards that this patch should be dropped. The properties accurately describes the hardware and I think there is value in that. That the driver currently don't parse or make use of them don't in my view reduce there value. Maybe you should break out this patch to a separate series? > --- > v3: > - remove bus-width from dt-bindings example > --- > Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - > arch/arm/boot/dts/r8a7790-lager.dts | 3 --- > arch/arm/boot/dts/r8a7791-koelsch.dts | 3 --- > arch/arm/boot/dts/r8a7791-porter.dts | 1 - > arch/arm/boot/dts/r8a7793-gose.dts | 3 --- > arch/arm/boot/dts/r8a7794-alt.dts | 1 - > arch/arm/boot/dts/r8a7794-silk.dts | 1 - > 7 files changed, 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt > index 024c109..c6d7f60 100644 > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt > @@ -128,7 +128,6 @@ Board setup example for Gen2 platforms (vin1 composite video input) > > vin1ep0: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts > index 092610e..9cdabfcf 100644 > --- a/arch/arm/boot/dts/r8a7790-lager.dts > +++ b/arch/arm/boot/dts/r8a7790-lager.dts > @@ -885,10 +885,8 @@ > port { > vin0ep2: endpoint { > remote-endpoint = <&adv7612_out>; > - bus-width = <24>; > hsync-active = <0>; > vsync-active = <0>; > - pclk-sample = <1>; > data-active = <1>; > }; > }; > @@ -904,7 +902,6 @@ > port { > vin1ep0: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts > index 8ab793d..033c9e3 100644 > --- a/arch/arm/boot/dts/r8a7791-koelsch.dts > +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts > @@ -857,10 +857,8 @@ > port { > vin0ep2: endpoint { > remote-endpoint = <&adv7612_out>; > - bus-width = <24>; > hsync-active = <0>; > vsync-active = <0>; > - pclk-sample = <1>; > data-active = <1>; > }; > }; > @@ -875,7 +873,6 @@ > port { > vin1ep: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts > index a01101b..c16e870 100644 > --- a/arch/arm/boot/dts/r8a7791-porter.dts > +++ b/arch/arm/boot/dts/r8a7791-porter.dts > @@ -388,7 +388,6 @@ > port { > vin0ep: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts > index aa209f6..60aaddb 100644 > --- a/arch/arm/boot/dts/r8a7793-gose.dts > +++ b/arch/arm/boot/dts/r8a7793-gose.dts > @@ -765,10 +765,8 @@ > port { > vin0ep2: endpoint { > remote-endpoint = <&adv7612_out>; > - bus-width = <24>; > hsync-active = <0>; > vsync-active = <0>; > - pclk-sample = <1>; > data-active = <1>; > }; > }; > @@ -784,7 +782,6 @@ > port { > vin1ep: endpoint { > remote-endpoint = <&adv7180_out>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts > index e170275..8ed7a71 100644 > --- a/arch/arm/boot/dts/r8a7794-alt.dts > +++ b/arch/arm/boot/dts/r8a7794-alt.dts > @@ -388,7 +388,6 @@ > port { > vin0ep: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts > index 7808aae..6adfcd6 100644 > --- a/arch/arm/boot/dts/r8a7794-silk.dts > +++ b/arch/arm/boot/dts/r8a7794-silk.dts > @@ -477,7 +477,6 @@ > port { > vin0ep: endpoint { > remote-endpoint = <&adv7180>; > - bus-width = <8>; > }; > }; > }; > -- > 2.7.4 >
On Mon, Jun 04, 2018 at 02:23:25PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2018-05-29 17:05:59 +0200, Jacopo Mondi wrote: > > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN > > driver and only confuse users. Remove them in all Gen2 SoC that use > > them. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > The more I think about this the more I lean towards that this patch > should be dropped. The properties accurately describes the hardware and > I think there is value in that. That the driver currently don't parse or > make use of them don't in my view reduce there value. Maybe you should > break out this patch to a separate series? I also think there is value in describing the hardware not the state of the driver at this time. Is there any missmatch between these properties and the bindings?
Hi Simon, On Tue, Jun 05, 2018 at 09:49:38AM +0200, Simon Horman wrote: > On Mon, Jun 04, 2018 at 02:23:25PM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2018-05-29 17:05:59 +0200, Jacopo Mondi wrote: > > > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN > > > driver and only confuse users. Remove them in all Gen2 SoC that use > > > them. > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > The more I think about this the more I lean towards that this patch > > should be dropped. The properties accurately describes the hardware and > > I think there is value in that. That the driver currently don't parse or > > make use of them don't in my view reduce there value. Maybe you should > > break out this patch to a separate series? > > I also think there is value in describing the hardware not the state of the > driver at this time. Is there any missmatch between these properties and > the bindings? Niklas and I discussed a bit offline on this yesterday. My main concern, and sorry for being pedant on this, is that changing those properties value does not change the interface behaviour, and this could cause troubles when integrating image sensor not known to be working on the VIN interface. This said, the documentation of those (and all other) properties is in the generic "video-interfaces.txt" file and it is my understanding, but I think Laurent and Rob agree on this as well from their replies to my previous series, that each driver should list which properties it actually supports, as some aspects are very implementation specific, like default values and what happens if the property is not specified [1]. Nonetheless, all properties describing hardware features and documented in the generic file should be accepted in DTS, as those aims to be OS-independent and even independent from the single driver implementation. A possible middle-ground would be documenting in the VIN device tree bindings description properties whose values actually affect the VIN interface configuration and state in bindings that all generic properties described in 'video-interfaces.txt' are valid ones but if not explicitly listed there their value won't affect the interface configuration. Niklas suggested this, and I quite like the fact it makes clear that, say, changing the 'pclk-sample' property won't change how the VIN interface would sample data but at the same time allows for extensive description of the hardware. Would something like this work in your opinion? As a consequence, this patch should be dropped as Niklas and you suggested. Thanks j [1] Ie. An interface that supports BT.656 encoding will use it if 'hsync-active' and 'vsync-active' are not specified. One that does not support embedded synchronization would instead use defaults for those two properties. This is specific to the single interface (or even the single driver implementation actually) and should be listed in the single driver's bindings description imo.
Hi Jacopo, On Tue, Jun 5, 2018 at 10:12 AM, jacopo mondi <jacopo@jmondi.org> wrote: > On Tue, Jun 05, 2018 at 09:49:38AM +0200, Simon Horman wrote: >> On Mon, Jun 04, 2018 at 02:23:25PM +0200, Niklas Söderlund wrote: >> > On 2018-05-29 17:05:59 +0200, Jacopo Mondi wrote: >> > > The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN >> > > driver and only confuse users. Remove them in all Gen2 SoC that use >> > > them. >> > > >> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> > >> > The more I think about this the more I lean towards that this patch >> > should be dropped. The properties accurately describes the hardware and >> > I think there is value in that. That the driver currently don't parse or >> > make use of them don't in my view reduce there value. Maybe you should >> > break out this patch to a separate series? >> >> I also think there is value in describing the hardware not the state of the >> driver at this time. Is there any missmatch between these properties and >> the bindings? > > Niklas and I discussed a bit offline on this yesterday. My main > concern, and sorry for being pedant on this, is that changing those > properties value does not change the interface behaviour, and this > could cause troubles when integrating image sensor not known to be > working on the VIN interface. > > This said, the documentation of those (and all other) properties is in the > generic "video-interfaces.txt" file and it is my understanding, but I think > Laurent and Rob agree on this as well from their replies to my previous series, > that each driver should list which properties it actually supports, as s/driver/device-specific binding/ > some aspects are very implementation specific, like default values and > what happens if the property is not specified [1]. Nonetheless, all In se defaults are not (Linux) implementation-specific, but fixed in the DT bindings. > properties describing hardware features and documented in the generic > file should be accepted in DTS, as those aims to be OS-independent and > even independent from the single driver implementation. Gr{oetje,eeting}s, Geert
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt index 024c109..c6d7f60 100644 --- a/Documentation/devicetree/bindings/media/rcar_vin.txt +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt @@ -128,7 +128,6 @@ Board setup example for Gen2 platforms (vin1 composite video input) vin1ep0: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts index 092610e..9cdabfcf 100644 --- a/arch/arm/boot/dts/r8a7790-lager.dts +++ b/arch/arm/boot/dts/r8a7790-lager.dts @@ -885,10 +885,8 @@ port { vin0ep2: endpoint { remote-endpoint = <&adv7612_out>; - bus-width = <24>; hsync-active = <0>; vsync-active = <0>; - pclk-sample = <1>; data-active = <1>; }; }; @@ -904,7 +902,6 @@ port { vin1ep0: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts b/arch/arm/boot/dts/r8a7791-koelsch.dts index 8ab793d..033c9e3 100644 --- a/arch/arm/boot/dts/r8a7791-koelsch.dts +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts @@ -857,10 +857,8 @@ port { vin0ep2: endpoint { remote-endpoint = <&adv7612_out>; - bus-width = <24>; hsync-active = <0>; vsync-active = <0>; - pclk-sample = <1>; data-active = <1>; }; }; @@ -875,7 +873,6 @@ port { vin1ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7791-porter.dts b/arch/arm/boot/dts/r8a7791-porter.dts index a01101b..c16e870 100644 --- a/arch/arm/boot/dts/r8a7791-porter.dts +++ b/arch/arm/boot/dts/r8a7791-porter.dts @@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7793-gose.dts b/arch/arm/boot/dts/r8a7793-gose.dts index aa209f6..60aaddb 100644 --- a/arch/arm/boot/dts/r8a7793-gose.dts +++ b/arch/arm/boot/dts/r8a7793-gose.dts @@ -765,10 +765,8 @@ port { vin0ep2: endpoint { remote-endpoint = <&adv7612_out>; - bus-width = <24>; hsync-active = <0>; vsync-active = <0>; - pclk-sample = <1>; data-active = <1>; }; }; @@ -784,7 +782,6 @@ port { vin1ep: endpoint { remote-endpoint = <&adv7180_out>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7794-alt.dts b/arch/arm/boot/dts/r8a7794-alt.dts index e170275..8ed7a71 100644 --- a/arch/arm/boot/dts/r8a7794-alt.dts +++ b/arch/arm/boot/dts/r8a7794-alt.dts @@ -388,7 +388,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; }; diff --git a/arch/arm/boot/dts/r8a7794-silk.dts b/arch/arm/boot/dts/r8a7794-silk.dts index 7808aae..6adfcd6 100644 --- a/arch/arm/boot/dts/r8a7794-silk.dts +++ b/arch/arm/boot/dts/r8a7794-silk.dts @@ -477,7 +477,6 @@ port { vin0ep: endpoint { remote-endpoint = <&adv7180>; - bus-width = <8>; }; }; };
The 'bus-width' and 'pclk-sample' properties are not parsed by the VIN driver and only confuse users. Remove them in all Gen2 SoC that use them. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- v3: - remove bus-width from dt-bindings example --- Documentation/devicetree/bindings/media/rcar_vin.txt | 1 - arch/arm/boot/dts/r8a7790-lager.dts | 3 --- arch/arm/boot/dts/r8a7791-koelsch.dts | 3 --- arch/arm/boot/dts/r8a7791-porter.dts | 1 - arch/arm/boot/dts/r8a7793-gose.dts | 3 --- arch/arm/boot/dts/r8a7794-alt.dts | 1 - arch/arm/boot/dts/r8a7794-silk.dts | 1 - 7 files changed, 13 deletions(-) -- 2.7.4