Re: [RFC PATCH 1/8] s5p-fimc: Add Exynos4x12 FIMC-IS driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On 03/12/2013 03:27 PM, Hans Verkuil wrote:
On Mon 11 March 2013 20:44:45 Sylwester Nawrocki wrote:
[...]
+
+/* Supported manual ISO values */
+static const s64 iso_qmenu[] = {
+	50, 100, 200, 400, 800,
+};
+
+static int __ctrl_set_iso(struct fimc_is *is, int value)
+{
+	unsigned int idx, iso;
+
+	if (value == V4L2_ISO_SENSITIVITY_AUTO) {
+		__is_set_isp_iso(is, ISP_ISO_COMMAND_AUTO, 0);
+		return 0;
+	}
+	idx = is->isp.ctrls.iso->val;
+	if (idx>= ARRAY_SIZE(iso_qmenu))
+		return -EINVAL;
+
+	iso = iso_qmenu[idx];
+	__is_set_isp_iso(is, ISP_ISO_COMMAND_MANUAL, iso);
+	return 0;
+}
[...]
+int fimc_isp_subdev_create(struct fimc_isp *isp)
+{
+	const struct v4l2_ctrl_ops *ops =&fimc_isp_ctrl_ops;
+	struct v4l2_ctrl_handler *handler =&isp->ctrls.handler;
+	struct v4l2_subdev *sd =&isp->subdev;
+	struct fimc_isp_ctrls *ctrls =&isp->ctrls;
+	int ret;
+
+	mutex_init(&isp->subdev_lock);
+
+	v4l2_subdev_init(sd,&fimc_is_subdev_ops);
+	sd->grp_id = GRP_ID_FIMC_IS;
+	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	snprintf(sd->name, sizeof(sd->name), "FIMC-IS-ISP");
+
+	isp->subdev_pads[FIMC_IS_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	isp->subdev_pads[FIMC_IS_SD_PAD_SRC_FIFO].flags = MEDIA_PAD_FL_SOURCE;
+	isp->subdev_pads[FIMC_IS_SD_PAD_SRC_DMA].flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&sd->entity, FIMC_IS_SD_PADS_NUM,
+				isp->subdev_pads, 0);
+	if (ret)
+		return ret;
+
+	v4l2_ctrl_handler_init(handler, 20);
+
+	ctrls->saturation = v4l2_ctrl_new_std(handler, ops, V4L2_CID_SATURATION,
+						-2, 2, 1, 0);
+	ctrls->brightness = v4l2_ctrl_new_std(handler, ops, V4L2_CID_BRIGHTNESS,
+						-4, 4, 1, 0);
+	ctrls->contrast = v4l2_ctrl_new_std(handler, ops, V4L2_CID_CONTRAST,
+						-2, 2, 1, 0);
+	ctrls->sharpness = v4l2_ctrl_new_std(handler, ops, V4L2_CID_SHARPNESS,
+						-2, 2, 1, 0);
+	ctrls->hue = v4l2_ctrl_new_std(handler, ops, V4L2_CID_HUE,
+						-2, 2, 1, 0);
+
+	ctrls->auto_wb = v4l2_ctrl_new_std_menu(handler, ops,
+					V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE,
+					8, ~0x14e, V4L2_WHITE_BALANCE_AUTO);
+
+	ctrls->exposure = v4l2_ctrl_new_std(handler, ops,
+					V4L2_CID_EXPOSURE_ABSOLUTE,
+					-4, 4, 1, 0);
+
+	ctrls->exp_metering = v4l2_ctrl_new_std_menu(handler, ops,
+					V4L2_CID_EXPOSURE_METERING, 3,
+					~0xf, V4L2_EXPOSURE_METERING_AVERAGE);
+
+	v4l2_ctrl_new_std_menu(handler, ops, V4L2_CID_POWER_LINE_FREQUENCY,
+					V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
+					V4L2_CID_POWER_LINE_FREQUENCY_AUTO);
+	/* ISO sensitivity */
+	ctrls->auto_iso = v4l2_ctrl_new_std_menu(handler, ops,
+			V4L2_CID_ISO_SENSITIVITY_AUTO, 1, 0,
+			V4L2_ISO_SENSITIVITY_AUTO);
+
+	ctrls->iso = v4l2_ctrl_new_int_menu(handler, ops,
+			V4L2_CID_ISO_SENSITIVITY, ARRAY_SIZE(iso_qmenu) - 1,
+			ARRAY_SIZE(iso_qmenu)/2 - 1, iso_qmenu);
+
+	ctrls->aewb_lock = v4l2_ctrl_new_std(handler, ops,
+					V4L2_CID_3A_LOCK, 0, 0x3, 0, 0);
+
+	/* FIXME: Adjust the enabled controls mask according
+	   to the ISP capabilities */
+	v4l2_ctrl_new_std_menu(handler, ops, V4L2_CID_COLORFX,
+					V4L2_COLORFX_ANTIQUE,
+					0, V4L2_COLORFX_NONE);
+	if (handler->error) {
+		media_entity_cleanup(&sd->entity);
+		return handler->error;
+	}
+
+	ctrls->auto_iso->flags |= V4L2_CTRL_FLAG_VOLATILE |
+				  V4L2_CTRL_FLAG_UPDATE;

Why would auto_iso be volatile? I would expect the iso to be volatile
(in which case the 'false' argument below would be 'true'). Also,
v4l2_ctrl_auto_cluster already sets the UPDATE flag.

Thanks for spotting this. I should have removed this flags set up
since the g_volatile_ctrl op is not currently supported and as far
as I know the firmware doesn't support reading actual ISO value in
auto mode. I'll need to check if there are any commands available
for that.

Anyway auto_iso is not supposed to have the flags set up like this
and that also tells me that I need to inspect my other driver where
this code originally came from. :)

