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:

quoted

On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote:

quoted

On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote:

quoted

On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:

quoted

On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:

quoted

On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:

quoted

On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:

quoted

On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:

quoted

Hi Jacopo,

(CC'ing Sakari)

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:

quoted

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 ?

quoted

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 :-)

quoted

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