Hi Todor, On 05/12/2016 05:59 PM, Todor Tomov wrote: > The ov5645 sensor from Omnivision supports up to 2592x1944 > and CSI2 interface. > > The driver adds support for the following modes: > - 1280x960 > - 1920x1080 > - 2592x1944 > > Output format is packed 8bit UYVY. > > Signed-off-by: Todor Tomov <todor.tomov@xxxxxxxxxx> > --- > .../devicetree/bindings/media/i2c/ov5645.txt | 54 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov5645.c | 1344 ++++++++++++++++++++ > 4 files changed, 1410 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > create mode 100644 drivers/media/i2c/ov5645.c > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5645.txt b/Documentation/devicetree/bindings/media/i2c/ov5645.txt > new file mode 100644 > index 0000000..1d65fa8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ov5645.txt The binding doc should be a separate patch. Also you should add devicetree ML in cc. > @@ -0,0 +1,54 @@ > +* Omnivision 1/4-Inch 5Mp CMOS Digital Image Sensor > + > +The Omnivision OV5645 is a 1/4-Inch CMOS active pixel digital image sensor with > +an active array size of 2592H x 1944V. It is programmable through a serial SCCB > +interface. > + > +Required Properties: > +- compatible: value should be "ovti,ov5645" > +- clocks: reference to the xclk clock > +- clock-names: should be "xclk" > +- clock-rates: the xclk clock frequency > + > +Optional Properties: > +- reset-gpio: Chip reset GPIO > +- pwdn-gpio: Chip power down GPIO > +- DOVDD-supply: Chip IO regulator > +- DVDD-supply: Chip core regulator > +- AVDD-supply: Chip analog regulator I'd use lower-case for the -supply properties <cut> > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > new file mode 100644 > index 0000000..bae07e4 > --- /dev/null > +++ b/drivers/media/i2c/ov5645.c > @@ -0,0 +1,1344 @@ > +/* > + * Driver for the OV5645 camera sensor. > + * > + * Copyright (c) 2011-2015, The Linux Foundation. All rights reserved. > + * Copyright (C) 2015 By Tech Design S.L. All Rights Reserved. > + * Copyright (C) 2012-2013 Freescale Semiconductor, Inc. All Rights Reserved. > + * > + * Based on: > + * - the OV5645 driver from QC msm-3.10 kernel on codeaurora.org: > + * https://us.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/ > + * media/platform/msm/camera_v2/sensor/ov5645.c?h=LA.BR.1.2.4_rb1.41 > + * - the OV5640 driver posted on linux-media: > + * https://www.mail-archive.com/linux-media%40vger.kernel.org/msg92671.html > + */ > + > +/* > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/ctype.h> do you need ctype.h really ? > +#include <linux/types.h> > +#include <linux/gpio.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> > +#include <linux/regulator/consumer.h> > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> those two includes are useless. > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-of.h> > +#include <media/v4l2-ctrls.h> > + > +#define OV5645_VOLTAGE_ANALOG 2800000 > +#define OV5645_VOLTAGE_DIGITAL_CORE 1500000 > +#define OV5645_VOLTAGE_DIGITAL_IO 1800000 > + > +#define OV5645_XCLK_MIN 6000000 > +#define OV5645_XCLK_MAX 24000000 > + > +#define OV5645_SYSTEM_CTRL0 0x3008 > +#define OV5645_SYSTEM_CTRL0_START 0x02 > +#define OV5645_SYSTEM_CTRL0_STOP 0x42 > +#define OV5645_CHIP_ID_HIGH_REG 0x300A > +#define OV5645_CHIP_ID_HIGH 0x56 > +#define OV5645_CHIP_ID_LOW_REG 0x300B > +#define OV5645_CHIP_ID_LOW 0x45 > +#define OV5645_AWB_MANUAL_CONTROL 0x3406 > +#define OV5645_AWB_MANUAL_ENABLE (1 << 0) use BIT(x) here and below for bit operations > +#define OV5645_AEC_PK_MANUAL 0x3503 > +#define OV5645_AEC_MANUAL_ENABLE (1 << 0) > +#define OV5645_AGC_MANUAL_ENABLE (1 << 1) > +#define OV5645_TIMING_TC_REG20 0x3820 > +#define OV5645_SENSOR_VFLIP (1 << 1) > +#define OV5645_ISP_VFLIP (1 << 2) > +#define OV5645_TIMING_TC_REG21 0x3821 > +#define OV5645_SENSOR_MIRROR (1 << 1) > +#define OV5645_PRE_ISP_TEST_SETTING_1 0x503d > +#define OV5645_TEST_PATTERN_MASK 0x3 > +#define OV5645_SET_TEST_PATTERN(x) ((x) & OV5645_TEST_PATTERN_MASK) > +#define OV5645_TEST_PATTERN_ENABLE (1 << 7) > +#define OV5645_SDE_SAT_U 0x5583 > +#define OV5645_SDE_SAT_V 0x5584 > + > +enum ov5645_mode { > + ov5645_mode_min = 0, > + ov5645_mode_sxga_1280_960 = 0, > + ov5645_mode_1080p_1920_1080 = 1, > + ov5645_mode_full_2592_1944 = 2, > + ov5645_mode_max = 2, I'd make enums upper-case and rename them to OV5645_MODE_SXGA, OV5645_FULLHD, OV5645_FULL or something similar ? > +}; > + > +struct reg_value { > + u16 reg; > + u8 val; > +}; > + > +struct ov5645_mode_info { > + enum ov5645_mode mode; > + u32 width; > + u32 height; > + struct reg_value *init_data_ptr; > + u32 init_data_size; > +}; > + > +struct ov5645 { > + struct i2c_client *i2c_client; > + struct device *dev; > + struct v4l2_subdev sd; > + struct media_pad pad; > + struct v4l2_of_endpoint ep; > + struct v4l2_mbus_framefmt fmt; > + struct v4l2_rect crop; just rect? > + struct clk *xclk; > + int xclk_freq; unsigned long xclk_freq > + > + struct regulator *io_regulator; > + struct regulator *core_regulator; > + struct regulator *analog_regulator; > + > + enum ov5645_mode current_mode; > + > + /* Cached control values */ > + struct v4l2_ctrl_handler ctrls; > + struct v4l2_ctrl *saturation; > + struct v4l2_ctrl *hflip; > + struct v4l2_ctrl *vflip; > + struct v4l2_ctrl *autogain; > + struct v4l2_ctrl *autoexposure; > + struct v4l2_ctrl *awb; > + struct v4l2_ctrl *pattern; > + > + struct mutex power_lock; /* lock to protect power_count */ > + int power_count; > + > + int pwdn_gpio; > + int rst_gpio; > +}; > + > +static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct ov5645, sd); > +} > + > +static struct reg_value ov5645_global_init_setting[] = { > + {0x3103, 0x11,}, could you add spaces and remove trailing comma? <cut> > +static struct ov5645_mode_info ov5645_mode_info_data[ov5645_mode_max + 1] = { > + {ov5645_mode_sxga_1280_960, 1280, 960, > + ov5645_setting_sxga, > + ARRAY_SIZE(ov5645_setting_sxga)}, > + {ov5645_mode_1080p_1920_1080, 1920, 1080, > + ov5645_setting_1080P, > + ARRAY_SIZE(ov5645_setting_1080P)}, > + {ov5645_mode_full_2592_1944, 2592, 1944, > + ov5645_setting_full, > + ARRAY_SIZE(ov5645_setting_full)}, > +}; > + could you format this better. > +static void ov5645_regulators_get(struct ov5645 *ov5645) > +{ > + ov5645->io_regulator = devm_regulator_get(ov5645->dev, "DOVDD"); could you move all devm_regulator_get calls in .probe > + if (IS_ERR(ov5645->io_regulator)) { > + ov5645->io_regulator = NULL; this can be EPROBE_DEFER, so you should take care and check the exact error code. > + dev_dbg(ov5645->dev, "cannot get io voltage\n"); > + } else { > + regulator_set_voltage(ov5645->io_regulator, > + OV5645_VOLTAGE_DIGITAL_IO, > + OV5645_VOLTAGE_DIGITAL_IO); error check for regulator_set_voltage? <cut> > + > +static void ov5645_regulators_disable(struct ov5645 *ov5645) > +{ > + if (ov5645->analog_regulator) > + regulator_disable(ov5645->analog_regulator); regulator_disable return error, check it > + if (ov5645->core_regulator) > + regulator_disable(ov5645->core_regulator); > + if (ov5645->io_regulator) > + regulator_disable(ov5645->io_regulator); > +} > + > +static s32 ov5645_write_reg(struct ov5645 *ov5645, u16 reg, u8 val) > +{ > + u8 regbuf[3] = {0}; you set all elements below, so no need to zeroing. > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + regbuf[2] = val; > + > + if (i2c_master_send(ov5645->i2c_client, regbuf, 3) < 0) { > + dev_err(ov5645->dev, "%s:write reg error:reg=%x,val=%x\n", > + __func__, reg, val); > + return -1; return the exact error from i2c_master_send() > + } > + > + return 0; > +} > + > +static s32 ov5645_read_reg(struct ov5645 *ov5645, u16 reg, u8 *val) > +{ > + u8 regbuf[2] = {0}; no need to zero the array > + u8 tmpval = 0; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + > + if (i2c_master_send(ov5645->i2c_client, regbuf, 2) != 2) { > + dev_err(ov5645->dev, "%s:write reg error:reg=%x\n", > + __func__, reg); > + return -1; > + } > + > + if (i2c_master_recv(ov5645->i2c_client, &tmpval, 1) != 1) { > + dev_err(ov5645->dev, "%s:read reg error:reg=%x,val=%x\n", > + __func__, reg, tmpval); > + return -1; > + } > + again, return the exact error from i2c_master > + *val = tmpval; > + > + return tmpval; return zero or the i2c_master error code. > +} > + > +static int ov5645_set_aec_mode(struct ov5645 *ov5645, u32 mode) > +{ > + u8 val; > + > + ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val); > + if (mode == V4L2_EXPOSURE_AUTO) { > + val |= OV5645_AEC_MANUAL_ENABLE; > + } else { /* V4L2_EXPOSURE_MANUAL */ wrong comment, just remove it > + val &= ~OV5645_AEC_MANUAL_ENABLE; > + } > + > + dev_dbg(ov5645->dev, "%s: mode = %d\n", __func__, mode); > + > + return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val); > +} > + > +static int ov5645_set_agc_mode(struct ov5645 *ov5645, u32 enable) > +{ > + u8 val; > + > + ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL, &val); > + if (!enable) > + val |= OV5645_AGC_MANUAL_ENABLE; > + else > + val &= ~OV5645_AGC_MANUAL_ENABLE; > + > + dev_dbg(ov5645->dev, "%s: enable = %d\n", __func__, enable); > + > + return ov5645_write_reg(ov5645, OV5645_AEC_PK_MANUAL, val); > +} > + > +static int ov5645_set_register_array(struct ov5645 *ov5645, > + struct reg_value *settings, > + s32 num_settings) unsigned int num_settings > +{ > + register u16 reg = 0; 'register' ? The compiler is smart enough. > + register u8 val = 0; no need to init val variable > + int i, ret = 0; unsigned int i > + > + for (i = 0; i < num_settings; ++i, ++settings) { > + reg = settings->reg; > + val = settings->val; > + > + ret = ov5645_write_reg(ov5645, reg, val); > + if (ret < 0) > + goto err; > + } > +err: > + return ret; > +} > + > +static int ov5645_init(struct ov5645 *ov5645) > +{ > + struct reg_value *settings = NULL; > + int num_settings = 0; no need to initialize *settings and num_settings variables > + int ret; > + > + settings = ov5645_global_init_setting; > + num_settings = ARRAY_SIZE(ov5645_global_init_setting); > + ret = ov5645_set_register_array(ov5645, settings, num_settings); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int ov5645_change_mode(struct ov5645 *ov5645, enum ov5645_mode mode) > +{ > + struct reg_value *settings = NULL; > + s32 num_settings = 0; > + int ret = 0; > + > + settings = ov5645_mode_info_data[mode].init_data_ptr; > + num_settings = ov5645_mode_info_data[mode].init_data_size; > + ret = ov5645_set_register_array(ov5645, settings, num_settings); > + > + return ret; > +} > + > +static void __ov5645_set_power(struct ov5645 *ov5645, bool on) I personally do not like __* prefix for functions in drivers. > +{ > + dev_dbg(ov5645->dev, "%s: on = %d\n", __func__, on); > + > + if (on) { > + clk_prepare_enable(ov5645->xclk); > + ov5645_regulators_enable(ov5645); check for errors, please. > + usleep_range(5000, 15000); > + if (ov5645->pwdn_gpio) > + gpio_set_value(ov5645->pwdn_gpio, 1); > + usleep_range(1000, 2000); > + if (ov5645->rst_gpio) > + gpio_set_value(ov5645->rst_gpio, 1); > + msleep(20); > + } else { > + if (ov5645->rst_gpio) > + gpio_set_value(ov5645->rst_gpio, 0); > + if (ov5645->pwdn_gpio) > + gpio_set_value(ov5645->pwdn_gpio, 0); > + ov5645_regulators_disable(ov5645); > + clk_disable_unprepare(ov5645->xclk); > + } > +} > + <cut> > + > +static int ov5645_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct device *dev = &client->dev; > + struct device_node *endpoint; > + struct ov5645 *ov5645; > + int ret = 0; > + > + ov5645 = devm_kzalloc(dev, sizeof(struct ov5645), GFP_KERNEL); > + if (!ov5645) > + return -ENOMEM; > + > + ov5645->i2c_client = client; > + ov5645->dev = dev; > + ov5645->fmt.code = MEDIA_BUS_FMT_UYVY8_2X8; > + ov5645->fmt.width = 1920; > + ov5645->fmt.height = 1080; > + ov5645->fmt.field = V4L2_FIELD_NONE; > + ov5645->current_mode = ov5645_mode_1080p_1920_1080; > + > + endpoint = of_graph_get_next_endpoint(dev->of_node, NULL); > + if (!endpoint) { > + dev_err(dev, "endpoint node not found\n"); > + return -EINVAL; > + } > + > + v4l2_of_parse_endpoint(endpoint, &ov5645->ep); check for errors? > + if (ov5645->ep.bus_type != V4L2_MBUS_CSI2) { > + dev_err(dev, "invalid bus type, must be CSI2\n"); > + of_node_put(endpoint); > + return -EINVAL; > + } > + of_node_put(endpoint); > + > + /* get system clock (xclk) frequency */ > + ret = of_property_read_u32(dev->of_node, "clock-rates", > + &ov5645->xclk_freq); > + if (!ret) { > + if (ov5645->xclk_freq < OV5645_XCLK_MIN || > + ov5645->xclk_freq > OV5645_XCLK_MAX) { > + dev_err(dev, "invalid xclk frequency: %d\n", > + ov5645->xclk_freq); > + return -EINVAL; why not just clamp the xclk_freq, and contunue ... > + } > + } > + > + /* get system clock (xclk) */ > + ov5645->xclk = devm_clk_get(dev, "xclk"); > + if (IS_ERR(ov5645->xclk)) { > + dev_err(dev, "could not get xclk"); > + return -EINVAL; just return the error from devm_clk_get > + } > + clk_set_rate(ov5645->xclk, ov5645->xclk_freq); set rate can fail, check the error code > + > + ov5645_regulators_get(ov5645); check for errors. > + > + ret = of_get_named_gpio(dev->of_node, "reset-gpio", 0); use the right api for gpios devm_gpiod_get here and below. > + if (!gpio_is_valid(ret)) { > + dev_dbg(dev, "no reset pin available\n"); > + ov5645->rst_gpio = 0; > + } else { > + ov5645->rst_gpio = ret; > + } > + > + if (ov5645->rst_gpio) { > + ret = devm_gpio_request_one(dev, ov5645->rst_gpio, > + GPIOF_OUT_INIT_LOW, "ov5645-reset"); > + if (ret < 0) { > + dev_err(dev, "could not request reset gpio\n"); > + return ret; > + } > + } regards, Stan -- 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
