Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693459 > unrolled thread

Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Vinod Koul <vinod.koul@intel.com> - 2017-07-21 10:00 +0200
    Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Pierre Yves MORDRET <pierre-yves.mordret@st.com> - 2017-07-21 11:40 +0200
      Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Vinod Koul <vinod.koul@intel.com> - 2017-07-21 12:00 +0200
        Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Pierre Yves MORDRET <pierre-yves.mordret@st.com> - 2017-07-21 12:40 +0200
          Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Vinod Koul <vinod.koul@intel.com> - 2017-07-21 19:20 +0200
            Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Pierre Yves MORDRET <pierre-yves.mordret@st.com> - 2017-07-24 11:40 +0200
              Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver Vinod Koul <vinod.koul@intel.com> - 2017-07-26 07:00 +0200

#1693459 — Re: [PATCH v2 2/4] dmaengine: Add STM32 MDMA driver

On Thu, Jul 06, 2017 at 02:25:39PM +0200, Pierre-Yves MORDRET wrote:

> +config STM32_MDMA
> +	bool "STMicroelectronics STM32 master dma support"
> +	depends on ARCH_STM32  || COMPILE_TEST
			    ^^^
why multiple spaces

> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
> +{
> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
> +
> +	while (((buf_len % max_width) || (tlen < max_width)) &&
> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
> +		max_width = max_width >> 1;

ok, this is a bit hard to read...

> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
> +				     enum dma_transfer_direction direction,
> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
> +				     u32 *mdma_ctbr, u32 buf_len)
> +{
> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
> +	phys_addr_t src_addr, dst_addr;
> +	int src_bus_width, dst_bus_width;
> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
> +	u32 ccr, ctcr, ctbr, tlen;
> +
> +	src_addr_width = chan->dma_config.src_addr_width;
> +	dst_addr_width = chan->dma_config.dst_addr_width;
> +	src_maxburst = chan->dma_config.src_maxburst;
> +	dst_maxburst = chan->dma_config.dst_maxburst;
> +	src_addr = chan->dma_config.src_addr;
> +	dst_addr = chan->dma_config.dst_addr;

this doesn't seem right to me, only the periphral address would come from
slave_config, the memory address is passed as an arg to transfer..

...

> +static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
> +				 struct stm32_mdma_desc *desc,
> +				 struct scatterlist *sgl, u32 sg_len,
> +				 enum dma_transfer_direction direction)
> +{
> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
> +	struct dma_slave_config *dma_config = &chan->dma_config;
> +	struct scatterlist *sg;
> +	dma_addr_t src_addr, dst_addr;
> +	u32 ccr, ctcr, ctbr;
> +	int i, ret = 0;
> +
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
> +			dev_err(chan2dev(chan), "Invalid block len\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
> +						&ctbr, sg_dma_len(sg));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (direction == DMA_MEM_TO_DEV) {
> +			src_addr = sg_dma_address(sg);
> +			dst_addr = dma_config->dst_addr;

and this seems correct, but then why are we doing it in
stm32_mdma_set_xfer_param()

> +static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
> +	struct dma_chan *c, struct scatterlist *sgl,
> +	u32 sg_len, enum dma_transfer_direction direction,
> +	unsigned long flags, void *context)

right justfied these please, it makes a terrible read

> +{
> +	struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c);
> +	struct stm32_mdma_desc *desc;
> +	int ret;
> +
> +	desc = stm32_mdma_alloc_desc(chan, sg_len);
> +	if (!desc)
> +		return NULL;
> +
> +	ret = stm32_mdma_setup_xfer(chan, desc, sgl, sg_len, direction);
> +	if (ret < 0)
> +		goto xfer_setup_err;
> +
> +	desc->cyclic = false;
> +
> +	return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
> +
> +xfer_setup_err:
> +	dma_pool_free(chan->desc_pool, &desc->hwdesc, desc->hwdesc_phys);
> +	kfree(desc);
> +	return NULL;
> +}
> +
> +static struct dma_async_tx_descriptor *stm32_mdma_prep_dma_cyclic(
> +	struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len,
> +	size_t period_len, enum dma_transfer_direction direction,
> +	unsigned long flags)

here too and few other places

> +static int stm32_mdma_probe(struct platform_device *pdev)
> +{
> +	struct stm32_mdma_chan *chan;
> +	struct stm32_mdma_device *dmadev;
> +	struct dma_device *dd;
> +	struct device_node *of_node;
> +	struct resource *res;
> +	u32 nr_channels, nr_requests;
> +	int i, count, ret;
> +
> +	of_node = pdev->dev.of_node;
> +	if (!of_node)
> +		return -ENODEV;
> +
> +	ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
> +	if (ret)
> +		nr_channels = STM32_MDMA_MAX_CHANNELS;
> +
> +	ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
> +	if (ret)
> +		nr_requests = STM32_MDMA_MAX_REQUESTS;

wouldn't it make sense to print error about these properties not being
present and continuing w/ defaults..?

and can we have device_property_xxx instead of of_ variants?

> +static int __init stm32_mdma_init(void)
> +{
> +	return platform_driver_probe(&stm32_mdma_driver, stm32_mdma_probe);
> +}
> +
> +subsys_initcall(stm32_mdma_init);

Where are the MODULE_xx tags, license is mandatory. You should add author
too.
 

-- 
~Vinod

[toc] | [next] | [standalone]

#1693522

On 07/21/2017 09:55 AM, Vinod Koul wrote:
> On Thu, Jul 06, 2017 at 02:25:39PM +0200, Pierre-Yves MORDRET wrote:
> 
>> +config STM32_MDMA
>> +	bool "STMicroelectronics STM32 master dma support"
>> +	depends on ARCH_STM32  || COMPILE_TEST
> 			    ^^^
> why multiple spaces

typo I guess

> 
>> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>> +{
>> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
>> +
>> +	while (((buf_len % max_width) || (tlen < max_width)) &&
>> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
>> +		max_width = max_width >> 1;
> 
> ok, this is a bit hard to read...

This code snippet has already been reworked and optimized. Would you mind to 
provide me a example with your expectation ? Thanks

> 
>> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
>> +				     enum dma_transfer_direction direction,
>> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
>> +				     u32 *mdma_ctbr, u32 buf_len)
>> +{
>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
>> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
>> +	phys_addr_t src_addr, dst_addr;
>> +	int src_bus_width, dst_bus_width;
>> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
>> +	u32 ccr, ctcr, ctbr, tlen;
>> +
>> +	src_addr_width = chan->dma_config.src_addr_width;
>> +	dst_addr_width = chan->dma_config.dst_addr_width;
>> +	src_maxburst = chan->dma_config.src_maxburst;
>> +	dst_maxburst = chan->dma_config.dst_maxburst;
>> +	src_addr = chan->dma_config.src_addr;
>> +	dst_addr = chan->dma_config.dst_addr;
> 
> this doesn't seem right to me, only the periphral address would come from
> slave_config, the memory address is passed as an arg to transfer..
> 
> ...
> 

Correct. But these locals are managed in the case statement below. if direction 
is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with 
Dev2Mem direction only src_addr(Peripheral) is considered.
However to disambiguate I can move src_addr & dst_addr affectation in the 
corresponding case statement if you'd like.

>> +static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
>> +				 struct stm32_mdma_desc *desc,
>> +				 struct scatterlist *sgl, u32 sg_len,
>> +				 enum dma_transfer_direction direction)
>> +{
>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>> +	struct dma_slave_config *dma_config = &chan->dma_config;
>> +	struct scatterlist *sg;
>> +	dma_addr_t src_addr, dst_addr;
>> +	u32 ccr, ctcr, ctbr;
>> +	int i, ret = 0;
>> +
>> +	for_each_sg(sgl, sg, sg_len, i) {
>> +		if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
>> +			dev_err(chan2dev(chan), "Invalid block len\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
>> +						&ctbr, sg_dma_len(sg));
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (direction == DMA_MEM_TO_DEV) {
>> +			src_addr = sg_dma_address(sg);
>> +			dst_addr = dma_config->dst_addr;
> 
> and this seems correct, but then why are we doing it in
> stm32_mdma_set_xfer_param()
> 

See comment above.

>> +static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
>> +	struct dma_chan *c, struct scatterlist *sgl,
>> +	u32 sg_len, enum dma_transfer_direction direction,
>> +	unsigned long flags, void *context)
> 
> right justfied these please, it makes a terrible read
> 

Given the amount of parameters difficult to right align.
Agree with this formatting ?

static struct dma_async_tx_descriptor
*stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
			  u32 sg_len, enum dma_transfer_direction direction,
			  unsigned long flags, void *context)

>> +{
>> +	struct stm32_mdma_chan *chan = to_stm32_mdma_chan(c);
>> +	struct stm32_mdma_desc *desc;
>> +	int ret;
>> +
>> +	desc = stm32_mdma_alloc_desc(chan, sg_len);
>> +	if (!desc)
>> +		return NULL;
>> +
>> +	ret = stm32_mdma_setup_xfer(chan, desc, sgl, sg_len, direction);
>> +	if (ret < 0)
>> +		goto xfer_setup_err;
>> +
>> +	desc->cyclic = false;
>> +
>> +	return vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
>> +
>> +xfer_setup_err:
>> +	dma_pool_free(chan->desc_pool, &desc->hwdesc, desc->hwdesc_phys);
>> +	kfree(desc);
>> +	return NULL;
>> +}
>> +
>> +static struct dma_async_tx_descriptor *stm32_mdma_prep_dma_cyclic(
>> +	struct dma_chan *c, dma_addr_t buf_addr, size_t buf_len,
>> +	size_t period_len, enum dma_transfer_direction direction,
>> +	unsigned long flags)
> 
> here too and few other places

ok. See comment above.

> 
>> +static int stm32_mdma_probe(struct platform_device *pdev)
>> +{
>> +	struct stm32_mdma_chan *chan;
>> +	struct stm32_mdma_device *dmadev;
>> +	struct dma_device *dd;
>> +	struct device_node *of_node;
>> +	struct resource *res;
>> +	u32 nr_channels, nr_requests;
>> +	int i, count, ret;
>> +
>> +	of_node = pdev->dev.of_node;
>> +	if (!of_node)
>> +		return -ENODEV;
>> +
>> +	ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
>> +	if (ret)
>> +		nr_channels = STM32_MDMA_MAX_CHANNELS;
>> +
>> +	ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
>> +	if (ret)
>> +		nr_requests = STM32_MDMA_MAX_REQUESTS;
> 
> wouldn't it make sense to print error about these properties not being
> present and continuing w/ defaults..?

Those are optional parameters as stated by bindings. I can print out a warning 
or info if you'd like but not error.

> 
> and can we have device_property_xxx instead of of_ variants?
> 

of course !

>> +static int __init stm32_mdma_init(void)
>> +{
>> +	return platform_driver_probe(&stm32_mdma_driver, stm32_mdma_probe);
>> +}
>> +
>> +subsys_initcall(stm32_mdma_init);
> 
> Where are the MODULE_xx tags, license is mandatory. You should add author
> too.
>   
> 

Correct. I will change the Header at the same time.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1693550

On Fri, Jul 21, 2017 at 09:30:00AM +0000, Pierre Yves MORDRET wrote:
> >> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
> >> +{
> >> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
> >> +
> >> +	while (((buf_len % max_width) || (tlen < max_width)) &&
> >> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
> >> +		max_width = max_width >> 1;
> > 
> > ok, this is a bit hard to read...
> 
> This code snippet has already been reworked and optimized. Would you mind to 
> provide me a example with your expectation ? Thanks

Code is optimized yes, but readable no

I would like readability to be improved upon...

> 
> > 
> >> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
> >> +				     enum dma_transfer_direction direction,
> >> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
> >> +				     u32 *mdma_ctbr, u32 buf_len)
> >> +{
> >> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
> >> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
> >> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
> >> +	phys_addr_t src_addr, dst_addr;
> >> +	int src_bus_width, dst_bus_width;
> >> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
> >> +	u32 ccr, ctcr, ctbr, tlen;
> >> +
> >> +	src_addr_width = chan->dma_config.src_addr_width;
> >> +	dst_addr_width = chan->dma_config.dst_addr_width;
> >> +	src_maxburst = chan->dma_config.src_maxburst;
> >> +	dst_maxburst = chan->dma_config.dst_maxburst;
> >> +	src_addr = chan->dma_config.src_addr;
> >> +	dst_addr = chan->dma_config.dst_addr;
> > 
> > this doesn't seem right to me, only the periphral address would come from
> > slave_config, the memory address is passed as an arg to transfer..
> > 
> > ...
> > 
> 
> Correct. But these locals are managed in the case statement below. if direction 
> is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with 
> Dev2Mem direction only src_addr(Peripheral) is considered.
> However to disambiguate I can move src_addr & dst_addr affectation in the 
> corresponding case statement if you'd like.

But below you are over writing both, so in effect this is wasted cycles..
anyway latter one is more clear, so lets remove from here.

> 
> >> +static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
> >> +				 struct stm32_mdma_desc *desc,
> >> +				 struct scatterlist *sgl, u32 sg_len,
> >> +				 enum dma_transfer_direction direction)
> >> +{
> >> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
> >> +	struct dma_slave_config *dma_config = &chan->dma_config;
> >> +	struct scatterlist *sg;
> >> +	dma_addr_t src_addr, dst_addr;
> >> +	u32 ccr, ctcr, ctbr;
> >> +	int i, ret = 0;
> >> +
> >> +	for_each_sg(sgl, sg, sg_len, i) {
> >> +		if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
> >> +			dev_err(chan2dev(chan), "Invalid block len\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
> >> +						&ctbr, sg_dma_len(sg));
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		if (direction == DMA_MEM_TO_DEV) {
> >> +			src_addr = sg_dma_address(sg);
> >> +			dst_addr = dma_config->dst_addr;
> > 
> > and this seems correct, but then why are we doing it in
> > stm32_mdma_set_xfer_param()
> > 
> 
> See comment above.
> 
> >> +static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
> >> +	struct dma_chan *c, struct scatterlist *sgl,
> >> +	u32 sg_len, enum dma_transfer_direction direction,
> >> +	unsigned long flags, void *context)
> > 
> > right justfied these please, it makes a terrible read
> > 
> 
> Given the amount of parameters difficult to right align.
> Agree with this formatting ?
> 
> static struct dma_async_tx_descriptor
> *stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
> 			  u32 sg_len, enum dma_transfer_direction direction,
> 			  unsigned long flags, void *context)

Yes looks much better :)

> >> +static int stm32_mdma_probe(struct platform_device *pdev)
> >> +{
> >> +	struct stm32_mdma_chan *chan;
> >> +	struct stm32_mdma_device *dmadev;
> >> +	struct dma_device *dd;
> >> +	struct device_node *of_node;
> >> +	struct resource *res;
> >> +	u32 nr_channels, nr_requests;
> >> +	int i, count, ret;
> >> +
> >> +	of_node = pdev->dev.of_node;
> >> +	if (!of_node)
> >> +		return -ENODEV;
> >> +
> >> +	ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
> >> +	if (ret)
> >> +		nr_channels = STM32_MDMA_MAX_CHANNELS;
> >> +
> >> +	ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
> >> +	if (ret)
> >> +		nr_requests = STM32_MDMA_MAX_REQUESTS;
> > 
> > wouldn't it make sense to print error about these properties not being
> > present and continuing w/ defaults..?
> 
> Those are optional parameters as stated by bindings. I can print out a warning 
> or info if you'd like but not error.

Are these mandatory properties or optional. In case of latter warn should
suffice.

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1693575

On 07/21/2017 11:54 AM, Vinod Koul wrote:
> On Fri, Jul 21, 2017 at 09:30:00AM +0000, Pierre Yves MORDRET wrote:
>>>> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 tlen)
>>>> +{
>>>> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
>>>> +
>>>> +	while (((buf_len % max_width) || (tlen < max_width)) &&
>>>> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
>>>> +		max_width = max_width >> 1;
>>>
>>> ok, this is a bit hard to read...
>>
>> This code snippet has already been reworked and optimized. Would you mind to
>> provide me a example with your expectation ? Thanks
> 
> Code is optimized yes, but readable no
> 
> I would like readability to be improved upon...
> 

gotcha

>>
>>>
>>>> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
>>>> +				     enum dma_transfer_direction direction,
>>>> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
>>>> +				     u32 *mdma_ctbr, u32 buf_len)
>>>> +{
>>>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>>>> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
>>>> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
>>>> +	phys_addr_t src_addr, dst_addr;
>>>> +	int src_bus_width, dst_bus_width;
>>>> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
>>>> +	u32 ccr, ctcr, ctbr, tlen;
>>>> +
>>>> +	src_addr_width = chan->dma_config.src_addr_width;
>>>> +	dst_addr_width = chan->dma_config.dst_addr_width;
>>>> +	src_maxburst = chan->dma_config.src_maxburst;
>>>> +	dst_maxburst = chan->dma_config.dst_maxburst;
>>>> +	src_addr = chan->dma_config.src_addr;
>>>> +	dst_addr = chan->dma_config.dst_addr;
>>>
>>> this doesn't seem right to me, only the periphral address would come from
>>> slave_config, the memory address is passed as an arg to transfer..
>>>
>>> ...
>>>
>>
>> Correct. But these locals are managed in the case statement below. if direction
>> is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
>> Dev2Mem direction only src_addr(Peripheral) is considered.
>> However to disambiguate I can move src_addr & dst_addr affectation in the
>> corresponding case statement if you'd like.
> 
> But below you are over writing both, so in effect this is wasted cycles..
> anyway latter one is more clear, so lets remove from here.
> 

Sorry I don't follow ... or miss something
For instance if direction is Mem2Dev ..._xfer_param is going to configure 
Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn 
will configure source given as parameter.
Don't the see the over-writing

>>
>>>> +static int stm32_mdma_setup_xfer(struct stm32_mdma_chan *chan,
>>>> +				 struct stm32_mdma_desc *desc,
>>>> +				 struct scatterlist *sgl, u32 sg_len,
>>>> +				 enum dma_transfer_direction direction)
>>>> +{
>>>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>>>> +	struct dma_slave_config *dma_config = &chan->dma_config;
>>>> +	struct scatterlist *sg;
>>>> +	dma_addr_t src_addr, dst_addr;
>>>> +	u32 ccr, ctcr, ctbr;
>>>> +	int i, ret = 0;
>>>> +
>>>> +	for_each_sg(sgl, sg, sg_len, i) {
>>>> +		if (sg_dma_len(sg) > STM32_MDMA_MAX_BLOCK_LEN) {
>>>> +			dev_err(chan2dev(chan), "Invalid block len\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		ret = stm32_mdma_set_xfer_param(chan, direction, &ccr, &ctcr,
>>>> +						&ctbr, sg_dma_len(sg));
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		if (direction == DMA_MEM_TO_DEV) {
>>>> +			src_addr = sg_dma_address(sg);
>>>> +			dst_addr = dma_config->dst_addr;
>>>
>>> and this seems correct, but then why are we doing it in
>>> stm32_mdma_set_xfer_param()
>>>
>>
>> See comment above.
>>
>>>> +static struct dma_async_tx_descriptor *stm32_mdma_prep_slave_sg(
>>>> +	struct dma_chan *c, struct scatterlist *sgl,
>>>> +	u32 sg_len, enum dma_transfer_direction direction,
>>>> +	unsigned long flags, void *context)
>>>
>>> right justfied these please, it makes a terrible read
>>>
>>
>> Given the amount of parameters difficult to right align.
>> Agree with this formatting ?
>>
>> static struct dma_async_tx_descriptor
>> *stm32_mdma_prep_slave_sg(struct dma_chan *c, struct scatterlist *sgl,
>> 			  u32 sg_len, enum dma_transfer_direction direction,
>> 			  unsigned long flags, void *context)
> 
> Yes looks much better :)
> 

Good :)

>>>> +static int stm32_mdma_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct stm32_mdma_chan *chan;
>>>> +	struct stm32_mdma_device *dmadev;
>>>> +	struct dma_device *dd;
>>>> +	struct device_node *of_node;
>>>> +	struct resource *res;
>>>> +	u32 nr_channels, nr_requests;
>>>> +	int i, count, ret;
>>>> +
>>>> +	of_node = pdev->dev.of_node;
>>>> +	if (!of_node)
>>>> +		return -ENODEV;
>>>> +
>>>> +	ret = of_property_read_u32(of_node, "dma-channels", &nr_channels);
>>>> +	if (ret)
>>>> +		nr_channels = STM32_MDMA_MAX_CHANNELS;
>>>> +
>>>> +	ret = of_property_read_u32(of_node, "dma-requests", &nr_requests);
>>>> +	if (ret)
>>>> +		nr_requests = STM32_MDMA_MAX_REQUESTS;
>>>
>>> wouldn't it make sense to print error about these properties not being
>>> present and continuing w/ defaults..?
>>
>> Those are optional parameters as stated by bindings. I can print out a warning
>> or info if you'd like but not error.
> 
> Are these mandatory properties or optional. In case of latter warn should
> suffice.
> 

optional. Let pick out warn then.

Thanks

[toc] | [prev] | [next] | [standalone]

#1693874

On Fri, Jul 21, 2017 at 10:32:49AM +0000, Pierre Yves MORDRET wrote:
> >>>> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
> >>>> +				     enum dma_transfer_direction direction,
> >>>> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
> >>>> +				     u32 *mdma_ctbr, u32 buf_len)
> >>>> +{
> >>>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
> >>>> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
> >>>> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
> >>>> +	phys_addr_t src_addr, dst_addr;
> >>>> +	int src_bus_width, dst_bus_width;
> >>>> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
> >>>> +	u32 ccr, ctcr, ctbr, tlen;
> >>>> +
> >>>> +	src_addr_width = chan->dma_config.src_addr_width;
> >>>> +	dst_addr_width = chan->dma_config.dst_addr_width;
> >>>> +	src_maxburst = chan->dma_config.src_maxburst;
> >>>> +	dst_maxburst = chan->dma_config.dst_maxburst;
> >>>> +	src_addr = chan->dma_config.src_addr;
> >>>> +	dst_addr = chan->dma_config.dst_addr;
> >>>
> >>> this doesn't seem right to me, only the periphral address would come from
> >>> slave_config, the memory address is passed as an arg to transfer..
> >>>
> >>> ...
> >>>
> >>
> >> Correct. But these locals are managed in the case statement below. if direction
> >> is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
> >> Dev2Mem direction only src_addr(Peripheral) is considered.
> >> However to disambiguate I can move src_addr & dst_addr affectation in the
> >> corresponding case statement if you'd like.
> > 
> > But below you are over writing both, so in effect this is wasted cycles..
> > anyway latter one is more clear, so lets remove from here.
> > 
> 
> Sorry I don't follow ... or miss something
> For instance if direction is Mem2Dev ..._xfer_param is going to configure 
> Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn 
> will configure source given as parameter.
> Don't the see the over-writing

ah re-looking at it, yes you are right.

The above two assignments threw me off, I should have read it properly.

But I think calculating for src and dstn always might not be optimal as you
would use one only, so should these be moved to respective case where they
are used...

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1694575

On 07/21/2017 12:32 PM, Pierre Yves MORDRET wrote:
 >
 >
 > On 07/21/2017 11:54 AM, Vinod Koul wrote:
 >> On Fri, Jul 21, 2017 at 09:30:00AM +0000, Pierre Yves MORDRET wrote:
 >>>>> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 
tlen)
 >>>>> +{
 >>>>> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
 >>>>> +
 >>>>> +	while (((buf_len % max_width) || (tlen < max_width)) &&
 >>>>> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
 >>>>> +		max_width = max_width >> 1;
 >>>>
 >>>> ok, this is a bit hard to read...
 >>>
 >>> This code snippet has already been reworked and optimized. Would you mind to
 >>> provide me a example with your expectation ? Thanks
 >>
 >> Code is optimized yes, but readable no
 >>
 >> I would like readability to be improved upon...
 >>
 >
 > gotcha
 >

Doest he code snippet below has a better looking for you ?

	for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
	     max_width > DMA_SLAVE_BUSWIDTH_1_BYTE; max_width >>= 1)
		if (((buf_len % max_width) == 0) && (tlen >= max_width))
			break;

On 07/21/2017 07:17 PM, Vinod Koul wrote:
> On Fri, Jul 21, 2017 at 10:32:49AM +0000, Pierre Yves MORDRET wrote:
>>>>>> +static int stm32_mdma_set_xfer_param(struct stm32_mdma_chan *chan,
>>>>>> +				     enum dma_transfer_direction direction,
>>>>>> +				     u32 *mdma_ccr, u32 *mdma_ctcr,
>>>>>> +				     u32 *mdma_ctbr, u32 buf_len)
>>>>>> +{
>>>>>> +	struct stm32_mdma_device *dmadev = stm32_mdma_get_dev(chan);
>>>>>> +	struct stm32_mdma_chan_config *chan_config = &chan->chan_config;
>>>>>> +	enum dma_slave_buswidth src_addr_width, dst_addr_width;
>>>>>> +	phys_addr_t src_addr, dst_addr;
>>>>>> +	int src_bus_width, dst_bus_width;
>>>>>> +	u32 src_maxburst, dst_maxburst, src_best_burst, dst_best_burst;
>>>>>> +	u32 ccr, ctcr, ctbr, tlen;
>>>>>> +
>>>>>> +	src_addr_width = chan->dma_config.src_addr_width;
>>>>>> +	dst_addr_width = chan->dma_config.dst_addr_width;
>>>>>> +	src_maxburst = chan->dma_config.src_maxburst;
>>>>>> +	dst_maxburst = chan->dma_config.dst_maxburst;
>>>>>> +	src_addr = chan->dma_config.src_addr;
>>>>>> +	dst_addr = chan->dma_config.dst_addr;
>>>>>
>>>>> this doesn't seem right to me, only the periphral address would come from
>>>>> slave_config, the memory address is passed as an arg to transfer..
>>>>>
>>>>> ...
>>>>>
>>>>
>>>> Correct. But these locals are managed in the case statement below. if direction
>>>> is Mem2Dev only dst_addr(Peripheral) is considered. In the other way around with
>>>> Dev2Mem direction only src_addr(Peripheral) is considered.
>>>> However to disambiguate I can move src_addr & dst_addr affectation in the
>>>> corresponding case statement if you'd like.
>>>
>>> But below you are over writing both, so in effect this is wasted cycles..
>>> anyway latter one is more clear, so lets remove from here.
>>>
>>
>> Sorry I don't follow ... or miss something
>> For instance if direction is Mem2Dev ..._xfer_param is going to configure
>> Destination Bus width and Addr given by slave_config. ..._setup_xfer in its turn
>> will configure source given as parameter.
>> Don't the see the over-writing
> 
> ah re-looking at it, yes you are right.
> 
> The above two assignments threw me off, I should have read it properly.
> 
> But I think calculating for src and dstn always might not be optimal as you
> would use one only, so should these be moved to respective case where they
> are used...
> 

Agree. This is my planned btw

Thanks
Py.

[toc] | [prev] | [next] | [standalone]

#1696775

On Mon, Jul 24, 2017 at 09:34:18AM +0000, Pierre Yves MORDRET wrote:
> 
> On 07/21/2017 12:32 PM, Pierre Yves MORDRET wrote:
>  >
>  >
>  > On 07/21/2017 11:54 AM, Vinod Koul wrote:
>  >> On Fri, Jul 21, 2017 at 09:30:00AM +0000, Pierre Yves MORDRET wrote:
>  >>>>> +static enum dma_slave_buswidth stm32_mdma_get_max_width(u32 buf_len, u32 
> tlen)
>  >>>>> +{
>  >>>>> +	enum dma_slave_buswidth max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
>  >>>>> +
>  >>>>> +	while (((buf_len % max_width) || (tlen < max_width)) &&
>  >>>>> +	       (max_width > DMA_SLAVE_BUSWIDTH_1_BYTE))
>  >>>>> +		max_width = max_width >> 1;
>  >>>>
>  >>>> ok, this is a bit hard to read...
>  >>>
>  >>> This code snippet has already been reworked and optimized. Would you mind to
>  >>> provide me a example with your expectation ? Thanks
>  >>
>  >> Code is optimized yes, but readable no
>  >>
>  >> I would like readability to be improved upon...
>  >>
>  >
>  > gotcha
>  >
> 
> Doest he code snippet below has a better looking for you ?
> 
> 	for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
> 	     max_width > DMA_SLAVE_BUSWIDTH_1_BYTE; max_width >>= 1)
> 		if (((buf_len % max_width) == 0) && (tlen >= max_width))
> 			break;

Am actually not sure :(

Indentation wise it is still a bit messy to follow..

How about:

	for (max_width = DMA_SLAVE_BUSWIDTH_8_BYTES;
			max_width > DMA_SLAVE_BUSWIDTH_1_BYTE;
			max_width >>=1) {
		if (((buf_len % max_width) == 0) && (tlen >= max_width))
			break;
	}

Thanks
-- 
~Vinod

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web