| Message ID | 0a9fa618bd74e74c135ebee2e40b30d361c1d905.1523346135.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Apr 10, 2018 at 03:46:07PM +0800, Baolin Wang wrote: > This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces > for users to configure DMA. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c > index f8038de..c923fb0 100644 > --- a/drivers/dma/sprd-dma.c > +++ b/drivers/dma/sprd-dma.c > @@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc, > return vchan_tx_prep(&schan->vc, &sdesc->vd, flags); > } > > +static struct dma_async_tx_descriptor * > +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > + unsigned int sglen, enum dma_transfer_direction dir, > + unsigned long flags, void *context) > +{ > + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); > + struct sprd_dma_config *slave_cfg = &schan->slave_cfg; > + struct sprd_dma_desc *sdesc; > + struct scatterlist *sg; > + int ret, i; > + > + /* TODO: now we only support one sg for each DMA configuration. */ > + if (!is_slave_direction(slave_cfg->config.direction) || sglen > 1) the slave direction check seems wrong to me. .device_config shall give you dma_slave_config. You should check here if dir passed is slave or not. If you want to check parameters in slave_config then please use .device_config > + return NULL; > + > + sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT); > + if (!sdesc) > + return NULL; > + > + for_each_sg(sgl, sg, sglen, i) { > + if (slave_cfg->config.direction == DMA_MEM_TO_DEV) > + slave_cfg->config.src_addr = sg_dma_address(sg); Nope slave_config specifies peripheral address and not memory one passed here
Hi Vinod, On 11 April 2018 at 17:40, Vinod Koul <vinod.koul@intel.com> wrote: > On Tue, Apr 10, 2018 at 03:46:07PM +0800, Baolin Wang wrote: >> This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces >> for users to configure DMA. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c >> index f8038de..c923fb0 100644 >> --- a/drivers/dma/sprd-dma.c >> +++ b/drivers/dma/sprd-dma.c >> @@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc, >> return vchan_tx_prep(&schan->vc, &sdesc->vd, flags); >> } >> >> +static struct dma_async_tx_descriptor * >> +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, >> + unsigned int sglen, enum dma_transfer_direction dir, >> + unsigned long flags, void *context) >> +{ >> + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); >> + struct sprd_dma_config *slave_cfg = &schan->slave_cfg; >> + struct sprd_dma_desc *sdesc; >> + struct scatterlist *sg; >> + int ret, i; >> + >> + /* TODO: now we only support one sg for each DMA configuration. */ >> + if (!is_slave_direction(slave_cfg->config.direction) || sglen > 1) > > the slave direction check seems wrong to me. .device_config shall give you > dma_slave_config. You should check here if dir passed is slave or not. If > you want to check parameters in slave_config then please use .device_config Correct. Sorry I missed this and I will fix it in next version. > >> + return NULL; >> + >> + sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT); >> + if (!sdesc) >> + return NULL; >> + >> + for_each_sg(sgl, sg, sglen, i) { >> + if (slave_cfg->config.direction == DMA_MEM_TO_DEV) >> + slave_cfg->config.src_addr = sg_dma_address(sg); > > Nope slave_config specifies peripheral address and not memory one passed here OK. Thanks for your comments.
On 04/10/2018 09:46 AM, Baolin Wang wrote: [...] > +static int sprd_dma_slave_config(struct dma_chan *chan, > + struct dma_slave_config *config) > +{ > + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); > + struct sprd_dma_config *slave_cfg = > + container_of(config, struct sprd_dma_config, config); > + Please do not overload standard API with custom semantics. This makes the driver incompatible to the API and negates the whole idea of having a common API. E.g. this will crash when somebody passes a normal dma_slave_config struct to this function. > + memcpy(&schan->slave_cfg, slave_cfg, sizeof(*slave_cfg)); > + return 0; > +} -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17 April 2018 at 18:45, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 04/10/2018 09:46 AM, Baolin Wang wrote: > [...] >> +static int sprd_dma_slave_config(struct dma_chan *chan, >> + struct dma_slave_config *config) >> +{ >> + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); >> + struct sprd_dma_config *slave_cfg = >> + container_of(config, struct sprd_dma_config, config); >> + > > Please do not overload standard API with custom semantics. This makes the > driver incompatible to the API and negates the whole idea of having a common > API. E.g. this will crash when somebody passes a normal dma_slave_config > struct to this function. > Yes, we have discussed with Vinod how to use 'dma_slave_config' to reach our requirements. Thanks for your comments.
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c index f8038de..c923fb0 100644 --- a/drivers/dma/sprd-dma.c +++ b/drivers/dma/sprd-dma.c @@ -869,6 +869,52 @@ static int sprd_dma_config(struct dma_chan *chan, struct sprd_dma_desc *sdesc, return vchan_tx_prep(&schan->vc, &sdesc->vd, flags); } +static struct dma_async_tx_descriptor * +sprd_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, + unsigned int sglen, enum dma_transfer_direction dir, + unsigned long flags, void *context) +{ + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); + struct sprd_dma_config *slave_cfg = &schan->slave_cfg; + struct sprd_dma_desc *sdesc; + struct scatterlist *sg; + int ret, i; + + /* TODO: now we only support one sg for each DMA configuration. */ + if (!is_slave_direction(slave_cfg->config.direction) || sglen > 1) + return NULL; + + sdesc = kzalloc(sizeof(*sdesc), GFP_NOWAIT); + if (!sdesc) + return NULL; + + for_each_sg(sgl, sg, sglen, i) { + if (slave_cfg->config.direction == DMA_MEM_TO_DEV) + slave_cfg->config.src_addr = sg_dma_address(sg); + else + slave_cfg->config.dst_addr = sg_dma_address(sg); + } + + ret = sprd_dma_config(chan, sdesc, slave_cfg); + if (ret) { + kfree(sdesc); + return NULL; + } + + return vchan_tx_prep(&schan->vc, &sdesc->vd, flags); +} + +static int sprd_dma_slave_config(struct dma_chan *chan, + struct dma_slave_config *config) +{ + struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); + struct sprd_dma_config *slave_cfg = + container_of(config, struct sprd_dma_config, config); + + memcpy(&schan->slave_cfg, slave_cfg, sizeof(*slave_cfg)); + return 0; +} + static int sprd_dma_pause(struct dma_chan *chan) { struct sprd_dma_chn *schan = to_sprd_dma_chan(chan); @@ -996,6 +1042,8 @@ static int sprd_dma_probe(struct platform_device *pdev) sdev->dma_dev.device_tx_status = sprd_dma_tx_status; sdev->dma_dev.device_issue_pending = sprd_dma_issue_pending; sdev->dma_dev.device_prep_dma_memcpy = sprd_dma_prep_dma_memcpy; + sdev->dma_dev.device_prep_slave_sg = sprd_dma_prep_slave_sg; + sdev->dma_dev.device_config = sprd_dma_slave_config; sdev->dma_dev.device_pause = sprd_dma_pause; sdev->dma_dev.device_resume = sprd_dma_resume; sdev->dma_dev.device_terminate_all = sprd_dma_terminate_all;
This patch adds the 'device_config' and 'device_prep_slave_sg' interfaces for users to configure DMA. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- drivers/dma/sprd-dma.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)