| Message ID | 20240626211529.2068473-1-shill@d3engineering.com |
|---|---|
| Headers | show |
| Series | media: i2c: Add driver for Sony IMX728 | expand |
Hi Spencer, thanks for the patch. Just having a glimpse and giving some feedback. Am Mittwoch, 26. Juni 2024, 23:15:28 CEST schrieb Spencer Hill: > Add a driver for the Sony IMX728 image sensor. > > Signed-off-by: Spencer Hill <shill@d3engineering.com> > --- > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx728.c | 1167 ++++++++++++ > drivers/media/i2c/imx728.h | 3458 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 4637 insertions(+) > create mode 100644 drivers/media/i2c/imx728.c > create mode 100644 drivers/media/i2c/imx728.h > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index c6d3ee472d81..46b6463c558a 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -233,6 +233,17 @@ config VIDEO_IMX415 > To compile this driver as a module, choose M here: the > module will be called imx415. > > +config VIDEO_IMX728 > + tristate "Sony IMX728 sensor support" > + depends on OF_GPIO > + select V4L2_CCI_I2C > + help > + This is a Video4Linux2 sensor driver for the Sony > + IMX728 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx728. > + > config VIDEO_MAX9271_LIB > tristate > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index dfbe6448b549..1188420ee1b4 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > obj-$(CONFIG_VIDEO_IMX412) += imx412.o > obj-$(CONFIG_VIDEO_IMX415) += imx415.o > +obj-$(CONFIG_VIDEO_IMX728) += imx728.o > obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o > obj-$(CONFIG_VIDEO_KS0127) += ks0127.o > diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c > new file mode 100644 > index 000000000000..b23359133a22 > --- /dev/null > +++ b/drivers/media/i2c/imx728.c > @@ -0,0 +1,1167 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Sony IMX728 CMOS Image Sensor Driver > + * > + * Copyright (c) 2024 Define Design Deploy Corp > + */ > + > +#include <linux/delay.h> > +#include <linux/clk.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/v4l2-mediabus.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > + > +#include "imx728.h" > + > +static inline struct imx728 *to_imx728(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct imx728, subdev); > +} > + > +static int imx728_read(struct imx728 *imx728, u16 addr, u32 *val, size_t nbytes) Use cci_read instead and remove this function. > +{ > + int ret; > + __le32 val_le = 0; > + > + ret = regmap_bulk_read(imx728->regmap, addr, &val_le, nbytes); > + if (ret < 0) { > + dev_err(imx728->dev, "%s: failed to read reg 0x%04x: %d\n", > + __func__, addr, ret); > + return ret; > + } > + > + *val = le32_to_cpu(val_le); > + > + return 0; > +} > + > +static int imx728_write(struct imx728 *imx728, u16 addr, u32 val, size_t nbytes) Use cci_write instead and remove this function. > +{ > + int ret; > + __le32 val_le = cpu_to_le32(val); > + > + ret = regmap_bulk_write(imx728->regmap, addr, &val_le, nbytes); > + if (ret < 0) > + dev_err(imx728->dev, "%s: failed to write reg 0x%04x: %d\n", > + __func__, addr, ret); > + > + return ret; > +} > + > +static int imx728_update_bits(struct imx728 *imx728, u16 addr, u32 val, u32 mask, size_t nbytes) Use cci_update_bits instead and remove this function. > +{ > + int ret; > + u32 cfg; > + > + ret = imx728_read(imx728, addr, &cfg, nbytes); > + if (ret < 0) > + return ret; > + > + cfg = (val) ? (cfg | mask) : (cfg & (~mask)); > + > + return imx728_write(imx728, addr, cfg, nbytes); > +} > + > +static int imx728_write_table(struct imx728 *imx728, > + const struct reg_sequence *regs, > + unsigned int nr_regs) Use cci_multi_reg_write instead and remove this function. > +{ > + int ret; > + > + ret = regmap_multi_reg_write(imx728->regmap, regs, nr_regs); > + if (ret < 0) > + dev_err(imx728->dev, > + "%s: failed to write reg table (%d)!\n", __func__, ret); > + return ret; > +} > + > +static int imx728_wait_for_state(struct imx728 *imx728, enum imx728_sensor_state state) > +{ > + int ret, i; > + u32 val; > + > + for (i = 0; i < 50; i++) { > + ret = imx728_read(imx728, 0x2CAC, &val, 1); Please add proper register defines using CCI_REG* macros. > + if (ret == 0 && val == state) { > + dev_info(imx728->dev, "%s: Enter state %u\n", __func__, val); > + return 0; > + } > + usleep_range(1000, 10000); > + } > + > + return -EBUSY; > +} > + > +static void imx728_init_formats(struct v4l2_subdev_state *state) > +{ > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_subdev_state_get_format(state, 0, 0); > + format->code = imx728_mbus_formats[0]; > + format->width = imx728_framesizes[0].width; > + format->height = imx728_framesizes[0].height; > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_SMPTE170M; Are you sure about this colorspace? I would have expected V4L2_COLORSPACE_RAW, but I don't know any details on this hardware. Also set ycbcr_enc, quantization and xfer_func. > +} > + > +static int _imx728_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) Why this special variant with a underscore? Just move the code into imx728_set_routing. > +{ > + struct v4l2_subdev_route routes[] = { > + { > + .source_pad = 0, > + .source_stream = 0, > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + }, > + { > + .source_pad = 0, > + .source_stream = 1, > + } > + }; > + > + struct v4l2_subdev_krouting routing = { > + .num_routes = ARRAY_SIZE(routes), > + .routes = routes, > + }; > + > + int ret; > + > + ret = v4l2_subdev_set_routing(sd, state, &routing); > + if (ret < 0) > + return ret; > + > + imx728_init_formats(state); > + > + return 0; > +} > + > +static int imx728_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + > + if (code->index >= ARRAY_SIZE(imx728_mbus_formats)) > + return -EINVAL; > + > + code->code = imx728_mbus_formats[code->index]; > + > + return 0; > +} > + > +static int imx728_enum_frame_sizes(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > + if (imx728_mbus_formats[i] == fse->code) > + break; > + } > + > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > + return -EINVAL; > + > + if (fse->index >= ARRAY_SIZE(imx728_framesizes)) > + return -EINVAL; > + > + fse->min_width = imx728_framesizes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->min_height = imx728_framesizes[fse->index].height; > + fse->max_height = fse->min_height; > + > + return 0; > +} > + > +static int imx728_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + struct v4l2_mbus_framefmt *format; > + const struct v4l2_area *fsize; > + unsigned int i; > + u32 code; > + int ret = 0; > + > + if (fmt->pad != 0) > + return -EINVAL; > + > + if (fmt->stream != 0) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > + if (imx728_mbus_formats[i] == fmt->format.code) { > + code = fmt->format.code; > + break; > + } > + } > + > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > + code = imx728_mbus_formats[0]; > + > + fsize = v4l2_find_nearest_size(imx728_framesizes, > + ARRAY_SIZE(imx728_framesizes), width, > + height, fmt->format.width, > + fmt->format.height); > + > + mutex_lock(&imx728->lock); > + > + format = v4l2_subdev_state_get_format(state, fmt->pad, fmt->stream); > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx728->streaming) { > + ret = -EBUSY; > + goto done; > + } > + > + format->code = code; > + format->width = fsize->width; > + format->height = fsize->height; This should be done before calling v4l2_subdev_state_get_format, no? Also ycbcr_enc, quantization and xfer_func are missing. > + > + fmt->format = *format; > + > +done: > + mutex_unlock(&imx728->lock); > + > + return ret; > +} > + > +static int imx728_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + struct v4l2_subdev_state *state; > + struct v4l2_mbus_framefmt *fmt; > + u32 bpp; > + int ret = 0; > + > + if (pad != 0) > + return -EINVAL; > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + fmt = v4l2_subdev_state_get_format(state, 0, 0); > + if (!fmt) { > + ret = -EPIPE; > + goto out; > + } > + > + memset(fd, 0, sizeof(*fd)); > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > + > + bpp = 10; > + > + fd->entry[fd->num_entries].stream = 0; > + > + fd->entry[fd->num_entries].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; > + fd->entry[fd->num_entries].length = fmt->width * fmt->height * bpp / 8; > + fd->entry[fd->num_entries].pixelcode = fmt->code; > + fd->entry[fd->num_entries].bus.csi2.vc = 0; > + fd->entry[fd->num_entries].bus.csi2.dt = 0x2b; > + > + fd->num_entries++; > + > +out: > + > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > + > +static int imx728_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + enum v4l2_subdev_format_whence which, > + struct v4l2_subdev_krouting *routing) > +{ > + int ret; > + > + if (routing->num_routes == 0 || routing->num_routes > 1) > + return -EINVAL; > + > + ret = _imx728_set_routing(sd, state); > + > + return ret; > +} > + > +static int imx728_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx728 *imx728 = container_of(ctrl->handler, > + struct imx728, ctrl.handler); > + int ret = 0; > + > + dev_dbg(imx728->dev, "%s: %s, value: %d\n", > + __func__, ctrl->name, ctrl->val); > + > + if (!pm_runtime_get_if_in_use(imx728->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = imx728_write(imx728, 0x98DC, ctrl->val, 4); > + break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = imx728_update_bits(imx728, 0x98F8, > + (ctrl->val * 10), > + 0x1FFFF, 4); > + break; > + case V4L2_CID_HFLIP: > + ret = imx728_update_bits(imx728, 0x9651, > + ctrl->val, 0x1, 1); > + ret |= imx728_update_bits(imx728, 0xB67C, > + ctrl->val, 0x1, 1); > + break; > + case V4L2_CID_VFLIP: > + ret = imx728_update_bits(imx728, 0x9651, > + ctrl->val << 1, 0x2, 1); > + ret = imx728_update_bits(imx728, 0xB67D, > + ctrl->val, 0x1, 1); > + break; > + case V4L2_CID_WIDE_DYNAMIC_RANGE: > + case V4L2_CID_TEST_PATTERN: > + // Both of these are configured during start stream. Are they not configurable while streaming? > + ret = 0; > + break; > + default: > + ret = -EINVAL; > + } > + > + pm_runtime_put_noidle(imx728->dev); > + return ret; > +} > + > +static int imx728_detect(struct imx728 *imx728) > +{ > + int ret; > + u32 minor, major; > + > + ret = imx728_read(imx728, 0x6002, &major, 1); > + if (ret != 0) { > + dev_err(imx728->dev, "Could not read PARAM_MAJOR_VER!"); > + return ret; > + } > + ret = imx728_read(imx728, 0x6000, &minor, 1); > + if (ret != 0) { > + dev_err(imx728->dev, "Could not read PARAM_MINOR_VER!"); > + return ret; > + } > + dev_dbg(imx728->dev, "Got version: %d.%d", major, minor); > + > + return 0; > +} > + > +static int imx728_reset(struct imx728 *imx728) > +{ > + > + if (!IS_ERR_OR_NULL(imx728->xclr_gpio)) { > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > + usleep_range(1000, 10000); > + gpiod_set_value_cansleep(imx728->xclr_gpio, 0); > + msleep(100); > + > + return 0; > + } > + > + return -1; > +} > + > +static int imx728_power_on(struct imx728 *imx728) > +{ > + int ret; > + > + ret = clk_prepare_enable(imx728->clk); > + if (ret < 0) > + return ret; > + > + imx728_reset(imx728); > + return 0; > +} > + > +static int imx728_power_off(struct imx728 *imx728) > +{ > + > + if (imx728->xclr_gpio) { > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > + > + usleep_range(1, 10); > + } > + clk_disable_unprepare(imx728->clk); > + return 0; > +} > + > +static int imx728_get_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + > + fi->interval.numerator = 1; > + fi->interval.denominator = imx728->fps; > + return 0; > +} > + > +static int imx728_set_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + u32 req_fps; > + > + mutex_lock(&imx728->lock); > + > + if (fi->interval.numerator == 0 || fi->interval.denominator == 0) { > + fi->interval.denominator = IMX728_FRAMERATE_DEFAULT; > + fi->interval.numerator = 1; > + } > + > + req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator, > + fi->interval.numerator), > + IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX); > + > + fi->interval.numerator = 1; > + fi->interval.denominator = req_fps; > + > + imx728->fps = req_fps; > + > + mutex_unlock(&imx728->lock); > + dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps); > + > + return 0; > +} > + > +static int imx728_powerup_to_standby(struct imx728 *imx728) > +{ > + int ret; > + > + dev_info(imx728->dev, "powerup -> standby..."); > + > + ret = imx728_reset(imx728); > + if (ret) { > + dev_err(imx728->dev, "Error resetting: %i", ret); > + return ret; > + } > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_SLEEP); > + if (ret < 0) { > + dev_err(imx728->dev, "Could not transition to Sleep state!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B20, imx728->clk_rate / 1000000, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B1C, 0x1, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); Error message doesn't seem to match. This is a fixed write, independent from any frequency. > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B05, 0xFF, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write to CK_SLEEP!"); > + return ret; > + } > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't transition from Sleep to Standby state!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xFFFF, IMX728_REMAP_MODE_STANDBY, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write regmap mode!"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_hdr_fixed_brightness(struct imx728 *imx728) > +{ > + int ret; > + > + // Sony recommended values > + unsigned int exposure_sp1_sp2_us = 10000; > + unsigned int exposure_sp1vs_us = 56; > + unsigned int sp1h_gain = 240; > + unsigned int sp1l_gain = 75; > + unsigned int sp1ec_gain = 21; > + unsigned int sp2_gain = 33; > + unsigned int sp1vs_gain = 84; > + > + ret = imx728_write(imx728, 0x98DC, exposure_sp1_sp2_us, 4); > + ret |= imx728_write(imx728, 0x98E4, exposure_sp1_sp2_us, 4); > + ret |= imx728_write(imx728, 0x98EC, exposure_sp1vs_us, 4); > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write fixed exposure values."); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x98F8, > + sp1h_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x98FC, > + sp1l_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9900, > + sp1ec_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9904, > + sp2_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9908, > + sp1vs_gain, > + 0x1FFFF, > + 4); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write fixed gain values."); > + return ret; > + } > + > + dev_info(imx728->dev, "Wrote fixed brightness for HDR"); > + > + return 0; > +} > + > +static int imx728_hdr_configure( > + struct imx728 *imx728, > + struct imx728_ctrl_point *points, > + int hdr_bits) > +{ > + u32 hdr_norm_x0; > + u32 hdr_norm_x1; > + u32 hdr_norm_y0; > + u32 hdr_norm_y1; > + > + int ret; > + int i; > + > + switch (hdr_bits) { > + case 20: > + hdr_norm_x0 = 0x2000; > + hdr_norm_x1 = 0x5000; > + > + hdr_norm_y0 = 0x0; > + hdr_norm_y1 = 0xd000; > + break; > + case 24: > + hdr_norm_x0 = 0x3000; > + hdr_norm_x1 = 0x5000; > + > + hdr_norm_y0 = 0x0; > + hdr_norm_y1 = 0xe000; > + break; > + default: > + dev_err(imx728->dev, "%i bit HDR not supported.", hdr_bits); > + break; > + } > + > + ret = imx728_write(imx728, 0x9C60, hdr_norm_x0, 4); > + ret |= imx728_write(imx728, 0x9C6C, hdr_norm_x0, 4); > + ret |= imx728_write(imx728, 0x9C64, hdr_norm_x1, 4); > + ret |= imx728_write(imx728, 0x9C70, hdr_norm_x1, 4); > + ret |= imx728_write(imx728, 0x9C68, hdr_norm_y0, 2); > + ret |= imx728_write(imx728, 0x9C74, hdr_norm_y0, 2); > + ret |= imx728_write(imx728, 0x9C6A, hdr_norm_y1, 2); > + ret |= imx728_write(imx728, 0x9C76, hdr_norm_y1, 2); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed when setting HDR Normalization gains: %i", ret); > + return ret; > + } > + > + for (i = 0; i < 16; i++) { > + ret = imx728_write( > + imx728, > + IMX728_REG_CTRL_POINT_X(i), > + points->x, 4); > + > + ret |= imx728_write( > + imx728, > + IMX728_REG_CTRL_POINT_Y(i), > + points->y, 4); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write control point %i", i); > + return ret; > + } > + > + if ((points+1)->x >= 0 && (points+1)->y >= 0) > + points++; > + } > + > + return imx728_hdr_fixed_brightness(imx728); > +} > + > +static int imx728_configure(struct imx728 *imx728) > +{ > + int ret; > + bool hdr = imx728->ctrl.wdr->val; > + enum imx728_img_raw_mode img_out_mode; > + enum imx728_drive_mode mode_sel; > + enum imx728_aemode ae_mode; > + > + if (hdr) { > + img_out_mode = IMX728_IMG_MODE_HDR; > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > + ae_mode = IMX728_AEMODE_FULL_ME; > + } else { > + img_out_mode = IMX728_IMG_MODE_LINEAR; > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > + ae_mode = IMX728_AEMODE_FULL_ME; > + } > + > + ret = imx728_write(imx728, 0x98AC, ae_mode, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't set AE mode!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xA248, IMX728_AWBMODE_FULL_MWB, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't set full manual white balance mode!"); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x1808, 0x1, 0x1, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't enable full manual white balance mode!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98E0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98E8, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98F0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + if (hdr) { > + ret = imx728_hdr_configure(imx728, imx728_hdr_20bit, 20); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't configure sensor for HDR mode: %i", ret); > + return ret; > + } > + } > + > + dev_info(imx728->dev, "Disabling metadata..."); > + ret = imx728_write(imx728, 0x1708, 0x00, 1); > + ret |= imx728_write(imx728, 0x1709, 0x00, 1); > + ret |= imx728_write(imx728, 0x170A, 0x00, 1); > + ret |= imx728_write(imx728, 0x1B40, 0x00, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error disabling metadata: %i", ret); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x9728, > + mode_sel, > + 0x7FFF, > + 2); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write mode select."); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0xEC7E, > + img_out_mode, > + 0x7, > + 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't select image out mode."); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xEC12, 0x28, 2); > + ret |= imx728_write(imx728, 0xEC14, 0x0, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error disabling OB output."); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1761, 0x0, 1); > + if (ret < 0) > + dev_err(imx728->dev, "Error disabling skew calibration from sensor to SER"); > + > + switch (imx728->ctrl.pg_mode->val) { > + case 0: > + break; > + case 1: > + // Horizontal Color Bars > + ret = imx728_write(imx728, 0x1A2A, 8, 2); > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > + ret |= imx728_write(imx728, 0xB58F, 0x0, 1); > + ret |= imx728_write(imx728, 0xB6C5, 0x0, 1); > + break; > + case 2: > + // Vertical Color Bars > + ret = imx728_write(imx728, 0x1A2C, 16, 2); > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > + ret |= imx728_write(imx728, 0xB58F, 0x1, 1); > + ret |= imx728_write(imx728, 0xB6C5, 0x1, 1); > + break; > + default: > + break; > + } > + > + // Assume that everything except 'disabled' requires pattern gen enable > + if (imx728->ctrl.pg_mode->val != 0) { > + ret |= imx728_write(imx728, 0xB58E, 0x1, 1); > + ret |= imx728_write(imx728, 0xB6C4, 0x1, 1); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to enable PG mode."); > + return ret; > + } > + } > + > + ret = imx728_update_bits(imx728, 0x9714, > + IMX728_RAW_SEL_SP1H, > + 0x7, > + 1); > + ret |= imx728_update_bits(imx728, 0xB684, > + IMX728_RAW_SEL_SP1H, > + 0x7, > + 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to set subpixel register"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_start_stream(struct imx728 *imx728) > +{ > + int ret; > + > + ret = imx728_powerup_to_standby(imx728); > + if (ret < 0) > + return ret; > + > + ret = imx728_write_table(imx728, imx728_3840x2160, ARRAY_SIZE(imx728_3840x2160)); > + if (ret < 0) > + return ret; > + > + msleep(100); > + > + ret = imx728_configure(imx728); > + if (ret < 0) > + return ret; > + > + ret = __v4l2_ctrl_handler_setup(imx728->subdev.ctrl_handler); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: failed to apply v4l2 ctrls: %d\n", __func__, ret); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B04, 0x5C, 1); > + if (ret < 0) > + return ret; > + ret = imx728_write(imx728, 0x1B04, 0xA3, 1); > + if (ret < 0) > + return ret; > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STREAMING); > + if (ret < 0) { > + dev_err(imx728->dev, "Could not transition to Streaming!"); > + return ret; > + } > + > + msleep(20); > + > + return 0; > +} > + > +static int imx728_stop_stream(struct imx728 *imx728) > +{ > + int ret; > + > + ret = imx728_write(imx728, 0x1B04, 0xFF, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error sending stop stream command: %i", ret); > + return ret; > + } > + > + imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't transition out of Streaming mode!"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_set_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + mutex_lock(&imx728->lock); > + if (imx728->streaming == enable) { > + mutex_unlock(&imx728->lock); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_get_sync(imx728->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(imx728->dev); > + goto err_unlock; > + } > + > + ret = imx728_start_stream(imx728); > + if (ret < 0) > + goto err_runtime_put; > + } else { > + ret = imx728_stop_stream(imx728); > + if (ret < 0) > + goto err_runtime_put; > + pm_runtime_mark_last_busy(imx728->dev); > + pm_runtime_put_autosuspend(imx728->dev); > + } > + > + imx728->streaming = enable; > + > + __v4l2_ctrl_grab(imx728->ctrl.wdr, enable); > + __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable); > + __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable); > + __v4l2_ctrl_grab(imx728->ctrl.pg_mode, enable); > + > + mutex_unlock(&imx728->lock); > + return 0; > + > +err_runtime_put: > + pm_runtime_put(imx728->dev); > + > +err_unlock: > + mutex_unlock(&imx728->lock); > + dev_err(imx728->dev, > + "%s: failed to setup streaming %d\n", __func__, ret); > + return ret; > +} > + > +static const struct v4l2_subdev_core_ops imx728_core_ops = { > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > +}; > + > +static const struct v4l2_subdev_video_ops imx728_subdev_video_ops = { > + .s_stream = imx728_set_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops imx728_subdev_pad_ops = { > + .enum_mbus_code = imx728_enum_mbus_code, > + .enum_frame_size = imx728_enum_frame_sizes, > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = imx728_set_fmt, > + .get_frame_interval = imx728_get_frame_interval, > + .set_frame_interval = imx728_set_frame_interval, > + .set_routing = imx728_set_routing, > + .get_frame_desc = imx728_get_frame_desc, > +}; > + > +static const struct v4l2_subdev_ops imx728_subdev_ops = { > + .core = &imx728_core_ops, > + .video = &imx728_subdev_video_ops, > + .pad = &imx728_subdev_pad_ops, > +}; > + > +static const struct v4l2_ctrl_ops imx728_ctrl_ops = { > + .s_ctrl = imx728_set_ctrl, > +}; > + > +static int imx728_probe(struct i2c_client *client) > +{ > + struct imx728 *imx728; > + struct v4l2_subdev *sd; > + struct v4l2_ctrl_handler *ctrl_hdr; > + int ret; > + > + imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL); > + if (!imx728) > + return -ENOMEM; > + > + imx728->dev = &client->dev; > + > + imx728->regmap = devm_regmap_init_i2c(client, &imx728_regmap_config); Please use devm_cci_regmap_init_i2c(). > + if (IS_ERR(imx728->regmap)) > + return PTR_ERR(imx728->regmap); > + > + imx728->xclr_gpio = devm_gpiod_get_optional(imx728->dev, > + "xclr", GPIOD_OUT_LOW); > + if (IS_ERR(imx728->xclr_gpio)) > + return PTR_ERR(imx728->xclr_gpio); > + > + imx728->clk = devm_clk_get(imx728->dev, "inck"); > + if (IS_ERR(imx728->clk)) > + return PTR_ERR(imx728->clk); > + > + imx728->clk_rate = clk_get_rate(imx728->clk); > + dev_info(imx728->dev, "inck rate: %lu Hz\n", imx728->clk_rate); No need for this. clock frequency is available in debugfs if necessary. > + > + if (imx728->clk_rate < 18000000 || imx728->clk_rate > 30000000) > + return -EINVAL; > + > + ret = imx728_power_on(imx728); > + if (ret < 0) > + return ret; > + > + ret = imx728_detect(imx728); > + if (ret < 0) > + return ret; > + > + sd = &imx728->subdev; > + v4l2_i2c_subdev_init(sd, client, &imx728_subdev_ops); > + > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS | > + V4L2_SUBDEV_FL_STREAMS; > + > + imx728->pad.flags = MEDIA_PAD_FL_SOURCE; > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ret = media_entity_pads_init(&sd->entity, 1, &imx728->pad); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: media entity init failed %d\n", __func__, ret); > + return ret; > + } > + > + ctrl_hdr = &imx728->ctrl.handler; > + ret = v4l2_ctrl_handler_init(ctrl_hdr, 8); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: ctrl handler init failed: %d\n", __func__, ret); > + goto err_media_cleanup; > + } > + > + mutex_init(&imx728->lock); > + imx728->ctrl.handler.lock = &imx728->lock; > + imx728->fps = IMX728_FRAMERATE_DEFAULT; > + > + imx728->ctrl.exposure = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_EXPOSURE, 0, > + 33000, 1, > + IMX728_EXPOSURE_DEFAULT); > + > + imx728->ctrl.again = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, 0, > + 102, 1, > + 24); > + > + imx728->ctrl.wdr = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_WIDE_DYNAMIC_RANGE, > + 0, 1, 1, 1); > + > + imx728->ctrl.h_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_HFLIP, 0, 1, 1, 0); > + > + imx728->ctrl.v_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_VFLIP, 0, 1, 1, 0); > + > + imx728->ctrl.pg_mode = v4l2_ctrl_new_std_menu_items(ctrl_hdr, > + &imx728_ctrl_ops, V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(imx728_ctrl_pg_qmenu) - 1, > + 0, 0, imx728_ctrl_pg_qmenu); > + > + imx728->ctrl.pixel_rate = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_PIXEL_RATE, > + IMX728_PIXEL_RATE, > + IMX728_PIXEL_RATE, 1, > + IMX728_PIXEL_RATE); > + > + imx728->ctrl.link_freq = v4l2_ctrl_new_int_menu(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(imx728_link_freq_menu) - 1, 0, > + imx728_link_freq_menu); > + > + if (imx728->ctrl.link_freq) > + imx728->ctrl.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + imx728->subdev.ctrl_handler = ctrl_hdr; > + if (imx728->ctrl.handler.error) { > + ret = imx728->ctrl.handler.error; > + dev_err(imx728->dev, > + "%s: failed to add the ctrls: %d\n", __func__, ret); > + goto err_ctrl_free; > + } > + > + pm_runtime_set_active(imx728->dev); > + pm_runtime_enable(imx728->dev); > + pm_runtime_set_autosuspend_delay(imx728->dev, IMX728_PM_IDLE_TIMEOUT); > + pm_runtime_use_autosuspend(imx728->dev); > + pm_runtime_get_noresume(imx728->dev); > + > + ret = v4l2_subdev_init_finalize(sd); > + if (ret < 0) > + goto err_pm_disable; > + > + ret = v4l2_async_register_subdev(sd); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: v4l2 subdev register failed %d\n", __func__, ret); > + goto err_subdev_cleanup; > + } > + > + dev_info(imx728->dev, "imx728 probed\n"); > + pm_runtime_mark_last_busy(imx728->dev); > + pm_runtime_put_autosuspend(imx728->dev); > + return 0; > + > +err_subdev_cleanup: > + v4l2_subdev_cleanup(&imx728->subdev); > + > +err_pm_disable: > + pm_runtime_dont_use_autosuspend(imx728->dev); > + pm_runtime_put_noidle(imx728->dev); > + pm_runtime_disable(imx728->dev); > + > +err_ctrl_free: > + v4l2_ctrl_handler_free(ctrl_hdr); > + mutex_destroy(&imx728->lock); > + > +err_media_cleanup: > + media_entity_cleanup(&imx728->subdev.entity); > + > + return ret; > +} > + > +static int __maybe_unused imx728_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + ret = imx728_power_on(imx728); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int __maybe_unused imx728_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + > + imx728_power_off(imx728); > + > + return 0; > +} > + > +static int __maybe_unused imx728_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + if (imx728->streaming) > + imx728_stop_stream(imx728); > + > + ret = pm_runtime_force_suspend(dev); > + if (ret < 0) > + dev_warn(dev, "%s: failed to suspend: %i\n", __func__, ret); > + > + return 0; > +} > + > +static int __maybe_unused imx728_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + ret = pm_runtime_force_resume(dev); > + if (ret < 0) > + dev_warn(dev, "%s: failed to resume: %i\n", __func__, ret); > + > + if (imx728->streaming) > + ret = imx728_start_stream(imx728); > + > + if (ret < 0) { > + imx728_stop_stream(imx728); > + imx728->streaming = 0; > + return ret; > + } > + > + return 0; > +} > + > +static void imx728_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + > + v4l2_async_unregister_subdev(sd); > + v4l2_ctrl_handler_free(&imx728->ctrl.handler); > + v4l2_subdev_cleanup(&imx728->subdev); > + media_entity_cleanup(&sd->entity); > + mutex_destroy(&imx728->lock); > + > + pm_runtime_disable(imx728->dev); > + pm_runtime_dont_use_autosuspend(imx728->dev); > + pm_runtime_set_suspended(imx728->dev); > +} > + > +static const struct dev_pm_ops imx728_pm_ops = { > + SET_RUNTIME_PM_OPS(imx728_runtime_suspend, > + imx728_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(imx728_suspend, imx728_resume) > +}; > + > +static const struct of_device_id imx728_dt_id[] = { > + { .compatible = "sony,imx728" }, > + { /* sentinel */ } > +}; > + > +MODULE_DEVICE_TABLE(of, imx728_dt_id); > + > +static struct i2c_driver imx728_i2c_driver = { > + .driver = { > + .name = "imx728", > + .of_match_table = of_match_ptr(imx728_dt_id), > + .pm = &imx728_pm_ops, > + }, > + .probe = imx728_probe, > + .remove = imx728_remove, > +}; > + > +module_i2c_driver(imx728_i2c_driver); > + > +MODULE_DESCRIPTION("Camera Sensor Driver for Sony IMX728"); > +MODULE_AUTHOR("Spencer Hill <shill@d3engineering.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/media/i2c/imx728.h b/drivers/media/i2c/imx728.h > new file mode 100644 > index 000000000000..6f320214b780 > --- /dev/null > +++ b/drivers/media/i2c/imx728.h There is only a single user of this header, move this into the c file. > @@ -0,0 +1,3458 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Sony IMX728 CMOS Image Sensor Driver > + * > + * Copyright (c) 2024 Define Design Deploy Corp > + */ > + > +#include <linux/types.h> > + > +#define IMX728_OUT_WIDTH 3840 > +#define IMX728_OUT_HEIGHT 2160 > + > +#define IMX728_FRAMERATE_MAX 30 > +#define IMX728_FRAMERATE_DEFAULT 30 > +#define IMX728_FRAMERATE_MIN 10 > + > +#define IMX728_PIXEL_RATE 225504000 > +#define IMX728_LINK_FREQ 800000000 > + > +#define IMX728_EXPOSURE_DEFAULT 10000 > + > +#define IMX728_PM_IDLE_TIMEOUT 1000 > + > + > +#define IMX728_REG_CTRL_POINT_X(i) (0xA198 + (i) * 8) > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4) > + > +enum imx728_sensor_state { > + IMX728_SENSOR_STATE_SLEEP = 0x02, > + IMX728_SENSOR_STATE_STANDBY = 0x04, > + IMX728_SENSOR_STATE_STREAMING = 0x10, > + IMX728_SENSOR_STATE_SAFE = 0x20, > +}; > + > + > +enum imx728_remap_mode_id { > + IMX728_REMAP_MODE_STANDBY = 0x00, > + IMX728_REMAP_MODE_STANDBY_PIXEL_SHADING_COMPENSATION = 0x01, > + IMX728_REMAP_MODE_STANDBY_SPOT_PIXEL_COMPENSATION = 0x02, > + IMX728_REMAP_MODE_STREAMING = 0x04, > + IMX728_REMAP_MODE_STREAMING_PIXEL_SHADING_COMPENSATION = 0x05, > + IMX728_REMAP_MODE_STREAMING_SPOT_PIXEL_COMPENSATION = 0x06, > + IMX728_REMAP_MODE_SLEEP = 0x20, > +}; > + > +enum imx728_drive_mode { > + IMX728_MODE_3856x2176_45_4LANE_RAW10 = 0x01, > + IMX728_MODE_3856x2176_45_4LANE_RAW12 = 0x02, > + IMX728_MODE_3856x2176_45_4LANE_RAW16 = 0x03, > + IMX728_MODE_3856x2176_45_4LANE_RAW20 = 0x04, > + IMX728_MODE_3856x2176_45_4LANE_RAW12_HDR = 0x05, > + IMX728_MODE_3856x2176_40_4LANE_RAW10 = 0x11, > + IMX728_MODE_3856x2176_40_4LANE_RAW12 = 0x12, > + IMX728_MODE_3856x2176_40_4LANE_RAW16 = 0x13, > + IMX728_MODE_3856x2176_40_4LANE_RAW20 = 0x14, > + IMX728_MODE_3856x2176_40_4LANE_RAW12_HDR = 0x16, > +}; > + > +enum imx728_awbmode { > + IMX728_AWBMODE_ATW = 0, > + IMX728_AWBMODE_ALL_PULL_IN = 1, > + IMX728_AWBMODE_USER_PRESET = 2, > + IMX728_AWBMODE_FULL_MWB = 3, > + IMX728_AWBMODE_HOLD = 4, > +}; > + > +enum imx728_img_raw_mode { > + IMX728_IMG_MODE_LINEAR = 0x0, > + IMX728_IMG_MODE_LI = 0x1, > + IMX728_IMG_MODE_HDR = 0x2, > + IMX728_IMG_MODE_LI_HDR = 0x3, > +}; > + > +enum imx728_aemode { > + IMX728_AEMODE_AE_AUTO = 0, > + IMX728_AEMODE_AE_HOLD = 1, > + IMX728_AEMODE_SCALE_ME = 2, > + IMX728_AEMODE_FULL_ME = 3, > +}; > + > +enum imx728_fme_shtval_unit { > + IMX728_FME_SHTVAL_UNIT_LINES = 1, > + IMX728_FME_SHTVAL_UNIT_MICROSECONDS = 3, > + IMX728_FME_SHTVAL_UNIT_FRAMES = 4, > +}; > + > +enum imx728_linear_raw_sel { > + IMX728_RAW_SEL_SP1H = 0x0, > + IMX728_RAW_SEL_SP1L = 0x1, > + IMX728_RAW_SEL_SP1EC = 0x2, > + IMX728_RAW_SEL_SP2 = 0x3, > + IMX728_RAW_SEL_SP1VS = 0x4 > +}; > + > +enum imx728_binn_avg { > + IMX728_BINN_SIMPLE_AVG, > + IMX728_BINN_WEIGHTED_AVG, > +}; > + > +struct imx728_ctrl { > + struct v4l2_ctrl_handler handler; > + struct v4l2_ctrl *wdr; > + struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *again; > + struct v4l2_ctrl *h_flip; > + struct v4l2_ctrl *v_flip; > + struct v4l2_ctrl *pg_mode; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *link_freq; > +}; > + > +struct imx728_ctrl_point { What does ctrl_point mean? What is x and y? > + int x, y; > +}; > + > +/* > + * struct imx728 - imx728 device structure > + * @dev: Device handle > + * @clk: Pointer to imx728 clock > + * @client: Pointer to I2C client > + * @regmap: Pointer to regmap structure > + * @xclr_gpio: Pointer to XCLR gpio > + * @subdev: V4L2 subdevice structure > + * @format: V4L2 media bus frame format structure > + * (width and height are in sync with the compose rect) > + * @pad: Media pad structure > + * @ctrl: imx728 control structure > + * @clk_rate: Frequency of imx728 clock > + * @lock: Mutex structure for V4L2 ctrl handler > + * @streaming: Flag to store streaming on/off status > + */ > +struct imx728 { > + struct device *dev; > + > + struct clk *clk; > + struct i2c_client *client; > + struct regmap *regmap; > + struct gpio_desc *xclr_gpio; > + > + struct v4l2_subdev subdev; > + struct v4l2_mbus_framefmt format; > + struct media_pad pad; > + > + struct imx728_ctrl ctrl; > + > + unsigned long clk_rate; > + u32 fps; > + > + struct mutex lock; > + bool streaming; > +}; > + > +static const struct v4l2_area imx728_framesizes[] = { > + { > + .width = IMX728_OUT_WIDTH, > + .height = IMX728_OUT_HEIGHT, > + }, Are you sure this is the only supported resolution? I would prefer using actual numbers here. > +}; > + > +static const u32 imx728_mbus_formats[] = { > + MEDIA_BUS_FMT_SRGGB10_1X10, > +}; > + > +static const s64 imx728_link_freq_menu[] = { > + IMX728_LINK_FREQ, > +}; > + > +static const struct regmap_config imx728_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > +}; > + > +static const char *const imx728_ctrl_pg_qmenu[] = { > + "Disabled", > + "Horizontal Color Bars", > + "Vertical Color Bars", > +}; > + > +static struct imx728_ctrl_point imx728_hdr_20bit[] = { > + {0, 0}, > + {1566 >> 4, 938}, > + {105740 >> 4, 1863}, > + {387380 >> 4, 2396}, > + {3818601 >> 4, 3251}, > + {16777215 >> 4, 4095}, > + {-1, -1} > +}; > + > +static const struct reg_sequence imx728_3840x2160[] = { Please use struct cci_reg_sequence. Best regards, Alexander > + {0xFFFF, 0x00, 1000}, > + {0x1749, 0x01}, > + {0x174B, 0x01}, > [snip] > +}; > -- > 2.40.1 > > Please be aware that this email includes email addresses outside of the organization. > >
Hi Spencer Thanks for the patch - always nice to see new sensors being supported. I assume there's no public datasheet for the sensor, but are you aware of even a product brief? Reviews are tricky without data to work with. I've just read through looking for discrepancies and common errors. On Wed, 26 Jun 2024 at 22:16, Spencer Hill <shill@d3engineering.com> wrote: > > Add a driver for the Sony IMX728 image sensor. > > Signed-off-by: Spencer Hill <shill@d3engineering.com> > --- > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx728.c | 1167 ++++++++++++ > drivers/media/i2c/imx728.h | 3458 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 4637 insertions(+) > create mode 100644 drivers/media/i2c/imx728.c > create mode 100644 drivers/media/i2c/imx728.h > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index c6d3ee472d81..46b6463c558a 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -233,6 +233,17 @@ config VIDEO_IMX415 > To compile this driver as a module, choose M here: the > module will be called imx415. > > +config VIDEO_IMX728 > + tristate "Sony IMX728 sensor support" > + depends on OF_GPIO > + select V4L2_CCI_I2C > + help > + This is a Video4Linux2 sensor driver for the Sony > + IMX728 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx728. > + > config VIDEO_MAX9271_LIB > tristate > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index dfbe6448b549..1188420ee1b4 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > obj-$(CONFIG_VIDEO_IMX412) += imx412.o > obj-$(CONFIG_VIDEO_IMX415) += imx415.o > +obj-$(CONFIG_VIDEO_IMX728) += imx728.o > obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o > obj-$(CONFIG_VIDEO_KS0127) += ks0127.o > diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c > new file mode 100644 > index 000000000000..b23359133a22 > --- /dev/null > +++ b/drivers/media/i2c/imx728.c > @@ -0,0 +1,1167 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Sony IMX728 CMOS Image Sensor Driver > + * > + * Copyright (c) 2024 Define Design Deploy Corp > + */ > + > +#include <linux/delay.h> > +#include <linux/clk.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/v4l2-mediabus.h> > +#include <linux/videodev2.h> > +#include <media/v4l2-subdev.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > + > +#include "imx728.h" > + > +static inline struct imx728 *to_imx728(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct imx728, subdev); > +} > + > +static int imx728_read(struct imx728 *imx728, u16 addr, u32 *val, size_t nbytes) > +{ > + int ret; > + __le32 val_le = 0; > + > + ret = regmap_bulk_read(imx728->regmap, addr, &val_le, nbytes); > + if (ret < 0) { > + dev_err(imx728->dev, "%s: failed to read reg 0x%04x: %d\n", > + __func__, addr, ret); > + return ret; > + } > + > + *val = le32_to_cpu(val_le); > + > + return 0; > +} > + > +static int imx728_write(struct imx728 *imx728, u16 addr, u32 val, size_t nbytes) > +{ > + int ret; > + __le32 val_le = cpu_to_le32(val); > + > + ret = regmap_bulk_write(imx728->regmap, addr, &val_le, nbytes); > + if (ret < 0) > + dev_err(imx728->dev, "%s: failed to write reg 0x%04x: %d\n", > + __func__, addr, ret); > + > + return ret; > +} > + > +static int imx728_update_bits(struct imx728 *imx728, u16 addr, u32 val, u32 mask, size_t nbytes) > +{ > + int ret; > + u32 cfg; > + > + ret = imx728_read(imx728, addr, &cfg, nbytes); > + if (ret < 0) > + return ret; > + > + cfg = (val) ? (cfg | mask) : (cfg & (~mask)); > + > + return imx728_write(imx728, addr, cfg, nbytes); > +} > + > +static int imx728_write_table(struct imx728 *imx728, > + const struct reg_sequence *regs, > + unsigned int nr_regs) > +{ > + int ret; > + > + ret = regmap_multi_reg_write(imx728->regmap, regs, nr_regs); > + if (ret < 0) > + dev_err(imx728->dev, > + "%s: failed to write reg table (%d)!\n", __func__, ret); > + return ret; > +} > + > +static int imx728_wait_for_state(struct imx728 *imx728, enum imx728_sensor_state state) > +{ > + int ret, i; > + u32 val; > + > + for (i = 0; i < 50; i++) { > + ret = imx728_read(imx728, 0x2CAC, &val, 1); > + if (ret == 0 && val == state) { > + dev_info(imx728->dev, "%s: Enter state %u\n", __func__, val); > + return 0; > + } > + usleep_range(1000, 10000); > + } > + > + return -EBUSY; > +} > + > +static void imx728_init_formats(struct v4l2_subdev_state *state) > +{ > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_subdev_state_get_format(state, 0, 0); > + format->code = imx728_mbus_formats[0]; > + format->width = imx728_framesizes[0].width; > + format->height = imx728_framesizes[0].height; > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_SMPTE170M; > +} > + > +static int _imx728_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_route routes[] = { > + { > + .source_pad = 0, > + .source_stream = 0, > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + }, > + { > + .source_pad = 0, > + .source_stream = 1, > + } > + }; > + > + struct v4l2_subdev_krouting routing = { > + .num_routes = ARRAY_SIZE(routes), > + .routes = routes, > + }; > + > + int ret; > + > + ret = v4l2_subdev_set_routing(sd, state, &routing); > + if (ret < 0) > + return ret; > + > + imx728_init_formats(state); > + > + return 0; > +} > + > +static int imx728_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + > + if (code->index >= ARRAY_SIZE(imx728_mbus_formats)) imx728_mbus_formats only lists MEDIA_BUS_FMT_SRGGB10_1X10. Is this for potential addition of the 12, 16, or 20 bit readout modes? How likely are those to actually happen? > + return -EINVAL; > + > + code->code = imx728_mbus_formats[code->index]; > + > + return 0; > +} > + > +static int imx728_enum_frame_sizes(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > + if (imx728_mbus_formats[i] == fse->code) > + break; > + } > + > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > + return -EINVAL; > + > + if (fse->index >= ARRAY_SIZE(imx728_framesizes)) > + return -EINVAL; > + > + fse->min_width = imx728_framesizes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->min_height = imx728_framesizes[fse->index].height; > + fse->max_height = fse->min_height; > + > + return 0; > +} > + > +static int imx728_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + struct v4l2_mbus_framefmt *format; > + const struct v4l2_area *fsize; > + unsigned int i; > + u32 code; > + int ret = 0; > + > + if (fmt->pad != 0) > + return -EINVAL; > + > + if (fmt->stream != 0) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > + if (imx728_mbus_formats[i] == fmt->format.code) { > + code = fmt->format.code; > + break; > + } > + } > + > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > + code = imx728_mbus_formats[0]; > + > + fsize = v4l2_find_nearest_size(imx728_framesizes, > + ARRAY_SIZE(imx728_framesizes), width, > + height, fmt->format.width, > + fmt->format.height); > + > + mutex_lock(&imx728->lock); > + > + format = v4l2_subdev_state_get_format(state, fmt->pad, fmt->stream); > + > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx728->streaming) { > + ret = -EBUSY; > + goto done; > + } > + > + format->code = code; > + format->width = fsize->width; > + format->height = fsize->height; > + > + fmt->format = *format; > + > +done: > + mutex_unlock(&imx728->lock); > + > + return ret; > +} > + > +static int imx728_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + struct v4l2_subdev_state *state; > + struct v4l2_mbus_framefmt *fmt; > + u32 bpp; > + int ret = 0; > + > + if (pad != 0) > + return -EINVAL; > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + fmt = v4l2_subdev_state_get_format(state, 0, 0); > + if (!fmt) { > + ret = -EPIPE; > + goto out; > + } > + > + memset(fd, 0, sizeof(*fd)); > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > + > + bpp = 10; > + > + fd->entry[fd->num_entries].stream = 0; > + > + fd->entry[fd->num_entries].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; > + fd->entry[fd->num_entries].length = fmt->width * fmt->height * bpp / 8; > + fd->entry[fd->num_entries].pixelcode = fmt->code; > + fd->entry[fd->num_entries].bus.csi2.vc = 0; > + fd->entry[fd->num_entries].bus.csi2.dt = 0x2b; Presumably this 0x2b is MIPI_CSI2_DT_RAW10. That comes back to the question above as to whether supporting other bit depths is really likely, as if so then prepare to change this value based on mbus_code. > + > + fd->num_entries++; Presumably this fd->num_entries manipulation is due to expecting to support multiple data types? Whilst not wrong, at the moment it feels a little redundant. > + > +out: > + > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > + > +static int imx728_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + enum v4l2_subdev_format_whence which, > + struct v4l2_subdev_krouting *routing) > +{ > + int ret; > + > + if (routing->num_routes == 0 || routing->num_routes > 1) > + return -EINVAL; > + > + ret = _imx728_set_routing(sd, state); > + > + return ret; > +} > + > +static int imx728_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx728 *imx728 = container_of(ctrl->handler, > + struct imx728, ctrl.handler); > + int ret = 0; > + > + dev_dbg(imx728->dev, "%s: %s, value: %d\n", > + __func__, ctrl->name, ctrl->val); > + > + if (!pm_runtime_get_if_in_use(imx728->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE: > + ret = imx728_write(imx728, 0x98DC, ctrl->val, 4); > + break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = imx728_update_bits(imx728, 0x98F8, > + (ctrl->val * 10), > + 0x1FFFF, 4); > + break; > + case V4L2_CID_HFLIP: > + ret = imx728_update_bits(imx728, 0x9651, > + ctrl->val, 0x1, 1); > + ret |= imx728_update_bits(imx728, 0xB67C, > + ctrl->val, 0x1, 1); > + break; > + case V4L2_CID_VFLIP: > + ret = imx728_update_bits(imx728, 0x9651, > + ctrl->val << 1, 0x2, 1); > + ret = imx728_update_bits(imx728, 0xB67D, > + ctrl->val, 0x1, 1); > + break; > + case V4L2_CID_WIDE_DYNAMIC_RANGE: > + case V4L2_CID_TEST_PATTERN: > + // Both of these are configured during start stream. > + ret = 0; > + break; > + default: > + ret = -EINVAL; > + } > + > + pm_runtime_put_noidle(imx728->dev); > + return ret; > +} > + > +static int imx728_detect(struct imx728 *imx728) > +{ > + int ret; > + u32 minor, major; > + > + ret = imx728_read(imx728, 0x6002, &major, 1); > + if (ret != 0) { > + dev_err(imx728->dev, "Could not read PARAM_MAJOR_VER!"); > + return ret; > + } > + ret = imx728_read(imx728, 0x6000, &minor, 1); > + if (ret != 0) { > + dev_err(imx728->dev, "Could not read PARAM_MINOR_VER!"); > + return ret; > + } > + dev_dbg(imx728->dev, "Got version: %d.%d", major, minor); > + > + return 0; > +} > + > +static int imx728_reset(struct imx728 *imx728) > +{ > + > + if (!IS_ERR_OR_NULL(imx728->xclr_gpio)) { > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > + usleep_range(1000, 10000); > + gpiod_set_value_cansleep(imx728->xclr_gpio, 0); > + msleep(100); > + > + return 0; > + } > + > + return -1; > +} > + > +static int imx728_power_on(struct imx728 *imx728) > +{ > + int ret; > + > + ret = clk_prepare_enable(imx728->clk); > + if (ret < 0) > + return ret; > + > + imx728_reset(imx728); > + return 0; > +} > + > +static int imx728_power_off(struct imx728 *imx728) > +{ > + > + if (imx728->xclr_gpio) { > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > + > + usleep_range(1, 10); > + } > + clk_disable_unprepare(imx728->clk); > + return 0; > +} > + > +static int imx728_get_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + > + fi->interval.numerator = 1; > + fi->interval.denominator = imx728->fps; > + return 0; > +} > + > +static int imx728_set_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + u32 req_fps; > + > + mutex_lock(&imx728->lock); > + > + if (fi->interval.numerator == 0 || fi->interval.denominator == 0) { > + fi->interval.denominator = IMX728_FRAMERATE_DEFAULT; > + fi->interval.numerator = 1; > + } > + > + req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator, > + fi->interval.numerator), > + IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX); > + > + fi->interval.numerator = 1; > + fi->interval.denominator = req_fps; > + > + imx728->fps = req_fps; > + > + mutex_unlock(&imx728->lock); > + dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps); Is setting the frame rate actually supported in the driver? I can't see imx728->fps being used other than to return the value in imx728_get_frame_interval. Frame rate control on raw sensors is normally controlled through V4L2_CID_VBLANK and V4L2_CID_HBLANK, not these ioctls. See https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#raw-camera-sensors > + > + return 0; > +} > + > +static int imx728_powerup_to_standby(struct imx728 *imx728) > +{ > + int ret; > + > + dev_info(imx728->dev, "powerup -> standby..."); > + > + ret = imx728_reset(imx728); > + if (ret) { > + dev_err(imx728->dev, "Error resetting: %i", ret); > + return ret; > + } > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_SLEEP); > + if (ret < 0) { > + dev_err(imx728->dev, "Could not transition to Sleep state!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B20, imx728->clk_rate / 1000000, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B1C, 0x1, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B05, 0xFF, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write to CK_SLEEP!"); > + return ret; > + } > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't transition from Sleep to Standby state!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xFFFF, IMX728_REMAP_MODE_STANDBY, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write regmap mode!"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_hdr_fixed_brightness(struct imx728 *imx728) > +{ > + int ret; > + > + // Sony recommended values > + unsigned int exposure_sp1_sp2_us = 10000; > + unsigned int exposure_sp1vs_us = 56; > + unsigned int sp1h_gain = 240; > + unsigned int sp1l_gain = 75; > + unsigned int sp1ec_gain = 21; > + unsigned int sp2_gain = 33; > + unsigned int sp1vs_gain = 84; > + > + ret = imx728_write(imx728, 0x98DC, exposure_sp1_sp2_us, 4); > + ret |= imx728_write(imx728, 0x98E4, exposure_sp1_sp2_us, 4); > + ret |= imx728_write(imx728, 0x98EC, exposure_sp1vs_us, 4); > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write fixed exposure values."); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x98F8, > + sp1h_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x98FC, > + sp1l_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9900, > + sp1ec_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9904, > + sp2_gain, > + 0x1FFFF, > + 4); > + ret |= imx728_update_bits(imx728, 0x9908, > + sp1vs_gain, > + 0x1FFFF, > + 4); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write fixed gain values."); > + return ret; > + } > + > + dev_info(imx728->dev, "Wrote fixed brightness for HDR"); > + > + return 0; > +} > + > +static int imx728_hdr_configure( > + struct imx728 *imx728, > + struct imx728_ctrl_point *points, > + int hdr_bits) > +{ > + u32 hdr_norm_x0; > + u32 hdr_norm_x1; > + u32 hdr_norm_y0; > + u32 hdr_norm_y1; > + > + int ret; > + int i; > + > + switch (hdr_bits) { > + case 20: > + hdr_norm_x0 = 0x2000; > + hdr_norm_x1 = 0x5000; > + > + hdr_norm_y0 = 0x0; > + hdr_norm_y1 = 0xd000; > + break; > + case 24: > + hdr_norm_x0 = 0x3000; > + hdr_norm_x1 = 0x5000; > + > + hdr_norm_y0 = 0x0; > + hdr_norm_y1 = 0xe000; > + break; > + default: > + dev_err(imx728->dev, "%i bit HDR not supported.", hdr_bits); > + break; > + } > + > + ret = imx728_write(imx728, 0x9C60, hdr_norm_x0, 4); > + ret |= imx728_write(imx728, 0x9C6C, hdr_norm_x0, 4); > + ret |= imx728_write(imx728, 0x9C64, hdr_norm_x1, 4); > + ret |= imx728_write(imx728, 0x9C70, hdr_norm_x1, 4); > + ret |= imx728_write(imx728, 0x9C68, hdr_norm_y0, 2); > + ret |= imx728_write(imx728, 0x9C74, hdr_norm_y0, 2); > + ret |= imx728_write(imx728, 0x9C6A, hdr_norm_y1, 2); > + ret |= imx728_write(imx728, 0x9C76, hdr_norm_y1, 2); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed when setting HDR Normalization gains: %i", ret); > + return ret; > + } > + > + for (i = 0; i < 16; i++) { > + ret = imx728_write( > + imx728, > + IMX728_REG_CTRL_POINT_X(i), > + points->x, 4); > + > + ret |= imx728_write( > + imx728, > + IMX728_REG_CTRL_POINT_Y(i), > + points->y, 4); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to write control point %i", i); > + return ret; > + } > + > + if ((points+1)->x >= 0 && (points+1)->y >= 0) > + points++; > + } > + > + return imx728_hdr_fixed_brightness(imx728); > +} > + > +static int imx728_configure(struct imx728 *imx728) > +{ > + int ret; > + bool hdr = imx728->ctrl.wdr->val; > + enum imx728_img_raw_mode img_out_mode; > + enum imx728_drive_mode mode_sel; > + enum imx728_aemode ae_mode; > + > + if (hdr) { > + img_out_mode = IMX728_IMG_MODE_HDR; > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > + ae_mode = IMX728_AEMODE_FULL_ME; > + } else { > + img_out_mode = IMX728_IMG_MODE_LINEAR; > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > + ae_mode = IMX728_AEMODE_FULL_ME; Unless my eyes deceive me, mode_sel and ae_mode are the same in both modes. Why do they need to be conditional? > + } > + > + ret = imx728_write(imx728, 0x98AC, ae_mode, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't set AE mode!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xA248, IMX728_AWBMODE_FULL_MWB, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't set full manual white balance mode!"); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x1808, 0x1, 0x1, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't enable full manual white balance mode!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98E0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98E8, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x98F0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > + return ret; > + } > + > + if (hdr) { The xclr GPIO is optional, so is there a guaranteed register reset in the big table to reset these HDR registers back to defaults? > + ret = imx728_hdr_configure(imx728, imx728_hdr_20bit, 20); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't configure sensor for HDR mode: %i", ret); > + return ret; > + } > + } > + > + dev_info(imx728->dev, "Disabling metadata..."); > + ret = imx728_write(imx728, 0x1708, 0x00, 1); > + ret |= imx728_write(imx728, 0x1709, 0x00, 1); > + ret |= imx728_write(imx728, 0x170A, 0x00, 1); > + ret |= imx728_write(imx728, 0x1B40, 0x00, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error disabling metadata: %i", ret); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0x9728, > + mode_sel, > + 0x7FFF, > + 2); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't write mode select."); > + return ret; > + } > + > + ret = imx728_update_bits(imx728, 0xEC7E, > + img_out_mode, > + 0x7, > + 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't select image out mode."); > + return ret; > + } > + > + ret = imx728_write(imx728, 0xEC12, 0x28, 2); > + ret |= imx728_write(imx728, 0xEC14, 0x0, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error disabling OB output."); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1761, 0x0, 1); > + if (ret < 0) > + dev_err(imx728->dev, "Error disabling skew calibration from sensor to SER"); > + > + switch (imx728->ctrl.pg_mode->val) { Even if the test mode can't be changed whilst running, this could be in imx728_set_ctrl as it'll be called by __v4l2_ctrl_handler_setup > + case 0: Again xclr GPIO is optional. Are these reset to defaults? > + break; > + case 1: > + // Horizontal Color Bars > + ret = imx728_write(imx728, 0x1A2A, 8, 2); > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > + ret |= imx728_write(imx728, 0xB58F, 0x0, 1); > + ret |= imx728_write(imx728, 0xB6C5, 0x0, 1); > + break; > + case 2: > + // Vertical Color Bars > + ret = imx728_write(imx728, 0x1A2C, 16, 2); > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > + ret |= imx728_write(imx728, 0xB58F, 0x1, 1); > + ret |= imx728_write(imx728, 0xB6C5, 0x1, 1); > + break; > + default: > + break; > + } > + > + // Assume that everything except 'disabled' requires pattern gen enable > + if (imx728->ctrl.pg_mode->val != 0) { > + ret |= imx728_write(imx728, 0xB58E, 0x1, 1); > + ret |= imx728_write(imx728, 0xB6C4, 0x1, 1); > + > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to enable PG mode."); > + return ret; > + } > + } > + > + ret = imx728_update_bits(imx728, 0x9714, > + IMX728_RAW_SEL_SP1H, > + 0x7, > + 1); > + ret |= imx728_update_bits(imx728, 0xB684, > + IMX728_RAW_SEL_SP1H, > + 0x7, > + 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Failed to set subpixel register"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_start_stream(struct imx728 *imx728) > +{ > + int ret; > + > + ret = imx728_powerup_to_standby(imx728); > + if (ret < 0) > + return ret; > + > + ret = imx728_write_table(imx728, imx728_3840x2160, ARRAY_SIZE(imx728_3840x2160)); You've enabled delayed suspend through pm_runtime, but you're always sending the whole of this array. Is there an option for only resending on actual power up to reduce the time to restart if the device hasn't suspended? > + if (ret < 0) > + return ret; > + > + msleep(100); > + > + ret = imx728_configure(imx728); > + if (ret < 0) > + return ret; > + > + ret = __v4l2_ctrl_handler_setup(imx728->subdev.ctrl_handler); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: failed to apply v4l2 ctrls: %d\n", __func__, ret); > + return ret; > + } > + > + ret = imx728_write(imx728, 0x1B04, 0x5C, 1); > + if (ret < 0) > + return ret; > + ret = imx728_write(imx728, 0x1B04, 0xA3, 1); > + if (ret < 0) > + return ret; > + > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STREAMING); > + if (ret < 0) { > + dev_err(imx728->dev, "Could not transition to Streaming!"); > + return ret; > + } > + > + msleep(20); > + > + return 0; > +} > + > +static int imx728_stop_stream(struct imx728 *imx728) > +{ > + int ret; > + > + ret = imx728_write(imx728, 0x1B04, 0xFF, 1); > + if (ret < 0) { > + dev_err(imx728->dev, "Error sending stop stream command: %i", ret); > + return ret; > + } > + > + imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > + if (ret < 0) { > + dev_err(imx728->dev, "Couldn't transition out of Streaming mode!"); > + return ret; > + } > + > + return 0; > +} > + > +static int imx728_set_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + mutex_lock(&imx728->lock); > + if (imx728->streaming == enable) { > + mutex_unlock(&imx728->lock); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_get_sync(imx728->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(imx728->dev); > + goto err_unlock; > + } > + > + ret = imx728_start_stream(imx728); > + if (ret < 0) > + goto err_runtime_put; > + } else { > + ret = imx728_stop_stream(imx728); > + if (ret < 0) > + goto err_runtime_put; > + pm_runtime_mark_last_busy(imx728->dev); > + pm_runtime_put_autosuspend(imx728->dev); > + } > + > + imx728->streaming = enable; > + > + __v4l2_ctrl_grab(imx728->ctrl.wdr, enable); > + __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable); > + __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable); > + __v4l2_ctrl_grab(imx728->ctrl.pg_mode, enable); > + > + mutex_unlock(&imx728->lock); > + return 0; > + > +err_runtime_put: > + pm_runtime_put(imx728->dev); > + > +err_unlock: > + mutex_unlock(&imx728->lock); > + dev_err(imx728->dev, > + "%s: failed to setup streaming %d\n", __func__, ret); > + return ret; > +} > + > +static const struct v4l2_subdev_core_ops imx728_core_ops = { > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > +}; > + > +static const struct v4l2_subdev_video_ops imx728_subdev_video_ops = { > + .s_stream = imx728_set_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops imx728_subdev_pad_ops = { > + .enum_mbus_code = imx728_enum_mbus_code, > + .enum_frame_size = imx728_enum_frame_sizes, > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = imx728_set_fmt, > + .get_frame_interval = imx728_get_frame_interval, > + .set_frame_interval = imx728_set_frame_interval, > + .set_routing = imx728_set_routing, > + .get_frame_desc = imx728_get_frame_desc, Support for get_selection would be nice to reflect the array geometry, assuming the information is in the datasheet. It's likely to be mandatory for libcamera soon. > +}; > + > +static const struct v4l2_subdev_ops imx728_subdev_ops = { > + .core = &imx728_core_ops, > + .video = &imx728_subdev_video_ops, > + .pad = &imx728_subdev_pad_ops, > +}; > + > +static const struct v4l2_ctrl_ops imx728_ctrl_ops = { > + .s_ctrl = imx728_set_ctrl, > +}; > + > +static int imx728_probe(struct i2c_client *client) > +{ > + struct imx728 *imx728; > + struct v4l2_subdev *sd; > + struct v4l2_ctrl_handler *ctrl_hdr; > + int ret; > + > + imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL); > + if (!imx728) > + return -ENOMEM; > + > + imx728->dev = &client->dev; > + > + imx728->regmap = devm_regmap_init_i2c(client, &imx728_regmap_config); > + if (IS_ERR(imx728->regmap)) > + return PTR_ERR(imx728->regmap); > + > + imx728->xclr_gpio = devm_gpiod_get_optional(imx728->dev, > + "xclr", GPIOD_OUT_LOW); > + if (IS_ERR(imx728->xclr_gpio)) > + return PTR_ERR(imx728->xclr_gpio); > + > + imx728->clk = devm_clk_get(imx728->dev, "inck"); > + if (IS_ERR(imx728->clk)) > + return PTR_ERR(imx728->clk); > + > + imx728->clk_rate = clk_get_rate(imx728->clk); > + dev_info(imx728->dev, "inck rate: %lu Hz\n", imx728->clk_rate); > + > + if (imx728->clk_rate < 18000000 || imx728->clk_rate > 30000000) > + return -EINVAL; > + > + ret = imx728_power_on(imx728); > + if (ret < 0) > + return ret; > + > + ret = imx728_detect(imx728); > + if (ret < 0) > + return ret; > + > + sd = &imx728->subdev; > + v4l2_i2c_subdev_init(sd, client, &imx728_subdev_ops); > + > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS | > + V4L2_SUBDEV_FL_STREAMS; > + > + imx728->pad.flags = MEDIA_PAD_FL_SOURCE; > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ret = media_entity_pads_init(&sd->entity, 1, &imx728->pad); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: media entity init failed %d\n", __func__, ret); > + return ret; > + } > + > + ctrl_hdr = &imx728->ctrl.handler; > + ret = v4l2_ctrl_handler_init(ctrl_hdr, 8); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: ctrl handler init failed: %d\n", __func__, ret); > + goto err_media_cleanup; > + } > + > + mutex_init(&imx728->lock); > + imx728->ctrl.handler.lock = &imx728->lock; > + imx728->fps = IMX728_FRAMERATE_DEFAULT; > + > + imx728->ctrl.exposure = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_EXPOSURE, 0, > + 33000, 1, > + IMX728_EXPOSURE_DEFAULT); That looks like you're clipping the max exposure time based on 30fps, and the units are usecs. Units on V4L2_CID_EXPOSURE are undefined, and normally expected to be lines for raw image sensors, but that then needs to know HBLANK and PIXEL_RATE. There is V4L2_CID_EXPOSURE_ABSOLUTE which has defined units of 100usecs, but I'm not sure if that is very useful in this case. > + > + imx728->ctrl.again = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, 0, > + 102, 1, > + 24); > + > + imx728->ctrl.wdr = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_WIDE_DYNAMIC_RANGE, > + 0, 1, 1, 1); > + > + imx728->ctrl.h_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_HFLIP, 0, 1, 1, 0); > + > + imx728->ctrl.v_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_VFLIP, 0, 1, 1, 0); Other Sony sensors end up changing the Bayer order based on flips. I just want to check that this isn't the case for this sensor, otherwise you need to set the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag and report the mbus code based on flips. > + > + imx728->ctrl.pg_mode = v4l2_ctrl_new_std_menu_items(ctrl_hdr, > + &imx728_ctrl_ops, V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(imx728_ctrl_pg_qmenu) - 1, > + 0, 0, imx728_ctrl_pg_qmenu); > + > + imx728->ctrl.pixel_rate = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_PIXEL_RATE, > + IMX728_PIXEL_RATE, > + IMX728_PIXEL_RATE, 1, > + IMX728_PIXEL_RATE); > + > + imx728->ctrl.link_freq = v4l2_ctrl_new_int_menu(ctrl_hdr, &imx728_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(imx728_link_freq_menu) - 1, 0, > + imx728_link_freq_menu); > + > + if (imx728->ctrl.link_freq) > + imx728->ctrl.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + imx728->subdev.ctrl_handler = ctrl_hdr; > + if (imx728->ctrl.handler.error) { > + ret = imx728->ctrl.handler.error; > + dev_err(imx728->dev, > + "%s: failed to add the ctrls: %d\n", __func__, ret); > + goto err_ctrl_free; > + } > + > + pm_runtime_set_active(imx728->dev); > + pm_runtime_enable(imx728->dev); > + pm_runtime_set_autosuspend_delay(imx728->dev, IMX728_PM_IDLE_TIMEOUT); > + pm_runtime_use_autosuspend(imx728->dev); > + pm_runtime_get_noresume(imx728->dev); > + > + ret = v4l2_subdev_init_finalize(sd); > + if (ret < 0) > + goto err_pm_disable; > + > + ret = v4l2_async_register_subdev(sd); > + if (ret < 0) { > + dev_err(imx728->dev, > + "%s: v4l2 subdev register failed %d\n", __func__, ret); > + goto err_subdev_cleanup; > + } > + > + dev_info(imx728->dev, "imx728 probed\n"); > + pm_runtime_mark_last_busy(imx728->dev); > + pm_runtime_put_autosuspend(imx728->dev); > + return 0; > + > +err_subdev_cleanup: > + v4l2_subdev_cleanup(&imx728->subdev); > + > +err_pm_disable: > + pm_runtime_dont_use_autosuspend(imx728->dev); > + pm_runtime_put_noidle(imx728->dev); > + pm_runtime_disable(imx728->dev); > + > +err_ctrl_free: > + v4l2_ctrl_handler_free(ctrl_hdr); > + mutex_destroy(&imx728->lock); > + > +err_media_cleanup: > + media_entity_cleanup(&imx728->subdev.entity); > + > + return ret; > +} > + > +static int __maybe_unused imx728_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + ret = imx728_power_on(imx728); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int __maybe_unused imx728_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + > + imx728_power_off(imx728); > + > + return 0; > +} > + > +static int __maybe_unused imx728_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + if (imx728->streaming) > + imx728_stop_stream(imx728); > + > + ret = pm_runtime_force_suspend(dev); > + if (ret < 0) > + dev_warn(dev, "%s: failed to suspend: %i\n", __func__, ret); > + > + return 0; > +} > + > +static int __maybe_unused imx728_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + int ret; > + > + ret = pm_runtime_force_resume(dev); > + if (ret < 0) > + dev_warn(dev, "%s: failed to resume: %i\n", __func__, ret); > + > + if (imx728->streaming) > + ret = imx728_start_stream(imx728); You're likely to get comment from Laurent on this one. He went through and removed all the suspend/resume handlers as they should really be handled by the CSI2 receiver driver, not the sensor driver. Resuming streaming in a random order on system resume is unlikely to work. > + > + if (ret < 0) { > + imx728_stop_stream(imx728); > + imx728->streaming = 0; > + return ret; > + } > + > + return 0; > +} > + > +static void imx728_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx728 *imx728 = to_imx728(sd); > + > + v4l2_async_unregister_subdev(sd); > + v4l2_ctrl_handler_free(&imx728->ctrl.handler); > + v4l2_subdev_cleanup(&imx728->subdev); > + media_entity_cleanup(&sd->entity); > + mutex_destroy(&imx728->lock); > + > + pm_runtime_disable(imx728->dev); > + pm_runtime_dont_use_autosuspend(imx728->dev); > + pm_runtime_set_suspended(imx728->dev); > +} > + > +static const struct dev_pm_ops imx728_pm_ops = { > + SET_RUNTIME_PM_OPS(imx728_runtime_suspend, > + imx728_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(imx728_suspend, imx728_resume) > +}; > + > +static const struct of_device_id imx728_dt_id[] = { > + { .compatible = "sony,imx728" }, > + { /* sentinel */ } > +}; > + > +MODULE_DEVICE_TABLE(of, imx728_dt_id); > + > +static struct i2c_driver imx728_i2c_driver = { > + .driver = { > + .name = "imx728", > + .of_match_table = of_match_ptr(imx728_dt_id), > + .pm = &imx728_pm_ops, > + }, > + .probe = imx728_probe, > + .remove = imx728_remove, > +}; > + > +module_i2c_driver(imx728_i2c_driver); > + > +MODULE_DESCRIPTION("Camera Sensor Driver for Sony IMX728"); > +MODULE_AUTHOR("Spencer Hill <shill@d3engineering.com>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/media/i2c/imx728.h b/drivers/media/i2c/imx728.h > new file mode 100644 > index 000000000000..6f320214b780 > --- /dev/null > +++ b/drivers/media/i2c/imx728.h > @@ -0,0 +1,3458 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Sony IMX728 CMOS Image Sensor Driver > + * > + * Copyright (c) 2024 Define Design Deploy Corp > + */ > + > +#include <linux/types.h> > + > +#define IMX728_OUT_WIDTH 3840 > +#define IMX728_OUT_HEIGHT 2160 https://leopardimaging.com/wp-content/uploads/pdf/LI-IMX728-9295-xxxH-_Datasheet.pdf lists IMX728 as 3857x2177 (strange to see odd numbers in a Bayer sensor). Do you have definitive numbers for the array size? Seeing as these defines are only used in imx728_framesizes, I'd stick the values directly in the structure. > + > +#define IMX728_FRAMERATE_MAX 30 > +#define IMX728_FRAMERATE_DEFAULT 30 > +#define IMX728_FRAMERATE_MIN 10 > + > +#define IMX728_PIXEL_RATE 225504000 225504000 / 3840 / 2160 = 27.18fps. How does that square with achieving 30fps? > +#define IMX728_LINK_FREQ 800000000 1.6Gbit/s per lane feels high. I assume it has been checked. > + > +#define IMX728_EXPOSURE_DEFAULT 10000 > + > +#define IMX728_PM_IDLE_TIMEOUT 1000 > + > + > +#define IMX728_REG_CTRL_POINT_X(i) (0xA198 + (i) * 8) > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4) > + > +enum imx728_sensor_state { > + IMX728_SENSOR_STATE_SLEEP = 0x02, > + IMX728_SENSOR_STATE_STANDBY = 0x04, > + IMX728_SENSOR_STATE_STREAMING = 0x10, > + IMX728_SENSOR_STATE_SAFE = 0x20, > +}; > + > + > +enum imx728_remap_mode_id { > + IMX728_REMAP_MODE_STANDBY = 0x00, > + IMX728_REMAP_MODE_STANDBY_PIXEL_SHADING_COMPENSATION = 0x01, > + IMX728_REMAP_MODE_STANDBY_SPOT_PIXEL_COMPENSATION = 0x02, > + IMX728_REMAP_MODE_STREAMING = 0x04, > + IMX728_REMAP_MODE_STREAMING_PIXEL_SHADING_COMPENSATION = 0x05, > + IMX728_REMAP_MODE_STREAMING_SPOT_PIXEL_COMPENSATION = 0x06, > + IMX728_REMAP_MODE_SLEEP = 0x20, > +}; > + > +enum imx728_drive_mode { > + IMX728_MODE_3856x2176_45_4LANE_RAW10 = 0x01, > + IMX728_MODE_3856x2176_45_4LANE_RAW12 = 0x02, > + IMX728_MODE_3856x2176_45_4LANE_RAW16 = 0x03, > + IMX728_MODE_3856x2176_45_4LANE_RAW20 = 0x04, > + IMX728_MODE_3856x2176_45_4LANE_RAW12_HDR = 0x05, > + IMX728_MODE_3856x2176_40_4LANE_RAW10 = 0x11, > + IMX728_MODE_3856x2176_40_4LANE_RAW12 = 0x12, > + IMX728_MODE_3856x2176_40_4LANE_RAW16 = 0x13, > + IMX728_MODE_3856x2176_40_4LANE_RAW20 = 0x14, > + IMX728_MODE_3856x2176_40_4LANE_RAW12_HDR = 0x16, > +}; > + > +enum imx728_awbmode { > + IMX728_AWBMODE_ATW = 0, > + IMX728_AWBMODE_ALL_PULL_IN = 1, > + IMX728_AWBMODE_USER_PRESET = 2, > + IMX728_AWBMODE_FULL_MWB = 3, > + IMX728_AWBMODE_HOLD = 4, > +}; > + > +enum imx728_img_raw_mode { > + IMX728_IMG_MODE_LINEAR = 0x0, > + IMX728_IMG_MODE_LI = 0x1, > + IMX728_IMG_MODE_HDR = 0x2, > + IMX728_IMG_MODE_LI_HDR = 0x3, > +}; > + > +enum imx728_aemode { > + IMX728_AEMODE_AE_AUTO = 0, > + IMX728_AEMODE_AE_HOLD = 1, > + IMX728_AEMODE_SCALE_ME = 2, > + IMX728_AEMODE_FULL_ME = 3, > +}; > + > +enum imx728_fme_shtval_unit { > + IMX728_FME_SHTVAL_UNIT_LINES = 1, > + IMX728_FME_SHTVAL_UNIT_MICROSECONDS = 3, > + IMX728_FME_SHTVAL_UNIT_FRAMES = 4, > +}; > + > +enum imx728_linear_raw_sel { > + IMX728_RAW_SEL_SP1H = 0x0, > + IMX728_RAW_SEL_SP1L = 0x1, > + IMX728_RAW_SEL_SP1EC = 0x2, > + IMX728_RAW_SEL_SP2 = 0x3, > + IMX728_RAW_SEL_SP1VS = 0x4 > +}; > + > +enum imx728_binn_avg { > + IMX728_BINN_SIMPLE_AVG, > + IMX728_BINN_WEIGHTED_AVG, > +}; Not used. > + > +struct imx728_ctrl { > + struct v4l2_ctrl_handler handler; > + struct v4l2_ctrl *wdr; > + struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *again; > + struct v4l2_ctrl *h_flip; > + struct v4l2_ctrl *v_flip; > + struct v4l2_ctrl *pg_mode; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *link_freq; > +}; > + > +struct imx728_ctrl_point { > + int x, y; > +}; > + > +/* > + * struct imx728 - imx728 device structure > + * @dev: Device handle > + * @clk: Pointer to imx728 clock > + * @client: Pointer to I2C client > + * @regmap: Pointer to regmap structure > + * @xclr_gpio: Pointer to XCLR gpio > + * @subdev: V4L2 subdevice structure > + * @format: V4L2 media bus frame format structure > + * (width and height are in sync with the compose rect) > + * @pad: Media pad structure > + * @ctrl: imx728 control structure > + * @clk_rate: Frequency of imx728 clock > + * @lock: Mutex structure for V4L2 ctrl handler > + * @streaming: Flag to store streaming on/off status > + */ > +struct imx728 { > + struct device *dev; > + > + struct clk *clk; > + struct i2c_client *client; > + struct regmap *regmap; > + struct gpio_desc *xclr_gpio; > + > + struct v4l2_subdev subdev; > + struct v4l2_mbus_framefmt format; > + struct media_pad pad; > + > + struct imx728_ctrl ctrl; > + > + unsigned long clk_rate; > + u32 fps; > + > + struct mutex lock; > + bool streaming; > +}; > + > +static const struct v4l2_area imx728_framesizes[] = { > + { > + .width = IMX728_OUT_WIDTH, > + .height = IMX728_OUT_HEIGHT, > + }, > +}; > + > +static const u32 imx728_mbus_formats[] = { > + MEDIA_BUS_FMT_SRGGB10_1X10, > +}; > + > +static const s64 imx728_link_freq_menu[] = { > + IMX728_LINK_FREQ, > +}; > + > +static const struct regmap_config imx728_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > +}; > + > +static const char *const imx728_ctrl_pg_qmenu[] = { > + "Disabled", > + "Horizontal Color Bars", > + "Vertical Color Bars", > +}; > + > +static struct imx728_ctrl_point imx728_hdr_20bit[] = { > + {0, 0}, > + {1566 >> 4, 938}, > + {105740 >> 4, 1863}, > + {387380 >> 4, 2396}, > + {3818601 >> 4, 3251}, > + {16777215 >> 4, 4095}, > + {-1, -1} > +}; > + > +static const struct reg_sequence imx728_3840x2160[] = { > + {0xFFFF, 0x00, 1000}, <snip> This is one heck of a table at 3268 register writes. Are they really all necessary, or are some setting registers to default values, or duplicated elsewhere in the driver? Thanks Dave
On Thu, Jun 27, 2024 at 04:03:36PM +0200, Alexander Stein wrote: > Hi Spencer, > > thanks for the patch. > > Just having a glimpse and giving some feedback. > > Am Mittwoch, 26. Juni 2024, 23:15:28 CEST schrieb Spencer Hill: > > Add a driver for the Sony IMX728 image sensor. > > > > Signed-off-by: Spencer Hill <shill@d3engineering.com> > > --- > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/imx728.c | 1167 ++++++++++++ > > drivers/media/i2c/imx728.h | 3458 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 4637 insertions(+) > > create mode 100644 drivers/media/i2c/imx728.c > > create mode 100644 drivers/media/i2c/imx728.h > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index c6d3ee472d81..46b6463c558a 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -233,6 +233,17 @@ config VIDEO_IMX415 > > To compile this driver as a module, choose M here: the > > module will be called imx415. > > > > +config VIDEO_IMX728 > > + tristate "Sony IMX728 sensor support" > > + depends on OF_GPIO > > + select V4L2_CCI_I2C > > + help > > + This is a Video4Linux2 sensor driver for the Sony > > + IMX728 camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called imx728. > > + > > config VIDEO_MAX9271_LIB > > tristate > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index dfbe6448b549..1188420ee1b4 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -56,6 +56,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > > obj-$(CONFIG_VIDEO_IMX412) += imx412.o > > obj-$(CONFIG_VIDEO_IMX415) += imx415.o > > +obj-$(CONFIG_VIDEO_IMX728) += imx728.o > > obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > > obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o > > obj-$(CONFIG_VIDEO_KS0127) += ks0127.o > > diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c > > new file mode 100644 > > index 000000000000..b23359133a22 > > --- /dev/null > > +++ b/drivers/media/i2c/imx728.c > > @@ -0,0 +1,1167 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Sony IMX728 CMOS Image Sensor Driver > > + * > > + * Copyright (c) 2024 Define Design Deploy Corp > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/clk.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <linux/v4l2-mediabus.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-event.h> > > + > > +#include "imx728.h" > > + > > +static inline struct imx728 *to_imx728(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct imx728, subdev); > > +} > > + > > +static int imx728_read(struct imx728 *imx728, u16 addr, u32 *val, size_t nbytes) > > Use cci_read instead and remove this function. > Will do. > > +{ > > + int ret; > > + __le32 val_le = 0; > > + > > + ret = regmap_bulk_read(imx728->regmap, addr, &val_le, nbytes); > > + if (ret < 0) { > > + dev_err(imx728->dev, "%s: failed to read reg 0x%04x: %d\n", > > + __func__, addr, ret); > > + return ret; > > + } > > + > > + *val = le32_to_cpu(val_le); > > + > > + return 0; > > +} > > + > > +static int imx728_write(struct imx728 *imx728, u16 addr, u32 val, size_t nbytes) > > Use cci_write instead and remove this function. > Will do. > > +{ > > + int ret; > > + __le32 val_le = cpu_to_le32(val); > > + > > + ret = regmap_bulk_write(imx728->regmap, addr, &val_le, nbytes); > > + if (ret < 0) > > + dev_err(imx728->dev, "%s: failed to write reg 0x%04x: %d\n", > > + __func__, addr, ret); > > + > > + return ret; > > +} > > + > > +static int imx728_update_bits(struct imx728 *imx728, u16 addr, u32 val, u32 mask, size_t nbytes) > > Use cci_update_bits instead and remove this function. > Will do. > > +{ > > + int ret; > > + u32 cfg; > > + > > + ret = imx728_read(imx728, addr, &cfg, nbytes); > > + if (ret < 0) > > + return ret; > > + > > + cfg = (val) ? (cfg | mask) : (cfg & (~mask)); > > + > > + return imx728_write(imx728, addr, cfg, nbytes); > > +} > > + > > +static int imx728_write_table(struct imx728 *imx728, > > + const struct reg_sequence *regs, > > + unsigned int nr_regs) > > Use cci_multi_reg_write instead and remove this function. > Will do. > > +{ > > + int ret; > > + > > + ret = regmap_multi_reg_write(imx728->regmap, regs, nr_regs); > > + if (ret < 0) > > + dev_err(imx728->dev, > > + "%s: failed to write reg table (%d)!\n", __func__, ret); > > + return ret; > > +} > > + > > +static int imx728_wait_for_state(struct imx728 *imx728, enum imx728_sensor_state state) > > +{ > > + int ret, i; > > + u32 val; > > + > > + for (i = 0; i < 50; i++) { > > + ret = imx728_read(imx728, 0x2CAC, &val, 1); > > Please add proper register defines using CCI_REG* macros. > Is using those macros in place in these functions OK? > > + if (ret == 0 && val == state) { > > + dev_info(imx728->dev, "%s: Enter state %u\n", __func__, val); > > + return 0; > > + } > > + usleep_range(1000, 10000); > > + } > > + > > + return -EBUSY; > > +} > > + > > +static void imx728_init_formats(struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_mbus_framefmt *format; > > + > > + format = v4l2_subdev_state_get_format(state, 0, 0); > > + format->code = imx728_mbus_formats[0]; > > + format->width = imx728_framesizes[0].width; > > + format->height = imx728_framesizes[0].height; > > + format->field = V4L2_FIELD_NONE; > > + format->colorspace = V4L2_COLORSPACE_SMPTE170M; > > Are you sure about this colorspace? I would have expected > V4L2_COLORSPACE_RAW, but I don't know any details on this hardware. > > Also set ycbcr_enc, quantization and xfer_func. > V4L2_COLORSPACE_RAW is probably more correct. I am not sure what would be correct for the ycbcr_enc, similar drivers seem to use V4L2_YCBCR_ENC_601, would that be correct here? > > +} > > + > > +static int _imx728_set_routing(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > Why this special variant with a underscore? Just move the code > into imx728_set_routing. > I will put this into the other function. > > +{ > > + struct v4l2_subdev_route routes[] = { > > + { > > + .source_pad = 0, > > + .source_stream = 0, > > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > + }, > > + { > > + .source_pad = 0, > > + .source_stream = 1, > > + } > > + }; > > + > > + struct v4l2_subdev_krouting routing = { > > + .num_routes = ARRAY_SIZE(routes), > > + .routes = routes, > > + }; > > + > > + int ret; > > + > > + ret = v4l2_subdev_set_routing(sd, state, &routing); > > + if (ret < 0) > > + return ret; > > + > > + imx728_init_formats(state); > > + > > + return 0; > > +} > > + > > +static int imx728_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + > > + if (code->index >= ARRAY_SIZE(imx728_mbus_formats)) > > + return -EINVAL; > > + > > + code->code = imx728_mbus_formats[code->index]; > > + > > + return 0; > > +} > > + > > +static int imx728_enum_frame_sizes(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > > + if (imx728_mbus_formats[i] == fse->code) > > + break; > > + } > > + > > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > > + return -EINVAL; > > + > > + if (fse->index >= ARRAY_SIZE(imx728_framesizes)) > > + return -EINVAL; > > + > > + fse->min_width = imx728_framesizes[fse->index].width; > > + fse->max_width = fse->min_width; > > + fse->min_height = imx728_framesizes[fse->index].height; > > + fse->max_height = fse->min_height; > > + > > + return 0; > > +} > > + > > +static int imx728_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + struct v4l2_mbus_framefmt *format; > > + const struct v4l2_area *fsize; > > + unsigned int i; > > + u32 code; > > + int ret = 0; > > + > > + if (fmt->pad != 0) > > + return -EINVAL; > > + > > + if (fmt->stream != 0) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > > + if (imx728_mbus_formats[i] == fmt->format.code) { > > + code = fmt->format.code; > > + break; > > + } > > + } > > + > > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > > + code = imx728_mbus_formats[0]; > > + > > + fsize = v4l2_find_nearest_size(imx728_framesizes, > > + ARRAY_SIZE(imx728_framesizes), width, > > + height, fmt->format.width, > > + fmt->format.height); > > + > > + mutex_lock(&imx728->lock); > > + > > + format = v4l2_subdev_state_get_format(state, fmt->pad, fmt->stream); > > + > > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx728->streaming) { > > + ret = -EBUSY; > > + goto done; > > + } > > + > > + format->code = code; > > + format->width = fsize->width; > > + format->height = fsize->height; > > This should be done before calling v4l2_subdev_state_get_format, no? > Also ycbcr_enc, quantization and xfer_func are missing. > I will move these to be before get_format, more similar to the imx290 driver. > > + > > + fmt->format = *format; > > + > > +done: > > + mutex_unlock(&imx728->lock); > > + > > + return ret; > > +} > > + > > +static int imx728_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + struct v4l2_subdev_state *state; > > + struct v4l2_mbus_framefmt *fmt; > > + u32 bpp; > > + int ret = 0; > > + > > + if (pad != 0) > > + return -EINVAL; > > + > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + > > + fmt = v4l2_subdev_state_get_format(state, 0, 0); > > + if (!fmt) { > > + ret = -EPIPE; > > + goto out; > > + } > > + > > + memset(fd, 0, sizeof(*fd)); > > + > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > + > > + bpp = 10; > > + > > + fd->entry[fd->num_entries].stream = 0; > > + > > + fd->entry[fd->num_entries].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; > > + fd->entry[fd->num_entries].length = fmt->width * fmt->height * bpp / 8; > > + fd->entry[fd->num_entries].pixelcode = fmt->code; > > + fd->entry[fd->num_entries].bus.csi2.vc = 0; > > + fd->entry[fd->num_entries].bus.csi2.dt = 0x2b; > > + > > + fd->num_entries++; > > + > > +out: > > + > > + v4l2_subdev_unlock_state(state); > > + > > + return ret; > > +} > > + > > +static int imx728_set_routing(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + enum v4l2_subdev_format_whence which, > > + struct v4l2_subdev_krouting *routing) > > +{ > > + int ret; > > + > > + if (routing->num_routes == 0 || routing->num_routes > 1) > > + return -EINVAL; > > + > > + ret = _imx728_set_routing(sd, state); > > + > > + return ret; > > +} > > + > > +static int imx728_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct imx728 *imx728 = container_of(ctrl->handler, > > + struct imx728, ctrl.handler); > > + int ret = 0; > > + > > + dev_dbg(imx728->dev, "%s: %s, value: %d\n", > > + __func__, ctrl->name, ctrl->val); > > + > > + if (!pm_runtime_get_if_in_use(imx728->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_EXPOSURE: > > + ret = imx728_write(imx728, 0x98DC, ctrl->val, 4); > > + break; > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = imx728_update_bits(imx728, 0x98F8, > > + (ctrl->val * 10), > > + 0x1FFFF, 4); > > + break; > > + case V4L2_CID_HFLIP: > > + ret = imx728_update_bits(imx728, 0x9651, > > + ctrl->val, 0x1, 1); > > + ret |= imx728_update_bits(imx728, 0xB67C, > > + ctrl->val, 0x1, 1); > > + break; > > + case V4L2_CID_VFLIP: > > + ret = imx728_update_bits(imx728, 0x9651, > > + ctrl->val << 1, 0x2, 1); > > + ret = imx728_update_bits(imx728, 0xB67D, > > + ctrl->val, 0x1, 1); > > + break; > > + case V4L2_CID_WIDE_DYNAMIC_RANGE: > > + case V4L2_CID_TEST_PATTERN: > > + // Both of these are configured during start stream. > > Are they not configurable while streaming? > My understanding is that this depends on the mode that the sensor is operating in. I believe in the current mode it should be possible to configure this during streaming. I will move the configuration into a new function and allow it to be configured while streaming. > > + ret = 0; > > + break; > > + default: > > + ret = -EINVAL; > > + } > > + > > + pm_runtime_put_noidle(imx728->dev); > > + return ret; > > +} > > + > > +static int imx728_detect(struct imx728 *imx728) > > +{ > > + int ret; > > + u32 minor, major; > > + > > + ret = imx728_read(imx728, 0x6002, &major, 1); > > + if (ret != 0) { > > + dev_err(imx728->dev, "Could not read PARAM_MAJOR_VER!"); > > + return ret; > > + } > > + ret = imx728_read(imx728, 0x6000, &minor, 1); > > + if (ret != 0) { > > + dev_err(imx728->dev, "Could not read PARAM_MINOR_VER!"); > > + return ret; > > + } > > + dev_dbg(imx728->dev, "Got version: %d.%d", major, minor); > > + > > + return 0; > > +} > > + > > +static int imx728_reset(struct imx728 *imx728) > > +{ > > + > > + if (!IS_ERR_OR_NULL(imx728->xclr_gpio)) { > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > > + usleep_range(1000, 10000); > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 0); > > + msleep(100); > > + > > + return 0; > > + } > > + > > + return -1; > > +} > > + > > +static int imx728_power_on(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = clk_prepare_enable(imx728->clk); > > + if (ret < 0) > > + return ret; > > + > > + imx728_reset(imx728); > > + return 0; > > +} > > + > > +static int imx728_power_off(struct imx728 *imx728) > > +{ > > + > > + if (imx728->xclr_gpio) { > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > > + > > + usleep_range(1, 10); > > + } > > + clk_disable_unprepare(imx728->clk); > > + return 0; > > +} > > + > > +static int imx728_get_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + fi->interval.numerator = 1; > > + fi->interval.denominator = imx728->fps; > > + return 0; > > +} > > + > > +static int imx728_set_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + u32 req_fps; > > + > > + mutex_lock(&imx728->lock); > > + > > + if (fi->interval.numerator == 0 || fi->interval.denominator == 0) { > > + fi->interval.denominator = IMX728_FRAMERATE_DEFAULT; > > + fi->interval.numerator = 1; > > + } > > + > > + req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator, > > + fi->interval.numerator), > > + IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX); > > + > > + fi->interval.numerator = 1; > > + fi->interval.denominator = req_fps; > > + > > + imx728->fps = req_fps; > > + > > + mutex_unlock(&imx728->lock); > > + dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps); > > + > > + return 0; > > +} > > + > > +static int imx728_powerup_to_standby(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + dev_info(imx728->dev, "powerup -> standby..."); > > + > > + ret = imx728_reset(imx728); > > + if (ret) { > > + dev_err(imx728->dev, "Error resetting: %i", ret); > > + return ret; > > + } > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_SLEEP); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Could not transition to Sleep state!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B20, imx728->clk_rate / 1000000, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B1C, 0x1, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > > Error message doesn't seem to match. This is a fixed write, independent from any frequency. > This write is the enable flag for the configured INCK frequency. By default the sensor ignores the user configured frequency until this is set. > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B05, 0xFF, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write to CK_SLEEP!"); > > + return ret; > > + } > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't transition from Sleep to Standby state!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xFFFF, IMX728_REMAP_MODE_STANDBY, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write regmap mode!"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_hdr_fixed_brightness(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + // Sony recommended values > > + unsigned int exposure_sp1_sp2_us = 10000; > > + unsigned int exposure_sp1vs_us = 56; > > + unsigned int sp1h_gain = 240; > > + unsigned int sp1l_gain = 75; > > + unsigned int sp1ec_gain = 21; > > + unsigned int sp2_gain = 33; > > + unsigned int sp1vs_gain = 84; > > + > > + ret = imx728_write(imx728, 0x98DC, exposure_sp1_sp2_us, 4); > > + ret |= imx728_write(imx728, 0x98E4, exposure_sp1_sp2_us, 4); > > + ret |= imx728_write(imx728, 0x98EC, exposure_sp1vs_us, 4); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write fixed exposure values."); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x98F8, > > + sp1h_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x98FC, > > + sp1l_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9900, > > + sp1ec_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9904, > > + sp2_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9908, > > + sp1vs_gain, > > + 0x1FFFF, > > + 4); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write fixed gain values."); > > + return ret; > > + } > > + > > + dev_info(imx728->dev, "Wrote fixed brightness for HDR"); > > + > > + return 0; > > +} > > + > > +static int imx728_hdr_configure( > > + struct imx728 *imx728, > > + struct imx728_ctrl_point *points, > > + int hdr_bits) > > +{ > > + u32 hdr_norm_x0; > > + u32 hdr_norm_x1; > > + u32 hdr_norm_y0; > > + u32 hdr_norm_y1; > > + > > + int ret; > > + int i; > > + > > + switch (hdr_bits) { > > + case 20: > > + hdr_norm_x0 = 0x2000; > > + hdr_norm_x1 = 0x5000; > > + > > + hdr_norm_y0 = 0x0; > > + hdr_norm_y1 = 0xd000; > > + break; > > + case 24: > > + hdr_norm_x0 = 0x3000; > > + hdr_norm_x1 = 0x5000; > > + > > + hdr_norm_y0 = 0x0; > > + hdr_norm_y1 = 0xe000; > > + break; > > + default: > > + dev_err(imx728->dev, "%i bit HDR not supported.", hdr_bits); > > + break; > > + } > > + > > + ret = imx728_write(imx728, 0x9C60, hdr_norm_x0, 4); > > + ret |= imx728_write(imx728, 0x9C6C, hdr_norm_x0, 4); > > + ret |= imx728_write(imx728, 0x9C64, hdr_norm_x1, 4); > > + ret |= imx728_write(imx728, 0x9C70, hdr_norm_x1, 4); > > + ret |= imx728_write(imx728, 0x9C68, hdr_norm_y0, 2); > > + ret |= imx728_write(imx728, 0x9C74, hdr_norm_y0, 2); > > + ret |= imx728_write(imx728, 0x9C6A, hdr_norm_y1, 2); > > + ret |= imx728_write(imx728, 0x9C76, hdr_norm_y1, 2); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed when setting HDR Normalization gains: %i", ret); > > + return ret; > > + } > > + > > + for (i = 0; i < 16; i++) { > > + ret = imx728_write( > > + imx728, > > + IMX728_REG_CTRL_POINT_X(i), > > + points->x, 4); > > + > > + ret |= imx728_write( > > + imx728, > > + IMX728_REG_CTRL_POINT_Y(i), > > + points->y, 4); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write control point %i", i); > > + return ret; > > + } > > + > > + if ((points+1)->x >= 0 && (points+1)->y >= 0) > > + points++; > > + } > > + > > + return imx728_hdr_fixed_brightness(imx728); > > +} > > + > > +static int imx728_configure(struct imx728 *imx728) > > +{ > > + int ret; > > + bool hdr = imx728->ctrl.wdr->val; > > + enum imx728_img_raw_mode img_out_mode; > > + enum imx728_drive_mode mode_sel; > > + enum imx728_aemode ae_mode; > > + > > + if (hdr) { > > + img_out_mode = IMX728_IMG_MODE_HDR; > > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > > + ae_mode = IMX728_AEMODE_FULL_ME; > > + } else { > > + img_out_mode = IMX728_IMG_MODE_LINEAR; > > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > > + ae_mode = IMX728_AEMODE_FULL_ME; > > + } > > + > > + ret = imx728_write(imx728, 0x98AC, ae_mode, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't set AE mode!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xA248, IMX728_AWBMODE_FULL_MWB, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't set full manual white balance mode!"); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x1808, 0x1, 0x1, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't enable full manual white balance mode!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98E0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98E8, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98F0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + if (hdr) { > > + ret = imx728_hdr_configure(imx728, imx728_hdr_20bit, 20); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't configure sensor for HDR mode: %i", ret); > > + return ret; > > + } > > + } > > + > > + dev_info(imx728->dev, "Disabling metadata..."); > > + ret = imx728_write(imx728, 0x1708, 0x00, 1); > > + ret |= imx728_write(imx728, 0x1709, 0x00, 1); > > + ret |= imx728_write(imx728, 0x170A, 0x00, 1); > > + ret |= imx728_write(imx728, 0x1B40, 0x00, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error disabling metadata: %i", ret); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x9728, > > + mode_sel, > > + 0x7FFF, > > + 2); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write mode select."); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0xEC7E, > > + img_out_mode, > > + 0x7, > > + 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't select image out mode."); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xEC12, 0x28, 2); > > + ret |= imx728_write(imx728, 0xEC14, 0x0, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error disabling OB output."); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1761, 0x0, 1); > > + if (ret < 0) > > + dev_err(imx728->dev, "Error disabling skew calibration from sensor to SER"); > > + > > + switch (imx728->ctrl.pg_mode->val) { > > + case 0: > > + break; > > + case 1: > > + // Horizontal Color Bars > > + ret = imx728_write(imx728, 0x1A2A, 8, 2); > > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > > + ret |= imx728_write(imx728, 0xB58F, 0x0, 1); > > + ret |= imx728_write(imx728, 0xB6C5, 0x0, 1); > > + break; > > + case 2: > > + // Vertical Color Bars > > + ret = imx728_write(imx728, 0x1A2C, 16, 2); > > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > > + ret |= imx728_write(imx728, 0xB58F, 0x1, 1); > > + ret |= imx728_write(imx728, 0xB6C5, 0x1, 1); > > + break; > > + default: > > + break; > > + } > > + > > + // Assume that everything except 'disabled' requires pattern gen enable > > + if (imx728->ctrl.pg_mode->val != 0) { > > + ret |= imx728_write(imx728, 0xB58E, 0x1, 1); > > + ret |= imx728_write(imx728, 0xB6C4, 0x1, 1); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to enable PG mode."); > > + return ret; > > + } > > + } > > + > > + ret = imx728_update_bits(imx728, 0x9714, > > + IMX728_RAW_SEL_SP1H, > > + 0x7, > > + 1); > > + ret |= imx728_update_bits(imx728, 0xB684, > > + IMX728_RAW_SEL_SP1H, > > + 0x7, > > + 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to set subpixel register"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_start_stream(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = imx728_powerup_to_standby(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_write_table(imx728, imx728_3840x2160, ARRAY_SIZE(imx728_3840x2160)); > > + if (ret < 0) > > + return ret; > > + > > + msleep(100); > > + > > + ret = imx728_configure(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = __v4l2_ctrl_handler_setup(imx728->subdev.ctrl_handler); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: failed to apply v4l2 ctrls: %d\n", __func__, ret); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B04, 0x5C, 1); > > + if (ret < 0) > > + return ret; > > + ret = imx728_write(imx728, 0x1B04, 0xA3, 1); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STREAMING); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Could not transition to Streaming!"); > > + return ret; > > + } > > + > > + msleep(20); > > + > > + return 0; > > +} > > + > > +static int imx728_stop_stream(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = imx728_write(imx728, 0x1B04, 0xFF, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error sending stop stream command: %i", ret); > > + return ret; > > + } > > + > > + imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't transition out of Streaming mode!"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_set_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + mutex_lock(&imx728->lock); > > + if (imx728->streaming == enable) { > > + mutex_unlock(&imx728->lock); > > + return 0; > > + } > > + > > + if (enable) { > > + ret = pm_runtime_get_sync(imx728->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(imx728->dev); > > + goto err_unlock; > > + } > > + > > + ret = imx728_start_stream(imx728); > > + if (ret < 0) > > + goto err_runtime_put; > > + } else { > > + ret = imx728_stop_stream(imx728); > > + if (ret < 0) > > + goto err_runtime_put; > > + pm_runtime_mark_last_busy(imx728->dev); > > + pm_runtime_put_autosuspend(imx728->dev); > > + } > > + > > + imx728->streaming = enable; > > + > > + __v4l2_ctrl_grab(imx728->ctrl.wdr, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.pg_mode, enable); > > + > > + mutex_unlock(&imx728->lock); > > + return 0; > > + > > +err_runtime_put: > > + pm_runtime_put(imx728->dev); > > + > > +err_unlock: > > + mutex_unlock(&imx728->lock); > > + dev_err(imx728->dev, > > + "%s: failed to setup streaming %d\n", __func__, ret); > > + return ret; > > +} > > + > > +static const struct v4l2_subdev_core_ops imx728_core_ops = { > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > +}; > > + > > +static const struct v4l2_subdev_video_ops imx728_subdev_video_ops = { > > + .s_stream = imx728_set_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops imx728_subdev_pad_ops = { > > + .enum_mbus_code = imx728_enum_mbus_code, > > + .enum_frame_size = imx728_enum_frame_sizes, > > + .get_fmt = v4l2_subdev_get_fmt, > > + .set_fmt = imx728_set_fmt, > > + .get_frame_interval = imx728_get_frame_interval, > > + .set_frame_interval = imx728_set_frame_interval, > > + .set_routing = imx728_set_routing, > > + .get_frame_desc = imx728_get_frame_desc, > > +}; > > + > > +static const struct v4l2_subdev_ops imx728_subdev_ops = { > > + .core = &imx728_core_ops, > > + .video = &imx728_subdev_video_ops, > > + .pad = &imx728_subdev_pad_ops, > > +}; > > + > > +static const struct v4l2_ctrl_ops imx728_ctrl_ops = { > > + .s_ctrl = imx728_set_ctrl, > > +}; > > + > > +static int imx728_probe(struct i2c_client *client) > > +{ > > + struct imx728 *imx728; > > + struct v4l2_subdev *sd; > > + struct v4l2_ctrl_handler *ctrl_hdr; > > + int ret; > > + > > + imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL); > > + if (!imx728) > > + return -ENOMEM; > > + > > + imx728->dev = &client->dev; > > + > > + imx728->regmap = devm_regmap_init_i2c(client, &imx728_regmap_config); > > Please use devm_cci_regmap_init_i2c(). > Will do. > > + if (IS_ERR(imx728->regmap)) > > + return PTR_ERR(imx728->regmap); > > + > > + imx728->xclr_gpio = devm_gpiod_get_optional(imx728->dev, > > + "xclr", GPIOD_OUT_LOW); > > + if (IS_ERR(imx728->xclr_gpio)) > > + return PTR_ERR(imx728->xclr_gpio); > > + > > + imx728->clk = devm_clk_get(imx728->dev, "inck"); > > + if (IS_ERR(imx728->clk)) > > + return PTR_ERR(imx728->clk); > > + > > + imx728->clk_rate = clk_get_rate(imx728->clk); > > + dev_info(imx728->dev, "inck rate: %lu Hz\n", imx728->clk_rate); > > No need for this. clock frequency is available in debugfs if necessary. > I have changed this to a dev_dbg > > + > > + if (imx728->clk_rate < 18000000 || imx728->clk_rate > 30000000) > > + return -EINVAL; > > + > > + ret = imx728_power_on(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_detect(imx728); > > + if (ret < 0) > > + return ret; > > + > > + sd = &imx728->subdev; > > + v4l2_i2c_subdev_init(sd, client, &imx728_subdev_ops); > > + > > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > > + V4L2_SUBDEV_FL_HAS_EVENTS | > > + V4L2_SUBDEV_FL_STREAMS; > > + > > + imx728->pad.flags = MEDIA_PAD_FL_SOURCE; > > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + ret = media_entity_pads_init(&sd->entity, 1, &imx728->pad); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: media entity init failed %d\n", __func__, ret); > > + return ret; > > + } > > + > > + ctrl_hdr = &imx728->ctrl.handler; > > + ret = v4l2_ctrl_handler_init(ctrl_hdr, 8); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: ctrl handler init failed: %d\n", __func__, ret); > > + goto err_media_cleanup; > > + } > > + > > + mutex_init(&imx728->lock); > > + imx728->ctrl.handler.lock = &imx728->lock; > > + imx728->fps = IMX728_FRAMERATE_DEFAULT; > > + > > + imx728->ctrl.exposure = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_EXPOSURE, 0, > > + 33000, 1, > > + IMX728_EXPOSURE_DEFAULT); > > + > > + imx728->ctrl.again = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, 0, > > + 102, 1, > > + 24); > > + > > + imx728->ctrl.wdr = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_WIDE_DYNAMIC_RANGE, > > + 0, 1, 1, 1); > > + > > + imx728->ctrl.h_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_HFLIP, 0, 1, 1, 0); > > + > > + imx728->ctrl.v_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_VFLIP, 0, 1, 1, 0); > > + > > + imx728->ctrl.pg_mode = v4l2_ctrl_new_std_menu_items(ctrl_hdr, > > + &imx728_ctrl_ops, V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(imx728_ctrl_pg_qmenu) - 1, > > + 0, 0, imx728_ctrl_pg_qmenu); > > + > > + imx728->ctrl.pixel_rate = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_PIXEL_RATE, > > + IMX728_PIXEL_RATE, > > + IMX728_PIXEL_RATE, 1, > > + IMX728_PIXEL_RATE); > > + > > + imx728->ctrl.link_freq = v4l2_ctrl_new_int_menu(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_LINK_FREQ, > > + ARRAY_SIZE(imx728_link_freq_menu) - 1, 0, > > + imx728_link_freq_menu); > > + > > + if (imx728->ctrl.link_freq) > > + imx728->ctrl.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + imx728->subdev.ctrl_handler = ctrl_hdr; > > + if (imx728->ctrl.handler.error) { > > + ret = imx728->ctrl.handler.error; > > + dev_err(imx728->dev, > > + "%s: failed to add the ctrls: %d\n", __func__, ret); > > + goto err_ctrl_free; > > + } > > + > > + pm_runtime_set_active(imx728->dev); > > + pm_runtime_enable(imx728->dev); > > + pm_runtime_set_autosuspend_delay(imx728->dev, IMX728_PM_IDLE_TIMEOUT); > > + pm_runtime_use_autosuspend(imx728->dev); > > + pm_runtime_get_noresume(imx728->dev); > > + > > + ret = v4l2_subdev_init_finalize(sd); > > + if (ret < 0) > > + goto err_pm_disable; > > + > > + ret = v4l2_async_register_subdev(sd); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: v4l2 subdev register failed %d\n", __func__, ret); > > + goto err_subdev_cleanup; > > + } > > + > > + dev_info(imx728->dev, "imx728 probed\n"); > > + pm_runtime_mark_last_busy(imx728->dev); > > + pm_runtime_put_autosuspend(imx728->dev); > > + return 0; > > + > > +err_subdev_cleanup: > > + v4l2_subdev_cleanup(&imx728->subdev); > > + > > +err_pm_disable: > > + pm_runtime_dont_use_autosuspend(imx728->dev); > > + pm_runtime_put_noidle(imx728->dev); > > + pm_runtime_disable(imx728->dev); > > + > > +err_ctrl_free: > > + v4l2_ctrl_handler_free(ctrl_hdr); > > + mutex_destroy(&imx728->lock); > > + > > +err_media_cleanup: > > + media_entity_cleanup(&imx728->subdev.entity); > > + > > + return ret; > > +} > > + > > +static int __maybe_unused imx728_runtime_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + ret = imx728_power_on(imx728); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_runtime_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + imx728_power_off(imx728); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + if (imx728->streaming) > > + imx728_stop_stream(imx728); > > + > > + ret = pm_runtime_force_suspend(dev); > > + if (ret < 0) > > + dev_warn(dev, "%s: failed to suspend: %i\n", __func__, ret); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + ret = pm_runtime_force_resume(dev); > > + if (ret < 0) > > + dev_warn(dev, "%s: failed to resume: %i\n", __func__, ret); > > + > > + if (imx728->streaming) > > + ret = imx728_start_stream(imx728); > > + > > + if (ret < 0) { > > + imx728_stop_stream(imx728); > > + imx728->streaming = 0; > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void imx728_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + v4l2_ctrl_handler_free(&imx728->ctrl.handler); > > + v4l2_subdev_cleanup(&imx728->subdev); > > + media_entity_cleanup(&sd->entity); > > + mutex_destroy(&imx728->lock); > > + > > + pm_runtime_disable(imx728->dev); > > + pm_runtime_dont_use_autosuspend(imx728->dev); > > + pm_runtime_set_suspended(imx728->dev); > > +} > > + > > +static const struct dev_pm_ops imx728_pm_ops = { > > + SET_RUNTIME_PM_OPS(imx728_runtime_suspend, > > + imx728_runtime_resume, NULL) > > + SET_SYSTEM_SLEEP_PM_OPS(imx728_suspend, imx728_resume) > > +}; > > + > > +static const struct of_device_id imx728_dt_id[] = { > > + { .compatible = "sony,imx728" }, > > + { /* sentinel */ } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, imx728_dt_id); > > + > > +static struct i2c_driver imx728_i2c_driver = { > > + .driver = { > > + .name = "imx728", > > + .of_match_table = of_match_ptr(imx728_dt_id), > > + .pm = &imx728_pm_ops, > > + }, > > + .probe = imx728_probe, > > + .remove = imx728_remove, > > +}; > > + > > +module_i2c_driver(imx728_i2c_driver); > > + > > +MODULE_DESCRIPTION("Camera Sensor Driver for Sony IMX728"); > > +MODULE_AUTHOR("Spencer Hill <shill@d3engineering.com>"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/media/i2c/imx728.h b/drivers/media/i2c/imx728.h > > new file mode 100644 > > index 000000000000..6f320214b780 > > --- /dev/null > > +++ b/drivers/media/i2c/imx728.h > > There is only a single user of this header, move this into the c file. > I will combine these two files. > > @@ -0,0 +1,3458 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Sony IMX728 CMOS Image Sensor Driver > > + * > > + * Copyright (c) 2024 Define Design Deploy Corp > > + */ > > + > > +#include <linux/types.h> > > + > > +#define IMX728_OUT_WIDTH 3840 > > +#define IMX728_OUT_HEIGHT 2160 > > + > > +#define IMX728_FRAMERATE_MAX 30 > > +#define IMX728_FRAMERATE_DEFAULT 30 > > +#define IMX728_FRAMERATE_MIN 10 > > + > > +#define IMX728_PIXEL_RATE 225504000 > > +#define IMX728_LINK_FREQ 800000000 > > + > > +#define IMX728_EXPOSURE_DEFAULT 10000 > > + > > +#define IMX728_PM_IDLE_TIMEOUT 1000 > > + > > + > > +#define IMX728_REG_CTRL_POINT_X(i) (0xA198 + (i) * 8) > > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4) > > + > > +enum imx728_sensor_state { > > + IMX728_SENSOR_STATE_SLEEP = 0x02, > > + IMX728_SENSOR_STATE_STANDBY = 0x04, > > + IMX728_SENSOR_STATE_STREAMING = 0x10, > > + IMX728_SENSOR_STATE_SAFE = 0x20, > > +}; > > + > > + > > +enum imx728_remap_mode_id { > > + IMX728_REMAP_MODE_STANDBY = 0x00, > > + IMX728_REMAP_MODE_STANDBY_PIXEL_SHADING_COMPENSATION = 0x01, > > + IMX728_REMAP_MODE_STANDBY_SPOT_PIXEL_COMPENSATION = 0x02, > > + IMX728_REMAP_MODE_STREAMING = 0x04, > > + IMX728_REMAP_MODE_STREAMING_PIXEL_SHADING_COMPENSATION = 0x05, > > + IMX728_REMAP_MODE_STREAMING_SPOT_PIXEL_COMPENSATION = 0x06, > > + IMX728_REMAP_MODE_SLEEP = 0x20, > > +}; > > + > > +enum imx728_drive_mode { > > + IMX728_MODE_3856x2176_45_4LANE_RAW10 = 0x01, > > + IMX728_MODE_3856x2176_45_4LANE_RAW12 = 0x02, > > + IMX728_MODE_3856x2176_45_4LANE_RAW16 = 0x03, > > + IMX728_MODE_3856x2176_45_4LANE_RAW20 = 0x04, > > + IMX728_MODE_3856x2176_45_4LANE_RAW12_HDR = 0x05, > > + IMX728_MODE_3856x2176_40_4LANE_RAW10 = 0x11, > > + IMX728_MODE_3856x2176_40_4LANE_RAW12 = 0x12, > > + IMX728_MODE_3856x2176_40_4LANE_RAW16 = 0x13, > > + IMX728_MODE_3856x2176_40_4LANE_RAW20 = 0x14, > > + IMX728_MODE_3856x2176_40_4LANE_RAW12_HDR = 0x16, > > +}; > > + > > +enum imx728_awbmode { > > + IMX728_AWBMODE_ATW = 0, > > + IMX728_AWBMODE_ALL_PULL_IN = 1, > > + IMX728_AWBMODE_USER_PRESET = 2, > > + IMX728_AWBMODE_FULL_MWB = 3, > > + IMX728_AWBMODE_HOLD = 4, > > +}; > > + > > +enum imx728_img_raw_mode { > > + IMX728_IMG_MODE_LINEAR = 0x0, > > + IMX728_IMG_MODE_LI = 0x1, > > + IMX728_IMG_MODE_HDR = 0x2, > > + IMX728_IMG_MODE_LI_HDR = 0x3, > > +}; > > + > > +enum imx728_aemode { > > + IMX728_AEMODE_AE_AUTO = 0, > > + IMX728_AEMODE_AE_HOLD = 1, > > + IMX728_AEMODE_SCALE_ME = 2, > > + IMX728_AEMODE_FULL_ME = 3, > > +}; > > + > > +enum imx728_fme_shtval_unit { > > + IMX728_FME_SHTVAL_UNIT_LINES = 1, > > + IMX728_FME_SHTVAL_UNIT_MICROSECONDS = 3, > > + IMX728_FME_SHTVAL_UNIT_FRAMES = 4, > > +}; > > + > > +enum imx728_linear_raw_sel { > > + IMX728_RAW_SEL_SP1H = 0x0, > > + IMX728_RAW_SEL_SP1L = 0x1, > > + IMX728_RAW_SEL_SP1EC = 0x2, > > + IMX728_RAW_SEL_SP2 = 0x3, > > + IMX728_RAW_SEL_SP1VS = 0x4 > > +}; > > + > > +enum imx728_binn_avg { > > + IMX728_BINN_SIMPLE_AVG, > > + IMX728_BINN_WEIGHTED_AVG, > > +}; > > + > > +struct imx728_ctrl { > > + struct v4l2_ctrl_handler handler; > > + struct v4l2_ctrl *wdr; > > + struct v4l2_ctrl *exposure; > > + struct v4l2_ctrl *again; > > + struct v4l2_ctrl *h_flip; > > + struct v4l2_ctrl *v_flip; > > + struct v4l2_ctrl *pg_mode; > > + struct v4l2_ctrl *pixel_rate; > > + struct v4l2_ctrl *link_freq; > > +}; > > + > > +struct imx728_ctrl_point { > > What does ctrl_point mean? What is x and y? > Control points are used internally by the sensor to adjust how the HDR data from the sensor is compressed into the output bit depth. The values used were provided by Sony. > > + int x, y; > > +}; > > + > > +/* > > + * struct imx728 - imx728 device structure > > + * @dev: Device handle > > + * @clk: Pointer to imx728 clock > > + * @client: Pointer to I2C client > > + * @regmap: Pointer to regmap structure > > + * @xclr_gpio: Pointer to XCLR gpio > > + * @subdev: V4L2 subdevice structure > > + * @format: V4L2 media bus frame format structure > > + * (width and height are in sync with the compose rect) > > + * @pad: Media pad structure > > + * @ctrl: imx728 control structure > > + * @clk_rate: Frequency of imx728 clock > > + * @lock: Mutex structure for V4L2 ctrl handler > > + * @streaming: Flag to store streaming on/off status > > + */ > > +struct imx728 { > > + struct device *dev; > > + > > + struct clk *clk; > > + struct i2c_client *client; > > + struct regmap *regmap; > > + struct gpio_desc *xclr_gpio; > > + > > + struct v4l2_subdev subdev; > > + struct v4l2_mbus_framefmt format; > > + struct media_pad pad; > > + > > + struct imx728_ctrl ctrl; > > + > > + unsigned long clk_rate; > > + u32 fps; > > + > > + struct mutex lock; > > + bool streaming; > > +}; > > + > > +static const struct v4l2_area imx728_framesizes[] = { > > + { > > + .width = IMX728_OUT_WIDTH, > > + .height = IMX728_OUT_HEIGHT, > > + }, > > Are you sure this is the only supported resolution? I would prefer using > actual numbers here. > This is not the only supported resolution by the sensor, however it is the only resolution that I have a configuration for at the moment. I will remove the defines and switch to actual numbers to make supporting other resolutions easier in the future. > > +}; > > + > > +static const u32 imx728_mbus_formats[] = { > > + MEDIA_BUS_FMT_SRGGB10_1X10, > > +}; > > + > > +static const s64 imx728_link_freq_menu[] = { > > + IMX728_LINK_FREQ, > > +}; > > + > > +static const struct regmap_config imx728_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 8, > > +}; > > + > > +static const char *const imx728_ctrl_pg_qmenu[] = { > > + "Disabled", > > + "Horizontal Color Bars", > > + "Vertical Color Bars", > > +}; > > + > > +static struct imx728_ctrl_point imx728_hdr_20bit[] = { > > + {0, 0}, > > + {1566 >> 4, 938}, > > + {105740 >> 4, 1863}, > > + {387380 >> 4, 2396}, > > + {3818601 >> 4, 3251}, > > + {16777215 >> 4, 4095}, > > + {-1, -1} > > +}; > > + > > +static const struct reg_sequence imx728_3840x2160[] = { > > Please use struct cci_reg_sequence. > > Best regards, > Alexander > I will change this. > > + {0xFFFF, 0x00, 1000}, > > + {0x1749, 0x01}, > > + {0x174B, 0x01}, > > [snip] > > +}; > > -- > > 2.40.1 > > > > Please be aware that this email includes email addresses outside of the organization. > > > > > > > -- > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany > Amtsgericht München, HRB 105018 > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider > http://www.tq-group.com/ > > Please be aware that this email includes email addresses outside of the organization.
On Thu, Jun 27, 2024 at 04:39:37PM +0100, Dave Stevenson wrote: > Hi Spencer > > Thanks for the patch - always nice to see new sensors being supported. > > I assume there's no public datasheet for the sensor, but are you aware > of even a product brief? Reviews are tricky without data to work with. > I've just read through looking for discrepancies and common errors. > There currently isn't a public datasheet available for the sensor. I'll modify the device tree description with what is publicly available. > On Wed, 26 Jun 2024 at 22:16, Spencer Hill <shill@d3engineering.com> wrote: > > > > Add a driver for the Sony IMX728 image sensor. > > > > Signed-off-by: Spencer Hill <shill@d3engineering.com> > > --- > > drivers/media/i2c/Kconfig | 11 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/imx728.c | 1167 ++++++++++++ > > drivers/media/i2c/imx728.h | 3458 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 4637 insertions(+) > > create mode 100644 drivers/media/i2c/imx728.c > > create mode 100644 drivers/media/i2c/imx728.h > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > index c6d3ee472d81..46b6463c558a 100644 > > --- a/drivers/media/i2c/Kconfig > > +++ b/drivers/media/i2c/Kconfig > > @@ -233,6 +233,17 @@ config VIDEO_IMX415 > > To compile this driver as a module, choose M here: the > > module will be called imx415. > > > > +config VIDEO_IMX728 > > + tristate "Sony IMX728 sensor support" > > + depends on OF_GPIO > > + select V4L2_CCI_I2C > > + help > > + This is a Video4Linux2 sensor driver for the Sony > > + IMX728 camera. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called imx728. > > + > > config VIDEO_MAX9271_LIB > > tristate > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > index dfbe6448b549..1188420ee1b4 100644 > > --- a/drivers/media/i2c/Makefile > > +++ b/drivers/media/i2c/Makefile > > @@ -56,6 +56,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > > obj-$(CONFIG_VIDEO_IMX412) += imx412.o > > obj-$(CONFIG_VIDEO_IMX415) += imx415.o > > +obj-$(CONFIG_VIDEO_IMX728) += imx728.o > > obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o > > obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o > > obj-$(CONFIG_VIDEO_KS0127) += ks0127.o > > diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c > > new file mode 100644 > > index 000000000000..b23359133a22 > > --- /dev/null > > +++ b/drivers/media/i2c/imx728.c > > @@ -0,0 +1,1167 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Sony IMX728 CMOS Image Sensor Driver > > + * > > + * Copyright (c) 2024 Define Design Deploy Corp > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/clk.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <linux/v4l2-mediabus.h> > > +#include <linux/videodev2.h> > > +#include <media/v4l2-subdev.h> > > +#include <media/v4l2-ctrls.h> > > +#include <media/v4l2-event.h> > > + > > +#include "imx728.h" > > + > > +static inline struct imx728 *to_imx728(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct imx728, subdev); > > +} > > + > > +static int imx728_read(struct imx728 *imx728, u16 addr, u32 *val, size_t nbytes) > > +{ > > + int ret; > > + __le32 val_le = 0; > > + > > + ret = regmap_bulk_read(imx728->regmap, addr, &val_le, nbytes); > > + if (ret < 0) { > > + dev_err(imx728->dev, "%s: failed to read reg 0x%04x: %d\n", > > + __func__, addr, ret); > > + return ret; > > + } > > + > > + *val = le32_to_cpu(val_le); > > + > > + return 0; > > +} > > + > > +static int imx728_write(struct imx728 *imx728, u16 addr, u32 val, size_t nbytes) > > +{ > > + int ret; > > + __le32 val_le = cpu_to_le32(val); > > + > > + ret = regmap_bulk_write(imx728->regmap, addr, &val_le, nbytes); > > + if (ret < 0) > > + dev_err(imx728->dev, "%s: failed to write reg 0x%04x: %d\n", > > + __func__, addr, ret); > > + > > + return ret; > > +} > > + > > +static int imx728_update_bits(struct imx728 *imx728, u16 addr, u32 val, u32 mask, size_t nbytes) > > +{ > > + int ret; > > + u32 cfg; > > + > > + ret = imx728_read(imx728, addr, &cfg, nbytes); > > + if (ret < 0) > > + return ret; > > + > > + cfg = (val) ? (cfg | mask) : (cfg & (~mask)); > > + > > + return imx728_write(imx728, addr, cfg, nbytes); > > +} > > + > > +static int imx728_write_table(struct imx728 *imx728, > > + const struct reg_sequence *regs, > > + unsigned int nr_regs) > > +{ > > + int ret; > > + > > + ret = regmap_multi_reg_write(imx728->regmap, regs, nr_regs); > > + if (ret < 0) > > + dev_err(imx728->dev, > > + "%s: failed to write reg table (%d)!\n", __func__, ret); > > + return ret; > > +} > > + > > +static int imx728_wait_for_state(struct imx728 *imx728, enum imx728_sensor_state state) > > +{ > > + int ret, i; > > + u32 val; > > + > > + for (i = 0; i < 50; i++) { > > + ret = imx728_read(imx728, 0x2CAC, &val, 1); > > + if (ret == 0 && val == state) { > > + dev_info(imx728->dev, "%s: Enter state %u\n", __func__, val); > > + return 0; > > + } > > + usleep_range(1000, 10000); > > + } > > + > > + return -EBUSY; > > +} > > + > > +static void imx728_init_formats(struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_mbus_framefmt *format; > > + > > + format = v4l2_subdev_state_get_format(state, 0, 0); > > + format->code = imx728_mbus_formats[0]; > > + format->width = imx728_framesizes[0].width; > > + format->height = imx728_framesizes[0].height; > > + format->field = V4L2_FIELD_NONE; > > + format->colorspace = V4L2_COLORSPACE_SMPTE170M; > > +} > > + > > +static int _imx728_set_routing(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_subdev_route routes[] = { > > + { > > + .source_pad = 0, > > + .source_stream = 0, > > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > + }, > > + { > > + .source_pad = 0, > > + .source_stream = 1, > > + } > > + }; > > + > > + struct v4l2_subdev_krouting routing = { > > + .num_routes = ARRAY_SIZE(routes), > > + .routes = routes, > > + }; > > + > > + int ret; > > + > > + ret = v4l2_subdev_set_routing(sd, state, &routing); > > + if (ret < 0) > > + return ret; > > + > > + imx728_init_formats(state); > > + > > + return 0; > > +} > > + > > +static int imx728_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + > > + if (code->index >= ARRAY_SIZE(imx728_mbus_formats)) > > imx728_mbus_formats only lists MEDIA_BUS_FMT_SRGGB10_1X10. Is this for > potential addition of the 12, 16, or 20 bit readout modes? How likely > are those to actually happen? > The sensor supports 10, 12, 16, 20, and 24 bit output. I have settings that are supposed to be for 12bit output, however I do not have hardware that I can test it on, as my current set up is over FPD-Link 3, and when operating in 12 bit mode the sensor surpasses the maximum bandwidth of that interface and no longer functions. I would like to add more modes in the future when I get access to a better hardware setup for testing them. > > + return -EINVAL; > > + > > + code->code = imx728_mbus_formats[code->index]; > > + > > + return 0; > > +} > > + > > +static int imx728_enum_frame_sizes(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > > + if (imx728_mbus_formats[i] == fse->code) > > + break; > > + } > > + > > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > > + return -EINVAL; > > + > > + if (fse->index >= ARRAY_SIZE(imx728_framesizes)) > > + return -EINVAL; > > + > > + fse->min_width = imx728_framesizes[fse->index].width; > > + fse->max_width = fse->min_width; > > + fse->min_height = imx728_framesizes[fse->index].height; > > + fse->max_height = fse->min_height; > > + > > + return 0; > > +} > > + > > +static int imx728_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + struct v4l2_mbus_framefmt *format; > > + const struct v4l2_area *fsize; > > + unsigned int i; > > + u32 code; > > + int ret = 0; > > + > > + if (fmt->pad != 0) > > + return -EINVAL; > > + > > + if (fmt->stream != 0) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(imx728_mbus_formats); ++i) { > > + if (imx728_mbus_formats[i] == fmt->format.code) { > > + code = fmt->format.code; > > + break; > > + } > > + } > > + > > + if (i == ARRAY_SIZE(imx728_mbus_formats)) > > + code = imx728_mbus_formats[0]; > > + > > + fsize = v4l2_find_nearest_size(imx728_framesizes, > > + ARRAY_SIZE(imx728_framesizes), width, > > + height, fmt->format.width, > > + fmt->format.height); > > + > > + mutex_lock(&imx728->lock); > > + > > + format = v4l2_subdev_state_get_format(state, fmt->pad, fmt->stream); > > + > > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx728->streaming) { > > + ret = -EBUSY; > > + goto done; > > + } > > + > > + format->code = code; > > + format->width = fsize->width; > > + format->height = fsize->height; > > + > > + fmt->format = *format; > > + > > +done: > > + mutex_unlock(&imx728->lock); > > + > > + return ret; > > +} > > + > > +static int imx728_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + struct v4l2_subdev_state *state; > > + struct v4l2_mbus_framefmt *fmt; > > + u32 bpp; > > + int ret = 0; > > + > > + if (pad != 0) > > + return -EINVAL; > > + > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + > > + fmt = v4l2_subdev_state_get_format(state, 0, 0); > > + if (!fmt) { > > + ret = -EPIPE; > > + goto out; > > + } > > + > > + memset(fd, 0, sizeof(*fd)); > > + > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > + > > + bpp = 10; > > + > > + fd->entry[fd->num_entries].stream = 0; > > + > > + fd->entry[fd->num_entries].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; > > + fd->entry[fd->num_entries].length = fmt->width * fmt->height * bpp / 8; > > + fd->entry[fd->num_entries].pixelcode = fmt->code; > > + fd->entry[fd->num_entries].bus.csi2.vc = 0; > > + fd->entry[fd->num_entries].bus.csi2.dt = 0x2b; > > Presumably this 0x2b is MIPI_CSI2_DT_RAW10. > That comes back to the question above as to whether supporting other > bit depths is really likely, as if so then prepare to change this > value based on mbus_code. > I will make sure that this changes based on the mbus_code once more bit depths are supported. > > + > > + fd->num_entries++; > > Presumably this fd->num_entries manipulation is due to expecting to > support multiple data types? Whilst not wrong, at the moment it feels > a little redundant. > I can remove this from this patch set if that would be preferred, until more modes are available. > > + > > +out: > > + > > + v4l2_subdev_unlock_state(state); > > + > > + return ret; > > +} > > + > > +static int imx728_set_routing(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state, > > + enum v4l2_subdev_format_whence which, > > + struct v4l2_subdev_krouting *routing) > > +{ > > + int ret; > > + > > + if (routing->num_routes == 0 || routing->num_routes > 1) > > + return -EINVAL; > > + > > + ret = _imx728_set_routing(sd, state); > > + > > + return ret; > > +} > > + > > +static int imx728_set_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct imx728 *imx728 = container_of(ctrl->handler, > > + struct imx728, ctrl.handler); > > + int ret = 0; > > + > > + dev_dbg(imx728->dev, "%s: %s, value: %d\n", > > + __func__, ctrl->name, ctrl->val); > > + > > + if (!pm_runtime_get_if_in_use(imx728->dev)) > > + return 0; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_EXPOSURE: > > + ret = imx728_write(imx728, 0x98DC, ctrl->val, 4); > > + break; > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = imx728_update_bits(imx728, 0x98F8, > > + (ctrl->val * 10), > > + 0x1FFFF, 4); > > + break; > > + case V4L2_CID_HFLIP: > > + ret = imx728_update_bits(imx728, 0x9651, > > + ctrl->val, 0x1, 1); > > + ret |= imx728_update_bits(imx728, 0xB67C, > > + ctrl->val, 0x1, 1); > > + break; > > + case V4L2_CID_VFLIP: > > + ret = imx728_update_bits(imx728, 0x9651, > > + ctrl->val << 1, 0x2, 1); > > + ret = imx728_update_bits(imx728, 0xB67D, > > + ctrl->val, 0x1, 1); > > + break; > > + case V4L2_CID_WIDE_DYNAMIC_RANGE: > > + case V4L2_CID_TEST_PATTERN: > > + // Both of these are configured during start stream. > > + ret = 0; > > + break; > > + default: > > + ret = -EINVAL; > > + } > > + > > + pm_runtime_put_noidle(imx728->dev); > > + return ret; > > +} > > + > > +static int imx728_detect(struct imx728 *imx728) > > +{ > > + int ret; > > + u32 minor, major; > > + > > + ret = imx728_read(imx728, 0x6002, &major, 1); > > + if (ret != 0) { > > + dev_err(imx728->dev, "Could not read PARAM_MAJOR_VER!"); > > + return ret; > > + } > > + ret = imx728_read(imx728, 0x6000, &minor, 1); > > + if (ret != 0) { > > + dev_err(imx728->dev, "Could not read PARAM_MINOR_VER!"); > > + return ret; > > + } > > + dev_dbg(imx728->dev, "Got version: %d.%d", major, minor); > > + > > + return 0; > > +} > > + > > +static int imx728_reset(struct imx728 *imx728) > > +{ > > + > > + if (!IS_ERR_OR_NULL(imx728->xclr_gpio)) { > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > > + usleep_range(1000, 10000); > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 0); > > + msleep(100); > > + > > + return 0; > > + } > > + > > + return -1; > > +} > > + > > +static int imx728_power_on(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = clk_prepare_enable(imx728->clk); > > + if (ret < 0) > > + return ret; > > + > > + imx728_reset(imx728); > > + return 0; > > +} > > + > > +static int imx728_power_off(struct imx728 *imx728) > > +{ > > + > > + if (imx728->xclr_gpio) { > > + gpiod_set_value_cansleep(imx728->xclr_gpio, 1); > > + > > + usleep_range(1, 10); > > + } > > + clk_disable_unprepare(imx728->clk); > > + return 0; > > +} > > + > > +static int imx728_get_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + fi->interval.numerator = 1; > > + fi->interval.denominator = imx728->fps; > > + return 0; > > +} > > + > > +static int imx728_set_frame_interval(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *sd_state, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + u32 req_fps; > > + > > + mutex_lock(&imx728->lock); > > + > > + if (fi->interval.numerator == 0 || fi->interval.denominator == 0) { > > + fi->interval.denominator = IMX728_FRAMERATE_DEFAULT; > > + fi->interval.numerator = 1; > > + } > > + > > + req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator, > > + fi->interval.numerator), > > + IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX); > > + > > + fi->interval.numerator = 1; > > + fi->interval.denominator = req_fps; > > + > > + imx728->fps = req_fps; > > + > > + mutex_unlock(&imx728->lock); > > + dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps); > > Is setting the frame rate actually supported in the driver? I can't > see imx728->fps being used other than to return the value in > imx728_get_frame_interval. > > Frame rate control on raw sensors is normally controlled through > V4L2_CID_VBLANK and V4L2_CID_HBLANK, not these ioctls. > See https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#raw-camera-sensors > I don't presently have frame rate control implemented. There is a register that allows me to increase the number of lines read from the sensor in order to lower the frame rate. Would it be better to implement the V4L2_CID_HBLANK directly in that case rather than exposing the frame rate this way? > > + > > + return 0; > > +} > > + > > +static int imx728_powerup_to_standby(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + dev_info(imx728->dev, "powerup -> standby..."); > > + > > + ret = imx728_reset(imx728); > > + if (ret) { > > + dev_err(imx728->dev, "Error resetting: %i", ret); > > + return ret; > > + } > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_SLEEP); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Could not transition to Sleep state!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B20, imx728->clk_rate / 1000000, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B1C, 0x1, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write INCK frequency!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B05, 0xFF, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write to CK_SLEEP!"); > > + return ret; > > + } > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't transition from Sleep to Standby state!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xFFFF, IMX728_REMAP_MODE_STANDBY, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write regmap mode!"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_hdr_fixed_brightness(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + // Sony recommended values > > + unsigned int exposure_sp1_sp2_us = 10000; > > + unsigned int exposure_sp1vs_us = 56; > > + unsigned int sp1h_gain = 240; > > + unsigned int sp1l_gain = 75; > > + unsigned int sp1ec_gain = 21; > > + unsigned int sp2_gain = 33; > > + unsigned int sp1vs_gain = 84; > > + > > + ret = imx728_write(imx728, 0x98DC, exposure_sp1_sp2_us, 4); > > + ret |= imx728_write(imx728, 0x98E4, exposure_sp1_sp2_us, 4); > > + ret |= imx728_write(imx728, 0x98EC, exposure_sp1vs_us, 4); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write fixed exposure values."); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x98F8, > > + sp1h_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x98FC, > > + sp1l_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9900, > > + sp1ec_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9904, > > + sp2_gain, > > + 0x1FFFF, > > + 4); > > + ret |= imx728_update_bits(imx728, 0x9908, > > + sp1vs_gain, > > + 0x1FFFF, > > + 4); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write fixed gain values."); > > + return ret; > > + } > > + > > + dev_info(imx728->dev, "Wrote fixed brightness for HDR"); > > + > > + return 0; > > +} > > + > > +static int imx728_hdr_configure( > > + struct imx728 *imx728, > > + struct imx728_ctrl_point *points, > > + int hdr_bits) > > +{ > > + u32 hdr_norm_x0; > > + u32 hdr_norm_x1; > > + u32 hdr_norm_y0; > > + u32 hdr_norm_y1; > > + > > + int ret; > > + int i; > > + > > + switch (hdr_bits) { > > + case 20: > > + hdr_norm_x0 = 0x2000; > > + hdr_norm_x1 = 0x5000; > > + > > + hdr_norm_y0 = 0x0; > > + hdr_norm_y1 = 0xd000; > > + break; > > + case 24: > > + hdr_norm_x0 = 0x3000; > > + hdr_norm_x1 = 0x5000; > > + > > + hdr_norm_y0 = 0x0; > > + hdr_norm_y1 = 0xe000; > > + break; > > + default: > > + dev_err(imx728->dev, "%i bit HDR not supported.", hdr_bits); > > + break; > > + } > > + > > + ret = imx728_write(imx728, 0x9C60, hdr_norm_x0, 4); > > + ret |= imx728_write(imx728, 0x9C6C, hdr_norm_x0, 4); > > + ret |= imx728_write(imx728, 0x9C64, hdr_norm_x1, 4); > > + ret |= imx728_write(imx728, 0x9C70, hdr_norm_x1, 4); > > + ret |= imx728_write(imx728, 0x9C68, hdr_norm_y0, 2); > > + ret |= imx728_write(imx728, 0x9C74, hdr_norm_y0, 2); > > + ret |= imx728_write(imx728, 0x9C6A, hdr_norm_y1, 2); > > + ret |= imx728_write(imx728, 0x9C76, hdr_norm_y1, 2); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed when setting HDR Normalization gains: %i", ret); > > + return ret; > > + } > > + > > + for (i = 0; i < 16; i++) { > > + ret = imx728_write( > > + imx728, > > + IMX728_REG_CTRL_POINT_X(i), > > + points->x, 4); > > + > > + ret |= imx728_write( > > + imx728, > > + IMX728_REG_CTRL_POINT_Y(i), > > + points->y, 4); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to write control point %i", i); > > + return ret; > > + } > > + > > + if ((points+1)->x >= 0 && (points+1)->y >= 0) > > + points++; > > + } > > + > > + return imx728_hdr_fixed_brightness(imx728); > > +} > > + > > +static int imx728_configure(struct imx728 *imx728) > > +{ > > + int ret; > > + bool hdr = imx728->ctrl.wdr->val; > > + enum imx728_img_raw_mode img_out_mode; > > + enum imx728_drive_mode mode_sel; > > + enum imx728_aemode ae_mode; > > + > > + if (hdr) { > > + img_out_mode = IMX728_IMG_MODE_HDR; > > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > > + ae_mode = IMX728_AEMODE_FULL_ME; > > + } else { > > + img_out_mode = IMX728_IMG_MODE_LINEAR; > > + mode_sel = IMX728_MODE_3856x2176_40_4LANE_RAW10; > > + ae_mode = IMX728_AEMODE_FULL_ME; > > Unless my eyes deceive me, mode_sel and ae_mode are the same in both > modes. Why do they need to be conditional? > I'll adjust the conditional. When I was originally trying to bring up 12bit HDR this made more sense and was left over from then. > > + } > > + > > + ret = imx728_write(imx728, 0x98AC, ae_mode, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't set AE mode!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xA248, IMX728_AWBMODE_FULL_MWB, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't set full manual white balance mode!"); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x1808, 0x1, 0x1, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't enable full manual white balance mode!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98E0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98E8, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x98F0, IMX728_FME_SHTVAL_UNIT_MICROSECONDS, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write exposure time unit to microseconds!"); > > + return ret; > > + } > > + > > + if (hdr) { > > The xclr GPIO is optional, so is there a guaranteed register reset in > the big table to reset these HDR registers back to defaults? > I do not believe so. If the sensor does not have an xclr pin hooked up then these registers will remain the same between runs. If the sensor is not in HDR mode these registers do not impact the operation though, and the out mode is always configured. I will investigate adding a software reset as well just for the sake of sanity though. > > + ret = imx728_hdr_configure(imx728, imx728_hdr_20bit, 20); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't configure sensor for HDR mode: %i", ret); > > + return ret; > > + } > > + } > > + > > + dev_info(imx728->dev, "Disabling metadata..."); > > + ret = imx728_write(imx728, 0x1708, 0x00, 1); > > + ret |= imx728_write(imx728, 0x1709, 0x00, 1); > > + ret |= imx728_write(imx728, 0x170A, 0x00, 1); > > + ret |= imx728_write(imx728, 0x1B40, 0x00, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error disabling metadata: %i", ret); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0x9728, > > + mode_sel, > > + 0x7FFF, > > + 2); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't write mode select."); > > + return ret; > > + } > > + > > + ret = imx728_update_bits(imx728, 0xEC7E, > > + img_out_mode, > > + 0x7, > > + 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't select image out mode."); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0xEC12, 0x28, 2); > > + ret |= imx728_write(imx728, 0xEC14, 0x0, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error disabling OB output."); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1761, 0x0, 1); > > + if (ret < 0) > > + dev_err(imx728->dev, "Error disabling skew calibration from sensor to SER"); > > + > > + switch (imx728->ctrl.pg_mode->val) { > > Even if the test mode can't be changed whilst running, this could be > in imx728_set_ctrl as it'll be called by __v4l2_ctrl_handler_setup > I will move this code into that handler. > > + case 0: > > Again xclr GPIO is optional. Are these reset to defaults? > These are not reset back to default. I'll make sure that there is a software reset as well in the next version of the patch set. > > + break; > > + case 1: > > + // Horizontal Color Bars > > + ret = imx728_write(imx728, 0x1A2A, 8, 2); > > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > > + ret |= imx728_write(imx728, 0xB58F, 0x0, 1); > > + ret |= imx728_write(imx728, 0xB6C5, 0x0, 1); > > + break; > > + case 2: > > + // Vertical Color Bars > > + ret = imx728_write(imx728, 0x1A2C, 16, 2); > > + ret |= imx728_write(imx728, 0x1A30, 0, 3); > > + ret |= imx728_write(imx728, 0x1A38, 1, 3); > > + ret |= imx728_write(imx728, 0xB58F, 0x1, 1); > > + ret |= imx728_write(imx728, 0xB6C5, 0x1, 1); > > + break; > > + default: > > + break; > > + } > > + > > + // Assume that everything except 'disabled' requires pattern gen enable > > + if (imx728->ctrl.pg_mode->val != 0) { > > + ret |= imx728_write(imx728, 0xB58E, 0x1, 1); > > + ret |= imx728_write(imx728, 0xB6C4, 0x1, 1); > > + > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to enable PG mode."); > > + return ret; > > + } > > + } > > + > > + ret = imx728_update_bits(imx728, 0x9714, > > + IMX728_RAW_SEL_SP1H, > > + 0x7, > > + 1); > > + ret |= imx728_update_bits(imx728, 0xB684, > > + IMX728_RAW_SEL_SP1H, > > + 0x7, > > + 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Failed to set subpixel register"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_start_stream(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = imx728_powerup_to_standby(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_write_table(imx728, imx728_3840x2160, ARRAY_SIZE(imx728_3840x2160)); > > You've enabled delayed suspend through pm_runtime, but you're always > sending the whole of this array. Is there an option for only resending > on actual power up to reduce the time to restart if the device hasn't > suspended? > At the moment the system always tries to reset the sensor if possible, so it makes sense to write the table again. It may be possible to differentiate between a power on and a resume and not call that reset, and thus not need to write the entire table again. I will make changes adjusting it so that this write is only performed if the sensor was in a sleep state (indicating that it lost power or was otherwise reset). > > + if (ret < 0) > > + return ret; > > + > > + msleep(100); > > + > > + ret = imx728_configure(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = __v4l2_ctrl_handler_setup(imx728->subdev.ctrl_handler); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: failed to apply v4l2 ctrls: %d\n", __func__, ret); > > + return ret; > > + } > > + > > + ret = imx728_write(imx728, 0x1B04, 0x5C, 1); > > + if (ret < 0) > > + return ret; > > + ret = imx728_write(imx728, 0x1B04, 0xA3, 1); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STREAMING); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Could not transition to Streaming!"); > > + return ret; > > + } > > + > > + msleep(20); > > + > > + return 0; > > +} > > + > > +static int imx728_stop_stream(struct imx728 *imx728) > > +{ > > + int ret; > > + > > + ret = imx728_write(imx728, 0x1B04, 0xFF, 1); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Error sending stop stream command: %i", ret); > > + return ret; > > + } > > + > > + imx728_wait_for_state(imx728, IMX728_SENSOR_STATE_STANDBY); > > + if (ret < 0) { > > + dev_err(imx728->dev, "Couldn't transition out of Streaming mode!"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int imx728_set_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + mutex_lock(&imx728->lock); > > + if (imx728->streaming == enable) { > > + mutex_unlock(&imx728->lock); > > + return 0; > > + } > > + > > + if (enable) { > > + ret = pm_runtime_get_sync(imx728->dev); > > + if (ret < 0) { > > + pm_runtime_put_noidle(imx728->dev); > > + goto err_unlock; > > + } > > + > > + ret = imx728_start_stream(imx728); > > + if (ret < 0) > > + goto err_runtime_put; > > + } else { > > + ret = imx728_stop_stream(imx728); > > + if (ret < 0) > > + goto err_runtime_put; > > + pm_runtime_mark_last_busy(imx728->dev); > > + pm_runtime_put_autosuspend(imx728->dev); > > + } > > + > > + imx728->streaming = enable; > > + > > + __v4l2_ctrl_grab(imx728->ctrl.wdr, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable); > > + __v4l2_ctrl_grab(imx728->ctrl.pg_mode, enable); > > + > > + mutex_unlock(&imx728->lock); > > + return 0; > > + > > +err_runtime_put: > > + pm_runtime_put(imx728->dev); > > + > > +err_unlock: > > + mutex_unlock(&imx728->lock); > > + dev_err(imx728->dev, > > + "%s: failed to setup streaming %d\n", __func__, ret); > > + return ret; > > +} > > + > > +static const struct v4l2_subdev_core_ops imx728_core_ops = { > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > +}; > > + > > +static const struct v4l2_subdev_video_ops imx728_subdev_video_ops = { > > + .s_stream = imx728_set_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops imx728_subdev_pad_ops = { > > + .enum_mbus_code = imx728_enum_mbus_code, > > + .enum_frame_size = imx728_enum_frame_sizes, > > + .get_fmt = v4l2_subdev_get_fmt, > > + .set_fmt = imx728_set_fmt, > > + .get_frame_interval = imx728_get_frame_interval, > > + .set_frame_interval = imx728_set_frame_interval, > > + .set_routing = imx728_set_routing, > > + .get_frame_desc = imx728_get_frame_desc, > > Support for get_selection would be nice to reflect the array geometry, > assuming the information is in the datasheet. It's likely to be > mandatory for libcamera soon. > I have access to this information and will implement this. > > +}; > > + > > +static const struct v4l2_subdev_ops imx728_subdev_ops = { > > + .core = &imx728_core_ops, > > + .video = &imx728_subdev_video_ops, > > + .pad = &imx728_subdev_pad_ops, > > +}; > > + > > +static const struct v4l2_ctrl_ops imx728_ctrl_ops = { > > + .s_ctrl = imx728_set_ctrl, > > +}; > > + > > +static int imx728_probe(struct i2c_client *client) > > +{ > > + struct imx728 *imx728; > > + struct v4l2_subdev *sd; > > + struct v4l2_ctrl_handler *ctrl_hdr; > > + int ret; > > + > > + imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL); > > + if (!imx728) > > + return -ENOMEM; > > + > > + imx728->dev = &client->dev; > > + > > + imx728->regmap = devm_regmap_init_i2c(client, &imx728_regmap_config); > > + if (IS_ERR(imx728->regmap)) > > + return PTR_ERR(imx728->regmap); > > + > > + imx728->xclr_gpio = devm_gpiod_get_optional(imx728->dev, > > + "xclr", GPIOD_OUT_LOW); > > + if (IS_ERR(imx728->xclr_gpio)) > > + return PTR_ERR(imx728->xclr_gpio); > > + > > + imx728->clk = devm_clk_get(imx728->dev, "inck"); > > + if (IS_ERR(imx728->clk)) > > + return PTR_ERR(imx728->clk); > > + > > + imx728->clk_rate = clk_get_rate(imx728->clk); > > + dev_info(imx728->dev, "inck rate: %lu Hz\n", imx728->clk_rate); > > + > > + if (imx728->clk_rate < 18000000 || imx728->clk_rate > 30000000) > > + return -EINVAL; > > + > > + ret = imx728_power_on(imx728); > > + if (ret < 0) > > + return ret; > > + > > + ret = imx728_detect(imx728); > > + if (ret < 0) > > + return ret; > > + > > + sd = &imx728->subdev; > > + v4l2_i2c_subdev_init(sd, client, &imx728_subdev_ops); > > + > > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > > + V4L2_SUBDEV_FL_HAS_EVENTS | > > + V4L2_SUBDEV_FL_STREAMS; > > + > > + imx728->pad.flags = MEDIA_PAD_FL_SOURCE; > > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + ret = media_entity_pads_init(&sd->entity, 1, &imx728->pad); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: media entity init failed %d\n", __func__, ret); > > + return ret; > > + } > > + > > + ctrl_hdr = &imx728->ctrl.handler; > > + ret = v4l2_ctrl_handler_init(ctrl_hdr, 8); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: ctrl handler init failed: %d\n", __func__, ret); > > + goto err_media_cleanup; > > + } > > + > > + mutex_init(&imx728->lock); > > + imx728->ctrl.handler.lock = &imx728->lock; > > + imx728->fps = IMX728_FRAMERATE_DEFAULT; > > + > > + imx728->ctrl.exposure = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_EXPOSURE, 0, > > + 33000, 1, > > + IMX728_EXPOSURE_DEFAULT); > > That looks like you're clipping the max exposure time based on 30fps, > and the units are usecs. > Units on V4L2_CID_EXPOSURE are undefined, and normally expected to be > lines for raw image sensors, but that then needs to know HBLANK and > PIXEL_RATE. > There is V4L2_CID_EXPOSURE_ABSOLUTE which has defined units of > 100usecs, but I'm not sure if that is very useful in this case. > The recommended values for the sensor were provided to me in usecs and the sensor allows configuring what unit is used for exposure, so at the moment the exposure is configured in usecs. Would it be better if I implemented V4L2_CID_EXPOSURE_ABSOLUTE instead then? > > + > > + imx728->ctrl.again = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, 0, > > + 102, 1, > > + 24); > > + > > + imx728->ctrl.wdr = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_WIDE_DYNAMIC_RANGE, > > + 0, 1, 1, 1); > > + > > + imx728->ctrl.h_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_HFLIP, 0, 1, 1, 0); > > + > > + imx728->ctrl.v_flip = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_VFLIP, 0, 1, 1, 0); > > Other Sony sensors end up changing the Bayer order based on flips. I > just want to check that this isn't the case for this sensor, otherwise > you need to set the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag and report the > mbus code based on flips. > My understanding from the datasheet is that this sensor does not change the Bayer order when you perform HFLIP or VFLIP operations. It shifts the pixel readout horizontally or vertically so that the order of readout does not change. > > + > > + imx728->ctrl.pg_mode = v4l2_ctrl_new_std_menu_items(ctrl_hdr, > > + &imx728_ctrl_ops, V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(imx728_ctrl_pg_qmenu) - 1, > > + 0, 0, imx728_ctrl_pg_qmenu); > > + > > + imx728->ctrl.pixel_rate = v4l2_ctrl_new_std(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_PIXEL_RATE, > > + IMX728_PIXEL_RATE, > > + IMX728_PIXEL_RATE, 1, > > + IMX728_PIXEL_RATE); > > + > > + imx728->ctrl.link_freq = v4l2_ctrl_new_int_menu(ctrl_hdr, &imx728_ctrl_ops, > > + V4L2_CID_LINK_FREQ, > > + ARRAY_SIZE(imx728_link_freq_menu) - 1, 0, > > + imx728_link_freq_menu); > > + > > + if (imx728->ctrl.link_freq) > > + imx728->ctrl.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + imx728->subdev.ctrl_handler = ctrl_hdr; > > + if (imx728->ctrl.handler.error) { > > + ret = imx728->ctrl.handler.error; > > + dev_err(imx728->dev, > > + "%s: failed to add the ctrls: %d\n", __func__, ret); > > + goto err_ctrl_free; > > + } > > + > > + pm_runtime_set_active(imx728->dev); > > + pm_runtime_enable(imx728->dev); > > + pm_runtime_set_autosuspend_delay(imx728->dev, IMX728_PM_IDLE_TIMEOUT); > > + pm_runtime_use_autosuspend(imx728->dev); > > + pm_runtime_get_noresume(imx728->dev); > > + > > + ret = v4l2_subdev_init_finalize(sd); > > + if (ret < 0) > > + goto err_pm_disable; > > + > > + ret = v4l2_async_register_subdev(sd); > > + if (ret < 0) { > > + dev_err(imx728->dev, > > + "%s: v4l2 subdev register failed %d\n", __func__, ret); > > + goto err_subdev_cleanup; > > + } > > + > > + dev_info(imx728->dev, "imx728 probed\n"); > > + pm_runtime_mark_last_busy(imx728->dev); > > + pm_runtime_put_autosuspend(imx728->dev); > > + return 0; > > + > > +err_subdev_cleanup: > > + v4l2_subdev_cleanup(&imx728->subdev); > > + > > +err_pm_disable: > > + pm_runtime_dont_use_autosuspend(imx728->dev); > > + pm_runtime_put_noidle(imx728->dev); > > + pm_runtime_disable(imx728->dev); > > + > > +err_ctrl_free: > > + v4l2_ctrl_handler_free(ctrl_hdr); > > + mutex_destroy(&imx728->lock); > > + > > +err_media_cleanup: > > + media_entity_cleanup(&imx728->subdev.entity); > > + > > + return ret; > > +} > > + > > +static int __maybe_unused imx728_runtime_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + ret = imx728_power_on(imx728); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_runtime_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + imx728_power_off(imx728); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + if (imx728->streaming) > > + imx728_stop_stream(imx728); > > + > > + ret = pm_runtime_force_suspend(dev); > > + if (ret < 0) > > + dev_warn(dev, "%s: failed to suspend: %i\n", __func__, ret); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused imx728_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + int ret; > > + > > + ret = pm_runtime_force_resume(dev); > > + if (ret < 0) > > + dev_warn(dev, "%s: failed to resume: %i\n", __func__, ret); > > + > > + if (imx728->streaming) > > + ret = imx728_start_stream(imx728); > > You're likely to get comment from Laurent on this one. He went through > and removed all the suspend/resume handlers as they should really be > handled by the CSI2 receiver driver, not the sensor driver. > Resuming streaming in a random order on system resume is unlikely to work. > Should I remove these handlers then? Or wait for comment? > > + > > + if (ret < 0) { > > + imx728_stop_stream(imx728); > > + imx728->streaming = 0; > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void imx728_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx728 *imx728 = to_imx728(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + v4l2_ctrl_handler_free(&imx728->ctrl.handler); > > + v4l2_subdev_cleanup(&imx728->subdev); > > + media_entity_cleanup(&sd->entity); > > + mutex_destroy(&imx728->lock); > > + > > + pm_runtime_disable(imx728->dev); > > + pm_runtime_dont_use_autosuspend(imx728->dev); > > + pm_runtime_set_suspended(imx728->dev); > > +} > > + > > +static const struct dev_pm_ops imx728_pm_ops = { > > + SET_RUNTIME_PM_OPS(imx728_runtime_suspend, > > + imx728_runtime_resume, NULL) > > + SET_SYSTEM_SLEEP_PM_OPS(imx728_suspend, imx728_resume) > > +}; > > + > > +static const struct of_device_id imx728_dt_id[] = { > > + { .compatible = "sony,imx728" }, > > + { /* sentinel */ } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, imx728_dt_id); > > + > > +static struct i2c_driver imx728_i2c_driver = { > > + .driver = { > > + .name = "imx728", > > + .of_match_table = of_match_ptr(imx728_dt_id), > > + .pm = &imx728_pm_ops, > > + }, > > + .probe = imx728_probe, > > + .remove = imx728_remove, > > +}; > > + > > +module_i2c_driver(imx728_i2c_driver); > > + > > +MODULE_DESCRIPTION("Camera Sensor Driver for Sony IMX728"); > > +MODULE_AUTHOR("Spencer Hill <shill@d3engineering.com>"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/media/i2c/imx728.h b/drivers/media/i2c/imx728.h > > new file mode 100644 > > index 000000000000..6f320214b780 > > --- /dev/null > > +++ b/drivers/media/i2c/imx728.h > > @@ -0,0 +1,3458 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Sony IMX728 CMOS Image Sensor Driver > > + * > > + * Copyright (c) 2024 Define Design Deploy Corp > > + */ > > + > > +#include <linux/types.h> > > + > > +#define IMX728_OUT_WIDTH 3840 > > +#define IMX728_OUT_HEIGHT 2160 > > https://leopardimaging.com/wp-content/uploads/pdf/LI-IMX728-9295-xxxH-_Datasheet.pdf > lists IMX728 as 3857x2177 (strange to see odd numbers in a Bayer > sensor). Do you have definitive numbers for the array size? > > Seeing as these defines are only used in imx728_framesizes, I'd stick > the values directly in the structure. > I will move these numbers into the structure itself. > > + > > +#define IMX728_FRAMERATE_MAX 30 > > +#define IMX728_FRAMERATE_DEFAULT 30 > > +#define IMX728_FRAMERATE_MIN 10 > > + > > +#define IMX728_PIXEL_RATE 225504000 > > 225504000 / 3840 / 2160 = 27.18fps. How does that square with achieving 30fps? > Sorry this should be 248832000, I will change that. > > +#define IMX728_LINK_FREQ 800000000 > > 1.6Gbit/s per lane feels high. I assume it has been checked. > This is what the sensor seems to be operating at given my testing. It's my understanding that this sensor can actually go all the way to 2.5Gbit/s per lane, but I do not have a setup that can test that at the moment. > > + > > +#define IMX728_EXPOSURE_DEFAULT 10000 > > + > > +#define IMX728_PM_IDLE_TIMEOUT 1000 > > + > > + > > +#define IMX728_REG_CTRL_POINT_X(i) (0xA198 + (i) * 8) > > +#define IMX728_REG_CTRL_POINT_Y(i) (IMX728_REG_CTRL_POINT_X(i) + 4) > > + > > +enum imx728_sensor_state { > > + IMX728_SENSOR_STATE_SLEEP = 0x02, > > + IMX728_SENSOR_STATE_STANDBY = 0x04, > > + IMX728_SENSOR_STATE_STREAMING = 0x10, > > + IMX728_SENSOR_STATE_SAFE = 0x20, > > +}; > > + > > + > > +enum imx728_remap_mode_id { > > + IMX728_REMAP_MODE_STANDBY = 0x00, > > + IMX728_REMAP_MODE_STANDBY_PIXEL_SHADING_COMPENSATION = 0x01, > > + IMX728_REMAP_MODE_STANDBY_SPOT_PIXEL_COMPENSATION = 0x02, > > + IMX728_REMAP_MODE_STREAMING = 0x04, > > + IMX728_REMAP_MODE_STREAMING_PIXEL_SHADING_COMPENSATION = 0x05, > > + IMX728_REMAP_MODE_STREAMING_SPOT_PIXEL_COMPENSATION = 0x06, > > + IMX728_REMAP_MODE_SLEEP = 0x20, > > +}; > > + > > +enum imx728_drive_mode { > > + IMX728_MODE_3856x2176_45_4LANE_RAW10 = 0x01, > > + IMX728_MODE_3856x2176_45_4LANE_RAW12 = 0x02, > > + IMX728_MODE_3856x2176_45_4LANE_RAW16 = 0x03, > > + IMX728_MODE_3856x2176_45_4LANE_RAW20 = 0x04, > > + IMX728_MODE_3856x2176_45_4LANE_RAW12_HDR = 0x05, > > + IMX728_MODE_3856x2176_40_4LANE_RAW10 = 0x11, > > + IMX728_MODE_3856x2176_40_4LANE_RAW12 = 0x12, > > + IMX728_MODE_3856x2176_40_4LANE_RAW16 = 0x13, > > + IMX728_MODE_3856x2176_40_4LANE_RAW20 = 0x14, > > + IMX728_MODE_3856x2176_40_4LANE_RAW12_HDR = 0x16, > > +}; > > + > > +enum imx728_awbmode { > > + IMX728_AWBMODE_ATW = 0, > > + IMX728_AWBMODE_ALL_PULL_IN = 1, > > + IMX728_AWBMODE_USER_PRESET = 2, > > + IMX728_AWBMODE_FULL_MWB = 3, > > + IMX728_AWBMODE_HOLD = 4, > > +}; > > + > > +enum imx728_img_raw_mode { > > + IMX728_IMG_MODE_LINEAR = 0x0, > > + IMX728_IMG_MODE_LI = 0x1, > > + IMX728_IMG_MODE_HDR = 0x2, > > + IMX728_IMG_MODE_LI_HDR = 0x3, > > +}; > > + > > +enum imx728_aemode { > > + IMX728_AEMODE_AE_AUTO = 0, > > + IMX728_AEMODE_AE_HOLD = 1, > > + IMX728_AEMODE_SCALE_ME = 2, > > + IMX728_AEMODE_FULL_ME = 3, > > +}; > > + > > +enum imx728_fme_shtval_unit { > > + IMX728_FME_SHTVAL_UNIT_LINES = 1, > > + IMX728_FME_SHTVAL_UNIT_MICROSECONDS = 3, > > + IMX728_FME_SHTVAL_UNIT_FRAMES = 4, > > +}; > > + > > +enum imx728_linear_raw_sel { > > + IMX728_RAW_SEL_SP1H = 0x0, > > + IMX728_RAW_SEL_SP1L = 0x1, > > + IMX728_RAW_SEL_SP1EC = 0x2, > > + IMX728_RAW_SEL_SP2 = 0x3, > > + IMX728_RAW_SEL_SP1VS = 0x4 > > +}; > > + > > +enum imx728_binn_avg { > > + IMX728_BINN_SIMPLE_AVG, > > + IMX728_BINN_WEIGHTED_AVG, > > +}; > > Not used. > This is used for other resolution modes, I will remove it until those are in and it is actually used. > > + > > +struct imx728_ctrl { > > + struct v4l2_ctrl_handler handler; > > + struct v4l2_ctrl *wdr; > > + struct v4l2_ctrl *exposure; > > + struct v4l2_ctrl *again; > > + struct v4l2_ctrl *h_flip; > > + struct v4l2_ctrl *v_flip; > > + struct v4l2_ctrl *pg_mode; > > + struct v4l2_ctrl *pixel_rate; > > + struct v4l2_ctrl *link_freq; > > +}; > > + > > +struct imx728_ctrl_point { > > + int x, y; > > +}; > > + > > +/* > > + * struct imx728 - imx728 device structure > > + * @dev: Device handle > > + * @clk: Pointer to imx728 clock > > + * @client: Pointer to I2C client > > + * @regmap: Pointer to regmap structure > > + * @xclr_gpio: Pointer to XCLR gpio > > + * @subdev: V4L2 subdevice structure > > + * @format: V4L2 media bus frame format structure > > + * (width and height are in sync with the compose rect) > > + * @pad: Media pad structure > > + * @ctrl: imx728 control structure > > + * @clk_rate: Frequency of imx728 clock > > + * @lock: Mutex structure for V4L2 ctrl handler > > + * @streaming: Flag to store streaming on/off status > > + */ > > +struct imx728 { > > + struct device *dev; > > + > > + struct clk *clk; > > + struct i2c_client *client; > > + struct regmap *regmap; > > + struct gpio_desc *xclr_gpio; > > + > > + struct v4l2_subdev subdev; > > + struct v4l2_mbus_framefmt format; > > + struct media_pad pad; > > + > > + struct imx728_ctrl ctrl; > > + > > + unsigned long clk_rate; > > + u32 fps; > > + > > + struct mutex lock; > > + bool streaming; > > +}; > > + > > +static const struct v4l2_area imx728_framesizes[] = { > > + { > > + .width = IMX728_OUT_WIDTH, > > + .height = IMX728_OUT_HEIGHT, > > + }, > > +}; > > + > > +static const u32 imx728_mbus_formats[] = { > > + MEDIA_BUS_FMT_SRGGB10_1X10, > > +}; > > + > > +static const s64 imx728_link_freq_menu[] = { > > + IMX728_LINK_FREQ, > > +}; > > + > > +static const struct regmap_config imx728_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 8, > > +}; > > + > > +static const char *const imx728_ctrl_pg_qmenu[] = { > > + "Disabled", > > + "Horizontal Color Bars", > > + "Vertical Color Bars", > > +}; > > + > > +static struct imx728_ctrl_point imx728_hdr_20bit[] = { > > + {0, 0}, > > + {1566 >> 4, 938}, > > + {105740 >> 4, 1863}, > > + {387380 >> 4, 2396}, > > + {3818601 >> 4, 3251}, > > + {16777215 >> 4, 4095}, > > + {-1, -1} > > +}; > > + > > +static const struct reg_sequence imx728_3840x2160[] = { > > + {0xFFFF, 0x00, 1000}, > <snip> > This is one heck of a table at 3268 register writes. > Are they really all necessary, or are some setting registers to > default values, or duplicated elsewhere in the driver? > This table was provided by Sony for bringing up the sensor, I don't actually have much insight into which writes are absolutely necessary or not. A quick glance shows that some of them seem to be writing default values for registers that are changed later, but comparitively very few, and I'm hesitant to change anything in this table given it was provided by Sony directly. > Thanks > Dave Thanks, Spencer Please be aware that this email includes email addresses outside of the organization.
Add a v4l2 sensor driver for Sony IMX728 v4l2-compliance 1.24.1, 64 bits, 64-bit time_t Compliance test for device /dev/v4l-subdev4: Driver Info: Driver version : 6.1.80 Capabilities : 0x00000002 Streams Support Required ioctls: test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev4 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 12 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for device /dev/v4l-subdev4: 43, Succeeded: 43, Failed: 0, Warnings: 0 Spencer Hill (2): media: i2c: Add driver for Sony IMX728 media: dt-bindings: Add Sony IMX728 .../bindings/media/i2c/sony,imx728.yaml | 78 + MAINTAINERS | 9 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx728.c | 1167 ++++++ drivers/media/i2c/imx728.h | 3458 +++++++++++++++++ 6 files changed, 4724 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx728.yaml create mode 100644 drivers/media/i2c/imx728.c create mode 100644 drivers/media/i2c/imx728.h -- 2.40.1 Please be aware that this email includes email addresses outside of the organization.