[PATCH v5 0/9] media: adv748x: add support for HDMI audio

[PATCH v5 0/9] media: adv748x: add support for HDMI audio

[Alex Riesen]

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

Currently, the driver only supports I2S in TDM, 8 channels a 24bit at 48kHz.
Furthermore, only left-justified, 8 slots, 32bit/slot TDM, at 256fs has been
ever tried.

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

Changes since v4:
  - rebased on v5.6

  - Add dummy ssi4 node to the rcar sound card, as the r8a77961
    devices also reference salvator-common.dts.
    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Changes since v3:
  - use clk_hw instead of clk
    Suggested-by: Stephen Boyd <sboyd@kernel.org>

  - formatting improvements and use const where possible

  - removed implementation of log_status and EDID setting ioctls,
    those will be submitted as separate patches.
    Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Changes since v2:
  - prepare/enable the clock when it is used, as it seems nothing else does
    this otherwise

  - give the clock a unique name to ensure it can be registered if there are
    multiple adv748x devices in the system

  - remove optionality note from clock cell description to ensure the device
    description matches the real device (the line is always present, even
    if not used)

Changes since v1:
  - Add ssi4_ctrl pin group to the sound pins. The pins are responsible for
    SCK4 (sample clock) WS4 and (word boundary input), and are required for
    SSI audio input over I2S.
    Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

  - Removed the audio clock C from the list of clocks of adv748x,
    it is exactly the other way around.
    Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>

  - Add an instance of (currently) fixed rate I2S master clock (MCLK),
    connected to the audio_clk_c line of the R-Car SoC.
    Explicitly declare the device a clock producer and add it to the
    list of clocks used by the audio system of the Salvator-X board.
    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

  - The implementation of DAI driver has been moved in a separate file
    and modified to activate audio decoding and I2S streaming using
    snd_soc_dai_... interfaces. This allows the driver to be used with
    just ALSA interfaces.

  - The ioctls for selecting audio output and muting have been removed,
    as not applicable.
    Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
    I have left implementation of the QUERYCAP in, as it seems to be required
    by v4l-ctl to support loading of EDID for this node. And setting the EDID
    is one feature I desperately need: there are devices which plainly refuse
    to talk to the sink if it does not provide EDID they like.

  - A device tree configuration without audio port will disable the audio code
    altogether, supporting integrations where the port is not connected.

  - The patches have been re-arranged, starting with the generic changes and
    changes not related to audio directly. Those will be probably sent as a
    separate series later.

  - The whole series has been rebased on top of v5.6-rc6

Alex Riesen (9):
  media: adv748x: fix end-of-line terminators in diagnostic statements
  media: adv748x: include everything adv748x.h needs into the file
  media: adv748x: reduce amount of code for bitwise modifications of
    device registers
  media: adv748x: add definitions for audio output related registers
  media: adv748x: add support for HDMI audio
  media: adv748x: prepare/enable mclk when the audio is used
  media: adv748x: only activate DAI if it is described in device tree
  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 |  16 +-
 .../boot/dts/renesas/r8a77950-salvator-x.dts  |   3 +-
 arch/arm64/boot/dts/renesas/r8a77961.dtsi     |   1 +
 .../boot/dts/renesas/salvator-common.dtsi     |  47 ++-
 drivers/media/i2c/adv748x/Makefile            |   3 +-
 drivers/media/i2c/adv748x/adv748x-afe.c       |   6 +-
 drivers/media/i2c/adv748x/adv748x-core.c      |  45 +--
 drivers/media/i2c/adv748x/adv748x-csi2.c      |   8 +-
 drivers/media/i2c/adv748x/adv748x-dai.c       | 278 ++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x-hdmi.c      |   6 +-
 drivers/media/i2c/adv748x/adv748x.h           |  65 +++-
 11 files changed, 436 insertions(+), 42 deletions(-)
 create mode 100644 drivers/media/i2c/adv748x/adv748x-dai.c

-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 1/9] media: adv748x: fix end-of-line terminators in diagnostic statements

[Alex Riesen]

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++++------------
 drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 23e02ff27b17..c3fb113cef62 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -623,11 +623,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
 
 	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
 		of_graph_parse_endpoint(ep_np, &ep);
-		adv_info(state, "Endpoint %pOF on port %d", ep.local_node,
+		adv_info(state, "Endpoint %pOF on port %d\n", ep.local_node,
 			 ep.port);
 
 		if (ep.port >= ADV748X_PORT_MAX) {
-			adv_err(state, "Invalid endpoint %pOF on port %d",
+			adv_err(state, "Invalid endpoint %pOF on port %d\n",
 				ep.local_node, ep.port);
 
 			continue;
@@ -635,7 +635,7 @@ static int adv748x_parse_dt(struct adv748x_state *state)
 
 		if (state->endpoints[ep.port]) {
 			adv_err(state,
-				"Multiple port endpoints are not supported");
+				"Multiple port endpoints are not supported\n");
 			continue;
 		}
 
@@ -702,62 +702,62 @@ static int adv748x_probe(struct i2c_client *client)
 	/* Discover and process ports declared by the Device tree endpoints */
 	ret = adv748x_parse_dt(state);
 	if (ret) {
-		adv_err(state, "Failed to parse device tree");
+		adv_err(state, "Failed to parse device tree\n");
 		goto err_free_mutex;
 	}
 
 	/* Configure IO Regmap region */
 	ret = adv748x_configure_regmap(state, ADV748X_PAGE_IO);
 	if (ret) {
-		adv_err(state, "Error configuring IO regmap region");
+		adv_err(state, "Error configuring IO regmap region\n");
 		goto err_cleanup_dt;
 	}
 
 	ret = adv748x_identify_chip(state);
 	if (ret) {
-		adv_err(state, "Failed to identify chip");
+		adv_err(state, "Failed to identify chip\n");
 		goto err_cleanup_dt;
 	}
 
 	/* Configure remaining pages as I2C clients with regmap access */
 	ret = adv748x_initialise_clients(state);
 	if (ret) {
-		adv_err(state, "Failed to setup client regmap pages");
+		adv_err(state, "Failed to setup client regmap pages\n");
 		goto err_cleanup_clients;
 	}
 
 	/* SW reset ADV748X to its default values */
 	ret = adv748x_reset(state);
 	if (ret) {
-		adv_err(state, "Failed to reset hardware");
+		adv_err(state, "Failed to reset hardware\n");
 		goto err_cleanup_clients;
 	}
 
 	/* Initialise HDMI */
 	ret = adv748x_hdmi_init(&state->hdmi);
 	if (ret) {
-		adv_err(state, "Failed to probe HDMI");
+		adv_err(state, "Failed to probe HDMI\n");
 		goto err_cleanup_clients;
 	}
 
 	/* Initialise AFE */
 	ret = adv748x_afe_init(&state->afe);
 	if (ret) {
-		adv_err(state, "Failed to probe AFE");
+		adv_err(state, "Failed to probe AFE\n");
 		goto err_cleanup_hdmi;
 	}
 
 	/* Initialise TXA */
 	ret = adv748x_csi2_init(state, &state->txa);
 	if (ret) {
-		adv_err(state, "Failed to probe TXA");
+		adv_err(state, "Failed to probe TXA\n");
 		goto err_cleanup_afe;
 	}
 
 	/* Initialise TXB */
 	ret = adv748x_csi2_init(state, &state->txb);
 	if (ret) {
-		adv_err(state, "Failed to probe TXB");
+		adv_err(state, "Failed to probe TXB\n");
 		goto err_cleanup_txa;
 	}
 
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 2091cda50935..c43ce5d78723 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -72,7 +72,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 	struct adv748x_state *state = tx->state;
 	int ret;
 
-	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
+	adv_dbg(state, "Registered %s (%s)\n", is_txa(tx) ? "TXA":"TXB",
 			sd->name);
 
 	/*
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 2/9] media: adv748x: include everything adv748x.h needs into the file

[Alex Riesen]

To follow the established practice of not depending on others to
pull everything in. While at it, make sure it stays like this.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-afe.c  | 6 ++----
 drivers/media/i2c/adv748x/adv748x-core.c | 6 ++----
 drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++----
 drivers/media/i2c/adv748x/adv748x-hdmi.c | 6 ++----
 drivers/media/i2c/adv748x/adv748x.h      | 2 ++
 5 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index dbbb1e4d6363..5a25d1fbe25f 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -6,18 +6,16 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include "adv748x.h"
+
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/v4l2-dv-timings.h>
 
-#include <media/v4l2-ctrls.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-ioctl.h>
 
-#include "adv748x.h"
-
 /* -----------------------------------------------------------------------------
  * SDP
  */
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index c3fb113cef62..5c59aad319d1 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -10,6 +10,8 @@
  *	Kieran Bingham <kieran.bingham@ideasonboard.com>
  */
 
+#include "adv748x.h"
+
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
@@ -20,14 +22,10 @@
 #include <linux/slab.h>
 #include <linux/v4l2-dv-timings.h>
 
-#include <media/v4l2-ctrls.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-ioctl.h>
 
-#include "adv748x.h"
-
 /* -----------------------------------------------------------------------------
  * Register manipulation
  */
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index c43ce5d78723..c00d4f347d95 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -5,15 +5,13 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include "adv748x.h"
+
 #include <linux/module.h>
 #include <linux/mutex.h>
 
-#include <media/v4l2-ctrls.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 
-#include "adv748x.h"
-
 static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
 					    unsigned int vc)
 {
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index c557f8fdf11a..f598acec3b5c 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -5,18 +5,16 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include "adv748x.h"
+
 #include <linux/module.h>
 #include <linux/mutex.h>
 
-#include <media/v4l2-ctrls.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-ioctl.h>
 
 #include <uapi/linux/v4l2-dv-timings.h>
 
-#include "adv748x.h"
-
 /* -----------------------------------------------------------------------------
  * HDMI and CP
  */
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index fccb388ce179..09aab4138c3f 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -19,6 +19,8 @@
  */
 
 #include <linux/i2c.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
 
 #ifndef _ADV748X_H_
 #define _ADV748X_H_
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 3/9] media: adv748x: reduce amount of code for bitwise modifications of device registers

[Alex Riesen]

The regmap provides a convenient utility for this.
The hdmi_* and dpll_* register modification macros added for symmetry
with the existing operations (io_*, sdp_*).

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

--
v3: remove _update name in favor of existing _clrset
---
 drivers/media/i2c/adv748x/adv748x-core.c |  6 ++++++
 drivers/media/i2c/adv748x/adv748x.h      | 14 +++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 5c59aad319d1..8580e6624276 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -133,6 +133,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.h b/drivers/media/i2c/adv748x/adv748x.h
index 09aab4138c3f..0a9d78c2870b 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -393,25 +393,33 @@ 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_clrset(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_clrset(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_clrset(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)
 
 #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.25.1.25.g9ecbe7eb18

[PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Alex Riesen]

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

diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 0a9d78c2870b..1a1ea70086c6 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -226,6 +226,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)
@@ -248,7 +253,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)
@@ -260,6 +279,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
 
@@ -281,6 +310,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
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 5/9] media: adv748x: add support for HDMI audio

