[PATCH 0/3] drm/i2c: tda998x ASoC hdmi-codec support + BeagleBoneBlack audio support

[PATCH 0/3] drm/i2c: tda998x ASoC hdmi-codec support + BeagleBoneBlack audio support

[Jyri Sarha]

These patches are just a rebase of the same patches from my "[PATCH
v4 0/7] Implement generic ASoC HDMI codec and use it in tda998x"
-patch series. I have only made updates required by efc9194bcff8
("ASoC: hdmi-codec: callback function will be called with private
data") -patch.

The first patch changes tda998x pdata, so if there are any out of tree
users of tda998x-driver the out of tree code needs to be updated.

Jyri Sarha (3):
  drm/i2c: tda998x: Improve tda998x_configure_audio() audio related
    pdata
  drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding
  ARM: dts: am335x-boneblack: Add HDMI audio support

 .../devicetree/bindings/display/bridge/tda998x.txt |  18 ++
 arch/arm/boot/dts/am335x-boneblack.dts             |  71 +++++-
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 276 ++++++++++++++++++---
 include/drm/i2c/tda998x.h                          |  24 +-
 include/dt-bindings/display/tda998x.h              |   7 +
 6 files changed, 342 insertions(+), 55 deletions(-)
 create mode 100644 include/dt-bindings/display/tda998x.h

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Jyri Sarha]

Define struct tda998x_audio_params in include/drm/i2c/tda998x.h and
use it in pdata and for tda998x_configure_audio() parameters. Also
updates tda998x_write_aif() to take struct hdmi_audio_infoframe *
directly as a parameter.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 77 ++++++++++++++++++++-------------------
 include/drm/i2c/tda998x.h         | 24 +++++++-----
 2 files changed, 53 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f4315bc..f97b748 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -41,7 +41,7 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	struct tda998x_encoder_params params;
+	struct tda998x_audio_params audio_params;
 
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
@@ -666,26 +666,16 @@ tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 	reg_set(priv, REG_DIP_IF_FLAGS, bit);
 }
 
-static void
-tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
+static int tda998x_write_aif(struct tda998x_priv *priv,
+			     struct hdmi_audio_infoframe *cea)
 {
 	union hdmi_infoframe frame;
 
-	hdmi_audio_infoframe_init(&frame.audio);
-
-	frame.audio.channels = p->audio_frame[1] & 0x07;
-	frame.audio.channel_allocation = p->audio_frame[4];
-	frame.audio.level_shift_value = (p->audio_frame[5] & 0x78) >> 3;
-	frame.audio.downmix_inhibit = (p->audio_frame[5] & 0x80) >> 7;
-
-	/*
-	 * L-PCM and IEC61937 compressed audio shall always set sample
-	 * frequency to "refer to stream".  For others, see the HDMI
-	 * specification.
-	 */
-	frame.audio.sample_frequency = (p->audio_frame[2] & 0x1c) >> 2;
+	frame.audio = *cea;
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, &frame);
+
+	return 0;
 }
 
 static void
@@ -710,20 +700,21 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 	}
 }
 
-static void
+static int
 tda998x_configure_audio(struct tda998x_priv *priv,
-		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+			struct tda998x_audio_params *params,
+			unsigned mode_clock)
 {
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 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, params->config);
 
 	/* Set audio input source */
-	switch (p->audio_format) {
+	switch (params->format) {
 	case AFMT_SPDIF:
+		reg_write(priv, REG_ENA_ACLK, 0);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
@@ -731,15 +722,29 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		break;
 
 	case AFMT_I2S:
+		reg_write(priv, REG_ENA_ACLK, 1);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
-		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		switch (params->sample_width) {
+		case 16:
+			cts_n = CTS_N_M(3) | CTS_N_K(1);
+			break;
+		case 18:
+		case 20:
+		case 24:
+			cts_n = CTS_N_M(3) | CTS_N_K(2);
+			break;
+		default:
+		case 32:
+			cts_n = CTS_N_M(3) | CTS_N_K(3);
+			break;
+		}
 		break;
 
 	default:
 		BUG();
-		return;
+		return -EINVAL;
 	}
 
 	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
@@ -755,11 +760,11 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (mode->clock > 100000)
+	if (mode_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
-	if (p->audio_format == AFMT_SPDIF)
+	if (params->format == AFMT_SPDIF)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 or _32 */
 
 	reg_write(priv, REG_AUDIO_DIV, adiv);
@@ -768,7 +773,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 * params->sample_rate / 1000;
 
 	/* Write the CTS and N values */
 	buf[0] = 0x44;
@@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
 
 	/* Write the channel status */
-	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
-	buf[1] = 0x00;
-	buf[2] = IEC958_AES3_CON_FS_NOTID;
-	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
-			IEC958_AES4_CON_MAX_WORDLEN_24;
-	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
+	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);
 
 	tda998x_audio_mute(priv, true);
 	msleep(20);
 	tda998x_audio_mute(priv, false);
 
-	/* Write the audio information packet */
-	tda998x_write_aif(priv, p);
+	return tda998x_write_aif(priv, &params->cea);
 }
 
 /* DRM encoder functions */
