Vaibhav,
>[Murali] Shouldn't we remove omap_vout_uservirt_to_phys() and use
>videobuf_iolock() instead as we have done in vpfe_capture.c?
>
>As mentioned before, in my opinion we can address this in sub-sequent patch
>series, and should not block this patch in getting to main-line.
>
>> +/*
>> + * Convert V4L2 pixel format to DSS pixel format
>> + */
>> +static enum omap_color_mode video_mode_to_dss_mode(struct
>omap_vout_device
>> + *vout)
>> +{
>> + struct omap_overlay *ovl;
>> + struct omapvideo_info *ovid;
>> + struct v4l2_pix_format *pix = &vout->pix;
>> +
>> + ovid = &vout->vid_info;
>> + ovl = ovid->overlays[0];
>> +
>> + switch (pix->pixelformat) {
>> + case 0:
>> + break;
>> + case V4L2_PIX_FMT_YUYV:
>> + return OMAP_DSS_COLOR_YUV2;
>> +
>> + case V4L2_PIX_FMT_UYVY:
>> + return OMAP_DSS_COLOR_UYVY;
>> +
>> + case V4L2_PIX_FMT_RGB565:
>> + return OMAP_DSS_COLOR_RGB16;
>> +
>> + case V4L2_PIX_FMT_RGB24:
>> + return OMAP_DSS_COLOR_RGB24P;
>> +
>> + case V4L2_PIX_FMT_RGB32:
>> + return (ovl->id == OMAP_DSS_VIDEO1) ?
>> + OMAP_DSS_COLOR_RGB24U : OMAP_DSS_COLOR_ARGB32;
>> + case V4L2_PIX_FMT_BGR32:
>> + return OMAP_DSS_COLOR_RGBX32;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> + return -EINVAL;
>
>[Murali] Also return type is enum and you are returning a negative number
>here ???
>
>I think yes it is acceptable, since ultimately enum is of type integer
>constant. You can return the value which fits into this size.
>
[MK] I did some research into this and this code can behave differently
with different compilers. So if compiler treats the enum as int, it would
work fine, but not otherwise. IMO, we shouldn't write code that are dependant on the tool chain (rather write portable code). So suggest you
change the return type to int instead of enum. Doesn't this code give
you a warning?
>
>> I have also asked Hans to provide
>> his comments since this is a new driver that he might have to approve.
>Did
>> he review the code in the past?
>>
>[Hiremath, Vaibhav] Yes he reviewed it some time back; anyway he has
>already provided few more comments now which I have already fixed. The
>patch is following this reply.
>
>Thanks,
>Vaibhav
>
>> -Murali
>> >
><snip>
--
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
