[PATCH v2 00/10] media: adv748x: add support for HDMI audio

[PATCH v2 00/10] 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 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 (10):
  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: 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
  media: adv748x: add support for log_status ioctl
  media: adv748x: allow the HDMI sub-device to accept EDID

 .../devicetree/bindings/media/i2c/adv748x.txt |  16 +-
 .../boot/dts/renesas/r8a77950-salvator-x.dts  |   3 +-
 .../boot/dts/renesas/salvator-common.dtsi     |  47 +++-
 drivers/media/i2c/adv748x/Makefile            |   3 +-
 drivers/media/i2c/adv748x/adv748x-afe.c       |   2 -
 drivers/media/i2c/adv748x/adv748x-core.c      |  56 +++-
 drivers/media/i2c/adv748x/adv748x-csi2.c      |   4 +-
 drivers/media/i2c/adv748x/adv748x-dai.c       | 261 ++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x-hdmi.c      | 208 +++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h           |  67 ++++-
 10 files changed, 633 insertions(+), 34 deletions(-)
 create mode 100644 drivers/media/i2c/adv748x/adv748x-dai.c

-- 
2.25.1.25.g9ecbe7eb18

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 01/10] media: adv748x: fix end-of-line terminators in diagnostic statements

[Alex Riesen]

Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 02/10] 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.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index dbbb1e4d6363..ab0479641c10 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -11,8 +11,6 @@
 #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>
 
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index c3fb113cef62..345f009de121 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -20,8 +20,6 @@
 #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>
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index c43ce5d78723..78d391009b5a 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -8,8 +8,6 @@
 #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"
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index c557f8fdf11a..0dffcdf79ff2 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -8,8 +8,6 @@
 #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>
 
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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 03/10] media: adv748x: reduce amount of code for bitwise modifications of device registers

[Alex Riesen]

The regmap provides a convenient utility for this.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 345f009de121..36164a254d7b 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..c5245464fffc 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -393,25 +393,34 @@ 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 io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
 
 #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
 #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, (r)+1)) & (m))
 #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
+#define hdmi_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
+
+#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
+#define dpll_update(s, r, m, v) \
+	adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
 
 #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
 #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)
 
 #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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 04/10] 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 c5245464fffc..7bc1bb0b3756 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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 05/10] 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>
---
 drivers/media/i2c/adv748x/Makefile       |   3 +-
 drivers/media/i2c/adv748x/adv748x-core.c |   9 +-
 drivers/media/i2c/adv748x/adv748x-dai.c  | 256 +++++++++++++++++++++++
 drivers/media/i2c/adv748x/adv748x.h      |  16 +-
 4 files changed, 281 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 36164a254d7b..2c0bd58038e6 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..4775a0c7ed7f
--- /dev/null
+++ b/drivers/media/i2c/adv748x/adv748x-dai.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for Analog Devices ADV748X HDMI receiver with AFE
+ * The implementation of DAI.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <sound/pcm_params.h>
+
+#include "adv748x.h"
+
+#define state_of(soc_dai) \
+	adv748x_dai_to_state(container_of((soc_dai)->driver, \
+					  struct adv748x_dai, \
+					  drv))
+
+static const char ADV748X_DAI_NAME[] = "adv748x-i2s";
+
+static int set_audio_pads_state(struct adv748x_state *state, int on)
+{
+	return io_update(state, ADV748X_IO_PAD_CONTROLS,
+			 ADV748X_IO_PAD_CONTROLS_TRI_AUD |
+			 ADV748X_IO_PAD_CONTROLS_PDN_AUD,
+			 on ? 0 : 0xff);
+}
+
+static int set_dpll_mclk_fs(struct adv748x_state *state, int fs)
+{
+	return dpll_update(state, ADV748X_DPLL_MCLK_FS,
+			   ADV748X_DPLL_MCLK_FS_N_MASK, (fs / 128) - 1);
+}
+
+static int set_i2s_format(struct adv748x_state *state, uint outmode,
+			  uint bitwidth)
+{
+	return hdmi_update(state, ADV748X_HDMI_I2S,
+			   ADV748X_HDMI_I2SBITWIDTH_MASK |
+			   ADV748X_HDMI_I2SOUTMODE_MASK,
+			   (outmode << ADV748X_HDMI_I2SOUTMODE_SHIFT) |
+			   bitwidth);
+}
+
+static int set_i2s_tdm_mode(struct adv748x_state *state, int is_tdm)
+{
+	int ret;
+
+	ret = hdmi_update(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
+			  ADV748X_MAN_AUDIO_DL_BYPASS |
+			  ADV748X_AUDIO_DELAY_LINE_BYPASS,
+			  is_tdm ? 0xff : 0);
+	if (ret < 0)
+		return ret;
+	ret = hdmi_update(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_update(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(state->dai.mclk)) {
+		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 = clk_register_fixed_rate(state->dev,
+					    "adv748x-hdmi-i2s-mclk",
+					    NULL /* parent_name */,
+					    0 /* flags */,
+					    12288000 /* rate */);
+	if (IS_ERR_OR_NULL(dai->mclk)) {
+		ret = PTR_ERR(dai->mclk);
+		adv_err(state, "Failed to register MCLK (%d)\n", ret);
+		goto fail;
+	}
+	ret = of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get,
+				  dai->mclk);
+	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 = (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_unregister_fixed_rate(dai->mclk);
+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_unregister_fixed_rate(dai->mclk);
+}
+
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 7bc1bb0b3756..af70632926b5 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,12 @@ 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 *mclk;
+	unsigned int freq, fmt, tdm;
+};
+
 /**
  * struct adv748x_state - State of ADV748X
  * @dev:		(OF) device
@@ -182,6 +190,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 +206,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)
@@ -485,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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 06/10] 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 4775a0c7ed7f..ce3ae7de765c 100644
--- a/drivers/media/i2c/adv748x/adv748x-dai.c
+++ b/drivers/media/i2c/adv748x/adv748x-dai.c
@@ -204,6 +204,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 = clk_register_fixed_rate(state->dev,
 					    "adv748x-hdmi-i2s-mclk",
 					    NULL /* parent_name */,
-- 
2.25.1.25.g9ecbe7eb18


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 07/10] 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.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
---
 .../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..7d6db052c294 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> if the I2S port is used
