Re: [PATCH v2 6/7] dt-bindings: media: Add Renesas R-Car DRIF binding

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/03/2017 04:20 PM, Ramesh Shanmugasundaram wrote:
> Hi Laurent, Geert,
> 
> Thanks for the review comments.
> 
>>> On Wednesday 21 Dec 2016 08:10:37 Ramesh Shanmugasundaram wrote:
>>>> Add binding documentation for Renesas R-Car Digital Radio Interface
>>>> (DRIF) controller.
>>>>
>>>> Signed-off-by: Ramesh Shanmugasundaram
>>>> <ramesh.shanmugasundaram@bp.renesas.com> ---
>>>>  .../devicetree/bindings/media/renesas,drif.txt     | 202
>> ++++++++++++++++++
>>>>  1 file changed, 202 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/media/renesas,drif.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt
>>>> b/Documentation/devicetree/bindings/media/renesas,drif.txt new file
>>>> mode
>>>> 100644
>>>> index 0000000..1f3feaf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
>>
>>>> +Optional properties of an internal channel when:
>>>> +     - It is the only enabled channel of the bond (or)
>>>> +     - If it acts as primary among enabled bonds
>>>> +--------------------------------------------------------
>>>> +- renesas,syncmd       : sync mode
>>>> +                      0 (Frame start sync pulse mode. 1-bit width
>> pulse
>>>> +                         indicates start of a frame)
>>>> +                      1 (L/R sync or I2S mode) (default)
>>>> +- renesas,lsb-first    : empty property indicates lsb bit is received
>>>> first.
>>>> +                      When not defined msb bit is received first
>>>> +(default)
>>>> +- renesas,syncac-active: Indicates sync signal polarity, 0/1 for
>> low/high

Shouldn't this be 'renesas,sync-active' instead of syncac-active?

I'm not sure if syncac is intended or if it is a typo.

>>>> +                      respectively. The default is 1 (active high)
>>>> +- renesas,dtdl         : delay between sync signal and start of
>> reception.
>>>> +                      The possible values are represented in 0.5 clock
>>>> +                      cycle units and the range is 0 to 4. The default
>>>> +                      value is 2 (i.e.) 1 clock cycle delay.
>>>> +- renesas,syncdl       : delay between end of reception and sync
>> signal
>>>> edge.
>>>> +                      The possible values are represented in 0.5 clock
>>>> +                      cycle units and the range is 0 to 4 & 6. The
>> default
>>>> +                      value is 0 (i.e.) no delay.
>>>
>>> Most of these properties are pretty similar to the video bus
>>> properties defined at the endpoint level in
>>> Documentation/devicetree/bindings/media/video-interfaces.txt. I
>>> believe it would make sense to use OF graph and try to standardize
>>> these properties similarly.

Other than sync-active, is there really anything else that is similar? And
even the sync-active isn't a good fit since here there is only one sync
signal instead of two for video (h and vsync).

Regards,

	Hans

>> Note that the last two properties match the those in
>> Documentation/devicetree/bindings/spi/sh-msiof.txt.
>> We may want to use one DRIF channel as a plain SPI slave with the
>> (modified) MSIOF driver in the future.
> 
> Should I leave it as it is or modify these as in video-interfaces.txt? Shall we conclude on this please?
> 
> Thanks,
> Ramesh
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{���bj)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
>