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

[Vinod Koul <vinod.koul@intel.com>]

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