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

[Laurent Pinchart]

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 at 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.

-- 
Regards,

Laurent Pinchart