[PATCH 1/8] media: adv748x: add a device-specific wrapper for register block read

[PATCH 1/8] media: adv748x: add a device-specific wrapper for register block read

[Alex Riesen]

Some of the devices I2C-accessible registers (for instance, cs_data for
stereo channel information or tmds_params for TMDS channel information)
located in adjacent cells. According to manufacturers information, these
registers can be read using block transactions.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 15 +++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 23e02ff27b17..bc49aa93793c 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -97,6 +97,21 @@ static const struct adv748x_register_map adv748x_default_addresses[] = {
 	[ADV748X_PAGE_TXA] = { "txa", 0x4a },
 };
 
+int adv748x_read_block(struct adv748x_state *state, u8 client_page, u8 reg,
+		       void *val, size_t reg_count)
+{
+	struct i2c_client *client = state->i2c_clients[client_page];
+	int err;
+
+	err = regmap_bulk_read(state->regmap[client_page], reg, val, reg_count);
+	if (err) {
+		adv_err(state, "error reading %02x, %02x-%02lx: %d\n",
+				client->addr, reg, reg + reg_count - 1, err);
+		return err;
+	}
+	return 0;
+}
+
 static int adv748x_read_check(struct adv748x_state *state,
 			      int client_page, u8 reg)
 {
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 5042f9e94aee..db6346a06351 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -387,6 +387,8 @@ struct adv748x_state {
 /* Register handling */
 
 int adv748x_read(struct adv748x_state *state, u8 addr, u8 reg);
+int adv748x_read_block(struct adv748x_state *state, u8 page, u8 reg,
+		       void *val, size_t reg_count);
 int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
 int adv748x_write_block(struct adv748x_state *state, int client_page,
 			unsigned int init_reg, const void *val,
-- 
2.24.1.508.g91d2dafee0

[PATCH 2/8] media: adv748x: add audio mute control and output selection ioctls

[Alex Riesen]

This change implements audio-related V4L2 ioctls for the HDMI subdevice.

The master audio clock is configured for 256fs, as supported by the only
device available at the moment. For the same reason, the TDM slot is
formatted using left justification of its bits.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c |   6 +
 drivers/media/i2c/adv748x/adv748x-hdmi.c | 182 +++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  42 ++++++
 3 files changed, 230 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index bc49aa93793c..b6067ffb1e0d 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -150,6 +150,12 @@ static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
 	return *error;
 }
 
+int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg, u8 mask,
+			u8 value)
+{
+	return regmap_update_bits(state->regmap[page], reg, mask, value);
+}
+
 /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
  * size to one or more registers.
  *
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index c557f8fdf11a..9bc9237c9116 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include <linux/version.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 
@@ -603,11 +604,186 @@ static const struct v4l2_subdev_pad_ops adv748x_pad_ops_hdmi = {
 	.enum_dv_timings = adv748x_hdmi_enum_dv_timings,
 };
 
+static int adv748x_hdmi_audio_mute(struct adv748x_hdmi *hdmi, int enable)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+
+	return hdmi_update(state, ADV748X_HDMI_MUTE_CTRL,
+			   ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO,
+			   enable ? 0xff : 0);
+}
+
+
+#define HDMI_AOUT_NONE 0
+#define HDMI_AOUT_I2S 1
+#define HDMI_AOUT_I2S_TDM 2
+
+static int adv748x_hdmi_enumaudout(struct adv748x_hdmi *hdmi,
+				   struct v4l2_audioout *a)
+{
+	switch (a->index) {
+	case HDMI_AOUT_NONE:
+		strlcpy(a->name, "None", sizeof(a->name));
+		break;
+	case HDMI_AOUT_I2S:
+		strlcpy(a->name, "I2S/stereo", sizeof(a->name));
+		break;
+	case HDMI_AOUT_I2S_TDM:
+		strlcpy(a->name, "I2S-TDM/multichannel", sizeof(a->name));
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int adv748x_hdmi_g_audout(struct adv748x_hdmi *hdmi,
+				 struct v4l2_audioout *a)
+{
+	a->index = hdmi->audio_out;
+	return adv748x_hdmi_enumaudout(hdmi, a);
+}
+
+static int set_audio_pads_state(struct adv748x_state *state, int on)
+{
+	return io_update(state, ADV748X_IO_PAD_CONTROLS,
+			 ADV748X_IO_PAD_CONTROLS_TRI_AUD |
+			 ADV748X_IO_PAD_CONTROLS_PDN_AUD,
+			 on ? 0 : 0xff);
+}
+
+static int set_dpll_mclk_fs(struct adv748x_state *state, int fs)
+{
+	if (fs % 128 || fs > 768)
+		return -EINVAL;
+	return dpll_update(state, ADV748X_DPLL_MCLK_FS,
+			   ADV748X_DPLL_MCLK_FS_N_MASK, (fs / 128) - 1);
+}
+
+static int set_i2s_format(struct adv748x_state *state, uint outmode,
+			  uint bitwidth)
+{
+	return hdmi_update(state, ADV748X_HDMI_I2S,
+			   ADV748X_HDMI_I2SBITWIDTH_MASK |
+			   ADV748X_HDMI_I2SOUTMODE_MASK,
+			   (outmode << ADV748X_HDMI_I2SOUTMODE_SHIFT) |
+			   bitwidth);
+}
+
+static int set_i2s_tdm_mode(struct adv748x_state *state, int is_tdm)
+{
+	int ret;
+
+	ret = hdmi_update(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
+			  ADV748X_MAN_AUDIO_DL_BYPASS |
+			  ADV748X_AUDIO_DELAY_LINE_BYPASS,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		goto fail;
+	ret = hdmi_update(state, ADV748X_HDMI_REG_6D,
+			  ADV748X_I2S_TDM_MODE_ENABLE,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		goto fail;
+	ret = set_i2s_format(state, ADV748X_I2SOUTMODE_LEFT_J, 24);
+fail:
+	return ret;
+}
+
+static int set_audio_out(struct adv748x_state *state, int aout)
+{
+	int ret;
+
+	switch (aout) {
+	case HDMI_AOUT_NONE:
+		ret = set_audio_pads_state(state, 0);
+		break;
+	case HDMI_AOUT_I2S:
+		ret = set_dpll_mclk_fs(state, 256);
+		if (ret < 0)
+			goto fail;
+		ret = set_i2s_tdm_mode(state, 1);
+		if (ret < 0)
+			goto fail;
+		ret = set_audio_pads_state(state, 1);
+		if (ret < 0)
+			goto fail;
+		break;
+	case HDMI_AOUT_I2S_TDM:
+		ret = set_dpll_mclk_fs(state, 256);
+		if (ret < 0)
+			goto fail;
+		ret = set_i2s_tdm_mode(state, 1);
+		if (ret < 0)
+			goto fail;
+		ret = set_audio_pads_state(state, 1);
+		if (ret < 0)
+			goto fail;
+		break;
+	default:
+		ret = -EINVAL;
+		goto fail;
+	}
+	return 0;
+fail:
+	return ret;
+}
+
+static int adv748x_hdmi_s_audout(struct adv748x_hdmi *hdmi,
+				 const struct v4l2_audioout *a)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+	int ret = set_audio_out(state, a->index);
+
+	if (ret == 0)
+		hdmi->audio_out = a->index;
+	return ret;
+}
+
+static long adv748x_hdmi_querycap(struct adv748x_hdmi *hdmi,
+				  struct v4l2_capability *cap)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+
+	cap->version = LINUX_VERSION_CODE;
+	strlcpy(cap->driver, state->dev->driver->name, sizeof(cap->driver));
+	strlcpy(cap->card, "hdmi", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "i2c:%d-%04x",
+		 i2c_adapter_id(state->client->adapter),
+		 state->client->addr);
+	cap->device_caps = V4L2_CAP_AUDIO | V4L2_CAP_VIDEO_CAPTURE;
+	cap->capabilities = V4L2_CAP_DEVICE_CAPS;
+	return 0;
+}
+
+static long adv748x_hdmi_ioctl(struct v4l2_subdev *sd,
+			       unsigned int cmd, void *arg)
+{
+	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
+
+	switch (cmd) {
+	case VIDIOC_ENUMAUDOUT:
+		return adv748x_hdmi_enumaudout(hdmi, arg);
+	case VIDIOC_S_AUDOUT:
+		return adv748x_hdmi_s_audout(hdmi, arg);
+	case VIDIOC_G_AUDOUT:
+		return adv748x_hdmi_g_audout(hdmi, arg);
+	case VIDIOC_QUERYCAP:
+		return adv748x_hdmi_querycap(hdmi, arg);
+	}
+	return -ENOTTY;
+}
+
+static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
+	.ioctl = adv748x_hdmi_ioctl,
+};
+
 /* -----------------------------------------------------------------------------
  * v4l2_subdev_ops
  */
 
 static const struct v4l2_subdev_ops adv748x_ops_hdmi = {
+	.core = &adv748x_core_ops_hdmi,
 	.video = &adv748x_video_ops_hdmi,
 	.pad = &adv748x_pad_ops_hdmi,
 };
@@ -633,6 +809,8 @@ static int adv748x_hdmi_s_ctrl(struct v4l2_ctrl *ctrl)
 	int ret;
 	u8 pattern;
 
+	if (ctrl->id == V4L2_CID_AUDIO_MUTE)
+		return adv748x_hdmi_audio_mute(hdmi, ctrl->val);
 	/* Enable video adjustment first */
 	ret = cp_clrset(state, ADV748X_CP_VID_ADJ,
 			ADV748X_CP_VID_ADJ_ENABLE,
@@ -697,6 +875,8 @@ static int adv748x_hdmi_init_controls(struct adv748x_hdmi *hdmi)
 	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
 			  V4L2_CID_HUE, ADV748X_CP_HUE_MIN,
 			  ADV748X_CP_HUE_MAX, 1, ADV748X_CP_HUE_DEF);
+	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
+			  V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
 
 	/*
 	 * Todo: V4L2_CID_DV_RX_POWER_PRESENT should also be supported when
@@ -755,6 +935,8 @@ int adv748x_hdmi_init(struct adv748x_hdmi *hdmi)
 
 void adv748x_hdmi_cleanup(struct adv748x_hdmi *hdmi)
 {
+	adv748x_hdmi_audio_mute(hdmi, 1);
+	set_audio_out(adv748x_hdmi_to_state(hdmi), HDMI_AOUT_NONE);
 	v4l2_device_unregister_subdev(&hdmi->sd);
 	media_entity_cleanup(&hdmi->sd.entity);
 	v4l2_ctrl_handler_free(&hdmi->ctrl_hdl);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index db6346a06351..fdda6982e437 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -128,6 +128,7 @@ struct adv748x_hdmi {
 		u32 present;
 		unsigned int blocks;
 	} edid;
+	int audio_out;
 };
 
 #define adv748x_ctrl_to_hdmi(ctrl) \
@@ -224,6 +225,11 @@ struct adv748x_state {
 
 #define ADV748X_IO_VID_STD		0x05
 
+#define ADV748X_IO_PAD_CONTROLS		0x0e
+#define ADV748X_IO_PAD_CONTROLS_TRI_AUD	BIT(5)
+#define ADV748X_IO_PAD_CONTROLS_PDN_AUD	BIT(1)
+#define ADV748X_IO_PAD_CONTROLS1	0x1d
+
 #define ADV748X_IO_10			0x10	/* io_reg_10 */
 #define ADV748X_IO_10_CSI4_EN		BIT(7)
 #define ADV748X_IO_10_CSI1_EN		BIT(6)
@@ -246,7 +252,21 @@ struct adv748x_state {
 #define ADV748X_IO_REG_FF		0xff
 #define ADV748X_IO_REG_FF_MAIN_RESET	0xff
 
+/* DPLL Map */
+#define ADV748X_DPLL_MCLK_FS		0xb5
+#define ADV748X_DPLL_MCLK_FS_N_MASK	GENMASK(2, 0)
+
 /* HDMI RX Map */
+#define ADV748X_HDMI_I2S		0x03	/* I2S mode and width */
+#define ADV748X_HDMI_I2SBITWIDTH_MASK	GENMASK(4, 0)
+#define ADV748X_HDMI_I2SOUTMODE_SHIFT	5
+#define ADV748X_HDMI_I2SOUTMODE_MASK	\
+	GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT)
+#define ADV748X_I2SOUTMODE_I2S 0
+#define ADV748X_I2SOUTMODE_RIGHT_J 1
+#define ADV748X_I2SOUTMODE_LEFT_J 2
+#define ADV748X_I2SOUTMODE_SPDIF 3
+
 #define ADV748X_HDMI_LW1		0x07	/* line width_1 */
 #define ADV748X_HDMI_LW1_VERT_FILTER	BIT(7)
 #define ADV748X_HDMI_LW1_DE_REGEN	BIT(5)
@@ -258,6 +278,16 @@ struct adv748x_state {
 #define ADV748X_HDMI_F1H1		0x0b	/* field1 height_1 */
 #define ADV748X_HDMI_F1H1_INTERLACED	BIT(5)
 
+#define ADV748X_HDMI_MUTE_CTRL		0x1a
+#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4)
+#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK	GENMASK(3, 1)
+#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE	BIT(0)
+
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED	0x0f
+#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK	GENMASK(4, 0)
+#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7)
+#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6)
+
 #define ADV748X_HDMI_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
 #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
 
@@ -279,6 +309,9 @@ struct adv748x_state {
 #define ADV748X_HDMI_TMDS_1		0x51	/* hdmi_reg_51 */
 #define ADV748X_HDMI_TMDS_2		0x52	/* hdmi_reg_52 */
 
+#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
+#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
+
 /* HDMI RX Repeater Map */
 #define ADV748X_REPEATER_EDID_SZ	0x70	/* primary_edid_size */
 #define ADV748X_REPEATER_EDID_SZ_SHIFT	4
@@ -393,14 +426,23 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
 int adv748x_write_block(struct adv748x_state *state, int client_page,
 			unsigned int init_reg, const void *val,
 			size_t val_len);
+int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
+			u8 mask, u8 value);
 
 #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
 #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
 #define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~m) | v)
+#define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
 
 #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
 #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, r+1)) & m)
 #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
+#define hdmi_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
+
+#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
+#define dpll_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
 
 #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
 #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)
-- 
2.24.1.508.g91d2dafee0

[PATCH 3/8] media: adv748x: add log_status ioctl

[Alex Riesen]

The logged information provides insights about cable connection and the
state of the HDMI decoder. It is very useful when debugging hardware
problems in environments without easy access to the connectors.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x-hdmi.c | 173 +++++++++++++++++++++++
 1 file changed, 173 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 9bc9237c9116..69dfafc4e0f5 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -613,6 +613,178 @@ static int adv748x_hdmi_audio_mute(struct adv748x_hdmi *hdmi, int enable)
 			   enable ? 0xff : 0);
 }
 