@@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
-	priv->params = *p;
+	priv->audio_params = p->audio;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1057,9 +1056,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+		if (priv->audio_params.format != AFMT_UNUSED) {
+			tda998x_configure_audio(priv,
+						&priv->audio_params,
+						adjusted_mode->clock);
+		}
 	}
 }
 
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3e419d9..24be7aa 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -1,6 +1,19 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+#define AFMT_UNUSED	0
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2
+
+struct tda998x_audio_params {
+	u8 config;
+	u8 format;
+	unsigned sample_width;
+	unsigned sample_rate;
+	struct hdmi_audio_infoframe cea;
+	u8 status[4];
+};
+
 struct tda998x_encoder_params {
 	u8 swap_b:3;
 	u8 mirr_b:1;
@@ -15,16 +28,7 @@ struct tda998x_encoder_params {
 	u8 swap_e:3;
 	u8 mirr_e:1;
 
-	u8 audio_cfg;
-	u8 audio_clk_cfg;
-	u8 audio_frame[6];
-
-	enum {
-		AFMT_SPDIF,
-		AFMT_I2S
-	} audio_format;
-
-	unsigned audio_sample_rate;
+	struct tda998x_audio_params audio;
 };
 
 #endif
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Jyri Sarha]

Register ASoC HDMI codec for audio functionality and adds device tree
binding for audio configuration.

With the registered HDMI codec the tda998x node can be used like a
regular codec node in ASoC card configurations. HDMI audio info-frame
and audio stream header is generated by the ASoC HDMI codec. The codec
also applies constraints for available sample-rates based on Edid Like
Data from the display. The device tree binding document has been
updated [1].

Part of this patch has been inspired by Jean Francoise's "drm/i2c: tda998x:
Add support of a DT graph of ports"-patch [2]. There may still be some
identical lines left from the original patch and some of the ideas
have come from there.

[1] Documentation/devicetree/bindings/display/bridge/tda998x.txt
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/095255.html

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../devicetree/bindings/display/bridge/tda998x.txt |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 199 ++++++++++++++++++++-
 include/drm/i2c/tda998x.h                          |   4 +-
 include/dt-bindings/display/tda998x.h              |   7 +
 5 files changed, 224 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/display/tda998x.h

diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
index e178e6b..24cc246 100644
--- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
@@ -21,8 +21,19 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+  - audio-ports: array of 8-bit values, 2 values per one DAI[1].
+	The first value defines the DAI type: TDA998x_SPDIF or TDA998x_I2S[2].
+	The second value defines the tda998x AP_ENA reg content when the DAI
+	in question is used. The implementation allows one or two DAIs. If two
+	DAIs are defined, they must be of different type.
+
+[1] Documentation/sound/alsa/soc/DAI.txt
+[2] include/dt-bindings/display/tda998x.h
+
 Example:
 
+#include <dt-bindings/display/tda998x.h>
+
 	tda998x: hdmi-encoder {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;
@@ -30,4 +41,11 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+		video-ports = <0x230145>;
+
+		#sound-dai-cells = <2>;
+			     /*	DAI-format	AP_ENA reg value */
+		audio-ports = <	TDA998x_SPDIF	0x04
+				TDA998x_I2S	0x03>;
+
 	};
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 4d341db..a6c92be 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -22,6 +22,7 @@ config DRM_I2C_SIL164
 config DRM_I2C_NXP_TDA998X
 	tristate "NXP Semiconductors TDA998X HDMI encoder"
 	default m if DRM_TILCDC