[Alex Riesen]

This adds an implemention of SoC DAI driver which provides access to the
I2S port of the device.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
--

v3: fix clock registration in case of multiple adv748x devices
    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

v4: use clk_hw instead of clk
    Suggested-by: Stephen Boyd <sboyd@kernel.org>

v4: use const for capture snd_soc_pcm_stream instance
    Suggested-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/media/i2c/adv748x/Makefile       |   3 +-
 drivers/media/i2c/adv748x/adv748x-core.c |   9 +-
 drivers/media/i2c/adv748x/adv748x-dai.c  | 261 +++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  17 +-
 4 files changed, 287 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/i2c/adv748x/adv748x-dai.c

diff --git a/drivers/media/i2c/adv748x/Makefile b/drivers/media/i2c/adv748x/Makefile
index 93844f14cb10..6e7a302ef199 100644
--- a/drivers/media/i2c/adv748x/Makefile
+++ b/drivers/media/i2c/adv748x/Makefile
@@ -3,6 +3,7 @@ adv748x-objs	:= \
 		adv748x-afe.o \
 		adv748x-core.o \
 		adv748x-csi2.o \
-		adv748x-hdmi.o
+		adv748x-hdmi.o \
+		adv748x-dai.o
 
 obj-$(CONFIG_VIDEO_ADV748X)	+= adv748x.o
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 8580e6624276..3513ca138e53 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -765,8 +765,14 @@ static int adv748x_probe(struct i2c_client *client)
 		goto err_cleanup_txa;
 	}
 
