| Message ID | 20190712100443.221322-2-cychiang@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add HDMI jack support on RK3288 | expand |
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 <cychiang@chromium.org> > --- > 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?
On Fri, Jul 12, 2019 at 6:58 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> 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 <cychiang@chromium.org> > > --- > > 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 <tzungbi@google.com> wrote: > > On Fri, Jul 12, 2019 at 6:58 PM Russell King - ARM Linux admin > <linux@armlinux.org.uk> 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 <cychiang@chromium.org> > > > --- > > > 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?
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;
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 | 45 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+)