| Message ID | 20190705042623.129541-2-cychiang@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDMI jack support on RK3288 | expand |
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 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!
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.
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;
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(+)