+	ret = adv748x_dai_init(&state->dai);
+	if (ret) {
+		adv_err(state, "Failed to probe DAI\n");
+		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:
@@ -787,6 +793,7 @@ static int adv748x_remove(struct i2c_client *client)
 {
 	struct adv748x_state *state = i2c_get_clientdata(client);
 
+	adv748x_dai_cleanup(&state->dai);
 	adv748x_afe_cleanup(&state->afe);
 	adv748x_hdmi_cleanup(&state->hdmi);
 
diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
new file mode 100644
index 000000000000..c9191f8f1ca8
--- /dev/null
+++ b/drivers/media/i2c/adv748x/adv748x-dai.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Analog Devices ADV748X HDMI receiver with AFE
+ * The implementation of DAI.
+ */
+
+#include "adv748x.h"
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <sound/pcm_params.h>
+
+#define state_of(soc_dai) \
+	adv748x_dai_to_state(container_of((soc_dai)->driver, \
+					  struct adv748x_dai, drv))
+#define mclk_of(state) ((state)->dai.mclk_hw->clk)
+
+static const char ADV748X_DAI_NAME[] = "adv748x-i2s";
+
+static int set_audio_pads_state(struct adv748x_state *state, int on)
+{
+	return io_clrset(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)
+{
+	return dpll_clrset(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_clrset(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_clrset(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
+			  ADV748X_MAN_AUDIO_DL_BYPASS |
+			  ADV748X_AUDIO_DELAY_LINE_BYPASS,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		return ret;
+	ret = hdmi_clrset(state, ADV748X_HDMI_REG_6D,
+			  ADV748X_I2S_TDM_MODE_ENABLE,
+			  is_tdm ? 0xff : 0);
+	return ret;
+}
+
+static int set_audio_mute(struct adv748x_state *state, int enable)
+{
+	return hdmi_clrset(state, ADV748X_HDMI_MUTE_CTRL,
+			   ADV748X_HDMI_MUTE_CTRL_MUTE_AUDIO,
+			   enable ? 0xff : 0);
+}
+
+static int adv748x_dai_set_sysclk(struct snd_soc_dai *dai,
+				  int clk_id, unsigned int freq, int dir)
+{
+	struct adv748x_state *state = state_of(dai);
+
+	/* currently supporting only one fixed rate clock */
+	if (clk_id != 0 || freq != clk_get_rate(mclk_of(state))) {
+		dev_err(dai->dev, "invalid clock (%d) or frequency (%u, dir %d)\n",
+			clk_id, freq, dir);
+		return -EINVAL;
+	}
+	state->dai.freq = freq;
+	return 0;
+}
+
+static int adv748x_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct adv748x_state *state = state_of(dai);
+	int ret = 0;
+
+	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBM_CFM) {
+		dev_err(dai->dev, "only I2S master clock mode supported\n");
+		ret = -EINVAL;
+		goto done;
+	}
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAI_FORMAT_I2S:
+		state->dai.tdm = 0;
+		state->dai.fmt = ADV748X_I2SOUTMODE_I2S;
+		break;
+	case SND_SOC_DAI_FORMAT_RIGHT_J:
+		state->dai.tdm = 1;
+		state->dai.fmt = ADV748X_I2SOUTMODE_RIGHT_J;
+		break;
+	case SND_SOC_DAI_FORMAT_LEFT_J:
+		state->dai.tdm = 1;
+		state->dai.fmt = ADV748X_I2SOUTMODE_LEFT_J;
+		break;
+	default:
+		dev_err(dai->dev, "only i2s, left_j and right_j supported\n");
+		ret = -EINVAL;
+		goto done;
+	}
+	if ((fmt & SND_SOC_DAIFMT_INV_MASK) != SND_SOC_DAIFMT_NB_NF) {
+		dev_err(dai->dev, "only normal bit clock + frame supported\n");
+		ret = -EINVAL;
+	}
+done:
+	return ret;
+}
+
+static int adv748x_dai_startup(struct snd_pcm_substream *sub, struct snd_soc_dai *dai)
+{
+	struct adv748x_state *state = state_of(dai);
+
+	if (sub->stream != SNDRV_PCM_STREAM_CAPTURE)
+		return -EINVAL;
+	return set_audio_pads_state(state, 1);
+}
+
+static int adv748x_dai_hw_params(struct snd_pcm_substream *sub,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *dai)
+{
+	int ret;
+	struct adv748x_state *state = state_of(dai);
+	uint fs = state->dai.freq / params_rate(params);
+
+	dev_dbg(dai->dev, "dai %s substream %s rate=%u (fs=%u), channels=%u sample width=%u(%u)\n",
+		dai->name, sub->name,
+		params_rate(params), fs,
+		params_channels(params),
+		params_width(params),
+		params_physical_width(params));
+	switch (fs) {
+	case 128:
+	case 256:
+	case 384:
+	case 512:
+	case 640:
+	case 768:
+		break;
+	default:
+		ret = -EINVAL;
+		dev_err(dai->dev, "invalid clock frequency (%u) or rate (%u)\n",
+			state->dai.freq, params_rate(params));
+		goto done;
+	}
+	ret = set_dpll_mclk_fs(state, fs);
+	if (ret)
+		goto done;
+	ret = set_i2s_tdm_mode(state, state->dai.tdm);
+	if (ret)
+		goto done;
+	ret = set_i2s_format(state, state->dai.fmt, params_width(params));
+done:
+	return ret;
+}
+
+static int adv748x_dai_mute_stream(struct snd_soc_dai *dai, int mute, int dir)
+{
+	struct adv748x_state *state = state_of(dai);
+
+	return set_audio_mute(state, mute);
+}
+
+static void adv748x_dai_shutdown(struct snd_pcm_substream *sub, struct snd_soc_dai *dai)
+{
+	struct adv748x_state *state = state_of(dai);
+
+	set_audio_pads_state(state, 0);
+}
+
+static const struct snd_soc_dai_ops adv748x_dai_ops = {
+	.set_sysclk = adv748x_dai_set_sysclk,
+	.set_fmt = adv748x_dai_set_fmt,
+	.startup = adv748x_dai_startup,
+	.hw_params = adv748x_dai_hw_params,
+	.mute_stream = adv748x_dai_mute_stream,
+	.shutdown = adv748x_dai_shutdown,
+};
+
+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,
+};
+
+int adv748x_dai_init(struct adv748x_dai *dai)
+{
+	int ret;
+	struct adv748x_state *state = adv748x_dai_to_state(dai);
+
+	dai->mclk_name = kasprintf(GFP_KERNEL, "%s.%s-i2s-mclk",
+				   state->dev->driver->name,
+				   dev_name(state->dev));
+	if (!dai->mclk_name) {
+		ret = -ENOMEM;
+		adv_err(state, "No memory for MCLK\n");
+		goto fail;
+	}
+	dai->mclk_hw = clk_hw_register_fixed_rate(state->dev, dai->mclk_name,
+						  NULL, 0, 12288000);
+	if (IS_ERR(dai->mclk_hw)) {
+		ret = PTR_ERR(dai->mclk_hw);
+		adv_err(state, "Failed to register MCLK (%d)\n", ret);
+		goto fail;
+	}
+	ret = of_clk_add_hw_provider(state->dev->of_node, of_clk_hw_simple_get,
+				     dai->mclk_hw->clk);
+	if (ret < 0) {
+		adv_err(state, "Failed to add MCLK provider (%d)\n", ret);
+		goto unreg_mclk;
+	}
+	dai->drv.name = ADV748X_DAI_NAME;
+	dai->drv.ops = &adv748x_dai_ops;
+	dai->drv.capture = (const struct snd_soc_pcm_stream){
+		.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,
+	};
+
+	ret = devm_snd_soc_register_component(state->dev, &adv748x_codec,
+					      &dai->drv, 1);
+	if (ret < 0) {
+		adv_err(state, "Failed to register the codec (%d)\n", ret);
+		goto cleanup_mclk;
+	}
+	return 0;
+
+cleanup_mclk:
+	of_clk_del_provider(state->dev->of_node);
+unreg_mclk:
+	clk_hw_unregister_fixed_rate(dai->mclk_hw);
+fail:
+	return ret;
+}
+
+void adv748x_dai_cleanup(struct adv748x_dai *dai)
+{
+	struct adv748x_state *state = adv748x_dai_to_state(dai);
+
+	of_clk_del_provider(state->dev->of_node);
+	clk_hw_unregister_fixed_rate(dai->mclk_hw);
+	kfree(dai->mclk_name);
+}
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 1a1ea70086c6..454f97ff7b54 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -19,6 +19,7 @@
  */
 
 #include <linux/i2c.h>
+#include <sound/soc.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 
@@ -63,7 +64,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 {
@@ -166,6 +168,13 @@ struct adv748x_afe {
 	container_of(ctrl->handler, struct adv748x_afe, ctrl_hdl)
 #define adv748x_sd_to_afe(sd) container_of(sd, struct adv748x_afe, sd)
 
+struct adv748x_dai {
+	struct snd_soc_dai_driver drv;
+	struct clk_hw *mclk_hw;
+	char *mclk_name;
+	unsigned int freq, fmt, tdm;
+};
+
 /**
  * struct adv748x_state - State of ADV748X
  * @dev:		(OF) device
@@ -182,6 +191,7 @@ struct adv748x_afe {
  * @afe:		state of AFE receiver context
  * @txa:		state of TXA transmitter context
  * @txb:		state of TXB transmitter context
+ * @mclk:		MCLK clock of the I2S port
  */
 struct adv748x_state {
 	struct device *dev;
@@ -197,10 +207,12 @@ struct adv748x_state {
 	struct adv748x_afe afe;
 	struct adv748x_csi2 txa;
 	struct adv748x_csi2 txb;
+	struct adv748x_dai dai;
 };
 
 #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
 #define adv748x_afe_to_state(a) container_of(a, struct adv748x_state, afe)
+#define adv748x_dai_to_state(p) container_of(p, struct adv748x_state, dai)
 
 #define adv_err(a, fmt, arg...)	dev_err(a->dev, fmt, ##arg)
 #define adv_info(a, fmt, arg...) dev_info(a->dev, fmt, ##arg)
@@ -484,4 +496,7 @@ int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate);
 int adv748x_hdmi_init(struct adv748x_hdmi *hdmi);
 void adv748x_hdmi_cleanup(struct adv748x_hdmi *hdmi);
 
+int adv748x_dai_init(struct adv748x_dai *);
+void adv748x_dai_cleanup(struct adv748x_dai *);
+
 #endif /* _ADV748X_H_ */
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 6/9] media: adv748x: prepare/enable mclk when the audio is used

[Alex Riesen]

As there is nothing else (the consumers are supposed to do that) which
enables the clock, do it in the driver.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
--

v3: added
---
 drivers/media/i2c/adv748x/adv748x-dai.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
index c9191f8f1ca8..185f78023e91 100644
--- a/drivers/media/i2c/adv748x/adv748x-dai.c
+++ b/drivers/media/i2c/adv748x/adv748x-dai.c
@@ -117,11 +117,22 @@ static int adv748x_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 
 static int adv748x_dai_startup(struct snd_pcm_substream *sub, struct snd_soc_dai *dai)
 {
+	int ret;
 	struct adv748x_state *state = state_of(dai);
 
 	if (sub->stream != SNDRV_PCM_STREAM_CAPTURE)
 		return -EINVAL;
-	return set_audio_pads_state(state, 1);
+	ret = set_audio_pads_state(state, 1);
+	if (ret)
+		goto fail;
+	ret = clk_prepare_enable(mclk_of(state));
+	if (ret)
+		goto fail_pwdn;
+	return 0;
+fail_pwdn:
+	set_audio_pads_state(state, 0);
+fail:
+	return ret;
 }
 
 static int adv748x_dai_hw_params(struct snd_pcm_substream *sub,
@@ -174,6 +185,7 @@ static void adv748x_dai_shutdown(struct snd_pcm_substream *sub, struct snd_soc_d
 {
 	struct adv748x_state *state = state_of(dai);
 
+	clk_disable_unprepare(mclk_of(state));
 	set_audio_pads_state(state, 0);
 }
 
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 7/9] media: adv748x: only activate DAI if it is described in device tree

[Alex Riesen]

To avoid setting it up even if the hardware is not actually connected
to anything physically.

Besides, the bindings explicitly notes that port definitions are
"optional if they are not connected to anything at the hardware level".

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

diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
index 185f78023e91..f9cc47fa9ad1 100644
--- a/drivers/media/i2c/adv748x/adv748x-dai.c
+++ b/drivers/media/i2c/adv748x/adv748x-dai.c
@@ -216,6 +216,11 @@ int adv748x_dai_init(struct adv748x_dai *dai)
 	int ret;
 	struct adv748x_state *state = adv748x_dai_to_state(dai);
 
+	if (!state->endpoints[ADV748X_PORT_I2S]) {
+		adv_info(state, "no I2S port, DAI disabled\n");
+		ret = 0;
+		goto fail;
+	}
 	dai->mclk_name = kasprintf(GFP_KERNEL, "%s.%s-i2s-mclk",
 				   state->dev->driver->name,
 				   dev_name(state->dev));
-- 
2.25.1.25.g9ecbe7eb18

[PATCH v5 8/9] 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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

--

v3: remove optionality off MCLK clock cell to ensure the description
    matches the hardware no matter if the line is connected.
    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 .../devicetree/bindings/media/i2c/adv748x.txt    | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
index 4f91686e54a6..50a753189b81 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:
 
@@ -16,6 +18,8 @@ Required Properties:
     slave device on the I2C bus. The main address is mandatory, others are
     optional and remain at default values if not specified.
 
+  - #clock-cells: must be <0>
+
 Optional Properties:
 
   - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
@@ -47,6 +51,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
@@ -72,6 +77,7 @@ Example:
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#clock-cells = <0>;
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";
@@ -113,4 +119,12 @@ Example:
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				remote-endpoint = <&i2s_in>;
+			};
+		};
 	};
-- 
2.25.1.25.g9ecbe7eb18

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

[Alex Riesen]

As all known variants of the Salvator board have the HDMI decoder
chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482
endpoint and the connection definitions are placed in the common board
file.

For the same reason, the CLK_C clock line and I2C configuration (similar
to the ak4613, on the same interface) are added into the common file.

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>

--

v5: Add dummy ssi4 node to the rcar sound card in r8a77961, as the
    devices (Salvator-X 2nd version with R-Car M3 W+) also reference
    salvator-common.dtsi.
    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

v2: Also add ssi4_ctrl pin group in the sound pins. The pins are
    responsible for SCK4 (sample clock) WS4 and (word boundary input),
    and are required for SSI audio input over I2S.

    The adv748x shall provide its own implementation of the output clock
    (MCLK), connected to the audio_clk_c line of the R-Car SoC.

    If the frequency of the ADV748x MCLK were fixed, the clock
    implementation were not necessary, but it does not seem so: the MCLK
    depends on the value in a speed multiplier register and the input sample
    rate (48kHz).

    Remove audio clock C from the clocks of adv7482.

    The clocks property of the video-receiver node lists the input
    clocks of the device, which is quite the opposite from the
    original intention: the adv7482 on Salvator X boards is a
    provide of the MCLK clock for I2S audio output.

    Remove old definition of &sound_card.dais and reduce size of changes
    in the Salvator-X specific device tree source.

    Declare video-receiver a clock producer, as the adv748x driver
    implements the master clock used I2S audio output.

    Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>

v2: The driver provides only MCLK clock, not the SCLK and LRCLK,
    which are part of the I2S protocol.

    Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../boot/dts/renesas/r8a77950-salvator-x.dts  |  3 +-
 arch/arm64/boot/dts/renesas/r8a77961.dtsi     |  1 +
 .../boot/dts/renesas/salvator-common.dtsi     | 47 +++++++++++++++++--
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
index 2438825c9b22..e16c146808b6 100644
--- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
@@ -146,7 +146,8 @@ &sata {
 &sound_card {
 	dais = <&rsnd_port0	/* ak4613 */
 		&rsnd_port1	/* HDMI0  */
-		&rsnd_port2>;	/* HDMI1  */
+		&rsnd_port2	/* HDMI1  */
+		&rsnd_port3>;	/* adv7482 hdmi-in  */
 };
 
 &usb2_phy2 {
diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
index be3824bda632..b79907beaf31 100644
--- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
@@ -861,6 +861,7 @@ rcar_sound,src {
 			rcar_sound,ssi {
 				ssi0: ssi-0 { };
 				ssi1: ssi-1 { };
+				ssi4: ssi-4 { };
 			};
 		};
 
diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
index 98bbcafc8c0d..ead7f8d7a929 100644
--- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
@@ -460,7 +460,7 @@ pca9654: gpio@20 {
 		#gpio-cells = <2>;
 	};
 
-	video-receiver@70 {
+	adv7482_hdmi_in: video-receiver@70 {
 		compatible = "adi,adv7482";
 		reg = <0x70 0x71 0x72 0x73 0x74 0x75
 		       0x60 0x61 0x62 0x63 0x64 0x65>;
@@ -469,6 +469,7 @@ video-receiver@70 {
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#clock-cells = <0>; /* the MCLK for I2S output */
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";
@@ -510,6 +511,15 @@ adv7482_txb: endpoint {
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				remote-endpoint = <&rsnd_endpoint3>;
+				system-clock-direction-out;
+			};
+		};
 	};
 
 	csa_vdd: adc@7c {
@@ -684,7 +694,8 @@ sdhi3_pins_uhs: sd3_uhs {
 	};
 
 	sound_pins: sound {
-		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
+		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
+			 "ssi4_data", "ssi4_ctrl";
 		function = "ssi";
 	};
 
@@ -733,8 +744,8 @@ &rcar_sound {
 	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>;
@@ -758,8 +769,19 @@ &rcar_sound {
 		 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
 		 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
 		 <&audio_clk_a>, <&cs2000>,
-		 <&audio_clk_c>,
+		 <&adv7482_hdmi_in>,
 		 <&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_c", "clk_i";
 
 	ports {
 		#address-cells = <1>;
@@ -777,6 +799,21 @@ rsnd_endpoint0: endpoint {
 				capture  = <&ssi1 &src1 &dvc1>;
 			};
 		};
+		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>;
+
+				capture = <&ssi4>;
+			};
+		};
 	};
 };
 
-- 
2.25.1.25.g9ecbe7eb18

Re: [PATCH v5 1/9] media: adv748x: fix end-of-line terminators in diagnostic statements

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I guess we could have also added this directly to the helper macros, but
there is indeed already a mixed usage so either way would require fixups
to be consistent.

So this is a good option ;-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++++------------
>  drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 23e02ff27b17..c3fb113cef62 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -623,11 +623,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
>  
>  	for_each_endpoint_of_node(state->dev->of_node, ep_np) {
>  		of_graph_parse_endpoint(ep_np, &ep);
> -		adv_info(state, "Endpoint %pOF on port %d", ep.local_node,
> +		adv_info(state, "Endpoint %pOF on port %d\n", ep.local_node,
>  			 ep.port);
>  
>  		if (ep.port >= ADV748X_PORT_MAX) {
> -			adv_err(state, "Invalid endpoint %pOF on port %d",
> +			adv_err(state, "Invalid endpoint %pOF on port %d\n",
>  				ep.local_node, ep.port);
>  
>  			continue;
> @@ -635,7 +635,7 @@ static int adv748x_parse_dt(struct adv748x_state *state)
>  
>  		if (state->endpoints[ep.port]) {
>  			adv_err(state,
> -				"Multiple port endpoints are not supported");
> +				"Multiple port endpoints are not supported\n");
>  			continue;
>  		}
>  
> @@ -702,62 +702,62 @@ static int adv748x_probe(struct i2c_client *client)
>  	/* Discover and process ports declared by the Device tree endpoints */
>  	ret = adv748x_parse_dt(state);
>  	if (ret) {
> -		adv_err(state, "Failed to parse device tree");
> +		adv_err(state, "Failed to parse device tree\n");
>  		goto err_free_mutex;
>  	}
>  
>  	/* Configure IO Regmap region */
>  	ret = adv748x_configure_regmap(state, ADV748X_PAGE_IO);
>  	if (ret) {
> -		adv_err(state, "Error configuring IO regmap region");
> +		adv_err(state, "Error configuring IO regmap region\n");
>  		goto err_cleanup_dt;
>  	}
>  
>  	ret = adv748x_identify_chip(state);
>  	if (ret) {
> -		adv_err(state, "Failed to identify chip");
> +		adv_err(state, "Failed to identify chip\n");
>  		goto err_cleanup_dt;
>  	}
>  
>  	/* Configure remaining pages as I2C clients with regmap access */
>  	ret = adv748x_initialise_clients(state);
>  	if (ret) {
> -		adv_err(state, "Failed to setup client regmap pages");
> +		adv_err(state, "Failed to setup client regmap pages\n");
>  		goto err_cleanup_clients;
>  	}
>  
>  	/* SW reset ADV748X to its default values */
>  	ret = adv748x_reset(state);
>  	if (ret) {
> -		adv_err(state, "Failed to reset hardware");
> +		adv_err(state, "Failed to reset hardware\n");
>  		goto err_cleanup_clients;
>  	}
>  
>  	/* Initialise HDMI */
>  	ret = adv748x_hdmi_init(&state->hdmi);
>  	if (ret) {
> -		adv_err(state, "Failed to probe HDMI");
> +		adv_err(state, "Failed to probe HDMI\n");
>  		goto err_cleanup_clients;
>  	}
>  
>  	/* Initialise AFE */
>  	ret = adv748x_afe_init(&state->afe);
>  	if (ret) {
> -		adv_err(state, "Failed to probe AFE");
> +		adv_err(state, "Failed to probe AFE\n");
>  		goto err_cleanup_hdmi;
>  	}
>  
>  	/* Initialise TXA */
>  	ret = adv748x_csi2_init(state, &state->txa);
>  	if (ret) {
> -		adv_err(state, "Failed to probe TXA");
> +		adv_err(state, "Failed to probe TXA\n");
>  		goto err_cleanup_afe;
>  	}
>  
>  	/* Initialise TXB */
>  	ret = adv748x_csi2_init(state, &state->txb);
>  	if (ret) {
> -		adv_err(state, "Failed to probe TXB");
> +		adv_err(state, "Failed to probe TXB\n");
>  		goto err_cleanup_txa;
>  	}
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 2091cda50935..c43ce5d78723 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -72,7 +72,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  	struct adv748x_state *state = tx->state;
>  	int ret;
>  
> -	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> +	adv_dbg(state, "Registered %s (%s)\n", is_txa(tx) ? "TXA":"TXB",
>  			sd->name);
>  
>  	/*
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 2/9] media: adv748x: include everything adv748x.h needs into the file

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> To follow the established practice of not depending on others to
> pull everything in. While at it, make sure it stays like this.

Good call!

Small extra trivial comment below...

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  | 6 ++----
>  drivers/media/i2c/adv748x/adv748x-core.c | 6 ++----
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 6 ++----
>  drivers/media/i2c/adv748x/adv748x-hdmi.c | 6 ++----
>  drivers/media/i2c/adv748x/adv748x.h      | 2 ++
>  5 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index dbbb1e4d6363..5a25d1fbe25f 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -6,18 +6,16 @@
>   * Copyright (C) 2017 Renesas Electronics Corp.
>   */
>  
> +#include "adv748x.h"
> +
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/v4l2-dv-timings.h>
>  
> -#include <media/v4l2-ctrls.h>
> -#include <media/v4l2-device.h>
>  #include <media/v4l2-dv-timings.h>
>  #include <media/v4l2-ioctl.h>
>  
> -#include "adv748x.h"
> -
>  /* -----------------------------------------------------------------------------
>   * SDP
>   */
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index c3fb113cef62..5c59aad319d1 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -10,6 +10,8 @@
>   *	Kieran Bingham <kieran.bingham@ideasonboard.com>
>   */
>  
> +#include "adv748x.h"
> +
>  #include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/i2c.h>

As linux/i2c.h is included in adv748x.h, we can remove this entry.

> @@ -20,14 +22,10 @@
>  #include <linux/slab.h>
>  #include <linux/v4l2-dv-timings.h>
>  
> -#include <media/v4l2-ctrls.h>
> -#include <media/v4l2-device.h>
>  #include <media/v4l2-dv-timings.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-ioctl.h>
>  
> -#include "adv748x.h"
> -
>  /* -----------------------------------------------------------------------------
>   * Register manipulation
>   */
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index c43ce5d78723..c00d4f347d95 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -5,15 +5,13 @@
>   * Copyright (C) 2017 Renesas Electronics Corp.
>   */
>  
> +#include "adv748x.h"
> +
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  
> -#include <media/v4l2-ctrls.h>
> -#include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  
> -#include "adv748x.h"
> -
>  static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
>  					    unsigned int vc)
>  {
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> index c557f8fdf11a..f598acec3b5c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -5,18 +5,16 @@
>   * Copyright (C) 2017 Renesas Electronics Corp.
>   */
>  
> +#include "adv748x.h"
> +
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  
> -#include <media/v4l2-ctrls.h>
> -#include <media/v4l2-device.h>
>  #include <media/v4l2-dv-timings.h>
>  #include <media/v4l2-ioctl.h>
>  
>  #include <uapi/linux/v4l2-dv-timings.h>
>  
> -#include "adv748x.h"
> -
>  /* -----------------------------------------------------------------------------
>   * HDMI and CP
>   */
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index fccb388ce179..09aab4138c3f 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -19,6 +19,8 @@
>   */
>  
>  #include <linux/i2c.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
>  
>  #ifndef _ADV748X_H_
>  #define _ADV748X_H_
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 1/9] media: adv748x: fix end-of-line terminators in diagnostic statements

[Alex Riesen]

Kieran Bingham, Fri, Apr 03, 2020 12:43:38 +0200:
> Hi Alex,
> 
> On 02/04/2020 19:34, Alex Riesen wrote:
> > Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I guess we could have also added this directly to the helper macros, but
> there is indeed already a mixed usage so either way would require fixups
> to be consistent.
> 
> So this is a good option ;-)

Not in this particular driver (unless I missed it), but yes, this was my main
motivation. Also size of unrelated changes in the patch series and I prefer to
have the ability to format diagnostics sequentially (also used it this time,
only removed the debugging traces later).

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks!

Re: [PATCH v5 2/9] media: adv748x: include everything adv748x.h needs into the file

[Alex Riesen]

Hi Kiran,

Kieran Bingham, Fri, Apr 03, 2020 12:48:06 +0200:
> On 02/04/2020 19:34, Alex Riesen wrote:
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -10,6 +10,8 @@
> >   *	Kieran Bingham <kieran.bingham@ideasonboard.com>
> >   */
> >  
> > +#include "adv748x.h"
> > +
> >  #include <linux/delay.h>
> >  #include <linux/errno.h>
> >  #include <linux/i2c.h>
> 
> As linux/i2c.h is included in adv748x.h, we can remove this entry.
> 

Corrected in v6.

Thanks for review!
Regards,
Alex

Re: [PATCH v5 3/9] media: adv748x: reduce amount of code for bitwise modifications of device registers

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> The regmap provides a convenient utility for this.
> The hdmi_* and dpll_* register modification macros added for symmetry
> with the existing operations (io_*, sdp_*).


Ah yes, perhaps I should have done that when I converted this driver to
regmap :-) (although looking at the history I think that was
pre-submission of the driver, so it's all a long time ago anyway).

This also should prevent the issues we solved in 0d962e061a (media: i2c:
adv748x: Fix unsafe macros), so I think it's still a good move.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> --
> v3: remove _update name in favor of existing _clrset
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c |  6 ++++++
>  drivers/media/i2c/adv748x/adv748x.h      | 14 +++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 5c59aad319d1..8580e6624276 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -133,6 +133,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.h b/drivers/media/i2c/adv748x/adv748x.h
> index 09aab4138c3f..0a9d78c2870b 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -393,25 +393,33 @@ 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_clrset(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_clrset(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_clrset(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)
>  
>  #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)
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  drivers/media/i2c/adv748x/adv748x.h | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 0a9d78c2870b..1a1ea70086c6 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -226,6 +226,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

What is CONTROLS1 (1d) referenced from here?

There's no 'field' matching for this register, and the 'bits' (0, 2, 3,
4) correspond to "pdn_spi, pdn_pix, '-', tri_spi"

Perhaps we need to define those bit fields accordingly and reference
them where they get used directly?

Perhaps calling bit 3 as:
 #define ADV748X_IO_PAD_CONTROLS_BIT_3	BIT(3)

Or
 #define ADV748X_IO_PAD_CONTROLS_RESVD	BIT(3)

(Unless you have documentation that better describes it?)


> +
>  #define ADV748X_IO_10			0x10	/* io_reg_10 */
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> @@ -248,7 +253,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 */

Looks like a more appropriate name than the datasheets
"hdmi_register_03h" :-D


> +#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)

I'd be very tempted to ignore the 80char limit here and put that on the
line above ... or find a way to remove the 1 character...

In fact, given the entry there - how about just leaving this as:

#define ADV748X_HDMI_I2SOUTMODE_MASK	GENMASK(6, 5)


> +#define ADV748X_I2SOUTMODE_I2S 0
> +#define ADV748X_I2SOUTMODE_RIGHT_J 1
> +#define ADV748X_I2SOUTMODE_LEFT_J 2
> +#define ADV748X_I2SOUTMODE_SPDIF 3

Can we align these value in the column with the other values?

And as much as I hate long define names, it seems a bit odd that these
suddenly lack the HDMI_ part of the define prefix...

Should we either remove the HDMI_ from
 ADV748X_HDMI_I2SBITWIDTH_MASK
 ADV748X_HDMI_I2SOUTMODE_SHIFT
 ADV748X_HDMI_I2SOUTMODE_MASK

or add it to
 ADV748X_I2SOUTMODE_I2S
 ADV748X_I2SOUTMODE_RIGHT_J
 ADV748X_I2SOUTMODE_LEFT_J
 ADV748X_I2SOUTMODE_SPDIF

?

> +
>  #define ADV748X_HDMI_LW1		0x07	/* line width_1 */
>  #define ADV748X_HDMI_LW1_VERT_FILTER	BIT(7)
>  #define ADV748X_HDMI_LW1_DE_REGEN	BIT(5)
> @@ -260,6 +279,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

Can we keep the register definitions in address order please?

> +#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)

Those bits do not describe the register they are in, not sure how to
address that without exceptionally long names though.. :-(

So perhaps how you've got them might be the best option...

> +
>  #define ADV748X_HDMI_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
>  #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
>  
> @@ -281,6 +310,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
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Alex Riesen]

Hi Kieran,

Kieran Bingham, Tue, Apr 07, 2020 18:21:00 +0200:
> On 02/04/2020 19:34, Alex Riesen wrote:
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 0a9d78c2870b..1a1ea70086c6 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -226,6 +226,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
> 
> What is CONTROLS1 (1d) referenced from here?

I wish I knew... I afraid this is a left-over from the early development
attempts. It is obviously a mask of some bits. I don't even use the _CONTROLS1
anymore.

Removed the #define.

> There's no 'field' matching for this register, and the 'bits' (0, 2, 3,
> 4) correspond to "pdn_spi, pdn_pix, '-', tri_spi"

> Perhaps we need to define those bit fields accordingly and reference
> them where they get used directly?
> 
> Perhaps calling bit 3 as:
>  #define ADV748X_IO_PAD_CONTROLS_BIT_3	BIT(3)
> 
> Or
>  #define ADV748X_IO_PAD_CONTROLS_RESVD	BIT(3)

I would prefer _BIT_3, if only to stay as opaque as the documentation.

> (Unless you have documentation that better describes it?)

Mine matches what you described above.

Do you mind if I describe the other bits of the register even though the
driver does not use them? Just for completeness sake (and while I still have
access to the documentation).

> > @@ -248,7 +253,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 */
> 
> Looks like a more appropriate name than the datasheets
> "hdmi_register_03h" :-D

It was derived from the map and prefix of its bit-fields: i2soutmode and
i2sbitwidth. I too felt the name hdmi_register_03h lacking of depth.

> > +#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)
> 
> I'd be very tempted to ignore the 80char limit here and put that on the
> line above ... or find a way to remove the 1 character...
> 
> In fact, given the entry there - how about just leaving this as:
> 
> #define ADV748X_HDMI_I2SOUTMODE_MASK	GENMASK(6, 5)

No problem. Reformatted with two spaces.

> > +#define ADV748X_I2SOUTMODE_I2S 0
> > +#define ADV748X_I2SOUTMODE_RIGHT_J 1
> > +#define ADV748X_I2SOUTMODE_LEFT_J 2
> > +#define ADV748X_I2SOUTMODE_SPDIF 3
> 
> Can we align these value in the column with the other values?

Alignment corrected.

> And as much as I hate long define names, it seems a bit odd that these
> suddenly lack the HDMI_ part of the define prefix...
> 
> Should we either remove the HDMI_ from
>  ADV748X_HDMI_I2SBITWIDTH_MASK
>  ADV748X_HDMI_I2SOUTMODE_SHIFT
>  ADV748X_HDMI_I2SOUTMODE_MASK
> 
> or add it to
>  ADV748X_I2SOUTMODE_I2S
>  ADV748X_I2SOUTMODE_RIGHT_J
>  ADV748X_I2SOUTMODE_LEFT_J
>  ADV748X_I2SOUTMODE_SPDIF

Well, I see no reason for them to stand out like this, so perhaps I better add
the prefix. I didn't add the prefix initially because they weren't names of
fields or registers, but names of values of a field (i2soutmode of that
hdmi_register_03h).
But I see there is a precedent for such already:
ADV748X_CP_{CON,SAT,BRI}_{MIN,DEF,MAX}, so prefix is okay.

> > @@ -260,6 +279,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
> 
> Can we keep the register definitions in address order please?

Done.

> > +#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)
> 
> Those bits do not describe the register they are in, not sure how to
> address that without exceptionally long names though.. :-(
> 
> So perhaps how you've got them might be the best option...

Yes, please. Besides, they aren't even obviously related to the audio mute
speed.

And I corrected the alignment.

> > +#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
> > +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)

Alignment corrected.

Regards,
Alex

Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Kieran Bingham]

Hi Alex,

With all the changes you've described below:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

On 07/04/2020 18:13, Alex Riesen wrote:
> Hi Kieran,
> 
> Kieran Bingham, Tue, Apr 07, 2020 18:21:00 +0200:
>> On 02/04/2020 19:34, Alex Riesen wrote:
>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>> index 0a9d78c2870b..1a1ea70086c6 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -226,6 +226,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
>>
>> What is CONTROLS1 (1d) referenced from here?
> 
> I wish I knew... I afraid this is a left-over from the early development
> attempts. It is obviously a mask of some bits. I don't even use the _CONTROLS1
> anymore.
> 
> Removed the #define.
> 
>> There's no 'field' matching for this register, and the 'bits' (0, 2, 3,
>> 4) correspond to "pdn_spi, pdn_pix, '-', tri_spi"
> 
>> Perhaps we need to define those bit fields accordingly and reference
>> them where they get used directly?
>>
>> Perhaps calling bit 3 as:
>>  #define ADV748X_IO_PAD_CONTROLS_BIT_3	BIT(3)
>>
>> Or
>>  #define ADV748X_IO_PAD_CONTROLS_RESVD	BIT(3)
> 
> I would prefer _BIT_3, if only to stay as opaque as the documentation.
> 
>> (Unless you have documentation that better describes it?)
> 
> Mine matches what you described above.
> 
> Do you mind if I describe the other bits of the register even though the
> driver does not use them? Just for completeness sake (and while I still have
> access to the documentation).

I'm fine describing those extra BIT()s correctly.

> 
>>> @@ -248,7 +253,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 */
>>
>> Looks like a more appropriate name than the datasheets
>> "hdmi_register_03h" :-D
> 
> It was derived from the map and prefix of its bit-fields: i2soutmode and
> i2sbitwidth. I too felt the name hdmi_register_03h lacking of depth.
> 
>>> +#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)
>>
>> I'd be very tempted to ignore the 80char limit here and put that on the
>> line above ... or find a way to remove the 1 character...
>>
>> In fact, given the entry there - how about just leaving this as:
>>
>> #define ADV748X_HDMI_I2SOUTMODE_MASK	GENMASK(6, 5)
> 
> No problem. Reformatted with two spaces.
> 
>>> +#define ADV748X_I2SOUTMODE_I2S 0
>>> +#define ADV748X_I2SOUTMODE_RIGHT_J 1
>>> +#define ADV748X_I2SOUTMODE_LEFT_J 2
>>> +#define ADV748X_I2SOUTMODE_SPDIF 3
>>
>> Can we align these value in the column with the other values?
> 
> Alignment corrected.
> 
>> And as much as I hate long define names, it seems a bit odd that these
>> suddenly lack the HDMI_ part of the define prefix...
>>
>> Should we either remove the HDMI_ from
>>  ADV748X_HDMI_I2SBITWIDTH_MASK
>>  ADV748X_HDMI_I2SOUTMODE_SHIFT
>>  ADV748X_HDMI_I2SOUTMODE_MASK
>>
>> or add it to
>>  ADV748X_I2SOUTMODE_I2S
>>  ADV748X_I2SOUTMODE_RIGHT_J
>>  ADV748X_I2SOUTMODE_LEFT_J
>>  ADV748X_I2SOUTMODE_SPDIF
> 
> Well, I see no reason for them to stand out like this, so perhaps I better add
> the prefix. I didn't add the prefix initially because they weren't names of
> fields or registers, but names of values of a field (i2soutmode of that
> hdmi_register_03h).
> But I see there is a precedent for such already:
> ADV748X_CP_{CON,SAT,BRI}_{MIN,DEF,MAX}, so prefix is okay.
> 
>>> @@ -260,6 +279,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
>>
>> Can we keep the register definitions in address order please?
> 
> Done.
> 
>>> +#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)
>>
>> Those bits do not describe the register they are in, not sure how to
>> address that without exceptionally long names though.. :-(
>>
>> So perhaps how you've got them might be the best option...
> 
> Yes, please. Besides, they aren't even obviously related to the audio mute
> speed.
> 
> And I corrected the alignment.
> 
>>> +#define ADV748X_HDMI_REG_6D		0x6d	/* hdmi_reg_6d */
>>> +#define ADV748X_I2S_TDM_MODE_ENABLE BIT(7)
> 
> Alignment corrected.
> 
> Regards,
> Alex
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 4/9] media: adv748x: add definitions for audio output related registers

[Alex Riesen]

Kieran Bingham, Tue, Apr 07, 2020 20:44:04 +0200:
> Hi Alex,
> 
> With all the changes you've described below:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 

Thanks. Will be in v6 like this below:

diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 0a9d78c2870b..e1d8cb01ebf8 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -226,6 +226,16 @@ struct adv748x_state {
 
 #define ADV748X_IO_VID_STD		0x05
 
+#define ADV748X_IO_PAD_CONTROLS		0x0e
+#define ADV748X_IO_PAD_CONTROLS_TRI_LLC	BIT(7)
+#define ADV748X_IO_PAD_CONTROLS_TRI_PIX	BIT(6)
+#define ADV748X_IO_PAD_CONTROLS_TRI_AUD	BIT(5)
+#define ADV748X_IO_PAD_CONTROLS_TRI_SPI	BIT(4)
+#define ADV748X_IO_PAD_CONTROLS_BIT_3	BIT(3)
+#define ADV748X_IO_PAD_CONTROLS_PDN_PIX	BIT(2)
+#define ADV748X_IO_PAD_CONTROLS_PDN_AUD	BIT(1)
+#define ADV748X_IO_PAD_CONTROLS_PDN_SPI	BIT(0)
+
 #define ADV748X_IO_10			0x10	/* io_reg_10 */
 #define ADV748X_IO_10_CSI4_EN		BIT(7)
 #define ADV748X_IO_10_CSI1_EN		BIT(6)
@@ -248,7 +258,20 @@ 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_HDMI_I2SOUTMODE_I2S	0
+#define ADV748X_HDMI_I2SOUTMODE_RIGHT_J	1
+#define ADV748X_HDMI_I2SOUTMODE_LEFT_J	2
+#define ADV748X_HDMI_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)
@@ -260,6 +283,16 @@ struct adv748x_state {
 #define ADV748X_HDMI_F1H1		0x0b	/* field1 height_1 */
 #define ADV748X_HDMI_F1H1_INTERLACED	BIT(5)
 
+#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_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_HFRONT_PORCH	0x20	/* hsync_front_porch_1 */
 #define ADV748X_HDMI_HFRONT_PORCH_MASK	0x1fff
 
@@ -281,6 +314,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

Re: [PATCH v5 7/9] media: adv748x: only activate DAI if it is described in device tree

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> To avoid setting it up even if the hardware is not actually connected
> to anything physically.
> 
> Besides, the bindings explicitly notes that port definitions are
> "optional if they are not connected to anything at the hardware level".
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-dai.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
> index 185f78023e91..f9cc47fa9ad1 100644
> --- a/drivers/media/i2c/adv748x/adv748x-dai.c
> +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> @@ -216,6 +216,11 @@ int adv748x_dai_init(struct adv748x_dai *dai)
>  	int ret;
>  	struct adv748x_state *state = adv748x_dai_to_state(dai);
>  
> +	if (!state->endpoints[ADV748X_PORT_I2S]) {
> +		adv_info(state, "no I2S port, DAI disabled\n");
> +		ret = 0;
> +		goto fail;

How about just 'return 0'?

> +	}

And a blank line here ...

Otherwise,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

This could also be folded into 5/9 too I guess?, though it is easier to
review the sequential additions, rather than the one-big-feature
addition ;-)


>  	dai->mclk_name = kasprintf(GFP_KERNEL, "%s.%s-i2s-mclk",
>  				   state->dev->driver->name,
>  				   dev_name(state->dev));
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 6/9] media: adv748x: prepare/enable mclk when the audio is used

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:34, Alex Riesen wrote:
> As there is nothing else (the consumers are supposed to do that) which
> enables the clock, do it in the driver.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> --
> 
> v3: added
> ---
>  drivers/media/i2c/adv748x/adv748x-dai.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
> index c9191f8f1ca8..185f78023e91 100644
> --- a/drivers/media/i2c/adv748x/adv748x-dai.c
> +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> @@ -117,11 +117,22 @@ static int adv748x_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  
>  static int adv748x_dai_startup(struct snd_pcm_substream *sub, struct snd_soc_dai *dai)
>  {
> +	int ret;
>  	struct adv748x_state *state = state_of(dai);
>  
>  	if (sub->stream != SNDRV_PCM_STREAM_CAPTURE)
>  		return -EINVAL;
this looks quite bunched up so :

Newline...

> -	return set_audio_pads_state(state, 1);
> +	ret = set_audio_pads_state(state, 1);
> +	if (ret)
> +		goto fail;

With no action required to cleanup here, I would just
		return ret;
and remove the fail: label.


Newline...

> +	ret = clk_prepare_enable(mclk_of(state));
> +	if (ret)
> +		goto fail_pwdn;

newline...

> +	return 0;

newline...

Other than that:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> +fail_pwdn:
> +	set_audio_pads_state(state, 0);
> +fail:
> +	return ret;
>  }
>  
>  static int adv748x_dai_hw_params(struct snd_pcm_substream *sub,
> @@ -174,6 +185,7 @@ static void adv748x_dai_shutdown(struct snd_pcm_substream *sub, struct snd_soc_d
>  {
>  	struct adv748x_state *state = state_of(dai);
>  
> +	clk_disable_unprepare(mclk_of(state));
>  	set_audio_pads_state(state, 0);
>  }
>  
> 

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

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:35, 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>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> --
> 
> v3: remove optionality off MCLK clock cell to ensure the description
>     matches the hardware no matter if the line is connected.
>     Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  .../devicetree/bindings/media/i2c/adv748x.txt    | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> index 4f91686e54a6..50a753189b81 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:
>  
> @@ -16,6 +18,8 @@ Required Properties:
>      slave device on the I2C bus. The main address is mandatory, others are
>      optional and remain at default values if not specified.
>  
> +  - #clock-cells: must be <0>
> +
>  Optional Properties:
>  
>    - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or
> @@ -47,6 +51,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
> @@ -72,6 +77,7 @@ Example:
>  
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> +		#clock-cells = <0>;
>  
>  		interrupt-parent = <&gpio6>;
>  		interrupt-names = "intrq1", "intrq2";
> @@ -113,4 +119,12 @@ Example:
>  				remote-endpoint = <&csi20_in>;
>  			};
>  		};
> +
> +		port@c {
> +			reg = <12>;
> +
> +			adv7482_i2s: endpoint {
> +				remote-endpoint = <&i2s_in>;
> +			};
> +		};
>  	};
> 

-- 
Regards
--
Kieran

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

[Kieran Bingham]

Hi Alex,

On 02/04/2020 19:35, Alex Riesen wrote:
> As all known variants of the Salvator board have the HDMI decoder
> chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482
> endpoint and the connection definitions are placed in the common board
> file.
> 
> For the same reason, the CLK_C clock line and I2C configuration (similar
> to the ak4613, on the same interface) are added into the common file.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> 
> --
> 
> v5: Add dummy ssi4 node to the rcar sound card in r8a77961, as the
>     devices (Salvator-X 2nd version with R-Car M3 W+) also reference
>     salvator-common.dtsi.
>     Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> v2: Also add ssi4_ctrl pin group in the sound pins. The pins are
>     responsible for SCK4 (sample clock) WS4 and (word boundary input),
>     and are required for SSI audio input over I2S.
> 
>     The adv748x shall provide its own implementation of the output clock
>     (MCLK), connected to the audio_clk_c line of the R-Car SoC.
> 
>     If the frequency of the ADV748x MCLK were fixed, the clock
>     implementation were not necessary, but it does not seem so: the MCLK
>     depends on the value in a speed multiplier register and the input sample
>     rate (48kHz).
> 
>     Remove audio clock C from the clocks of adv7482.
> 
>     The clocks property of the video-receiver node lists the input
>     clocks of the device, which is quite the opposite from the
>     original intention: the adv7482 on Salvator X boards is a
>     provide of the MCLK clock for I2S audio output.
> 
>     Remove old definition of &sound_card.dais and reduce size of changes
>     in the Salvator-X specific device tree source.
> 
>     Declare video-receiver a clock producer, as the adv748x driver
>     implements the master clock used I2S audio output.
> 
>     Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> v2: The driver provides only MCLK clock, not the SCLK and LRCLK,
>     which are part of the I2S protocol.
> 
>     Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../boot/dts/renesas/r8a77950-salvator-x.dts  |  3 +-
>  arch/arm64/boot/dts/renesas/r8a77961.dtsi     |  1 +
>  .../boot/dts/renesas/salvator-common.dtsi     | 47 +++++++++++++++++--
>  3 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> index 2438825c9b22..e16c146808b6 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> @@ -146,7 +146,8 @@ &sata {
>  &sound_card {
>  	dais = <&rsnd_port0	/* ak4613 */
>  		&rsnd_port1	/* HDMI0  */
> -		&rsnd_port2>;	/* HDMI1  */
> +		&rsnd_port2	/* HDMI1  */
> +		&rsnd_port3>;	/* adv7482 hdmi-in  */

Ah - that was confusing at first... but HDMI0 and HDMI1 are *outputs*,
where of course the adv7482 is an input ;-)


Otherwise, I can't spot anything else yet so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

But I fear there may have been some churn around here, so it would be
good to see a rebase too.

--
Kieran



>  };
>  
>  &usb2_phy2 {
> diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> index be3824bda632..b79907beaf31 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> @@ -861,6 +861,7 @@ rcar_sound,src {
>  			rcar_sound,ssi {
>  				ssi0: ssi-0 { };
>  				ssi1: ssi-1 { };
> +				ssi4: ssi-4 { };
>  			};
>  		};
>  
> diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> index 98bbcafc8c0d..ead7f8d7a929 100644
> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
> @@ -460,7 +460,7 @@ pca9654: gpio@20 {
>  		#gpio-cells = <2>;
>  	};
>  
> -	video-receiver@70 {
> +	adv7482_hdmi_in: video-receiver@70 {
>  		compatible = "adi,adv7482";
>  		reg = <0x70 0x71 0x72 0x73 0x74 0x75
>  		       0x60 0x61 0x62 0x63 0x64 0x65>;
> @@ -469,6 +469,7 @@ video-receiver@70 {
>  
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> +		#clock-cells = <0>; /* the MCLK for I2S output */
>  
>  		interrupt-parent = <&gpio6>;
>  		interrupt-names = "intrq1", "intrq2";
> @@ -510,6 +511,15 @@ adv7482_txb: endpoint {
>  				remote-endpoint = <&csi20_in>;
>  			};
>  		};
> +
> +		port@c {
> +			reg = <12>;
> +
> +			adv7482_i2s: endpoint {
> +				remote-endpoint = <&rsnd_endpoint3>;
> +				system-clock-direction-out;
> +			};
> +		};
>  	};
>  
>  	csa_vdd: adc@7c {
> @@ -684,7 +694,8 @@ sdhi3_pins_uhs: sd3_uhs {
>  	};
>  
>  	sound_pins: sound {
> -		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
> +		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
> +			 "ssi4_data", "ssi4_ctrl";
>  		function = "ssi";
>  	};
>  
> @@ -733,8 +744,8 @@ &rcar_sound {
>  	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>;
> @@ -758,8 +769,19 @@ &rcar_sound {
>  		 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
>  		 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
>  		 <&audio_clk_a>, <&cs2000>,
> -		 <&audio_clk_c>,
> +		 <&adv7482_hdmi_in>,
>  		 <&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_c", "clk_i";
>  
>  	ports {
>  		#address-cells = <1>;
> @@ -777,6 +799,21 @@ rsnd_endpoint0: endpoint {
>  				capture  = <&ssi1 &src1 &dvc1>;
>  			};
>  		};
> +		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>;
> +
> +				capture = <&ssi4>;
> +			};
> +		};
>  	};
>  };
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v5 6/9] media: adv748x: prepare/enable mclk when the audio is used

[Alex Riesen]

Kieran Bingham, Thu, Jun 18, 2020 18:23:14 +0200:
> On 02/04/2020 19:34, Alex Riesen wrote:
> > --- a/drivers/media/i2c/adv748x/adv748x-dai.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> > @@ -117,11 +117,22 @@ static int adv748x_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> >  
> >  static int adv748x_dai_startup(struct snd_pcm_substream *sub, struct snd_soc_dai *dai)
> >  {
> > +	int ret;
> >  	struct adv748x_state *state = state_of(dai);
> >  
> >  	if (sub->stream != SNDRV_PCM_STREAM_CAPTURE)
> >  		return -EINVAL;
> this looks quite bunched up so :
> 
> Newline...

Will do.

> > -	return set_audio_pads_state(state, 1);
> > +	ret = set_audio_pads_state(state, 1);
> > +	if (ret)
> > +		goto fail;
> 
> With no action required to cleanup here, I would just
> 		return ret;
> and remove the fail: label.

Of course.

> > +	ret = clk_prepare_enable(mclk_of(state));
> > +	if (ret)
> > +		goto fail_pwdn;
> 
> newline...
> 
> > +	return 0;
> 
> newline...
> 
> Other than that:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks!

Re: [PATCH v5 7/9] media: adv748x: only activate DAI if it is described in device tree

[Alex Riesen]

Kieran Bingham, Thu, Jun 18, 2020 18:17:04 +0200:
> On 02/04/2020 19:34, Alex Riesen wrote:
> > To avoid setting it up even if the hardware is not actually connected
> > to anything physically.
> > 
> > Besides, the bindings explicitly notes that port definitions are
> > "optional if they are not connected to anything at the hardware level".
> > 
> > Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-dai.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
> > index 185f78023e91..f9cc47fa9ad1 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-dai.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> > @@ -216,6 +216,11 @@ int adv748x_dai_init(struct adv748x_dai *dai)
> >  	int ret;
> >  	struct adv748x_state *state = adv748x_dai_to_state(dai);
> >  
> > +	if (!state->endpoints[ADV748X_PORT_I2S]) {
> > +		adv_info(state, "no I2S port, DAI disabled\n");
> > +		ret = 0;
> > +		goto fail;
> 
> How about just 'return 0'?

Indeed. In the retrospect, the whole event of loading the DAI driver does not
feel that important anymore to warrant logging on info prio.

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> This could also be folded into 5/9 too I guess?, though it is easier to
> review the sequential additions, rather than the one-big-feature
> addition ;-)

I would prefer to have it separately, if you don't mind: maybe not a big one,
but loading a driver without hardware for it *is* an event.

Thanks,
Alex

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

[Alex Riesen]

Kieran Bingham, Thu, Jun 18, 2020 18:32:55 +0200:
> On 02/04/2020 19:35, Alex Riesen wrote:
> > --- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> > +++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> > @@ -146,7 +146,8 @@ &sata {
> >  &sound_card {
> >  	dais = <&rsnd_port0	/* ak4613 */
> >  		&rsnd_port1	/* HDMI0  */
> > -		&rsnd_port2>;	/* HDMI1  */
> > +		&rsnd_port2	/* HDMI1  */
> > +		&rsnd_port3>;	/* adv7482 hdmi-in  */
> 
> Ah - that was confusing at first... but HDMI0 and HDMI1 are *outputs*,
> where of course the adv7482 is an input ;-)

And ak4613 is both... Why? Are the "dais" of a sound_card more commonly
outputs than inputs?

> Otherwise, I can't spot anything else yet so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> But I fear there may have been some churn around here, so it would be
> good to see a rebase too.

Just rebased the series on top of linux-media/master and the only conflict was
in adding ssi4 node to rcar_sound,ssi in r8a77961.dtsi, as there was an
addition of ssi2 in the same line.

Thanks,
Alex

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

[Kieran Bingham]

Hi Alex,

On 18/06/2020 17:32, Kieran Bingham wrote:
> Hi Alex,
> 
> On 02/04/2020 19:35, Alex Riesen wrote:
>> As all known variants of the Salvator board have the HDMI decoder
>> chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482
>> endpoint and the connection definitions are placed in the common board
>> file.
>>
>> For the same reason, the CLK_C clock line and I2C configuration (similar
>> to the ak4613, on the same interface) are added into the common file.
>>
>> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
>>
>> --
>>
>> v5: Add dummy ssi4 node to the rcar sound card in r8a77961, as the
>>     devices (Salvator-X 2nd version with R-Car M3 W+) also reference
>>     salvator-common.dtsi.
>>     Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>
>> v2: Also add ssi4_ctrl pin group in the sound pins. The pins are
>>     responsible for SCK4 (sample clock) WS4 and (word boundary input),
>>     and are required for SSI audio input over I2S.
>>
>>     The adv748x shall provide its own implementation of the output clock
>>     (MCLK), connected to the audio_clk_c line of the R-Car SoC.
>>
>>     If the frequency of the ADV748x MCLK were fixed, the clock
>>     implementation were not necessary, but it does not seem so: the MCLK
>>     depends on the value in a speed multiplier register and the input sample
>>     rate (48kHz).
>>
>>     Remove audio clock C from the clocks of adv7482.
>>
>>     The clocks property of the video-receiver node lists the input
>>     clocks of the device, which is quite the opposite from the
>>     original intention: the adv7482 on Salvator X boards is a
>>     provide of the MCLK clock for I2S audio output.
>>
>>     Remove old definition of &sound_card.dais and reduce size of changes
>>     in the Salvator-X specific device tree source.
>>
>>     Declare video-receiver a clock producer, as the adv748x driver
>>     implements the master clock used I2S audio output.
>>
>>     Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>
>> v2: The driver provides only MCLK clock, not the SCLK and LRCLK,
>>     which are part of the I2S protocol.
>>
>>     Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  .../boot/dts/renesas/r8a77950-salvator-x.dts  |  3 +-
>>  arch/arm64/boot/dts/renesas/r8a77961.dtsi     |  1 +
>>  .../boot/dts/renesas/salvator-common.dtsi     | 47 +++++++++++++++++--

Once again I'm back trying to test this series, and one issue I've had
is that the board I have (r8a77951-salvator-xs.dts) isn't included in
this DT update.

For v6, Should we include the relevant changes to all the following?

arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77951-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77960-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77965-salvator-x.dts
arch/arm64/boot/dts/renesas/salvator-x.dtsi

And perhaps handle the salvator-xs in a second (yet very similar) patch?

arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77960-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77961-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77965-salvator-xs.dts
arch/arm64/boot/dts/renesas/salvator-xs.dtsi

I think I've added the relevant entries to my dtb, but I haven't
successfully captured audio yet.

I can see the device being listed through arecord:

kbingham@salvator-xs:~$ arecord -l
**** List of CAPTURE Hardware Devices ****
card 0: rcarsound [rcar-sound], device 0: rsnd-dai.0-ak4613-hifi
ak4613-hifi-0 []
  Subdevices: 0/1
  Subdevice #0: subdevice #0
card 0: rcarsound [rcar-sound], device 3: rsnd-dai.3-adv748x-i2s
adv748x.4-0070-3 []
  Subdevices: 1/1
  Subdevice #0: subdevice #0


But as yet, everything I try to record fails or is empty silence.

Debugging ...

--
Regards

Kieran



>>  3 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> index 2438825c9b22..e16c146808b6 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> @@ -146,7 +146,8 @@ &sata {
>>  &sound_card {
>>  	dais = <&rsnd_port0	/* ak4613 */
>>  		&rsnd_port1	/* HDMI0  */
>> -		&rsnd_port2>;	/* HDMI1  */
>> +		&rsnd_port2	/* HDMI1  */
>> +		&rsnd_port3>;	/* adv7482 hdmi-in  */
> 
> Ah - that was confusing at first... but HDMI0 and HDMI1 are *outputs*,
> where of course the adv7482 is an input ;-)
> 
> 
> Otherwise, I can't spot anything else yet so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> But I fear there may have been some churn around here, so it would be
> good to see a rebase too.
> 
> --
> Kieran
> 
> 
> 
>>  };
>>  
>>  &usb2_phy2 {
>> diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
>> index be3824bda632..b79907beaf31 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
>> @@ -861,6 +861,7 @@ rcar_sound,src {
>>  			rcar_sound,ssi {
>>  				ssi0: ssi-0 { };
>>  				ssi1: ssi-1 { };
>> +				ssi4: ssi-4 { };
>>  			};
>>  		};
>>  
>> diff --git a/arch/arm64/boot/dts/renesas/salvator-common.dtsi b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
>> index 98bbcafc8c0d..ead7f8d7a929 100644
>> --- a/arch/arm64/boot/dts/renesas/salvator-common.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/salvator-common.dtsi
>> @@ -460,7 +460,7 @@ pca9654: gpio@20 {
>>  		#gpio-cells = <2>;
>>  	};
>>  
>> -	video-receiver@70 {
>> +	adv7482_hdmi_in: video-receiver@70 {
>>  		compatible = "adi,adv7482";
>>  		reg = <0x70 0x71 0x72 0x73 0x74 0x75
>>  		       0x60 0x61 0x62 0x63 0x64 0x65>;
>> @@ -469,6 +469,7 @@ video-receiver@70 {
>>  
>>  		#address-cells = <1>;
>>  		#size-cells = <0>;
>> +		#clock-cells = <0>; /* the MCLK for I2S output */
>>  
>>  		interrupt-parent = <&gpio6>;
>>  		interrupt-names = "intrq1", "intrq2";
>> @@ -510,6 +511,15 @@ adv7482_txb: endpoint {
>>  				remote-endpoint = <&csi20_in>;
>>  			};
>>  		};
>> +
>> +		port@c {
>> +			reg = <12>;
>> +
>> +			adv7482_i2s: endpoint {
>> +				remote-endpoint = <&rsnd_endpoint3>;
>> +				system-clock-direction-out;
>> +			};
>> +		};
>>  	};
>>  
>>  	csa_vdd: adc@7c {
>> @@ -684,7 +694,8 @@ sdhi3_pins_uhs: sd3_uhs {
>>  	};
>>  
>>  	sound_pins: sound {
>> -		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a";
>> +		groups = "ssi01239_ctrl", "ssi0_data", "ssi1_data_a",
>> +			 "ssi4_data", "ssi4_ctrl";
>>  		function = "ssi";
>>  	};
>>  
>> @@ -733,8 +744,8 @@ &rcar_sound {
>>  	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>;
>> @@ -758,8 +769,19 @@ &rcar_sound {
>>  		 <&cpg CPG_MOD 1020>, <&cpg CPG_MOD 1021>,
>>  		 <&cpg CPG_MOD 1019>, <&cpg CPG_MOD 1018>,
>>  		 <&audio_clk_a>, <&cs2000>,
>> -		 <&audio_clk_c>,
>> +		 <&adv7482_hdmi_in>,
>>  		 <&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_c", "clk_i";
>>  
>>  	ports {
>>  		#address-cells = <1>;
>> @@ -777,6 +799,21 @@ rsnd_endpoint0: endpoint {
>>  				capture  = <&ssi1 &src1 &dvc1>;
>>  			};
>>  		};
>> +		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>;
>> +
>> +				capture = <&ssi4>;
>> +			};
>> +		};
>>  	};
>>  };
>>  
>>
> 

-- 
Regards
--
Kieran

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

[Alex Riesen]

Hi Kieran,

Kieran Bingham, Tue, Aug 25, 2020 16:57:04 +0200:
> On 18/06/2020 17:32, Kieran Bingham wrote:
> > On 02/04/2020 19:35, Alex Riesen wrote:
> >> As all known variants of the Salvator board have the HDMI decoder
> >> chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482
> >> endpoint and the connection definitions are placed in the common board
> >> file.
> >>
> >> For the same reason, the CLK_C clock line and I2C configuration (similar
> >> to the ak4613, on the same interface) are added into the common file.
> >> ...
> >> ---
> >>  .../boot/dts/renesas/r8a77950-salvator-x.dts  |  3 +-
> >>  arch/arm64/boot/dts/renesas/r8a77961.dtsi     |  1 +
> >>  .../boot/dts/renesas/salvator-common.dtsi     | 47 +++++++++++++++++--
> 
> Once again I'm back trying to test this series, and one issue I've had
> is that the board I have (r8a77951-salvator-xs.dts) isn't included in
> this DT update.
> 
> For v6, Should we include the relevant changes to all the following?

Ok. I shall add them as a separate patch though, as I have no way to verify
those boards (and some verification seem to be in order...)

> arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> arch/arm64/boot/dts/renesas/r8a77951-salvator-x.dts
> arch/arm64/boot/dts/renesas/r8a77960-salvator-x.dts
> arch/arm64/boot/dts/renesas/r8a77965-salvator-x.dts
> arch/arm64/boot/dts/renesas/salvator-x.dtsi
> 
> And perhaps handle the salvator-xs in a second (yet very similar) patch?
> 
> arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts
> arch/arm64/boot/dts/renesas/r8a77960-salvator-xs.dts
> arch/arm64/boot/dts/renesas/r8a77961-salvator-xs.dts
> arch/arm64/boot/dts/renesas/r8a77965-salvator-xs.dts
> arch/arm64/boot/dts/renesas/salvator-xs.dtsi
> 
> I think I've added the relevant entries to my dtb, but I haven't
> successfully captured audio yet.
> 
> I can see the device being listed through arecord:
> 
> kbingham@salvator-xs:~$ arecord -l
> **** List of CAPTURE Hardware Devices ****
> card 0: rcarsound [rcar-sound], device 0: rsnd-dai.0-ak4613-hifi ak4613-hifi-0 []
>   Subdevices: 0/1
>   Subdevice #0: subdevice #0
> card 0: rcarsound [rcar-sound], device 3: rsnd-dai.3-adv748x-i2s adv748x.4-0070-3 []
>   Subdevices: 1/1
>   Subdevice #0: subdevice #0
> 
> But as yet, everything I try to record fails or is empty silence.
> 
> Debugging ...

Does it fail somewhere in the ASoC infrastructure? If so, how'd you find out
where exactly and what fails?

Asking, because when I was writing this code I ended up adding quite a bit of
tracing into the SoC core to figure that out, and I just hope there is a
better way to get at the diagnostics.

> >> diff --git a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> >> index 2438825c9b22..e16c146808b6 100644
> >> --- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> >> +++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
> >> @@ -146,7 +146,8 @@ &sata {
> >>  &sound_card {
> >>  	dais = <&rsnd_port0	/* ak4613 */
> >>  		&rsnd_port1	/* HDMI0  */
> >> -		&rsnd_port2>;	/* HDMI1  */
> >> +		&rsnd_port2	/* HDMI1  */
> >> +		&rsnd_port3>;	/* adv7482 hdmi-in  */
> > 
> > Ah - that was confusing at first... but HDMI0 and HDMI1 are *outputs*,
> > where of course the adv7482 is an input ;-)

I shall add an "output" to HDMI0 and HDMI1.

> > Otherwise, I can't spot anything else yet so:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks!

> > But I fear there may have been some churn around here, so it would be
> > good to see a rebase too.

Of course, I shall rebase on top of linux-media/master.
Should I wait with submission until you get data out of your boards?

Regards,
Alex