Re: [PATCH 2/4] media: dt-bindings: Add bindings for TDA1997X

[Tim Harvey]

On Wed, Oct 4, 2017 at 2:14 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
> On Fri, Sep 22, 2017 at 03:24:11PM -0700, Tim Harvey wrote:
>> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
>

Hi Rob, thanks for the review!

I will add a commit message, vendor prefix to non-standard props, and
remove '_' in the next revision.

>
>> +
>> +Optional Properties:
>> + - max-pixel-rate  : Maximum pixel rate supported by the SoC (MP/sec)
>> + - audio-port      : parameters defining audio output port connection
>
> That description is meaningless to me.
>
<snip>>
>> +The Audio output port consists of A_CLK, A_WS, AP0, AP1, AP2, and AP3 pins
>> +and can support up to 8-chanenl audio using the following audio bus DAI formats:
>> + - I2S16
>> + - I2S32
>> + - SPDIF
>> + - OBA (One-Bit-Audio)
>> + - I2S16_HBR_STRAIGHT (High Bitrate straight through)
>> + - I2S16_HBR_DEMUX (High Bitrate demuxed)
>> + - I2S32_HBR_DEMUX (High Bitrate demuxed)
>> + - DST (Direct Stream Transfer)
>
> This either should be a standard, common property or not be in DT.
> Practically every system is going to have at least one end of the
> connection that is configurable. The kernel should be able to get lists
> of supported modes and pick one.
>

I struggled with 'audio-port' property quite a bit.

Looking at the datasheet a bit closer
(http://dev.gateworks.com/datasheets/TDA19971-datasheet-rev3.pdf) I
see that the audio output port consisting of WS, AP[0:3], CLK really
only supports I2S and S/PDIF output modes. The AP[0:3] provide support
for up to 8 audio channels. The way these pins are used is defined in
Table 5/6/7 in the datasheet but I think that the dt perhaps only
needs to number of data lines (1-4) (which could also be represented
as channels (1-8)), and a clock multiplier which could be described as
a 'mclk-fs' multiplication factor like simple-audio bindings does.

What would your recommendation be here?

>> +
<snip>
>> +             max-pixel-rate = <180>; /* IMX6 CSI max pixel rate 180MP/sec */
>
> That's a constraint that belongs in the i.MX CSI node or driver.

right - that makes sense. I'll talk to the maintainer of the i.MX CSI
driver to see what they think.

>
>> +
>> +             port@0 {
>> +                     reg = <0>;
>> +             };
>> +             port@1 {
>
> You need to describe how many ports and what they are.

ok. My example was a mistake anyway and I propose a single output port such as:

The device node must contain one 'port' child node for its digital output
video port, in accordance with the video interface bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt.

Example:
                port {
                        tda1997x_to_ipu1_csi0_mux: endpoint {
                                remote-endpoint =
<&ipu1_csi0_mux_from_parallel_sensor>;
                                bus-width = <16>;
                                hsync-active = <1>;
                                vsync-active = <1>;
                                data-active = <1>;
                        };
                };

>
>> +                     reg = <1>;
>> +                     hdmi_in: endpoint {
>> +                             remote-endpoint = <&ccdc_in>;
>> +                     };
>> +             };
>> +     };
>> + - VP[15:8] connected to IMX6 CSI_DATA[19:12] for 8bit BT656
>> +   16bit I2S layout0 with a 128*fs clock (A_WS, AP0, A_CLK pins)
>
> Do you really need 2 examples? It should be possible to figure out other
> configurations with better descriptions above.
>

Perhaps, but I feel like the vidout-portcfg is pretty confusing and
wanted to provide some different mappings as examples. The
vidout-portcfg property allows you to shift the bits around on the
output pins per 4-bit groups as well as reverse their order and
different defines are used depending on RGB444/YUV444/YUV422/BT656. I
could provide just portcfg examples above the full dts example though
under the short description of 'nxp,vidout-portcfg'.

Regards,

Tim