media: adv748x: add support for HDMI audio

Signed-off-by Alexander Riesen <[email protected]>

---
 drivers/media/i2c/adv748x/adv748x-core.c | 24 ++++++++++++------------
 drivers/media/i2c/adv748x/adv748x-csi2.c |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

drivers/media/i2c/adv748x/adv748x-core.c +12 -12

index 23e02ff27b17..c3fb113cef62 100644
@@ -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;
 	}
 

drivers/media/i2c/adv748x/adv748x-csi2.c +1 -1

index 2091cda50935..c43ce5d78723 100644
@@ -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);
 
 	/*

To follow the established practice of not depending on others to pull everything in.

Signed-off-by Alexander Riesen <[email protected]>

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

drivers/media/i2c/adv748x/adv748x-afe.c +0 -2

index dbbb1e4d6363..ab0479641c10 100644
@@ -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>
 

drivers/media/i2c/adv748x/adv748x-core.c +0 -2

index c3fb113cef62..345f009de121 100644
@@ -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>

drivers/media/i2c/adv748x/adv748x-csi2.c +0 -2

index c43ce5d78723..78d391009b5a 100644
@@ -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"

drivers/media/i2c/adv748x/adv748x-hdmi.c +0 -2

index c557f8fdf11a..0dffcdf79ff2 100644
@@ -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>
 

drivers/media/i2c/adv748x/adv748x.h +2 -0

index fccb388ce179..09aab4138c3f 100644
@@ -19,6 +19,8 @@
  */
 
 #include <linux/i2c.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
 
 #ifndef _ADV748X_H_
 #define _ADV748X_H_

Quoted 2 lines

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.

The regmap provides a convenient utility for this.

Signed-off-by Alexander Riesen <[email protected]>

---
 drivers/media/i2c/adv748x/adv748x-core.c |  6 ++++++
 drivers/media/i2c/adv748x/adv748x.h      | 15 ++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

drivers/media/i2c/adv748x/adv748x-core.c +6 -0

index 345f009de121..36164a254d7b 100644
@@ -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.
  *

drivers/media/i2c/adv748x/adv748x.h +0 -0

index 09aab4138c3f..c5245464fffc 100644
@@ -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,

drivers/media/i2c/adv748x/adv748x.h +8 -2

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

drivers/media/i2c/adv748x/adv748x.h int adv748x_write(struct adv748x_state *state, u8 page, u8 reg, u8 value); · line 402

Quoted 5 lines

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

drivers/media/i2c/adv748x/adv748x.h +2 -1

-#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 <[email protected]>

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

Signed-off-by Alexander Riesen <[email protected]>

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

drivers/media/i2c/adv748x/Makefile +2 -1

index 93844f14cb10..6e7a302ef199 100644
@@ -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

drivers/media/i2c/adv748x/adv748x-core.c +8 -1

index 36164a254d7b..2c0bd58038e6 100644
@@ -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);
 

drivers/media/i2c/adv748x/adv748x-dai.c new file +198 -0

