Re: [PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

Re: [PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Olof Johansson]

Hi,

On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt

Please cc bindings for review to devicetree@vger.kernel.org (cc:d here now)

> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> new file mode 100644
> index 0000000..72da71d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -0,0 +1,24 @@
> +Device-Tree bindings for the NXP TDA998x HDMI transmitter
> +
> +Required properties;
> +  - compatible: must be "nxp,tda998x"
> +
> +Optional properties:
> +  - interrupts: interrupt number for HDMI exchanges - default: by polling

What are HDMI exchanges, and how do they differ from other interrupts?

> +
> +  - pinctrl-0: pin control group to be used for this controller (IRQ).
> +
> +  - pinctrl-names: must contain a "default" entry.
> +
> +  - video-ports: 24 bits value - default: <0x230145>

What is this?


-Olof

Re: [PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Jean-Francois Moine]

On Sun, 19 Jan 2014 20:06:09 -0800
Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:

> Hi,
> 
> On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> > Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> > ---
> >  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> 
> Please cc bindings for review to devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (cc:d here now)
> 
> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > new file mode 100644
> > index 0000000..72da71d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > @@ -0,0 +1,24 @@
> > +Device-Tree bindings for the NXP TDA998x HDMI transmitter
> > +
> > +Required properties;
> > +  - compatible: must be "nxp,tda998x"
> > +
> > +Optional properties:
> > +  - interrupts: interrupt number for HDMI exchanges - default: by polling
> 
> What are HDMI exchanges, and how do they differ from other interrupts?

The used HDMI interrupt events are screen plug/unplug and EDID read.
There are also CEC read/write which are not yet implemented in the
tda998x driver.

There is no difference from normal interrupts, except that the events
may be get by polling, so, the interrupt number is optional.

> > +
> > +  - pinctrl-0: pin control group to be used for this controller (IRQ).
> > +
> > +  - pinctrl-names: must contain a "default" entry.
> > +
> > +  - video-ports: 24 bits value - default: <0x230145>
> 
> What is this?

The video-ports value defines how the video controller is connected to
the tda998x chip. Each 4 bits value tells from which input pins comes
the video data and if there is any bit inversion. Each byte of this
video-ports is used to load the VIP_CNTRL_{0,1,2} registers. These ones
are described in the TDA9983B documentation which is the closer
available document about the TDA998x family.

The default value is the one defined for TI boards.
A known other value is <0x234501> which is used for Russell's Armada
DRM driver in the Cubox (Marvell A510), but this driver has no DT
support.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
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

Re: [PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Olof Johansson]

On Mon, Jan 20, 2014 at 1:54 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> On Sun, 19 Jan 2014 20:06:09 -0800
> Olof Johansson <olof@lixom.net> wrote:
>
>> Hi,
>>
>> On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
>> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>> > ---
>> >  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
>> >  1 file changed, 24 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>>
>> Please cc bindings for review to devicetree@vger.kernel.org (cc:d here now)
>>
>> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > new file mode 100644
>> > index 0000000..72da71d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > @@ -0,0 +1,24 @@
>> > +Device-Tree bindings for the NXP TDA998x HDMI transmitter
>> > +
>> > +Required properties;
>> > +  - compatible: must be "nxp,tda998x"
>> > +
>> > +Optional properties:
>> > +  - interrupts: interrupt number for HDMI exchanges - default: by polling
>>
>> What are HDMI exchanges, and how do they differ from other interrupts?
>
> The used HDMI interrupt events are screen plug/unplug and EDID read.
> There are also CEC read/write which are not yet implemented in the
> tda998x driver.
>
> There is no difference from normal interrupts, except that the events
> may be get by polling, so, the interrupt number is optional.]

Ok, then it looks like the property description is a little confusing.
I'd remove the mentioning of HDMI exchanges from it.

>> > +
>> > +  - pinctrl-0: pin control group to be used for this controller (IRQ).
>> > +
>> > +  - pinctrl-names: must contain a "default" entry.
>> > +
>> > +  - video-ports: 24 bits value - default: <0x230145>
>>
>> What is this?
>
> The video-ports value defines how the video controller is connected to
> the tda998x chip. Each 4 bits value tells from which input pins comes
> the video data and if there is any bit inversion. Each byte of this
> video-ports is used to load the VIP_CNTRL_{0,1,2} registers. These ones
> are described in the TDA9983B documentation which is the closer
> available document about the TDA998x family.
>
> The default value is the one defined for TI boards.
> A known other value is <0x234501> which is used for Russell's Armada
> DRM driver in the Cubox (Marvell A510), but this driver has no DT
> support.

Ok, this is a classic case where the binding should describe how
things are configured/wired up instead of hardcoding a register value
like this. From the data sheet there seems to be a _lot_ of settings,
so selecting the needed subset for now might be acceptable (and go
with defaults on the rest).


-Olof

Re: [PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 10:17:56AM -0800, Olof Johansson wrote:
> Ok, this is a classic case where the binding should describe how
> things are configured/wired up instead of hardcoding a register value
> like this. From the data sheet there seems to be a _lot_ of settings,
> so selecting the needed subset for now might be acceptable (and go
> with defaults on the rest).

The issue is not that simple, because the register is kind of backwards.

Rather than describing what is on each nibble of the input bus, you
specify how each nibble of the internal bus maps to the external world.
The internal bus is mostly fixed as to which nibbles are red, green and
blue (there is internal support for swapping red/blue on the internal
bus, but we don't use that - it just adds extra unnecessary headaches
to the mapping.)

The way I ended up describing it in platform data was via the individual
nibble settings for the register:

        .swap_a = 2,
        .swap_b = 3,
        .swap_c = 4,
        .swap_d = 5,
        .swap_e = 0,
        .swap_f = 1,

which gives you a way to refer back to the data sheet.  There's also the
control whether the nibble is bit-big-endian or bit-little-endian.

So basically, swap_a controls which nibble of the input bus is mapped to
the internal bus bits 23:20, and mirror_a whether it's bit 0123 -> 0123
or 0123 -> 3210.

This means it's possible to tell the chip that each nibble of the interal
bus comes from the same four pins!

If we want to describe what each nibble set on the input pins does, it's
going to make for some complex code to convert that from "forward" notation
(which we'll have to invent) to the "reverse" notation that the chip needs.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".