+	select SND_SOC_HDMI_CODEC if SND_SOC
 	help
 	  Support for NXP Semiconductors TDA998X HDMI encoders.
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f97b748..bd720cc 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <sound/hdmi-codec.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -30,6 +31,11 @@
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
+struct tda998x_audio_port {
+	u8 format;		/* AFMT_xxx */
+	u8 config;		/* AP value */
+};
+
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
@@ -43,6 +49,8 @@ struct tda998x_priv {
 	u8 vip_cntrl_2;
 	struct tda998x_audio_params audio_params;
 
+	struct platform_device *audio_pdev;
+
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 
@@ -53,6 +61,8 @@ struct tda998x_priv {
 
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+
+	struct tda998x_audio_port audio_port[2];
 };
 
 #define conn_to_tda998x_priv(x) \
@@ -743,7 +753,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		break;
 
 	default:
-		BUG();
+		dev_err(&priv->hdmi->dev, "Unsupported I2S format\n");
 		return -EINVAL;
 	}
 
@@ -1160,6 +1170,8 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
 	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
+	drm_edid_to_eld(connector, edid);
+
 	kfree(edid);
 
 	return n;
@@ -1181,6 +1193,9 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
 	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
+	if (priv->audio_pdev)
+		platform_device_unregister(priv->audio_pdev);
+
 	if (priv->hdmi->irq)
 		free_irq(priv->hdmi->irq, priv);
 
@@ -1190,8 +1205,180 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	i2c_unregister_device(priv->cec);
 }
 
+static int tda998x_audio_hw_params(struct device *dev, void *data,
+				   struct hdmi_codec_daifmt *daifmt,
+				   struct hdmi_codec_params *params)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	int i, ret;
+	struct tda998x_audio_params audio = {
+		.sample_width = params->sample_width,
+		.sample_rate = params->sample_rate,
+		.cea = params->cea,
+	};
+
+	if (!priv->encoder.crtc)
+		return -ENODEV;
+
+	memcpy(audio.status, params->iec.status,
+	       min(sizeof(audio.status), sizeof(params->iec.status)));
+
+	switch (daifmt->fmt) {
+	case HDMI_I2S:
+		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
+			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+				daifmt->bit_clk_master,
+				daifmt->frame_clk_master);
+			return -EINVAL;
+		}
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_I2S)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_I2S;
+		break;
+	case HDMI_SPDIF:
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_SPDIF)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_SPDIF;
+		break;
+	default:
+		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
+		return -EINVAL;
+	}
+
+	if (audio.config == 0) {
+		dev_err(dev, "%s: No audio configutation found\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = tda998x_configure_audio(priv,
+				      &audio,
+				      priv->encoder.crtc->hwmode.clock);
+
+	if (ret == 0)
+		priv->audio_params = audio;
+
+	return ret;
+}
+
+static void tda998x_audio_shutdown(struct device *dev, void *data)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	reg_write(priv, REG_ENA_AP, 0);
+
+	priv->audio_params.format = AFMT_UNUSED;
+}
+
+int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	tda998x_audio_mute(priv, enable);
+
+	return 0;
+}
+
+static int tda998x_audio_get_eld(struct device *dev, void *data, 
+				 uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;
+
+	mutex_lock(&config->mutex);
+	list_for_each_entry(connector, &config->connector_list, head) {
+		if (&priv->encoder == connector->encoder) {
+			memcpy(buf, connector->eld,
+			       min(sizeof(connector->eld), len));
+			ret = 0;
+		}
+	}
+	mutex_unlock(&config->mutex);
+
+	return ret;
+}
+
+static const struct hdmi_codec_ops audio_codec_ops = {
+	.hw_params = tda998x_audio_hw_params,
+	.audio_shutdown = tda998x_audio_shutdown,
+	.digital_mute = tda998x_audio_digital_mute,
+	.get_eld = tda998x_audio_get_eld,
+};
+
+static int tda998x_audio_codec_init(struct tda998x_priv *priv,
+				    struct device *dev)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &audio_codec_ops,
+		.max_i2s_channels = 2,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
+		if (priv->audio_port[i].format == AFMT_I2S &&
+		    priv->audio_port[i].config != 0)
+			codec_data.i2s = 1;
+		if (priv->audio_port[i].format == AFMT_SPDIF &&
+		    priv->audio_port[i].config != 0)
+			codec_data.spdif = 1;
+	}
+
+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	return PTR_ERR_OR_ZERO(priv->audio_pdev);
+}
+
 /* I2C driver functions */
 