new file mode 100644
index 000000000000..4775a0c7ed7f
@@ -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 = {

drivers/media/i2c/adv748x/adv748x-dai.c new file +48 -0

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

drivers/media/i2c/adv748x/adv748x.h +15 -2

index 7bc1bb0b3756..af70632926b5 100644
@@ -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

CC linux-clk for the clock provider.

drivers/media/i2c/adv748x/adv748x-dai.c line 208

Quoted 7 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +7 -0

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

Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:

Quoted 10 lines

drivers/media/i2c/adv748x/adv748x-dai.c line 208

Quoted 7 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +7 -0

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

CC linux-clk for the clock provider.

Quoted 2 lines · Alex Riesen

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

Quoted 5 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +0 -0

new file mode 100644
index 000000000000..4775a0c7ed7f

drivers/media/i2c/adv748x/adv748x-dai.c line 125

Quoted 8 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +8 -0

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

drivers/media/i2c/adv748x/adv748x-dai.c line 178

Quoted 6 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +6 -0

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

Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:

Quoted 3 lines · Alex Riesen

Quoted 2 lines

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?

Quoted 5 lines

Quoted 5 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +0 -0

new file mode 100644
index 000000000000..4775a0c7ed7f

...

Quoted 8 lines

drivers/media/i2c/adv748x/adv748x-dai.c line 125

Quoted 8 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +8 -0

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

Quoted 6 lines

drivers/media/i2c/adv748x/adv748x-dai.c line 178

Quoted 6 lines

drivers/media/i2c/adv748x/adv748x-dai.c new file +6 -0

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

Quoted 7 lines

Geert Uytterhoeven, Fri, Mar 20, 2020 09:43:29 +0100:

Quoted 3 lines · Alex Riesen

Quoted 2 lines

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?

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

Reviewed-by Rob Herring <[email protected]>

Signed-off-by Alexander Riesen <[email protected]>

---
 .../devicetree/bindings/media/i2c/adv748x.txt    | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/media/i2c/adv748x.txt +0 -0

index 4f91686e54a6..7d6db052c294 100644

Documentation/devicetree/bindings/media/i2c/adv748x.txt +11 -0

+
 Optional Properties:
 
   - interrupt-names: Should specify the interrupts as "intrq1", "intrq2" and/or

@@ -47,6 +51,7 @@ are numbered as follows.
 	  TTL		sink		9
 	  TXA		source		10
 	  TXB		source		11
+	  I2S		source		12
 
 The digital output port nodes, when present, shall contain at least one
 endpoint. Each of those endpoints shall contain the data-lanes property as

@@ -72,6 +77,7 @@ Example:
 
 		#address-cells = <1>;
 		#size-cells = <0>;
+		#clock-cells = <0>;
 
 		interrupt-parent = <&gpio6>;
 		interrupt-names = "intrq1", "intrq2";

@@ -113,4 +119,12 @@ Example:
 				remote-endpoint = <&csi20_in>;
 			};
 		};
+
+		port@c {
+			reg = <12>;
+
+			adv7482_i2s: endpoint {
+				remote-endpoint = <&i2s_in>;
+			};
+		};
 	};

Quoted 2 lines

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.

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

Quoted 5 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +2 -1

-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/ ?

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 9 lines

Documentation/devicetree/bindings/media/i2c/adv748x.txt +2 -0

+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 <[email protected]>

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:

Quoted 8 lines · Alex Riesen

Quoted 7 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +0 -0

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

Quoted 3 lines

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 1 line

Documentation/devicetree/bindings/media/i2c/adv748x.txt +1 -0

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

Quoted 13 lines

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:

Quoted 11 lines · Alex Riesen

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 8 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +1 -0

@@ -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 ?

Quoted 2 lines

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

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:

Quoted 17 lines · Alex Riesen

Quoted 15 lines

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:

Quoted 11 lines · Alex Riesen

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 8 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +1 -0

@@ -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?

Quoted 2 lines

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?

Quoted 23 lines

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:

Quoted 14 lines · Alex Riesen

Quoted 13 lines

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:

Quoted 11 lines · Alex Riesen

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 8 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +1 -0

@@ -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 ?

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 2 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.

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:

Quoted 17 lines · Alex Riesen

Quoted 15 lines

Laurent Pinchart, Thu, Mar 19, 2020 19:01:25 +0100:

Quoted 11 lines · Alex Riesen

Documentation/devicetree/bindings/media/i2c/adv748x.txt Required Properties: · line 21

Quoted 8 lines

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

Documentation/devicetree/bindings/media/i2c/adv748x.txt +1 -0

@@ -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?

Quoted 2 lines

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?

Quoted 5 lines

Geert Uytterhoeven, Fri, Mar 20, 2020 09:48:14 +0100:

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 2 lines

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

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 <[email protected]>

Reviewed-by Geert Uytterhoeven <[email protected]>

arch/arm64/boot/dts/renesas/salvator-common.dtsi +2 -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",

Quoted 9 lines

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 <[email protected]>

Reviewed-by Geert Uytterhoeven <[email protected]>

(snip)

Quoted 12 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +6 -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",

arch/arm64/boot/dts/renesas/salvator-common.dtsi &rcar_sound { · line 784

Quoted 6 lines

arch/arm64/boot/dts/renesas/salvator-common.dtsi +6 -0

+		      "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 ?