+struct tmds_params {
+	u32 cts, n;
+	u16 tmdsfreq, tmdsfreq_frac;
+};
+
+static inline const char *cs_data_smpl_freq_str(u8 cs_data_3)
+{
+	switch (cs_data_3 & 0xf) {
+	case 0:
+		return "44.1";
+	case 2:
+		return "48";
+	case 3:
+		return "32";
+	case 8:
+		return "88.2";
+	case 10:
+		return "96";
+	case 12:
+		return "176";
+	case 14:
+		return "192";
+	}
+	return "reserved";
+}
+
+static inline const char *cs_data_clk_lvl_str(u8 cs_data_3)
+{
+	switch (cs_data_3 & 0x30) {
+	case 0:
+		return "Level II";
+	case 1:
+		return "Level I";
+	case 2:
+		return "Level III, variable pitch shifted";
+	}
+	return "reserved";
+}
+
+static inline const char *i2s_out_mode_str(u8 i2s_mode)
+{
+	switch (i2s_mode & ADV748X_HDMI_I2SOUTMODE_MASK) {
+	case 0 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "I2S";
+	case 1 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "right";
+	case 2 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "left";
+	case 3 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "spdif";
+	}
+	return "";
+}
+
+static int adv748x_hdmi_log_status(struct v4l2_subdev *sd)
+{
+	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+	u8 rv, i2s_tdm_mode_enable;
+	u8 cts_n[5];
+	u8 cs_data[0x3a - 0x36 + 1];
+	u8 tmdsfreq[2]; /* both tmdsfreq and tmdsfreq_frac */
+	struct tmds_params tmds_params;
+
+	/* Audio control and configuration */
+	rv = io_read(state, 0x71);
+	pr_info("cable_det_a_raw         %s\n",
+		rv & BIT(6) ? "detected" : "no cable");
+	pr_info("tmds_clk_a_raw          %s\n",
+		rv & BIT(3) ? "detected" : "no TMDS clock");
+	pr_info("tmdspll_lck_a_raw       %s\n",
+		rv & BIT(7) ? "locked to incoming clock" : "not locked");
+	pr_info("hdmi_encrpt_a_raw       %s\n",
+		rv & BIT(5) ? "current frame encrypted" : "not encrypted");
+	rv = hdmi_read(state, 0x04);
+	pr_info("audio_pll_locked        0x%02lx\n", rv & BIT(0));
+	pr_info("tmds_pll_locked         0x%02lx\n", rv & BIT(1));
+	rv = io_read(state, 0x6c);
+	pr_info("gamut_mdata_raw         %s\n",
+		rv & BIT(0) ? "received" : "-");
+	pr_info("audio_c_pckt_raw        %s\n",
+		rv & BIT(1) ? "ACR received" : "-");
+	pr_info("gen_ctl_pckt_raw        %s\n",
+		rv & BIT(2) ? "received" : "-");
+	pr_info("hdmi_mode_raw           %s\n",
+		rv & BIT(3) ? "HDMI/MHL" : "-");
+	pr_info("audio_ch_md_raw         %s\n",
+		rv & BIT(4) ? "multichannel" : "-");
+	pr_info("av_mute_raw             %s\n",
+		rv & BIT(5) ? "received" : "-");
+	pr_info("internal_mute_raw       %s\n",
+		rv & BIT(6) ? "asserted" : "-");
+	pr_info("cs_data_valid_raw       %s\n",
+		rv & BIT(7) ? "valid" : "-");
+	rv = hdmi_read(state, 0x6d);
+	pr_info("i2s_tdm_mode_enable     %s\n",
+		rv & BIT(7) ? "TDM (multichannel)" : "I2S (stereo)");
+	i2s_tdm_mode_enable = rv & BIT(7);
+
+	/* i2s_tdm_mode_enable must be unset */
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x36,
+			       cs_data, ARRAY_SIZE(cs_data)) == 0) {
+		pr_info("... cs_data %s\n",
+			cs_data[0] & BIT(0) ? "pro" : "consumer");
+		pr_info("... cs_data %s\n",
+			cs_data[0] & BIT(1) ? "other" : "L-PCM");
+		pr_info("... cs_data %s copyright\n",
+			cs_data[0] & BIT(2) ? "no" : "asserted");
+		pr_info("... cs_data %s (%lu)\n",
+			cs_data[0] & GENMASK(5, 3) ?
+			"50/15" : "no pre-emphasis",
+			(cs_data[0] & GENMASK(5, 3)) >> 4);
+		pr_info("... cs_data channels status mode %lu\n",
+			(cs_data[0] & GENMASK(7, 6)) >> 7);
+		pr_info("... cs_data category code 0x%02x\n", cs_data[1]);
+		pr_info("... cs_data source number %u\n", cs_data[2] & 0xf);
+		pr_info("... cs_data channel number %u\n",
+			(cs_data[2] & 0xf0) >> 4);
+		pr_info("... cs_data sampling frequency %s (%u)\n",
+			cs_data_smpl_freq_str(cs_data[3]), cs_data[3] & 0xf);
+		pr_info("... cs_data clock accuracy %s\n",
+			cs_data_clk_lvl_str(cs_data[3]));
+	}
+	rv = hdmi_read(state, ADV748X_HDMI_I2S);
+	pr_info("i2soutmode              %s\n", i2s_out_mode_str(rv));
+	pr_info("i2sbitwidth             %u\n", rv & 0x1fu);
+	rv = hdmi_read(state, 0x05);
+	pr_info("hdmi_mode               %s\n", rv & BIT(7) ? "HDMI" : "DVI");
+	rv = hdmi_read(state, 0x07);
+	pr_info("audio_channel_mode      %s\n",
+		rv & BIT(6) ? "multichannel" : "stereo or compressed");
+	rv = hdmi_read(state, 0x0f);
+	/* The bits 6 and 7 must be 1 if TDM mode */
+	pr_info("man_audio_dl_bypass     0x%02lx\n", rv & BIT(7));
+	pr_info("audio_delay_line_bypass 0x%02lx\n", rv & BIT(6));
+	rv = hdmi_read(state, 0x6e);
+	pr_info("mux_spdif_to_i2s_enable %s\n", rv & BIT(3) ? "SPDIF" : "I2S");
+	rv = dpll_read(state, ADV748X_DPLL_MCLK_FS);
+	pr_info("mclk_fs_n               %lu\n",
+		((rv & ADV748X_DPLL_MCLK_FS_N_MASK) + 1) * 128);
+
+	/* i2s_tdm_mode_enable must be set */
+	memset(&tmds_params, 0, sizeof(tmds_params));
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x5b, cts_n, 5) == 0) {
+		tmds_params.cts  = cts_n[0] << 12;
+		tmds_params.cts |= cts_n[1] << 4;
+		tmds_params.cts |= cts_n[2] >> 4;
+		tmds_params.n  = (cts_n[2] & 0xf) << 16;
+		tmds_params.n |= cts_n[3] << 8;
+		tmds_params.n |= cts_n[4];
+		pr_info("... TDM: ACR cts  %u\n", tmds_params.cts);
+		pr_info("... TDM: ACR n    %u\n", tmds_params.n);
+	}
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x51,
+			       tmdsfreq, 2) == 0) {
+		tmds_params.tmdsfreq  = tmdsfreq[0] << 1;
+		tmds_params.tmdsfreq |= tmdsfreq[1] >> 7;
+		tmds_params.tmdsfreq_frac = tmdsfreq[1] & 0x7f;
+		pr_info("... TDM: tmdsfreq       %d MHz\n",
+			tmds_params.tmdsfreq);
+		pr_info("... TDM: tmdsfreq_frac  %d 1/128\n",
+			tmds_params.tmdsfreq_frac);
+	}
+	if (i2s_tdm_mode_enable)
+		pr_info("... TDM: sampling frequency %u Hz\n",
+			tmds_params.cts ?
+			(tmds_params.tmdsfreq * tmds_params.n +
+			 tmds_params.tmdsfreq_frac * tmds_params.n / 128) *
+			1000 / (128 * tmds_params.cts / 1000) :
+			UINT_MAX);
+	return 0;
+}
 
 #define HDMI_AOUT_NONE 0
 #define HDMI_AOUT_I2S 1
@@ -775,6 +947,7 @@ static long adv748x_hdmi_ioctl(struct v4l2_subdev *sd,
 }
 
 static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
+	.log_status = adv748x_hdmi_log_status,
 	.ioctl = adv748x_hdmi_ioctl,
 };
 
-- 
2.24.1.508.g91d2dafee0

[PATCH 4/8] media: adv748x: reserve space for the audio (I2S) port in the driver structures

[Alex Riesen]

This allows use of the port in the device tree files.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index fdda6982e437..5db06410f102 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -61,7 +61,8 @@ enum adv748x_ports {
 	ADV748X_PORT_TTL = 9,
 	ADV748X_PORT_TXA = 10,
 	ADV748X_PORT_TXB = 11,
-	ADV748X_PORT_MAX = 12,
+	ADV748X_PORT_I2S = 12,
+	ADV748X_PORT_MAX = 13,
 };
 
 enum adv748x_csi2_pads {
-- 
2.24.1.508.g91d2dafee0

[PATCH 5/8] media: adv748x: add an ASoC DAI definition to the driver

[Alex Riesen]

The definition is used to publish hardware constraints and can be used
to implement in-demand device configuration.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 33 ++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index b6067ffb1e0d..75e4bf144ad7 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -25,6 +25,7 @@
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-ioctl.h>
+#include <sound/soc.h>
 
 #include "adv748x.h"
 
@@ -689,6 +690,30 @@ static void adv748x_dt_cleanup(struct adv748x_state *state)
 		of_node_put(state->endpoints[i]);
 }
 
+static struct snd_soc_dai_driver adv748x_dai = {
+	.name = "adv748x-i2s",
+	.capture = {
+		.stream_name	= "Capture",
+		.channels_min	= 8,
+		.channels_max	= 8,
+		.rates = SNDRV_PCM_RATE_48000,
+		.formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_U24_LE,
+	 },
+};
+
+static	int adv748x_of_xlate_dai_name(struct snd_soc_component *component,
+				      struct of_phandle_args *args,
+				      const char **dai_name)
+{
+	if (dai_name)
+		*dai_name = adv748x_dai.name;
+	return 0;
+}
+
+static const struct snd_soc_component_driver adv748x_codec = {
+	.of_xlate_dai_name = adv748x_of_xlate_dai_name,
+};
+
 static int adv748x_probe(struct i2c_client *client)
 {
 	struct adv748x_state *state;
@@ -782,8 +807,16 @@ static int adv748x_probe(struct i2c_client *client)
 		goto err_cleanup_txa;
 	}
 
