Re: [PATCH 1/4] Documentation: dt: Add bindings documentation for DW MIPI CSI-2 Host

[Ramiro Oliveira]

Hi Rob,

Thank you for your feedback and sorry for the late reply.

On 3/15/2017 6:26 PM, Rob Herring wrote:
> On Tue, Mar 07, 2017 at 02:37:48PM +0000, Ramiro Oliveira wrote:
>> Create device tree bindings documentation for the Synopsys DW MIPI CSI-2
>>  Host.
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@xxxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/media/snps,dw-mipi-csi.txt | 37 ++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt b/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
>> new file mode 100644
>> index 000000000000..5b24eb43d760
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/snps,dw-mipi-csi.txt
>> @@ -0,0 +1,37 @@
>> +Synopsys DesignWare CSI-2 Host controller
>> +
>> +Description
>> +-----------
>> +
>> +This HW block is used to receive image coming from an MIPI CSI-2 compatible
>> +camera.
>> +
>> +Required properties:
>> +- compatible: shall be "snps,dw-mipi-csi"
> 
> ... "and an SoC specific compatible string"
> 

I'm not sure what you mean by this.

>> +- reg		: physical base address and size of the device memory mapped
>> +  registers;
>> +- interrupts	: CSI-2 Host interrupt
>> +- output-type   : Core output to be used (IPI-> 0 or IDI->1 or BOTH->2) These
>> +  values choose which of the Core outputs will be used, it can be Image Data
>> +  Interface or Image Pixel Interface.
>> +- phys: List of one PHY specifier (as defined in
>> +  Documentation/devicetree/bindings/phy/phy-bindings.txt). This PHY is a MIPI
>> +  DPHY working in RX mode.
>> +- resets: Reference to a reset controller (optional)
>> +
>> +Optional properties(if in IPI mode):
>> +- ipi-mode 	: Mode to be used when in IPI(Camera -> 0 or Controller -> 1)
>> +  This property defines if the controller will use the video timings available
>> +  in the video stream or if it will use pre-defined ones.
> 
> This could be boolean instead. I'd expect the former to be the typical 
> mode, so name the property for the latter.
> 

Sure. I can do this.

>> +- ipi-color-mode: Bus depth to be used in IPI (48 bits -> 0 or 16 bits -> 1)
>> +  This property defines the width of the IPI bus.
> 
> Are these the only 2 modes that any h/w would ever have (not just your 
> controller)? Perhaps using the actual bits for the value would be 
> better. Also, if this is the bus width, then the property should be 
> named that as bus widths and pixel formats or color modes are not 
> necessarily the same.
> 

I'm not sure I understand what you mean, but I'll try to explain what this does.

Our controller, when in IPI mode, has a data bus that can be either 16 or 48
bits wide. However when in 48 bits mode some data types will not use the entire
bus width.

>> +- ipi-auto-flush: Data auto-flush (1 -> Yes or 0 -> No). This property defines
>> +  if the data is automatically flushed in each vsync or if this process is done
>> +  manually
> 
> This could be boolean.
> 

I'll change it.

>> +- virtual-channel: Virtual channel where data is present when in IPI mode. This
>> +  property chooses the virtual channel which IPI will use to retrieve the video
>> +  stream.
> 
> Again, some of these should probably be common properties (and therefore 
> documented in a common location). I'm looking for some feedback from 
> video/camera maintainers on this. I know I've seen some other similar 
> bindings for camera interfaces recently.
> 

You're right, Virtual channel is MIPI CSI-2 property, not a Synopsys one, so
this could be a common property, although I haven't found it anywhere.

> Rob
> 

-- 
Best Regards

Ramiro Oliveira
Ramiro.Oliveira@xxxxxxxxxxxx