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

[Sakari Ailus <sakari.ailus@iki.fi>]

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.

-- 
Kind regards,

Sakari Ailus