+	ret = devm_snd_soc_register_component(state->dev, &adv748x_codec,
+					      &adv748x_dai, 1);
+	if (ret < 0) {
+		adv_err(state, "Failed to register the codec");
+		goto err_cleanup_txb;
+	}
 	return 0;
 
+err_cleanup_txb:
+	adv748x_csi2_cleanup(&state->txb);
 err_cleanup_txa:
 	adv748x_csi2_cleanup(&state->txa);
 err_cleanup_afe:
-- 
2.24.1.508.g91d2dafee0

[PATCH 6/8] media: adv748x: reduce amount of code for bitwise modification of device registers

[Alex Riesen]

The regmap provides corresponding routine (regmap_update_bits) already
wrapped for this device earlier.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 drivers/media/i2c/adv748x/adv748x.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 5db06410f102..ab3f754542fe 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -432,7 +432,7 @@ int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
 
 #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
 #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
-#define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~m) | v)
+#define io_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
 #define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
 
 #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
@@ -450,11 +450,11 @@ int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
 
 #define sdp_read(s, r) adv748x_read(s, ADV748X_PAGE_SDP, r)
 #define sdp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_SDP, r, v)
-#define sdp_clrset(s, r, m, v) sdp_write(s, r, (sdp_read(s, r) & ~m) | v)
+#define sdp_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_SDP, r, m, v)
 
 #define cp_read(s, r) adv748x_read(s, ADV748X_PAGE_CP, r)
 #define cp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_CP, r, v)
-#define cp_clrset(s, r, m, v) cp_write(s, r, (cp_read(s, r) & ~m) | v)
+#define cp_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_CP, r, m, v)
 
 #define tx_read(t, r) adv748x_read(t->state, t->page, r)
 #define tx_write(t, r, v) adv748x_write(t->state, t->page, r, v)
-- 
2.24.1.508.g91d2dafee0

[PATCH 7/8] dt-bindings: adv748x: add information about serial audio interface (I2S/TDM)

[Alex Riesen]

As the driver has some support for the audio interface of the device,
the bindings file should mention it.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 .../devicetree/bindings/media/i2c/adv748x.txt       | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
index 4f91686e54a6..c42dffb37a82 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
@@ -2,7 +2,9 @@
 
 The ADV7481 and ADV7482 are multi format video decoders with an integrated
 HDMI receiver. They can output CSI-2 on two independent outputs TXA and TXB
-from three input sources HDMI, analog and TTL.
+from three input sources HDMI, analog and TTL. There is also support for an
+I2S compatible interface connected to the audio processor of the HDMI decoder.
+The interface has TDM capability (8 slots, 32 bits, left or right justified).
 
 Required Properties:
 
@@ -47,6 +49,7 @@ are numbered as follows.
 	  TTL		sink		9
 	  TXA		source		10
 	  TXB		source		11
+	  I2S		source		12
 
 The digital output port nodes, when present, shall contain at least one
 endpoint. Each of those endpoints shall contain the data-lanes property as
@@ -113,4 +116,12 @@ Example:
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				remote-endpoint = <&i2s_in>;
+			};
+		};
 	};
-- 
2.24.1.508.g91d2dafee0

[PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Not sure if all variants of the Salvator board have the HDMI decoder
chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
are placed in the board file.

I do assume though that all Salvator variants have the CLK_C clock line
hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
clocks provided by the R-Car sound system.

The I2C wiring is also likely to persist across the variants (similar
to ak4613, connected to the same interface), so that is in the common
file.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 .../dts/renesas/r8a7795-es1-salvator-x.dts    | 24 ++++++++++++-
 .../boot/dts/renesas/salvator-common.dtsi     | 35 ++++++++++++++++---
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
index c72968623e94..10f74f7a0efe 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
@@ -136,9 +136,29 @@
 				playback = <&ssi3>;
 			};
 		};
+		rsnd_port3: port@3 {
+			reg = <3>;
+			rsnd_endpoint3: endpoint {
+				remote-endpoint = <&adv7482_i2s>;
+
+				dai-tdm-slot-num = <8>;
+				dai-tdm-slot-width = <32>;
+				dai-format = "left_j";
+				mclk-fs = <256>;
+				bitclock-master = <&adv7482_i2s>;
+				frame-master = <&adv7482_i2s>;
+				system-clock-direction-out;
+
+				capture = <&ssi4>;
+			};
+		};
 	};
 };
 
+&adv7482_i2s {
+	remote-endpoint = <&rsnd_endpoint3>;
+};
+
 &sata {
 	status = "okay";
 };
@@ -146,9 +166,11 @@
 &sound_card {
 	dais = <&rsnd_port0	/* ak4613 */
 		&rsnd_port1	/* HDMI0  */
-		&rsnd_port2>;	/* HDMI1  */
+		&rsnd_port2	/* HDMI1  */
+		&rsnd_port3>;	/* adv7482 hdmi-in  */
 };
 
+
 &usb2_phy2 {
 	pinctrl-0 = <&usb2_pins>;
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 21e01056e759..e887805b16fc 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -322,6 +322,10 @@
 	clock-frequency = <22579200>;
 };
 
+&audio_clk_c {
+	clock-frequency = <12288000>;
+};
+
 &avb {
 	pinctrl-0 = <&avb_pins>;
 	pinctrl-names = "default";
@@ -471,12 +475,14 @@
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#sound-dai-cells = <0>;
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";
 		interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
 			     <31 IRQ_TYPE_LEVEL_LOW>;
-
+		clocks = <&rcar_sound 3>, <&audio_clk_c>;
+		clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
 		port@7 {
 			reg = <7>;
 
@@ -512,6 +518,14 @@
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				/* remote-endpoint defined in the board file */
+			};
+		};
 	};
 
 	csa_vdd: adc@7c {
@@ -686,7 +700,8 @@
 	};
 
 	sound_pins: sound {
-		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+			 "ssi4_data";
 		function = "ssi";
 	};
 
@@ -735,8 +750,8 @@
 	pinctrl-0 = <&sound_pins &sound_clk_pins>;
 	pinctrl-names = "default";
 
-	/* Single DAI */
-	#sound-dai-cells = <0>;
+	/* multi DAI */
+	#sound-dai-cells = <1>;
 
 	/* audio_clkout0/1/2/3 */
 	#clock-cells = <1>;
@@ -760,8 +775,18 @@
 		 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
 		 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
 		 <&audio_clk_a>, <&cs2000>,
-		 <&audio_clk_c>,
 		 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
+	clock-names = "ssi-all",
+		      "ssi.9", "ssi.8", "ssi.7", "ssi.6",
+		      "ssi.5", "ssi.4", "ssi.3", "ssi.2",
+		      "ssi.1", "ssi.0",
+		      "src.9", "src.8", "src.7", "src.6",
+		      "src.5", "src.4", "src.3", "src.2",
+		      "src.1", "src.0",
+		      "mix.1", "mix.0",
+		      "ctu.1", "ctu.0",
+		      "dvc.0", "dvc.1",
+		      "clk_a", "clk_b", "clk_i";
 
 	ports {
 		#address-cells = <1>;
-- 
2.24.1.508.g91d2dafee0

[PATCH 0/8] media: i2c: adv748x: add support for HDMI audio

[Alex Riesen]

This adds minimal support for controlling the audio output I2S port available
on ADV7481 and ADV7482 HDMI decoder devices by ADI. The port carries audio
signal from the decoded HDMI stream.

An ADV7482 on the Renesas Salvator-X ES1.1 was used during development of this
code.

Alex Riesen (8):
  media: adv748x: add a device-specific wrapper for register block read
  media: adv748x: add audio mute control and output selection ioctls
  media: adv748x: add log_status ioctl
  media: adv748x: reserve space for the audio (I2S) port in the driver
    structures
  media: adv748x: add an ASoC DAI definition to the driver
  media: adv748x: reduce amount of code for bitwise modification of
    device registers
  dt-bindings: adv748x: add information about serial audio interface
    (I2S/TDM)
  arm64: dts: renesas: salvator: add a connection from adv748x codec
    (HDMI input) to the R-Car SoC

 .../devicetree/bindings/media/i2c/adv748x.txt |  13 +-
 .../dts/renesas/r8a7795-es1-salvator-x.dts    |  24 +-
 .../boot/dts/renesas/salvator-common.dtsi     |  35 +-
 drivers/media/i2c/adv748x/adv748x-core.c      |  54 +++
 drivers/media/i2c/adv748x/adv748x-hdmi.c      | 355 ++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h           |  53 ++-
 6 files changed, 523 insertions(+), 11 deletions(-)

-- 
2.24.1.508.g91d2dafee0

Re: [PATCH 7/8] dt-bindings: adv748x: add information about serial audio interface (I2S/TDM)

[Rob Herring]

On Mon, 13 Jan 2020 15:15:50 +0100, Alex Riesen wrote:
> As the driver has some support for the audio interface of the device,
> the bindings file should mention it.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  .../devicetree/bindings/media/i2c/adv748x.txt       | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Geert Uytterhoeven]

Hi Alex,

Thanks for your patch!

On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Not sure if all variants of the Salvator board have the HDMI decoder
> chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> are placed in the board file.

Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.

> I do assume though that all Salvator variants have the CLK_C clock line
> hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> clocks provided by the R-Car sound system.

Yes, both Salvator-X and Salvator-XS have it wired that way.  But please
see below.

> The I2C wiring is also likely to persist across the variants (similar
> to ak4613, connected to the same interface), so that is in the common
> file.
>
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>

Below are my comments w.r.t. the board-specific wiring.
I'll defer to the multimedia people for commenting on the audio parts.

BTW, what is the status of the other patches in this series?

> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> @@ -322,6 +322,10 @@
>         clock-frequency = <22579200>;
>  };
>
> +&audio_clk_c {
> +       clock-frequency = <12288000>;
> +};

Does the ADV7482 always generate a 12.288 MHz clock signal?
Or is this programmable?

> +
>  &avb {
>         pinctrl-0 = <&avb_pins>;
>         pinctrl-names = "default";
> @@ -471,12 +475,14 @@
>
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> +               #sound-dai-cells = <0>;
>
>                 interrupt-parent = <&gpio6>;
>                 interrupt-names = "intrq1", "intrq2";
>                 interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
>                              <31 IRQ_TYPE_LEVEL_LOW>;
> -
> +               clocks = <&rcar_sound 3>, <&audio_clk_c>;
> +               clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";

The above declares the Audio CLK C to be a clock input of the ADV7482, while
it is an output.
Furthermore, the DT bindings do not document that clocks can be specified.

>                 port@7 {
>                         reg = <7>;
>
> @@ -512,6 +518,14 @@
>                                 remote-endpoint = <&csi20_in>;
>                         };
>                 };
> +
> +               port@c {
> +                       reg = <12>;
> +
> +                       adv7482_i2s: endpoint {
> +                               /* remote-endpoint defined in the board file */
> +                       };
> +               };
>         };
>
>         csa_vdd: adc@7c {
> @@ -686,7 +700,8 @@
>         };
>
>         sound_pins: sound {
> -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> +                        "ssi4_data";

Missing "ss4_ctrl", for the SCK4 and WS4 pins.

>                 function = "ssi";
>         };
>
> @@ -735,8 +750,8 @@
>         pinctrl-0 = <&sound_pins &sound_clk_pins>;
>         pinctrl-names = "default";
>
> -       /* Single DAI */
> -       #sound-dai-cells = <0>;
> +       /* multi DAI */
> +       #sound-dai-cells = <1>;
>
>         /* audio_clkout0/1/2/3 */
>         #clock-cells = <1>;
> @@ -760,8 +775,18 @@
>                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
>                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
>                  <&audio_clk_a>, <&cs2000>,
> -                <&audio_clk_c>,

Why remove it? This is the list of clock inputs, not outputs.

>                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> +       clock-names = "ssi-all",
> +                     "ssi.9", "ssi.8", "ssi.7", "ssi.6",
> +                     "ssi.5", "ssi.4", "ssi.3", "ssi.2",
> +                     "ssi.1", "ssi.0",
> +                     "src.9", "src.8", "src.7", "src.6",
> +                     "src.5", "src.4", "src.3", "src.2",
> +                     "src.1", "src.0",
> +                     "mix.1", "mix.0",
> +                     "ctu.1", "ctu.0",
> +                     "dvc.0", "dvc.1",
> +                     "clk_a", "clk_b", "clk_i";
>
>         ports {
>                 #address-cells = <1>;
> --
> 2.24.1.508.g91d2dafee0

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Geert Uytterhoeven, Mon, Mar 02, 2020 13:28:13 +0100:
> Hi Alex,
> 
> Thanks for your patch!
> 
> On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
> <alexander.riesen@cetitec.com> wrote:
> > Not sure if all variants of the Salvator board have the HDMI decoder
> > chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> > Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> > are placed in the board file.
> 
> Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.
> 
> > I do assume though that all Salvator variants have the CLK_C clock line
> > hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> > clocks provided by the R-Car sound system.
> 
> Yes, both Salvator-X and Salvator-XS have it wired that way.

Ok, seems like I can move that part into the common file as well.
Integrations of ADV7482 and R-Car which use salvator-common.dts can still
redefine the endpoint settings in their board files, right?

> But please see below.

...

> > The I2C wiring is also likely to persist across the variants (similar
> > to ak4613, connected to the same interface), so that is in the common
> > file.
> >
> > Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> 
> Below are my comments w.r.t. the board-specific wiring.
> I'll defer to the multimedia people for commenting on the audio parts.
> 
> BTW, what is the status of the other patches in this series?

"Submitted", at the moment. Besides you and Rob Herring no one said anything
yet (either that or I missed the replies).

> > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > @@ -322,6 +322,10 @@
> >         clock-frequency = <22579200>;
> >  };
> >
> > +&audio_clk_c {
> > +       clock-frequency = <12288000>;
> > +};
> 
> Does the ADV7482 always generate a 12.288 MHz clock signal?
> Or is this programmable?

