linux-kernel - Re: [PATCH v2 12/28] drm/i2c: tda998x: add DT support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140111173536.GY15937@n2100.arm.linux.org.uk>
Date:	Sat, 11 Jan 2014 17:35:37 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Jean-Francois Moine <moinejf@...e.fr>
Cc:	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
	Rob Clark <robdclark@...il.com>,
	Dave Airlie <airlied@...il.com>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 12/28] drm/i2c: tda998x: add DT support

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.

-- 
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".
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/