Dear Guennadi,
On 10/19/2015 10:03 AM, Guennadi Liakhovetski wrote:
Hi Josh,
On Wed, 14 Oct 2015, Josh Wu wrote:
Dear Guennadi,
Thanks for the review.
On 10/5/2015 1:02 AM, Guennadi Liakhovetski wrote:
On Tue, 22 Sep 2015, Josh Wu wrote:
This patch enable Atmel ISI preview path to convert the YUV to RGB format.
Signed-off-by: Josh Wu <josh.wu@xxxxxxxxx>
---
drivers/media/platform/soc_camera/atmel-isi.c | 38
++++++++++++++++++++-------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index e87d354..e33a16a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device
*icd,
case V4L2_PIX_FMT_UYVY:
case V4L2_PIX_FMT_YVYU:
case V4L2_PIX_FMT_VYUY:
+ /* RGB */
+ case V4L2_PIX_FMT_RGB565:
return true;
- /* RGB, TODO */
default:
return false;
}
}
+static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
+{
+ return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
+ host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
+}
+
Why not just pass fourcc to this function? Or maybe just embed it in
start_streaming - it won't clutter it a lot.
I think pass fourcc to the function is good.
Since configure_geometry() is hardware related, and the enable_preview_path is
also hardware related, so I prefer initialize enable_preview_path in
configure_geometry().
But you don't, you do it in start_streaming() below.
Right, then I'll move it to configure_geometry() in v2..
But actually my
comment was not about _where_ to do it, but whether this calculation is
worth a separate function. I would just perform this calculation directly
where you're calling it:
static ... start_streaming(...)
{
...
u32 fourcc = icd->current_fmt->host_fmt->fourcc;
isi->enable_preview_path = fourcc == V4L2_PIX_FMT_RGB565 ||
fourcc == V4L2_PIX_FMT_RGB32;
I thought this "function" might be called in other place, but actually
no one call it.
So yes, I think there is no need to have such separated function. I'll
fix it in v2. Thanks.
Best Regards,
Josh Wu
Thanks
Guennadi
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
{
if (isi->active) {
@@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq,
unsigned int count)
struct atmel_isi *isi = ici->priv;
int ret;
+ isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
+
pm_runtime_get_sync(ici->v4l2_dev.dev);
/* Reset ISI */
@@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt
isi_camera_formats[] = {
.order = SOC_MBUS_ORDER_LE,
.layout = SOC_MBUS_LAYOUT_PACKED,
},
+ {
+ .fourcc = V4L2_PIX_FMT_RGB565,
+ .name = "RGB565",
+ .bits_per_sample = 8,
+ .packing = SOC_MBUS_PACKING_2X8_PADHI,
+ .order = SOC_MBUS_ORDER_LE,
+ .layout = SOC_MBUS_LAYOUT_PACKED,
+ },
};
/* This will be corrected as we get more formats */
@@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct
soc_camera_device *icd,
struct soc_camera_format_xlate *xlate)
{
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
- int formats = 0, ret;
+ int formats = 0, ret, i, n;
/* sensor format */
struct v4l2_subdev_mbus_code_enum code = {
.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct
soc_camera_device *icd,
case MEDIA_BUS_FMT_VYUY8_2X8:
case MEDIA_BUS_FMT_YUYV8_2X8:
case MEDIA_BUS_FMT_YVYU8_2X8:
- formats++;
- if (xlate) {
- xlate->host_fmt = &isi_camera_formats[0];
- xlate->code = code.code;
- xlate++;
- dev_dbg(icd->parent, "Providing format %s using code
%d\n",
- isi_camera_formats[0].name, code.code);
+ n = ARRAY_SIZE(isi_camera_formats);
+ formats += n;
+ for (i = 0; i < n; i++) {
+ if (xlate) {
I'd put if outside of the loop, or just do
+ for (i = 0; xlate && i < n; i++) {
yes, that simpler one. I'll take it. Thanks.
Best Regards,
Josh Wu
+ xlate->host_fmt = &isi_camera_formats[i];
+ xlate->code = code.code;
+ dev_dbg(icd->parent, "Providing format %s
using code %d\n",
+ isi_camera_formats[0].name,
code.code);
+ xlate++;
+ }
}
break;
default:
--
1.9.1
--
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