Oops. It looks like it is and the value is derived from the sampling rate
(48kHz) and the master clock multiplier. Both hard-coded in the board file.

> > video-receiver@70 {
> > 	compatible = "adi,adv7482";
> > ...
> > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> 
> The above declares the Audio CLK C to be a clock input of the ADV7482, while
> it is an output.

I would gladly give it right direction if I *really* understood what I was
doing...

> Furthermore, the DT bindings do not document that clocks can be specified.

Should the DT bindings document that the clock cannot be specified than?

> > @@ -686,7 +700,8 @@
> >         };
> >
> >         sound_pins: sound {
> > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > +                        "ssi4_data";
> 
> Missing "ss4_ctrl", for the SCK4 and WS4 pins.

I'll add them.
As the device seems to function even without thoes, does this mean the pins in
the group are used "on demand" by whatever needs them?

> > @@ -760,8 +775,18 @@
> >                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
> >                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
> >                  <&audio_clk_a>, <&cs2000>,
> > -                <&audio_clk_c>,
> 
> Why remove it? This is the list of clock inputs, not outputs.

...probably because I was thinking the specification was exactly the other way
around.

Does a "clocks = ..." statement always mean input clocks?

I shall correct that and re-test (might take a while, I don't have the
hardware anymore).

Thanks for looking!
Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Geert Uytterhoeven]

Hi Alex,

On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 13:28:13 +0100:
> > On Mon, Jan 13, 2020 at 3:24 PM Alex Riesen
> > <alexander.riesen@cetitec.com> wrote:
> > > Not sure if all variants of the Salvator board have the HDMI decoder
> > > chip (the ADV7482) connected to the SSI4 on R-Car SoC, as it is on
> > > Salvator-X ES1, so the the ADV7482 endpoint and connection definitions
> > > are placed in the board file.
> >
> > Both Salvator-X and Salvator-XS have SSI4 wired to the ADV7482.
> >
> > > I do assume though that all Salvator variants have the CLK_C clock line
> > > hard-wired to the ADV7482 HDMI decoder, and remove it from the list of
> > > clocks provided by the R-Car sound system.
> >
> > Yes, both Salvator-X and Salvator-XS have it wired that way.
>
> Ok, seems like I can move that part into the common file as well.
> Integrations of ADV7482 and R-Car which use salvator-common.dts can still
> redefine the endpoint settings in their board files, right?

Indeed.

> > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > @@ -322,6 +322,10 @@
> > >         clock-frequency = <22579200>;
> > >  };
> > >
> > > +&audio_clk_c {
> > > +       clock-frequency = <12288000>;
> > > +};
> >
> > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > Or is this programmable?
>
> Oops. It looks like it is and the value is derived from the sampling rate
> (48kHz) and the master clock multiplier. Both hard-coded in the board file.

Where are these hardcoded in the board file?
Even if they are, technically this is a clock output of the ADC7482.

> > > video-receiver@70 {
> > >     compatible = "adi,adv7482";
> > > ...
> > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> >
> > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > it is an output.
>
> I would gladly give it right direction if I *really* understood what I was
> doing...

:-)

> > Furthermore, the DT bindings do not document that clocks can be specified.
>
> Should the DT bindings document that the clock cannot be specified than?

It currently does say so, as it doesn't list "clocks" in its properties section.

> > > @@ -686,7 +700,8 @@
> > >         };
> > >
> > >         sound_pins: sound {
> > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > +                        "ssi4_data";
> >
> > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
>
> I'll add them.
> As the device seems to function even without thoes, does this mean the pins in
> the group are used "on demand" by whatever needs them?

Probably the SCK4/WS4 functions are the reset-state defaults.

> > > @@ -760,8 +775,18 @@
> > >                  <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
> > >                  <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
> > >                  <&audio_clk_a>, <&cs2000>,
> > > -                <&audio_clk_c>,
> >
> > Why remove it? This is the list of clock inputs, not outputs.
>
> ...probably because I was thinking the specification was exactly the other way
> around.
>
> Does a "clocks = ..." statement always mean input clocks?

Yes it does.
If a device has clock outputs and is thus a clock provider, it should
have a #clock-cells property, and this should be documented in the bindings.

A clock consumer will refer to clocks of a provider using the "clocks"
property, specifying a clock specifier (phandle and zero or more indices)
for each clock referenced.

> I shall correct that and re-test (might take a while, I don't have the
> hardware anymore).

Oops.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100:
> On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > @@ -322,6 +322,10 @@
> > > >         clock-frequency = <22579200>;
> > > >  };
> > > >
> > > > +&audio_clk_c {
> > > > +       clock-frequency = <12288000>;
> > > > +};
> > >
> > > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > > Or is this programmable?
> >
> > Oops. It looks like it is and the value is derived from the sampling rate
> > (48kHz) and the master clock multiplier. Both hard-coded in the board file.
> 
> Where are these hardcoded in the board file?

In the endpoint definition, arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts

So the frequency can be set at the run-time, perhaps even derived from
endpoint connected to the output. In this case, rsnd_endpoint3,
which has the "mclk-fs" setting. Not sure if the sampling rate
can be set to something else for the HDMI, though.

> Even if they are, technically this is a clock output of the ADV7482.

... which I hope to correct as soon as I steal the hardware from whoever stole
it from me...

> > > > video-receiver@70 {
> > > >     compatible = "adi,adv7482";
> > > > ...
> > > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> > >
> > > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > > it is an output.
> >
> > I would gladly give it right direction if I *really* understood what I was
> > doing...
> 
> :-)
> 
> > > Furthermore, the DT bindings do not document that clocks can be specified.
> >
> > Should the DT bindings document that the clock cannot be specified than?
> 
> It currently does say so, as it doesn't list "clocks" in its properties section.

The bindings documentation file, which we're talking about here and which does
not list the specifiable input clocks in its properties, is it the

    Documentation/devicetree/bindings/media/i2c/adv748x.txt

?

And this absence of documentation also means that whatever clocks (both input
in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
an integration detail?

Does this below makes more sense, than?

    video-receiver@70 {
        compatible = "adi,adv7482";
        clocks = <&rcar_sound 3>;
        clock-names = "clk-hdmi-video";
        adv748x_mclk: mclk {
            compatible = "fixed-clock";
            #clock-cells =  <0>;
            /* frequency hard-coded for illustration */
            clock-frequency = <12288000>;
            clock-output-names = "clk-hdmi-i2s-mclk";
        };
    };

Now I'm a bit hazy on how to declare that the MCLK output of the
video-receiver@70 is connected to the Audio Clock C of the SoC...
Probably remove use of "audio_clk_c" completely?

> > > > @@ -686,7 +700,8 @@
> > > >         };
> > > >
> > > >         sound_pins: sound {
> > > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > > +                        "ssi4_data";
> > >
> > > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
> >
> > I'll add them.
> > As the device seems to function even without thoes, does this mean the
> > pins in the group are used "on demand" by whatever needs them?
> 
> Probably the SCK4/WS4 functions are the reset-state defaults.

That ... might require some trial and testing: when I add them to the group,
the reset defaults will be overridden by the platform initialization, which is
not necessarily the reset default. Will see.

> >
> > Does a "clocks = ..." statement always mean input clocks?
> 
> Yes it does.
> If a device has clock outputs and is thus a clock provider, it should
> have a #clock-cells property, and this should be documented in the bindings.
> 
> A clock consumer will refer to clocks of a provider using the "clocks"
> property, specifying a clock specifier (phandle and zero or more indices)
> for each clock referenced.

Something like this?

    &rcar_sound {
        clocks = ...,
                 <&adv748x_mclk>,
                 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
        clock-names = ...,
                      "clk_c",
                      "clk_i";
    };

Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Geert Uytterhoeven]

Hi Alex,

On Mon, Mar 2, 2020 at 4:07 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 14:47:46 +0100:
> > On Mon, Mar 2, 2020 at 2:40 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > > --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > > +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> > > > > @@ -322,6 +322,10 @@
> > > > >         clock-frequency = <22579200>;
> > > > >  };
> > > > >
> > > > > +&audio_clk_c {
> > > > > +       clock-frequency = <12288000>;
> > > > > +};
> > > >
> > > > Does the ADV7482 always generate a 12.288 MHz clock signal?
> > > > Or is this programmable?
> > >
> > > Oops. It looks like it is and the value is derived from the sampling rate
> > > (48kHz) and the master clock multiplier. Both hard-coded in the board file.
> >
> > Where are these hardcoded in the board file?
>
> In the endpoint definition, arch/arm64/boot/dts/renesas/r8a7795-es1-salvator-x.dts
>
> So the frequency can be set at the run-time, perhaps even derived from
> endpoint connected to the output. In this case, rsnd_endpoint3,
> which has the "mclk-fs" setting. Not sure if the sampling rate
> can be set to something else for the HDMI, though.
>
> > Even if they are, technically this is a clock output of the ADV7482.
>
> ... which I hope to correct as soon as I steal the hardware from whoever stole
> it from me...
>
> > > > > video-receiver@70 {
> > > > >     compatible = "adi,adv7482";
> > > > > ...
> > > > > +   clocks = <&rcar_sound 3>, <&audio_clk_c>;
> > > > > +   clock-names = "clk-hdmi-video", "clk-hdmi-i2s-mclk";
> > > >
> > > > The above declares the Audio CLK C to be a clock input of the ADV7482, while
> > > > it is an output.
> > >
> > > I would gladly give it right direction if I *really* understood what I was
> > > doing...
> >
> > :-)
> >
> > > > Furthermore, the DT bindings do not document that clocks can be specified.
> > >
> > > Should the DT bindings document that the clock cannot be specified than?
> >
> > It currently does say so, as it doesn't list "clocks" in its properties section.
>
> The bindings documentation file, which we're talking about here and which does
> not list the specifiable input clocks in its properties, is it the
>
>     Documentation/devicetree/bindings/media/i2c/adv748x.txt
>
> ?

Yes.

