| Message ID | 87o91ktd6m.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: add soc-component.c | expand |
On Wed, Jul 24, 2019 at 10:52:05AM +0900, Kuninori Morimoto wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Current ALSA SoC is directly using component->driver->ops->xxx, > thus, it is deep nested, and makes code difficult to read, > and is not good for encapsulation. > This patch adds new snd_soc_component_ioctrl() and use it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c > index 7ab68de..064d16c 100644 > --- a/sound/soc/soc-pcm.c > +++ b/sound/soc/soc-pcm.c > @@ -1116,6 +1116,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) > component = rtdcom->component; > > offset = snd_soc_component_pointer(component, substream); > + This whitespace change belongs in the previous patch. > /* FIXME: use 1st pointer */ > if (offset > 0) > break; > @@ -2455,16 +2456,15 @@ static int soc_pcm_ioctl(struct snd_pcm_substream *substream, > struct snd_soc_pcm_runtime *rtd = substream->private_data; > struct snd_soc_component *component; > struct snd_soc_rtdcom_list *rtdcom; > + int ret; > > for_each_rtdcom(rtd, rtdcom) { > component = rtdcom->component; > > - if (!component->driver->ops || > - !component->driver->ops->ioctl) > - continue; > - > + ret = snd_soc_component_ioctl(component, substream, cmd, arg); > /* FIXME: use 1st ioctl */ > - return component->driver->ops->ioctl(substream, cmd, arg); > + if (ret != -ENOTSUPP) > + return ret; This feels a little forced, and also changes behaviour if any ioctl callbacks already report -ENOTSUPP. I wonder if for some/all of these functions it might be worth abstracting them at a higher level so the whole for_each_rtdcom loop moves into the helper function similar to what I did with compress stuff, for example in these patches: 1e57b82891ad ("ASoC: compress: Add helper functions for component open/free") 4ef0ecb80e34 ("ASoC: compress: Add helper functions for component trigger/set_params") Thanks, Charles
Hi Charles > > for_each_rtdcom(rtd, rtdcom) { > > component = rtdcom->component; > > > > - if (!component->driver->ops || > > - !component->driver->ops->ioctl) > > - continue; > > - > > + ret = snd_soc_component_ioctl(component, substream, cmd, arg); > > /* FIXME: use 1st ioctl */ > > - return component->driver->ops->ioctl(substream, cmd, arg); > > + if (ret != -ENOTSUPP) > > + return ret; > > This feels a little forced, and also changes behaviour if any > ioctl callbacks already report -ENOTSUPP. > > I wonder if for some/all of these functions it might be worth > abstracting them at a higher level so the whole for_each_rtdcom > loop moves into the helper function similar to what I did with > compress stuff, for example in these patches: > > 1e57b82891ad ("ASoC: compress: Add helper functions for component open/free") > 4ef0ecb80e34 ("ASoC: compress: Add helper functions for component trigger/set_params") Thank you for your feedback. It sounds nice idea for me. will fix in v2 Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index c923d04..8230519 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -339,5 +339,8 @@ int snd_soc_component_trigger(struct snd_soc_component *component, int cmd); int snd_soc_component_pointer(struct snd_soc_component *component, struct snd_pcm_substream *substream); +int snd_soc_component_ioctl(struct snd_soc_component *component, + struct snd_pcm_substream *substream, + unsigned int cmd, void *arg); #endif /* __SOC_COMPONENT_H */ diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index 7cb936a..dbc8295 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -357,3 +357,14 @@ int snd_soc_component_pointer(struct snd_soc_component *component, return 0; } + +int snd_soc_component_ioctl(struct snd_soc_component *component, + struct snd_pcm_substream *substream, + unsigned int cmd, void *arg) +{ + if (component->driver->ops && + component->driver->ops->ioctl) + return component->driver->ops->ioctl(substream, cmd, arg); + + return -ENOTSUPP; +} diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7ab68de..064d16c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1116,6 +1116,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) component = rtdcom->component; offset = snd_soc_component_pointer(component, substream); + /* FIXME: use 1st pointer */ if (offset > 0) break; @@ -2455,16 +2456,15 @@ static int soc_pcm_ioctl(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom; + int ret; for_each_rtdcom(rtd, rtdcom) { component = rtdcom->component; - if (!component->driver->ops || - !component->driver->ops->ioctl) - continue; - + ret = snd_soc_component_ioctl(component, substream, cmd, arg); /* FIXME: use 1st ioctl */ - return component->driver->ops->ioctl(substream, cmd, arg); + if (ret != -ENOTSUPP) + return ret; } return snd_pcm_lib_ioctl(substream, cmd, arg);