+	v4l2_ctrl_auto_cluster(2,&ctrls->auto_iso, 0, false);
+
+	sd->ctrl_handler = handler;
+	sd->internal_ops =&fimc_is_subdev_internal_ops;
+	sd->entity.ops =&fimc_is_subdev_media_ops;
+	v4l2_set_subdevdata(sd, isp);
+
+	return 0;
+}

diff --git a/drivers/media/platform/s5p-fimc/fimc-isp.h b/drivers/media/platform/s5p-fimc/fimc-isp.h
new file mode 100644
index 0000000..654039e
--- /dev/null
+++ b/drivers/media/platform/s5p-fimc/fimc-isp.h
@@ -0,0 +1,205 @@
[...]
+struct fimc_isp_ctrls {
+	struct v4l2_ctrl_handler handler;
+	/* Internal mode selection */
+	struct v4l2_ctrl *scenario;
+	/* Frame rate */
+	struct v4l2_ctrl *fps;
+	/* Touch AF position */
+	struct v4l2_ctrl *af_position_x;
+	struct v4l2_ctrl *af_position_y;
+	/* Auto white balance */
+	struct v4l2_ctrl *auto_wb;
+	/* ISO sensitivity */
+	struct v4l2_ctrl *auto_iso;
+	struct v4l2_ctrl *iso;

I suggest putting this in an anonymous struct:

	struct { /* Auto ISO control cluster */
		struct v4l2_ctrl *auto_iso;
		struct v4l2_ctrl *iso;
	};

That way you visually emphasize that these belong together and that you
shouldn't move them around.

Agreed. I'll make them grouped in separate structs wherever a cluster
is used.

+	struct v4l2_ctrl *contrast;
+	struct v4l2_ctrl *saturation;
+	struct v4l2_ctrl *sharpness;
+	/* Auto/manual exposure */
+	struct v4l2_ctrl *auto_exp;
+	/* Manual exposure value */
+	struct v4l2_ctrl *exposure;
+	/* Adjust - brightness */
+	struct v4l2_ctrl *brightness;
+	/* Adjust - hue */
+	struct v4l2_ctrl *hue;
+	/* Exposure metering mode */
+	struct v4l2_ctrl *exp_metering;
+	/* AFC */
+	struct v4l2_ctrl *afc;
+	/* AE/AWB lock/unlock */
+	struct v4l2_ctrl *aewb_lock;
+	/* AF */
+	struct v4l2_ctrl *focus_mode;
+	/* AF status */
+	struct v4l2_ctrl *af_status;
+};
[...]

Otherwise this patch looks very clean and I really have no other comments.

Thanks a lot for a prompt review!

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux