Re: [PATCH 1/2] media: i2c: Add driver for Sony IMX728

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi,

Am Donnerstag, 27. Juni 2024, 20:39:04 CEST schrieb Spencer Hill:
> 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 @@
> [snip]
> > > +{
> > > +       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?

List the registers on top of file, see for example imx415.c or imx290.c

> 
> > > +               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?

AFAICS V4L2_MAP_YCBCR_ENC_DEFAULT() converts V4L2_COLORSPACE_RAW to
V4L2_YCBCR_ENC_601, so yes this seems correct. For the others use
V4L2_XFER_FUNC_NONE and V4L2_QUANTIZATION_DEFAULT.

> > > +}
> > > +
> > > +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.
> 
> [snip]
> > > +
> > > +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.

Okay, I see. But if you use a sequence of cci_write() calls you can instead
raise an error message on call site, see imx290_set_clock() for that.

> > > +               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;
> > > +}
> > > +

> [snip]

> > > 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.

Nice, thanks.

Best regards,
Alexander

> > > +};
> > > +
> > > +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.
> 
> 


-- 
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/