On Fri, Jan 17, 2014 at 11:49:27PM +0100, Arnd Bergmann wrote: > On Friday 10 January 2014, Andy Gross wrote: > > > +static bool bam_dma_filter(struct dma_chan *chan, void *data) > > +{ > > + struct bam_filter_args *args = data; > > + struct bam_chan *bchan = to_bam_chan(chan); > > + > > + if (args->dev == chan->device && > > + args->id == bchan->id) { > > + > > + /* we found the channel, so lets set the EE and dir */ > > + bchan->ee = args->ee; > > + bchan->slave.direction = args->dir ? > > + DMA_DEV_TO_MEM : DMA_MEM_TO_DEV; > > + return true; > > + } > > + > > + return false; > > +} > > A filter function should no longer be needed. > Ok, will remove. > > +static struct dma_chan *bam_dma_xlate(struct of_phandle_args *dma_spec, > > + struct of_dma *of) > > +{ > > + struct bam_filter_args args; > > + dma_cap_mask_t cap; > > + > > + if (dma_spec->args_count != 3) > > + return NULL; > > + > > + args.dev = of->of_dma_data; > > + args.id = dma_spec->args[0]; > > + args.ee = dma_spec->args[1]; > > + args.dir = dma_spec->args[2]; > > + > > + dma_cap_zero(cap); > > + dma_cap_set(DMA_SLAVE, cap); > > + > > + return dma_request_channel(cap, bam_dma_filter, &args); > > +} > > Instead, call dma_get_slave_channel() with the right channel that you already know > here. > Agreed. > > + if (pdev->dev.of_node) { > > + ret = of_dma_controller_register(pdev->dev.of_node, > > + bam_dma_xlate, &bdev->common); > > + > > + if (ret) { > > + dev_err(bdev->dev, "failed to register of_dma\n"); > > + goto err_unregister_dma; > > + } > > + } > > No need to check for pdev->dev.of_node when that is the only mode of probing. > Good point. I'll remove extraneous check. > > + > > +#ifdef CONFIG_OF > > +static const struct of_device_id bam_of_match[] = { > > + { .compatible = "qcom,bam-v1.4.0", }, > > + { .compatible = "qcom,bam-v1.4.1", }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, bam_of_match); > > +#endif > > Also, you can remove the #ifdef here and the of_match_ptr() below. > If this is removed, then I'll have to add the OF dependency in the Kconfig, correct? > > + > > +static struct platform_driver bam_dma_driver = { > > + .probe = bam_dma_probe, > > + .remove = bam_dma_remove, > > + .driver = { > > + .name = "bam-dma-engine", > > + .owner = THIS_MODULE, > > + .of_match_table = of_match_ptr(bam_of_match), > > + }, > > +}; > > + > > +static int __init bam_dma_init(void) > > +{ > > + return platform_driver_register(&bam_dma_driver); > > +} > > + > > +static void __exit bam_dma_exit(void) > > +{ > > + return platform_driver_unregister(&bam_dma_driver); > > +} > > + > > module_platform_driver() > Will fix. > > diff --git a/drivers/dma/qcom_bam_dma.h b/drivers/dma/qcom_bam_dma.h > > new file mode 100644 > > index 0000000..2cb3b5f > > --- /dev/null > > +++ b/drivers/dma/qcom_bam_dma.h > > @@ -0,0 +1,268 @@ > > +#ifndef __QCOM_BAM_DMA_H__ > > +#define __QCOM_BAM_DMA_H__ > > + > > +#include <linux/dmaengine.h> > > +#include "virt-dma.h" > > + > > +enum bam_channel_dir { > > + BAM_PIPE_CONSUMER = 0, /* channel reads from data-fifo or memory */ > > + BAM_PIPE_PRODUCER, /* channel writes to data-fifo or memory */ > > +}; > > Since the header does not serve as an interface, just move all the contents > into the driver directly. > OK. SBoyd made the same comment. I'll go ahead and collapse both down to 1 file. > > +struct bam_desc_hw { > > + u32 addr; /* Buffer physical address */ > > + u16 size; /* Buffer size in bytes */ > > + u16 flags; > > +} __packed; > > Remove __packed here, it only makes the access less efficient but does not change > the layout, which is already packed. Ok. Will fix. -- sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html