Hi,
On 07/25/2015 06:21 AM, Stephen Boyd wrote:
On 07/21/2015 03:34 AM, Archit Taneja wrote:
+ int size)
+{
+ struct desc_info *desc;
+ struct dma_async_tx_descriptor *dma_desc;
+ struct scatterlist *sgl;
+ int r;
+
+ desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ list_add_tail(&desc->list, &this->list);
+
+ sgl = &desc->sgl;
+
+ sg_init_one(sgl, vaddr, size);
+
+ desc->dir = DMA_MEM_TO_DEV;
+
+ r = dma_map_sg(this->dev, sgl, 1, desc->dir);
+ if (r == 0)
+ goto err;
Should we return an error in this case? Looks like return 0.
dma_map_sg returns the number of sg entries successfully mapped. In
this case, it should be 1.
+
+ this->slave_conf.device_fc = 0;
+ this->slave_conf.dst_addr = this->res->start + reg_off;
+ this->slave_conf.dst_maxburst = 16;
Is there any reason why slave_conf can't be on the stack? Otherwise it's
odd that it's overwritten a few times before we submit the descriptors,
so it must be copied by the dmaengine provider, but that isn't clear at
all from the code. If it isn't copied, perhaps it should be part of the
desc_info structure. If it is copied I wonder why it isn't const in the
function signature.
The dmaengine drivers either memcpy slave_config in their
device_config() dmaengine op, or populate their local members reading
params in the passed slave_config.
I'll move slave_conf to stack. As you said, the config argument
in dmaengine_slave_config should ideally use const.
+
+ r = dmaengine_slave_config(this->chan, &this->slave_conf);
+ if (r) {
+ dev_err(this->dev, "failed to configure dma channel\n");
+ goto err;
+ }
+
+ dma_desc = dmaengine_prep_slave_sg(this->chan, sgl, 1, desc->dir, 0);
+ if (!dma_desc) {
+ dev_err(this->dev, "failed to prepare data write desc\n");
+ r = PTR_ERR(dma_desc);
+ goto err;
+ }
+
+ desc->dma_desc = dma_desc;
+
+ return 0;
+err:
+ kfree(desc);
+
+ return r;
+}
+
+/*
+ * helper to prepare dma descriptors to configure registers needed for reading a
+ * codeword/step in a page
+ */
+static void config_cw_read(struct qcom_nandc_data *this)
+{
+ struct nandc_regs *regs = this->regs;
+
+ write_reg_dma(this, NAND_FLASH_CMD, ®s->cmd, 3, true);
Maybe it would be better to have a case statement inside
{write,read}_reg_dma() that looked at the second argument and matched it
up with an offset in regs. Then this could be
write_reg_dma(this, NAND_FLASH_CMD, 3, true);
That's a good idea. However, we have at least one programming seqeunce (in nandc_param) where we need to write two different values to the same register. In such a case, we need two different locations to store the two values.
I can split up the programming sequence into two parts such that we won't write to the same register twice. But doing this for the sake of reducing an argument to write_reg_dma seems a bit unnecessary.
Are we guaranteed that this is called within the same context as where
the buffer is passed to this function? Otherwise this stack check isn't
going to work because object_is_on_stack() will silently fail.
These are funcs are finally tied to mtd ops. I think in normal operation
it'll be the same context. I'll still cross check. The aim of the check
is to prevent a memcpy of the buffer to/from the layer above. A false
negative will result in a slower read/write operation.