On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
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?
That's true, I'll fix those.
+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"?
sure, will change it.
+ * 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.
okay.
+/* + * 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?
VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise VPDMA doesn't work. wmb() ensures the ordering.
+ 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.
Okay. I think I don't understand the interruptible version correctly. We don't need to msleep_interruptible here, we aren't waiting on any wake up event, we just want to wait till a bit gets set.
I am thinking of implementing something similar to wait_for_bit_change() in 'drivers/video/omap2/dss/dsi.c'
Archit -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