+
 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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 08/10] 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>
Reviewed-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.

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

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).

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

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.

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

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>

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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../boot/dts/renesas/r8a77950-salvator-x.dts  |  3 +-
 .../boot/dts/renesas/salvator-common.dtsi     | 47 +++++++++++++++++--
 2 files changed, 44 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/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


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 09/10] media: adv748x: add support for log_status ioctl

[Alex Riesen]

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

This change adds a device-specific wrapper for register block read,
because some of the devices I2C-accessible registers (for instance,
cs_data for stereo channel information or tmds_params for TMDS channel
information) located in adjacent cells. According to manufacturers
information, these registers can be read using block transactions.

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

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 2c0bd58038e6..fbc502ad12b5 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -95,6 +95,21 @@ static const struct adv748x_register_map adv748x_default_addresses[] = {
 	[ADV748X_PAGE_TXA] = { "txa", 0x4a },
 };
 
+int adv748x_read_block(struct adv748x_state *state, u8 client_page, u8 reg,
+		       void *val, size_t reg_count)
+{
+	struct i2c_client *client = state->i2c_clients[client_page];
+	int err;
+
+	err = regmap_bulk_read(state->regmap[client_page], reg, val, reg_count);
+	if (err) {
+		adv_err(state, "error reading %02x, %02x-%02lx: %d\n",
+				client->addr, reg, reg + reg_count - 1, err);
+		return err;
+	}
+	return 0;
+}
+
 static int adv748x_read_check(struct adv748x_state *state,
 			      int client_page, u8 reg)
 {
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 0dffcdf79ff2..7655d817ceb6 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Renesas Electronics Corp.
  */
 
+#include <linux/version.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 
@@ -601,11 +602,189 @@ static const struct v4l2_subdev_pad_ops adv748x_pad_ops_hdmi = {
 	.enum_dv_timings = adv748x_hdmi_enum_dv_timings,
 };
 
+struct tmds_params {
+	u32 cts, n;
+	u16 tmdsfreq, tmdsfreq_frac;
+};
+
+static inline const char *cs_data_smpl_freq_str(u8 cs_data_3)
+{
+	switch (cs_data_3 & 0xf) {
+	case 0:
+		return "44.1";
+	case 2:
+		return "48";
+	case 3:
+		return "32";
+	case 8:
+		return "88.2";
+	case 10:
+		return "96";
+	case 12:
+		return "176";
+	case 14:
+		return "192";
+	}
+	return "reserved";
+}
+
+static inline const char *cs_data_clk_lvl_str(u8 cs_data_3)
+{
+	switch (cs_data_3 & 0x30) {
+	case 0:
+		return "Level II";
+	case 1:
+		return "Level I";
+	case 2:
+		return "Level III, variable pitch shifted";
+	}
+	return "reserved";
+}
+
+static inline const char *i2s_out_mode_str(u8 i2s_mode)
+{
+	switch (i2s_mode & ADV748X_HDMI_I2SOUTMODE_MASK) {
+	case 0 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "I2S";
+	case 1 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "right";
+	case 2 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "left";
+	case 3 << ADV748X_HDMI_I2SOUTMODE_SHIFT:
+		return "spdif";
+	}
+	return "";
+}
+
+static int adv748x_hdmi_log_status(struct v4l2_subdev *sd)
+{
+	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+	u8 rv, i2s_tdm_mode_enable;
+	u8 cts_n[5];
+	u8 cs_data[0x3a - 0x36 + 1];
+	u8 tmdsfreq[2]; /* both tmdsfreq and tmdsfreq_frac */
+	struct tmds_params tmds_params;
+
+	/* Audio control and configuration */
+	rv = io_read(state, 0x71);
+	pr_info("cable_det_a_raw         %s\n",
+		rv & BIT(6) ? "detected" : "no cable");
+	pr_info("tmds_clk_a_raw          %s\n",
+		rv & BIT(3) ? "detected" : "no TMDS clock");
+	pr_info("tmdspll_lck_a_raw       %s\n",
+		rv & BIT(7) ? "locked to incoming clock" : "not locked");
+	pr_info("hdmi_encrpt_a_raw       %s\n",
+		rv & BIT(5) ? "current frame encrypted" : "not encrypted");
+	rv = hdmi_read(state, 0x04);
+	pr_info("audio_pll_locked        0x%02lx\n", rv & BIT(0));
+	pr_info("tmds_pll_locked         0x%02lx\n", rv & BIT(1));
+	rv = io_read(state, 0x6c);
+	pr_info("gamut_mdata_raw         %s\n",
+		rv & BIT(0) ? "received" : "-");
+	pr_info("audio_c_pckt_raw        %s\n",
+		rv & BIT(1) ? "ACR received" : "-");
+	pr_info("gen_ctl_pckt_raw        %s\n",
+		rv & BIT(2) ? "received" : "-");
+	pr_info("hdmi_mode_raw           %s\n",
+		rv & BIT(3) ? "HDMI/MHL" : "-");
+	pr_info("audio_ch_md_raw         %s\n",
+		rv & BIT(4) ? "multichannel" : "-");
+	pr_info("av_mute_raw             %s\n",
+		rv & BIT(5) ? "received" : "-");
+	pr_info("internal_mute_raw       %s\n",
+		rv & BIT(6) ? "asserted" : "-");
+	pr_info("cs_data_valid_raw       %s\n",
+		rv & BIT(7) ? "valid" : "-");
+	rv = hdmi_read(state, 0x6d);
+	pr_info("i2s_tdm_mode_enable     %s\n",
+		rv & BIT(7) ? "TDM (multichannel)" : "I2S (stereo)");
+	i2s_tdm_mode_enable = rv & BIT(7);
+
+	/* i2s_tdm_mode_enable must be unset */
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x36,
+			       cs_data, ARRAY_SIZE(cs_data)) == 0) {
+		pr_info("... cs_data %s\n",
+			cs_data[0] & BIT(0) ? "pro" : "consumer");
+		pr_info("... cs_data %s\n",
+			cs_data[0] & BIT(1) ? "other" : "L-PCM");
+		pr_info("... cs_data %s copyright\n",
+			cs_data[0] & BIT(2) ? "no" : "asserted");
+		pr_info("... cs_data %s (%lu)\n",
+			cs_data[0] & GENMASK(5, 3) ?
+			"50/15" : "no pre-emphasis",
+			(cs_data[0] & GENMASK(5, 3)) >> 4);
+		pr_info("... cs_data channels status mode %lu\n",
+			(cs_data[0] & GENMASK(7, 6)) >> 7);
+		pr_info("... cs_data category code 0x%02x\n", cs_data[1]);
+		pr_info("... cs_data source number %u\n", cs_data[2] & 0xf);
+		pr_info("... cs_data channel number %u\n",
+			(cs_data[2] & 0xf0) >> 4);
+		pr_info("... cs_data sampling frequency %s (%u)\n",
+			cs_data_smpl_freq_str(cs_data[3]), cs_data[3] & 0xf);
+		pr_info("... cs_data clock accuracy %s\n",
+			cs_data_clk_lvl_str(cs_data[3]));
+	}
+	rv = hdmi_read(state, ADV748X_HDMI_I2S);
+	pr_info("i2soutmode              %s\n", i2s_out_mode_str(rv));
+	pr_info("i2sbitwidth             %u\n", rv & 0x1fu);
+	rv = hdmi_read(state, 0x05);
+	pr_info("hdmi_mode               %s\n", rv & BIT(7) ? "HDMI" : "DVI");
+	rv = hdmi_read(state, 0x07);
+	pr_info("audio_channel_mode      %s\n",
+		rv & BIT(6) ? "multichannel" : "stereo or compressed");
+	rv = hdmi_read(state, 0x0f);
+	/* The bits 6 and 7 must be 1 if TDM mode */
+	pr_info("man_audio_dl_bypass     0x%02lx\n", rv & BIT(7));
+	pr_info("audio_delay_line_bypass 0x%02lx\n", rv & BIT(6));
+	rv = hdmi_read(state, 0x6e);
+	pr_info("mux_spdif_to_i2s_enable %s\n", rv & BIT(3) ? "SPDIF" : "I2S");
+	rv = dpll_read(state, ADV748X_DPLL_MCLK_FS);
+	pr_info("mclk_fs_n               %lu\n",
+		((rv & ADV748X_DPLL_MCLK_FS_N_MASK) + 1) * 128);
+
+	/* i2s_tdm_mode_enable must be set */
+	memset(&tmds_params, 0, sizeof(tmds_params));
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x5b, cts_n, 5) == 0) {
+		tmds_params.cts  = cts_n[0] << 12;
+		tmds_params.cts |= cts_n[1] << 4;
+		tmds_params.cts |= cts_n[2] >> 4;
+		tmds_params.n  = (cts_n[2] & 0xf) << 16;
+		tmds_params.n |= cts_n[3] << 8;
+		tmds_params.n |= cts_n[4];
+		pr_info("... TDM: ACR cts  %u\n", tmds_params.cts);
+		pr_info("... TDM: ACR n    %u\n", tmds_params.n);
+	}
+	if (adv748x_read_block(state, ADV748X_PAGE_HDMI, 0x51,
+			       tmdsfreq, 2) == 0) {
+		tmds_params.tmdsfreq  = tmdsfreq[0] << 1;
+		tmds_params.tmdsfreq |= tmdsfreq[1] >> 7;
+		tmds_params.tmdsfreq_frac = tmdsfreq[1] & 0x7f;
+		pr_info("... TDM: tmdsfreq       %d MHz\n",
+			tmds_params.tmdsfreq);
+		pr_info("... TDM: tmdsfreq_frac  %d 1/128\n",
+			tmds_params.tmdsfreq_frac);
+	}
+	if (i2s_tdm_mode_enable)
+		pr_info("... TDM: sampling frequency %u Hz\n",
+			tmds_params.cts ?
+			(tmds_params.tmdsfreq * tmds_params.n +
+			 tmds_params.tmdsfreq_frac * tmds_params.n / 128) *
+			1000 / (128 * tmds_params.cts / 1000) :
+			UINT_MAX);
+	return 0;
+}
+
+static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
+	.log_status = adv748x_hdmi_log_status,
+};
+
 /* -----------------------------------------------------------------------------
  * v4l2_subdev_ops
  */
 
 static const struct v4l2_subdev_ops adv748x_ops_hdmi = {
+	.core = &adv748x_core_ops_hdmi,
 	.video = &adv748x_video_ops_hdmi,
 	.pad = &adv748x_pad_ops_hdmi,
 };
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index af70632926b5..3ce7ae7f2ec5 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -432,6 +432,8 @@ struct adv748x_state {
 /* Register handling */
 
 int adv748x_read(struct adv748x_state *state, u8 addr, u8 reg);
+int adv748x_read_block(struct adv748x_state *state, u8 page, u8 reg,
+		       void *val, size_t reg_count);
 int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value);
 int adv748x_write_block(struct adv748x_state *state, int client_page,
 			unsigned int init_reg, const void *val,
-- 
2.25.1.25.g9ecbe7eb18


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 10/10] media: adv748x: allow the HDMI sub-device to accept EDID

