Hi Archit,

On Monday 05 August 2013 16:56:46 Archit Taneja wrote:
> On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
> > 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() calls iowrite32(), which already includes an __iowmb().

> >> +  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'

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to