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.��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
