[alsa-devel] [PATCH 0/4] Add HDMI jack support on RK3288
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.
Cheng-Yi Chiang (4): ASoC: hdmi-codec: Add an op to set callback function for plug event drm: bridge: dw-hdmi: Report connector status using callback ASoC: rockchip_max98090: Add dai_link for HDMI ASoC: rockchip_max98090: Add HDMI jack support
.../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 | 32 ++++- include/sound/hdmi-codec.h | 16 +++ sound/soc/codecs/hdmi-codec.c | 52 ++++++++ sound/soc/rockchip/rockchip_max98090.c | 112 ++++++++++++++---- 6 files changed, 201 insertions(+), 24 deletions(-)
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 cychiang@chromium.org --- include/sound/hdmi-codec.h | 16 +++++++++++ sound/soc/codecs/hdmi-codec.c | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 7fea496f1f34..26c02abb8eba 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 platform_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..5e7075f78c38 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,55 @@ 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) + return; + + if (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 platform_device *pdev, bool plugged) +{ + struct platform_device *codec_pdev = platform_get_drvdata(pdev); + struct hdmi_codec_priv *hcp = platform_get_drvdata(codec_pdev); + + 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; + + 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; + } + return 0; + } + return -EOPNOTSUPP; +} +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;
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 7fea496f1f34..26c02abb8eba 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 platform_device *dev,
bool plugged);
The callback prototype is "weird" by struct platform_device. Is it possible to having snd_soc_component instead of platform_device?
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);};
The first parameter dev could be removed. Not used.
On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote:
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
+typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev,
bool plugged);
The callback prototype is "weird" by struct platform_device. Is it possible to having snd_soc_component instead of platform_device?
Or if it's got to be a device why not just a generic device so we're not tied to a particular bus here?
On Fri, Jul 5, 2019 at 8:12 PM Mark Brown broonie@kernel.org wrote:
On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote:
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
+typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev,
bool plugged);The callback prototype is "weird" by struct platform_device. Is it possible to having snd_soc_component instead of platform_device?
Or if it's got to be a device why not just a generic device so we're not tied to a particular bus here?
My intention was to invoke the call in dw-hdmi.c like this:
hdmi->plugged_cb(hdmi->audio, result == connector_status_connected);
Here hdmi->audio is a platform_device. I think dw-hdmi can not get snd_soc_component easily. I can use a generic device here so the ops is more general. The calling will be like hdmi->plugged_cb(&hdmi->audio->dev, result == connector_status_connected); I will update this in v2. Thanks!
On Mon, Jul 8, 2019 at 1:03 PM Cheng-yi Chiang cychiang@chromium.org wrote:
On Fri, Jul 5, 2019 at 8:12 PM Mark Brown broonie@kernel.org wrote:
On Fri, Jul 05, 2019 at 03:08:37PM +0800, Tzung-Bi Shih wrote:
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
+typedef void (*hdmi_codec_plugged_cb)(struct platform_device *dev,
bool plugged);The callback prototype is "weird" by struct platform_device. Is it possible to having snd_soc_component instead of platform_device?
Or if it's got to be a device why not just a generic device so we're not tied to a particular bus here?
My intention was to invoke the call in dw-hdmi.c like this:
hdmi->plugged_cb(hdmi->audio, result == connector_status_connected);Here hdmi->audio is a platform_device. I think dw-hdmi can not get snd_soc_component easily. I can use a generic device here so the ops is more general. The calling will be like hdmi->plugged_cb(&hdmi->audio->dev, result == connector_status_connected); I will update this in v2. Thanks!
I have thought about this a bit more. And I think the more proper interface is to pass in a generic struct device* for codec. This way, the user of hdmi-codec driver on the DRM side is not limited to the relation chain of audio platform device -> codec platform device, which is just a special case in dw-hdmi driver. As long as DRM side can get hdmi-codec device pointer through drv_data, it can use this callback. Hope this makes the interface more generic.
On 2019-07-05 06:26, Cheng-Yi Chiang wrote:
+static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp,
unsigned int jack_status)+{
- if (!hcp->jack)
return;- if (jack_status != hcp->jack_status) {
snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT);hcp->jack_status = jack_status;- }
+}
Single "if" statement instead? The first "if" does not even cover all cases - if the secondary check fails, you'll "return;" too.
+/**
- 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;
- 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;}return 0;- }
- return -EOPNOTSUPP;
+} +EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect);
int ret = -EOPNOTSUPP; (...)
return ret;
In consequence, you can reduce the number of "return(s)" and also remove the redundant parenthesis for the if-statement used to set jack to NULL.
Czarek
On Tue, Jul 9, 2019 at 7:47 PM Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2019-07-05 06:26, Cheng-Yi Chiang wrote:
+static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp,
unsigned int jack_status)+{
if (!hcp->jack)return;if (jack_status != hcp->jack_status) {snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT);hcp->jack_status = jack_status;}+}
Single "if" statement instead? The first "if" does not even cover all cases - if the secondary check fails, you'll "return;" too.
ACK. I will fix in v2.
+/**
- 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;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;}return 0;}return -EOPNOTSUPP;+} +EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect);
int ret = -EOPNOTSUPP; (...)
return ret;
In consequence, you can reduce the number of "return(s)" and also remove the redundant parenthesis for the if-statement used to set jack to NULL.
Czarek
ACK will fix in v2.
Thanks a lot for the review!
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 cychiang@chromium.org --- .../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 | 34 ++++++++++++++++++- 3 files changed, 46 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..c69a399fc7ca 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,17 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) return val; }
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn) +{ + mutex_lock(&hdmi->mutex); + hdmi->plugged_cb = fn; + if (hdmi->audio && !IS_ERR(hdmi->audio)) + fn(hdmi->audio, + hdmi->last_connector_result == connector_status_connected); + 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 +2060,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 +2068,20 @@ 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); + if (hdmi->plugged_cb && hdmi->audio && !IS_ERR(hdmi->audio)) { + hdmi->plugged_cb(hdmi->audio, + 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 +2490,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 +2684,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;
On 2019-07-05 06:26, 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.
I have a similar notification need for making a snd_ctl_notify() call from hdmi-codec when ELD changes, see [1] for work in progress patches (part of a dw-hdmi multi-channel lpcm series I am preparing).
Any suggestions on how to handle a ELD change notification? Should I use a similar pattern as in this series? (I lost track of the hdmi-notifier/drm_audio_component discussion)
[1] https://github.com/Kwiboo/linux-rockchip/compare/54b40fdd264c7ed96017271eb65...
Best regards, Jonas
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org
.../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 | 34 ++++++++++++++++++- 3 files changed, 46 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..c69a399fc7ca 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,17 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) return val; }
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn) +{
- mutex_lock(&hdmi->mutex);
- hdmi->plugged_cb = fn;
- if (hdmi->audio && !IS_ERR(hdmi->audio))
fn(hdmi->audio,hdmi->last_connector_result == connector_status_connected);- 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 +2060,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 +2068,20 @@ 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);if (hdmi->plugged_cb && hdmi->audio && !IS_ERR(hdmi->audio)) {hdmi->plugged_cb(hdmi->audio,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 +2490,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 +2684,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;
On Fri, Jul 5, 2019 at 1:45 PM Jonas Karlman jonas@kwiboo.se wrote:
On 2019-07-05 06:26, 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.
I have a similar notification need for making a snd_ctl_notify() call from hdmi-codec when ELD changes, see [1] for work in progress patches (part of a dw-hdmi multi-channel lpcm series I am preparing).
Any suggestions on how to handle a ELD change notification? Should I use a similar pattern as in this series?
Hi Jonas, I think we are using a very similar pattern. The difference is that in my series the function is not exposed on hdmi-codec.h. I think your method makes sense for your case because dw-hdmi-i2s-audio.c needs to access and update data inside dw_hdmi_i2s_audio_data, while in my use case it is only a thin layer setting up the callback for jack status.
(I lost track of the hdmi-notifier/drm_audio_component discussion)
It was a long discussion. I think the conclusion was that if we are only talking about hdmi-codec, then we just need to extend the ops exposed in hdmi-codec and don't need to use hdmi-notifier or drm_audio_component.
[1] https://github.com/Kwiboo/linux-rockchip/compare/54b40fdd264c7ed96017271eb65...
Best regards, Jonas
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org
.../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 | 34 ++++++++++++++++++- 3 files changed, 46 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..c69a399fc7ca 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,17 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) return val; }
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn) +{
mutex_lock(&hdmi->mutex);hdmi->plugged_cb = fn;if (hdmi->audio && !IS_ERR(hdmi->audio))fn(hdmi->audio,hdmi->last_connector_result == connector_status_connected);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 +2060,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 +2068,20 @@ 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);if (hdmi->plugged_cb && hdmi->audio && !IS_ERR(hdmi->audio)) {hdmi->plugged_cb(hdmi->audio,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 +2490,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 +2684,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;
On Fri, Jul 05, 2019 at 03:31:24PM +0800, Cheng-yi Chiang wrote:
It was a long discussion. I think the conclusion was that if we are only talking about hdmi-codec, then we just need to extend the ops exposed in hdmi-codec and don't need to use hdmi-notifier or drm_audio_component.
What I'd picked up from the bits of that discussion that I followed was that there was some desire to come up with a unified approach to ELD notification rather than having to go through this discussion repeatedly? That would certianly seem more sensible. Admittedly it was a long thread with lots of enormous mails so I didn't follow the whole thing.
On Sat, Jul 6, 2019 at 1:16 AM Mark Brown broonie@kernel.org wrote:
On Fri, Jul 05, 2019 at 03:31:24PM +0800, Cheng-yi Chiang wrote:
It was a long discussion. I think the conclusion was that if we are only talking about hdmi-codec, then we just need to extend the ops exposed in hdmi-codec and don't need to use hdmi-notifier or drm_audio_component.
What I'd picked up from the bits of that discussion that I followed was that there was some desire to come up with a unified approach to ELD notification rather than having to go through this discussion repeatedly? That would certianly seem more sensible. Admittedly it was a long thread with lots of enormous mails so I didn't follow the whole thing.
Hi Mark, thanks for following the long thread. The end of the discussion was at https://lkml.org/lkml/2019/6/20/1397
Quoted from Daniel's suggestion: " I need to think about this more, but if all we need to look at is hdmi_codec, then I think this becomes a lot easier. And we can ignore drm_audio_component.h completely. "
My thought is that the codec driver under ASoC are only these two: hdac_hdmi.c and hdmi-codec.c ( forgive me if I missed others. I just grep "hdmi" under sound/soc/codecs/ ) hdac_hdmi.c is like a wrapper for HDA and drm_audio_components. hdmi-codec.c is the only other codec driver that cares HDMI under ASoC. So adding the jack/eld support at hdmi-codec driver can cover the existing use cases for HDMI codec driver in ASoC. That said, adding a new unified approach for Jack/ELD notification that will only be used by hdmi-codec seems not a high priority. Hope this explanation helps your decision. Thanks!
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
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);+}
The first parameter dev could be removed. Not used.
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..c69a399fc7ca 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,17 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) return val; }
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn) +{
mutex_lock(&hdmi->mutex);hdmi->plugged_cb = fn;if (hdmi->audio && !IS_ERR(hdmi->audio))
I would expect if IS_ERR(hdmi->audio), then this should not be called (i.e. should exit somewhere earlier).
fn(hdmi->audio,hdmi->last_connector_result == connector_status_connected);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 +2060,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 +2068,20 @@ 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);if (hdmi->plugged_cb && hdmi->audio && !IS_ERR(hdmi->audio)) {
Share the same concern above.
On Fri, Jul 5, 2019 at 3:09 PM Tzung-Bi Shih tzungbi@google.com wrote:
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
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);+}
The first parameter dev could be removed. Not used.
Hi Tzungbi, thanks for the review.
I am not sure about this one. I think it depends on the DRM driver so I need to keep both.. E.g. drivers/gpu/drm/rockchip/cdn-dp-core.c it needs dev to access the required data. You can check this patch:
"efc9194bcff8 ASoC: hdmi-codec: callback function will be called with private data"
It explains that some cases use *dev, some cases use *data.
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..c69a399fc7ca 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,17 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) return val; }
+static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn) +{
mutex_lock(&hdmi->mutex);hdmi->plugged_cb = fn;if (hdmi->audio && !IS_ERR(hdmi->audio))I would expect if IS_ERR(hdmi->audio), then this should not be called (i.e. should exit somewhere earlier).
ACK. I should fix that. Thanks!
fn(hdmi->audio,hdmi->last_connector_result == connector_status_connected);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 +2060,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 +2068,20 @@ 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);if (hdmi->plugged_cb && hdmi->audio && !IS_ERR(hdmi->audio)) {Share the same concern above.
ACK
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 cychiang@chromium.org --- sound/soc/rockchip/rockchip_max98090.c | 91 +++++++++++++++++++------- 1 file changed, 68 insertions(+), 23 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c index c5fc24675a33..195309d1225a 100644 --- a/sound/soc/rockchip/rockchip_max98090.c +++ b/sound/soc/rockchip/rockchip_max98090.c @@ -41,6 +41,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 +53,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 +61,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 +95,59 @@ 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 && ret != -ENOTSUPP) { + dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret); return ret; }
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 && ret != -ENOTSUPP) { + 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-audio-codec.3.auto", "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 +160,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 +197,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);
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c index c5fc24675a33..195309d1225a 100644 --- a/sound/soc/rockchip/rockchip_max98090.c +++ b/sound/soc/rockchip/rockchip_max98090.c static int rk_aif1_hw_params(struct snd_pcm_substream *substream, @@ -92,38 +95,59 @@ 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 && ret != -ENOTSUPP) {dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret); return ret; } 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 && ret != -ENOTSUPP) {dev_err(codec_dai->dev, "Can't set codec dai clock %d\n", ret); return ret; }
Does it imply: it is acceptable even if they are "not supported"?
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-audio-codec.3.auto", "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 +160,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 +197,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.410.gd8fdbe21b5-goog
On Fri, Jul 5, 2019 at 3:10 PM Tzung-Bi Shih tzungbi@google.com wrote:
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang cychiang@chromium.org wrote:
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c index c5fc24675a33..195309d1225a 100644 --- a/sound/soc/rockchip/rockchip_max98090.c +++ b/sound/soc/rockchip/rockchip_max98090.c static int rk_aif1_hw_params(struct snd_pcm_substream *substream, @@ -92,38 +95,59 @@ 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 && ret != -ENOTSUPP) {dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n", ret);
I should remove this change because cpu dai should support sysclk ops.
return ret; } 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 && ret != -ENOTSUPP) {dev_err(codec_dai->dev, "Can't set codec dai clock %d\n", ret); return ret; }Does it imply: it is acceptable even if they are "not supported"?
Thank you for this good catch. Here machine driver has the knowledge of deciding whether it is expected to see the ops is not supported. For HDMI path using hdmi-codec driver, it is expected to see -ENOTSUPP. But it is not expected for codec DAI of max98090. I should distinguish them.
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-audio-codec.3.auto", "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 +160,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 +197,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.410.gd8fdbe21b5-goog
In machine driver, create a jack and let hdmi-codec report jack status.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org --- sound/soc/rockchip/rockchip_max98090.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c index 195309d1225a..c0e430ca4d12 100644 --- a/sound/soc/rockchip/rockchip_max98090.c +++ b/sound/soc/rockchip/rockchip_max98090.c @@ -15,6 +15,7 @@ #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <sound/hdmi-codec.h>
#include "rockchip_i2s.h" #include "../codecs/ts3a227e.h" @@ -129,6 +130,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] = { @@ -146,6 +166,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), } };
On Fri, Jul 05, 2019 at 12:26:19PM +0800, Cheng-Yi Chiang wrote:
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.
I think this looks reasonable. There's the entire question of getting rid of the platform_device in hdmi_codec an roll our own devices (so that it all looks a bit cleaner, like e.g. the cec stuff does). But that can also be done in a follow-up (if you can convince reviewers of that). -Daniel
Cheng-Yi Chiang (4): ASoC: hdmi-codec: Add an op to set callback function for plug event drm: bridge: dw-hdmi: Report connector status using callback ASoC: rockchip_max98090: Add dai_link for HDMI ASoC: rockchip_max98090: Add HDMI jack support
.../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 | 32 ++++- include/sound/hdmi-codec.h | 16 +++ sound/soc/codecs/hdmi-codec.c | 52 ++++++++ sound/soc/rockchip/rockchip_max98090.c | 112 ++++++++++++++---- 6 files changed, 201 insertions(+), 24 deletions(-)
-- 2.22.0.410.gd8fdbe21b5-goog
participants (6)
-
Cezary Rojewski -
Cheng-Yi Chiang -
Daniel Vetter -
Jonas Karlman -
Mark Brown -
Tzung-Bi Shih