[Alex Riesen]

This makes it possible to load a EDID reported by the device
with v4l2-ctl utility:

  vdev=/dev/$(grep -l '^adv748x.*hdmi$' /sys/class/video4linux/v4l-subdev*/name |cut -d/ -f5-5)
  v4l2-ctl -d $vdev --set-edid=pad=0,file=/etc/adv7482.edid

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

diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index 7655d817ceb6..88e309de3d56 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -775,7 +775,34 @@ static int adv748x_hdmi_log_status(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static long adv748x_hdmi_querycap(struct adv748x_hdmi *hdmi,
+				  struct v4l2_capability *cap)
+{
+	struct adv748x_state *state = adv748x_hdmi_to_state(hdmi);
+
+	cap->version = LINUX_VERSION_CODE;
+	strlcpy(cap->driver, state->dev->driver->name, sizeof(cap->driver));
+	strlcpy(cap->card, "hdmi", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "i2c:%d-%04x",
+		 i2c_adapter_id(state->client->adapter),
+		 state->client->addr);
+	cap->device_caps = V4L2_CAP_AUDIO | V4L2_CAP_VIDEO_CAPTURE;
+	cap->capabilities = V4L2_CAP_DEVICE_CAPS;
+	return 0;
+}
+
+static long adv748x_hdmi_ioctl(struct v4l2_subdev *sd,
+			       unsigned int cmd, void *arg)
+{
+	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
+
+	if (cmd == VIDIOC_QUERYCAP)
+		return adv748x_hdmi_querycap(hdmi, arg);
+	return -ENOTTY;
+}
+
 static const struct v4l2_subdev_core_ops adv748x_core_ops_hdmi = {
+	.ioctl = adv748x_hdmi_ioctl,
 	.log_status = adv748x_hdmi_log_status,
 };
 
-- 
2.25.1.25.g9ecbe7eb18

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 02/10] media: adv748x: include everything adv748x.h needs into the file

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Thu, Mar 19, 2020 at 06:41:48PM +0100, Alex Riesen wrote:
> To follow the established practice of not depending on others to
> pull everything in.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>