+static int tda998x_get_audio_ports(struct tda998x_priv *priv,
+				   struct device_node *np)
+{
+	const u32 *port_data;
+	u32 size;
+	int i;
+
+	port_data = of_get_property(np, "audio-ports", &size);
+	if (!port_data)
+		return 0;
+
+	size /= sizeof(u32);
+	if (size > 2 * ARRAY_SIZE(priv->audio_port) || size % 2 != 0) {
+		dev_err(&priv->hdmi->dev,
+			"Bad number of elements in audio-ports dt-property\n");
+		return -EINVAL;
+	}
+
+	size /= 2;
+
+	for (i = 0; i < size; i++) {
+		u8 afmt = be32_to_cpup(&port_data[2*i]);
+		u8 ena_ap = be32_to_cpup(&port_data[2*i+1]);
+
+		if (afmt != AFMT_SPDIF && afmt != AFMT_I2S) {
+			dev_err(&priv->hdmi->dev,
+				"Bad audio format %u\n", afmt);
+			return -EINVAL;
+		}
+
+		priv->audio_port[i].format = afmt;
+		priv->audio_port[i].config = ena_ap;
+	}
+
+	if (priv->audio_port[0].format == priv->audio_port[1].format) {
+		dev_err(&priv->hdmi->dev,
+			"There can only be on I2S port and one SPDIF port\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
@@ -1305,7 +1492,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	if (!np)
 		return 0;		/* non-DT */
 
-	/* get the optional video properties */
+	/* get the device tree parameters */
 	ret = of_property_read_u32(np, "video-ports", &video);
 	if (ret == 0) {
 		priv->vip_cntrl_0 = video >> 16;
@@ -1313,8 +1500,14 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 		priv->vip_cntrl_2 = video;
 	}
 
-	return 0;
+	ret = tda998x_get_audio_ports(priv, np);
+	if (ret)
+		goto fail;
 
+	if (priv->audio_port[0].format != AFMT_UNUSED)
+		tda998x_audio_codec_init(priv, &client->dev);
+
+	return 0;
 fail:
 	/* if encoder_init fails, the encoder slave is never registered,
 	 * so cleanup here:
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 24be7aa..b575332 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -1,9 +1,9 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+#include <dt-bindings/display/tda998x.h>
+
 #define AFMT_UNUSED	0
-#define AFMT_SPDIF	1
-#define AFMT_I2S	2
 
 struct tda998x_audio_params {
 	u8 config;
diff --git a/include/dt-bindings/display/tda998x.h b/include/dt-bindings/display/tda998x.h
new file mode 100644
index 0000000..38ef741
--- /dev/null
+++ b/include/dt-bindings/display/tda998x.h
@@ -0,0 +1,7 @@
+#ifndef _DT_BINDINGS_TDA998X_H
+#define _DT_BINDINGS_TDA998X_H
+
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2
+
+#endif /*_DT_BINDINGS_TDA998X_H */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] ARM: dts: am335x-boneblack: Add HDMI audio support

[Jyri Sarha]

Add HDMI audio support. Adds mcasp0_pins, clk_mcasp0_fixed,
clk_mcasp0, mcasp0, sound node, and updates the tda19988 node to
follow the new binding.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 arch/arm/boot/dts/am335x-boneblack.dts | 71 ++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index 55c0e95..2bae4d1 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -9,6 +9,7 @@
 
 #include "am33xx.dtsi"
 #include "am335x-bone-common.dtsi"
+#include <dt-bindings/display/tda998x.h>
 
 / {
 	model = "TI AM335x BeagleBone Black";
@@ -64,6 +65,16 @@
 			AM33XX_IOPAD(0x9b0, PIN_OUTPUT_PULLDOWN | MUX_MODE3)	/* xdma_event_intr0 */
 		>;
 	};
+
+	mcasp0_pins: mcasp0_pins {
+		pinctrl-single,pins = <
+			AM33XX_IOPAD(0x9ac, PIN_INPUT_PULLUP | MUX_MODE0) /* mcasp0_ahcklx.mcasp0_ahclkx */
+			AM33XX_IOPAD(0x99c, PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2*/
+			AM33XX_IOPAD(0x994, PIN_OUTPUT_PULLUP | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */
+			AM33XX_IOPAD(0x990, PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */
+			AM33XX_IOPAD(0x86c, PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */
+		>;
+	};
 };
 
 &lcdc {
@@ -76,16 +87,22 @@
 };
 
 &i2c0 {
-	tda19988 {
+	tda19988: tda19988 {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;
+
 		pinctrl-names = "default", "off";
 		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
 		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
 
-		port {
-			hdmi_0: endpoint@0 {
-				remote-endpoint = <&lcdc_0>;
+		#sound-dai-cells = <0>;
+		audio-ports = <	AFMT_I2S	0x03>;
+
+		ports {
+			port@0 {
+				hdmi_0: endpoint@0 {
+					remote-endpoint = <&lcdc_0>;
+				};
 			};
 		};
 	};
@@ -94,3 +111,49 @@
 &rtc {
 	system-power-controller;
 };
+
+&mcasp0	{
+	#sound-dai-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcasp0_pins>;
+	status = "okay";
+	op-mode = <0>;	/* MCASP_IIS_MODE */
+	tdm-slots = <2>;
+	serial-dir = <	/* 0: INACTIVE, 1: TX, 2: RX */
+			0 0 1 0
+		>;
+	tx-num-evt = <32>;
+	rx-num-evt = <32>;
+};
+
+/ {
+	clk_mcasp0_fixed: clk_mcasp0_fixed {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <24576000>;
+	};
+
+	clk_mcasp0: clk_mcasp0 {
+		#clock-cells = <0>;
+		compatible = "gpio-gate-clock";
+		clocks = <&clk_mcasp0_fixed>;
+		enable-gpios = <&gpio1 27 0>; /* BeagleBone Black Clk enable on GPIO1_27 */
+	};
+
+	sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,name = "TI BeagleBone Black";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,bitclock-master = <&dailink0_master>;
+		simple-audio-card,frame-master = <&dailink0_master>;
+
+		dailink0_master: simple-audio-card,cpu {
+			sound-dai = <&mcasp0>;
+			clocks = <&clk_mcasp0>;
+		};
+
+		simple-audio-card,codec {
+			sound-dai = <&tda19988>;
+		};
+	};
+};
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Russell King - ARM Linux]

On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
>  
>  	/* Write the channel status */
> -	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
> -	buf[1] = 0x00;
> -	buf[2] = IEC958_AES3_CON_FS_NOTID;
> -	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
> -			IEC958_AES4_CON_MAX_WORDLEN_24;
> -	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
> +	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);

Take a close look at the code you are replacing here - the buffer
contents is not AES bytes 0, 1, 2, 3, 4, but AES bytes 0, 1, 3, 4
- byte 2 is not present in this array.  I don't think you've noticed
this as your second patch merely copies verboten the IEC status:

+       memcpy(audio.status, params->iec.status,
+              min(sizeof(audio.status), sizeof(params->iec.status)));

assuming that it is bytes 0-3.  Byte 2 is stored separately for each
I2S channel in the following registers.

> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 3e419d9..24be7aa 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -1,6 +1,19 @@
>  #ifndef __DRM_I2C_TDA998X_H__
>  #define __DRM_I2C_TDA998X_H__
>  
> +#define AFMT_UNUSED	0
> +#define AFMT_SPDIF	1
> +#define AFMT_I2S	2

I'd prefer this to stay an enum please.

> +struct tda998x_audio_params {
> +	u8 config;
> +	u8 format;
> +	unsigned sample_width;
> +	unsigned sample_rate;
> +	struct hdmi_audio_infoframe cea;

With this addition, this file will need to include linux/hdmi.h.

> +	u8 status[4];

A comment here about the missing byte 2 would probably be a good idea.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Russell King - ARM Linux]

On Tue, Aug 02, 2016 at 03:05:08PM +0300, Jyri Sarha wrote:
> +	memcpy(audio.status, params->iec.status,
> +	       min(sizeof(audio.status), sizeof(params->iec.status)));

As mentioned in the other patch, the audio status does not directly
correspond with the AES bytes, so this ends up causing the driver to
write the wrong bytes to the wrong registers.

> +	ret = tda998x_configure_audio(priv,
> +				      &audio,
> +				      priv->encoder.crtc->hwmode.clock);

What happens if audio changes at the same time that the video mode
changes?  What protection is there against two threads concurrently
executing tda998x_configure_audio() ?

> +	priv->audio_pdev = platform_device_register_data(
> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
> +		&codec_data, sizeof(codec_data));

I'd much prefer that this was a child of the I2C device, which will
show the relationship between this virtual platform device and the
device which it's associated with.  That can be done via
platform_device_register_full().

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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 1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Jyri Sarha]

On 08/04/16 16:31, Russell King - ARM Linux wrote:
> On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
>> @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>>  	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
>>  
>>  	/* Write the channel status */
>> -	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
>> -	buf[1] = 0x00;
>> -	buf[2] = IEC958_AES3_CON_FS_NOTID;
>> -	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
>> -			IEC958_AES4_CON_MAX_WORDLEN_24;
>> -	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
>> +	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);
> 
> Take a close look at the code you are replacing here - the buffer
> contents is not AES bytes 0, 1, 2, 3, 4, but AES bytes 0, 1, 3, 4
> - byte 2 is not present in this array.  I don't think you've noticed
> this as your second patch merely copies verboten the IEC status:
> 
> +       memcpy(audio.status, params->iec.status,
> +              min(sizeof(audio.status), sizeof(params->iec.status)));
> 
> assuming that it is bytes 0-3.  Byte 2 is stored separately for each
> I2S channel in the following registers.
> 

Oh, yes. I missed that. I think it would be better to increase
tda998x_audio_params status buffer length by one and keep it otherwise
as it is. Then just write the bytes 0-1 and 3-4 separately into channel
status registers, with appropriate comments. I guess the the AES byte 2
can remain 0 for now (source and channel unspecified, with assumption of
consumer stream).

>> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
>> index 3e419d9..24be7aa 100644
>> --- a/include/drm/i2c/tda998x.h
>> +++ b/include/drm/i2c/tda998x.h
>> @@ -1,6 +1,19 @@
>>  #ifndef __DRM_I2C_TDA998X_H__
>>  #define __DRM_I2C_TDA998X_H__
>>  
>> +#define AFMT_UNUSED	0
>> +#define AFMT_SPDIF	1
>> +#define AFMT_I2S	2
> 
> I'd prefer this to stay an enum please.
> 

Ok.

>> +struct tda998x_audio_params {
>> +	u8 config;
>> +	u8 format;
>> +	unsigned sample_width;
>> +	unsigned sample_rate;
>> +	struct hdmi_audio_infoframe cea;
> 
> With this addition, this file will need to include linux/hdmi.h.
> 

Oh yes, thanks.

>> +	u8 status[4];
> 
> A comment here about the missing byte 2 would probably be a good idea.
> 

As I said earlier, I'd rather keep this as plain channel status buffer
and just increase its size by one.

Thanks a lot for the review!

Best reagards,
Jyri

Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Jyri Sarha]

On 08/04/16 17:07, Russell King - ARM Linux wrote:
> On Tue, Aug 02, 2016 at 03:05:08PM +0300, Jyri Sarha wrote:
>> +	memcpy(audio.status, params->iec.status,
>> +	       min(sizeof(audio.status), sizeof(params->iec.status)));
> 
> As mentioned in the other patch, the audio status does not directly
> correspond with the AES bytes, so this ends up causing the driver to
> write the wrong bytes to the wrong registers.
> 

As I said earlier, I'd rather have it as plain AES/IEC958 channel status
bits buffer.

>> +	ret = tda998x_configure_audio(priv,
>> +				      &audio,
>> +				      priv->encoder.crtc->hwmode.clock);
> 
> What happens if audio changes at the same time that the video mode
> changes?  What protection is there against two threads concurrently
> executing tda998x_configure_audio() ?
> 

Oh, yes. I definitely need some locking here. The same lock could
protect the atomicity of tda998x_audio_params update and usage.

>> +	priv->audio_pdev = platform_device_register_data(
>> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
>> +		&codec_data, sizeof(codec_data));
> 
> I'd much prefer that this was a child of the I2C device, which will
> show the relationship between this virtual platform device and the
> device which it's associated with.  That can be done via
> platform_device_register_full().
> 

That may be a problem. The ASoC card device tree binding current looks
for the ASoC DAI's parent's of-node if it can not find the node for the
DAI-device itself. Indicating ASoC DAI-link by referencing the parent
i2c device simply won't work, because there may be other ASoC codecs on
the same i2c bus. Adding a separate DT-node for a virtual audio device
should be possible, but it needs some thinking and may have its own
problems. I can not follow the reasoning behind this, is this really
necessary?

Best regards,
Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Russell King - ARM Linux]

On Fri, Aug 05, 2016 at 12:02:39PM +0300, Jyri Sarha wrote:
> On 08/04/16 17:07, Russell King - ARM Linux wrote:
> > On Tue, Aug 02, 2016 at 03:05:08PM +0300, Jyri Sarha wrote:
> >> +	priv->audio_pdev = platform_device_register_data(
> >> +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
> >> +		&codec_data, sizeof(codec_data));
> > 
> > I'd much prefer that this was a child of the I2C device, which will
> > show the relationship between this virtual platform device and the
> > device which it's associated with.  That can be done via
> > platform_device_register_full().
> > 
> 
> That may be a problem. The ASoC card device tree binding current looks
> for the ASoC DAI's parent's of-node if it can not find the node for the
> DAI-device itself.

I don't follow.  The "parent" of this audio_pdev device above is
the "platform" device - /sys/devices/platform.  If ASoC is
referencing the parent of the above platform device for some
reason, it's probably not going to get anything useful from such
an attempt.

Any other ASoC codec driver where the codec platform device was
declared with a NULL parent pointer also ends up as a child of
that same location.

I'd _really_ be surprised if ASoC is even doing what you describe,
because such an action is totally illogical (as can be seen from
my description above.)

Moving it under the I2C device means it stays as a platform device,
but the relationship between the platform device and its I2C parent
is properly represented in the tree of devices, and also ensures
that, should PM hooks be added, that the platform device gets
properly ordered wrt the I2C device.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On Fri, Aug 05, 2016 at 05:48:45PM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 05, 2016 at 12:02:39PM +0300, Jyri Sarha wrote:

> > That may be a problem. The ASoC card device tree binding current looks
> > for the ASoC DAI's parent's of-node if it can not find the node for the
> > DAI-device itself.

> I don't follow.  The "parent" of this audio_pdev device above is
> the "platform" device - /sys/devices/platform.  If ASoC is
> referencing the parent of the above platform device for some
> reason, it's probably not going to get anything useful from such
> an attempt.

> Any other ASoC codec driver where the codec platform device was
> declared with a NULL parent pointer also ends up as a child of
> that same location.

> I'd _really_ be surprised if ASoC is even doing what you describe,
> because such an action is totally illogical (as can be seen from
> my description above.)

We do have some stuff in there in order to handle MFDs - they'll
instantiate a Linux-internal virtual platform device as a child of the
DT device that represents the device as a whole.  We're definitely not
expecting to find anything for parentless platform devices though, it's
only done if the ASoC level device has no of_node and the parent does.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Russell King - ARM Linux]

On Fri, Aug 05, 2016 at 05:59:59PM +0100, Mark Brown wrote:
> On Fri, Aug 05, 2016 at 05:48:45PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Aug 05, 2016 at 12:02:39PM +0300, Jyri Sarha wrote:
> 
> > > That may be a problem. The ASoC card device tree binding current looks
> > > for the ASoC DAI's parent's of-node if it can not find the node for the
> > > DAI-device itself.
> 
> > I don't follow.  The "parent" of this audio_pdev device above is
> > the "platform" device - /sys/devices/platform.  If ASoC is
> > referencing the parent of the above platform device for some
> > reason, it's probably not going to get anything useful from such
> > an attempt.
> 
> > Any other ASoC codec driver where the codec platform device was
> > declared with a NULL parent pointer also ends up as a child of
> > that same location.
> 
> > I'd _really_ be surprised if ASoC is even doing what you describe,
> > because such an action is totally illogical (as can be seen from
> > my description above.)
> 
> We do have some stuff in there in order to handle MFDs - they'll
> instantiate a Linux-internal virtual platform device as a child of the
> DT device that represents the device as a whole.  We're definitely not
> expecting to find anything for parentless platform devices though, it's
> only done if the ASoC level device has no of_node and the parent does.

And if we make the hdmi-codec device a child of the I2C device which
does have an of-node, what will happen?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 876 bytes --]

On Fri, Aug 05, 2016 at 06:04:50PM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 05, 2016 at 05:59:59PM +0100, Mark Brown wrote:

> > We do have some stuff in there in order to handle MFDs - they'll
> > instantiate a Linux-internal virtual platform device as a child of the
> > DT device that represents the device as a whole.  We're definitely not
> > expecting to find anything for parentless platform devices though, it's
> > only done if the ASoC level device has no of_node and the parent does.

> And if we make the hdmi-codec device a child of the I2C device which
> does have an of-node, what will happen?

It'll pick up that as the DT device to hang things off which I'd expect
to be the desired outcome given that this is a very similar situation to
the MFD situation.  I've not been following the full thread so there is
probably context I'm missing here...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Russell King - ARM Linux]

On Fri, Aug 05, 2016 at 06:59:08PM +0100, Mark Brown wrote:
> On Fri, Aug 05, 2016 at 06:04:50PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Aug 05, 2016 at 05:59:59PM +0100, Mark Brown wrote:
> 
> > > We do have some stuff in there in order to handle MFDs - they'll
> > > instantiate a Linux-internal virtual platform device as a child of the
> > > DT device that represents the device as a whole.  We're definitely not
> > > expecting to find anything for parentless platform devices though, it's
> > > only done if the ASoC level device has no of_node and the parent does.
> 
> > And if we make the hdmi-codec device a child of the I2C device which
> > does have an of-node, what will happen?
> 
> It'll pick up that as the DT device to hang things off which I'd expect
> to be the desired outcome given that this is a very similar situation to
> the MFD situation.  I've not been following the full thread so there is
> probably context I'm missing here...

Okay, and that sounds to me like a very reasonable thing to want to
happen - so that the audio side can be clearly identified as being
coupled with the video side.

So, I'm not seeing a problem with my suggestion... I'm actually more
reasons why it's a good thing to want.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Russell King - ARM Linux]

On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
>  
>  	/* Write the channel status */
> -	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
> -	buf[1] = 0x00;
> -	buf[2] = IEC958_AES3_CON_FS_NOTID;
> -	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
> -			IEC958_AES4_CON_MAX_WORDLEN_24;
> -	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
> +	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);

