Hi Steve,
Thank you for the patch. Many of the comments below apply to the ov5642 driver
too, please take them into account when reworking patch 23/24.
On Friday 06 Jan 2017 18:11:40 Steve Longerbeam wrote:
This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_betaYou're missing DT bindings.
branch, modified heavily to bring forward to latest interfaces and code
cleanup.
Signed-off-by: Steve Longerbeam<steve_longerbeam@xxxxxxxxxx>
---
drivers/staging/media/imx/Kconfig | 8 +
drivers/staging/media/imx/Makefile | 2 +
drivers/staging/media/imx/ov5640-mipi.c | 2348 ++++++++++++++++++++++++++++
The driver should go to drivers/media/i2c/ as it should not be specific to the
i.MX6, and you can just call it ov5640.c.
diff --git a/drivers/staging/media/imx/KconfigThe sensor driver is generic, it shouldn't depend on IMX. It should however
b/drivers/staging/media/imx/Kconfig index ce2d2c8..09f373d 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -17,5 +17,13 @@ config VIDEO_IMX_CAMERA
---help---
A video4linux camera capture driver for i.MX5/6.
+config IMX_OV5640_MIPI
+ tristate "OmniVision OV5640 MIPI CSI-2 camera support"
+ depends on GPIOLIB && VIDEO_IMX_CAMERA
depend on at least I2C and OF by the look of it.
+Pet peeve of mine, please sort the headers alphabetically. It makes it easier
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/ctype.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/of_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-of.h>
+#include <media/v4l2-ctrls.h>
to locate duplicated.
+You're mixing lower-case and upper-case hex constants. Let's pick one. Kernel
+#define OV5640_CHIP_ID 0x300A
+#define OV5640_SLAVE_ID 0x3100
+#define OV5640_DEFAULT_SLAVE_ID 0x3c
code usually favours lower-case.
Please define macros for all the other numerical constants you use in the
driver (register addresses and values). The large registers tables can be an
exception if you don't have access to the information, but for registers
written manually, hardcoding numerical values isn't good.
+Please add spaces after /* and before */.
+#define OV5640_MAX_CONTROLS 64
+
+enum ov5640_mode {
+ ov5640_mode_MIN = 0,
+ ov5640_mode_QCIF_176_144 = 0,
+ ov5640_mode_QVGA_320_240,
+ ov5640_mode_VGA_640_480,
+ ov5640_mode_NTSC_720_480,
+ ov5640_mode_PAL_720_576,
+ ov5640_mode_XGA_1024_768,
+ ov5640_mode_720P_1280_720,
+ ov5640_mode_1080P_1920_1080,
+ ov5640_mode_QSXGA_2592_1944,
+ ov5640_num_modes,
+ ov5640_mode_INIT = 0xff, /*only for sensor init*/
Enumerated values should be all upper-case.
+The kernel multi-line comment style is
+/* image size under 1280 * 960 are SUBSAMPLING
+ * image size upper 1280 * 960 are SCALING
+ */
/*
* text
* text
*/
+Store a (const) pointer to the corresponding ov5640_mode_info instead, it will
+struct ov5640_dev {
+ struct i2c_client *i2c_client;
+ struct device *dev;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct v4l2_ctrl_handler ctrl_hdl;
+ struct v4l2_of_endpoint ep; /* the parsed DT endpoint info */
+ struct v4l2_mbus_framefmt fmt;
+ struct v4l2_captureparm streamcap;
+ struct clk *xclk; /* system clock to OV5640 */
+ int xclk_freq; /* requested xclk freq from devicetree */
+
+ enum ov5640_mode current_mode;
simplify the code and allow you to get rid of the ov5640_mode enum.
+Can't these be unsigned int ?
+ int prev_sysclk, prev_hts;
+ int ae_low, ae_high, ae_target;
+No forward declarations please. You should reorder functions as needed (and of
+static void ov5640_power(struct ov5640_dev *sensor, bool enable);
+static void ov5640_reset(struct ov5640_dev *sensor);
+static int ov5640_restore_ctrls(struct ov5640_dev *sensor);
+static int ov5640_set_agc(struct ov5640_dev *sensor, int value);
+static int ov5640_set_exposure(struct ov5640_dev *sensor, int value);
+static int ov5640_get_exposure(struct ov5640_dev *sensor);
+static int ov5640_set_gain(struct ov5640_dev *sensor, int value);
+static int ov5640_get_gain(struct ov5640_dev *sensor);
course still group related functions together).
+static struct reg_value ov5640_init_setting_30fps_VGA[] = {You only use the delay feature of the registers tables twice, once after
+
<snip>
+ {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
+};
writing the first two registers (to select the clock source and perform a
software reset) and once at the very end.
Remove it, write the first two
registers manually in the code with a manual delay afterwards, and add another
manual delay after writing the whole table.
I'm actually wondering whether you couldn't remove the 300ms delay at the end,
the 50/60Hz control register (0x3c00) doesn't look like it needs a delay after
being written.
[snip]
+static struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {Don't turn the stream on in the init sequences, it should only be turned on
+ {0x4202, 0x0f, 0, 0}, /* stream off the sensor */
<snip>
+ {0x4202, 0x00, 0, 0}, /* stream on the sensor */
from the .s_stream() operation.
+};There's very few differences between the 15fps and 30fps tables. It would be
+
+static struct ov5640_mode_info
+ov5640_mode_info_data[ov5640_num_framerates][ov5640_num_modes] = {
better if you could merge them, and manually write the registers that differ.
+No forward declarations please.
+static int ov5640_probe(struct i2c_client *adapter,
+ const struct i2c_device_id *device_id);
+static int ov5640_remove(struct i2c_client *client);
+Wouldn't i2c_transfer() be more efficient here ?
+static int ov5640_read_reg(struct ov5640_dev *sensor, u16 reg, u8 *val)
+{
+ u8 reg_buf[2] = {0};
+ u8 read_val = 0;
+
+ reg_buf[0] = reg >> 8;
+ reg_buf[1] = reg & 0xff;
+
+ if (2 != i2c_master_send(sensor->i2c_client, reg_buf, 2)) {
+ v4l2_err(&sensor->sd, "%s: write reg error: reg=%x\n",
+ __func__, reg);
+ return -EIO;
+ }
+
+ if (1 != i2c_master_recv(sensor->i2c_client, &read_val, 1)) {
+ v4l2_err(&sensor->sd, "%s: read reg error: reg=%x, val=%x\n",
+ __func__, reg, read_val);
+ return -EIO;
+ }
+ *val = read_val;No. No. No no no. Don't ever return from a macro. Hiding the return makes
+ return 0;
+}
+
+#define OV5640_READ_REG(s, r, v) { \
+ ret = ov5640_read_reg((s), (r), (v)); \
+ if (ret) \
+ return ret; \
+ }
following the code flow much more difficult, it's just asking for trouble.
And don't use externally defined variables (ret in this case), that's also
asking for trouble.
+
+static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
+ u8 mask, u8 val)
+{
+ u8 readval;
+ int ret;
+
+ OV5640_READ_REG(sensor, reg, &readval);
+
+ readval &= ~mask;
+ val &= mask;
+ val |= readval;
+
+ OV5640_WRITE_REG(sensor, reg, val);
+ return 0;
+}
If you need to modify registers a lot, switch to regmap for register access.
It will provide you with a cache, removing the need to read registers back
from the device.
+/* download ov5640 settings to sensor through i2c */size is never negative.
+static int ov5640_load_regs(struct ov5640_dev *sensor,
+ struct reg_value *regs,
+ int size)
+{register ? The compiler is nowadays likely smarter than us when it comes to
+ register u32 delay_ms = 0;
+ register u16 reg_addr = 0;
+ register u8 mask = 0;
+ register u8 val = 0;
register allocation.
+Function names and variables are lower case.
+static int ov5640_get_HTS(struct ov5640_dev *sensor)
+{
+ /* read HTS from register settings */
+ u16 HTS;
+Aren't these values unsigned ?
+static int ov5640_set_bandingfilter(struct ov5640_dev *sensor)
+{
+ int prev_vts;
+ int band_step60, max_band60, band_step50, max_band50;
+Aren't these values unsigned ?
+static int ov5640_set_AE_target(struct ov5640_dev *sensor, int target)
+{
+ /* stable in high */
+ int fast_high, fast_low;
+ int ret;Missing spaces around <<
+
+ sensor->ae_low = target * 23 / 25; /* 0.92 */
+ sensor->ae_high = target * 27 / 25; /* 1.08 */
+
+ fast_high = sensor->ae_high<<1;
+The endpoint id isn't meant to select a virtual channel. V4L2 has no virtual
+static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
+{
+ u8 temp, channel = sensor->ep.base.id;
channel API at the moment, you can hardcode the VC to 0 for now.
+ int ret;How about using v4l2_find_nearest_format() ?
+
+ OV5640_READ_REG(sensor, 0x4814, &temp);
+ temp &= ~(3 << 6);
+ temp |= (channel << 6);
+ OV5640_WRITE_REG(sensor, 0x4814, temp);
+
+ return 0;
+}
+
+static enum ov5640_mode
+ov5640_find_nearest_mode(struct ov5640_dev *sensor,
+ int width, int height)
+This will turn streaming on when you set the format, which I don't think is
+ ret = ov5640_set_stream(sensor, true);
correct. You should only turn streaming on in the .s_stream() operation. The
same comment applies to the previous function.
+No need to initialize ret to 0.
+static int ov5640_change_mode(struct ov5640_dev *sensor,
+ enum ov5640_frame_rate frame_rate,
+ enum ov5640_mode mode,
+ enum ov5640_mode orig_mode)
+{
+ enum ov5640_downsize_mode dn_mode, orig_dn_mode;
+ struct reg_value *mode_data = NULL;
+ int mode_size = 0;
+ int ret = 0;
+ sensor->fmt.width = 640;Don't reset the format here. The format must be preserved across subdev
+ sensor->fmt.height = 480;
open/close.
+No need to initialize ret to 0.
+/* restore the last set video mode after chip power-on */
+static int ov5640_restore_mode(struct ov5640_dev *sensor)
+{
+ int ret = 0;
+I wouldn't use ov5640_change_mode() here. You only need to apply the init
+ /* first we need to set some initial register values */
+ ret = ov5640_change_mode(sensor, sensor->current_fr,
+ ov5640_mode_INIT, ov5640_mode_INIT);
register values, just call ov5640_load_regs(). The rest of the
ov5640_change_mode() isn't needed, as you're calling it below. This will also
allow you to get rid of ov5640_mode_INIT, simplifying the logic.
+Maybe you should use the bulk regulator API ?
+static int ov5640_regulators_on(struct ov5640_dev *sensor)
+{
+ int ret;
+
+ if (sensor->io_regulator) {
+ ret = regulator_enable(sensor->io_regulator);
+ if (ret) {
+ v4l2_err(&sensor->sd, "io reg enable failed\n");
+ return ret;
+ }
+ }
+ if (sensor->core_regulator) {
+ ret = regulator_enable(sensor->core_regulator);
+ if (ret) {
+ v4l2_err(&sensor->sd, "core reg enable failed\n");
+ return ret;
+ }
+ }
+ if (sensor->gpo_regulator) {
+ ret = regulator_enable(sensor->gpo_regulator);
+ if (ret) {
+ v4l2_err(&sensor->sd, "gpo reg enable failed\n");
+ return ret;
+ }
+ }
+ if (sensor->analog_regulator) {
+ ret = regulator_enable(sensor->analog_regulator);
+ if (ret) {
+ v4l2_err(&sensor->sd, "analog reg enable failed\n");
+ return ret;
+ }
+ }
+The .s_power() calls need to be ref-counted, similarly to how the regulator
+/* --------------- Subdev Operations --------------- */
+
+static int ov5640_s_power(struct v4l2_subdev *sd, int on)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ int ret;
+
+ v4l2_info(sd, "power %s\n", on ? "ON" : "OFF");
+
+ if (on && !sensor->on) {
enable/disable calls work. See the mt9p031 sensor driver for an example.
+ if (sensor->xclk)Why is this needed ?
+ clk_prepare_enable(sensor->xclk);
+
+ ret = ov5640_regulators_on(sensor);
+ if (ret)
+ return ret;
+
+ ov5640_reset(sensor);
+ ov5640_power(sensor, true);
+
+ ret = ov5640_init_slave_id(sensor);
+No need to initialize ret to 0.
+static int ov5640_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *a)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ struct v4l2_fract *timeperframe = &a->parm.capture.timeperframe;
+ enum ov5640_frame_rate frame_rate;
+ u32 tgt_fps; /* target frames per secound */
+ int ret = 0;
+ else {Don't print an error message that is userspace-triggerable, we have enough
+ v4l2_err(&sensor->sd, "frame rate %u not supported!\n",
+ tgt_fps);
ways for applications to flood the kernel log already :-)
+You need to handle the TRY format here. You can have a look at the mt9p031
+static int ov5640_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+ if (format->pad != 0)
+ return -EINVAL;
+
+ format->format = sensor->fmt;
driver for an example on how to do so.
+You can move this code above the test to avoid the duplicated call below.
+static int ov5640_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *format)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ enum ov5640_mode new_mode;
+ int ret;
+
+ if (format->pad != 0)
+ return -EINVAL;
+
+ if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+ ret = ov5640_try_fmt_internal(sd, &format->format, NULL);
+ if (ret)
+ return ret;
+ cfg->try_fmt = format->format;Please use the v4l2_subdev_get_try_format() function to get the try format,
don't dereference cfg directly.
+According to the kernel coding style, you need curly braces around the else
+static int ov5640_set_hue(struct ov5640_dev *sensor, int value)
+{
+ int ret;
+
+ if (value) {
+ OV5640_MOD_REG(sensor, 0x5580, 1 << 0, 1 << 0);
+ OV5640_WRITE_REG16(sensor, 0x5581, value);
+ } else
+ OV5640_MOD_REG(sensor, 0x5580, 1 << 0, 0);
branch if you use them around the if branch.
+No compiled-out code please.
+#if 0
+static int ov5640_set_green_balance(struct ov5640_dev *sensor, int value)You can create a cluster with the autogain and manual gain controls
+{
+ int ret;
+
+ if (sensor->awb_on)
+ return -EINVAL;
+
+ OV5640_WRITE_REG(sensor, 0x3403, value & 0xff);
+ OV5640_WRITE_REG(sensor, 0x3402, (value & 0xf00) >> 8);
+ return 0;
+}
+#endif
+
<snip>
+
+static int ov5640_set_gain(struct ov5640_dev *sensor, int value)
+{
+ int ret;
+
+ if (sensor->agc_on)
+ return -EINVAL;
(v4l2_ctrl_auto_cluster) to have the control framework disabling the manual
control automatically when autogain is enabled.
+ OV5640_WRITE_REG16(sensor, 0x350a, value & 0x3ff);You can use a control to expose the test pattern function, it's quite useful.
+ return 0;
+}
+
+static int ov5640_get_gain(struct ov5640_dev *sensor)
+{
+ u16 gain;
+ int ret;
+
+ if (sensor->agc_on)
+ return -EINVAL;
+
+ OV5640_READ_REG16(sensor, 0x350a, &gain);
+
+ return gain & 0x3ff;
+}
+
+#if 0
+static int ov5640_set_test_pattern(struct ov5640_dev *sensor, int value)
+{
+ int ret;
+
+ OV5640_MOD_REG(sensor, 0x503d, 0xa4, value ? 0xa4 : 0);
+ return 0;
+}
+#endif
+static struct ov5640_control *ov5640_get_ctrl(int id, int *index)i is never negative, it should be unsigned int.
+{
+ struct ov5640_control *ret = NULL;
+ int i;
+i is never negative, it should be unsigned int.
+static int ov5640_restore_ctrls(struct ov5640_dev *sensor)
+{
+ struct ov5640_control *c;
+ int i;
+You can inline the function call here as it's not used anywhere else.
+static int ov5640_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct ov5640_dev *sensor = ctrl_to_ov5640_dev(ctrl);
+ struct ov5640_control *c;
+ int ret = 0;
+ int i;
+
+ c = ov5640_get_ctrl(ctrl->id, &i);
+I would name this ctrl or control, c can be a bit confusing. You can also
+static int ov5640_init_controls(struct ov5640_dev *sensor)
+{
+ struct ov5640_control *c;
declare it inside the loop.
+ int i;i can never be negative, you can make it an unsigned int.
+I'm not sure this brings much value.
+ v4l2_ctrl_handler_init(&sensor->ctrl_hdl, OV5640_NUM_CONTROLS);
+
+ for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
+ c = &ov5640_ctrls[i];
+
+ v4l2_ctrl_new_std(&sensor->ctrl_hdl, &ov5640_ctrl_ops,
+ c->ctrl.id, c->ctrl.minimum, c-
ctrl.maximum,
+ c->ctrl.step, c->ctrl.default_value);
+ }
+
+ sensor->sd.ctrl_handler = &sensor->ctrl_hdl;
+ if (sensor->ctrl_hdl.error) {
+ int err = sensor->ctrl_hdl.error;
+
+ v4l2_ctrl_handler_free(&sensor->ctrl_hdl);
+
+ v4l2_err(&sensor->sd, "%s: error %d\n", __func__, err);
+You should find an exact mode here, not the nearest one. If with and height
+static int ov5640_enum_frame_interval(
+ struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+ enum ov5640_mode mode;
+
+ if (fie->pad != 0)
+ return -EINVAL;
+ if (fie->index < 0 || fie->index >= ov5640_num_framerates)
+ return -EINVAL;
+
+ if (fie->width == 0 || fie->height == 0)
+ return -EINVAL;
+
+ mode = ov5640_find_nearest_mode(sensor, fie->width, fie->height);
don't match, return -EINVAL. That will replace with above width == 0 and
height == 0 test.
+I'm not sure this is very useful, you can use ftrace if you want to trace
+ dev_dbg(sensor->dev, "%dx%d: [%d] = %d fps\n",
+ fie->width, fie->height, fie->index, fie-
interval.denominator);
function calls.
+ return 0;The g_input_status and s_routing subdev operations are not mandatory, you
+}
+
+static int ov5640_g_input_status(struct v4l2_subdev *sd, u32 *status)
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+ *status = !sensor->on ? V4L2_IN_ST_NO_POWER : 0;
+
+ return 0;
+}
+
+static int ov5640_s_routing(struct v4l2_subdev *sd, u32 input,
+ u32 output, u32 config)
+{
+ return (input != 0) ? -EINVAL : 0;
+}
don't have to implement them as the sensor doesn't have multiple inputs.
+static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)You can use ftrace to trace function calls, there's no need to add debugging
+{
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+ v4l2_info(sd, "stream %s\n", enable ? "ON" : "OFF");
statements that duplicate the functionality.
+All these structures should be static const.
+static struct v4l2_subdev_ops ov5640_subdev_ops = {
+ .core = &ov5640_core_ops,
+ .video = &ov5640_video_ops,
+ .pad = &ov5640_pad_ops,
+};
+How about making the power supplies mandatory in DT instead ? They are
+static void ov5640_power(struct ov5640_dev *sensor, bool enable)
+{
+ gpiod_set_value(sensor->pwdn_gpio, enable ? 0 : 1);
+}
+
+static void ov5640_reset(struct ov5640_dev *sensor)
+{
+ gpiod_set_value(sensor->reset_gpio, 0);
+
+ /* camera power cycle */
+ ov5640_power(sensor, false);
+ usleep_range(5000, 10000);
+ ov5640_power(sensor, true);
+ usleep_range(5000, 10000);
+
+ gpiod_set_value(sensor->reset_gpio, 1);
+ usleep_range(1000, 2000);
+
+ gpiod_set_value(sensor->reset_gpio, 0);
+ usleep_range(5000, 10000);
+}
+
+static void ov5640_get_regulators(struct ov5640_dev *sensor)
+{
+ sensor->io_regulator = devm_regulator_get(sensor->dev, "DOVDD");
+ if (!IS_ERR(sensor->io_regulator)) {
+ regulator_set_voltage(sensor->io_regulator,
+ OV5640_VOLTAGE_DIGITAL_IO,
+ OV5640_VOLTAGE_DIGITAL_IO);
+ } else {
+ dev_dbg(sensor->dev, "%s: no io voltage reg found\n",
+ __func__);
+ sensor->io_regulator = NULL;
mandatory after all, if they're not controllable DT should just declare fixed
supplies.
+i and xclk are never negative, you can make them unsigned int.
+static int ov5640_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct device_node *endpoint;
+ struct ov5640_dev *sensor;
+ int i, xclk, ret;
+Please use sizeof(*variable) instead of sizeof(type).
+ sensor = devm_kzalloc(dev, sizeof(struct ov5640_dev), GFP_KERNEL);
devm_kzalloc() doesn't play nicely with dynamic removal of devices. We're in
the process of fixing related race conditions in the media subsystem. In order
not to make the problem worse, please use kzalloc() instead.
+ if (!sensor)Do you really need to store both i2c_client and dev, given that the latter is
+ return -ENOMEM;
+
+ sensor->i2c_client = client;
+ sensor->dev = dev;
just &client->dev ?
+ sensor->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8;Please fix the indentation.
+ sensor->fmt.width = 640;
+ sensor->fmt.height = 480;
+ sensor->fmt.field = V4L2_FIELD_NONE;
+ sensor->streamcap.capability = V4L2_MODE_HIGHQUALITY |
+ V4L2_CAP_TIMEPERFRAME;
+You're leaking endpoint here. You should move the of_node_put() call right
+ v4l2_of_parse_endpoint(endpoint, &sensor->ep);
+ if (sensor->ep.bus_type != V4L2_MBUS_CSI2) {
+ dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
+ return -EINVAL;
after the v4l2_of_parse_endpoint() call.
+Instead of adding a custom DT property for this, use assigned-clock-rates. You
+ /* get system clock (xclk) frequency */
+ ret = of_property_read_u32(dev->of_node, "xclk", &xclk);
won't need to set it manually in the driver, and can verify its frequency with
clk_get_rate().
+ if (!ret) {Are your register tables above independent of the clock frequency ? You should
+ if (xclk < OV5640_XCLK_MIN || xclk > OV5640_XCLK_MAX) {
ideally compute register values at runtime instead of hardcoding them, but
given the lack of information from Omnivision I understand this isn't
possible. You thus need to be stricter here and reject any value other than
the nominal frequency.
+Please don't. The clock should be mandatory.
+ /* get system clock (xclk) */
+ sensor->xclk = devm_clk_get(dev, "xclk");
+ if (!IS_ERR(sensor->xclk)) {
+ if (!sensor->xclk_freq) {
+ dev_err(dev, "xclk requires xclk frequency!\n");
+ return -EINVAL;
+ }
+ clk_set_rate(sensor->xclk, sensor->xclk_freq);
+ } else {
+ /* assume system clock enabled by default */
+ sensor->xclk = NULL;
+ }Are the GPIOs mandatory or optional ? Can a system tie some of them to ground
+
+ /* request power down pin */
+ sensor->pwdn_gpio = devm_gpiod_get(dev, "pwdn", GPIOD_OUT_HIGH);
or a voltage rail, or do they need to always be manually controllable ?
+Writing the controls here is pointless, as powering the chip down will lose
+ ret = ov5640_s_power(&sensor->sd, 1);
+ if (ret)
+ goto entity_cleanup;
+ ret = ov5640_init_controls(sensor);
+ if (ret)
+ goto power_off;
+
+ ret = ov5640_s_power(&sensor->sd, 0);
+ if (ret)
+ goto free_ctrls;
all the values. You shouldn't call v4l2_ctrl_handler_setup() in
ov5640_init_controls(), and you can then remove the ov5640_s_power() calls
here.
+Won't ov5640_s_power(0) already disable the regulators ?
+free_ctrls:
+ v4l2_ctrl_handler_free(&sensor->ctrl_hdl);
+power_off:
+ ov5640_s_power(&sensor->sd, 0);
+entity_cleanup:
+ media_entity_cleanup(&sensor->sd.entity);
+ ov5640_regulators_off(sensor);
+ return ret;That's not the kerneldoc comment style. Given that this is the only documented
+}
+
+/*!
+ * ov5640 I2C detach function
+ *
+ * @param client struct i2c_client *
+ * @return Error code indicating success or failure
+ */
function, and that the comment is completely useless, you can just drop it.
+static int ov5640_remove(struct i2c_client *client)This function is called by v4l2_async_unregister_subdev(), there's no need to
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov5640_dev *sensor = to_ov5640_dev(sd);
+
+ ov5640_regulators_off(sensor);
+
+ v4l2_async_unregister_subdev(&sensor->sd);
+ media_entity_cleanup(&sensor->sd.entity);
+ v4l2_device_unregister_subdev(sd);
duplicate it.
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");MODULE_AUTHOR isn't a synonym for copyright ownership. I don't think you
should add Freescale as an author. If you know who wrote the original code you
can list that developer explicitly.
+MODULE_AUTHOR("Steve Longerbeam<steve_longerbeam@xxxxxxxxxx>");Version numbers are never updated. I wouldn't bother adding one.
+MODULE_DESCRIPTION("OV5640 MIPI Camera Subdev Driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");