Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On 26.04.2018 15:04, Sakari Ailus wrote: > On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote: >> On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: >>> Hi Sakari, >>> >>> On 26.04.2018 09:50, Sakari Ailus wrote: Hi Todor, On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: ... > +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > +{ > + u8 regbuf[3]; > + int ret; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + regbuf[2] = val; > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > + __func__, ret, reg, val); > + return ret; > + } > + > + return 0; How about: return ov7251_write_seq_regs(ov7251, reg, , 1); And put the function below ov2751_write_seq_regs(). >>> >>> I'm not sure... It will calculate message length each time and then check >>> that it is not greater than 5, which it is. Seems redundant. >>> > +} > + > +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > + u8 num) > +{ > + const u8 maxregbuf = 5; > + u8 regbuf[maxregbuf]; > > Apparently this leads to bad positive sparse warning. I'd fix it by: > > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index 3e2c0c03dfa9..d3ebb7529fca 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 > reg, u8 val) > static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > u8 num) > { > - const u8 maxregbuf = 5; > - u8 regbuf[maxregbuf]; > + u8 regbuf[5]; > u8 nregbuf = sizeof(reg) + num * sizeof(*val); > int ret = 0; > > - if (nregbuf > maxregbuf) > + if (nregbuf > sizeof(regbuf)) > return -EINVAL; > > regbuf[0] = reg >> 8; > > Let me know if you're happy with that; I can merge it to the original > patch. Yes, thanks. > > + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > + int ret = 0; > + > + if (nregbuf > maxregbuf) > + return -EINVAL; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + > + memcpy(regbuf + 2, val, num); > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write seq regs error %d: first > reg=%x\n", This line is over 80... >>> >>> Yes indeed. Somehow checkpatch does not report this line, I don't know why. >>> If you're happy with these, I can make the changes, too; they're trivial. >>> >>> Only the second one? Thanks :) >> >> Works for me. I'd still think the overhead of managing the buffer is >> irrelevant where to having an extra function to do essentially the same >> thing is a source of maintenance and review work. Note that we're even now >> spending time to discuss it. ;-) >> >> -- >> Kind regards, >> >> Sakari Ailus >> e-mail: sakari.ai...@iki.fi > -- Best regards, Todor Tomov
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote: > On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: > > Hi Sakari, > > > > On 26.04.2018 09:50, Sakari Ailus wrote: > > > Hi Todor, > > > > > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > > > ... > > >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > > >> +{ > > >> +u8 regbuf[3]; > > >> +int ret; > > >> + > > >> +regbuf[0] = reg >> 8; > > >> +regbuf[1] = reg & 0xff; > > >> +regbuf[2] = val; > > >> + > > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > > >> +if (ret < 0) { > > >> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, > > >> val=%x\n", > > >> +__func__, ret, reg, val); > > >> +return ret; > > >> +} > > >> + > > >> +return 0; > > > > > > How about: > > > > > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > > > > > And put the function below ov2751_write_seq_regs(). > > > > I'm not sure... It will calculate message length each time and then check > > that it is not greater than 5, which it is. Seems redundant. > > > > > > > >> +} > > >> + > > >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 > > >> *val, > > >> + u8 num) > > >> +{ > > >> +const u8 maxregbuf = 5; > > >> +u8 regbuf[maxregbuf]; Apparently this leads to bad positive sparse warning. I'd fix it by: diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c index 3e2c0c03dfa9..d3ebb7529fca 100644 --- a/drivers/media/i2c/ov7251.c +++ b/drivers/media/i2c/ov7251.c @@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, u8 num) { - const u8 maxregbuf = 5; - u8 regbuf[maxregbuf]; + u8 regbuf[5]; u8 nregbuf = sizeof(reg) + num * sizeof(*val); int ret = 0; - if (nregbuf > maxregbuf) + if (nregbuf > sizeof(regbuf)) return -EINVAL; regbuf[0] = reg >> 8; Let me know if you're happy with that; I can merge it to the original patch. > > >> +u8 nregbuf = sizeof(reg) + num * sizeof(*val); > > >> +int ret = 0; > > >> + > > >> +if (nregbuf > maxregbuf) > > >> +return -EINVAL; > > >> + > > >> +regbuf[0] = reg >> 8; > > >> +regbuf[1] = reg & 0xff; > > >> + > > >> +memcpy(regbuf + 2, val, num); > > >> + > > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > > >> +if (ret < 0) { > > >> +dev_err(ov7251->dev, "%s: write seq regs error %d: > > >> first reg=%x\n", > > > > > > This line is over 80... > > > > Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > > > > > > > If you're happy with these, I can make the changes, too; they're trivial. > > > > Only the second one? Thanks :) > > Works for me. I'd still think the overhead of managing the buffer is > irrelevant where to having an extra function to do essentially the same > thing is a source of maintenance and review work. Note that we're even now > spending time to discuss it. ;-) > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ai...@iki.fi -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: > Hi Sakari, > > On 26.04.2018 09:50, Sakari Ailus wrote: > > Hi Todor, > > > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > > ... > >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > >> +{ > >> + u8 regbuf[3]; > >> + int ret; > >> + > >> + regbuf[0] = reg >> 8; > >> + regbuf[1] = reg & 0xff; > >> + regbuf[2] = val; > >> + > >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > >> + if (ret < 0) { > >> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > >> + __func__, ret, reg, val); > >> + return ret; > >> + } > >> + > >> + return 0; > > > > How about: > > > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > > > And put the function below ov2751_write_seq_regs(). > > I'm not sure... It will calculate message length each time and then check > that it is not greater than 5, which it is. Seems redundant. > > > > >> +} > >> + > >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > >> + u8 num) > >> +{ > >> + const u8 maxregbuf = 5; > >> + u8 regbuf[maxregbuf]; > >> + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > >> + int ret = 0; > >> + > >> + if (nregbuf > maxregbuf) > >> + return -EINVAL; > >> + > >> + regbuf[0] = reg >> 8; > >> + regbuf[1] = reg & 0xff; > >> + > >> + memcpy(regbuf + 2, val, num); > >> + > >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > >> + if (ret < 0) { > >> + dev_err(ov7251->dev, "%s: write seq regs error %d: first > >> reg=%x\n", > > > > This line is over 80... > > Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > > > > If you're happy with these, I can make the changes, too; they're trivial. > > Only the second one? Thanks :) Works for me. I'd still think the overhead of managing the buffer is irrelevant where to having an extra function to do essentially the same thing is a source of maintenance and review work. Note that we're even now spending time to discuss it. ;-) -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
Hi Sakari, On 26.04.2018 09:50, Sakari Ailus wrote: > Hi Todor, > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > ... >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) >> +{ >> +u8 regbuf[3]; >> +int ret; >> + >> +regbuf[0] = reg >> 8; >> +regbuf[1] = reg & 0xff; >> +regbuf[2] = val; >> + >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); >> +if (ret < 0) { >> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", >> +__func__, ret, reg, val); >> +return ret; >> +} >> + >> +return 0; > > How about: > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > And put the function below ov2751_write_seq_regs(). I'm not sure... It will calculate message length each time and then check that it is not greater than 5, which it is. Seems redundant. > >> +} >> + >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, >> + u8 num) >> +{ >> +const u8 maxregbuf = 5; >> +u8 regbuf[maxregbuf]; >> +u8 nregbuf = sizeof(reg) + num * sizeof(*val); >> +int ret = 0; >> + >> +if (nregbuf > maxregbuf) >> +return -EINVAL; >> + >> +regbuf[0] = reg >> 8; >> +regbuf[1] = reg & 0xff; >> + >> +memcpy(regbuf + 2, val, num); >> + >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); >> +if (ret < 0) { >> +dev_err(ov7251->dev, "%s: write seq regs error %d: first >> reg=%x\n", > > This line is over 80... Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > If you're happy with these, I can make the changes, too; they're trivial. Only the second one? Thanks :) > >> +__func__, ret, reg); >> +return ret; >> +} >> + >> +return 0; >> +} > -- Best regards, Todor Tomov
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
Hi Todor, On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: ... > +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > +{ > + u8 regbuf[3]; > + int ret; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + regbuf[2] = val; > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > + __func__, ret, reg, val); > + return ret; > + } > + > + return 0; How about: return ov7251_write_seq_regs(ov7251, reg, , 1); And put the function below ov2751_write_seq_regs(). > +} > + > +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > + u8 num) > +{ > + const u8 maxregbuf = 5; > + u8 regbuf[maxregbuf]; > + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > + int ret = 0; > + > + if (nregbuf > maxregbuf) > + return -EINVAL; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + > + memcpy(regbuf + 2, val, num); > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write seq regs error %d: first > reg=%x\n", This line is over 80... If you're happy with these, I can make the changes, too; they're trivial. > + __func__, ret, reg); > + return ret; > + } > + > + return 0; > +} -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
[PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
The ov7251 sensor is a 1/7.5-Inch B VGA (640x480) CMOS Digital Image Sensor from Omnivision. The driver supports the following modes: - 640x480 30fps - 640x480 60fps - 640x480 90fps Output format is 10bit B RAW - MEDIA_BUS_FMT_Y10_1X10. The driver supports configuration via user controls for: - exposure and gain; - horizontal and vertical flip; - test pattern. Signed-off-by: Todor TomovReviewed-by: Jacopo Mondi --- drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/ov7251.c | 1503 3 files changed, 1516 insertions(+) create mode 100644 drivers/media/i2c/ov7251.c diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 541f0d28..3dd16c6 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -688,6 +688,18 @@ config VIDEO_OV5695 To compile this driver as a module, choose M here: the module will be called ov5695. +config VIDEO_OV7251 + tristate "OmniVision OV7251 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + select V4L2_FWNODE + help + This is a Video4Linux2 sensor-level driver for the OmniVision + OV7251 camera. + + To compile this driver as a module, choose M here: the + module will be called ov7251. + config VIDEO_OV772X tristate "OmniVision OV772x sensor support" depends on I2C && VIDEO_V4L2 diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index ea34aee..c8585b1 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o obj-$(CONFIG_VIDEO_OV5670) += ov5670.o obj-$(CONFIG_VIDEO_OV5695) += ov5695.o obj-$(CONFIG_VIDEO_OV6650) += ov6650.o +obj-$(CONFIG_VIDEO_OV7251) += ov7251.o obj-$(CONFIG_VIDEO_OV7640) += ov7640.o obj-$(CONFIG_VIDEO_OV7670) += ov7670.o obj-$(CONFIG_VIDEO_OV772X) += ov772x.o diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c new file mode 100644 index 000..c05a9de --- /dev/null +++ b/drivers/media/i2c/ov7251.c @@ -0,0 +1,1503 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for the OV7251 camera sensor. + * + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. + * Copyright (c) 2017-2018, Linaro Ltd. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define OV7251_SC_MODE_SELECT 0x0100 +#define OV7251_SC_MODE_SELECT_SW_STANDBY 0x0 +#define OV7251_SC_MODE_SELECT_STREAMING0x1 + +#define OV7251_CHIP_ID_HIGH0x300a +#define OV7251_CHIP_ID_HIGH_BYTE 0x77 +#define OV7251_CHIP_ID_LOW 0x300b +#define OV7251_CHIP_ID_LOW_BYTE0x50 +#define OV7251_SC_GP_IO_IN10x3029 +#define OV7251_AEC_EXPO_0 0x3500 +#define OV7251_AEC_EXPO_1 0x3501 +#define OV7251_AEC_EXPO_2 0x3502 +#define OV7251_AEC_AGC_ADJ_0 0x350a +#define OV7251_AEC_AGC_ADJ_1 0x350b +#define OV7251_TIMING_FORMAT1 0x3820 +#define OV7251_TIMING_FORMAT1_VFLIPBIT(2) +#define OV7251_TIMING_FORMAT2 0x3821 +#define OV7251_TIMING_FORMAT2_MIRROR BIT(2) +#define OV7251_PRE_ISP_00 0x5e00 +#define OV7251_PRE_ISP_00_TEST_PATTERN BIT(7) + +struct reg_value { + u16 reg; + u8 val; +}; + +struct ov7251_mode_info { + u32 width; + u32 height; + const struct reg_value *data; + u32 data_size; + u32 pixel_clock; + u32 link_freq; + u16 exposure_max; + u16 exposure_def; + struct v4l2_fract timeperframe; +}; + +struct ov7251 { + struct i2c_client *i2c_client; + struct device *dev; + struct v4l2_subdev sd; + struct media_pad pad; + struct v4l2_fwnode_endpoint ep; + struct v4l2_mbus_framefmt fmt; + struct v4l2_rect crop; + struct clk *xclk; + u32 xclk_freq; + + struct regulator *io_regulator; + struct regulator *core_regulator; + struct regulator *analog_regulator; + + const struct ov7251_mode_info *current_mode; + + struct v4l2_ctrl_handler ctrls; + struct v4l2_ctrl *pixel_clock; + struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *exposure; + struct v4l2_ctrl *gain; + + /* Cached register values */ + u8 aec_pk_manual; + u8 pre_isp_00; + u8 timing_format1; + u8 timing_format2; + + struct mutex lock; /* lock to protect power state, ctrls and mode */ + bool power_on; + + struct gpio_desc *enable_gpio; +}; + +static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd) +{ + return container_of(sd, struct ov7251, sd); +} + +static const struct reg_value ov7251_global_init_setting[] = { +