This patch series supports HDMI jack reporting on RK3288, which uses
DRM dw-hdmi driver and hdmi-codec codec driver.
The previous discussion about reporting jack status using hdmi-notifier
and drm_audio_component is at
https://lore.kernel.org/patchwork/patch/1083027/
The new approach is to use a callback mechanism that is
specific to hdmi-codec.
Changes from v2 to v3:
- dw-hdmi-i2s-audio.c: Use fixed ID instead of auto ID.
- rk3288_hdmi_analog.c: Use the fixed name hdmi-audio-codec for codec device.
- rockchip_max98090: Use the fixed name hdmi-audio-codec for codec device.
- rockchip_max98090: Fix the dependency of hdmi-codec in Kconfig.
Cheng-Yi Chiang (5):
ASoC: hdmi-codec: Add an op to set callback function for plug event
drm: bridge: dw-hdmi: Report connector status using callback
drm: dw-hdmi-i2s: Use fixed id for codec device
ASoC: rockchip_max98090: Add dai_link for HDMI
FROMLIST: ASoC: rockchip_max98090: Add HDMI jack support
.../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 +
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 12 +-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 55 ++++++++-
include/sound/hdmi-codec.h | 16 +++
sound/soc/codecs/hdmi-codec.c | 45 +++++++
sound/soc/rockchip/Kconfig | 3 +-
sound/soc/rockchip/rk3288_hdmi_analog.c | 3 +-
sound/soc/rockchip/rockchip_max98090.c | 116 ++++++++++++++----
8 files changed, 226 insertions(+), 27 deletions(-)
--
2.22.0.510.g264f2c817a-goog
Add an op in hdmi_codec_ops so codec driver can register callback
function to handle plug event.
Driver in DRM can use this callback function to report connector status.
Signed-off-by: Cheng-Yi Chiang <[email protected]>
---
include/sound/hdmi-codec.h | 16 +++++++++++++
sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 7fea496f1f34..9a8661680256 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -47,6 +47,9 @@ struct hdmi_codec_params {
int channels;
};
+typedef void (*hdmi_codec_plugged_cb)(struct device *dev,
+ bool plugged);
+
struct hdmi_codec_pdata;
struct hdmi_codec_ops {
/*
@@ -88,6 +91,13 @@ struct hdmi_codec_ops {
*/
int (*get_dai_id)(struct snd_soc_component *comment,
struct device_node *endpoint);
+
+ /*
+ * Hook callback function to handle connector plug event.
+ * Optional
+ */
+ int (*hook_plugged_cb)(struct device *dev, void *data,
+ hdmi_codec_plugged_cb fn);
};
/* HDMI codec initalization data */
@@ -99,6 +109,12 @@ struct hdmi_codec_pdata {
void *data;
};
+struct snd_soc_component;
+struct snd_soc_jack;
+
+int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
+ struct snd_soc_jack *jack);
+
#define HDMI_CODEC_DRV_NAME "hdmi-audio-codec"
#endif /* __HDMI_CODEC_H__ */
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 0bf1c8cad108..32bf7441be5c 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -7,6 +7,7 @@
#include <linux/module.h>
#include <linux/string.h>
#include <sound/core.h>
+#include <sound/jack.h>
#include <sound/pcm.h>
#include <sound/pcm_params.h>
#include <sound/soc.h>
@@ -274,6 +275,8 @@ struct hdmi_codec_priv {
struct snd_pcm_chmap *chmap_info;
unsigned int chmap_idx;
struct mutex lock;
+ struct snd_soc_jack *jack;
+ unsigned int jack_status;
};
static const struct snd_soc_dapm_widget hdmi_widgets[] = {
@@ -663,6 +666,48 @@ static int hdmi_dai_probe(struct snd_soc_dai *dai)
return 0;
}
+static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp,
+ unsigned int jack_status)
+{
+ if (hcp->jack && jack_status != hcp->jack_status) {
+ snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT);
+ hcp->jack_status = jack_status;
+ }
+}
+
+static void plugged_cb(struct device *dev, bool plugged)
+{
+ struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
+
+ if (plugged)
+ hdmi_codec_jack_report(hcp, SND_JACK_LINEOUT);
+ else
+ hdmi_codec_jack_report(hcp, 0);
+}
+
+/**
+ * hdmi_codec_set_jack_detect - register HDMI plugged callback
+ * @component: the hdmi-codec instance
+ * @jack: ASoC jack to report (dis)connection events on
+ */
+int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
+ struct snd_soc_jack *jack)
+{
+ struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
+ int ret = -EOPNOTSUPP;
+
+ if (hcp->hcd.ops->hook_plugged_cb) {
+ hcp->jack = jack;
+ ret = hcp->hcd.ops->hook_plugged_cb(component->dev->parent,
+ hcp->hcd.data,
+ plugged_cb);
+ if (ret)
+ hcp->jack = NULL;
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect);
+
static int hdmi_dai_spdif_probe(struct snd_soc_dai *dai)
{
struct hdmi_codec_daifmt *cf = dai->playback_dma_data;
--
2.22.0.510.g264f2c817a-goog
Allow codec driver register callback function for plug event.
The callback registration flow:
dw-hdmi <--- hw-hdmi-i2s-audio <--- hdmi-codec
dw-hdmi-i2s-audio implements hook_plugged_cb op
so codec driver can register the callback.
dw-hdmi implements set_plugged_cb op so platform device can register the
callback.
When connector plug/unplug event happens, report this event using the
callback.
Make sure that audio and drm are using the single source of truth for
connector status.
Signed-off-by: Cheng-Yi Chiang <[email protected]>
---
.../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 +
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 55 ++++++++++++++++++-
3 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
index 63b5756f463b..f523c590984e 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
@@ -2,6 +2,8 @@
#ifndef DW_HDMI_AUDIO_H
#define DW_HDMI_AUDIO_H
+#include <sound/hdmi-codec.h>
+
struct dw_hdmi;
struct dw_hdmi_audio_data {
@@ -17,6 +19,7 @@ struct dw_hdmi_i2s_audio_data {
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset);
+ int (*set_plugged_cb)(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn);
};
#endif
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 5cbb71a866d5..7b93cf05c985 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -104,10 +104,20 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
return -EINVAL;
}
+static int dw_hdmi_i2s_hook_plugged_cb(struct device *dev, void *data,
+ hdmi_codec_plugged_cb fn)
+{
+ struct dw_hdmi_i2s_audio_data *audio = data;
+ struct dw_hdmi *hdmi = audio->hdmi;
+
+ return audio->set_plugged_cb(hdmi, fn);
+}
+
static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
.hw_params = dw_hdmi_i2s_hw_params,
.audio_shutdown = dw_hdmi_i2s_audio_shutdown,
.get_dai_id = dw_hdmi_i2s_get_dai_id,
+ .hook_plugged_cb = dw_hdmi_i2s_hook_plugged_cb,
};
static int snd_dw_hdmi_probe(struct platform_device *pdev)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 045b1b13fd0e..ce6646067472 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -26,6 +26,8 @@
#include <drm/drm_probe_helper.h>
#include <drm/bridge/dw_hdmi.h>
+#include <sound/hdmi-codec.h>
+
#include <uapi/linux/media-bus-format.h>
#include <uapi/linux/videodev2.h>
@@ -185,6 +187,9 @@ struct dw_hdmi {
void (*disable_audio)(struct dw_hdmi *hdmi);
struct cec_notifier *cec_notifier;
+
+ hdmi_codec_plugged_cb plugged_cb;
+ enum drm_connector_status last_connector_result;
};
#define HDMI_IH_PHY_STAT0_RX_SENSE \
@@ -209,6 +214,40 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset)
return val;
}
+static void handle_plugged_change(struct dw_hdmi *hdmi, bool plugged)
+{
+ struct platform_device *codec_pdev;
+
+ if (!hdmi->audio || IS_ERR(hdmi->audio))
+ return;
+ codec_pdev = platform_get_drvdata(hdmi->audio);
+ if (!codec_pdev || IS_ERR(codec_pdev))
+ return;
+ if (!hdmi->plugged_cb)
+ return;
+
+ hdmi->plugged_cb(&codec_pdev->dev, plugged);
+}
+
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn)
+{
+ bool plugged;
+ struct platform_device *codec_pdev;
+
+ if (!hdmi->audio || IS_ERR(hdmi->audio))
+ return -EINVAL;
+ codec_pdev = platform_get_drvdata(hdmi->audio);
+ if (!codec_pdev || IS_ERR(codec_pdev))
+ return -EINVAL;
+
+ mutex_lock(&hdmi->mutex);
+ hdmi->plugged_cb = fn;
+ plugged = hdmi->last_connector_result == connector_status_connected;
+ handle_plugged_change(hdmi, plugged);
+ mutex_unlock(&hdmi->mutex);
+ return 0;
+}
+
static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
{
regmap_update_bits(hdmi->regm, reg << hdmi->reg_shift, mask, data);
@@ -2044,6 +2083,7 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
{
struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
connector);
+ enum drm_connector_status result;
mutex_lock(&hdmi->mutex);
hdmi->force = DRM_FORCE_UNSPECIFIED;
@@ -2051,7 +2091,18 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
dw_hdmi_update_phy_mask(hdmi);
mutex_unlock(&hdmi->mutex);
- return hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
+ result = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
+
+ mutex_lock(&hdmi->mutex);
+ if (result != hdmi->last_connector_result) {
+ dev_dbg(hdmi->dev, "read_hpd result: %d", result);
+ handle_plugged_change(hdmi,
+ result == connector_status_connected);
+ hdmi->last_connector_result = result;
+ }
+ mutex_unlock(&hdmi->mutex);
+
+ return result;
}
static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
@@ -2460,6 +2511,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
hdmi->rxsense = true;
hdmi->phy_mask = (u8)~(HDMI_PHY_HPD | HDMI_PHY_RX_SENSE);
hdmi->mc_clkdis = 0x7f;
+ hdmi->last_connector_result = connector_status_disconnected;
mutex_init(&hdmi->mutex);
mutex_init(&hdmi->audio_mutex);
@@ -2653,6 +2705,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.hdmi = hdmi;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
+ audio.set_plugged_cb = hdmi_set_plugged_cb;
hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
hdmi->disable_audio = dw_hdmi_i2s_audio_disable;
--
2.22.0.510.g264f2c817a-goog
Use two dai_links. One for HDMI and one for max98090.
With this setup, audio can play to speaker and HDMI selectively.
Signed-off-by: Cheng-Yi Chiang <[email protected]>
---
sound/soc/rockchip/Kconfig | 3 +-
sound/soc/rockchip/rockchip_max98090.c | 96 ++++++++++++++++++++------
2 files changed, 75 insertions(+), 24 deletions(-)
diff --git a/sound/soc/rockchip/Kconfig b/sound/soc/rockchip/Kconfig
index b43657e6e655..d610b553ea3b 100644
--- a/sound/soc/rockchip/Kconfig
+++ b/sound/soc/rockchip/Kconfig
@@ -40,9 +40,10 @@ config SND_SOC_ROCKCHIP_MAX98090
select SND_SOC_ROCKCHIP_I2S
select SND_SOC_MAX98090
select SND_SOC_TS3A227E
+ select SND_SOC_HDMI_CODEC
help
Say Y or M here if you want to add support for SoC audio on Rockchip
- boards using the MAX98090 codec, such as Veyron.
+ boards using the MAX98090 codec and HDMI codec, such as Veyron.
config SND_SOC_ROCKCHIP_RT5645
tristate "ASoC support for Rockchip boards using a RT5645/RT5650 codec"
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c
index c5fc24675a33..c82948e383da 100644
--- a/sound/soc/rockchip/rockchip_max98090.c
+++ b/sound/soc/rockchip/rockchip_max98090.c
@@ -11,6 +11,7 @@
#include <linux/gpio.h>
#include <linux/of_gpio.h>
#include <sound/core.h>
+#include <sound/hdmi-codec.h>
#include <sound/jack.h>
#include <sound/pcm.h>
#include <sound/pcm_params.h>
@@ -41,6 +42,7 @@ static const struct snd_soc_dapm_widget rk_dapm_widgets[] = {
SND_SOC_DAPM_MIC("Headset Mic", NULL),
SND_SOC_DAPM_MIC("Int Mic", NULL),
SND_SOC_DAPM_SPK("Speaker", NULL),
+ SND_SOC_DAPM_LINE("HDMI", NULL),
};
static const struct snd_soc_dapm_route rk_audio_map[] = {
@@ -52,6 +54,7 @@ static const struct snd_soc_dapm_route rk_audio_map[] = {
{"Headphone", NULL, "HPR"},
{"Speaker", NULL, "SPKL"},
{"Speaker", NULL, "SPKR"},
+ {"HDMI", NULL, "TX"},
};
static const struct snd_kcontrol_new rk_mc_controls[] = {
@@ -59,6 +62,7 @@ static const struct snd_kcontrol_new rk_mc_controls[] = {
SOC_DAPM_PIN_SWITCH("Headset Mic"),
SOC_DAPM_PIN_SWITCH("Int Mic"),
SOC_DAPM_PIN_SWITCH("Speaker"),
+ SOC_DAPM_PIN_SWITCH("HDMI"),
};
static int rk_aif1_hw_params(struct snd_pcm_substream *substream,
@@ -92,38 +96,63 @@ static int rk_aif1_hw_params(struct snd_pcm_substream *substream,
ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk,
SND_SOC_CLOCK_OUT);
- if (ret < 0) {
- dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret);
+ if (ret) {
+ dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret);
return ret;
}
+ /* HDMI codec dai does not need to set sysclk. */
+ if (!strcmp(rtd->dai_link->name, "HDMI"))
+ return 0;
+
ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
SND_SOC_CLOCK_IN);
- if (ret < 0) {
- dev_err(codec_dai->dev, "Can't set codec clock %d\n", ret);
+ if (ret) {
+ dev_err(codec_dai->dev, "Can't set codec dai clock %d\n", ret);
return ret;
}
- return ret;
+ return 0;
}
static const struct snd_soc_ops rk_aif1_ops = {
.hw_params = rk_aif1_hw_params,
};
-SND_SOC_DAILINK_DEFS(hifi,
+SND_SOC_DAILINK_DEFS(analog,
DAILINK_COMP_ARRAY(COMP_EMPTY()),
DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")),
DAILINK_COMP_ARRAY(COMP_EMPTY()));
-static struct snd_soc_dai_link rk_dailink = {
- .name = "max98090",
- .stream_name = "Audio",
- .ops = &rk_aif1_ops,
- /* set max98090 as slave */
- .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
- SND_SOC_DAIFMT_CBS_CFS,
- SND_SOC_DAILINK_REG(hifi),
+SND_SOC_DAILINK_DEFS(hdmi,
+ DAILINK_COMP_ARRAY(COMP_EMPTY()),
+ DAILINK_COMP_ARRAY(COMP_CODEC(HDMI_CODEC_DRV_NAME, "i2s-hifi")),
+ DAILINK_COMP_ARRAY(COMP_EMPTY()));
+
+enum {
+ DAILINK_MAX98090,
+ DAILINK_HDMI,
+};
+
+/* max98090 and HDMI codec dai_link */
+static struct snd_soc_dai_link rk_dailinks[] = {
+ [DAILINK_MAX98090] = {
+ .name = "max98090",
+ .stream_name = "Analog",
+ .ops = &rk_aif1_ops,
+ /* set max98090 as slave */
+ .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+ SND_SOC_DAIFMT_CBS_CFS,
+ SND_SOC_DAILINK_REG(analog),
+ },
+ [DAILINK_HDMI] = {
+ .name = "HDMI",
+ .stream_name = "HDMI",
+ .ops = &rk_aif1_ops,
+ .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+ SND_SOC_DAIFMT_CBS_CFS,
+ SND_SOC_DAILINK_REG(hdmi),
+ }
};
static int rk_98090_headset_init(struct snd_soc_component *component);
@@ -136,8 +165,8 @@ static struct snd_soc_aux_dev rk_98090_headset_dev = {
static struct snd_soc_card snd_soc_card_rk = {
.name = "ROCKCHIP-I2S",
.owner = THIS_MODULE,
- .dai_link = &rk_dailink,
- .num_links = 1,
+ .dai_link = rk_dailinks,
+ .num_links = ARRAY_SIZE(rk_dailinks),
.aux_dev = &rk_98090_headset_dev,
.num_aux_devs = 1,
.dapm_widgets = rk_dapm_widgets,
@@ -173,27 +202,48 @@ static int snd_rk_mc_probe(struct platform_device *pdev)
int ret = 0;
struct snd_soc_card *card = &snd_soc_card_rk;
struct device_node *np = pdev->dev.of_node;
+ struct device_node *np_analog;
+ struct device_node *np_cpu;
+ struct of_phandle_args args;
/* register the soc card */
card->dev = &pdev->dev;
- rk_dailink.codecs->of_node = of_parse_phandle(np,
- "rockchip,audio-codec", 0);
- if (!rk_dailink.codecs->of_node) {
+ np_analog = of_parse_phandle(np, "rockchip,audio-codec", 0);
+ if (!np_analog) {
dev_err(&pdev->dev,
"Property 'rockchip,audio-codec' missing or invalid\n");
return -EINVAL;
}
+ rk_dailinks[DAILINK_MAX98090].codecs->of_node = np_analog;
+
+ ret = of_parse_phandle_with_fixed_args(np, "rockchip,audio-codec",
+ 0, 0, &args);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Unable to parse property 'rockchip,audio-codec'\n");
+ return ret;
+ }
+
+ ret = snd_soc_get_dai_name(
+ &args, &rk_dailinks[DAILINK_MAX98090].codecs->dai_name);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to get codec dai_name\n");
+ return ret;
+ }
+
+ np_cpu = of_parse_phandle(np, "rockchip,i2s-controller", 0);
- rk_dailink.cpus->of_node = of_parse_phandle(np,
- "rockchip,i2s-controller", 0);
- if (!rk_dailink.cpus->of_node) {
+ if (!np_cpu) {
dev_err(&pdev->dev,
"Property 'rockchip,i2s-controller' missing or invalid\n");
return -EINVAL;
}
- rk_dailink.platforms->of_node = rk_dailink.cpus->of_node;
+ rk_dailinks[DAILINK_MAX98090].cpus->of_node = np_cpu;
+ rk_dailinks[DAILINK_MAX98090].platforms->of_node = np_cpu;
+ rk_dailinks[DAILINK_HDMI].cpus->of_node = np_cpu;
+ rk_dailinks[DAILINK_HDMI].platforms->of_node = np_cpu;
rk_98090_headset_dev.codec_of_node = of_parse_phandle(np,
"rockchip,headset-codec", 0);
--
2.22.0.510.g264f2c817a-goog
The problem of using auto ID is that the device name will be like
hdmi-audio-codec.<id number>.auto.
The number might be changed when there are other platform devices being
created before hdmi-audio-codec device.
Use a fixed name so machine driver can set codec name on the DAI link.
Using the fixed name should be fine because there will only be one
hdmi-audio-codec device.
Fix the codec name in rockchip rk3288_hdmi_analog machine driver.
Signed-off-by: Cheng-Yi Chiang <[email protected]>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 2 +-
sound/soc/rockchip/rk3288_hdmi_analog.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 7b93cf05c985..4974a32af31f 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -134,7 +134,7 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
memset(&pdevinfo, 0, sizeof(pdevinfo));
pdevinfo.parent = pdev->dev.parent;
- pdevinfo.id = PLATFORM_DEVID_AUTO;
+ pdevinfo.id = PLATFORM_DEVID_NONE;
pdevinfo.name = HDMI_CODEC_DRV_NAME;
pdevinfo.data = &pdata;
pdevinfo.size_data = sizeof(pdata);
diff --git a/sound/soc/rockchip/rk3288_hdmi_analog.c b/sound/soc/rockchip/rk3288_hdmi_analog.c
index 767700c34ee2..8286025a8747 100644
--- a/sound/soc/rockchip/rk3288_hdmi_analog.c
+++ b/sound/soc/rockchip/rk3288_hdmi_analog.c
@@ -15,6 +15,7 @@
#include <linux/gpio.h>
#include <linux/of_gpio.h>
#include <sound/core.h>
+#include <sound/hdmi-codec.h>
#include <sound/jack.h>
#include <sound/pcm.h>
#include <sound/pcm_params.h>
@@ -142,7 +143,7 @@ static const struct snd_soc_ops rk_ops = {
SND_SOC_DAILINK_DEFS(audio,
DAILINK_COMP_ARRAY(COMP_EMPTY()),
DAILINK_COMP_ARRAY(COMP_CODEC(NULL, NULL),
- COMP_CODEC("hdmi-audio-codec.2.auto", "i2s-hifi")),
+ COMP_CODEC(HDMI_CODEC_DRV_NAME, "i2s-hifi")),
DAILINK_COMP_ARRAY(COMP_EMPTY()));
static struct snd_soc_dai_link rk_dailink = {
--
2.22.0.510.g264f2c817a-goog
In machine driver, create a jack and let hdmi-codec report jack status.
Signed-off-by: Cheng-Yi Chiang <[email protected]>
---
sound/soc/rockchip/rockchip_max98090.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c
index c82948e383da..c81c4acda917 100644
--- a/sound/soc/rockchip/rockchip_max98090.c
+++ b/sound/soc/rockchip/rockchip_max98090.c
@@ -134,6 +134,25 @@ enum {
DAILINK_HDMI,
};
+static struct snd_soc_jack rk_hdmi_jack;
+
+static int rk_hdmi_init(struct snd_soc_pcm_runtime *runtime)
+{
+ struct snd_soc_card *card = runtime->card;
+ struct snd_soc_component *component = runtime->codec_dai->component;
+ int ret;
+
+ /* enable jack detection */
+ ret = snd_soc_card_jack_new(card, "HDMI Jack", SND_JACK_LINEOUT,
+ &rk_hdmi_jack, NULL, 0);
+ if (ret) {
+ dev_err(card->dev, "Can't new HDMI Jack %d\n", ret);
+ return ret;
+ }
+
+ return hdmi_codec_set_jack_detect(component, &rk_hdmi_jack);
+}
+
/* max98090 and HDMI codec dai_link */
static struct snd_soc_dai_link rk_dailinks[] = {
[DAILINK_MAX98090] = {
@@ -151,6 +170,7 @@ static struct snd_soc_dai_link rk_dailinks[] = {
.ops = &rk_aif1_ops,
.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
SND_SOC_DAIFMT_CBS_CFS,
+ .init = rk_hdmi_init,
SND_SOC_DAILINK_REG(hdmi),
}
};
--
2.22.0.510.g264f2c817a-goog
On Fri, Jul 12, 2019 at 6:06 PM Cheng-Yi Chiang <[email protected]> wrote:
>
> In machine driver, create a jack and let hdmi-codec report jack status.
>
> Signed-off-by: Cheng-Yi Chiang <[email protected]>
> ---
> sound/soc/rockchip/rockchip_max98090.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c
> index c82948e383da..c81c4acda917 100644
> --- a/sound/soc/rockchip/rockchip_max98090.c
> +++ b/sound/soc/rockchip/rockchip_max98090.c
> @@ -134,6 +134,25 @@ enum {
> DAILINK_HDMI,
> };
>
> +static struct snd_soc_jack rk_hdmi_jack;
> +
> +static int rk_hdmi_init(struct snd_soc_pcm_runtime *runtime)
> +{
> + struct snd_soc_card *card = runtime->card;
> + struct snd_soc_component *component = runtime->codec_dai->component;
> + int ret;
> +
> + /* enable jack detection */
> + ret = snd_soc_card_jack_new(card, "HDMI Jack", SND_JACK_LINEOUT,
> + &rk_hdmi_jack, NULL, 0);
> + if (ret) {
> + dev_err(card->dev, "Can't new HDMI Jack %d\n", ret);
> + return ret;
> + }
> +
> + return hdmi_codec_set_jack_detect(component, &rk_hdmi_jack);
> +}
> +
> /* max98090 and HDMI codec dai_link */
> static struct snd_soc_dai_link rk_dailinks[] = {
> [DAILINK_MAX98090] = {
> @@ -151,6 +170,7 @@ static struct snd_soc_dai_link rk_dailinks[] = {
> .ops = &rk_aif1_ops,
> .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> SND_SOC_DAIFMT_CBS_CFS,
> + .init = rk_hdmi_init,
> SND_SOC_DAILINK_REG(hdmi),
> }
> };
> --
> 2.22.0.510.g264f2c817a-goog
>
Sorry for the wrong title.
I forgot to remove FROMLIST in the title.
I'll wait for comments on other patches and fix the title in v4.
Thanks!
On Fri, Jul 12, 2019 at 06:04:40PM +0800, Cheng-Yi Chiang wrote:
> Allow codec driver register callback function for plug event.
>
> The callback registration flow:
> dw-hdmi <--- hw-hdmi-i2s-audio <--- hdmi-codec
>
> dw-hdmi-i2s-audio implements hook_plugged_cb op
> so codec driver can register the callback.
>
> dw-hdmi implements set_plugged_cb op so platform device can register the
> callback.
>
> When connector plug/unplug event happens, report this event using the
> callback.
>
> Make sure that audio and drm are using the single source of truth for
> connector status.
>
> Signed-off-by: Cheng-Yi Chiang <[email protected]>
> ---
> .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 +
> .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 55 ++++++++++++++++++-
> 3 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> index 63b5756f463b..f523c590984e 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> @@ -2,6 +2,8 @@
> #ifndef DW_HDMI_AUDIO_H
> #define DW_HDMI_AUDIO_H
>
> +#include <sound/hdmi-codec.h>
> +
> struct dw_hdmi;
>
> struct dw_hdmi_audio_data {
> @@ -17,6 +19,7 @@ struct dw_hdmi_i2s_audio_data {
>
> void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
> u8 (*read)(struct dw_hdmi *hdmi, int offset);
> + int (*set_plugged_cb)(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn);
> };
>
> #endif
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> index 5cbb71a866d5..7b93cf05c985 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> @@ -104,10 +104,20 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
> return -EINVAL;
> }
>
> +static int dw_hdmi_i2s_hook_plugged_cb(struct device *dev, void *data,
> + hdmi_codec_plugged_cb fn)
> +{
> + struct dw_hdmi_i2s_audio_data *audio = data;
> + struct dw_hdmi *hdmi = audio->hdmi;
> +
> + return audio->set_plugged_cb(hdmi, fn);
> +}
> +
> static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
> .hw_params = dw_hdmi_i2s_hw_params,
> .audio_shutdown = dw_hdmi_i2s_audio_shutdown,
> .get_dai_id = dw_hdmi_i2s_get_dai_id,
> + .hook_plugged_cb = dw_hdmi_i2s_hook_plugged_cb,
> };
>
> static int snd_dw_hdmi_probe(struct platform_device *pdev)
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 045b1b13fd0e..ce6646067472 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -26,6 +26,8 @@
> #include <drm/drm_probe_helper.h>
> #include <drm/bridge/dw_hdmi.h>
>
> +#include <sound/hdmi-codec.h>
> +
> #include <uapi/linux/media-bus-format.h>
> #include <uapi/linux/videodev2.h>
>
> @@ -185,6 +187,9 @@ struct dw_hdmi {
> void (*disable_audio)(struct dw_hdmi *hdmi);
>
> struct cec_notifier *cec_notifier;
> +
> + hdmi_codec_plugged_cb plugged_cb;
> + enum drm_connector_status last_connector_result;
> };
>
> #define HDMI_IH_PHY_STAT0_RX_SENSE \
> @@ -209,6 +214,40 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset)
> return val;
> }
>
> +static void handle_plugged_change(struct dw_hdmi *hdmi, bool plugged)
> +{
> + struct platform_device *codec_pdev;
> +
> + if (!hdmi->audio || IS_ERR(hdmi->audio))
> + return;
> + codec_pdev = platform_get_drvdata(hdmi->audio);
> + if (!codec_pdev || IS_ERR(codec_pdev))
> + return;
This looks fragile to me, poking about in another device's driver data
from another driver is really not a good design decision. I think this
can be simplified if the registration function took the function
pointer and the struct device pointer, and then you only need one test
below:
> + if (!hdmi->plugged_cb)
> + return;
> +
> + hdmi->plugged_cb(&codec_pdev->dev, plugged);
> +}
> +
> +static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn)
> +{
> + bool plugged;
> + struct platform_device *codec_pdev;
> +
> + if (!hdmi->audio || IS_ERR(hdmi->audio))
> + return -EINVAL;
Given the current code structure, how can this ever be true when the
function is called?
> + codec_pdev = platform_get_drvdata(hdmi->audio);
> + if (!codec_pdev || IS_ERR(codec_pdev))
> + return -EINVAL;
This doesn't seem like a good idea as I've pointed out above.
> +
> + mutex_lock(&hdmi->mutex);
> + hdmi->plugged_cb = fn;
> + plugged = hdmi->last_connector_result == connector_status_connected;
> + handle_plugged_change(hdmi, plugged);
> + mutex_unlock(&hdmi->mutex);
Should be a blank line here for readability.
> + return 0;
> +}
> +
> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
> {
> regmap_update_bits(hdmi->regm, reg << hdmi->reg_shift, mask, data);
> @@ -2044,6 +2083,7 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> {
> struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> connector);
> + enum drm_connector_status result;
>
> mutex_lock(&hdmi->mutex);
> hdmi->force = DRM_FORCE_UNSPECIFIED;
> @@ -2051,7 +2091,18 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> dw_hdmi_update_phy_mask(hdmi);
> mutex_unlock(&hdmi->mutex);
>
> - return hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> + result = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> +
> + mutex_lock(&hdmi->mutex);
> + if (result != hdmi->last_connector_result) {
> + dev_dbg(hdmi->dev, "read_hpd result: %d", result);
> + handle_plugged_change(hdmi,
> + result == connector_status_connected);
> + hdmi->last_connector_result = result;
> + }
> + mutex_unlock(&hdmi->mutex);
> +
> + return result;
> }
>
> static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> @@ -2460,6 +2511,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
> hdmi->rxsense = true;
> hdmi->phy_mask = (u8)~(HDMI_PHY_HPD | HDMI_PHY_RX_SENSE);
> hdmi->mc_clkdis = 0x7f;
> + hdmi->last_connector_result = connector_status_disconnected;
>
> mutex_init(&hdmi->mutex);
> mutex_init(&hdmi->audio_mutex);
> @@ -2653,6 +2705,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
> audio.hdmi = hdmi;
> audio.write = hdmi_writeb;
> audio.read = hdmi_readb;
> + audio.set_plugged_cb = hdmi_set_plugged_cb;
Why is this necessary?
The I2S audio driver already depends on the dw-hdmi module through its
use of functions already exported. Indirecting this through the
platform data makes no sense.
Just rename hdmi_set_plugged_cb to dw_hdmi_set_plugged_cb() and export
it for dw-hdmi-i2s-audio.c to use.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
On Fri, Jul 12, 2019 at 06:04:39PM +0800, Cheng-Yi Chiang wrote:
> Add an op in hdmi_codec_ops so codec driver can register callback
> function to handle plug event.
>
> Driver in DRM can use this callback function to report connector status.
>
> Signed-off-by: Cheng-Yi Chiang <[email protected]>
> ---
> include/sound/hdmi-codec.h | 16 +++++++++++++
> sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> index 7fea496f1f34..9a8661680256 100644
> --- a/include/sound/hdmi-codec.h
> +++ b/include/sound/hdmi-codec.h
> @@ -47,6 +47,9 @@ struct hdmi_codec_params {
> int channels;
> };
>
> +typedef void (*hdmi_codec_plugged_cb)(struct device *dev,
> + bool plugged);
> +
I'd like to pose a question for people to think about.
Firstly, typedefs are generally shunned in the kernel. However, for
these cases it seems to make sense.
However, should the "pointer"-ness be part of the typedef or not? To
see what I mean, consider:
typedef void (*hdmi_foo)(void);
int register_foo(hdmi_foo foo);
vs
typedef void hdmi_foo(void);
int register_foo(hdmi_foo *foo);
which is more in keeping with how we code non-typedef'd code - it's
obvious that foo is a pointer while reading the code.
It seems to me that the latter better matches what is in the kernel's
coding style, which states:
In general, a pointer, or a struct that has elements that can
reasonably be directly accessed should **never** be a typedef.
or maybe Documentation/process/coding-style.rst needs updating?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
On Fri, Jul 12, 2019 at 6:58 PM Russell King - ARM Linux admin
<[email protected]> wrote:
>
> On Fri, Jul 12, 2019 at 06:04:39PM +0800, Cheng-Yi Chiang wrote:
> > Add an op in hdmi_codec_ops so codec driver can register callback
> > function to handle plug event.
> >
> > Driver in DRM can use this callback function to report connector status.
> >
> > Signed-off-by: Cheng-Yi Chiang <[email protected]>
> > ---
> > include/sound/hdmi-codec.h | 16 +++++++++++++
> > sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 61 insertions(+)
> >
> > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> > index 7fea496f1f34..9a8661680256 100644
> > --- a/include/sound/hdmi-codec.h
> > +++ b/include/sound/hdmi-codec.h
> > @@ -47,6 +47,9 @@ struct hdmi_codec_params {
> > int channels;
> > };
> >
> > +typedef void (*hdmi_codec_plugged_cb)(struct device *dev,
> > + bool plugged);
> > +
>
> I'd like to pose a question for people to think about.
>
> Firstly, typedefs are generally shunned in the kernel. However, for
> these cases it seems to make sense.
>
> However, should the "pointer"-ness be part of the typedef or not? To
> see what I mean, consider:
>
> typedef void (*hdmi_foo)(void);
>
> int register_foo(hdmi_foo foo);
>
> vs
>
> typedef void hdmi_foo(void);
>
> int register_foo(hdmi_foo *foo);
>
> which is more in keeping with how we code non-typedef'd code - it's
> obvious that foo is a pointer while reading the code.
I have a different opinion. Its suffix "_cb" self-described it is a
callback function. Since function and function pointer are equivalent
in the language, I think we don't need to emphasize that it is a
function "pointer".
> It seems to me that the latter better matches what is in the kernel's
> coding style, which states:
>
> In general, a pointer, or a struct that has elements that can
> reasonably be directly accessed should **never** be a typedef.
>
> or maybe Documentation/process/coding-style.rst needs updating?
On Mon, Jul 15, 2019 at 11:56 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Fri, Jul 12, 2019 at 6:58 PM Russell King - ARM Linux admin
> <[email protected]> wrote:
> >
> > On Fri, Jul 12, 2019 at 06:04:39PM +0800, Cheng-Yi Chiang wrote:
> > > Add an op in hdmi_codec_ops so codec driver can register callback
> > > function to handle plug event.
> > >
> > > Driver in DRM can use this callback function to report connector status.
> > >
> > > Signed-off-by: Cheng-Yi Chiang <[email protected]>
> > > ---
> > > include/sound/hdmi-codec.h | 16 +++++++++++++
> > > sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++
> > > 2 files changed, 61 insertions(+)
> > >
> > > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> > > index 7fea496f1f34..9a8661680256 100644
> > > --- a/include/sound/hdmi-codec.h
> > > +++ b/include/sound/hdmi-codec.h
> > > @@ -47,6 +47,9 @@ struct hdmi_codec_params {
> > > int channels;
> > > };
> > >
> > > +typedef void (*hdmi_codec_plugged_cb)(struct device *dev,
> > > + bool plugged);
> > > +
> >
> > I'd like to pose a question for people to think about.
> >
> > Firstly, typedefs are generally shunned in the kernel. However, for
> > these cases it seems to make sense.
> >
> > However, should the "pointer"-ness be part of the typedef or not? To
> > see what I mean, consider:
> >
> > typedef void (*hdmi_foo)(void);
> >
> > int register_foo(hdmi_foo foo);
> >
> > vs
> >
> > typedef void hdmi_foo(void);
> >
> > int register_foo(hdmi_foo *foo);
> >
> > which is more in keeping with how we code non-typedef'd code - it's
> > obvious that foo is a pointer while reading the code.
> I have a different opinion. Its suffix "_cb" self-described it is a
> callback function. Since function and function pointer are equivalent
> in the language, I think we don't need to emphasize that it is a
> function "pointer".
>
>
Hi Russell and Tzungbi, thank you for the review.
Regarding this typedef of callback function, I found a thread
discussing this very long time ago:
https://yarchive.net/comp/linux/typedefs.html
From that thread, Linus gave an example of using typedef for function
pointer that is following to this pattern.
I also looked around how other driver use it:
$ git grep typedef | grep _cb | less | wc -l
138
$ git grep typedef | grep _cb | grep "(\*" | wc -l
115
Most of the typedef of callback function use this pattern.
So I think this should be fine.
Thanks!
> > It seems to me that the latter better matches what is in the kernel's
> > coding style, which states:
> >
> > In general, a pointer, or a struct that has elements that can
> > reasonably be directly accessed should **never** be a typedef.
> >
> > or maybe Documentation/process/coding-style.rst needs updating?
On Fri, Jul 12, 2019 at 6:48 PM Russell King - ARM Linux admin
<[email protected]> wrote:
>
> On Fri, Jul 12, 2019 at 06:04:40PM +0800, Cheng-Yi Chiang wrote:
> > Allow codec driver register callback function for plug event.
> >
> > The callback registration flow:
> > dw-hdmi <--- hw-hdmi-i2s-audio <--- hdmi-codec
> >
> > dw-hdmi-i2s-audio implements hook_plugged_cb op
> > so codec driver can register the callback.
> >
> > dw-hdmi implements set_plugged_cb op so platform device can register the
> > callback.
> >
> > When connector plug/unplug event happens, report this event using the
> > callback.
> >
> > Make sure that audio and drm are using the single source of truth for
> > connector status.
> >
> > Signed-off-by: Cheng-Yi Chiang <[email protected]>
> > ---
> > .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 +
> > .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++++
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 55 ++++++++++++++++++-
> > 3 files changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> > index 63b5756f463b..f523c590984e 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> > @@ -2,6 +2,8 @@
> > #ifndef DW_HDMI_AUDIO_H
> > #define DW_HDMI_AUDIO_H
> >
> > +#include <sound/hdmi-codec.h>
> > +
> > struct dw_hdmi;
> >
> > struct dw_hdmi_audio_data {
> > @@ -17,6 +19,7 @@ struct dw_hdmi_i2s_audio_data {
> >
> > void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
> > u8 (*read)(struct dw_hdmi *hdmi, int offset);
> > + int (*set_plugged_cb)(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn);
> > };
> >
> > #endif
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> > index 5cbb71a866d5..7b93cf05c985 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> > @@ -104,10 +104,20 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
> > return -EINVAL;
> > }
> >
> > +static int dw_hdmi_i2s_hook_plugged_cb(struct device *dev, void *data,
> > + hdmi_codec_plugged_cb fn)
> > +{
> > + struct dw_hdmi_i2s_audio_data *audio = data;
> > + struct dw_hdmi *hdmi = audio->hdmi;
> > +
> > + return audio->set_plugged_cb(hdmi, fn);
> > +}
> > +
> > static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
> > .hw_params = dw_hdmi_i2s_hw_params,
> > .audio_shutdown = dw_hdmi_i2s_audio_shutdown,
> > .get_dai_id = dw_hdmi_i2s_get_dai_id,
> > + .hook_plugged_cb = dw_hdmi_i2s_hook_plugged_cb,
> > };
> >
> > static int snd_dw_hdmi_probe(struct platform_device *pdev)
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index 045b1b13fd0e..ce6646067472 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -26,6 +26,8 @@
> > #include <drm/drm_probe_helper.h>
> > #include <drm/bridge/dw_hdmi.h>
> >
> > +#include <sound/hdmi-codec.h>
> > +
> > #include <uapi/linux/media-bus-format.h>
> > #include <uapi/linux/videodev2.h>
> >
> > @@ -185,6 +187,9 @@ struct dw_hdmi {
> > void (*disable_audio)(struct dw_hdmi *hdmi);
> >
> > struct cec_notifier *cec_notifier;
> > +
> > + hdmi_codec_plugged_cb plugged_cb;
> > + enum drm_connector_status last_connector_result;
> > };
> >
> > #define HDMI_IH_PHY_STAT0_RX_SENSE \
> > @@ -209,6 +214,40 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset)
> > return val;
> > }
> >
> > +static void handle_plugged_change(struct dw_hdmi *hdmi, bool plugged)
> > +{
> > + struct platform_device *codec_pdev;
> > +
> > + if (!hdmi->audio || IS_ERR(hdmi->audio))
> > + return;
> > + codec_pdev = platform_get_drvdata(hdmi->audio);
> > + if (!codec_pdev || IS_ERR(codec_pdev))
> > + return;
>
> This looks fragile to me, poking about in another device's driver data
> from another driver is really not a good design decision. I think this
> can be simplified if the registration function took the function
> pointer and the struct device pointer, and then you only need one test
> below:
>
Hi Russell, Thank you for the detailed review.
ACK to this suggestion.
I have updated the registration function following your suggestion in v4.
It looks much cleaner.
> > + if (!hdmi->plugged_cb)
> > + return;
> > +
> > + hdmi->plugged_cb(&codec_pdev->dev, plugged);
> > +}
> > +
> > +static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn)
> > +{
> > + bool plugged;
> > + struct platform_device *codec_pdev;
> > +
> > + if (!hdmi->audio || IS_ERR(hdmi->audio))
> > + return -EINVAL;
>
> Given the current code structure, how can this ever be true when the
> function is called?
>
ACK
Removed in v4.
> > + codec_pdev = platform_get_drvdata(hdmi->audio);
> > + if (!codec_pdev || IS_ERR(codec_pdev))
> > + return -EINVAL;
>
> This doesn't seem like a good idea as I've pointed out above.
>
ACK
Fixed in v4.
> > +
> > + mutex_lock(&hdmi->mutex);
> > + hdmi->plugged_cb = fn;
> > + plugged = hdmi->last_connector_result == connector_status_connected;
> > + handle_plugged_change(hdmi, plugged);
> > + mutex_unlock(&hdmi->mutex);
>
> Should be a blank line here for readability.
>
ACK
Fixed in v4.
> > + return 0;
> > +}
> > +
> > static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
> > {
> > regmap_update_bits(hdmi->regm, reg << hdmi->reg_shift, mask, data);
> > @@ -2044,6 +2083,7 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> > {
> > struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> > connector);
> > + enum drm_connector_status result;
> >
> > mutex_lock(&hdmi->mutex);
> > hdmi->force = DRM_FORCE_UNSPECIFIED;
> > @@ -2051,7 +2091,18 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> > dw_hdmi_update_phy_mask(hdmi);
> > mutex_unlock(&hdmi->mutex);
> >
> > - return hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> > + result = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
> > +
> > + mutex_lock(&hdmi->mutex);
> > + if (result != hdmi->last_connector_result) {
> > + dev_dbg(hdmi->dev, "read_hpd result: %d", result);
> > + handle_plugged_change(hdmi,
> > + result == connector_status_connected);
> > + hdmi->last_connector_result = result;
> > + }
> > + mutex_unlock(&hdmi->mutex);
> > +
> > + return result;
> > }
> >
> > static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> > @@ -2460,6 +2511,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
> > hdmi->rxsense = true;
> > hdmi->phy_mask = (u8)~(HDMI_PHY_HPD | HDMI_PHY_RX_SENSE);
> > hdmi->mc_clkdis = 0x7f;
> > + hdmi->last_connector_result = connector_status_disconnected;
> >
> > mutex_init(&hdmi->mutex);
> > mutex_init(&hdmi->audio_mutex);
> > @@ -2653,6 +2705,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
> > audio.hdmi = hdmi;
> > audio.write = hdmi_writeb;
> > audio.read = hdmi_readb;
> > + audio.set_plugged_cb = hdmi_set_plugged_cb;
>
> Why is this necessary?
>
> The I2S audio driver already depends on the dw-hdmi module through its
> use of functions already exported. Indirecting this through the
> platform data makes no sense.
>
> Just rename hdmi_set_plugged_cb to dw_hdmi_set_plugged_cb() and export
> it for dw-hdmi-i2s-audio.c to use.
ACK.
Your suggestion makes sense.
Removed in v4.
>
> Thanks.
Thanks so much!
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up