Hello Eugen,

On Fri, Oct 16, 2020 at 12:30:59PM +0000, Eugen.Hristev@microchip.com wrote:

On 12.10.2020 16:04, Jacopo Mondi wrote:

quoted

Hello,
    just my 2 cents, as I've noticed this patch skimming through the
    list

On Mon, Oct 12, 2020 at 07:19:43AM +0000, Eugen.Hristev@microchip.com wrote:

[snip]

Hi,

Thanks for helping,

quoted

Is this property describing the CSI-2 clock continuous, non-continuous
mode configuration, or did I mis-interpreted it ?

I think so. This is a setting inside the csi2dc regarding clock. If we
can obtain it from pads operations, then it's good, but the question is,
if the devices can provide this or not ?

The transmitter can provide this information, as it knows which clock
mode it is going to use (it's not clear from the CSI-2 spec which side
is in charge of the selection, but it seems natural to me that it's up
to the transmitter side). Whether they implement the right operation
to do so, well... see below

quoted

We added support for retrieving run-time configuration of the media
bus with the get_mbus_config pad operations recently. Among the
configuration flags for MBUS_CSI2_DPHY there are inded CONTINUOUS and
NON_CONTINUOUS clock flags.

quoted

quoted

quoted

+
+  microchip,inter-line-delay:
+    allOf:
+    - $ref: /schemas/types.yaml#/definitions/uint32
+    - minimum: 1
+    - maximum: 16
+    default: 16
+    description:
+      Indicates how many clock cycles should be introduced between each line.

This also sounds like a configuration parameter. How does one compute
the right value for this ?

I think this is a delay that can be added inside the hardware block,
depending on the interface speed and bandwidth. I will try to understand
more details from the hardware design and come back with a more detailed
answer.

Regarding this, I will remove it. Our design team advised to have a
hardcoded value for this product.

quoted

quoted

quoted

quoted

+
+
+        properties:
+          reg:
+            enum: [0, 1, 2, 3]
+            description: virtual channel for the endpoint

The virtual channel used by the source is also something that needs to
be queried from the source at runtime, it doesn't belong to this
binding.

The same question as for the gated clock configuration. How can we use
v4l2 subdevice API to obtain such information from the subdevice? And if
the subdevice does not offer such information ?

I think the subdev driver should be instrumented to report it instead of
hard-coding the information in DT which should be otherwise updated
depending on which sensor is connected to the board. Does it make
sense to you ?

It does, but then, it won't work unless connected to instrumented
subdevices. Which is not really something I would do, since it would
completely limit the usability.
Do you have any example on how to get the virtual id from the subdev ?

As examples of CSI-2 transmitters implementing get_mbus_config() we
have tc358743 and adv748x reporting the number of active data lanes.

Reporting the virtual channel in use is a matter of using one of the
following flags:

/* CSI-2 Virtual Channel identifiers. */
#define V4L2_MBUS_CSI2_CHANNEL_0		BIT(4)
#define V4L2_MBUS_CSI2_CHANNEL_1		BIT(5)
#define V4L2_MBUS_CSI2_CHANNEL_2		BIT(6)
#define V4L2_MBUS_CSI2_CHANNEL_3		BIT(7)

As an example of a receiver driver rcar-csi2 uses the operation to
dynamically negotiate the number of data lanes. To handle virtual
channel you would need to inspect which of the above flags have been
set by the subdevice.

On deciding if this better handled by using a subdev operation or a DT
property, I understand the subdev driver needs to implement
get_mbus_config, but hardcoding it in DT I fear is a no-go for
mainline and honestly it doesn't sound much more advantageous, as it
needs to be adapted according to the connected sensor anyway, doesn't
it ? But I'm happy to defer this call to the maintainers, I hope I
just have provided some useful references.

Cheers
   j

Thanks again,

Eugen

quoted

Cheers
    j

quoted

Thanks again,

Eugen

quoted

quoted

+
+          remote-endpoint: true
+
+        required:
+          - remote-endpoint
+          - reg
+
+        additionalProperties: false
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - port@0
+
+examples:
+  - |
+    csi2dc@e1404000 {
+        compatible = "microchip,sama7g5-csi2dc";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0xe1404000 0x500>;
+        clocks = <&pclk>, <&scck>;
+        clock-names = "pclk", "scck";
+
+        port@0 {
+               reg = <0>; /* must be 0, first child port */
+               csi2dc_in: endpoint { /* input from IDI interface */
+                     remote-endpoint = <&csi2host_out>;
+               };
+        };
+
+        port@1 {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <1>; /* must be 1, second child port */
+                csi2dc_out: endpoint@2 {
+                        reg = <2>;  /* virtual channel identifier */
+                        remote-endpoint = <&xisc_in>; /* output to sensor controller */
+                };
+        };
+    };
+
+...

--
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel