| Message ID | 20200213155159.3235792-1-jbrunet@baylibre.com |
|---|---|
| Headers | show |
| Series | ASoC: meson: gx: add audio output support | expand |
On Thu, Feb 13, 2020 at 04:51:51PM +0100, Jerome Brunet wrote: > At the moment, querying the dai_name will stop of the first component > matching the dt node. This does not allow a device (single dt node) to > provide several ASoC components which could then be used through DT. > This change let the search go on if the xlate function of the component > returns an error, giving the possibility to another component to match > and return the dai_name. My first question here would be why you'd want to do that rather than combine everything into a single component since the hardware seems to be doing that anyway. Hopefully the rest of the series will answer this but it'd be good in the changelog here.
On Thu 13 Feb 2020 at 18:18, Mark Brown <broonie@kernel.org> wrote: > On Thu, Feb 13, 2020 at 04:51:51PM +0100, Jerome Brunet wrote: > >> At the moment, querying the dai_name will stop of the first component >> matching the dt node. This does not allow a device (single dt node) to >> provide several ASoC components which could then be used through DT. > >> This change let the search go on if the xlate function of the component >> returns an error, giving the possibility to another component to match >> and return the dai_name. > > My first question here would be why you'd want to do that rather than > combine everything into a single component since the hardware seems to > be doing that anyway. Hopefully the rest of the series will answer this > but it'd be good in the changelog here. Hi Mark, Sorry if I was not clear enough. This HW is messy. It is indeed one monolithic device which provides several functions/sub-devices/components I tried several approaches: * Just 1 component: This was ugly because the part that is present only on 1 SoC variant, I needed to reconstruct the dai, widget, route and control table which involved a fair amount of useless copies. * A lot of devices (and components) with syscon: This ended up being even uglier, difficult to work with since it did not really reflected the actual HW. The solution proposed here is just one device with 3 possible components (groups): * The CPU producers a associated path * The HDMI control * The Internal DAC control The impact on ASoC is rather small, the driver reflect quite well what the HW is and, with a sound-dai-cell=2, it fairly simple in DT as well. Do you think there is something wrong with a linux device providing several ASoC components ?
On Thu, Feb 13, 2020 at 06:37:41PM +0100, Jerome Brunet wrote: > > My first question here would be why you'd want to do that rather than > > combine everything into a single component since the hardware seems to > > be doing that anyway. Hopefully the rest of the series will answer this > > but it'd be good in the changelog here. > Do you think there is something wrong with a linux device providing > several ASoC components ? I don't know that it's actively wrong, it's more a comment about the changelog only describing the what of the change and not the why - the original idea for a component was that there should be a 1:1 mapping between components and devices but as you say it's not actually a big change to let things get split up more.
On Thu, Feb 13, 2020 at 04:51:55PM +0100, Jerome Brunet wrote: > +int aiu_add_component(struct device *dev, > + const struct snd_soc_component_driver *component_driver, > + struct snd_soc_dai_driver *dai_drv, > + int num_dai, > + const char *debugfs_prefix) > +{ > + struct snd_soc_component *component; > + > + component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL); > + if (!component) > + return -ENOMEM; > + > +#ifdef CONFIG_DEBUG_FS > + component->debugfs_prefix = debugfs_prefix; > +#endif You really shouldn't be doing this as it could conflict with something the machine driver wants to do however it's probably not going to be an issue in practice as it's not like there's going to be multiple SoCs in the card at once and if there were there'd doubltess be other issues.
On Thu 13 Feb 2020 at 19:21, Mark Brown <broonie@kernel.org> wrote: > On Thu, Feb 13, 2020 at 04:51:55PM +0100, Jerome Brunet wrote: > >> +int aiu_add_component(struct device *dev, >> + const struct snd_soc_component_driver *component_driver, >> + struct snd_soc_dai_driver *dai_drv, >> + int num_dai, >> + const char *debugfs_prefix) >> +{ >> + struct snd_soc_component *component; >> + >> + component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL); >> + if (!component) >> + return -ENOMEM; >> + >> +#ifdef CONFIG_DEBUG_FS >> + component->debugfs_prefix = debugfs_prefix; >> +#endif > > You really shouldn't be doing this as it could conflict with something > the machine driver wants to do however it's probably not going to be an > issue in practice as it's not like there's going to be multiple SoCs in > the card at once and if there were there'd doubltess be other issues. I'm not sure I understand (and I'd prefer to :) ) As you said before, initially the there was supposed to be a 1:1 mapping between device and component. The component name is directly derived from the device name, and the debugfs directory is created from component name. I would have preferred to use snd_soc_register_component() directly, but with multiple components from the same device I got: debugfs: Directory 'c1105400.audio-controller' with parent 'AWESOME-CARD' already present! debugfs: Directory 'c1105400.audio-controller' with parent 'AWESOME-CARD' already present! I copied the code above from other direct users of snd_soc_add_component() (soc-generic-dmaengine-pcm.c and stm32_adfsdm.c). I suppose they had the same name collision issue. Instead of addressing the debugfs side effect, maybe we could just make sure that each component name is unique within ASoC ? I'd be happy submit something if you think this can helpful.
On Fri, Feb 14, 2020 at 02:16:10PM +0100, Jerome Brunet wrote: > On Thu 13 Feb 2020 at 19:21, Mark Brown <broonie@kernel.org> wrote: > >> +#ifdef CONFIG_DEBUG_FS > >> + component->debugfs_prefix = debugfs_prefix; > >> +#endif > > You really shouldn't be doing this as it could conflict with something > > the machine driver wants to do however it's probably not going to be an > > issue in practice as it's not like there's going to be multiple SoCs in > > the card at once and if there were there'd doubltess be other issues. > I'm not sure I understand (and I'd prefer to :) ) > As you said before, initially the there was supposed to be a 1:1 mapping > between device and component. The component name is directly derived > from the device name, and the debugfs directory is created from component name. I understand why you're doing it but that feature is intended for the use of cards when they're integrating components, not for devices trying to register multiple components on the same device. This means that a card that tries to use the feature will conflict with what the driver is doing, but like I say there's no obvious use case for a card doing that. > Instead of addressing the debugfs side effect, maybe we could just make > sure that each component name is unique within ASoC ? I'd be happy submit > something if you think this can helpful. That'd be better.