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 | 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;
--
2.22.0.410.gd8fdbe21b5-goog
On Fri, Jul 5, 2019 at 12:26 PM Cheng-Yi Chiang <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 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 Mon, Jul 8, 2019 at 1:03 PM Cheng-yi Chiang <[email protected]> wrote:
>
> On Fri, Jul 5, 2019 at 8:12 PM Mark Brown <[email protected]> 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 <[email protected]> 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 Tue, Jul 9, 2019 at 7:47 PM Cezary Rojewski
<[email protected]> 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!