diff mbox series

[08/13] dt-bindings: media: ov5640: Remove data-shift

Message ID 20200717132859.237120-9-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series dt-bindings: media: ov5640: Convert to json-schema | expand

Commit Message

Jacopo Mondi July 17, 2020, 1:28 p.m. UTC
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

Comments

Laurent Pinchart July 17, 2020, 8:57 p.m. UTC | #1
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 = <&parallel_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>;
Sakari Ailus July 23, 2020, 10:22 p.m. UTC | #2
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 = <&parallel_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>;
>
Laurent Pinchart July 23, 2020, 11:15 p.m. UTC | #3
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 = <&parallel_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>;
Sakari Ailus July 25, 2020, 9:18 p.m. UTC | #4
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.
Laurent Pinchart July 25, 2020, 9:31 p.m. UTC | #5
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.
Sakari Ailus July 29, 2020, 2:29 p.m. UTC | #6
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?
Laurent Pinchart July 29, 2020, 2:46 p.m. UTC | #7
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.
Sakari Ailus July 30, 2020, 4:22 p.m. UTC | #8
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.
Laurent Pinchart July 30, 2020, 4:32 p.m. UTC | #9
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.
Sakari Ailus July 30, 2020, 4:43 p.m. UTC | #10
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 mbox series

Patch

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 = <&parallel_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>;