>
> And this absence of documentation also means that whatever clocks (both input
> in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> an integration detail?

No, the absence probably means that any clock-related properties in a .dts
file will just be ignored.

Looking at the driver source, it indeed has no support related to clocks at all.

> Does this below makes more sense, than?
>
>     video-receiver@70 {
>         compatible = "adi,adv7482";
>         clocks = <&rcar_sound 3>;
>         clock-names = "clk-hdmi-video";
>         adv748x_mclk: mclk {
>             compatible = "fixed-clock";
>             #clock-cells =  <0>;
>             /* frequency hard-coded for illustration */
>             clock-frequency = <12288000>;
>             clock-output-names = "clk-hdmi-i2s-mclk";
>         };
>     };

The #clock-cells should be in the main video-receiver node.
Probably there is more than one clock output, so #clock-cells may be 1?
There is no need for a fixed-clock compatible, nor for clock-frequency
and clock-output-names.

But most important: this should be documented in the adv748x DT bindings,
and implemented in the adv748x driver.

> Now I'm a bit hazy on how to declare that the MCLK output of the
> video-receiver@70 is connected to the Audio Clock C of the SoC...
> Probably remove use of "audio_clk_c" completely?

Yes, the current audio_clk_c definition in the DTS assumes a fixed
crystal.

> > > > > @@ -686,7 +700,8 @@
> > > > >         };
> > > > >
> > > > >         sound_pins: sound {
> > > > > -               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> > > > > +               groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> > > > > +                        "ssi4_data";
> > > >
> > > > Missing "ss4_ctrl", for the SCK4 and WS4 pins.
> > >
> > > I'll add them.
> > > As the device seems to function even without thoes, does this mean the
> > > pins in the group are used "on demand" by whatever needs them?
> >
> > Probably the SCK4/WS4 functions are the reset-state defaults.
>
> That ... might require some trial and testing: when I add them to the group,
> the reset defaults will be overridden by the platform initialization, which is
> not necessarily the reset default. Will see.

Or by the boot loader.  Anyway, you need to specify these in the DTS.

> > > Does a "clocks = ..." statement always mean input clocks?
> >
> > Yes it does.
> > If a device has clock outputs and is thus a clock provider, it should
> > have a #clock-cells property, and this should be documented in the bindings.
> >
> > A clock consumer will refer to clocks of a provider using the "clocks"
> > property, specifying a clock specifier (phandle and zero or more indices)
> > for each clock referenced.
>
> Something like this?
>
>     &rcar_sound {
>         clocks = ...,
>                  <&adv748x_mclk>,
>                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
>         clock-names = ...,
>                       "clk_c",
>                       "clk_i";
>     };

More or less.

Might become

    find_a_better_label_choice: video-receiver@70 {
            ...
    };

    &rcar_sound {
            clock = ...,
                    <&find_a_better_label_choice 0>,
                    ...
    };

as there may be multiple clock outputs on the ADV7482.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > And this absence of documentation also means that whatever clocks (both input
> > in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> > an integration detail?
> 
> No, the absence probably means that any clock-related properties in a .dts
> file will just be ignored.
> 
> Looking at the driver source, it indeed has no support related to clocks at all.

...

> > Does this below makes more sense, than?
> >
> >     video-receiver@70 {
> >         compatible = "adi,adv7482";
> >         clocks = <&rcar_sound 3>;
> >         clock-names = "clk-hdmi-video";
> >         adv748x_mclk: mclk {
> >             compatible = "fixed-clock";
> >             #clock-cells =  <0>;
> >             /* frequency hard-coded for illustration */
> >             clock-frequency = <12288000>;
> >             clock-output-names = "clk-hdmi-i2s-mclk";
> >         };
> >     };
> 
> The #clock-cells should be in the main video-receiver node.
> Probably there is more than one clock output, so #clock-cells may be 1?

AFAICS, the device can provide only this one clock line (audio master clock
for I2S output)... I shall re-check, just in case.

> There is no need for a fixed-clock compatible, nor for clock-frequency
> and clock-output-names.
> 
> But most important: this should be documented in the adv748x DT bindings,
> and implemented in the adv748x driver.

So if the driver is to export that clock for the kernel (like in this case),
it must implement its support?

> > > > Does a "clocks = ..." statement always mean input clocks?
> > >
> > > Yes it does.
> > > If a device has clock outputs and is thus a clock provider, it should
> > > have a #clock-cells property, and this should be documented in the bindings.
> > >
> > > A clock consumer will refer to clocks of a provider using the "clocks"
> > > property, specifying a clock specifier (phandle and zero or more indices)
> > > for each clock referenced.
> >
> > Something like this?
> >
> >     &rcar_sound {
> >         clocks = ...,
> >                  <&adv748x_mclk>,
> >                  <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> >         clock-names = ...,
> >                       "clk_c",
> >                       "clk_i";
> >     };
> 
> More or less.
> 
> Might become
> 
>     find_a_better_label_choice: video-receiver@70 {
>             ...
>     };
> 
>     &rcar_sound {
>             clock = ...,
>                     <&find_a_better_label_choice 0>,
>                     ...
>     };
> 
> as there may be multiple clock outputs on the ADV7482.

I see. Working on it.

Thanks a lot!

Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Geert Uytterhoeven]

Hi Alex,

On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > And this absence of documentation also means that whatever clocks (both input
> > > in "clocks=" and output in "#clock-cells") listed in a specific .dts are just
> > > an integration detail?
> >
> > No, the absence probably means that any clock-related properties in a .dts
> > file will just be ignored.
> >
> > Looking at the driver source, it indeed has no support related to clocks at all.
>
> ...
>
> > > Does this below makes more sense, than?
> > >
> > >     video-receiver@70 {
> > >         compatible = "adi,adv7482";
> > >         clocks = <&rcar_sound 3>;
> > >         clock-names = "clk-hdmi-video";
> > >         adv748x_mclk: mclk {
> > >             compatible = "fixed-clock";
> > >             #clock-cells =  <0>;
> > >             /* frequency hard-coded for illustration */
> > >             clock-frequency = <12288000>;
> > >             clock-output-names = "clk-hdmi-i2s-mclk";
> > >         };
> > >     };
> >
> > The #clock-cells should be in the main video-receiver node.
> > Probably there is more than one clock output, so #clock-cells may be 1?
>
> AFAICS, the device can provide only this one clock line (audio master clock
> for I2S output)... I shall re-check, just in case.
>
> > There is no need for a fixed-clock compatible, nor for clock-frequency
> > and clock-output-names.
> >
> > But most important: this should be documented in the adv748x DT bindings,
> > and implemented in the adv748x driver.
>
> So if the driver is to export that clock for the kernel (like in this case),
> it must implement its support?

Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
you can just override the existing audio_clk_c rate.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > >
> > > The #clock-cells should be in the main video-receiver node.
> > > Probably there is more than one clock output, so #clock-cells may be 1?
> >
> > AFAICS, the device can provide only this one clock line (audio master clock
> > for I2S output)... I shall re-check, just in case.

And you're right, of course: the audio output formatting module of the ADV748x
devices provides a set of clock lines related to the I2S pins: the already
discussed master clock, left-right channel clock and the serial clock (bit
clock?).

> > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > and clock-output-names.
> > >
> > > But most important: this should be documented in the adv748x DT bindings,
> > > and implemented in the adv748x driver.
> >
> > So if the driver is to export that clock for the kernel (like in this case),
> > it must implement its support?
> 
> Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> you can just override the existing audio_clk_c rate.

Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
clock in the driver, added a clock provider:

adv748x_probe:

    clk = clk_register_fixed_rate(state->dev,
				  "clk-hdmi-i2s-mclk",
				  NULL     /* parent_name */,
				  0        /* flags */,
				  12288000 /* rate */);
    of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);

And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
in the list of input clocks of the R-Car-side sound card with the phandle of
the adv7482 main node:

salvator-common.dtsi:

    &i2c4 {
	status = "okay";

	adv7482_hdmi_decoder: video-receiver@70 {
	    #clock-cells = <0>; // to be replaced with <1>
	};
    };

    &rcar_sound {
	clocks = ..., <&adv7482_hdmi_decoder>, ...;
    };

As everything continues to work as before, I assume that at least the clock
dependencies were resolved.

Is there a way to verify that the added input clock is actually used?
IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
receiving hardware) registers, and not just a left-over from previuos attempts
or plain default setting?

As the ADV748x devices seem to provide also the clocks for video outputs, will
it make any sense to place the clock definition into the port node?
Or should all provided clocks be indexed in the main device node?

Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Geert Uytterhoeven]

Hi Alex,

On Thu, Mar 5, 2020 at 3:36 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > >
> > > > The #clock-cells should be in the main video-receiver node.
> > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > >
> > > AFAICS, the device can provide only this one clock line (audio master clock
> > > for I2S output)... I shall re-check, just in case.
>
> And you're right, of course: the audio output formatting module of the ADV748x
> devices provides a set of clock lines related to the I2S pins: the already
> discussed master clock, left-right channel clock and the serial clock (bit
> clock?).
>
> > > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > > and clock-output-names.
> > > >
> > > > But most important: this should be documented in the adv748x DT bindings,
> > > > and implemented in the adv748x driver.
> > >
> > > So if the driver is to export that clock for the kernel (like in this case),
> > > it must implement its support?
> >
> > Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> > you can just override the existing audio_clk_c rate.
>
> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> clock in the driver, added a clock provider:
>
> adv748x_probe:
>
>     clk = clk_register_fixed_rate(state->dev,
>                                   "clk-hdmi-i2s-mclk",
>                                   NULL     /* parent_name */,
>                                   0        /* flags */,
>                                   12288000 /* rate */);
>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
>
> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> in the list of input clocks of the R-Car-side sound card with the phandle of
> the adv7482 main node:
>
> salvator-common.dtsi:
>
>     &i2c4 {
>         status = "okay";
>
>         adv7482_hdmi_decoder: video-receiver@70 {
>             #clock-cells = <0>; // to be replaced with <1>
>         };
>     };
>
>     &rcar_sound {
>         clocks = ..., <&adv7482_hdmi_decoder>, ...;
>     };
>
> As everything continues to work as before, I assume that at least the clock
> dependencies were resolved.
>
> Is there a way to verify that the added input clock is actually used?
> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> receiving hardware) registers, and not just a left-over from previuos attempts
> or plain default setting?

Have a look at /sys/kernel/debug/clk/clk_summary

> As the ADV748x devices seem to provide also the clocks for video outputs, will
> it make any sense to place the clock definition into the port node?
> Or should all provided clocks be indexed in the main device node?

Sorry, I don't know.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Laurent Pinchart]

Hi Alex,

On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > >
> > > > The #clock-cells should be in the main video-receiver node.
> > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > >
> > > AFAICS, the device can provide only this one clock line (audio master clock
> > > for I2S output)... I shall re-check, just in case.
> 
> And you're right, of course: the audio output formatting module of the ADV748x
> devices provides a set of clock lines related to the I2S pins: the already
> discussed master clock, left-right channel clock and the serial clock (bit
> clock?).

I don't think we need to model the last two clocks through CCF though,
they're part of the I2S protocol, not clock sources that need to be
explicitly controlled (or queried).

> > > > There is no need for a fixed-clock compatible, nor for clock-frequency
> > > > and clock-output-names.
> > > >
> > > > But most important: this should be documented in the adv748x DT bindings,
> > > > and implemented in the adv748x driver.
> > >
> > > So if the driver is to export that clock for the kernel (like in this case),
> > > it must implement its support?
> > 
> > Exactly.  Unless that pin is hardcoded to output a fixed clock, in which case
> > you can just override the existing audio_clk_c rate.
> 
> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> clock in the driver, added a clock provider:
> 
> adv748x_probe:
> 
>     clk = clk_register_fixed_rate(state->dev,
> 				  "clk-hdmi-i2s-mclk",
> 				  NULL     /* parent_name */,
> 				  0        /* flags */,
> 				  12288000 /* rate */);
>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> 
> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> in the list of input clocks of the R-Car-side sound card with the phandle of
> the adv7482 main node:
> 
> salvator-common.dtsi:
> 
>     &i2c4 {
> 	status = "okay";
> 
> 	adv7482_hdmi_decoder: video-receiver@70 {
> 	    #clock-cells = <0>; // to be replaced with <1>
> 	};
>     };
> 
>     &rcar_sound {
> 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
>     };
> 
> As everything continues to work as before, I assume that at least the clock
> dependencies were resolved.

This looks good to me.

> Is there a way to verify that the added input clock is actually used?
> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> receiving hardware) registers, and not just a left-over from previuos attempts
> or plain default setting?
> 
> As the ADV748x devices seem to provide also the clocks for video outputs, will
> it make any sense to place the clock definition into the port node?
> Or should all provided clocks be indexed in the main device node?

Those clocks are part of the CSI-2 protocol and also don't need to be
explicitly controlled. As far as I can tell from a quick check of the
ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
needs to be exposed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Hi Laurent,

Laurent Pinchart, Fri, Mar 06, 2020 14:16:32 +0100:
> On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> > Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> > > On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> > > > >
> > > > > The #clock-cells should be in the main video-receiver node.
> > > > > Probably there is more than one clock output, so #clock-cells may be 1?
> > > >
> > > > AFAICS, the device can provide only this one clock line (audio master clock
> > > > for I2S output)... I shall re-check, just in case.
> > 
> > And you're right, of course: the audio output formatting module of the ADV748x
> > devices provides a set of clock lines related to the I2S pins: the already
> > discussed master clock, left-right channel clock and the serial clock (bit
> > clock?).
> 
> I don't think we need to model the last two clocks through CCF though,
> they're part of the I2S protocol, not clock sources that need to be
> explicitly controlled (or queried).

That's good, because I'm right now having hard time finding out how to
calculate the frequencies!

> > Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> > clock in the driver, added a clock provider:
> > 
> > adv748x_probe:
> > 
> >     clk = clk_register_fixed_rate(state->dev,
> > 				  "clk-hdmi-i2s-mclk",
> > 				  NULL     /* parent_name */,
> > 				  0        /* flags */,
> > 				  12288000 /* rate */);
> >     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> > 
> > And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> > in the list of input clocks of the R-Car-side sound card with the phandle of
> > the adv7482 main node:
> > 
> > salvator-common.dtsi:
> > 
> >     &i2c4 {
> > 	status = "okay";
> > 
> > 	adv7482_hdmi_decoder: video-receiver@70 {
> > 	    #clock-cells = <0>; // to be replaced with <1>
> > 	};
> >     };
> > 
> >     &rcar_sound {
> > 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
> >     };
> > 
> > As everything continues to work as before, I assume that at least the clock
> > dependencies were resolved.
> 
> This looks good to me.

