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

[Sakari Ailus <sakari.ailus@xxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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>;
> 

-- 
Kind regards,

Sakari Ailus