| Message ID | 1418020087-22470-3-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi Vinod, Thank you for the patch. On Monday 08 December 2014 11:58:03 Vinod Koul wrote: > dma_slave_config is expected to be set for slave operations Only, not for > memcpy ones > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > Documentation/dmaengine/provider.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/dmaengine/provider.txt > b/Documentation/dmaengine/provider.txt index 2c391cfe37eb..e7ea9f87f499 > 100644 > --- a/Documentation/dmaengine/provider.txt > +++ b/Documentation/dmaengine/provider.txt > @@ -310,6 +310,8 @@ supported. > - Even though that structure contains a direction field, this > field is deprecated in favor of the direction argument given to > the prep_* functions > + - This call is required for only slave operations only. s/is required for only slave operations only/is only required for slave operations/ ? > + This should NOT be set or expected to be set for memcpy operations How about "Drivers that implement memcpy operations don't need to implement this call." ? It makes it clearer that drivers that support both slave and memcpy must implement dma_slave_config. > * device_pause > - Pauses a transfer on the channel
On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > Hi Vinod, > > Thank you for the patch. > > On Monday 08 December 2014 11:58:03 Vinod Koul wrote: > > dma_slave_config is expected to be set for slave operations Only, not for > > memcpy ones > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > Documentation/dmaengine/provider.txt | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/dmaengine/provider.txt > > b/Documentation/dmaengine/provider.txt index 2c391cfe37eb..e7ea9f87f499 > > 100644 > > --- a/Documentation/dmaengine/provider.txt > > +++ b/Documentation/dmaengine/provider.txt > > @@ -310,6 +310,8 @@ supported. > > - Even though that structure contains a direction field, this > > field is deprecated in favor of the direction argument given to > > the prep_* functions > > + - This call is required for only slave operations only. > > s/is required for only slave operations only/is only required for slave > operations/ ? Yup, reading back this looks terrible > > > + This should NOT be set or expected to be set for memcpy operations > > How about "Drivers that implement memcpy operations don't need to implement > this call." ? It makes it clearer that drivers that support both slave and > memcpy must implement dma_slave_config. That is a problem we want to fix of not having drivers which implement both slave and memcpy rely on dma_slave_config for memcpy operations. Maxime got bitten by that recently so lets fix documentation for this
On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > + This should NOT be set or expected to be set for memcpy operations > > > > How about "Drivers that implement memcpy operations don't need to implement > > this call." ? It makes it clearer that drivers that support both slave and > > memcpy must implement dma_slave_config. > > That is a problem we want to fix of not having drivers which implement both > slave and memcpy rely on dma_slave_config for memcpy operations. Maxime got > bitten by that recently so lets fix documentation for this I really think that while the documentation should make it clear, we should be able to support dmaengine drivers that implement both slave and async operations. It is totally allowed by the framework for now, and some hardware doesn't make any distinction between what's considered a slave transfer and a memcpy for example. So I'm not really convinced we should make that distinction in the framework either. Maxime
On Mon, Dec 08, 2014 at 03:13:16PM +0100, Maxime Ripard wrote: > On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > > + This should NOT be set or expected to be set for memcpy operations > > > > > > How about "Drivers that implement memcpy operations don't need to implement > > > this call." ? It makes it clearer that drivers that support both slave and > > > memcpy must implement dma_slave_config. > > > > That is a problem we want to fix of not having drivers which implement both > > slave and memcpy rely on dma_slave_config for memcpy operations. Maxime got > > bitten by that recently so lets fix documentation for this > > I really think that while the documentation should make it clear, we > should be able to support dmaengine drivers that implement both slave > and async operations. > > It is totally allowed by the framework for now, and some hardware > doesn't make any distinction between what's considered a slave > transfer and a memcpy for example. So I'm not really convinced we > should make that distinction in the framework either. the dma_slave_config simply doesn't make sense for memcpy. User should be able to invoke memcpy operation without making any other configuration.
Hi Vinod, On Monday 08 December 2014 21:44:49 Vinod Koul wrote: > On Mon, Dec 08, 2014 at 03:13:16PM +0100, Maxime Ripard wrote: > > On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > > > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > > > + This should NOT be set or expected to be set for memcpy > > > > > operations > > > > > > > > How about "Drivers that implement memcpy operations don't need to > > > > implement this call." ? It makes it clearer that drivers that support > > > > both slave and memcpy must implement dma_slave_config. > > > > > > That is a problem we want to fix of not having drivers which implement > > > both slave and memcpy rely on dma_slave_config for memcpy operations. > > > Maxime got bitten by that recently so lets fix documentation for this > > > > I really think that while the documentation should make it clear, we > > should be able to support dmaengine drivers that implement both slave > > and async operations. > > > > It is totally allowed by the framework for now, and some hardware > > doesn't make any distinction between what's considered a slave > > transfer and a memcpy for example. So I'm not really convinced we > > should make that distinction in the framework either. > > the dma_slave_config simply doesn't make sense for memcpy. User should be > able to invoke memcpy operation without making any other configuration. Of course. My point was that your proposed patch appears (to me) to mean that a driver that supports memcpy must no implement dma_slave_config. What the documentation ought to make clear is that memcpy must work without dma_slave_config, but drivers that support both memcpy and slave operations must implement dma_slave_config for the slave operations.
On Mon, Dec 08, 2014 at 09:44:49PM +0530, Vinod Koul wrote: > On Mon, Dec 08, 2014 at 03:13:16PM +0100, Maxime Ripard wrote: > > On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > > > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > > > + This should NOT be set or expected to be set for memcpy operations > > > > > > > > How about "Drivers that implement memcpy operations don't need to implement > > > > this call." ? It makes it clearer that drivers that support both slave and > > > > memcpy must implement dma_slave_config. > > > > > > That is a problem we want to fix of not having drivers which implement both > > > slave and memcpy rely on dma_slave_config for memcpy operations. Maxime got > > > bitten by that recently so lets fix documentation for this > > > > I really think that while the documentation should make it clear, we > > should be able to support dmaengine drivers that implement both slave > > and async operations. > > > > It is totally allowed by the framework for now, and some hardware > > doesn't make any distinction between what's considered a slave > > transfer and a memcpy for example. So I'm not really convinced we > > should make that distinction in the framework either. > > the dma_slave_config simply doesn't make sense for memcpy. User should be > able to invoke memcpy operation without making any other configuration. We do agree on that. But we shouldn't discourage people from implementing dma_slave_config if that makes sense either, and that's what you were implying. Maxime
On Mon, Dec 08, 2014 at 06:42:09PM +0200, Laurent Pinchart wrote: > Hi Vinod, > > On Monday 08 December 2014 21:44:49 Vinod Koul wrote: > > On Mon, Dec 08, 2014 at 03:13:16PM +0100, Maxime Ripard wrote: > > > On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > > > > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > > > > + This should NOT be set or expected to be set for memcpy > > > > > > operations > > > > > > > > > > How about "Drivers that implement memcpy operations don't need to > > > > > implement this call." ? It makes it clearer that drivers that support > > > > > both slave and memcpy must implement dma_slave_config. > > > > > > > > That is a problem we want to fix of not having drivers which implement > > > > both slave and memcpy rely on dma_slave_config for memcpy operations. > > > > Maxime got bitten by that recently so lets fix documentation for this > > > > > > I really think that while the documentation should make it clear, we > > > should be able to support dmaengine drivers that implement both slave > > > and async operations. > > > > > > It is totally allowed by the framework for now, and some hardware > > > doesn't make any distinction between what's considered a slave > > > transfer and a memcpy for example. So I'm not really convinced we > > > should make that distinction in the framework either. > > > > the dma_slave_config simply doesn't make sense for memcpy. User should be > > able to invoke memcpy operation without making any other configuration. > > Of course. My point was that your proposed patch appears (to me) to mean that > a driver that supports memcpy must no implement dma_slave_config. What the > documentation ought to make clear is that memcpy must work without > dma_slave_config, but drivers that support both memcpy and slave operations > must implement dma_slave_config for the slave operations. Very aptly said. Here is the updated text, that I am planning to add: - This call is mandatory for slave operations only. This should NOT be set or expected to be set for memcpy operations. If a driver support both, it should use this call for slave operations only and not for memcpy ones Let me know if we have any more holes in this :)
On Mon, Dec 08, 2014 at 05:59:58PM +0100, Maxime Ripard wrote: > On Mon, Dec 08, 2014 at 09:44:49PM +0530, Vinod Koul wrote: > > On Mon, Dec 08, 2014 at 03:13:16PM +0100, Maxime Ripard wrote: > > > On Mon, Dec 08, 2014 at 06:23:52PM +0530, Vinod Koul wrote: > > > > On Mon, Dec 08, 2014 at 10:00:41AM +0200, Laurent Pinchart wrote: > > > > > > + This should NOT be set or expected to be set for memcpy operations > > > > > > > > > > How about "Drivers that implement memcpy operations don't need to implement > > > > > this call." ? It makes it clearer that drivers that support both slave and > > > > > memcpy must implement dma_slave_config. > > > > > > > > That is a problem we want to fix of not having drivers which implement both > > > > slave and memcpy rely on dma_slave_config for memcpy operations. Maxime got > > > > bitten by that recently so lets fix documentation for this > > > > > > I really think that while the documentation should make it clear, we > > > should be able to support dmaengine drivers that implement both slave > > > and async operations. > > > > > > It is totally allowed by the framework for now, and some hardware > > > doesn't make any distinction between what's considered a slave > > > transfer and a memcpy for example. So I'm not really convinced we > > > should make that distinction in the framework either. > > > > the dma_slave_config simply doesn't make sense for memcpy. User should be > > able to invoke memcpy operation without making any other configuration. > > We do agree on that. But we shouldn't discourage people from > implementing dma_slave_config if that makes sense either, and that's > what you were implying. Yup :)
diff --git a/Documentation/dmaengine/provider.txt b/Documentation/dmaengine/provider.txt index 2c391cfe37eb..e7ea9f87f499 100644 --- a/Documentation/dmaengine/provider.txt +++ b/Documentation/dmaengine/provider.txt @@ -310,6 +310,8 @@ supported. - Even though that structure contains a direction field, this field is deprecated in favor of the direction argument given to the prep_* functions + - This call is required for only slave operations only. + This should NOT be set or expected to be set for memcpy operations * device_pause - Pauses a transfer on the channel
dma_slave_config is expected to be set for slave operations Only, not for memcpy ones Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- Documentation/dmaengine/provider.txt | 2 ++ 1 file changed, 2 insertions(+)