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@xxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxx>");
> > +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.
