Hi Guennadi,
thank you for your review.
>> u32 frame_count;
>> + struct vb2_alloc_ctx *alloc_ctx;
>> +};
>> +
>> +enum mx2_buffer_state {
>> + MX2_STATE_NEEDS_INIT = 0,
>> + MX2_STATE_PREPARED = 1,
>> + MX2_STATE_QUEUED = 2,
>> + MX2_STATE_ACTIVE = 3,
>> + MX2_STATE_DONE = 4,
>> + MX2_STATE_ERROR = 5,
>> + MX2_STATE_IDLE = 6,
>
> Are the numerical values important? If not - please, drop. And actually,
> you don't need most of these states, I wouldn't be surprised, if you
> didn't need them at all. You might want to revise them in a future patch.
Yes, you are right, I might have been too cautious here. I will make
mx27 not to depend on these states and will try to reduce them.
However, those ones used by mx25 I can't eliminate since I don't have
the possibility to test it.
> [snip]
>
>> @@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
>> /*
>> * Videobuf operations
>> */
>> -static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
>> - unsigned int *size)
>> +static int mx2_videobuf_setup(struct vb2_queue *vq,
>> + const struct v4l2_format *fmt,
>> + unsigned int *count, unsigned int *num_planes,
>> + unsigned int sizes[], void *alloc_ctxs[])
>> {
>> - struct soc_camera_device *icd = vq->priv_data;
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> + struct mx2_camera_dev *pcdev = ici->priv;
>> int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>> icd->current_fmt->host_fmt);
>
> You choose not to support VIDIOC_CREATE_BUFS? You have to at least return
> an error if fmt != NULL. Or consider supporting it - look at mx3_camera.c
> or sh_mobile_ceu_camera.c (btw, atmel-isi.c has to be fixed with this
> respect too). If you decide to support it, you'll also have to drop
> .buf_prepare() (see, e.g., 07f92448045a23d27dbc3ece3abcb6bafc618d43)
I'm a bit tight on schedule and would prefer just returning NULL by
now and add this in a further patch if you are so kind.
> [snip]
>
>> @@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue *vq,
>> * This can be useful if you want to see if we actually fill
>> * the buffer with something
>> */
>> - memset((void *)vb->baddr, 0xaa, vb->bsize);
>> + memset((void *)vb2_plane_vaddr(vb, 0),
>> + 0xaa, vb2_get_plane_payload(vb, 0));
>> #endif
>>
>> - if (buf->code != icd->current_fmt->code ||
>> - vb->width != icd->user_width ||
>> - vb->height != icd->user_height ||
>> - vb->field != field) {
>> + if (buf->code != icd->current_fmt->code) {
>> buf->code = icd->current_fmt->code;
>> - vb->width = icd->user_width;
>> - vb->height = icd->user_height;
>> - vb->field = field;
>> - vb->state = VIDEOBUF_NEEDS_INIT;
>> + buf->state = MX2_STATE_NEEDS_INIT;
>
> This looks broken or most likely redundant to me. The check for a changed
> code was there to reallocate the buffer, doesn't seem to make much sense
> now.
Yes, this will definitely go away and will take MX2_STATE_NEEDS_INIT with it.
> [snip]
>
>> @@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
>> * types.
>> */
>> spin_lock_irqsave(&pcdev->lock, flags);
>> - if (vb->state == VIDEOBUF_QUEUED) {
>> - list_del(&vb->queue);
>> - vb->state = VIDEOBUF_ERROR;
>> - } else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
>> + if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
>> + list_del_init(&buf->queue);
>> + buf->state = MX2_STATE_NEEDS_INIT;
>> + } else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>
> This doesn't look right. You already have " || buf->state ==
> MX2_STATE_ACTIVE" above, so, this latter condition is never entered?
Yeah, I'm probably breaking m25 support here. This will be fixed in
the following version of my patches.
Regards.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
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
