Hi,

On 02/08/13 17:03, Archit Taneja wrote:

> +struct vpdma_data_format vpdma_yuv_fmts[] = {
> +	[VPDMA_DATA_FMT_Y444] = {
> +		.data_type	= DATA_TYPE_Y444,
> +		.depth		= 8,
> +	},

This, and all the other tables, should probably be consts?

> +static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
> +{
> +	u32 val = *valp;
> +
> +	val &= ~(mask << shift);
> +	val |= (field & mask) << shift;
> +	*valp = val;
> +}

I think "insert" normally means, well, inserting a thing in between
something. What you do here is overwriting.

Why not just call it "write_field"?

> + * Allocate a DMA buffer
> + */
> +int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
> +{
> +	buf->size = size;
> +	buf->mapped = 0;

Maybe true/false is clearer here that 0/1.

> +/*
> + * submit a list of DMA descriptors to the VPE VPDMA, do not wait for completion
> + */
> +int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list *list)
> +{
> +	/* we always use the first list */
> +	int list_num = 0;
> +	int list_size;
> +
> +	if (vpdma_list_busy(vpdma, list_num))
> +		return -EBUSY;
> +
> +	/* 16-byte granularity */
> +	list_size = (list->next - list->buf.addr) >> 4;
> +
> +	write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list->buf.dma_addr);
> +	wmb();

What is the wmb() for?

> +	write_reg(vpdma, VPDMA_LIST_ATTR,
> +			(list_num << VPDMA_LIST_NUM_SHFT) |
> +			(list->type << VPDMA_LIST_TYPE_SHFT) |
> +			list_size);
> +
> +	return 0;
> +}

> +static void vpdma_firmware_cb(const struct firmware *f, void *context)
> +{
> +	struct vpdma_data *vpdma = context;
> +	struct vpdma_buf fw_dma_buf;
> +	int i, r;
> +
> +	dev_dbg(&vpdma->pdev->dev, "firmware callback\n");
> +
> +	if (!f || !f->data) {
> +		dev_err(&vpdma->pdev->dev, "couldn't get firmware\n");
> +		return;
> +	}
> +
> +	/* already initialized */
> +	if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
> +			VPDMA_LIST_RDY_SHFT)) {
> +		vpdma->ready = true;
> +		return;
> +	}
> +
> +	r = vpdma_buf_alloc(&fw_dma_buf, f->size);
> +	if (r) {
> +		dev_err(&vpdma->pdev->dev,
> +			"failed to allocate dma buffer for firmware\n");
> +		goto rel_fw;
> +	}
> +
> +	memcpy(fw_dma_buf.addr, f->data, f->size);
> +
> +	vpdma_buf_map(vpdma, &fw_dma_buf);
> +
> +	write_reg(vpdma, VPDMA_LIST_ADDR, (u32) fw_dma_buf.dma_addr);
> +
> +	for (i = 0; i < 100; i++) {		/* max 1 second */
> +		msleep_interruptible(10);

You call interruptible version here, but you don't handle the
interrupted case. I believe the loop will just continue looping, even if
the user interrupted.

> +		if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
> +				VPDMA_LIST_RDY_SHFT))
> +			break;
> +	}
> +
> +	if (i == 100) {
> +		dev_err(&vpdma->pdev->dev, "firmware upload failed\n");
> +		goto free_buf;
> +	}
> +
> +	vpdma->ready = true;
> +
> +free_buf:
> +	vpdma_buf_unmap(vpdma, &fw_dma_buf);
> +
> +	vpdma_buf_free(&fw_dma_buf);
> +rel_fw:
> +	release_firmware(f);
> +}

 Tomi