| Message ID | E1T8ZzV-0002vp-2j@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/03/2012 06:59 PM, Liam Girdwood wrote: > Use a dedicated member to store dmaengine data so that drivers can > use private data for their own purposes. > The idea was that we'll eventually get to a point where we won't need private data for the drivers using the generic dmaengine code. But for the transitional period there is snd_dmaengine_pcm_{set,get}_data which allows to attach driver private data to the dmaengine pcm. For an example see how the other users of dmaengine pcm handle this. > Signed-off-by: Liam Girdwood <lrg@ti.com> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > include/sound/pcm.h | 2 ++ > sound/soc/soc-dmaengine-pcm.c | 2 +- > 2 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index cdca2ab..f9e4909 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -269,6 +269,7 @@ struct snd_pcm_hw_constraint_list { > }; > > struct snd_pcm_hwptr_log; > +struct dmaengine_pcm_runtime_data; > > struct snd_pcm_runtime { > /* -- Status -- */ > @@ -345,6 +346,7 @@ struct snd_pcm_runtime { > unsigned char *dma_area; /* DMA area */ > dma_addr_t dma_addr; /* physical bus address (not accessible from main CPU) */ > size_t dma_bytes; /* size of DMA area */ > + struct dmaengine_pcm_runtime_data *dmaengine_data; > > struct snd_dma_buffer *dma_buffer_p; /* allocated buffer */ > > diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c > index 5df529e..27fa5ad 100644 > --- a/sound/soc/soc-dmaengine-pcm.c > +++ b/sound/soc/soc-dmaengine-pcm.c > @@ -40,7 +40,7 @@ struct dmaengine_pcm_runtime_data { > static inline struct dmaengine_pcm_runtime_data *substream_to_prtd( > const struct snd_pcm_substream *substream) > { > - return substream->runtime->private_data; > + return substream->runtime->dmaengine_data; > } > > /** -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 03, 2012 at 10:25:49PM +0200, Lars-Peter Clausen wrote: > On 09/03/2012 06:59 PM, Liam Girdwood wrote: > > Use a dedicated member to store dmaengine data so that drivers can > > use private data for their own purposes. > > > > The idea was that we'll eventually get to a point where we won't need private > data for the drivers using the generic dmaengine code. But for the transitional > period there is snd_dmaengine_pcm_{set,get}_data which allows to attach driver > private data to the dmaengine pcm. For an example see how the other users of > dmaengine pcm handle this. That's fine if you are writing new drivers from scatch, or know the driver you're converting inside-out. Neither applies here (I've struggled to do anything with the OMAP audio stuff for many many reasons.) I rather wish that people who did know the OMAP ASoC driver had stepped up to this conversion, but alas they haven't. In any case, if you want people to use the this soc-dmaengine helper then you have to make the conversion to it simple, and requiring everyone to totally restructure their drivers to use it does not make that process simple. What you have here is the result of several transformations to the driver, which would _not_ have been possible without this first patch from Liam. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/03/2012 10:43 PM, Russell King - ARM Linux wrote: > On Mon, Sep 03, 2012 at 10:25:49PM +0200, Lars-Peter Clausen wrote: >> On 09/03/2012 06:59 PM, Liam Girdwood wrote: >>> Use a dedicated member to store dmaengine data so that drivers can >>> use private data for their own purposes. >>> >> >> The idea was that we'll eventually get to a point where we won't need private >> data for the drivers using the generic dmaengine code. But for the transitional >> period there is snd_dmaengine_pcm_{set,get}_data which allows to attach driver >> private data to the dmaengine pcm. For an example see how the other users of >> dmaengine pcm handle this. > > That's fine if you are writing new drivers from scatch, or know the > driver you're converting inside-out. Neither applies here (I've > struggled to do anything with the OMAP audio stuff for many many > reasons.) > > I rather wish that people who did know the OMAP ASoC driver had stepped > up to this conversion, but alas they haven't. > > In any case, if you want people to use the this soc-dmaengine helper > then you have to make the conversion to it simple, and requiring > everyone to totally restructure their drivers to use it does not make > that process simple. > > What you have here is the result of several transformations to the > driver, which would _not_ have been possible without this first patch > from Liam. Ok, it might have been helpful in the conversion process, but for the final patch it would be nice if you could replace struct snd_pcm_runtime *runtime = substream->runtime; struct omap_runtime_data *prtd = runtime->private_data; struct omap_pcm_dma_data *dma_data = prtd->dma_data; with struct omap_pcm_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream); and in the hwparams callback use snd_dmaengine_pcm_set_data(substream, dma_data); and then drop patch 1 and 2 from the series. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At Mon, 03 Sep 2012 22:59:54 +0200, Lars-Peter Clausen wrote: > > On 09/03/2012 10:43 PM, Russell King - ARM Linux wrote: > > On Mon, Sep 03, 2012 at 10:25:49PM +0200, Lars-Peter Clausen wrote: > >> On 09/03/2012 06:59 PM, Liam Girdwood wrote: > >>> Use a dedicated member to store dmaengine data so that drivers can > >>> use private data for their own purposes. > >>> > >> > >> The idea was that we'll eventually get to a point where we won't need private > >> data for the drivers using the generic dmaengine code. But for the transitional > >> period there is snd_dmaengine_pcm_{set,get}_data which allows to attach driver > >> private data to the dmaengine pcm. For an example see how the other users of > >> dmaengine pcm handle this. > > > > That's fine if you are writing new drivers from scatch, or know the > > driver you're converting inside-out. Neither applies here (I've > > struggled to do anything with the OMAP audio stuff for many many > > reasons.) > > > > I rather wish that people who did know the OMAP ASoC driver had stepped > > up to this conversion, but alas they haven't. > > > > In any case, if you want people to use the this soc-dmaengine helper > > then you have to make the conversion to it simple, and requiring > > everyone to totally restructure their drivers to use it does not make > > that process simple. > > > > What you have here is the result of several transformations to the > > driver, which would _not_ have been possible without this first patch > > from Liam. > > Ok, it might have been helpful in the conversion process, but for the final > patch it would be nice if you could replace > > struct snd_pcm_runtime *runtime = substream->runtime; > struct omap_runtime_data *prtd = runtime->private_data; > struct omap_pcm_dma_data *dma_data = prtd->dma_data; > with > struct omap_pcm_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream); > > and in the hwparams callback use > > snd_dmaengine_pcm_set_data(substream, dma_data); > > and then drop patch 1 and 2 from the series. We discussed with Liam about the addition of new field in ALSA core, and concluded that a bit different approach, at least, more generic name is preferred, even if a new field is inevitably needed. So, eventually some change may happen in near future in ALSA core side, but still it'd be really helpful if the callers have been standardized beforehand with a helper function like above. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Takashi, On 09/04/2012 04:14 PM, Takashi Iwai wrote: >> Ok, it might have been helpful in the conversion process, but for the final >> patch it would be nice if you could replace >> >> struct snd_pcm_runtime *runtime = substream->runtime; >> struct omap_runtime_data *prtd = runtime->private_data; >> struct omap_pcm_dma_data *dma_data = prtd->dma_data; >> with >> struct omap_pcm_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream); >> >> and in the hwparams callback use >> >> snd_dmaengine_pcm_set_data(substream, dma_data); >> >> and then drop patch 1 and 2 from the series. > > We discussed with Liam about the addition of new field in ALSA core, > and concluded that a bit different approach, at least, more generic > name is preferred, even if a new field is inevitably needed. > > So, eventually some change may happen in near future in ALSA core > side, but still it'd be really helpful if the callers have been > standardized beforehand with a helper function like above. If the omap-pcm dmaengine conversion works on all OMAP versions (from OMAP1 to OMAP5) it is possible to avoid the additional field. My main concern at the moment is if we will need two sets of drivers to support OMAP1 and OMAP2/3/4/5. In all case we use the same omap-mcbsp driver which would need deal with two different type of ASoC platform driver (dmaengine and non-dmaengine). I hope we get confirmation from Janusz soon regarding to OMAP1 with dmaengine so we can plan on how to move forward.
On Tue, Sep 04, 2012 at 04:26:28PM +0300, Peter Ujfalusi wrote: > Hi Takashi, > > On 09/04/2012 04:14 PM, Takashi Iwai wrote: > >> Ok, it might have been helpful in the conversion process, but for the final > >> patch it would be nice if you could replace > >> > >> struct snd_pcm_runtime *runtime = substream->runtime; > >> struct omap_runtime_data *prtd = runtime->private_data; > >> struct omap_pcm_dma_data *dma_data = prtd->dma_data; > >> with > >> struct omap_pcm_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream); > >> > >> and in the hwparams callback use > >> > >> snd_dmaengine_pcm_set_data(substream, dma_data); > >> > >> and then drop patch 1 and 2 from the series. > > > > We discussed with Liam about the addition of new field in ALSA core, > > and concluded that a bit different approach, at least, more generic > > name is preferred, even if a new field is inevitably needed. > > > > So, eventually some change may happen in near future in ALSA core > > side, but still it'd be really helpful if the callers have been > > standardized beforehand with a helper function like above. > > If the omap-pcm dmaengine conversion works on all OMAP versions (from OMAP1 to > OMAP5) it is possible to avoid the additional field. > My main concern at the moment is if we will need two sets of drivers to > support OMAP1 and OMAP2/3/4/5. > In all case we use the same omap-mcbsp driver which would need deal with two > different type of ASoC platform driver (dmaengine and non-dmaengine). > I hope we get confirmation from Janusz soon regarding to OMAP1 with dmaengine > so we can plan on how to move forward. As the target for the DMA engine work is to kill off the OMAP private DMA APIs, then if OMAP1 doesn't work with the OMAP DMA engine driver, that's what needs fixing, rather than having two ASoC platform drivers. Note that time is ticking for the removal of the OMAP private DMA APIs (see feature-removal-schedule.txt) and it has to happen so that the next stage of the OMAP DMA engine conversion can happen - that is, to make the OMAP DMA engine support be a proper driver rather than just a DMA Engine to OMAP private DMA API conversion shim. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index cdca2ab..f9e4909 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -269,6 +269,7 @@ struct snd_pcm_hw_constraint_list { }; struct snd_pcm_hwptr_log; +struct dmaengine_pcm_runtime_data; struct snd_pcm_runtime { /* -- Status -- */ @@ -345,6 +346,7 @@ struct snd_pcm_runtime { unsigned char *dma_area; /* DMA area */ dma_addr_t dma_addr; /* physical bus address (not accessible from main CPU) */ size_t dma_bytes; /* size of DMA area */ + struct dmaengine_pcm_runtime_data *dmaengine_data; struct snd_dma_buffer *dma_buffer_p; /* allocated buffer */ diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c index 5df529e..27fa5ad 100644 --- a/sound/soc/soc-dmaengine-pcm.c +++ b/sound/soc/soc-dmaengine-pcm.c @@ -40,7 +40,7 @@ struct dmaengine_pcm_runtime_data { static inline struct dmaengine_pcm_runtime_data *substream_to_prtd( const struct snd_pcm_substream *substream) { - return substream->runtime->private_data; + return substream->runtime->dmaengine_data; } /**