Good idea. While at it, could you include "adv748x.h" as the very first
header in at least one of the C files ? That will help ensuring the
header stays self-contained in the future.

> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  | 2 --
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 --
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 --
>  drivers/media/i2c/adv748x/adv748x-hdmi.c | 2 --
>  drivers/media/i2c/adv748x/adv748x.h      | 2 ++
>  5 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index dbbb1e4d6363..ab0479641c10 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -11,8 +11,6 @@
>  #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>
>  
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index c3fb113cef62..345f009de121 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -20,8 +20,6 @@
>  #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>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index c43ce5d78723..78d391009b5a 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -8,8 +8,6 @@
>  #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"
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> index c557f8fdf11a..0dffcdf79ff2 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -8,8 +8,6 @@
>  #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>
>  
> 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,

Laurent Pinchart
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Laurent Pinchart]

Hi Alex,

Thank you for the patch.

On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> As the driver has some support for the audio interface of the device,
> the bindings file should mention it.

While at it, how about converting the bindings to YAML ? :-) It can of
course be done on top.

> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  .../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..7d6db052c294 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.

s/I2S compatible/I2S-compatible/ ?

> +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> if the I2S port is used

Wouldn't it be simpler to set it to 0 unconditionally ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  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,

Laurent Pinchart
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 01/10] media: adv748x: fix end-of-line terminators in diagnostic statements