Another couple of points here:

1. I think we should only write the channel status if I2S mode is
selected - channel status won't be used on SPDIF mode as SPDIF
supplies its own channel status.

2. I think we should continue writing all the channel status in a
single I2C transaction, and not breaking it into several different
transactions to cater for the odd register order.  I don't have
any direct evidence that one way is better than the other in terms
of atomicity of the update, but I think having it as a single
transaction stands a better chance of the chip atomically updating
the channel status.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Russell King - ARM Linux]

On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -41,7 +41,7 @@ struct tda998x_priv {
>  	u8 vip_cntrl_0;
>  	u8 vip_cntrl_1;
>  	u8 vip_cntrl_2;
> -	struct tda998x_encoder_params params;
> +	struct tda998x_audio_params audio_params;
...
> @@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
>  			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
>  			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
>  
> -	priv->params = *p;
> +	priv->audio_params = p->audio;
...
> @@ -15,16 +28,7 @@ struct tda998x_encoder_params {
>  	u8 swap_e:3;
>  	u8 mirr_e:1;
>  
> -	u8 audio_cfg;
> -	u8 audio_clk_cfg;
> -	u8 audio_frame[6];
> -
> -	enum {
> -		AFMT_SPDIF,
> -		AFMT_I2S
> -	} audio_format;
> -
> -	unsigned audio_sample_rate;
> +	struct tda998x_audio_params audio;

Another point - it would be nice if the name for both tda998x_audio_params
structures was the same.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding

[Jyri Sarha]

On 08/06/16 01:19, Russell King - ARM Linux wrote:
>> > It'll pick up that as the DT device to hang things off which I'd expect
>> > to be the desired outcome given that this is a very similar situation to
>> > the MFD situation.  I've not been following the full thread so there is
>> > probably context I'm missing here...
> Okay, and that sounds to me like a very reasonable thing to want to
> happen - so that the audio side can be clearly identified as being
> coupled with the video side.
> 
> So, I'm not seeing a problem with my suggestion... I'm actually more
> reasons why it's a good thing to want.


Ok, so getting back to this comment:

>> +	priv->audio_pdev = platform_device_register_data(
>> > +		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
>> > +		&codec_data, sizeof(codec_data));
> I'd much prefer that this was a child of the I2C device, which will
> show the relationship between this virtual platform device and the
> device which it's associated with.  That can be done via
> platform_device_register_full().

The platform_device_register_data() sets the parent of the registered
platform device according to the first parameter, the tda998x i2c-client
in this case. Is there some reason why I should use
platform_device_register_full() and should I set parent to something
else than the tda998x i2c device?

BR,
Jyri
--
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