| Message ID | 20200717132859.237120-9-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | dt-bindings: media: ov5640: Convert to json-schema | expand |
Hi Jacopo, (CC'ing Sakari) Thank you for the patch. On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > The value of the data-shift property solely depend on the selected > bus width and it's not freely configurable. > > Remove it from the bindings document and update its users accordingly. Hmmmm that's an interesting one. Sakari, what do you think ? > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > 2 files changed, 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > index 5e1662e848bd..ab700a1830aa 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > @@ -92,12 +92,6 @@ properties: > parallel bus. > enum: [8, 10] > > - data-shift: > - description: | > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > - <0> for 10 bits parallel bus. > - enum: [0, 2] Should you document in the description of bus-width that data-shift is implied ? > - > hsync-active: > enum: [0, 1] > > @@ -115,7 +109,6 @@ properties: > then: > properties: > bus-width: false > - data-shift: false > hsync-active: false > vsync-active: false > pclk-sample: false > @@ -135,7 +128,6 @@ properties: > - remote-endpoint > - bus-type > - bus-width > - - data-shift > - hsync-active > - vsync-active > - pclk-sample > @@ -204,7 +196,6 @@ examples: > remote-endpoint = <¶llel_from_ov5640>; > bus-type = <5>; > bus-width = <10>; > - data-shift = <0>; > hsync-active = <1>; > vsync-active = <1>; > pclk-sample = <1>; > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts > index 613ede73b65b..96f96202ca63 100644 > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts > @@ -192,7 +192,6 @@ ov5640_0: endpoint { > remote-endpoint = <&dcmi_0>; > bus-type = <5>; > bus-width = <8>; > - data-shift = <2>; /* lines 9:2 are used */ > hsync-active = <0>; > vsync-active = <0>; > pclk-sample = <1>;
Hi Laurent, On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > (CC'ing Sakari) > > Thank you for the patch. > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > The value of the data-shift property solely depend on the selected > > bus width and it's not freely configurable. > > > > Remove it from the bindings document and update its users accordingly. > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > 2 files changed, 10 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > index 5e1662e848bd..ab700a1830aa 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > @@ -92,12 +92,6 @@ properties: > > parallel bus. > > enum: [8, 10] > > > > - data-shift: > > - description: | > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > - <0> for 10 bits parallel bus. > > - enum: [0, 2] > > Should you document in the description of bus-width that data-shift is > implied ? The purpose of the datas-shift property is to convey how the parallel bus lines are connected for a given bus width for devices where it is configurable. As this device does not not support that, then indeed this property is not relevant for the device IMO. > > > - > > hsync-active: > > enum: [0, 1] > > > > @@ -115,7 +109,6 @@ properties: > > then: > > properties: > > bus-width: false > > - data-shift: false > > hsync-active: false > > vsync-active: false > > pclk-sample: false > > @@ -135,7 +128,6 @@ properties: > > - remote-endpoint > > - bus-type > > - bus-width > > - - data-shift > > - hsync-active > > - vsync-active > > - pclk-sample > > @@ -204,7 +196,6 @@ examples: > > remote-endpoint = <¶llel_from_ov5640>; > > bus-type = <5>; > > bus-width = <10>; > > - data-shift = <0>; > > hsync-active = <1>; > > vsync-active = <1>; > > pclk-sample = <1>; > > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts > > index 613ede73b65b..96f96202ca63 100644 > > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts > > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts > > @@ -192,7 +192,6 @@ ov5640_0: endpoint { > > remote-endpoint = <&dcmi_0>; > > bus-type = <5>; > > bus-width = <8>; > > - data-shift = <2>; /* lines 9:2 are used */ > > hsync-active = <0>; > > vsync-active = <0>; > > pclk-sample = <1>; >
Hi Sakari, On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > (CC'ing Sakari) > > > > Thank you for the patch. > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > The value of the data-shift property solely depend on the selected > > > bus width and it's not freely configurable. > > > > > > Remove it from the bindings document and update its users accordingly. > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > 2 files changed, 10 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > index 5e1662e848bd..ab700a1830aa 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > @@ -92,12 +92,6 @@ properties: > > > parallel bus. > > > enum: [8, 10] > > > > > > - data-shift: > > > - description: | > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > - <0> for 10 bits parallel bus. > > > - enum: [0, 2] > > > > Should you document in the description of bus-width that data-shift is > > implied ? > > The purpose of the datas-shift property is to convey how the parallel bus > lines are connected for a given bus width for devices where it is > configurable. As this device does not not support that, then indeed this > property is not relevant for the device IMO. Could you elaborate on this ? I believe the case that Jacopo is describing connects D[9:2] from the sensor to D[7:0] of the receiver (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > - > > > hsync-active: > > > enum: [0, 1] > > > > > > @@ -115,7 +109,6 @@ properties: > > > then: > > > properties: > > > bus-width: false > > > - data-shift: false > > > hsync-active: false > > > vsync-active: false > > > pclk-sample: false > > > @@ -135,7 +128,6 @@ properties: > > > - remote-endpoint > > > - bus-type > > > - bus-width > > > - - data-shift > > > - hsync-active > > > - vsync-active > > > - pclk-sample > > > @@ -204,7 +196,6 @@ examples: > > > remote-endpoint = <¶llel_from_ov5640>; > > > bus-type = <5>; > > > bus-width = <10>; > > > - data-shift = <0>; > > > hsync-active = <1>; > > > vsync-active = <1>; > > > pclk-sample = <1>; > > > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts > > > index 613ede73b65b..96f96202ca63 100644 > > > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts > > > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts > > > @@ -192,7 +192,6 @@ ov5640_0: endpoint { > > > remote-endpoint = <&dcmi_0>; > > > bus-type = <5>; > > > bus-width = <8>; > > > - data-shift = <2>; /* lines 9:2 are used */ > > > hsync-active = <0>; > > > vsync-active = <0>; > > > pclk-sample = <1>;
Hi Laurent, On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > (CC'ing Sakari) > > > > > > Thank you for the patch. > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > The value of the data-shift property solely depend on the selected > > > > bus width and it's not freely configurable. > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > --- > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > 2 files changed, 10 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > @@ -92,12 +92,6 @@ properties: > > > > parallel bus. > > > > enum: [8, 10] > > > > > > > > - data-shift: > > > > - description: | > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > - <0> for 10 bits parallel bus. > > > > - enum: [0, 2] > > > > > > Should you document in the description of bus-width that data-shift is > > > implied ? > > > > The purpose of the datas-shift property is to convey how the parallel bus > > lines are connected for a given bus width for devices where it is > > configurable. As this device does not not support that, then indeed this > > property is not relevant for the device IMO. > > Could you elaborate on this ? I believe the case that Jacopo is > describing connects D[9:2] from the sensor to D[7:0] of the receiver > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? Yes, it is. But in this case what data-shift configures is not configurable as such but defined by another configuration, making the data-shift property redundant. We generally haven't documented redundant things in DT bindings --- for instance data-lanes is documented in bindings only if it is configurable. That said, it'd be nice to say which pins are used on less than full width busses.
Hi Sakari, On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > Hi Jacopo, > > > > > > > > (CC'ing Sakari) > > > > > > > > Thank you for the patch. > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > The value of the data-shift property solely depend on the selected > > > > > bus width and it's not freely configurable. > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > --- > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > @@ -92,12 +92,6 @@ properties: > > > > > parallel bus. > > > > > enum: [8, 10] > > > > > > > > > > - data-shift: > > > > > - description: | > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > - <0> for 10 bits parallel bus. > > > > > - enum: [0, 2] > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > implied ? > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > lines are connected for a given bus width for devices where it is > > > configurable. As this device does not not support that, then indeed this > > > property is not relevant for the device IMO. > > > > Could you elaborate on this ? I believe the case that Jacopo is > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > Yes, it is. But in this case what data-shift configures is not configurable > as such but defined by another configuration, making the data-shift > property redundant. We generally haven't documented redundant things in DT > bindings --- for instance data-lanes is documented in bindings only if it > is configurable. Then I think we share the same understanding. I believe the documentation in video-interfaces.txt needs to be expanded, as it's quite terse and not very clear. > That said, it'd be nice to say which pins are used on less than full width > busses.
On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > > Hi Jacopo, > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > > The value of the data-shift property solely depend on the selected > > > > > > bus width and it's not freely configurable. > > > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > --- > > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > @@ -92,12 +92,6 @@ properties: > > > > > > parallel bus. > > > > > > enum: [8, 10] > > > > > > > > > > > > - data-shift: > > > > > > - description: | > > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > > - <0> for 10 bits parallel bus. > > > > > > - enum: [0, 2] > > > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > > implied ? > > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > > lines are connected for a given bus width for devices where it is > > > > configurable. As this device does not not support that, then indeed this > > > > property is not relevant for the device IMO. > > > > > > Could you elaborate on this ? I believe the case that Jacopo is > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > > Yes, it is. But in this case what data-shift configures is not configurable > > as such but defined by another configuration, making the data-shift > > property redundant. We generally haven't documented redundant things in DT > > bindings --- for instance data-lanes is documented in bindings only if it > > is configurable. > > Then I think we share the same understanding. I believe the > documentation in video-interfaces.txt needs to be expanded, as it's > quite terse and not very clear. The DT spec states that: A DTSpec-compliant devicetree describes device information in a system that cannot necessarily be dynamically detected by a client program. For example, the architecture of PCI enables a client to probe and detect attached devices, and thus devicetree nodes describing PCI devices might not be required. However, a device node is required to describe a PCI host bridge device in the system if it cannot be detected by probing. I'd read that as there's no need to specify properties that do not provide additional information to software. As some properties are dependent on others and and this depends on hardware features, I don't think we can in general case take this account in generic binding documentation, but device specific ones. Of course we could add this to data-shift documentation, but then I wonder how many other similar cases there are where in hardware the configuration defined by one property determines the value of another?
Hi Sakari, On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote: > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote: > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > > > Hi Jacopo, > > > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > > > The value of the data-shift property solely depend on the selected > > > > > > > bus width and it's not freely configurable. > > > > > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > > --- > > > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > @@ -92,12 +92,6 @@ properties: > > > > > > > parallel bus. > > > > > > > enum: [8, 10] > > > > > > > > > > > > > > - data-shift: > > > > > > > - description: | > > > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > > > - <0> for 10 bits parallel bus. > > > > > > > - enum: [0, 2] > > > > > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > > > implied ? > > > > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > > > lines are connected for a given bus width for devices where it is > > > > > configurable. As this device does not not support that, then indeed this > > > > > property is not relevant for the device IMO. > > > > > > > > Could you elaborate on this ? I believe the case that Jacopo is > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > > > > Yes, it is. But in this case what data-shift configures is not configurable > > > as such but defined by another configuration, making the data-shift > > > property redundant. We generally haven't documented redundant things in DT > > > bindings --- for instance data-lanes is documented in bindings only if it > > > is configurable. > > > > Then I think we share the same understanding. I believe the > > documentation in video-interfaces.txt needs to be expanded, as it's > > quite terse and not very clear. > > The DT spec states that: > > A DTSpec-compliant devicetree describes device information in a > system that cannot necessarily be dynamically detected by a client > program. For example, the architecture of PCI enables a client to > probe and detect attached devices, and thus devicetree nodes > describing PCI devices might not be required. However, a device > node is required to describe a PCI host bridge device in the system > if it cannot be detected by probing. > > I'd read that as there's no need to specify properties that do not provide > additional information to software. That's a bit of a stretch interpretation :-) > As some properties are dependent on > others and and this depends on hardware features, I don't think we can in > general case take this account in generic binding documentation, but device > specific ones. > > Of course we could add this to data-shift documentation, but then I wonder > how many other similar cases there are where in hardware the configuration > defined by one property determines the value of another? I was mostly thinking about documenting *how* data-shift interacts with bus-width. I think that specifying the default data-shift value based on the bus-width value, for the case where data-shift is not specified, would also make sense.
Hi Laurent, On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote: > > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote: > > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > > > > Hi Jacopo, > > > > > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > > > > The value of the data-shift property solely depend on the selected > > > > > > > > bus width and it's not freely configurable. > > > > > > > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > > > --- > > > > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > @@ -92,12 +92,6 @@ properties: > > > > > > > > parallel bus. > > > > > > > > enum: [8, 10] > > > > > > > > > > > > > > > > - data-shift: > > > > > > > > - description: | > > > > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > > > > - <0> for 10 bits parallel bus. > > > > > > > > - enum: [0, 2] > > > > > > > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > > > > implied ? > > > > > > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > > > > lines are connected for a given bus width for devices where it is > > > > > > configurable. As this device does not not support that, then indeed this > > > > > > property is not relevant for the device IMO. > > > > > > > > > > Could you elaborate on this ? I believe the case that Jacopo is > > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > > > > > > Yes, it is. But in this case what data-shift configures is not configurable > > > > as such but defined by another configuration, making the data-shift > > > > property redundant. We generally haven't documented redundant things in DT > > > > bindings --- for instance data-lanes is documented in bindings only if it > > > > is configurable. > > > > > > Then I think we share the same understanding. I believe the > > > documentation in video-interfaces.txt needs to be expanded, as it's > > > quite terse and not very clear. > > > > The DT spec states that: > > > > A DTSpec-compliant devicetree describes device information in a > > system that cannot necessarily be dynamically detected by a client > > program. For example, the architecture of PCI enables a client to > > probe and detect attached devices, and thus devicetree nodes > > describing PCI devices might not be required. However, a device > > node is required to describe a PCI host bridge device in the system > > if it cannot be detected by probing. > > > > I'd read that as there's no need to specify properties that do not provide > > additional information to software. > > That's a bit of a stretch interpretation :-) > > > As some properties are dependent on > > others and and this depends on hardware features, I don't think we can in > > general case take this account in generic binding documentation, but device > > specific ones. > > > > Of course we could add this to data-shift documentation, but then I wonder > > how many other similar cases there are where in hardware the configuration > > defined by one property determines the value of another? > > I was mostly thinking about documenting *how* data-shift interacts with > bus-width. I think that specifying the default data-shift value based on > the bus-width value, for the case where data-shift is not specified, > would also make sense. Do you mean in device binding documentation or in generic documentation? Device bindings should have this information, yes.
Hi Sakari, On Thu, Jul 30, 2020 at 07:22:19PM +0300, Sakari Ailus wrote: > On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote: > > > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote: > > > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > > > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > > > > > Hi Jacopo, > > > > > > > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > > > > > The value of the data-shift property solely depend on the selected > > > > > > > > > bus width and it's not freely configurable. > > > > > > > > > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > > > > --- > > > > > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > @@ -92,12 +92,6 @@ properties: > > > > > > > > > parallel bus. > > > > > > > > > enum: [8, 10] > > > > > > > > > > > > > > > > > > - data-shift: > > > > > > > > > - description: | > > > > > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > > > > > - <0> for 10 bits parallel bus. > > > > > > > > > - enum: [0, 2] > > > > > > > > > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > > > > > implied ? > > > > > > > > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > > > > > lines are connected for a given bus width for devices where it is > > > > > > > configurable. As this device does not not support that, then indeed this > > > > > > > property is not relevant for the device IMO. > > > > > > > > > > > > Could you elaborate on this ? I believe the case that Jacopo is > > > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > > > > > > > > Yes, it is. But in this case what data-shift configures is not configurable > > > > > as such but defined by another configuration, making the data-shift > > > > > property redundant. We generally haven't documented redundant things in DT > > > > > bindings --- for instance data-lanes is documented in bindings only if it > > > > > is configurable. > > > > > > > > Then I think we share the same understanding. I believe the > > > > documentation in video-interfaces.txt needs to be expanded, as it's > > > > quite terse and not very clear. > > > > > > The DT spec states that: > > > > > > A DTSpec-compliant devicetree describes device information in a > > > system that cannot necessarily be dynamically detected by a client > > > program. For example, the architecture of PCI enables a client to > > > probe and detect attached devices, and thus devicetree nodes > > > describing PCI devices might not be required. However, a device > > > node is required to describe a PCI host bridge device in the system > > > if it cannot be detected by probing. > > > > > > I'd read that as there's no need to specify properties that do not provide > > > additional information to software. > > > > That's a bit of a stretch interpretation :-) > > > > > As some properties are dependent on > > > others and and this depends on hardware features, I don't think we can in > > > general case take this account in generic binding documentation, but device > > > specific ones. > > > > > > Of course we could add this to data-shift documentation, but then I wonder > > > how many other similar cases there are where in hardware the configuration > > > defined by one property determines the value of another? > > > > I was mostly thinking about documenting *how* data-shift interacts with > > bus-width. I think that specifying the default data-shift value based on > > the bus-width value, for the case where data-shift is not specified, > > would also make sense. > > Do you mean in device binding documentation or in generic documentation? > Device bindings should have this information, yes. I mean in video-interfaces.txt (which should become video-interfaces.yaml :-)) for the general rules, and in specific bindings for any device-specific rule. We will likely need a runtime API too, it's entirely conceivable that a 10-bit parallel sensor, which D[9:0] signals, could use either D[9:2] or D[7:0] when configured to transmit 10-bit data. This isn't something that can be encoded in DT. It's a separate topic though.
Hi Laurent, On Thu, Jul 30, 2020 at 07:32:11PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Jul 30, 2020 at 07:22:19PM +0300, Sakari Ailus wrote: > > On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote: > > > On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote: > > > > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote: > > > > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote: > > > > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote: > > > > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote: > > > > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote: > > > > > > > > > Hi Jacopo, > > > > > > > > > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote: > > > > > > > > > > The value of the data-shift property solely depend on the selected > > > > > > > > > > bus width and it's not freely configurable. > > > > > > > > > > > > > > > > > > > > Remove it from the bindings document and update its users accordingly. > > > > > > > > > > > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ? > > > > > > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > > > > > > > --- > > > > > > > > > > Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- > > > > > > > > > > arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - > > > > > > > > > > 2 files changed, 10 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > > index 5e1662e848bd..ab700a1830aa 100644 > > > > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml > > > > > > > > > > @@ -92,12 +92,6 @@ properties: > > > > > > > > > > parallel bus. > > > > > > > > > > enum: [8, 10] > > > > > > > > > > > > > > > > > > > > - data-shift: > > > > > > > > > > - description: | > > > > > > > > > > - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or > > > > > > > > > > - <0> for 10 bits parallel bus. > > > > > > > > > > - enum: [0, 2] > > > > > > > > > > > > > > > > > > Should you document in the description of bus-width that data-shift is > > > > > > > > > implied ? > > > > > > > > > > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus > > > > > > > > lines are connected for a given bus width for devices where it is > > > > > > > > configurable. As this device does not not support that, then indeed this > > > > > > > > property is not relevant for the device IMO. > > > > > > > > > > > > > > Could you elaborate on this ? I believe the case that Jacopo is > > > > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver > > > > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ? > > > > > > > > > > > > Yes, it is. But in this case what data-shift configures is not configurable > > > > > > as such but defined by another configuration, making the data-shift > > > > > > property redundant. We generally haven't documented redundant things in DT > > > > > > bindings --- for instance data-lanes is documented in bindings only if it > > > > > > is configurable. > > > > > > > > > > Then I think we share the same understanding. I believe the > > > > > documentation in video-interfaces.txt needs to be expanded, as it's > > > > > quite terse and not very clear. > > > > > > > > The DT spec states that: > > > > > > > > A DTSpec-compliant devicetree describes device information in a > > > > system that cannot necessarily be dynamically detected by a client > > > > program. For example, the architecture of PCI enables a client to > > > > probe and detect attached devices, and thus devicetree nodes > > > > describing PCI devices might not be required. However, a device > > > > node is required to describe a PCI host bridge device in the system > > > > if it cannot be detected by probing. > > > > > > > > I'd read that as there's no need to specify properties that do not provide > > > > additional information to software. > > > > > > That's a bit of a stretch interpretation :-) > > > > > > > As some properties are dependent on > > > > others and and this depends on hardware features, I don't think we can in > > > > general case take this account in generic binding documentation, but device > > > > specific ones. > > > > > > > > Of course we could add this to data-shift documentation, but then I wonder > > > > how many other similar cases there are where in hardware the configuration > > > > defined by one property determines the value of another? > > > > > > I was mostly thinking about documenting *how* data-shift interacts with > > > bus-width. I think that specifying the default data-shift value based on > > > the bus-width value, for the case where data-shift is not specified, > > > would also make sense. > > > > Do you mean in device binding documentation or in generic documentation? > > Device bindings should have this information, yes. > > I mean in video-interfaces.txt (which should become > video-interfaces.yaml :-)) for the general rules, and in specific > bindings for any device-specific rule. Please send a patch. :-) > > We will likely need a runtime API too, it's entirely conceivable that a > 10-bit parallel sensor, which D[9:0] signals, could use either D[9:2] or > D[7:0] when configured to transmit 10-bit data. This isn't something > that can be encoded in DT. It's a separate topic though. You can set defaults in the current API but those defaults are basically a C struct. I don't think we should be looking into making those defaults depend on property values, unless there's a clear reason to do so --- and in this case there isn't one.
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml index 5e1662e848bd..ab700a1830aa 100644 --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml @@ -92,12 +92,6 @@ properties: parallel bus. enum: [8, 10] - data-shift: - description: | - Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or - <0> for 10 bits parallel bus. - enum: [0, 2] - hsync-active: enum: [0, 1] @@ -115,7 +109,6 @@ properties: then: properties: bus-width: false - data-shift: false hsync-active: false vsync-active: false pclk-sample: false @@ -135,7 +128,6 @@ properties: - remote-endpoint - bus-type - bus-width - - data-shift - hsync-active - vsync-active - pclk-sample @@ -204,7 +196,6 @@ examples: remote-endpoint = <¶llel_from_ov5640>; bus-type = <5>; bus-width = <10>; - data-shift = <0>; hsync-active = <1>; vsync-active = <1>; pclk-sample = <1>; diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts index 613ede73b65b..96f96202ca63 100644 --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts @@ -192,7 +192,6 @@ ov5640_0: endpoint { remote-endpoint = <&dcmi_0>; bus-type = <5>; bus-width = <8>; - data-shift = <2>; /* lines 9:2 are used */ hsync-active = <0>; vsync-active = <0>; pclk-sample = <1>;
The value of the data-shift property solely depend on the selected bus width and it's not freely configurable. Remove it from the bindings document and update its users accordingly. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 --------- arch/arm/boot/dts/stm32mp157c-ev1.dts | 1 - 2 files changed, 10 deletions(-) -- 2.27.0