Ok, I settle on this than.

> > Is there a way to verify that the added input clock is actually used?
> > IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> > receiving hardware) registers, and not just a left-over from previuos attempts
> > or plain default setting?
> > 
> > As the ADV748x devices seem to provide also the clocks for video outputs, will
> > it make any sense to place the clock definition into the port node?
> > Or should all provided clocks be indexed in the main device node?
> 
> Those clocks are part of the CSI-2 protocol and also don't need to be
> explicitly controlled. As far as I can tell from a quick check of the
> ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> needs to be exposed.

Thanks, that's good to know!

Do you know, by chance, which of the snd_soc* callbacks should be used to
implement setting of the MCLK? The one in snd_soc_component_driver or
snd_soc_dai_driver->ops (snd_soc_dai_ops)?

Or how the userspace interface looks like? Or, if there is no userspace
interface for this, how the MCLK is supposed to be set? Through mclk-fs?

Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Laurent Pinchart]

Hi Alex,

(CC'ing Morimoto-san)

On Fri, Mar 06, 2020 at 02:41:54PM +0100, Alex Riesen wrote:
> Laurent Pinchart, Fri, Mar 06, 2020 14:16:32 +0100:
> > On Thu, Mar 05, 2020 at 03:36:28PM +0100, Alex Riesen wrote:
> >> Geert Uytterhoeven, Mon, Mar 02, 2020 17:13:30 +0100:
> >>> On Mon, Mar 2, 2020 at 5:09 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> >>>> Geert Uytterhoeven, Mon, Mar 02, 2020 16:32:32 +0100:
> >>>>>
> >>>>> The #clock-cells should be in the main video-receiver node.
> >>>>> Probably there is more than one clock output, so #clock-cells may be 1?
> >>>>
> >>>> AFAICS, the device can provide only this one clock line (audio master clock
> >>>> for I2S output)... I shall re-check, just in case.
> >> 
> >> And you're right, of course: the audio output formatting module of the ADV748x
> >> devices provides a set of clock lines related to the I2S pins: the already
> >> discussed master clock, left-right channel clock and the serial clock (bit
> >> clock?).
> > 
> > I don't think we need to model the last two clocks through CCF though,
> > they're part of the I2S protocol, not clock sources that need to be
> > explicitly controlled (or queried).
> 
> That's good, because I'm right now having hard time finding out how to
> calculate the frequencies!
> 
> >> Just to try it out (I'll set #clock-cells to 1), I registered a fixed rate
> >> clock in the driver, added a clock provider:
> >> 
> >> adv748x_probe:
> >> 
> >>     clk = clk_register_fixed_rate(state->dev,
> >> 				  "clk-hdmi-i2s-mclk",
> >> 				  NULL     /* parent_name */,
> >> 				  0        /* flags */,
> >> 				  12288000 /* rate */);
> >>     of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get, clk);
> >> 
> >> And removed the audio_clk_c frequency setting. I also replaced the audio_clk_c
> >> in the list of input clocks of the R-Car-side sound card with the phandle of
> >> the adv7482 main node:
> >> 
> >> salvator-common.dtsi:
> >> 
> >>     &i2c4 {
> >> 	status = "okay";
> >> 
> >> 	adv7482_hdmi_decoder: video-receiver@70 {
> >> 	    #clock-cells = <0>; // to be replaced with <1>
> >> 	};
> >>     };
> >> 
> >>     &rcar_sound {
> >> 	clocks = ..., <&adv7482_hdmi_decoder>, ...;
> >>     };
> >> 
> >> As everything continues to work as before, I assume that at least the clock
> >> dependencies were resolved.
> > 
> > This looks good to me.
> 
> Ok, I settle on this than.
> 
> >> Is there a way to verify that the added input clock is actually used?
> >> IOW, if its frequency is actually has been programmed into the ssi4 (R-Car
> >> receiving hardware) registers, and not just a left-over from previuos attempts
> >> or plain default setting?
> >> 
> >> As the ADV748x devices seem to provide also the clocks for video outputs, will
> >> it make any sense to place the clock definition into the port node?
> >> Or should all provided clocks be indexed in the main device node?
> > 
> > Those clocks are part of the CSI-2 protocol and also don't need to be
> > explicitly controlled. As far as I can tell from a quick check of the
> > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > needs to be exposed.
> 
> Thanks, that's good to know!
> 
> Do you know, by chance, which of the snd_soc* callbacks should be used to
> implement setting of the MCLK? The one in snd_soc_component_driver or
> snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> 
> Or how the userspace interface looks like? Or, if there is no userspace
> interface for this, how the MCLK is supposed to be set? Through mclk-fs?

I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
is the main developer and maintainer of Renesas sound drivers.
Morimoto-sensei, would you have an answer to that question ? :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Kuninori Morimoto]

Hi

> > > Those clocks are part of the CSI-2 protocol and also don't need to be
> > > explicitly controlled. As far as I can tell from a quick check of the
> > > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > > needs to be exposed.
(snip)
> > Do you know, by chance, which of the snd_soc* callbacks should be used to
> > implement setting of the MCLK? The one in snd_soc_component_driver or
> > snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> > 
> > Or how the userspace interface looks like? Or, if there is no userspace
> > interface for this, how the MCLK is supposed to be set? Through mclk-fs?
> 
> I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
> is the main developer and maintainer of Renesas sound drivers.
> Morimoto-sensei, would you have an answer to that question ? :-)

In my quick check, it goes to AUDIO_CLKC.
If so, you can update rcar_sound::clocks.

	&rcar_sound {
		...
-		/* update <audio_clk_b> to <cs2000> */
+		/* update <audio_clk_b> to <cs2000>,
+		 *        <audio_clk_c> to <adv748x> */
		clocks = <&cpg CPG_MOD 1005>,
			...
			 <&audio_clk_a>, <&cs2000>,
-			 <&audio_clk_c>,
+			 <&adv748x>,
			 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Hi,

Kuninori Morimoto, Mon, Mar 09, 2020 02:31:01 +0100:
> > > > Those clocks are part of the CSI-2 protocol and also don't need to be
> > > > explicitly controlled. As far as I can tell from a quick check of the
> > > > ADV7482 documentation, only the I2S MCLK is a general-purpose clock that
> > > > needs to be exposed.
> (snip)
> > > Do you know, by chance, which of the snd_soc* callbacks should be used to
> > > implement setting of the MCLK? The one in snd_soc_component_driver or
> > > snd_soc_dai_driver->ops (snd_soc_dai_ops)?
> > > 
> > > Or how the userspace interface looks like? Or, if there is no userspace
> > > interface for this, how the MCLK is supposed to be set? Through mclk-fs?
> > 
> > I'm afraid my knowledge of the sound subsystem is limited. Morimoto-san
> > is the main developer and maintainer of Renesas sound drivers.
> > Morimoto-sensei, would you have an answer to that question ? :-)
> 
> In my quick check, it goes to AUDIO_CLKC.
> If so, you can update rcar_sound::clocks.
> 
> 	&rcar_sound {
> 		...
> -		/* update <audio_clk_b> to <cs2000> */
> +		/* update <audio_clk_b> to <cs2000>,
> +		 *        <audio_clk_c> to <adv748x> */
> 		clocks = <&cpg CPG_MOD 1005>,
> 			...
> 			 <&audio_clk_a>, <&cs2000>,
> -			 <&audio_clk_c>,
> +			 <&adv748x>,
> 			 <&cpg CPG_CORE CPG_AUDIO_CLK_I>;
> 
> Thank you for your help !!

Thanks.

Should the adv748x driver also implement anything to configure the frequency
of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
snd_soc_dai_ops?

Or is the driver implementation, which depends on mclk-fs to be 256, the audio
stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
the HDMI input, a totally acceptable first attempt at writing a DAI driver?

I'm a bit bothered by that, as the hardware is also capable of decoding
stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
formats, 128-768fs MCLK multipliers, and a row of I2S options.

I just find it confusing to place the configuration interfaces.
For instance, the patches use the media ioctl for audio output selection to
select I2S protocol. While works, it does not feel right (shouldn't it be in
the device tree?)

Maybe you can point me at a driver doing something similar? I'm studying media
drivers now, but not many of them use ASoC interfaces for devices providing a
clock. Or maybe I should better look at sound/soc/...?

Thanks in advance,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Kuninori Morimoto]

Hi Alex

> Should the adv748x driver also implement anything to configure the frequency
> of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> snd_soc_dai_ops?
> 
> Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> 
> I'm a bit bothered by that, as the hardware is also capable of decoding
> stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> formats, 128-768fs MCLK multipliers, and a row of I2S options.
> 
> I just find it confusing to place the configuration interfaces.
> For instance, the patches use the media ioctl for audio output selection to
> select I2S protocol. While works, it does not feel right (shouldn't it be in
> the device tree?)
> 
> Maybe you can point me at a driver doing something similar? I'm studying media
> drivers now, but not many of them use ASoC interfaces for devices providing a
> clock. Or maybe I should better look at sound/soc/...?

Setting Sound Clock for all cases/patterns are very complex and difficult actually.
(ADV7482 configuration) x (ADG divider / selector) x etc, etc...

Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
route clock (= no configuration, fixed clock), and ADG divides it,
and provide best clock to each SSIx.

Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
Or providing specific clock for some case is enough
(ADG will automatically select it if necessary).

If ADV7482 needs more detail clock settings combination,
then, there is no method to adjust to it.
We need to consider such system somehow.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Alex Riesen]

Hello Morimoto-san,

Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:
> > Should the adv748x driver also implement anything to configure the frequency
> > of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> > snd_soc_dai_ops?
> > 
> > Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> > stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> > the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> > 
> > I'm a bit bothered by that, as the hardware is also capable of decoding
> > stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> > formats, 128-768fs MCLK multipliers, and a row of I2S options.
> > 
> > I just find it confusing to place the configuration interfaces.
> > For instance, the patches use the media ioctl for audio output selection to
> > select I2S protocol. While works, it does not feel right (shouldn't it be in
> > the device tree?)
> > 
> > Maybe you can point me at a driver doing something similar? I'm studying media
> > drivers now, but not many of them use ASoC interfaces for devices providing a
> > clock. Or maybe I should better look at sound/soc/...?
> 
> Setting Sound Clock for all cases/patterns are very complex and difficult actually.
> (ADV7482 configuration) x (ADG divider / selector) x etc, etc...
> 
> Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
> route clock (= no configuration, fixed clock), and ADG divides it,
> and provide best clock to each SSIx.
> Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
> and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
> Or providing specific clock for some case is enough
> (ADG will automatically select it if necessary).

In this particular case, the ADV7482 *must* provide the clock, I believe: it
extracts the audio stream from the HDMI connection (in addition to everything
else) and serves the stream on I2S. Its MCLK line is physically connected to
the CLK_C line (which is an input) of the R-Car SoC. The I2S audio
transmission does not work if the ADV7482 clock is not programmed (or
programmed incorrectly).
Yes, I tried (I also tried programming it incorrectly, just because I didn't
know what I was doing).

> If ADV7482 needs more detail clock settings combination,
> then, there is no method to adjust to it.
> We need to consider such system somehow.

Not encouraging...

Maybe I should leave the clock fixed, with the frequency configuration in the
device tree, e.g. as adv7482 port node property "clock-frequency".
Which feels rather pathetic, but at least serves my purpose (48k, 8x24).

But let me describe the situation as I see it first.

As far as I understand, the SSI4 (Salvator-X board) should be programmed by
the snd-soc-rcar driver in the "slave receiver" mode for this use case, which
is HDMI input ADV7482 (I2S master, TDM) -> SSI4 (I2S slave)):

[   63.305990] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: rsnd-dai.1 : sysclk = 66666664, direction 0
[   63.306028] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: adv748x-i2s : sysclk = 12288000, direction 1

I am a bit bothered by the fact that sysclk of rsnd-dai.1 does not match that
sysclk of adv7482-i2s, but I think it's just DT node configuration.

[   63.306033] asoc_simple_card_set_dailink_name: asoc-audio-graph-card sound: name : rsnd-dai.1-adv748x-i2s
...
[   63.332641] asoc-audio-graph-card sound: adv748x.4-0070 <-> rsnd-dai.1 mapping ok
...
[   63.341317] dapm_connect_dai_link_widgets:  rsnd-dai.1-adv748x-i2s: connected DAI link adv748x.4-0070:Capture -> ec500000.sound:DAI1 Capture
...
[  128.961389] rsnd_write: rcar_sound ec500000.sound: w ssi[4] - SSICR ( 124) : 9ceb0100

Decoding this last line (9ceb0100) gives SSICR.TRMD (bit1) =0, SSICR.SCKD
(bit15) =0, SSICR.SWSD (bit14) =0. The combination is documented as "slave
receiver". Which, I assume, makes SSI4 use the external clock. Given the
received stream looks ok, something also must have set the dividers correctly.

