Re: [PATCH v3 07/14] media: dt-bindings: add bindings for i.MX7 media driver

[Rui Miguel Silva <rui.silva@linaro.org>]

Hi Philipp,
Thanks for the review.

On Tue 08 May 2018 at 14:30, Philipp Zabel wrote:
> On Mon, 2018-05-07 at 17:21 +0100, Rui Miguel Silva wrote:
>> Add bindings documentation for i.MX7 media drivers.
>> 
>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> ---
>>  .../devicetree/bindings/media/imx7.txt        | 152 
>>  ++++++++++++++++++
>>  1 file changed, 152 insertions(+)
>>  create mode 100644 
>>  Documentation/devicetree/bindings/media/imx7.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/media/imx7.txt 
>> b/Documentation/devicetree/bindings/media/imx7.txt
>> new file mode 100644
>> index 000000000000..06d723d6354d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/imx7.txt
>> @@ -0,0 +1,152 @@
>> +Freescale i.MX7 Media Video Device
>> +==================================
>> +
>> +Video Media Controller node
>> +---------------------------
>> +
>> +This is the media controller node for video capture support. 
>> It is a
>> +virtual device that lists the camera serial interface nodes 
>> that the
>> +media device will control.
>> +
>> +Required properties:
>> +- compatible : "fsl,imx7-capture-subsystem";
>> +- ports      : Should contain a list of phandles pointing to 
>> camera
>> +		sensor interface port of CSI
>> +
>> +example:
>> +
>> +capture-subsystem {
>> +	compatible = "fsl,imx7-capture-subsystem";
>> +	ports = <&csi>;
>> +};
>
> Purely from a device tree perspective, I think this node is 
> unnecessary
> on i.MX7. The reason we have it for i.MX6 is that there are two
> identical IPUs on some of them, which makes it impossible to 
> reasonably
> bind the subsystem driver to one or the other of the ipu nodes. 
> On i.MX7
> the csi node is unique.
>
> The relevant imx-media-dev.c code in imx_media_probe could be 
> refactored
> into a utility function that could be called from the probe 
> function of
> the csi driver as well.

I think is a good idea, I will do something like this in v4
>
> [...]
>> +csi node
>> +--------
>> +
>> +This is device node for the CMOS Sensor Interface (CSI) which 
>> enables the chip
>> +to connect directly to external CMOS image sensors.
>> +
>> +Required properties:
>> +
>> +- compatible    : "fsl,imx7-csi";
>> +- reg           : base address and length of the register set 
>> for the device;
>> +- interrupts    : should contain CSI interrupt;
>> +- clocks        : list of clock specifiers, see
>> + 
>> Documentation/devicetree/bindings/clock/clock-bindings.txt for 
>> details;
>> +- clock-names   : must contain "axi", "mclk" and "dcic" 
>> entries, matching
>> +                 entries in the clock property;
>> +
>> +port node
>> +---------
>> +
>> +- reg		  : (required) should be 0 for the sink 
>> port;
>
> Not necessary, see below.
>
>> +
>> +example:
>> +
>> +                csi: csi@30710000 {
>> +                        #address-cells = <1>;
>> +                        #size-cells = <0>;
>> +
>> +                        compatible = "fsl,imx7-csi";
>> +                        reg = <0x30710000 0x10000>;
>> +                        interrupts = <GIC_SPI 7 
>> IRQ_TYPE_LEVEL_HIGH>;
>> +                        clocks = <&clks IMX7D_CLK_DUMMY>,
>> +                                        <&clks 
>> IMX7D_CSI_MCLK_ROOT_CLK>,
>> +                                        <&clks 
>> IMX7D_CLK_DUMMY>;
>> +                        clock-names = "axi", "mclk", "dcic";
>> +
>> +                        port@0 {
>> +                                reg = <0>;
>
> Since there is only one port, it does not need to be numbered.

Yes, ack.

---
Cheers,
	Rui