On 01/21/2013 10:34 AM, Hans Verkuil wrote:
On Sat January 19 2013 22:27:22 Sylwester Nawrocki wrote:
This patch adds V4L2 sub-device driver for OV9650/OV9652 image sensors.
The driver exposes following V4L2 controls:
- auto/manual exposure,
- auto/manual white balance,
- auto/manual gain,
- brightness, saturation, sharpness,
- horizontal/vertical flip,
- color bar test pattern,
- banding filter (power line frequency).
Frame rate can be configured with g/s_frame_interval pad level ops.
Supported resolution are only: SXGA, VGA, QVGA.
Signed-off-by: Sylwester Nawrocki<sylvester.nawrocki@xxxxxxxxx>
Some small comments:
<snip>
+
+static int ov965x_log_status(struct v4l2_subdev *sd)
+{
+ v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
+ return 0;
+}
A short helper function (v4l2_ctrl_subdev_log_status) would simplify this
as that can be used as a core op directly.
+
<snip>
+
+static int ov965x_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub)
+{
+ return v4l2_ctrl_subscribe_event(fh, sub);
+}
+
+static int ov965x_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub)
+{
+ return v4l2_event_unsubscribe(fh, sub);
+}
I suggest that two helper functions are added (v4l2_ctrl_subdev_subscribe_event
and v4l2_event_subdev_unsubscribe) that can be used as a core op directly.
I had a feeling such helpers are indeed missing. I guess I just needed some
incentive to add them myself ;D
diff --git a/include/media/ov9650.h b/include/media/ov9650.h
new file mode 100644
index 0000000..2fab780
--- /dev/null
+++ b/include/media/ov9650.h
@@ -0,0 +1,27 @@
+/*
+ * OV9650/OV9652 camera sensors driver
+ *
+ * Copyright (C) 2013 Sylwester Nawrocki<sylvester.nawrocki@xxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef OV9650_H_
+#define OV9650_H_
+
+/**
+ * struct ov9650_platform_data - ov9650 driver platform data
+ * @mclk_frequency: the sensor's master clock frequency
What's the unit? Hz?
Oh, too bad, didn't mention the unit. It is supposed to be in Hz, yes.
I'll fix it.
+ * @gpio_pwdn: number of a GPIO connected to OV965X PWDN pin
+ * @gpio_reset: number of a GPIO connected to OV965X RESET pin
+ *
+ * If any of @gpio_pwdn or @gpio_reset are unused then should be
s/then should/then they should/
+ * set to negative value. @mclk_frequency must always be specified.
s/set to/set to a/
Amended. Thank you for the review!
--
Regards,
Sylwester
--
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
