| Message ID | 1460967452-24574-2-git-send-email-zhengxing@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 18, 2016 at 04:17:30PM +0800, Xing Zheng wrote: > This patch add a callback when a codec have the jack detect feature. > +void snd_soc_jack_codec_detect(struct snd_soc_codec *codec, > + struct snd_soc_jack *jack) > +{ > + if (codec && codec->driver && codec->driver->detect_jack) > + codec->driver->detect_jack(codec, jack); > +} > +EXPORT_SYMBOL_GPL(snd_soc_jack_codec_detect); I've no idea what this is supposed to do sorry, you need a much better changelog and probably also a restructuring of the code. This says it is adding a callback but it doesn't do that, it adds an export of a function that calls an operation in a driver with no explanation.
Hi Mark, On 2016?04?18? 17:22, Mark Brown wrote: > On Mon, Apr 18, 2016 at 04:17:30PM +0800, Xing Zheng wrote: >> This patch add a callback when a codec have the jack detect feature. >> +void snd_soc_jack_codec_detect(struct snd_soc_codec *codec, >> + struct snd_soc_jack *jack) >> +{ >> + if (codec&& codec->driver&& codec->driver->detect_jack) >> + codec->driver->detect_jack(codec, jack); >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_jack_codec_detect); > I've no idea what this is supposed to do sorry, you need a much better > changelog and probably also a restructuring of the code. This says it > is adding a callback but it doesn't do that, it adds an export of a > function that calls an operation in a driver with no explanation. Yes, sorry to forgot to add description for this API. The patch_1/3 add an export of a function that calls the _detect_jack_ function when a codec supports jack detection and fill the _detect_jack_ function in the struct snd_soc_codec_driver. I will clean up the commit message. Thanks.
On Mon, Apr 18, 2016 at 05:43:51PM +0800, Xing Zheng wrote: > The patch_1/3 add an export of a function that calls the _detect_jack_ > function > when a codec supports jack detection and fill the _detect_jack_ function in > the > struct snd_soc_codec_driver. But why would anything ever call this function? How is this supposed to be used?
HI Mark, On 2016?04?18? 17:53, Mark Brown wrote: > On Mon, Apr 18, 2016 at 05:43:51PM +0800, Xing Zheng wrote: > >> The patch_1/3 add an export of a function that calls the _detect_jack_ >> function >> when a codec supports jack detection and fill the _detect_jack_ function in >> the >> struct snd_soc_codec_driver. > But why would anything ever call this function? How is this supposed to > be used? In my opinion, this function is mainly to provide simple-card (patch_2/3). This function will call and initialize jack detection if a codec supports it and fill the detect_jack function (patch_3/3 use it), and, we don't need to create a customized machine driver to call the codec jack detection, the simple-card is able to parse them. Thanks.
On 2016?04?18? 18:03, Xing Zheng wrote: > HI Mark, > > On 2016?04?18? 17:53, Mark Brown wrote: >> On Mon, Apr 18, 2016 at 05:43:51PM +0800, Xing Zheng wrote: >> >>> The patch_1/3 add an export of a function that calls the _detect_jack_ >>> function >>> when a codec supports jack detection and fill the _detect_jack_ >>> function in >>> the >>> struct snd_soc_codec_driver. >> But why would anything ever call this function? How is this supposed to >> be used? > In my opinion, this function is mainly to provide simple-card > (patch_2/3). > > This function will call and initialize jack detection if a codec > supports it and fill the detect_jack function (patch_3/3 use it), > and, we don't need to create a customized machine driver to call the > codec jack detection, the simple-card is able to > parse them. > > Thanks. > The dts usage like this: sound { ...... simple-audio-card,dai-link@0 { format = "i2s"; cpu { sound-dai = <&i2s0>; }; codec { sound-dai = <&codec>; simple-audio-card,codec-jack = "JACK_HEADSET", "JACK_BTN_0", "JACK_BTN_1", "JACK_BTN_2", "JACK_BTN_3"; }; }; ...... };
On Mon, Apr 18, 2016 at 06:20:00PM +0800, Xing Zheng wrote: > On 2016?04?18? 18:03, Xing Zheng wrote: > sound { > ...... > simple-audio-card,dai-link@0 { > format = "i2s"; > cpu { > sound-dai = <&i2s0>; > }; > > codec { > sound-dai = <&codec>; > > simple-audio-card,codec-jack = > "JACK_HEADSET", > "JACK_BTN_0", > "JACK_BTN_1", > "JACK_BTN_2", > "JACK_BTN_3"; > }; > }; This seems like it's only half the job and worryingly close to Linux internals. In particular the fact that the binding is specific to simple-card and the fact that it's being placed on the CODEC (rather than a separate object that the CODEC references) so that we can't combine multiple devices are both a concern. Dylan Reid did have an earlier go at defining a binding for this: http://thread.gmane.org/gmane.linux.alsa.devel/138906 which went through a couple more iterations but the work on that seemed to die off a bit.
On Mon, Apr 18, 2016 at 4:46 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Apr 18, 2016 at 06:20:00PM +0800, Xing Zheng wrote: >> On 2016?04?18? 18:03, Xing Zheng wrote: > >> sound { >> ...... >> simple-audio-card,dai-link@0 { >> format = "i2s"; >> cpu { >> sound-dai = <&i2s0>; >> }; >> >> codec { >> sound-dai = <&codec>; >> >> simple-audio-card,codec-jack = >> "JACK_HEADSET", >> "JACK_BTN_0", >> "JACK_BTN_1", >> "JACK_BTN_2", >> "JACK_BTN_3"; >> }; >> }; > > This seems like it's only half the job and worryingly close to Linux > internals. In particular the fact that the binding is specific to > simple-card and the fact that it's being placed on the CODEC (rather > than a separate object that the CODEC references) so that we can't > combine multiple devices are both a concern. > > Dylan Reid did have an earlier go at defining a binding for this: > > http://thread.gmane.org/gmane.linux.alsa.devel/138906 > > which went through a couple more iterations but the work on that seemed > to die off a bit. Thanks for forwarding this. I did intend to get back to that. But, almost a year later, obviously I haven't. Xing if you want to pick this up it would be great. Lars had some good points on that original discussion, which I think we can integrate into a single solution.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 02b4a21..ff105a4 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -547,6 +547,10 @@ static inline void snd_soc_jack_free_gpios(struct snd_soc_jack *jack, int count, } #endif +/* init jack detect via codec */ +void snd_soc_jack_codec_detect(struct snd_soc_codec *codec, + struct snd_soc_jack *jack); + /* codec register bit access */ int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned int reg, unsigned int mask, unsigned int value); @@ -920,6 +924,8 @@ struct snd_soc_codec_driver { enum snd_soc_dapm_type, int); bool ignore_pmdown_time; /* Doesn't benefit from pmdown delay */ + + void (*detect_jack)(struct snd_soc_codec *codec, struct snd_soc_jack *jack); }; /* SoC platform interface */ diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c index fbaa1bb..e292fb4 100644 --- a/sound/soc/soc-jack.c +++ b/sound/soc/soc-jack.c @@ -436,3 +436,11 @@ void snd_soc_jack_free_gpios(struct snd_soc_jack *jack, int count, } EXPORT_SYMBOL_GPL(snd_soc_jack_free_gpios); #endif /* CONFIG_GPIOLIB */ + +void snd_soc_jack_codec_detect(struct snd_soc_codec *codec, + struct snd_soc_jack *jack) +{ + if (codec && codec->driver && codec->driver->detect_jack) + codec->driver->detect_jack(codec, jack); +} +EXPORT_SYMBOL_GPL(snd_soc_jack_codec_detect);
This patch add a callback when a codec have the jack detect feature. Signed-off-by: Xing Zheng <zhengxing@rock-chips.com> --- include/sound/soc.h | 6 ++++++ sound/soc/soc-jack.c | 8 ++++++++ 2 files changed, 14 insertions(+)