[Laurent Pinchart]

Hi Alex,

Thank you for the patch.

On Thu, Mar 19, 2020 at 06:41:43PM +0100, Alex Riesen wrote:
> 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);
>  
>  	/*

-- 
Regards,

Laurent Pinchart
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/10] media: adv748x: reduce amount of code for bitwise modifications of device registers

[Laurent Pinchart]

Hi Alex,

Thank you for the patch.

On Thu, Mar 19, 2020 at 06:41:53PM +0100, Alex Riesen wrote:
> The regmap provides a convenient utility for this.
> 
> Signed-off-by: Alexander Riesen <alexander.riesen@cetitec.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c |  6 ++++++
>  drivers/media/i2c/adv748x/adv748x.h      | 15 ++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 345f009de121..36164a254d7b 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..c5245464fffc 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -393,25 +393,34 @@ 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 io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)

Those two are identical. Do we need both ? I would standardize on either
*_update or *_clrset for all the functions here. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  #define hdmi_read(s, r) adv748x_read(s, ADV748X_PAGE_HDMI, r)
>  #define hdmi_read16(s, r, m) (((hdmi_read(s, r) << 8) | hdmi_read(s, (r)+1)) & (m))
>  #define hdmi_write(s, r, v) adv748x_write(s, ADV748X_PAGE_HDMI, r, v)
> +#define hdmi_update(s, r, m, v) \
> +	adv748x_update_bits(s, ADV748X_PAGE_HDMI, r, m, v)
> +
> +#define dpll_read(s, r) adv748x_read(s, ADV748X_PAGE_DPLL, r)
> +#define dpll_update(s, r, m, v) \
> +	adv748x_update_bits(s, ADV748X_PAGE_DPLL, r, m, v)
>  
>  #define repeater_read(s, r) adv748x_read(s, ADV748X_PAGE_REPEATER, r)
>  #define repeater_write(s, r, v) adv748x_write(s, ADV748X_PAGE_REPEATER, r, v)
>  
>  #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,

Laurent Pinchart
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 02/10] media: adv748x: include everything adv748x.h needs into the file

[Alex Riesen]

Hi Laurent,

Laurent Pinchart, Thu, Mar 19, 2020 18:48:02 +0100:
> On Thu, Mar 19, 2020 at 06:41:48PM +0100, Alex Riesen wrote:
> > To follow the established practice of not depending on others to
> > pull everything in.
> 
> Good idea. While at it, could you include "adv748x.h" as the very first
> header in at least one of the C files ? That will help ensuring the
> header stays self-contained in the future.

Done.

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Geert Uytterhoeven]

Hi Alex,

CC linux-clk for the clock provider.

On Thu, Mar 19, 2020 at 6:42 PM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> 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>

Thanks for your patch!

One comment below.

> ---
>  drivers/media/i2c/adv748x/Makefile       |   3 +-
>  drivers/media/i2c/adv748x/adv748x-core.c |   9 +-
>  drivers/media/i2c/adv748x/adv748x-dai.c  | 256 +++++++++++++++++++++++
>  drivers/media/i2c/adv748x/adv748x.h      |  16 +-
>  4 files changed, 281 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 36164a254d7b..2c0bd58038e6 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..4775a0c7ed7f
> --- /dev/null
> +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Analog Devices ADV748X HDMI receiver with AFE
> + * The implementation of DAI.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <sound/pcm_params.h>
> +
> +#include "adv748x.h"
> +
> +#define state_of(soc_dai) \
> +       adv748x_dai_to_state(container_of((soc_dai)->driver, \
> +                                         struct adv748x_dai, \
> +                                         drv))
> +
> +static const char ADV748X_DAI_NAME[] = "adv748x-i2s";
> +
> +static int set_audio_pads_state(struct adv748x_state *state, int on)
> +{
> +       return io_update(state, ADV748X_IO_PAD_CONTROLS,
> +                        ADV748X_IO_PAD_CONTROLS_TRI_AUD |
> +                        ADV748X_IO_PAD_CONTROLS_PDN_AUD,
> +                        on ? 0 : 0xff);
> +}
> +
> +static int set_dpll_mclk_fs(struct adv748x_state *state, int fs)
> +{
> +       return dpll_update(state, ADV748X_DPLL_MCLK_FS,
> +                          ADV748X_DPLL_MCLK_FS_N_MASK, (fs / 128) - 1);
> +}
> +
> +static int set_i2s_format(struct adv748x_state *state, uint outmode,
> +                         uint bitwidth)
> +{
> +       return hdmi_update(state, ADV748X_HDMI_I2S,
> +                          ADV748X_HDMI_I2SBITWIDTH_MASK |
> +                          ADV748X_HDMI_I2SOUTMODE_MASK,
> +                          (outmode << ADV748X_HDMI_I2SOUTMODE_SHIFT) |
> +                          bitwidth);
> +}
> +
> +static int set_i2s_tdm_mode(struct adv748x_state *state, int is_tdm)
> +{
> +       int ret;
> +
> +       ret = hdmi_update(state, ADV748X_HDMI_AUDIO_MUTE_SPEED,
> +                         ADV748X_MAN_AUDIO_DL_BYPASS |
> +                         ADV748X_AUDIO_DELAY_LINE_BYPASS,
> +                         is_tdm ? 0xff : 0);
> +       if (ret < 0)
> +               return ret;
> +       ret = hdmi_update(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_update(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(state->dai.mclk)) {
> +               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 = clk_register_fixed_rate(state->dev,
> +                                           "adv748x-hdmi-i2s-mclk",

I assume there can be multiple adv748x instances in the system?
Hence the clock name should be unique for each instance.

> +                                           NULL /* parent_name */,
> +                                           0 /* flags */,
> +                                           12288000 /* rate */);
> +       if (IS_ERR_OR_NULL(dai->mclk)) {
> +               ret = PTR_ERR(dai->mclk);
> +               adv_err(state, "Failed to register MCLK (%d)\n", ret);
> +               goto fail;
> +       }
> +       ret = of_clk_add_provider(state->dev->of_node, of_clk_src_simple_get,
> +                                 dai->mclk);
> +       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 = (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_unregister_fixed_rate(dai->mclk);
> +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_unregister_fixed_rate(dai->mclk);
> +}
> +
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 7bc1bb0b3756..af70632926b5 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,12 @@ 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 *mclk;
> +       unsigned int freq, fmt, tdm;
> +};
> +
>  /**
>   * struct adv748x_state - State of ADV748X
>   * @dev:               (OF) device
> @@ -182,6 +190,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 +206,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)
> @@ -485,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

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alex Riesen]

Hi Laurent,

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > As the driver has some support for the audio interface of the device,
> > the bindings file should mention it.
> 
> While at it, how about converting the bindings to YAML ? :-) It can of
> course be done on top.

Of course. I shall take a look at that.

> >  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.
> 
> s/I2S compatible/I2S-compatible/ ?

Done.

> > +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> if the I2S port is used
> 
> Wouldn't it be simpler to set it to 0 unconditionally ?

Would it? If the port itself is optional, shouldn't the clock be an option
too?

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Regards,
Alex

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Geert Uytterhoeven]

Hi Alex,

On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > As the driver has some support for the audio interface of the device,
> > > the bindings file should mention it.

> > > @@ -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> if the I2S port is used
> >
> > Wouldn't it be simpler to set it to 0 unconditionally ?
>
> Would it? If the port itself is optional, shouldn't the clock be an option
> too?

You'd be surprised how many board designers would consider this a cheap
12.288 MHz clock source, without using the I2S port ;-)

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:
> CC linux-clk for the clock provider.

Thanks!

> > +int adv748x_dai_init(struct adv748x_dai *dai)
> > +{
> > +       int ret;
> > +       struct adv748x_state *state = adv748x_dai_to_state(dai);
> > +
> > +       dai->mclk = clk_register_fixed_rate(state->dev,
> > +                                           "adv748x-hdmi-i2s-mclk",
> 
> I assume there can be multiple adv748x instances in the system?
> Hence the clock name should be unique for each instance.

I think that can happen.

Is it alright to derive the clock name from the device name? E.g.:
adv748x.4-0070-mclk? Where "adv748x.4-0070" is a struct device->name.

Regards,
Alex

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > As the driver has some support for the audio interface of the device,
> > > > the bindings file should mention it.
> 
> > > > @@ -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> if the I2S port is used
> > >
> > > Wouldn't it be simpler to set it to 0 unconditionally ?
> >
> > Would it? If the port itself is optional, shouldn't the clock be an option
> > too?
> 
> You'd be surprised how many board designers would consider this a cheap
> 12.288 MHz clock source, without using the I2S port ;-)

Well, I am :-)

Especially considering that the driver will not switch the MCLK pin aktive
(all I2S-related pins are tristate by default).

And how do I require it to be set unconditionally? By just removing the
"if ..." part of the statement?

Regards,
Alex
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 03/10] media: adv748x: reduce amount of code for bitwise modifications of device registers

[Alex Riesen]

Laurent Pinchart, Thu, Mar 19, 2020 19:06:14 +0100:
> On Thu, Mar 19, 2020 at 06:41:53PM +0100, Alex Riesen wrote:
> >  #define io_read(s, r) adv748x_read(s, ADV748X_PAGE_IO, r)
> >  #define io_write(s, r, v) adv748x_write(s, ADV748X_PAGE_IO, r, v)
> > -#define io_clrset(s, r, m, v) io_write(s, r, (io_read(s, r) & ~(m)) | (v))
> > +#define io_clrset(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
> > +#define io_update(s, r, m, v) adv748x_update_bits(s, ADV748X_PAGE_IO, r, m, v)
> 
> Those two are identical. Do we need both ? I would standardize on either
> *_update or *_clrset for all the functions here. Apart from that,

Shame on me. *_clrset that is (it was there before me).

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Regards,
Alex
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Geert Uytterhoeven]

Hi Alex,

On Fri, Mar 20, 2020 at 9:58 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:
> > > +int adv748x_dai_init(struct adv748x_dai *dai)
> > > +{
> > > +       int ret;
> > > +       struct adv748x_state *state = adv748x_dai_to_state(dai);
> > > +
> > > +       dai->mclk = clk_register_fixed_rate(state->dev,
> > > +                                           "adv748x-hdmi-i2s-mclk",
> >
> > I assume there can be multiple adv748x instances in the system?
> > Hence the clock name should be unique for each instance.
>
> I think that can happen.
>
> Is it alright to derive the clock name from the device name? E.g.:
> adv748x.4-0070-mclk? Where "adv748x.4-0070" is a struct device->name.

Yes, that's the idea.

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Geert Uytterhoeven]

Hi Alex,

On Fri, Mar 20, 2020 at 10:03 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > As the driver has some support for the audio interface of the device,
> > > > > the bindings file should mention it.
> >
> > > > > @@ -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> if the I2S port is used
> > > >
> > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > >
> > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > too?
> >
> > You'd be surprised how many board designers would consider this a cheap
> > 12.288 MHz clock source, without using the I2S port ;-)
>
> Well, I am :-)
>
> Especially considering that the driver will not switch the MCLK pin aktive
> (all I2S-related pins are tristate by default).

OK, didn't consider that.  But that still won't stop the hardware designer.
E.g. on Lager, the clock input of the PMIC is tied to the clock line of an SPI
bus, so to use that feature, the SPI clock must be kept running all the time,
not just when doing a transfer.

> And how do I require it to be set unconditionally? By just removing the
> "if ..." part of the statement?

Indeed.  This is still the plain text binding, not yaml.

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alex Riesen]

Geert Uytterhoeven, Fri, Mar 20, 2020 10:15:17 +0100:
> On Fri, Mar 20, 2020 at 10:03 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > > As the driver has some support for the audio interface of the device,
> > > > > > the bindings file should mention it.
> > >
> > > > > > @@ -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> if the I2S port is used
> > > > >
> > > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > > >
> > > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > > too?
> > >
> > > You'd be surprised how many board designers would consider this a cheap
> > > 12.288 MHz clock source, without using the I2S port ;-)
> >
> > Well, I am :-)
> >
> > Especially considering that the driver will not switch the MCLK pin aktive
> > (all I2S-related pins are tristate by default).
> 
> OK, didn't consider that.  But that still won't stop the hardware designer.
> E.g. on Lager, the clock input of the PMIC is tied to the clock line of an SPI
> bus, so to use that feature, the SPI clock must be kept running all the time,
> not just when doing a transfer.

Well... Maybe there is a convention to spell out the default state of the
clock lines?

> > And how do I require it to be set unconditionally? By just removing the
> > "if ..." part of the statement?
> 
> Indeed.  This is still the plain text binding, not yaml.

Conversion to YAML is on the list :)

Regards,
Alex

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Laurent Pinchart]

Hi Alex,

On Fri, Mar 20, 2020 at 10:03:39AM +0100, Alex Riesen wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > On Fri, Mar 20, 2020 at 9:44 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:
> > > > On Thu, Mar 19, 2020 at 06:42:36PM +0100, Alex Riesen wrote:
> > > > > As the driver has some support for the audio interface of the device,
> > > > > the bindings file should mention it.
> > > > >
> > > > > @@ -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> if the I2S port is used
> > > >
> > > > Wouldn't it be simpler to set it to 0 unconditionally ?
> > >
> > > Would it? If the port itself is optional, shouldn't the clock be an option
> > > too?
> > 
> > You'd be surprised how many board designers would consider this a cheap
> > 12.288 MHz clock source, without using the I2S port ;-)
> 
> Well, I am :-)
> 
> Especially considering that the driver will not switch the MCLK pin aktive
> (all I2S-related pins are tristate by default).

If the MCLK can't be output without enabling the I2S then I don't mind
if we make the #clock-cells optional, although, as Geert mentioned,
someone may still want to use it.

> And how do I require it to be set unconditionally? By just removing the
> "if ..." part of the statement?

Yes. For YAML it's easy too, the hard part is making properties
conditional :-)

-- 
Regards,

Laurent Pinchart
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:
> CC linux-clk for the clock provider.
> 
> On Thu, Mar 19, 2020 at 6:42 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > This adds an implemention of SoC DAI driver which provides access to the
> > I2S port of the device.

I just noticed I don't do clk_prepare_enable anywhere.
Shouldn't the clock master enable its clocks somewhere?

> > diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
> > new file mode 100644
> > index 000000000000..4775a0c7ed7f
> > --- /dev/null
> > +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
...
> > +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);
> > +}

For example, here, after activation of the lines succeeded?

> > +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);
> > +}

And clk_disable_unprepare here, before shutting down the pads?

Regards,
Alex

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Geert Uytterhoeven]

Hi Alex,

On Fri, Mar 20, 2020 at 11:58 AM Alex Riesen
<alexander.riesen@cetitec.com> wrote:
> Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:
> > On Thu, Mar 19, 2020 at 6:42 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > This adds an implemention of SoC DAI driver which provides access to the
> > > I2S port of the device.
>
> I just noticed I don't do clk_prepare_enable anywhere.
> Shouldn't the clock master enable its clocks somewhere?

Usually the consumer is responsible for doing that.
Does the rcar-sound driver do that?

But in this case, perhaps the clock should be enabled implicitly in response
to a request from the audio subsystem, like you do below.

Note that you register a fixed-rate clock, which is assumed to be always
enabled. Perhaps a gateable clock type is more appropriate?

> > > diff --git a/drivers/media/i2c/adv748x/adv748x-dai.c b/drivers/media/i2c/adv748x/adv748x-dai.c
> > > new file mode 100644
> > > index 000000000000..4775a0c7ed7f
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/adv748x/adv748x-dai.c
> ...
> > > +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);
> > > +}
>
> For example, here, after activation of the lines succeeded?
>
> > > +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);
> > > +}
>
> And clk_disable_unprepare here, before shutting down the pads?
Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 05/10] media: adv748x: add support for HDMI audio

[Alex Riesen]

Hi Geert,

Geert Uytterhoeven, Fri, Mar 20, 2020 12:05:20 +0100:
> On Fri, Mar 20, 2020 at 11:58 AM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:
> > > On Thu, Mar 19, 2020 at 6:42 PM Alex Riesen <alexander.riesen@cetitec.com> wrote:
> > > > This adds an implemention of SoC DAI driver which provides access to the
> > > > I2S port of the device.
> >
> > I just noticed I don't do clk_prepare_enable anywhere.
> > Shouldn't the clock master enable its clocks somewhere?
> 
> Usually the consumer is responsible for doing that.
> Does the rcar-sound driver do that?

No, it does not (verified by /sys/kernel/debug/clk/clk_summary during
transfer).

> But in this case, perhaps the clock should be enabled implicitly in response
> to a request from the audio subsystem, like you do below.

Ok...

> Note that you register a fixed-rate clock, which is assumed to be always
> enabled. Perhaps a gateable clock type is more appropriate?

The gated clock implementation requires use of an I/O register, which I don't
have in this case (an I2C connected device).

I considered implementing full clk_hw set of operations, but decided against
it: it's a lot for this simple configuration. Few other drivers do that, too.

Regards,
Alex


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alex Riesen]

Hi Laurent,

Laurent Pinchart, Fri, Mar 20, 2020 10:59:07 +0100:
> On Fri, Mar 20, 2020 at 10:03:39AM +0100, Alex Riesen wrote:
> > Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:
> > > 
> > > You'd be surprised how many board designers would consider this a cheap
> > > 12.288 MHz clock source, without using the I2S port ;-)
> > 
> > Well, I am :-)
> > 
> > Especially considering that the driver will not switch the MCLK pin aktive
> > (all I2S-related pins are tristate by default).
> 
> If the MCLK can't be output without enabling the I2S then I don't mind
> if we make the #clock-cells optional, although, as Geert mentioned,
> someone may still want to use it.

So I settled on just removing the option.

> > And how do I require it to be set unconditionally? By just removing the
> > "if ..." part of the statement?
> 
> Yes. For YAML it's easy too, the hard part is making properties
> conditional :-)

Converting it into YAML turned out a bit more than just reformatting:
some of the explicit bindings schema is only implied in the text format :-(

Takes a while to find out what is what.

Regards,
Alex
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Kuninori Morimoto]

Hi Alex

Thank you for your pa

> 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>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
(snip)
> @@ -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";

I think you don't need to overwrite clock-names here in this case ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Alex Riesen]

Hi Morimoto-san,

Kuninori Morimoto, Mon, Mar 23, 2020 01:12:00 +0100:
> > 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>
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> (snip)
> > @@ -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";
> 
> I think you don't need to overwrite clock-names here in this case ?

I vaguely remember something using the names and failing to enable clk_c
without the list spelled out...

I shall re-test though, perhaps it was my own code (since removed) using it.

Regards,
Alex

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel