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

Hi Hans,

Many thanks for the response & comments.

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

On 01/10/2017 10:31 AM, Ramesh Shanmugasundaram wrote:

quoted

Hi Laurent,

quoted

quoted

quoted

quoted

quoted

On Wednesday 21 Dec 2016 08:10:37 Ramesh Shanmugasundaram wrote:

quoted

Add binding documentation for Renesas R-Car Digital Radio
Interface
(DRIF) controller.

Signed-off-by: Ramesh Shanmugasundaram
[off-list ref] ---

 .../devicetree/bindings/media/renesas,drif.txt     | 202

+++++++++++++

quoted

quoted

quoted

quoted

quoted

 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

quoted

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

+                      respectively. The default is 1 (active

high)

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+- renesas,dtdl         : delay between sync signal and start of
reception.
+                      The possible values are represented in
+ 0.5

clock

quoted

quoted

quoted

quoted

quoted

+                      cycle units and the range is 0 to 4. The

default

quoted

quoted

quoted

quoted

quoted

+                      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

quoted

quoted

quoted

quoted

quoted

+                      cycle units and the range is 0 to 4 & 6.
+ The
default
+                      value is 0 (i.e.) no delay.

Are these properties actually going to be used by anyone? Just curious.

Yes. Each of this property should be set appropriately based on the master device it interfaces with. 

quoted

quoted

quoted

quoted

quoted

quoted

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

That's why I said similar, not identical :-) My point is that, if we
consider that we could connect multiple sources to the DRIF, using OF
graph would make sense, and the above properties should then be
defined per endpoint.

Thanks for the clarifications. I have some questions.

- Assuming two devices are interfaced with DRIF and they are represented

using two endpoints, the control signal related properties of DRIF might
still need to be same for both endpoints? For e.g. syncac-active cannot be
different in both endpoints?

Usually that's the case, but HW designers are weird :-)

quoted

- I suppose "lsb-first", "dtdl" & "syncdl" may be defined per endpoint.

However, h/w manual says same register values needs to be programmed for
both the internal channels of a channel. Same with "syncmd" property.

quoted

We could still define them as per endpoint property with a note that

they need to be same. But I am not sure if that is what you intended?

quoted

 If we define them per endpoint we should then also try

quoted

standardize the ones that are not really Renesas-specific (that's at
least syncac-active).

OK. I will call it "sync-active".

That's better, yes.

Thanks.

quoted

 For the syncmd and lsb-first properties, it could also

quoted

make sense to query them from the connected subdev at runtime, as
they're similar in purpose to formats and media bus configuration
(struct v4l2_mbus_config).

I consider this unlikely. I wouldn't spend time on that as this can always
be done later.

quoted

May I know in bit more detail about what you had in mind? Please
correct me if my understanding is wrong here but when I looked at the
code

1) mbus_config is part of subdev_video_ops only. I assume we don't want

to support this as part of tuner subdev. The next closest is pad_ops with
"struct v4l2_mbus_framefmt" but it is fully video specific config unless I
come up with new MEDIA_BUS_FMT_xxxx in media-bus-format.h and use the code
field? For e.g.

quoted

#define MEDIA_BUS_FMT_SDR_I2S_PADHI_BE       0x7001
#define MEDIA_BUS_FMT_SDR_I2S_PADHI_LE       0x7002

2) The framework does not seem to mandate pad ops for all subdev. As the

tuner can be any third party subdev, is it fair to assume that these
properties can be queried from subdev?

quoted

3) Assuming pad ops is not available on the subdev shouldn't we still

need a way to define these properties on DRIF DT?

quoted

quoted

I'm not an SDR expert, so I'd like to have your opinion on this.

Neither am I :-)

I think using the endpoint idea does make sense since this helps in
describing the routing. 

OK.

I am not sure what properties to put in there,

though.

Can you describe a bit more which properties belong to which syncmd mode?
That will help me make a decision.

OK. The documentation for this control is not accurate in many places of the h/w manual. My observations are based on tests with MAX2175 tuner master device using I2S mode.

Applicable parameters for both Frame Start Sync pulse & I2S modes
 - sync-active
 - lsb/msb first
 - dtdl
 
For I2S mode, syncdl property should be set to value zero only. 

Below are timing diagram examples.

Frame start sync pulse mode:
-----------------------------

   <------------ 1 frame ------------------>
   .---.   .---.   .---.   .---.   .---.   .---.   .---.
___|   |___|   |___|   |___|   |___|   |___|   |___|   |___    SCK

   .-------.                               .-------.
___|       |_______________________________|       |_______    SYNC/SS

   |                                       |
   |-No data delay          No sync delay  |
__  ______  ______  ______  ______  ______  ______  ______
  \/      \/      \/      \/      \/      \/      \/           Rx Data
__/\______/\______/\______/\______/\______/\______/\______

   |-1 bit delay            No sync delay  |
__________  ______  ______  ______  ______  ______  ______
          \/      \/      \/      \/      \/      \/           Rx Data
__________/\______/\______/\______/\______/\______/\______

1 bit delay => DTDL = 1 (i.e.) 1 clock cycle delay
No Sync delay => SYNCDL = 0

I2S mode:
----------

   <------------ 1 frame -------------------------->
   .---.   .---.   .---.   .---.   .---.   .---.   .---.
___|   |___|   |___|   |___|   |___|   |___|   |___|   |___    SCK


___                         _______________________
   |                       |                       |          SYNC/SS
   +-----------------------+                       +-------


   |                                               |
   |-No data delay                   No sync delay |
__  ______  ______  ______  ______  ______  ______  ______
  \/      \/      \/      \/      \/      \/      \/           Rx Data
__/\______/\______/\______/\______/\______/\______/\______

   |-1 bit delay                   No sync delay  |
__________  ______  ______  ______  ______  ______  ______
          \/      \/      \/      \/      \/      \/           Rx Data
__________/\______/\______/\______/\______/\______/\______

Please share your thoughts.

Thanks,
Ramesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html