From the above, I conclude, whatever the complexity of the audio system clock
configuration, it seems to be implemented for the case.

I only miss a more or less clear way to configure the I2S master (ADV7482, that is).

Regards,
Alex

Re: [PATCH 8/8] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC

[Laurent Pinchart]

Hi Alex,

On Tue, Mar 10, 2020 at 09:17:14AM +0100, Alex Riesen wrote:
> Kuninori Morimoto, Tue, Mar 10, 2020 02:07:23 +0100:
> > > Should the adv748x driver also implement anything to configure the frequency
> > > of MCLK clock? I mean something like .set_sysclk and .set_fmt callbacks of
> > > snd_soc_dai_ops?
> > > 
> > > Or is the driver implementation, which depends on mclk-fs to be 256, the audio
> > > stream format to be 8x S24_LE, and requires strictly 48kHz sampling rate on
> > > the HDMI input, a totally acceptable first attempt at writing a DAI driver?
> > > 
> > > I'm a bit bothered by that, as the hardware is also capable of decoding
> > > stereo, sampling rate 32-192kHz, a variety of PCM and compressed/encrypted
> > > formats, 128-768fs MCLK multipliers, and a row of I2S options.
> > > 
> > > I just find it confusing to place the configuration interfaces.
> > > For instance, the patches use the media ioctl for audio output selection to
> > > select I2S protocol. While works, it does not feel right (shouldn't it be in
> > > the device tree?)
> > > 
> > > Maybe you can point me at a driver doing something similar? I'm studying media
> > > drivers now, but not many of them use ASoC interfaces for devices providing a
> > > clock. Or maybe I should better look at sound/soc/...?
> > 
> > Setting Sound Clock for all cases/patterns are very complex and difficult actually.
> > (ADV7482 configuration) x (ADG divider / selector) x etc, etc...
> > 
> > Thus, Current R-Car sound is assuming that audio_clk_a/b/c/i are providing
> > route clock (= no configuration, fixed clock), and ADG divides it,
> > and provide best clock to each SSIx.
> > Current Salvator/ULCB already have 44.1/48kHz route clock (= CS2000 and Audio_CLK_A),
> > and we can reuse it for all SSIx. Thus, ADV7482 clock is not necessary, I guess ?
> > Or providing specific clock for some case is enough
> > (ADG will automatically select it if necessary).
> 
> In this particular case, the ADV7482 *must* provide the clock, I believe: it
> extracts the audio stream from the HDMI connection (in addition to everything
> else) and serves the stream on I2S. Its MCLK line is physically connected to
> the CLK_C line (which is an input) of the R-Car SoC. The I2S audio
> transmission does not work if the ADV7482 clock is not programmed (or
> programmed incorrectly).
> Yes, I tried (I also tried programming it incorrectly, just because I didn't
> know what I was doing).
> 
> > If ADV7482 needs more detail clock settings combination,
> > then, there is no method to adjust to it.
> > We need to consider such system somehow.
> 
> Not encouraging...
> 
> Maybe I should leave the clock fixed, with the frequency configuration in the
> device tree, e.g. as adv7482 port node property "clock-frequency".
> Which feels rather pathetic, but at least serves my purpose (48k, 8x24).
> 
> But let me describe the situation as I see it first.
> 
> As far as I understand, the SSI4 (Salvator-X board) should be programmed by
> the snd-soc-rcar driver in the "slave receiver" mode for this use case, which
> is HDMI input ADV7482 (I2S master, TDM) -> SSI4 (I2S slave)):
> 
> [   63.305990] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: rsnd-dai.1 : sysclk = 66666664, direction 0
> [   63.306028] asoc_simple_card_parse_clk: asoc-audio-graph-card sound: adv748x-i2s : sysclk = 12288000, direction 1
> 
> I am a bit bothered by the fact that sysclk of rsnd-dai.1 does not match that
> sysclk of adv7482-i2s, but I think it's just DT node configuration.
> 
> [   63.306033] asoc_simple_card_set_dailink_name: asoc-audio-graph-card sound: name : rsnd-dai.1-adv748x-i2s
> ...
> [   63.332641] asoc-audio-graph-card sound: adv748x.4-0070 <-> rsnd-dai.1 mapping ok
> ...
> [   63.341317] dapm_connect_dai_link_widgets:  rsnd-dai.1-adv748x-i2s: connected DAI link adv748x.4-0070:Capture -> ec500000.sound:DAI1 Capture
> ...
> [  128.961389] rsnd_write: rcar_sound ec500000.sound: w ssi[4] - SSICR ( 124) : 9ceb0100
> 
> Decoding this last line (9ceb0100) gives SSICR.TRMD (bit1) =0, SSICR.SCKD
> (bit15) =0, SSICR.SWSD (bit14) =0. The combination is documented as "slave
> receiver". Which, I assume, makes SSI4 use the external clock. Given the
> received stream looks ok, something also must have set the dividers correctly.
> 
> From the above, I conclude, whatever the complexity of the audio system clock
> configuration, it seems to be implemented for the case.
> 
> I only miss a more or less clear way to configure the I2S master (ADV7482, that is).

As a stop-gap measure, until the sound driver programs the clock, you
can set its frequency in DT with the assigned-clock-rates property.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] media: adv748x: add audio mute control and output selection ioctls

[Hans Verkuil]

Hi Alex,

I apologize for the (very) slow reply, but better late than never.

On 1/13/20 3:15 PM, Alex Riesen wrote:
> This change implements audio-related V4L2 ioctls for the HDMI subdevice.

This is really where things go wrong. These V4L2 audio ioctls are meant for
old PCI TV tuner devices where the audio was implemented as audio jack outputs
that are typically looped back to audio inputs on a (PCI) soundcard. And when
these ioctls were designed ALSA didn't even exist.

None of that applies here.

Generally an hdmi driver will configure the i2s audio automatically, which is
typically connected to the SoC and controlled by the ALSA driver of the SoC,
but there may well be missing features (audio never got a lot of attention in
hdmi receivers). So what I would like to know is: what features are missing?

Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls,
which would be the right approach for this.

So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be
added here:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html

Regards,

	Hans

