| Message ID | 1519373550-2545-6-git-send-email-rakesh.a.ughreja@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2/23/18 2:12 AM, Rakesh Ughreja wrote: > Split snd_hda_codec_new into two separate functions. > snd_hda_codec_device_init allocates memory and registers with bus. > snd_hda_codec_device_new initialializes the fields and performs > snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC > codec drivers. > > In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be > called by ASoC codec drivers. I don't get the commit message. You first say that we can now reuse legacy HDaudio codec drivers in an ASoC framework, then say that there will be additional ASoC codec drivers? Why would we do this, it seems like a contradicting goal? > > Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com> > --- > sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++------------ > sound/pci/hda/hda_codec.h | 2 ++ > 2 files changed, 52 insertions(+), 17 deletions(-) > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > index 5bc3a74..c487411 100644 > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device *dev) > kfree(codec); > } > > +static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card, > + unsigned int codec_addr, struct hda_codec **codecp) > +{ > + int err; > + char component[31]; looks like magic number? > + struct hda_codec *codec; reverse x-mas tree order? > + > + dev_dbg(card->dev, "%s: entry\n", __func__); > + > + if (snd_BUG_ON(!bus)) > + return -EINVAL; > + if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) > + return -EINVAL; > + > + codec = kzalloc(sizeof(*codec), GFP_KERNEL); > + if (!codec) > + return -ENOMEM; > + > + sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); > + err = snd_hdac_device_init(&codec->core, &bus->core, component, > + codec_addr); > + if (err < 0) { > + kfree(codec); > + return err; > + } > + > + codec->core.type = HDA_DEV_LEGACY; > + *codecp = codec; > + > + return err; > +} > + > /** > * snd_hda_codec_new - create a HDA codec > * @bus: the bus to assign > @@ -867,7 +899,19 @@ static void snd_hda_codec_dev_release(struct device *dev) > int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, > unsigned int codec_addr, struct hda_codec **codecp) > { > - struct hda_codec *codec; > + int ret; > + > + ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp); > + if (ret < 0) > + return ret; > + > + return snd_hda_codec_device_new(bus, card, codec_addr, *codecp); > +} > +EXPORT_SYMBOL_GPL(snd_hda_codec_new); > + > +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, > + unsigned int codec_addr, struct hda_codec *codec) > +{ > char component[31]; > hda_nid_t fg; > int err; > @@ -877,25 +921,14 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, > .dev_free = snd_hda_codec_dev_free, > }; > > + dev_dbg(card->dev, "%s: entry\n", __func__); > + > if (snd_BUG_ON(!bus)) > return -EINVAL; > if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) > return -EINVAL; > > - codec = kzalloc(sizeof(*codec), GFP_KERNEL); > - if (!codec) > - return -ENOMEM; > - > - sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); > - err = snd_hdac_device_init(&codec->core, &bus->core, component, > - codec_addr); > - if (err < 0) { > - kfree(codec); > - return err; > - } > - > codec->core.dev.release = snd_hda_codec_dev_release; > - codec->core.type = HDA_DEV_LEGACY; > codec->core.exec_verb = codec_exec_verb; > > codec->bus = bus; > @@ -955,15 +988,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, > if (err < 0) > goto error; > > - if (codecp) > - *codecp = codec; > return 0; > > error: > put_device(hda_codec_dev(codec)); > return err; > } > -EXPORT_SYMBOL_GPL(snd_hda_codec_new); > +EXPORT_SYMBOL_GPL(snd_hda_codec_device_new); > > /** > * snd_hda_codec_update_widgets - Refresh widget caps and pin defaults > @@ -2979,6 +3010,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec) > sync_power_up_states(codec); > return 0; > } > +EXPORT_SYMBOL_GPL(snd_hda_codec_build_controls); > > /* > * PCM stuff > @@ -3184,6 +3216,7 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec) > > return 0; > } > +EXPORT_SYMBOL_GPL(snd_hda_codec_parse_pcms); > > /* assign all PCMs of the given codec */ > int snd_hda_codec_build_pcms(struct hda_codec *codec) > diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h > index 681c360..8bbedf7 100644 > --- a/sound/pci/hda/hda_codec.h > +++ b/sound/pci/hda/hda_codec.h > @@ -307,6 +307,8 @@ struct hda_codec { > */ > int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, > unsigned int codec_addr, struct hda_codec **codecp); > +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, > + unsigned int codec_addr, struct hda_codec *codec); > int snd_hda_codec_configure(struct hda_codec *codec); > int snd_hda_codec_update_widgets(struct hda_codec *codec); > >
On Fri, 23 Feb 2018 09:12:26 +0100, Rakesh Ughreja wrote: > > +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, > + unsigned int codec_addr, struct hda_codec *codec) > +{ > char component[31]; Unused after the patch? Takashi
>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Friday, February 23, 2018 10:20 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa- >project.org; broonie@kernel.org; tiwai@suse.de; >liam.r.girdwood@linux.intel.com >Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio ><patches.audio@intel.com> >Subject: Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function > >On 2/23/18 2:12 AM, Rakesh Ughreja wrote: >> Split snd_hda_codec_new into two separate functions. >> snd_hda_codec_device_init allocates memory and registers with bus. >> snd_hda_codec_device_new initialializes the fields and performs >> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC >> codec drivers. >> >> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be >> called by ASoC codec drivers. > >I don't get the commit message. >You first say that we can now reuse legacy HDaudio codec drivers in an >ASoC framework, then say that there will be additional ASoC codec >drivers? Why would we do this, it seems like a contradicting goal? > Yes, its misleading, so correcting the line as, In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be called by ASoC wrapper around the legacy HDA driver (hdac_hda). >> >> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com> >> --- >> sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++-- >---------- >> sound/pci/hda/hda_codec.h | 2 ++ >> 2 files changed, 52 insertions(+), 17 deletions(-) >> >> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c >> index 5bc3a74..c487411 100644 >> --- a/sound/pci/hda/hda_codec.c >> +++ b/sound/pci/hda/hda_codec.c >> @@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device >*dev) >> kfree(codec); >> } >> >> +static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card >*card, >> + unsigned int codec_addr, struct hda_codec **codecp) >> +{ >> + int err; >> + char component[31]; > >looks like magic number? I kept it as it was in the previous code, but now defined a macro #define COMPONENT_NAME_SIZE 31 > >> + struct hda_codec *codec; > >reverse x-mas tree order? Yes, changed. Regards, Rakesh
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Friday, February 23, 2018 10:22 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: alsa-devel@alsa-project.org; broonie@kernel.org; >liam.r.girdwood@linux.intel.com; Koul, Vinod <vinod.koul@intel.com>; Patches >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new >function > >On Fri, 23 Feb 2018 09:12:26 +0100, >Rakesh Ughreja wrote: >> >> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, >> + unsigned int codec_addr, struct hda_codec *codec) >> +{ >> char component[31]; > >Unused after the patch? We still use the component variable to construct the name, so I think we still need it. Following code, + sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); + err = snd_hdac_device_init(&codec->core, &bus->core, component, + codec_addr); Regards, Rakesh
On Mon, 26 Feb 2018 09:33:01 +0100, Ughreja, Rakesh A wrote: > > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@suse.de] > >Sent: Friday, February 23, 2018 10:22 PM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> > >Cc: alsa-devel@alsa-project.org; broonie@kernel.org; > >liam.r.girdwood@linux.intel.com; Koul, Vinod <vinod.koul@intel.com>; Patches > >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com > >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new > >function > > > >On Fri, 23 Feb 2018 09:12:26 +0100, > >Rakesh Ughreja wrote: > >> > >> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, > >> + unsigned int codec_addr, struct hda_codec *codec) > >> +{ > >> char component[31]; > > > >Unused after the patch? > > We still use the component variable to construct the name, so I think we still > need it. Following code, > > + sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); > + err = snd_hdac_device_init(&codec->core, &bus->core, component, > + codec_addr); Ah yeah, I re-sused the variable for the name string. It should have been named as "name" or such. Takashi
>-----Original Message----- >From: Takashi Iwai [mailto:tiwai@suse.de] >Sent: Monday, February 26, 2018 2:11 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >Cc: alsa-devel@alsa-project.org; broonie@kernel.org; >liam.r.girdwood@linux.intel.com; Koul, Vinod <vinod.koul@intel.com>; Patches >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new >function > >On Mon, 26 Feb 2018 09:33:01 +0100, >Ughreja, Rakesh A wrote: >> >> >> >> >-----Original Message----- >> >From: Takashi Iwai [mailto:tiwai@suse.de] >> >Sent: Friday, February 23, 2018 10:22 PM >> >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com> >> >Cc: alsa-devel@alsa-project.org; broonie@kernel.org; >> >liam.r.girdwood@linux.intel.com; Koul, Vinod <vinod.koul@intel.com>; >Patches >> >Audio <patches.audio@intel.com>; pierre-louis.bossart@linux.intel.com >> >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new >> >function >> > >> >On Fri, 23 Feb 2018 09:12:26 +0100, >> >Rakesh Ughreja wrote: >> >> >> >> +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card >*card, >> >> + unsigned int codec_addr, struct hda_codec *codec) >> >> +{ >> >> char component[31]; >> > >> >Unused after the patch? >> >> We still use the component variable to construct the name, so I think we still >> need it. Following code, >> >> + sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); >> + err = snd_hdac_device_init(&codec->core, &bus->core, component, >> + codec_addr); > >Ah yeah, I re-sused the variable for the name string. >It should have been named as "name" or such. Let me correct it then. I will rename it to char name[DEVICE_NAME_LEN]; Regards, Rakesh
On 2/26/18 1:28 AM, Ughreja, Rakesh A wrote: > > >> -----Original Message----- >> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >> Sent: Friday, February 23, 2018 10:20 PM >> To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa- >> project.org; broonie@kernel.org; tiwai@suse.de; >> liam.r.girdwood@linux.intel.com >> Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio >> <patches.audio@intel.com> >> Subject: Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function >> >> On 2/23/18 2:12 AM, Rakesh Ughreja wrote: >>> Split snd_hda_codec_new into two separate functions. >>> snd_hda_codec_device_init allocates memory and registers with bus. >>> snd_hda_codec_device_new initialializes the fields and performs >>> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC >>> codec drivers. >>> >>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be >>> called by ASoC codec drivers. >> >> I don't get the commit message. >> You first say that we can now reuse legacy HDaudio codec drivers in an >> ASoC framework, then say that there will be additional ASoC codec >> drivers? Why would we do this, it seems like a contradicting goal? >> > > Yes, its misleading, so correcting the line as, > > In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be > called by ASoC wrapper around the legacy HDA driver (hdac_hda). Still confusing. You are both reusing codec drivers and defining a wrapper, so are you reusing parts of the codec drivers only?
>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >Sent: Tuesday, February 27, 2018 12:44 AM >To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa- >project.org; broonie@kernel.org; tiwai@suse.de; >liam.r.girdwood@linux.intel.com >Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio ><patches.audio@intel.com> >Subject: Re: [alsa-devel] [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new >function > >On 2/26/18 1:28 AM, Ughreja, Rakesh A wrote: >> >> >>> -----Original Message----- >>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com] >>> Sent: Friday, February 23, 2018 10:20 PM >>> To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa- >>> project.org; broonie@kernel.org; tiwai@suse.de; >>> liam.r.girdwood@linux.intel.com >>> Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio >>> <patches.audio@intel.com> >>> Subject: Re: [PATCH v1 5/9] ALSA: hda - split snd_hda_codec_new function >>> >>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote: >>>> Split snd_hda_codec_new into two separate functions. >>>> snd_hda_codec_device_init allocates memory and registers with bus. >>>> snd_hda_codec_device_new initialializes the fields and performs >>>> snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC >>>> codec drivers. >>>> >>>> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be >>>> called by ASoC codec drivers. >>> >>> I don't get the commit message. >>> You first say that we can now reuse legacy HDaudio codec drivers in an >>> ASoC framework, then say that there will be additional ASoC codec >>> drivers? Why would we do this, it seems like a contradicting goal? >>> >> >> Yes, its misleading, so correcting the line as, >> >> In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be >> called by ASoC wrapper around the legacy HDA driver (hdac_hda). > >Still confusing. You are both reusing codec drivers and defining a >wrapper, so are you reusing parts of the codec drivers only? There is a kernel module called snd-hda-codec and it reused by all the legacy codec drivers. I am marking few additional functions with EXPORT_SYMBOL in the snd-hda-codec and not in the codec driver. These new additional functions are used by hdac_hda kernel module. Existing legacy codec driver is reused completely as it is and not in parts. Regards, Rakesh
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 5bc3a74..c487411 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -856,6 +856,38 @@ static void snd_hda_codec_dev_release(struct device *dev) kfree(codec); } +static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card, + unsigned int codec_addr, struct hda_codec **codecp) +{ + int err; + char component[31]; + struct hda_codec *codec; + + dev_dbg(card->dev, "%s: entry\n", __func__); + + if (snd_BUG_ON(!bus)) + return -EINVAL; + if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) + return -EINVAL; + + codec = kzalloc(sizeof(*codec), GFP_KERNEL); + if (!codec) + return -ENOMEM; + + sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); + err = snd_hdac_device_init(&codec->core, &bus->core, component, + codec_addr); + if (err < 0) { + kfree(codec); + return err; + } + + codec->core.type = HDA_DEV_LEGACY; + *codecp = codec; + + return err; +} + /** * snd_hda_codec_new - create a HDA codec * @bus: the bus to assign @@ -867,7 +899,19 @@ static void snd_hda_codec_dev_release(struct device *dev) int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp) { - struct hda_codec *codec; + int ret; + + ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp); + if (ret < 0) + return ret; + + return snd_hda_codec_device_new(bus, card, codec_addr, *codecp); +} +EXPORT_SYMBOL_GPL(snd_hda_codec_new); + +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, + unsigned int codec_addr, struct hda_codec *codec) +{ char component[31]; hda_nid_t fg; int err; @@ -877,25 +921,14 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, .dev_free = snd_hda_codec_dev_free, }; + dev_dbg(card->dev, "%s: entry\n", __func__); + if (snd_BUG_ON(!bus)) return -EINVAL; if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) return -EINVAL; - codec = kzalloc(sizeof(*codec), GFP_KERNEL); - if (!codec) - return -ENOMEM; - - sprintf(component, "hdaudioC%dD%d", card->number, codec_addr); - err = snd_hdac_device_init(&codec->core, &bus->core, component, - codec_addr); - if (err < 0) { - kfree(codec); - return err; - } - codec->core.dev.release = snd_hda_codec_dev_release; - codec->core.type = HDA_DEV_LEGACY; codec->core.exec_verb = codec_exec_verb; codec->bus = bus; @@ -955,15 +988,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, if (err < 0) goto error; - if (codecp) - *codecp = codec; return 0; error: put_device(hda_codec_dev(codec)); return err; } -EXPORT_SYMBOL_GPL(snd_hda_codec_new); +EXPORT_SYMBOL_GPL(snd_hda_codec_device_new); /** * snd_hda_codec_update_widgets - Refresh widget caps and pin defaults @@ -2979,6 +3010,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec) sync_power_up_states(codec); return 0; } +EXPORT_SYMBOL_GPL(snd_hda_codec_build_controls); /* * PCM stuff @@ -3184,6 +3216,7 @@ int snd_hda_codec_parse_pcms(struct hda_codec *codec) return 0; } +EXPORT_SYMBOL_GPL(snd_hda_codec_parse_pcms); /* assign all PCMs of the given codec */ int snd_hda_codec_build_pcms(struct hda_codec *codec) diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 681c360..8bbedf7 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -307,6 +307,8 @@ struct hda_codec { */ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp); +int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, + unsigned int codec_addr, struct hda_codec *codec); int snd_hda_codec_configure(struct hda_codec *codec); int snd_hda_codec_update_widgets(struct hda_codec *codec);
Split snd_hda_codec_new into two separate functions. snd_hda_codec_device_init allocates memory and registers with bus. snd_hda_codec_device_new initialializes the fields and performs snd_device_new. This enables reuse of legacy HDA codec drivers as ASoC codec drivers. In addition mark some functions with EXPORT_SYMBOL_GPL so that it can be called by ASoC codec drivers. Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com> --- sound/pci/hda/hda_codec.c | 67 +++++++++++++++++++++++++++++++++++------------ sound/pci/hda/hda_codec.h | 2 ++ 2 files changed, 52 insertions(+), 17 deletions(-)