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
signature.asc
Description: OpenPGP digital signature
