Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library

[Archit Taneja <archit@ti.com>]

On Friday 09 August 2013 03:05 AM, Laurent Pinchart wrote:
> 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().

I wasn't aware of that. I'll remove the wmb() call. Thanks for sharing 
this info.

Archit