> 
> The master audio clock is configured for 256fs, as supported by the only
> device available at the moment. For the same reason, the TDM slot is
> formatted using left justification of its bits.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c |   6 +
>  drivers/media/i2c/adv748x/adv748x-hdmi.c | 182 +++++++++++++++++++++++
>  drivers/media/i2c/adv748x/adv748x.h      |  42 ++++++
>  3 files changed, 230 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index bc49aa93793c..b6067ffb1e0d 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -150,6 +150,12 @@ static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
>  	return *error;
>  }
>  
> +int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg, u8 mask,
> +			u8 value)
> +{
> +	return regmap_update_bits(state->regmap[page], reg, mask, value);
> +}
> +
>  /* adv748x_write_block(): Write raw data with a maximum of I2C_SMBUS_BLOCK_MAX
>   * size to one or more registers.
>   *
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> index c557f8fdf11a..9bc9237c9116 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2017 Renesas Electronics Corp.
>   */
>  
> +#include <linux/version.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  
> @@ -603,11 +604,186 @@ static const struct v4l2_subdev_pad_ops adv748x_pad_ops_hdmi = {
>  	.enum_dv_timings = adv748x_hdmi_enum_dv_timings,
>  };
>  
> +static int adv748x_hdmi_audio_mute(struct adv748x_hdmi *hdmi, int enable)
> +{
> +	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
> +
> +	return hdmi_update(state, ADV748X_HDMI_MUTE_CTRL,
> +			   ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO,
> +			   enable ? 0xff : 0);
> +}
> +
> +
> +#define HDMI_AOUT_NONE 0
> +#define HDMI_AOUT_I2S 1
> +#define HDMI_AOUT_I2S_TDM 2
> +
> +static int adv748x_hdmi_enumaudout(struct adv748x_hdmi *hdmi,
> +				   struct v4l2_audioout *a)
> +{
> +	switch (a->index) {
> +	case HDMI_AOUT_NONE:
> +		strlcpy(a->name, "None", sizeof(a->name));
> +		break;
> +	case HDMI_AOUT_I2S:
> +		strlcpy(a->name, "I2S/stereo", sizeof(a->name));
> +		break;
> +	case HDMI_AOUT_I2S_TDM:
> +		strlcpy(a->name, "I2S-TDM/multichannel", sizeof(a->name));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int adv748x_hdmi_g_audout(struct adv748x_hdmi *hdmi,
> +				 struct v4l2_audioout *a)
> +{
> +	a->index = hdmi->audio_out;
> +	return adv748x_hdmi_enumaudout(hdmi, a);
> +}
> +
> +static int set_audio_pads_state(struct adv748x_state *state, int on)
> +{
> +	return io_update(state, ADV748X_IO_PAD_CONTROLS,
> +			 ADV748X_IO_PAD_CONTROLS_TRI_AUD |
> +			 ADV748X_IO_PAD_CONTROLS_PDN_AUD,
> +			 on ? 0 : 0xff);
> +}
> +
> +static int set_dpll_mclk_fs(struct adv748x_state *state, int fs)
> +{
> +	if (fs % 128 || fs > 768)
> +		return -EINVAL;
> +	return dpll_update(state, ADV748X_DPLL_MCLK_FS,
> +			   ADV748X_DPLL_MCLK_FS_N_MASK, (fs / 128) - 1);
> +}
> +
> +static int set_i2s_format(struct adv748x_state *state, uint outmode,
> +			  uint bitwidth)
> +{
> +	return hdmi_update(state, ADV748X_HDMI_I2S,
> +			   ADV748X_HDMI_I2SBITWIDTH_MASK |
> +			   ADV748X_HDMI_I2SOUTMODE_MASK,
> +			   (outmode << ADV748X_HDMI_I2SOUTMODE_SHIFT) |
> +			   bitwidth);
> +}
> +
> +static int set_i2s_tdm_mode(struct adv748x_state *state, int is_tdm)
> +{
> +	int ret;
> +
> +	ret = hdmi_update(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
> +			  ADV748X_MAN_AUDIO_DL_BYPASS |
> +			  ADV748X_AUDIO_DELAY_LINE_BYPASS,
> +			  is_tdm ? 0xff : 0);
> +	if (ret < 0)
> +		goto fail;
> +	ret = hdmi_update(state, ADV748X_HDMI_REG_6D,
> +			  ADV748X_I2S_TDM_MODE_ENABLE,
> +			  is_tdm ? 0xff : 0);
> +	if (ret < 0)
> +		goto fail;
> +	ret = set_i2s_format(state, ADV748X_I2SOUTMODE_LEFT_J, 24);
> +fail:
> +	return ret;
> +}
> +
> +static int set_audio_out(struct adv748x_state *state, int aout)
> +{
> +	int ret;
> +
> +	switch (aout) {
> +	case HDMI_AOUT_NONE:
> +		ret = set_audio_pads_state(state, 0);
> +		break;
> +	case HDMI_AOUT_I2S:
> +		ret = set_dpll_mclk_fs(state, 256);
> +		if (ret < 0)
> +			goto fail;
> +		ret = set_i2s_tdm_mode(state, 1);
> +		if (ret < 0)
> +			goto fail;
> +		ret = set_audio_pads_state(state, 1);
> +		if (ret < 0)
> +			goto fail;
> +		break;
> +	case HDMI_AOUT_I2S_TDM:
> +		ret = set_dpll_mclk_fs(state, 256);
> +		if (ret < 0)
> +			goto fail;
> +		ret = set_i2s_tdm_mode(state, 1);
> +		if (ret < 0)
> +			goto fail;
> +		ret = set_audio_pads_state(state, 1);
> +		if (ret < 0)
> +			goto fail;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return ret;
> +}
> +
> +static int adv748x_hdmi_s_audout(struct adv748x_hdmi *hdmi,
> +				 const struct v4l2_audioout *a)
> +{
> +	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
> +	int ret = set_audio_out(state, a->index);
> +
> +	if (ret == 0)
> +		hdmi->audio_out = a->index;
> +	return ret;
> +}
> +
> +static long adv748x_hdmi_querycap(struct adv748x_hdmi *hdmi,
> +				  struct v4l2_capability *cap)
> +{
> +	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
> +
> +	cap->version = LINUX_VERSION_CODE;
> +	strlcpy(cap->driver, state->dev->driver->name, sizeof(cap->driver));
> +	strlcpy(cap->card, "hdmi", sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "i2c:%d-%04x",
> +		 i2c_adapter_id(state->client->adapter),
> +		 state->client->addr);
> +	cap->device_caps = V4L2_CAP_AUDIO | V4L2_CAP_VIDEO_CAPTURE;
> +	cap->capabilities = V4L2_CAP_DEVICE_CAPS;
> +	return 0;
> +}
> +
> +static long adv748x_hdmi_ioctl(struct v4l2_subdev *sd,
> +			       unsigned int cmd, void *arg)
> +{
> +	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
> +
> +	switch (cmd) {
> +	case VIDIOC_ENUMAUDOUT:
> +		return adv748x_hdmi_enumaudout(hdmi, arg);
> +	case VIDIOC_S_AUDOUT:
> +		return adv748x_hdmi_s_audout(hdmi, arg);
> +	case VIDIOC_G_AUDOUT:
> +		return adv748x_hdmi_g_audout(hdmi, arg);
> +	case VIDIOC_QUERYCAP:
> +		return adv748x_hdmi_querycap(hdmi, arg);
> +	}
> +	return -ENOTTY;
> +}
> +
> +static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
> +	.ioctl = adv748x_hdmi_ioctl,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * v4l2_subdev_ops
>   */
>  
>  static const struct v4l2_subdev_ops adv748x_ops_hdmi = {
> +	.core = &adv748x_core_ops_hdmi,
>  	.video = &adv748x_video_ops_hdmi,
>  	.pad = &adv748x_pad_ops_hdmi,
>  };
> @@ -633,6 +809,8 @@ static int adv748x_hdmi_s_ctrl(struct v4l2_ctrl *ctrl)
>  	int ret;
>  	u8 pattern;
>  
> +	if (ctrl->id == V4L2_CID_AUDIO_MUTE)
> +		return adv748x_hdmi_audio_mute(hdmi, ctrl->val);
>  	/* Enable video adjustment first */
>  	ret = cp_clrset(state, ADV748X_CP_VID_ADJ,
>  			ADV748X_CP_VID_ADJ_ENABLE,
> @@ -697,6 +875,8 @@ static int adv748x_hdmi_init_controls(struct adv748x_hdmi *hdmi)
>  	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
>  			  V4L2_CID_HUE, ADV748X_CP_HUE_MIN,
>  			  ADV748X_CP_HUE_MAX, 1, ADV748X_CP_HUE_DEF);
> +	v4l2_ctrl_new_std(&hdmi->ctrl_hdl, &adv748x_hdmi_ctrl_ops,
> +			  V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
>  
>  	/*
>  	 * Todo: V4L2_CID_DV_RX_POWER_PRESENT should also be supported when
> @@ -755,6 +935,8 @@ int adv748x_hdmi_init(struct adv748x_hdmi *hdmi)
>  
>  void adv748x_hdmi_cleanup(struct adv748x_hdmi *hdmi)
>  {
> +	adv748x_hdmi_audio_mute(hdmi, 1);
> +	set_audio_out(adv748x_hdmi_to_state(hdmi), HDMI_AOUT_NONE);
>  	v4l2_device_unregister_subdev(&hdmi->sd);
>  	media_entity_cleanup(&hdmi->sd.entity);
>  	v4l2_ctrl_handler_free(&hdmi->ctrl_hdl);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index db6346a06351..fdda6982e437 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -128,6 +128,7 @@ struct adv748x_hdmi {
>  		u32 present;
>  		unsigned int blocks;
>  	} edid;
> +	int audio_out;
>  };
>  
>  #define adv748x_ctrl_to_hdmi(ctrl) \
> @@ -224,6 +225,11 @@ struct adv748x_state {
>  
>  #define ADV748X_IO_VID_STD		0x05
>  
> +#define ADV748X_IO_PAD_CONTROLS		0x0e
> +#define ADV748X_IO_PAD_CONTROLS_TRI_AUD	BIT(5)
> +#define ADV748X_IO_PAD_CONTROLS_PDN_AUD	BIT(1)
> +#define ADV748X_IO_PAD_CONTROLS1	0x1d
> +
>  #define ADV748X_IO_10			0x10	/* io_reg_10 */
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> @@ -246,7 +252,21 @@ struct adv748x_state {
>  #define ADV748X_IO_REG_FF		0xff
>  #define ADV748X_IO_REG_FF_MAIN_RESET	0xff
>  
> +/* DPLL Map */
> +#define ADV748X_DPLL_MCLK_FS		0xb5
> +#define ADV748X_DPLL_MCLK_FS_N_MASK	GENMASK(2, 0)
> +
>  /* HDMI RX Map */
> +#define ADV748X_HDMI_I2S		0x03	/* I2S mode and width */
> +#define ADV748X_HDMI_I2SBITWIDTH_MASK	GENMASK(4, 0)
> +#define ADV748X_HDMI_I2SOUTMODE_SHIFT	5
> +#define ADV748X_HDMI_I2SOUTMODE_MASK	\
> +	GENMASK(6, ADV748X_HDMI_I2SOUTMODE_SHIFT)
> +#define ADV748X_I2SOUTMODE_I2S 0
> +#define ADV748X_I2SOUTMODE_RIGHT_J 1
> +#define ADV748X_I2SOUTMODE_LEFT_J 2
> +#define ADV748X_I2SOUTMODE_SPDIF 3
> +
>  #define ADV748X_HDMI_LW1		0x07	/* line width_1 */
>  #define ADV748X_HDMI_LW1_VERT_FILTER	BIT(7)
>  #define ADV748X_HDMI_LW1_DE_REGEN	BIT(5)
> @@ -258,6 +278,16 @@ struct adv748x_state {
>  #define ADV748X_HDMI_F1H1		0x0b	/* field1 height_1 */
>  #define ADV748X_HDMI_F1H1_INTERLACED	BIT(5)
>  
> +#define ADV748X_HDMI_MUTE_CTRL		0x1a
> +#define ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO BIT(4)
> +#define ADV748X_HDMI_MUTE_CTRL_WAIT_UNMUTE_MASK	GENMASK(3, 1)
> +#define ADV748X_HDMI_MUTE_CTRL_NOT_AUTO_UNMUTE	BIT(0)
> +
> +#define ADV748X_HDMI_AUDIO_MUTE_SPEED	0x0f
> +#define ADV748X_HDMI_AUDIO_MUTE_SPEED_MASK	GENMASK(4, 0)
> +#define ADV748X_MAN_AUDIO_DL_BYPASS BIT(7)
> +#define ADV748X_AUDIO_DELAY_LINE_BYPASS BIT(6)
> +
>  #define ADV748X_HDMI_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
>  #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
>  
> @@ -279,6 +309,9 @@ struct adv748x_state {
>  #define ADV748X_HDMI_TMDS_1		0x51	/* hdmi_reg_51 */
>  #define ADV748X_HDMI_TMDS_2		0x52	/* hdmi_reg_52 */
>  
> +#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
> +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
> +
>  /* HDMI RX Repeater Map */
>  #define ADV748X_REPEATER_EDID_SZ	0x70	/* primary_edid_size */
>  #define ADV748X_REPEATER_EDID_SZ_SHIFT	4
> @@ -393,14 +426,23 @@ int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
>  int adv748x_write_block(struct adv748x_state *state, int client_page,
>  			unsigned int init_reg, const void *val,
>  			size_t val_len);
> +int adv748x_update_bits(struct adv748x_state *state, u8 page, u8 reg,
> +			u8 mask, u8 value);
>  
>  #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
>  #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
>  #define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~m) | v)
> +#define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
>  
>  #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
>  #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, r+1)) & m)
>  #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
> +#define hdmi_update(s, r, m, v) \
> +	adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
> +
> +#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
> +#define dpll_update(s, r, m, v) \
> +	adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
>  
>  #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
>  #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)
> 

Re: [PATCH 2/8] media: adv748x: add audio mute control and output selection ioctls

[Alex Riesen]

Hi Hans,

Hans Verkuil, Fri, Mar 13, 2020 09:16:11 +0100:
> On 1/13/20 3:15 PM, Alex Riesen wrote:
> > This change implements audio-related V4L2 ioctls for the HDMI subdevice.
> 
> This is really where things go wrong. These V4L2 audio ioctls are meant for
> old PCI TV tuner devices where the audio was implemented as audio jack outputs
> that are typically looped back to audio inputs on a (PCI) soundcard. And when
> these ioctls were designed ALSA didn't even exist.

I see. That was before my time :)

> Generally an hdmi driver will configure the i2s audio automatically, which is
> typically connected to the SoC and controlled by the ALSA driver of the SoC,
> but there may well be missing features (audio never got a lot of attention in
> hdmi receivers). So what I would like to know is: what features are missing?

Well, the audio is missing. The current adv748x driver does not export the
audio features of the device at all. There is no code to enable the I2S audio
output and it is disabled (all clock and the data lines) by default.

But, by now it seems to be clear that implementation of ALSA SoC DAI
interfaces is the way to support the audio.

And I am already slowly working on it.

> Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls,
> which would be the right approach for this.
> 
> So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be
> added here:
> 
> https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html

This seems to be an explicitly "digital video" control class. And it has no
control option for mute. Or did you mean a similarly structured new class for
"digital audio"?

This feels like an overkill for this particular driver...

Regards,
Alex

Re: [PATCH 2/8] media: adv748x: add audio mute control and output selection ioctls

[Hans Verkuil]

On 3/13/20 11:26 AM, Alex Riesen wrote:
> Hi Hans,
> 
> Hans Verkuil, Fri, Mar 13, 2020 09:16:11 +0100:
>> On 1/13/20 3:15 PM, Alex Riesen wrote:
>>> This change implements audio-related V4L2 ioctls for the HDMI subdevice.
>>
>> This is really where things go wrong. These V4L2 audio ioctls are meant for
>> old PCI TV tuner devices where the audio was implemented as audio jack outputs
>> that are typically looped back to audio inputs on a (PCI) soundcard. And when
>> these ioctls were designed ALSA didn't even exist.
> 
> I see. That was before my time :)
> 
>> Generally an hdmi driver will configure the i2s audio automatically, which is
>> typically connected to the SoC and controlled by the ALSA driver of the SoC,
>> but there may well be missing features (audio never got a lot of attention in
>> hdmi receivers). So what I would like to know is: what features are missing?
> 
> Well, the audio is missing. The current adv748x driver does not export the
> audio features of the device at all. There is no code to enable the I2S audio
> output and it is disabled (all clock and the data lines) by default.

Sorry, I was vague in my question. Obviously that needs to be added, but besides
adding the low-level i2s support I was wondering if there are additional things
that need to be exposed to userspace in order for audio to fully work.

> 
> But, by now it seems to be clear that implementation of ALSA SoC DAI
> interfaces is the way to support the audio.
> 
> And I am already slowly working on it.
> 
>> Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls,
>> which would be the right approach for this.
>>
>> So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be
>> added here:
>>
>> https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html
> 
> This seems to be an explicitly "digital video" control class. And it has no
> control option for mute. Or did you mean a similarly structured new class for
> "digital audio"?

There are no DV_ audio controls at all today. So any new audio controls would be
added to the DV class. But if there is nothing that needs to be exposed, then
nothing needs to be added :-)

Regards,

	Hans

> 
> This feels like an overkill for this particular driver...
> 
> Regards,
> Alex
> 

Re: [PATCH 2/8] media: adv748x: add audio mute control and output selection ioctls

[Alex Riesen]

Hans Verkuil, Fri, Mar 13, 2020 11:52:03 +0100:
> On 3/13/20 11:26 AM, Alex Riesen wrote:
> > Hans Verkuil, Fri, Mar 13, 2020 09:16:11 +0100:
> >> Generally an hdmi driver will configure the i2s audio automatically, which is
> >> typically connected to the SoC and controlled by the ALSA driver of the SoC,
> >> but there may well be missing features (audio never got a lot of attention in
> >> hdmi receivers). So what I would like to know is: what features are missing?
> > 
> > Well, the audio is missing. The current adv748x driver does not export the
> > audio features of the device at all. There is no code to enable the I2S audio
> > output and it is disabled (all clock and the data lines) by default.
> 
> Sorry, I was vague in my question. Obviously that needs to be added, but besides
> adding the low-level i2s support I was wondering if there are additional things
> that need to be exposed to userspace in order for audio to fully work.

None that I can't expose over the DAI interfaces: clocks, I2S format,
mute/demute ... All covered.

> >> Anything missing can likely be resolved by adding HDMI audio specific V4L2 controls,
> >> which would be the right approach for this.
> >>
> >> So I would expect to see a proposal for V4L2_CID_DV_RX_AUDIO_ controls to be
> >> added here:
> >>
> >> https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/ext-ctrls-dv.html
> > 
> > This seems to be an explicitly "digital video" control class. And it has no
> > control option for mute. Or did you mean a similarly structured new class for
> > "digital audio"?
> 
> There are no DV_ audio controls at all today. So any new audio controls would be
> added to the DV class. But if there is nothing that needs to be exposed, then
> nothing needs to be added :-)

Ah, alright. I shall keep an eye on it, maybe I shall find something to expose :)

Regards,
Alex