[v2,12/28] drm/i2c: tda998x: add DT support

This patch adds DT support to the tda998x.

As a side effect, now, the audio sample rate is always 48kHz and the
audio clock is always set.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt | 16 ++++++
 drivers/gpu/drm/i2c/tda998x_drv.c           | 66 ++++++++++++++++------
 2 file changed, 66 insertions(+), 16 deletions(-)

On Thu, Jan 09, 2014 at 12:03:24PM +0100, Jean-Francois Moine wrote:
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 2ba0355..b87802f 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -28,17 +28,22 @@
>  
>  #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
>  
> +#define AUDIO_SAMPLE 48		/* 48 kHz */

Horrid definition.  It says nothing about it's units, and given that
things like 44.1kHz are common place, should _not_ be kHz.

> +
>  struct tda998x_priv {
>  	struct i2c_client *cec;
>  	struct i2c_client *hdmi;
>  	uint16_t rev;
>  	uint8_t current_page;
> -	int dpms;
> +	u8 dpms;

A 'dpms' is defined in the DRM interfaces as an 'int' and should remain
as such.

> @@ -586,17 +591,17 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
>  
>  static void
>  tda998x_configure_audio(struct tda998x_priv *priv,
> -		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
> +		struct drm_display_mode *mode)
>  {
>  	uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
>  	uint32_t n;
>  
>  	/* Enable audio ports */
> -	reg_write(priv, REG_ENA_AP, p->audio_cfg);
> -	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
> +	reg_write(priv, REG_ENA_AP, priv->audio_port);
> +	reg_write(priv, REG_ENA_ACLK, 0x01);	/* enable clock */

This is a change of configuration for SPDIF.  SPDIF _can_ have an external
clock, or the TDA998x can recover the clock itself.  Enabling the clock
unconditionally is probably the wrong thing to do.

>  
>  	/* Set audio input source */
> -	switch (p->audio_format) {
> +	switch (priv->audio_type) {
>  	case AFMT_SPDIF:
>  		reg_write(priv, REG_MUX_AP, 0x40);
>  		clksel_aip = AIP_CLKSEL_AIP(0);
> @@ -644,7 +649,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	 * This is the approximate value of N, which happens to be
>  	 * the recommended values for non-coherent clocks.
>  	 */
> -	n = 128 * p->audio_sample_rate / 1000;
> +	n = 128 * AUDIO_SAMPLE;		/* acr_n = 128 * sample_rate / 1000 */

If you keep the sample rate in Hz, you don't need the comment.

>  
>  	/* Write the CTS and N values */
>  	buf[0] = 0x44;
> @@ -674,7 +679,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	tda998x_audio_mute(priv, false);
>  
>  	/* Write the audio information packet */
> -	tda998x_write_aif(priv, p);
> +	tda998x_write_aif(priv);
>  }
>  
>  /* DRM encoder functions */
> @@ -698,7 +703,13 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>  			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
>  			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
>  
> -	priv->params = *p;
> +	memcpy(priv->audio_frame, p->audio_frame,
> +			sizeof priv->audio_frame);
> +
> +	if (p->audio_cfg) {
> +		priv->audio_port = p->audio_cfg;
> +		priv->audio_type = p->audio_format;
> +	}

Does this really make things better?

> @@ -1157,6 +1168,8 @@ tda998x_encoder_init(struct i2c_client *client,
>  		    struct drm_encoder_slave *encoder_slave)
>  {
>  	struct tda998x_priv *priv;
> +	struct device_node *np = client->dev.of_node;
> +	u32 video;
>  	int ret;
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -1166,6 +1179,7 @@ tda998x_encoder_init(struct i2c_client *client,
>  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
>  	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
>  	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
> +	priv->audio_frame[1] = 1;		/* channels - 1 */
>  
>  	priv->current_page = 0xff;
>  	priv->hdmi = client;
> @@ -1225,6 +1239,17 @@ tda998x_encoder_init(struct i2c_client *client,
>  	cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
>  			CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
>  
> +	if (!np)
> +		return 0;		/* non-DT */
> +
> +	/* get the optional video properties */
> +	ret = of_property_read_u32(np, "video-ports", &video);
> +	if (ret == 0) {
> +		priv->vip_cntrl_0 = video >> 16;
> +		priv->vip_cntrl_1 = video >> 8;
> +		priv->vip_cntrl_2 = video;
> +	}

The creation of new DT bindings requires that the bindings are documented
in Documentation/devicetree/bindings, and this is done as a separate patch
to be submitted to the device tree maintainers for review, and this must
be reviewed before the corresponding DT code can be accepted into the
kernel.  Please create such a patch and submit it for their review.

Thanks.

On Thu, Jan 09, 2014 at 12:03:24PM +0100, Jean-Francois Moine wrote:
> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> new file mode 100644
> index 0000000..f07339e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -0,0 +1,16 @@
> +Device-Tree bindings for the NXP TDA998x HDMI transmitter
> +
> +Required properties;
> +  - compatible: must be "nxp,tda998x"
> +
> +Optional properties:
> +  - video-ports: 24 bits value - default: <0x230145>
> +
> +Example:
> +
> +	tda998x: hdmi-encoder {
> +		compatible = "nxp,tda998x";
> +		reg = <0x70>;
> +		pinctrl-0 = <&pmx_camera>;
> +		pinctrl-names = "default";

This example seems to be meaningless.  What SoC pinctrl settings are there
on this device?  What has a HDMI encoder got to do with a camera?

On Thu, Jan 09, 2014 at 12:03:24PM +0100, Jean-Francois Moine wrote:
> This patch adds DT support to the tda998x.
> 
> As a side effect, now, the audio sample rate is always 48kHz and the
> audio clock is always set.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

This patch breaks audio through the renaming of variable names.  When you
do such changes, *never* change all three together.  Do them one at a time,
replacing all instances of one variable before moving on to the next one.
This avoids getting the names muddled up.

> -	struct tda998x_encoder_params params;
> +
> +	u8 audio_type;		/* audio type */
> +	u8 audio_frame[6];
> +	u32 audio_port;

"audio type" is a pointless comment for a variable called "audio_type".
Explain what it is.  It's the input format, which may be SPDIF or I2S.

>  	/* Set audio input source */
> -	switch (p->audio_format) {
> +	switch (priv->audio_type) {

So, "audio_format" has become "audio_type" here.

> @@ -698,7 +703,13 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>  			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
>  			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
>  
> -	priv->params = *p;
> +	memcpy(priv->audio_frame, p->audio_frame,
> +			sizeof priv->audio_frame);
> +
> +	if (p->audio_cfg) {
> +		priv->audio_port = p->audio_cfg;
> +		priv->audio_type = p->audio_format;
> +	}

Here, audio_port and audio_type are only written to if audio_cfg is
non-zero, which is not quite what is intended here.  If you want DT
to override this, then make that explicit.

Moreover, we can see that this confirms that "audio_format" becomes
"audio_type", and "audio_cfg" becomes "audio_port".

> @@ -947,8 +958,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  
>  		tda998x_write_avi(priv, mode);
>  
> -		if (priv->params.audio_cfg)
> -			tda998x_configure_audio(priv, mode, &priv->params);
> +		if (priv->audio_type)
> +			tda998x_configure_audio(priv, mode);

And here we have the real bug.  "audio_cfg" has become "audio_type",
which in the case of SPDIF, is zero.  Hence, with a SPDIF source,
tda998x_configure_audio() is no longer called.