Another batch of improvements of the imx335 driver.
Patch 1/5 adds support for 2 or 4 lane operation modes.
Patch 2/5 call the V4L2 fwnode device parser to handle controls that are
standardised by the framework.
Patch 3/5 introduces the use of CCI for registers access.
Patch 4/4 fixes the height value discrepency. Accessible height is 1944,
as per the data sheet in all-pixel scan mode.
Patch 5/5 fixes the max analogue gain value.
Kieran Bingham (2):
media: imx335: Support 2 or 4 lane operation modes
media: imx335: Parse fwnode properties
Umang Jain (3):
media: imx335: Use V4L2 CCI for accessing sensor registers
media: imx335: Fix active area height discrepency
media: imx335: Limit analogue gain value
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/imx335.c | 596 ++++++++++++++++++-------------------
2 files changed, 287 insertions(+), 310 deletions(-)
--
2.43.0
From: Kieran Bingham <[email protected]>
The IMX335 can support both 2 and 4 lane configurations.
Extend the driver to configure the lane mode accordingly.
Update the pixel rate depending on the number of lanes in use.
Signed-off-by: Kieran Bingham <[email protected]>
Signed-off-by: Umang Jain <[email protected]>
---
drivers/media/i2c/imx335.c | 46 +++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index dab6d080bc4c..a42f48823515 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -21,6 +21,11 @@
#define IMX335_MODE_STANDBY 0x01
#define IMX335_MODE_STREAMING 0x00
+/* Data Lanes */
+#define IMX335_LANEMODE 0x3a01
+#define IMX335_2LANE 1
+#define IMX335_4LANE 3
+
/* Lines per frame */
#define IMX335_REG_LPFR 0x3030
@@ -67,8 +72,6 @@
#define IMX335_LINK_FREQ_594MHz 594000000LL
#define IMX335_LINK_FREQ_445MHz 445500000LL
-#define IMX335_NUM_DATA_LANES 4
-
#define IMX335_REG_MIN 0x00
#define IMX335_REG_MAX 0xfffff
@@ -115,7 +118,6 @@ static const char * const imx335_supply_name[] = {
* @vblank: Vertical blanking in lines
* @vblank_min: Minimum vertical blanking in lines
* @vblank_max: Maximum vertical blanking in lines
- * @pclk: Sensor pixel clock
* @reg_list: Register list for sensor mode
*/
struct imx335_mode {
@@ -126,7 +128,6 @@ struct imx335_mode {
u32 vblank;
u32 vblank_min;
u32 vblank_max;
- u64 pclk;
struct imx335_reg_list reg_list;
};
@@ -147,6 +148,7 @@ struct imx335_mode {
* @exp_ctrl: Pointer to exposure control
* @again_ctrl: Pointer to analog gain control
* @vblank: Vertical blanking in lines
+ * @lane_mode Mode for number of connected data lanes
* @cur_mode: Pointer to current selected sensor mode
* @mutex: Mutex for serializing sensor controls
* @link_freq_bitmap: Menu bitmap for link_freq_ctrl
@@ -171,6 +173,7 @@ struct imx335 {
struct v4l2_ctrl *again_ctrl;
};
u32 vblank;
+ u32 lane_mode;
const struct imx335_mode *cur_mode;
struct mutex mutex;
unsigned long link_freq_bitmap;
@@ -377,7 +380,6 @@ static const struct imx335_mode supported_mode = {
.vblank = 2560,
.vblank_min = 2560,
.vblank_max = 133060,
- .pclk = 396000000,
.reg_list = {
.num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
.regs = mode_2592x1940_regs,
@@ -936,6 +938,11 @@ static int imx335_start_streaming(struct imx335 *imx335)
return ret;
}
+ /* Configure lanes */
+ ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
+ if (ret)
+ return ret;
+
/* Setup handler will write actual exposure and gain */
ret = __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
if (ret) {
@@ -1096,7 +1103,14 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
if (ret)
return ret;
- if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX335_NUM_DATA_LANES) {
+ switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
+ case 2:
+ imx335->lane_mode = IMX335_2LANE;
+ break;
+ case 4:
+ imx335->lane_mode = IMX335_4LANE;
+ break;
+ default:
dev_err(imx335->dev,
"number of CSI2 data lanes %d is not supported\n",
bus_cfg.bus.mipi_csi2.num_data_lanes);
@@ -1209,6 +1223,9 @@ static int imx335_init_controls(struct imx335 *imx335)
struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
const struct imx335_mode *mode = imx335->cur_mode;
u32 lpfr;
+ u64 pclk;
+ s64 link_freq_in_use;
+ u8 bpp;
int ret;
ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
@@ -1252,11 +1269,24 @@ static int imx335_init_controls(struct imx335 *imx335)
0, 0, imx335_tpg_menu);
/* Read only controls */
+
+ /* pixel rate = link frequency * lanes * 2 / bits_per_pixel */
+ switch (imx335->cur_mbus_code) {
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ bpp = 10;
+ break;
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ bpp = 12;
+ break;
+ }
+
+ link_freq_in_use = link_freq[__ffs(imx335->link_freq_bitmap)];
+ pclk = link_freq_in_use * (imx335->lane_mode + 1) * 2 / bpp;
imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
&imx335_ctrl_ops,
V4L2_CID_PIXEL_RATE,
- mode->pclk, mode->pclk,
- 1, mode->pclk);
+ pclk, pclk,
+ 1, pclk);
imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
&imx335_ctrl_ops,
--
2.43.0
Use the new comon CCI register access helpers to replace the private
register access helpers in the imx335 driver.
Select V4L2_CCI_I2C Kconfig option which the imx335 driver now
depends on.
Signed-off-by: Umang Jain <[email protected]>
---
drivers/media/i2c/Kconfig | 1 +
drivers/media/i2c/imx335.c | 521 ++++++++++++++++---------------------
2 files changed, 225 insertions(+), 297 deletions(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 56f276b920ab..8d248b9c9562 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -195,6 +195,7 @@ config VIDEO_IMX334
config VIDEO_IMX335
tristate "Sony IMX335 sensor support"
depends on OF_GPIO
+ select V4L2_CCI_I2C
help
This is a Video4Linux2 sensor driver for the Sony
IMX335 camera.
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 7f3f74240cd0..6ea09933e47b 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -11,47 +11,49 @@
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <media/v4l2-cci.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-fwnode.h>
#include <media/v4l2-subdev.h>
/* Streaming Mode */
-#define IMX335_REG_MODE_SELECT 0x3000
+#define IMX335_REG_MODE_SELECT CCI_REG8(0x3000)
#define IMX335_MODE_STANDBY 0x01
#define IMX335_MODE_STREAMING 0x00
/* Data Lanes */
-#define IMX335_LANEMODE 0x3a01
+#define IMX335_REG_LANEMODE CCI_REG8(0x3a01)
#define IMX335_2LANE 1
#define IMX335_4LANE 3
/* Lines per frame */
-#define IMX335_REG_LPFR 0x3030
+#define IMX335_REG_VMAX CCI_REG24_LE(0x3030)
/* Chip ID */
-#define IMX335_REG_ID 0x3912
+#define IMX335_REG_ID CCI_REG8(0x3912)
#define IMX335_ID 0x00
/* Exposure control */
-#define IMX335_REG_SHUTTER 0x3058
+#define IMX335_REG_SHUTTER CCI_REG24_LE(0x3058)
#define IMX335_EXPOSURE_MIN 1
#define IMX335_EXPOSURE_OFFSET 9
#define IMX335_EXPOSURE_STEP 1
#define IMX335_EXPOSURE_DEFAULT 0x0648
/* Analog gain control */
-#define IMX335_REG_AGAIN 0x30e8
+#define IMX335_REG_AGAIN CCI_REG8(0x30e8)
#define IMX335_AGAIN_MIN 0
#define IMX335_AGAIN_MAX 240
#define IMX335_AGAIN_STEP 1
#define IMX335_AGAIN_DEFAULT 0
/* Group hold register */
-#define IMX335_REG_HOLD 0x3001
+#define IMX335_REG_HOLD CCI_REG8(0x3001)
/* Test pattern generator */
-#define IMX335_REG_TPG 0x329e
+#define IMX335_REG_TPG CCI_REG8(0x329e)
#define IMX335_TPG_ALL_000 0
#define IMX335_TPG_ALL_FFF 1
#define IMX335_TPG_ALL_555 2
@@ -65,6 +67,46 @@
#define IMX335_TPG_H_COLOR_BARS 10
#define IMX335_TPG_V_COLOR_BARS 11
+#define IMX335_REG_MASTER_MODE CCI_REG8(0x3002)
+#define IMX335_REG_WINMODE CCI_REG8(0x3018)
+#define IMX335_REG_HTRIMMING_START CCI_REG16_LE(0x302c)
+#define IMX335_REG_HNUM CCI_REG8(0x302e)
+#define IMX335_REG_XVS_XHS_DRV CCI_REG8(0x31a1)
+#define IMX335_REG_Y_OUT_SIZE CCI_REG16_LE(0x3056)
+#define IMX335_REG_VCROP_POS CCI_REG16_LE(0x3074)
+#define IMX335_REG_VCROP_SIZE CCI_REG16_LE(0x3076)
+#define IMX335_REG_OPB_SIZE_V CCI_REG8(0x304c)
+
+#define IMX335_REG_ADBIT CCI_REG8(0x3050)
+#define IMX335_REG_MDBIT CCI_REG8(0x319d)
+#define IMX335_REG_ADBIT1 CCI_REG16_LE(0x341c)
+
+#define IMX335_REG_BCWAIT_TIME CCI_REG8(0x300c)
+#define IMX335_REG_CPWAIT_TIME CCI_REG8(0x300d)
+#define IMX335_REG_INCLKSEL1 CCI_REG16_LE(0x314c)
+#define IMX335_REG_INCLKSEL2 CCI_REG8(0x315a)
+#define IMX335_REG_INCLKSEL3 CCI_REG8(0x3168)
+#define IMX335_REG_INCLKSEL4 CCI_REG8(0x316a)
+#define IMX335_REG_SYSMODE CCI_REG8(0x319e)
+
+#define IMX335_REG_TCLKPOST CCI_REG16_LE(0x3a18)
+#define IMX335_REG_TCLKPREPARE CCI_REG16_LE(0x3a1a)
+#define IMX335_REG_TCLK_TRAIL CCI_REG16_LE(0x3a1c)
+#define IMX335_REG_TCLK_ZERO CCI_REG16_LE(0x3a1e)
+#define IMX335_REG_THS_PREPARE CCI_REG16_LE(0x3a20)
+#define IMX335_REG_THS_ZERO CCI_REG16_LE(0x3a22)
+#define IMX335_REG_THS_TRAIL CCI_REG16_LE(0x3a24)
+#define IMX335_REG_THS_EXIT CCI_REG16_LE(0x3a26)
+#define IMX335_REG_TPLX CCI_REG16_LE(0x3a28)
+
+
+#define IMX335_REG_TPG_TESTCLKEN CCI_REG8(0x3148)
+#define IMX335_REG_TPG_DIG_CLP_MODE CCI_REG8(0x3280)
+#define IMX335_REG_TPG_EN_DUOUT CCI_REG8(0x329c)
+#define IMX335_REG_TPG_COLORWIDTH CCI_REG8(0x32a0)
+#define IMX335_REG_TPG_BLKLEVEL CCI_REG16_LE(0x3302)
+#define IMX335_REG_TPG_WRJ_OPEN CCI_REG8(0x336c)
+
/* Input clock rate */
#define IMX335_INCLK_RATE 24000000
@@ -83,16 +125,6 @@
#define IMX335_PIXEL_ARRAY_WIDTH 2592U
#define IMX335_PIXEL_ARRAY_HEIGHT 1944U
-/**
- * struct imx335_reg - imx335 sensor register
- * @address: Register address
- * @val: Register value
- */
-struct imx335_reg {
- u16 address;
- u8 val;
-};
-
/**
* struct imx335_reg_list - imx335 sensor register list
* @num_of_regs: Number of registers in the list
@@ -100,7 +132,7 @@ struct imx335_reg {
*/
struct imx335_reg_list {
u32 num_of_regs;
- const struct imx335_reg *regs;
+ const struct cci_reg_sequence *regs;
};
static const char * const imx335_supply_name[] = {
@@ -161,6 +193,7 @@ struct imx335 {
struct media_pad pad;
struct gpio_desc *reset_gpio;
struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
+ struct regmap *cci;
struct clk *inclk;
struct v4l2_ctrl_handler ctrl_handler;
@@ -213,140 +246,135 @@ static const int imx335_tpg_val[] = {
};
/* Sensor mode registers */
-static const struct imx335_reg mode_2592x1940_regs[] = {
- {0x3000, 0x01},
- {0x3002, 0x00},
- {0x3018, 0x04},
- {0x302c, 0x3c},
- {0x302e, 0x20},
- {0x3056, 0x94},
- {0x3074, 0xc8},
- {0x3076, 0x28},
- {0x304c, 0x00},
- {0x31a1, 0x00},
- {0x3288, 0x21},
- {0x328a, 0x02},
- {0x3414, 0x05},
- {0x3416, 0x18},
- {0x3648, 0x01},
- {0x364a, 0x04},
- {0x364c, 0x04},
- {0x3678, 0x01},
- {0x367c, 0x31},
- {0x367e, 0x31},
- {0x3706, 0x10},
- {0x3708, 0x03},
- {0x3714, 0x02},
- {0x3715, 0x02},
- {0x3716, 0x01},
- {0x3717, 0x03},
- {0x371c, 0x3d},
- {0x371d, 0x3f},
- {0x372c, 0x00},
- {0x372d, 0x00},
- {0x372e, 0x46},
- {0x372f, 0x00},
- {0x3730, 0x89},
- {0x3731, 0x00},
- {0x3732, 0x08},
- {0x3733, 0x01},
- {0x3734, 0xfe},
- {0x3735, 0x05},
- {0x3740, 0x02},
- {0x375d, 0x00},
- {0x375e, 0x00},
- {0x375f, 0x11},
- {0x3760, 0x01},
- {0x3768, 0x1b},
- {0x3769, 0x1b},
- {0x376a, 0x1b},
- {0x376b, 0x1b},
- {0x376c, 0x1a},
- {0x376d, 0x17},
- {0x376e, 0x0f},
- {0x3776, 0x00},
- {0x3777, 0x00},
- {0x3778, 0x46},
- {0x3779, 0x00},
- {0x377a, 0x89},
- {0x377b, 0x00},
- {0x377c, 0x08},
- {0x377d, 0x01},
- {0x377e, 0x23},
- {0x377f, 0x02},
- {0x3780, 0xd9},
- {0x3781, 0x03},
- {0x3782, 0xf5},
- {0x3783, 0x06},
- {0x3784, 0xa5},
- {0x3788, 0x0f},
- {0x378a, 0xd9},
- {0x378b, 0x03},
- {0x378c, 0xeb},
- {0x378d, 0x05},
- {0x378e, 0x87},
- {0x378f, 0x06},
- {0x3790, 0xf5},
- {0x3792, 0x43},
- {0x3794, 0x7a},
- {0x3796, 0xa1},
- {0x37b0, 0x36},
- {0x3a00, 0x00},
+static const struct cci_reg_sequence mode_2592x1940_regs[] = {
+ {IMX335_REG_MODE_SELECT, 0x01},
+ {IMX335_REG_MASTER_MODE, 0x00},
+ {IMX335_REG_WINMODE, 0x04},
+ {IMX335_REG_HTRIMMING_START, 0x0180},
+ {IMX335_REG_HNUM, 0x0a20},
+ {IMX335_REG_Y_OUT_SIZE, 0x0794},
+ {IMX335_REG_VCROP_POS, 0x00b0},
+ {IMX335_REG_VCROP_SIZE, 0x0f58},
+ {IMX335_REG_OPB_SIZE_V, 0x00},
+ {IMX335_REG_XVS_XHS_DRV, 0x00},
+ {CCI_REG8(0x3288), 0x21}, /* undocumented */
+ {CCI_REG8(0x328a), 0x02}, /* undocumented */
+ {CCI_REG8(0x3414), 0x05}, /* undocumented */
+ {CCI_REG8(0x3416), 0x18}, /* undocumented */
+ {CCI_REG8(0x3648), 0x01}, /* undocumented */
+ {CCI_REG8(0x364a), 0x04}, /* undocumented */
+ {CCI_REG8(0x364c), 0x04}, /* undocumented */
+ {CCI_REG8(0x3678), 0x01}, /* undocumented */
+ {CCI_REG8(0x367c), 0x31}, /* undocumented */
+ {CCI_REG8(0x367e), 0x31}, /* undocumented */
+ {CCI_REG8(0x3706), 0x10}, /* undocumented */
+ {CCI_REG8(0x3708), 0x03}, /* undocumented */
+ {CCI_REG8(0x3714), 0x02}, /* undocumented */
+ {CCI_REG8(0x3715), 0x02}, /* undocumented */
+ {CCI_REG8(0x3716), 0x01}, /* undocumented */
+ {CCI_REG8(0x3717), 0x03}, /* undocumented */
+ {CCI_REG8(0x371c), 0x3d}, /* undocumented */
+ {CCI_REG8(0x371d), 0x3f}, /* undocumented */
+ {CCI_REG8(0x372c), 0x00}, /* undocumented */
+ {CCI_REG8(0x372d), 0x00}, /* undocumented */
+ {CCI_REG8(0x372e), 0x46}, /* undocumented */
+ {CCI_REG8(0x372f), 0x00}, /* undocumented */
+ {CCI_REG8(0x3730), 0x89}, /* undocumented */
+ {CCI_REG8(0x3731), 0x00}, /* undocumented */
+ {CCI_REG8(0x3732), 0x08}, /* undocumented */
+ {CCI_REG8(0x3733), 0x01}, /* undocumented */
+ {CCI_REG8(0x3734), 0xfe}, /* undocumented */
+ {CCI_REG8(0x3735), 0x05}, /* undocumented */
+ {CCI_REG8(0x3740), 0x02}, /* undocumented */
+ {CCI_REG8(0x375d), 0x00}, /* undocumented */
+ {CCI_REG8(0x375e), 0x00}, /* undocumented */
+ {CCI_REG8(0x375f), 0x11}, /* undocumented */
+ {CCI_REG8(0x3760), 0x01}, /* undocumented */
+ {CCI_REG8(0x3768), 0x1b}, /* undocumented */
+ {CCI_REG8(0x3769), 0x1b}, /* undocumented */
+ {CCI_REG8(0x376a), 0x1b}, /* undocumented */
+ {CCI_REG8(0x376b), 0x1b}, /* undocumented */
+ {CCI_REG8(0x376c), 0x1a}, /* undocumented */
+ {CCI_REG8(0x376d), 0x17}, /* undocumented */
+ {CCI_REG8(0x376e), 0x0f}, /* undocumented */
+ {CCI_REG8(0x3776), 0x00}, /* undocumented */
+ {CCI_REG8(0x3777), 0x00}, /* undocumented */
+ {CCI_REG8(0x3778), 0x46}, /* undocumented */
+ {CCI_REG8(0x3779), 0x00}, /* undocumented */
+ {CCI_REG8(0x377a), 0x89}, /* undocumented */
+ {CCI_REG8(0x377b), 0x00}, /* undocumented */
+ {CCI_REG8(0x377c), 0x08}, /* undocumented */
+ {CCI_REG8(0x377d), 0x01}, /* undocumented */
+ {CCI_REG8(0x377e), 0x23}, /* undocumented */
+ {CCI_REG8(0x377f), 0x02}, /* undocumented */
+ {CCI_REG8(0x3780), 0xd9}, /* undocumented */
+ {CCI_REG8(0x3781), 0x03}, /* undocumented */
+ {CCI_REG8(0x3782), 0xf5}, /* undocumented */
+ {CCI_REG8(0x3783), 0x06}, /* undocumented */
+ {CCI_REG8(0x3784), 0xa5}, /* undocumented */
+ {CCI_REG8(0x3788), 0x0f}, /* undocumented */
+ {CCI_REG8(0x378a), 0xd9}, /* undocumented */
+ {CCI_REG8(0x378b), 0x03}, /* undocumented */
+ {CCI_REG8(0x378c), 0xeb}, /* undocumented */
+ {CCI_REG8(0x378d), 0x05}, /* undocumented */
+ {CCI_REG8(0x378e), 0x87}, /* undocumented */
+ {CCI_REG8(0x378f), 0x06}, /* undocumented */
+ {CCI_REG8(0x3790), 0xf5}, /* undocumented */
+ {CCI_REG8(0x3792), 0x43}, /* undocumented */
+ {CCI_REG8(0x3794), 0x7a}, /* undocumented */
+ {CCI_REG8(0x3796), 0xa1}, /* undocumented */
+ {CCI_REG8(0x37b0), 0x36}, /* undocumented */
+ {CCI_REG8(0x3a00), 0x00}, /* undocumented */
};
-static const struct imx335_reg raw10_framefmt_regs[] = {
- {0x3050, 0x00},
- {0x319d, 0x00},
- {0x341c, 0xff},
- {0x341d, 0x01},
+static const struct cci_reg_sequence raw10_framefmt_regs[] = {
+ {IMX335_REG_ADBIT, 0x00},
+ {IMX335_REG_MDBIT, 0x00},
+ {IMX335_REG_ADBIT1, 0x1ff},
};
-static const struct imx335_reg raw12_framefmt_regs[] = {
- {0x3050, 0x01},
- {0x319d, 0x01},
- {0x341c, 0x47},
- {0x341d, 0x00},
+static const struct cci_reg_sequence raw12_framefmt_regs[] = {
+ {IMX335_REG_ADBIT, 0x01},
+ {IMX335_REG_MDBIT, 0x01},
+ {IMX335_REG_ADBIT1, 0x47},
};
-static const struct imx335_reg mipi_data_rate_1188Mbps[] = {
- {0x300c, 0x3b},
- {0x300d, 0x2a},
- {0x314c, 0xc6},
- {0x314d, 0x00},
- {0x315a, 0x02},
- {0x3168, 0xa0},
- {0x316a, 0x7e},
- {0x319e, 0x01},
- {0x3a18, 0x8f},
- {0x3a1a, 0x4f},
- {0x3a1c, 0x47},
- {0x3a1e, 0x37},
- {0x3a1f, 0x01},
- {0x3a20, 0x4f},
- {0x3a22, 0x87},
- {0x3a24, 0x4f},
- {0x3a26, 0x7f},
- {0x3a28, 0x3f},
+static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
+ {IMX335_REG_BCWAIT_TIME, 0x3b},
+ {IMX335_REG_CPWAIT_TIME, 0x2a},
+ {IMX335_REG_INCLKSEL1, 0x00c6},
+ {IMX335_REG_INCLKSEL2, 0x02},
+ {IMX335_REG_INCLKSEL3, 0xa0},
+ {IMX335_REG_INCLKSEL4, 0x7e},
+ {IMX335_REG_SYSMODE, 0x01},
+ {IMX335_REG_TCLKPOST, 0x8f},
+ {IMX335_REG_TCLKPREPARE, 0x4f},
+ {IMX335_REG_TCLK_TRAIL, 0x47},
+ {IMX335_REG_TCLK_ZERO, 0x0137},
+ {IMX335_REG_THS_PREPARE, 0x4f},
+ {IMX335_REG_THS_ZERO, 0x87},
+ {IMX335_REG_THS_TRAIL, 0x4f},
+ {IMX335_REG_THS_EXIT, 0x7f},
+ {IMX335_REG_TPLX, 0x3f},
};
-static const struct imx335_reg mipi_data_rate_891Mbps[] = {
- {0x300c, 0x3b},
- {0x300d, 0x2a},
- {0x314c, 0x29},
- {0x314d, 0x01},
- {0x315a, 0x06},
- {0x3168, 0xa0},
- {0x316a, 0x7e},
- {0x319e, 0x02},
- {0x3a18, 0x7f},
- {0x3a1a, 0x37},
- {0x3a1c, 0x37},
- {0x3a1e, 0xf7},
- {0x3a20, 0x3f},
- {0x3a22, 0x6f},
- {0x3a24, 0x3f},
- {0x3a26, 0x5f},
- {0x3a28, 0x2f},
+static const struct cci_reg_sequence mipi_data_rate_891Mbps[] = {
+ {IMX335_REG_BCWAIT_TIME, 0x3b},
+ {IMX335_REG_CPWAIT_TIME, 0x2a},
+ {IMX335_REG_INCLKSEL1, 0x0129},
+ {IMX335_REG_INCLKSEL2, 0x06},
+ {IMX335_REG_INCLKSEL3, 0xa0},
+ {IMX335_REG_INCLKSEL4, 0x7e},
+ {IMX335_REG_SYSMODE, 0x02},
+ {IMX335_REG_TCLKPOST, 0x7f},
+ {IMX335_REG_TCLKPREPARE, 0x37},
+ {IMX335_REG_TCLK_TRAIL, 0x37},
+ {IMX335_REG_TCLK_ZERO, 0xf7},
+ {IMX335_REG_THS_PREPARE, 0x3f},
+ {IMX335_REG_THS_ZERO, 0x6f},
+ {IMX335_REG_THS_TRAIL, 0x3f},
+ {IMX335_REG_THS_EXIT, 0x5f},
+ {IMX335_REG_TPLX, 0x2f},
};
static const s64 link_freq[] = {
@@ -397,101 +425,6 @@ static inline struct imx335 *to_imx335(struct v4l2_subdev *subdev)
return container_of(subdev, struct imx335, sd);
}
-/**
- * imx335_read_reg() - Read registers.
- * @imx335: pointer to imx335 device
- * @reg: register address
- * @len: length of bytes to read. Max supported bytes is 4
- * @val: pointer to register value to be filled.
- *
- * Big endian register addresses with little endian values.
- *
- * Return: 0 if successful, error code otherwise.
- */
-static int imx335_read_reg(struct imx335 *imx335, u16 reg, u32 len, u32 *val)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
- struct i2c_msg msgs[2] = {0};
- u8 addr_buf[2] = {0};
- u8 data_buf[4] = {0};
- int ret;
-
- if (WARN_ON(len > 4))
- return -EINVAL;
-
- put_unaligned_be16(reg, addr_buf);
-
- /* Write register address */
- msgs[0].addr = client->addr;
- msgs[0].flags = 0;
- msgs[0].len = ARRAY_SIZE(addr_buf);
- msgs[0].buf = addr_buf;
-
- /* Read data from register */
- msgs[1].addr = client->addr;
- msgs[1].flags = I2C_M_RD;
- msgs[1].len = len;
- msgs[1].buf = data_buf;
-
- ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
- if (ret != ARRAY_SIZE(msgs))
- return -EIO;
-
- *val = get_unaligned_le32(data_buf);
-
- return 0;
-}
-
-/**
- * imx335_write_reg() - Write register
- * @imx335: pointer to imx335 device
- * @reg: register address
- * @len: length of bytes. Max supported bytes is 4
- * @val: register value
- *
- * Big endian register addresses with little endian values.
- *
- * Return: 0 if successful, error code otherwise.
- */
-static int imx335_write_reg(struct imx335 *imx335, u16 reg, u32 len, u32 val)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
- u8 buf[6] = {0};
-
- if (WARN_ON(len > 4))
- return -EINVAL;
-
- put_unaligned_be16(reg, buf);
- put_unaligned_le32(val, buf + 2);
- if (i2c_master_send(client, buf, len + 2) != len + 2)
- return -EIO;
-
- return 0;
-}
-
-/**
- * imx335_write_regs() - Write a list of registers
- * @imx335: pointer to imx335 device
- * @regs: list of registers to be written
- * @len: length of registers array
- *
- * Return: 0 if successful. error code otherwise.
- */
-static int imx335_write_regs(struct imx335 *imx335,
- const struct imx335_reg *regs, u32 len)
-{
- unsigned int i;
- int ret;
-
- for (i = 0; i < len; i++) {
- ret = imx335_write_reg(imx335, regs[i].address, 1, regs[i].val);
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
/**
* imx335_update_controls() - Update control ranges based on streaming mode
* @imx335: pointer to imx335 device
@@ -528,7 +461,7 @@ static int imx335_update_controls(struct imx335 *imx335,
static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
{
u32 lpfr, shutter;
- int ret;
+ int ret = 0;
lpfr = imx335->vblank + imx335->cur_mode->height;
shutter = lpfr - exposure;
@@ -536,64 +469,49 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
exposure, gain, shutter, lpfr);
- ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
- if (ret)
- return ret;
-
- ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
- if (ret)
- goto error_release_group_hold;
-
- ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
- if (ret)
- goto error_release_group_hold;
-
- ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);
-
-error_release_group_hold:
- imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);
+ cci_write(imx335->cci, IMX335_REG_HOLD, 1, &ret);
+ cci_write(imx335->cci, IMX335_REG_VMAX, lpfr, &ret);
+ cci_write(imx335->cci, IMX335_REG_SHUTTER, shutter, &ret);
+ cci_write(imx335->cci, IMX335_REG_AGAIN, gain, &ret);
+ cci_write(imx335->cci, IMX335_REG_HOLD, 0, &ret);
return ret;
}
static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
{
- int ret;
+ int ret = 0;
if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
return -EINVAL;
if (pattern_index) {
- const struct imx335_reg tpg_enable_regs[] = {
- { 0x3148, 0x10 },
- { 0x3280, 0x00 },
- { 0x329c, 0x01 },
- { 0x32a0, 0x11 },
- { 0x3302, 0x00 },
- { 0x3303, 0x00 },
- { 0x336c, 0x00 },
+ const struct cci_reg_sequence tpg_enable_regs[] = {
+ {IMX335_REG_TPG_TESTCLKEN, 0x10},
+ {IMX335_REG_TPG_DIG_CLP_MODE, 0x00},
+ {IMX335_REG_TPG_EN_DUOUT, 0x01},
+ {IMX335_REG_TPG_COLORWIDTH, 0x11},
+ {IMX335_REG_TPG_BLKLEVEL, 0x00},
+ {IMX335_REG_TPG_WRJ_OPEN, 0x00},
};
- ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1,
- imx335_tpg_val[pattern_index]);
- if (ret)
- return ret;
+ cci_write(imx335->cci, IMX335_REG_TPG,
+ imx335_tpg_val[pattern_index], &ret);
- ret = imx335_write_regs(imx335, tpg_enable_regs,
- ARRAY_SIZE(tpg_enable_regs));
+ cci_multi_reg_write(imx335->cci, tpg_enable_regs,
+ ARRAY_SIZE(tpg_enable_regs), &ret);
} else {
- const struct imx335_reg tpg_disable_regs[] = {
- { 0x3148, 0x00 },
- { 0x3280, 0x01 },
- { 0x329c, 0x00 },
- { 0x32a0, 0x10 },
- { 0x3302, 0x32 },
- { 0x3303, 0x00 },
- { 0x336c, 0x01 },
+ const struct cci_reg_sequence tpg_disable_regs[] = {
+ {IMX335_REG_TPG_TESTCLKEN, 0x00},
+ {IMX335_REG_TPG_DIG_CLP_MODE, 0x01},
+ {IMX335_REG_TPG_EN_DUOUT, 0x00},
+ {IMX335_REG_TPG_COLORWIDTH, 0x10},
+ {IMX335_REG_TPG_BLKLEVEL, 0x32},
+ {IMX335_REG_TPG_WRJ_OPEN, 0x01},
};
- ret = imx335_write_regs(imx335, tpg_disable_regs,
- ARRAY_SIZE(tpg_disable_regs));
+ cci_multi_reg_write(imx335->cci, tpg_disable_regs,
+ ARRAY_SIZE(tpg_disable_regs), &ret);
}
return ret;
@@ -892,12 +810,14 @@ static int imx335_set_framefmt(struct imx335 *imx335)
{
switch (imx335->cur_mbus_code) {
case MEDIA_BUS_FMT_SRGGB10_1X10:
- return imx335_write_regs(imx335, raw10_framefmt_regs,
- ARRAY_SIZE(raw10_framefmt_regs));
+ return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
+ ARRAY_SIZE(raw10_framefmt_regs),
+ NULL);
case MEDIA_BUS_FMT_SRGGB12_1X12:
- return imx335_write_regs(imx335, raw12_framefmt_regs,
- ARRAY_SIZE(raw12_framefmt_regs));
+ return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
+ ARRAY_SIZE(raw12_framefmt_regs),
+ NULL);
}
return -EINVAL;
@@ -916,7 +836,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
/* Setup PLL */
reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
- ret = imx335_write_regs(imx335, reg_list->regs, reg_list->num_of_regs);
+ ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
+ reg_list->num_of_regs, NULL);
if (ret) {
dev_err(imx335->dev, "%s failed to set plls\n", __func__);
return ret;
@@ -924,8 +845,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
/* Write sensor mode registers */
reg_list = &imx335->cur_mode->reg_list;
- ret = imx335_write_regs(imx335, reg_list->regs,
- reg_list->num_of_regs);
+ ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
+ reg_list->num_of_regs, NULL);
if (ret) {
dev_err(imx335->dev, "fail to write initial registers\n");
return ret;
@@ -939,7 +860,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
}
/* Configure lanes */
- ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
+ ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
+ imx335->lane_mode, NULL);
if (ret)
return ret;
@@ -951,8 +873,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
}
/* Start streaming */
- ret = imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
- 1, IMX335_MODE_STREAMING);
+ ret = cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
+ IMX335_MODE_STREAMING, NULL);
if (ret) {
dev_err(imx335->dev, "fail to start streaming\n");
return ret;
@@ -972,8 +894,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
*/
static int imx335_stop_streaming(struct imx335 *imx335)
{
- return imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
- 1, IMX335_MODE_STANDBY);
+ return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
+ IMX335_MODE_STANDBY, NULL);
}
/**
@@ -1024,14 +946,14 @@ static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
static int imx335_detect(struct imx335 *imx335)
{
int ret;
- u32 val;
+ u64 val;
- ret = imx335_read_reg(imx335, IMX335_REG_ID, 2, &val);
+ ret = cci_read(imx335->cci, IMX335_REG_ID, &val, NULL);
if (ret)
return ret;
if (val != IMX335_ID) {
- dev_err(imx335->dev, "chip id mismatch: %x!=%x\n",
+ dev_err(imx335->dev, "chip id mismatch: %x!=%llx\n",
IMX335_ID, val);
return -ENXIO;
}
@@ -1345,6 +1267,11 @@ static int imx335_probe(struct i2c_client *client)
return -ENOMEM;
imx335->dev = &client->dev;
+ imx335->cci = devm_cci_regmap_init_i2c(client, 16);
+ if (IS_ERR(imx335->cci)) {
+ dev_err(imx335->dev, "Unable to initialize I2C\n");
+ return -ENODEV;
+ }
/* Initialize subdev */
v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops);
--
2.43.0
The imx335 reports a recommended pixel area of - 2592x1944.
The driver supported mode however limits it to height=1940.
Fix the height discrepency by correctly the value of height
(with updates to vblank and mode registers).
Signed-off-by: Umang Jain <[email protected]>
---
drivers/media/i2c/imx335.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 6ea09933e47b..c00e0c2be3f3 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -246,13 +246,13 @@ static const int imx335_tpg_val[] = {
};
/* Sensor mode registers */
-static const struct cci_reg_sequence mode_2592x1940_regs[] = {
+static const struct cci_reg_sequence mode_2592x1944_regs[] = {
{IMX335_REG_MODE_SELECT, 0x01},
{IMX335_REG_MASTER_MODE, 0x00},
- {IMX335_REG_WINMODE, 0x04},
- {IMX335_REG_HTRIMMING_START, 0x0180},
+ {IMX335_REG_WINMODE, 0x00},
+ {IMX335_REG_HTRIMMING_START, 0x30},
{IMX335_REG_HNUM, 0x0a20},
- {IMX335_REG_Y_OUT_SIZE, 0x0794},
+ {IMX335_REG_Y_OUT_SIZE, 0x0798},
{IMX335_REG_VCROP_POS, 0x00b0},
{IMX335_REG_VCROP_SIZE, 0x0f58},
{IMX335_REG_OPB_SIZE_V, 0x00},
@@ -403,14 +403,14 @@ static const u32 imx335_mbus_codes[] = {
/* Supported sensor mode configurations */
static const struct imx335_mode supported_mode = {
.width = 2592,
- .height = 1940,
+ .height = 1944,
.hblank = 342,
- .vblank = 2560,
- .vblank_min = 2560,
+ .vblank = 2556,
+ .vblank_min = 2556,
.vblank_max = 133060,
.reg_list = {
- .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
- .regs = mode_2592x1940_regs,
+ .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
+ .regs = mode_2592x1944_regs,
},
};
--
2.43.0
From: Kieran Bingham <[email protected]>
Call the V4L2 fwnode device parser to handle controls that are
standardised by the framework.
Signed-off-by: Kieran Bingham <[email protected]>
Signed-off-by: Umang Jain <[email protected]>
---
drivers/media/i2c/imx335.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index a42f48823515..7f3f74240cd0 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -1222,13 +1222,15 @@ static int imx335_init_controls(struct imx335 *imx335)
{
struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
const struct imx335_mode *mode = imx335->cur_mode;
+ struct v4l2_fwnode_device_properties props;
u32 lpfr;
u64 pclk;
s64 link_freq_in_use;
u8 bpp;
int ret;
- ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
+ /* v4l2_fwnode_device_properties can add two more controls */
+ ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
if (ret)
return ret;
@@ -1313,6 +1315,15 @@ static int imx335_init_controls(struct imx335 *imx335)
return ctrl_hdlr->error;
}
+ ret = v4l2_fwnode_device_parse(imx335->dev, &props);
+ if (ret)
+ return ret;
+
+ ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx335_ctrl_ops,
+ &props);
+ if (ret)
+ return ret;
+
imx335->sd.ctrl_handler = ctrl_hdlr;
return 0;
--
2.43.0
The sensor gain (both analog and digital) are controlled by a
single gain value where:
- 0dB to 30dB correspond to analog gain
- 30.3dB to 72dB correspond to digital gain
(with 0.3dB step)
Hence, limit the analogue gain value to 100.
For digital gain, support can be added later if needed.
Signed-off-by: Umang Jain <[email protected]>
---
drivers/media/i2c/imx335.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index c00e0c2be3f3..701bd5318b45 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -45,7 +45,7 @@
/* Analog gain control */
#define IMX335_REG_AGAIN CCI_REG8(0x30e8)
#define IMX335_AGAIN_MIN 0
-#define IMX335_AGAIN_MAX 240
+#define IMX335_AGAIN_MAX 100
#define IMX335_AGAIN_STEP 1
#define IMX335_AGAIN_DEFAULT 0
@@ -1169,6 +1169,14 @@ static int imx335_init_controls(struct imx335 *imx335)
IMX335_EXPOSURE_STEP,
IMX335_EXPOSURE_DEFAULT);
+ /*
+ * The sensor has an analog gain and a digital gain, both controlled
+ * through a single gain value, expressed in 0.3dB increments. Values
+ * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
+ * up to 72.0dB (240) add further digital gain. Limit the range to
+ * analog gain only, support for digital gain can be added separately
+ * if needed.
+ */
imx335->again_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
&imx335_ctrl_ops,
V4L2_CID_ANALOGUE_GAIN,
--
2.43.0
Hi Umang and Kieran
On Wed, 6 Mar 2024 at 08:11, Umang Jain <[email protected]> wrote:
>
> From: Kieran Bingham <[email protected]>
>
> The IMX335 can support both 2 and 4 lane configurations.
> Extend the driver to configure the lane mode accordingly.
> Update the pixel rate depending on the number of lanes in use.
>
> Signed-off-by: Kieran Bingham <[email protected]>
> Signed-off-by: Umang Jain <[email protected]>
> ---
> drivers/media/i2c/imx335.c | 46 +++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index dab6d080bc4c..a42f48823515 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -21,6 +21,11 @@
> #define IMX335_MODE_STANDBY 0x01
> #define IMX335_MODE_STREAMING 0x00
>
> +/* Data Lanes */
> +#define IMX335_LANEMODE 0x3a01
> +#define IMX335_2LANE 1
> +#define IMX335_4LANE 3
> +
> /* Lines per frame */
> #define IMX335_REG_LPFR 0x3030
>
> @@ -67,8 +72,6 @@
> #define IMX335_LINK_FREQ_594MHz 594000000LL
> #define IMX335_LINK_FREQ_445MHz 445500000LL
>
> -#define IMX335_NUM_DATA_LANES 4
> -
> #define IMX335_REG_MIN 0x00
> #define IMX335_REG_MAX 0xfffff
>
> @@ -115,7 +118,6 @@ static const char * const imx335_supply_name[] = {
> * @vblank: Vertical blanking in lines
> * @vblank_min: Minimum vertical blanking in lines
> * @vblank_max: Maximum vertical blanking in lines
> - * @pclk: Sensor pixel clock
> * @reg_list: Register list for sensor mode
> */
> struct imx335_mode {
> @@ -126,7 +128,6 @@ struct imx335_mode {
> u32 vblank;
> u32 vblank_min;
> u32 vblank_max;
> - u64 pclk;
> struct imx335_reg_list reg_list;
> };
>
> @@ -147,6 +148,7 @@ struct imx335_mode {
> * @exp_ctrl: Pointer to exposure control
> * @again_ctrl: Pointer to analog gain control
> * @vblank: Vertical blanking in lines
> + * @lane_mode Mode for number of connected data lanes
> * @cur_mode: Pointer to current selected sensor mode
> * @mutex: Mutex for serializing sensor controls
> * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> @@ -171,6 +173,7 @@ struct imx335 {
> struct v4l2_ctrl *again_ctrl;
> };
> u32 vblank;
> + u32 lane_mode;
> const struct imx335_mode *cur_mode;
> struct mutex mutex;
> unsigned long link_freq_bitmap;
> @@ -377,7 +380,6 @@ static const struct imx335_mode supported_mode = {
> .vblank = 2560,
> .vblank_min = 2560,
> .vblank_max = 133060,
> - .pclk = 396000000,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
> .regs = mode_2592x1940_regs,
> @@ -936,6 +938,11 @@ static int imx335_start_streaming(struct imx335 *imx335)
> return ret;
> }
>
> + /* Configure lanes */
> + ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
> + if (ret)
> + return ret;
> +
> /* Setup handler will write actual exposure and gain */
> ret = __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
> if (ret) {
> @@ -1096,7 +1103,14 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
> if (ret)
> return ret;
>
> - if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX335_NUM_DATA_LANES) {
> + switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
> + case 2:
> + imx335->lane_mode = IMX335_2LANE;
> + break;
> + case 4:
> + imx335->lane_mode = IMX335_4LANE;
> + break;
> + default:
> dev_err(imx335->dev,
> "number of CSI2 data lanes %d is not supported\n",
> bus_cfg.bus.mipi_csi2.num_data_lanes);
> @@ -1209,6 +1223,9 @@ static int imx335_init_controls(struct imx335 *imx335)
> struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
> const struct imx335_mode *mode = imx335->cur_mode;
> u32 lpfr;
> + u64 pclk;
> + s64 link_freq_in_use;
> + u8 bpp;
> int ret;
>
> ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
> @@ -1252,11 +1269,24 @@ static int imx335_init_controls(struct imx335 *imx335)
> 0, 0, imx335_tpg_menu);
>
> /* Read only controls */
> +
> + /* pixel rate = link frequency * lanes * 2 / bits_per_pixel */
> + switch (imx335->cur_mbus_code) {
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + bpp = 10;
> + break;
> + case MEDIA_BUS_FMT_SRGGB12_1X12:
> + bpp = 12;
> + break;
> + }
> +
> + link_freq_in_use = link_freq[__ffs(imx335->link_freq_bitmap)];
> + pclk = link_freq_in_use * (imx335->lane_mode + 1) * 2 / bpp;
> imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> &imx335_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> - mode->pclk, mode->pclk,
> - 1, mode->pclk);
> + pclk, pclk,
> + 1, pclk);
Is this actually correct?
A fair number of the sensors I've encountered have 2 PLL paths - one
for the pixel array, and one for the CSI block. The bpp will generally
be fed into the CSI block PLL path, but not into the pixel array one.
The link frequency will therefore vary with bit depth, but
V4L2_CID_PIXEL_RATE doesn't change.
imx290 certainly has a disjoin between pixel rate and link freq
(cropping reduces link freq, but not pixel rate), and we run imx477 in
2 lane mode with the pixel array at full tilt (840MPix/s) but large
horizontal blanking to allow CSI2 enough time to send the data.
If you've validated that for a range of frame rates you get the
correct output from the sensor in both 10 and 12 bit modes, then I
don't object. I just have an instinctive tick whenever I see drivers
computing PIXEL_RATE from LINK_FREQ or vice versa :)
If you get the right frame rate it may also imply that the link
frequency isn't as configured, but that rarely has any negative
effects. You need a reasonably good oscilloscope to be able to measure
the link frequency.
Dave
>
> imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> &imx335_ctrl_ops,
> --
> 2.43.0
>
>
Quoting Umang Jain (2024-03-06 08:10:36)
> Use the new comon CCI register access helpers to replace the private
> register access helpers in the imx335 driver.
>
> Select V4L2_CCI_I2C Kconfig option which the imx335 driver now
> depends on.
>
> Signed-off-by: Umang Jain <[email protected]>
> ---
> drivers/media/i2c/Kconfig | 1 +
> drivers/media/i2c/imx335.c | 521 ++++++++++++++++---------------------
> 2 files changed, 225 insertions(+), 297 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 56f276b920ab..8d248b9c9562 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -195,6 +195,7 @@ config VIDEO_IMX334
> config VIDEO_IMX335
> tristate "Sony IMX335 sensor support"
> depends on OF_GPIO
> + select V4L2_CCI_I2C
> help
> This is a Video4Linux2 sensor driver for the Sony
> IMX335 camera.
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 7f3f74240cd0..6ea09933e47b 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -11,47 +11,49 @@
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>
> +#include <media/v4l2-cci.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> /* Streaming Mode */
> -#define IMX335_REG_MODE_SELECT 0x3000
> +#define IMX335_REG_MODE_SELECT CCI_REG8(0x3000)
> #define IMX335_MODE_STANDBY 0x01
> #define IMX335_MODE_STREAMING 0x00
>
> /* Data Lanes */
> -#define IMX335_LANEMODE 0x3a01
> +#define IMX335_REG_LANEMODE CCI_REG8(0x3a01)
> #define IMX335_2LANE 1
> #define IMX335_4LANE 3
>
> /* Lines per frame */
> -#define IMX335_REG_LPFR 0x3030
> +#define IMX335_REG_VMAX CCI_REG24_LE(0x3030)
>
> /* Chip ID */
> -#define IMX335_REG_ID 0x3912
> +#define IMX335_REG_ID CCI_REG8(0x3912)
> #define IMX335_ID 0x00
>
> /* Exposure control */
> -#define IMX335_REG_SHUTTER 0x3058
> +#define IMX335_REG_SHUTTER CCI_REG24_LE(0x3058)
> #define IMX335_EXPOSURE_MIN 1
> #define IMX335_EXPOSURE_OFFSET 9
> #define IMX335_EXPOSURE_STEP 1
> #define IMX335_EXPOSURE_DEFAULT 0x0648
>
> /* Analog gain control */
> -#define IMX335_REG_AGAIN 0x30e8
> +#define IMX335_REG_AGAIN CCI_REG8(0x30e8)
> #define IMX335_AGAIN_MIN 0
> #define IMX335_AGAIN_MAX 240
> #define IMX335_AGAIN_STEP 1
> #define IMX335_AGAIN_DEFAULT 0
>
> /* Group hold register */
> -#define IMX335_REG_HOLD 0x3001
> +#define IMX335_REG_HOLD CCI_REG8(0x3001)
>
> /* Test pattern generator */
> -#define IMX335_REG_TPG 0x329e
> +#define IMX335_REG_TPG CCI_REG8(0x329e)
> #define IMX335_TPG_ALL_000 0
> #define IMX335_TPG_ALL_FFF 1
> #define IMX335_TPG_ALL_555 2
> @@ -65,6 +67,46 @@
> #define IMX335_TPG_H_COLOR_BARS 10
> #define IMX335_TPG_V_COLOR_BARS 11
>
> +#define IMX335_REG_MASTER_MODE CCI_REG8(0x3002)
> +#define IMX335_REG_WINMODE CCI_REG8(0x3018)
> +#define IMX335_REG_HTRIMMING_START CCI_REG16_LE(0x302c)
> +#define IMX335_REG_HNUM CCI_REG8(0x302e)
> +#define IMX335_REG_XVS_XHS_DRV CCI_REG8(0x31a1)
> +#define IMX335_REG_Y_OUT_SIZE CCI_REG16_LE(0x3056)
> +#define IMX335_REG_VCROP_POS CCI_REG16_LE(0x3074)
> +#define IMX335_REG_VCROP_SIZE CCI_REG16_LE(0x3076)
> +#define IMX335_REG_OPB_SIZE_V CCI_REG8(0x304c)
> +
> +#define IMX335_REG_ADBIT CCI_REG8(0x3050)
> +#define IMX335_REG_MDBIT CCI_REG8(0x319d)
> +#define IMX335_REG_ADBIT1 CCI_REG16_LE(0x341c)
> +
> +#define IMX335_REG_BCWAIT_TIME CCI_REG8(0x300c)
> +#define IMX335_REG_CPWAIT_TIME CCI_REG8(0x300d)
> +#define IMX335_REG_INCLKSEL1 CCI_REG16_LE(0x314c)
> +#define IMX335_REG_INCLKSEL2 CCI_REG8(0x315a)
> +#define IMX335_REG_INCLKSEL3 CCI_REG8(0x3168)
> +#define IMX335_REG_INCLKSEL4 CCI_REG8(0x316a)
> +#define IMX335_REG_SYSMODE CCI_REG8(0x319e)
> +
> +#define IMX335_REG_TCLKPOST CCI_REG16_LE(0x3a18)
> +#define IMX335_REG_TCLKPREPARE CCI_REG16_LE(0x3a1a)
> +#define IMX335_REG_TCLK_TRAIL CCI_REG16_LE(0x3a1c)
> +#define IMX335_REG_TCLK_ZERO CCI_REG16_LE(0x3a1e)
> +#define IMX335_REG_THS_PREPARE CCI_REG16_LE(0x3a20)
> +#define IMX335_REG_THS_ZERO CCI_REG16_LE(0x3a22)
> +#define IMX335_REG_THS_TRAIL CCI_REG16_LE(0x3a24)
> +#define IMX335_REG_THS_EXIT CCI_REG16_LE(0x3a26)
> +#define IMX335_REG_TPLX CCI_REG16_LE(0x3a28)
> +
> +
> +#define IMX335_REG_TPG_TESTCLKEN CCI_REG8(0x3148)
> +#define IMX335_REG_TPG_DIG_CLP_MODE CCI_REG8(0x3280)
> +#define IMX335_REG_TPG_EN_DUOUT CCI_REG8(0x329c)
> +#define IMX335_REG_TPG_COLORWIDTH CCI_REG8(0x32a0)
> +#define IMX335_REG_TPG_BLKLEVEL CCI_REG16_LE(0x3302)
> +#define IMX335_REG_TPG_WRJ_OPEN CCI_REG8(0x336c)
I would try to keep all these new definitions in register address order,
along with the existing definitions rather than add them at the end.
> +
> /* Input clock rate */
> #define IMX335_INCLK_RATE 24000000
>
> @@ -83,16 +125,6 @@
> #define IMX335_PIXEL_ARRAY_WIDTH 2592U
> #define IMX335_PIXEL_ARRAY_HEIGHT 1944U
>
> -/**
> - * struct imx335_reg - imx335 sensor register
> - * @address: Register address
> - * @val: Register value
> - */
> -struct imx335_reg {
> - u16 address;
> - u8 val;
> -};
> -
> /**
> * struct imx335_reg_list - imx335 sensor register list
> * @num_of_regs: Number of registers in the list
> @@ -100,7 +132,7 @@ struct imx335_reg {
> */
> struct imx335_reg_list {
> u32 num_of_regs;
> - const struct imx335_reg *regs;
> + const struct cci_reg_sequence *regs;
> };
>
> static const char * const imx335_supply_name[] = {
> @@ -161,6 +193,7 @@ struct imx335 {
> struct media_pad pad;
> struct gpio_desc *reset_gpio;
> struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
> + struct regmap *cci;
>
> struct clk *inclk;
> struct v4l2_ctrl_handler ctrl_handler;
> @@ -213,140 +246,135 @@ static const int imx335_tpg_val[] = {
> };
>
> /* Sensor mode registers */
> -static const struct imx335_reg mode_2592x1940_regs[] = {
> - {0x3000, 0x01},
> - {0x3002, 0x00},
> - {0x3018, 0x04},
> - {0x302c, 0x3c},
> - {0x302e, 0x20},
> - {0x3056, 0x94},
> - {0x3074, 0xc8},
> - {0x3076, 0x28},
> - {0x304c, 0x00},
> - {0x31a1, 0x00},
> - {0x3288, 0x21},
> - {0x328a, 0x02},
> - {0x3414, 0x05},
> - {0x3416, 0x18},
> - {0x3648, 0x01},
> - {0x364a, 0x04},
> - {0x364c, 0x04},
> - {0x3678, 0x01},
> - {0x367c, 0x31},
> - {0x367e, 0x31},
> - {0x3706, 0x10},
> - {0x3708, 0x03},
> - {0x3714, 0x02},
> - {0x3715, 0x02},
> - {0x3716, 0x01},
> - {0x3717, 0x03},
> - {0x371c, 0x3d},
> - {0x371d, 0x3f},
> - {0x372c, 0x00},
> - {0x372d, 0x00},
> - {0x372e, 0x46},
> - {0x372f, 0x00},
> - {0x3730, 0x89},
> - {0x3731, 0x00},
> - {0x3732, 0x08},
> - {0x3733, 0x01},
> - {0x3734, 0xfe},
> - {0x3735, 0x05},
> - {0x3740, 0x02},
> - {0x375d, 0x00},
> - {0x375e, 0x00},
> - {0x375f, 0x11},
> - {0x3760, 0x01},
> - {0x3768, 0x1b},
> - {0x3769, 0x1b},
> - {0x376a, 0x1b},
> - {0x376b, 0x1b},
> - {0x376c, 0x1a},
> - {0x376d, 0x17},
> - {0x376e, 0x0f},
> - {0x3776, 0x00},
> - {0x3777, 0x00},
> - {0x3778, 0x46},
> - {0x3779, 0x00},
> - {0x377a, 0x89},
> - {0x377b, 0x00},
> - {0x377c, 0x08},
> - {0x377d, 0x01},
> - {0x377e, 0x23},
> - {0x377f, 0x02},
> - {0x3780, 0xd9},
> - {0x3781, 0x03},
> - {0x3782, 0xf5},
> - {0x3783, 0x06},
> - {0x3784, 0xa5},
> - {0x3788, 0x0f},
> - {0x378a, 0xd9},
> - {0x378b, 0x03},
> - {0x378c, 0xeb},
> - {0x378d, 0x05},
> - {0x378e, 0x87},
> - {0x378f, 0x06},
> - {0x3790, 0xf5},
> - {0x3792, 0x43},
> - {0x3794, 0x7a},
> - {0x3796, 0xa1},
> - {0x37b0, 0x36},
> - {0x3a00, 0x00},
> +static const struct cci_reg_sequence mode_2592x1940_regs[] = {
> + {IMX335_REG_MODE_SELECT, 0x01},
> + {IMX335_REG_MASTER_MODE, 0x00},
> + {IMX335_REG_WINMODE, 0x04},
> + {IMX335_REG_HTRIMMING_START, 0x0180},
> + {IMX335_REG_HNUM, 0x0a20},
> + {IMX335_REG_Y_OUT_SIZE, 0x0794},
It could be nice for a patch on top to convert values that we would
consider 'integer' (like a size) to be changed to decimal for
readability (but not in this patch, I think it's better for review to
keep this as a conversion from hex values to hex values.
1940 will be far more comprehendable here than 0x0794. Same for any
other position or size register values... But perhaps they'll get
converted to more programattic implementations later too.
> + {IMX335_REG_VCROP_POS, 0x00b0},
> + {IMX335_REG_VCROP_SIZE, 0x0f58},
> + {IMX335_REG_OPB_SIZE_V, 0x00},
> + {IMX335_REG_XVS_XHS_DRV, 0x00},
> + {CCI_REG8(0x3288), 0x21}, /* undocumented */
> + {CCI_REG8(0x328a), 0x02}, /* undocumented */
> + {CCI_REG8(0x3414), 0x05}, /* undocumented */
> + {CCI_REG8(0x3416), 0x18}, /* undocumented */
> + {CCI_REG8(0x3648), 0x01}, /* undocumented */
> + {CCI_REG8(0x364a), 0x04}, /* undocumented */
> + {CCI_REG8(0x364c), 0x04}, /* undocumented */
> + {CCI_REG8(0x3678), 0x01}, /* undocumented */
> + {CCI_REG8(0x367c), 0x31}, /* undocumented */
> + {CCI_REG8(0x367e), 0x31}, /* undocumented */
> + {CCI_REG8(0x3706), 0x10}, /* undocumented */
> + {CCI_REG8(0x3708), 0x03}, /* undocumented */
> + {CCI_REG8(0x3714), 0x02}, /* undocumented */
> + {CCI_REG8(0x3715), 0x02}, /* undocumented */
> + {CCI_REG8(0x3716), 0x01}, /* undocumented */
> + {CCI_REG8(0x3717), 0x03}, /* undocumented */
> + {CCI_REG8(0x371c), 0x3d}, /* undocumented */
> + {CCI_REG8(0x371d), 0x3f}, /* undocumented */
> + {CCI_REG8(0x372c), 0x00}, /* undocumented */
> + {CCI_REG8(0x372d), 0x00}, /* undocumented */
> + {CCI_REG8(0x372e), 0x46}, /* undocumented */
> + {CCI_REG8(0x372f), 0x00}, /* undocumented */
> + {CCI_REG8(0x3730), 0x89}, /* undocumented */
> + {CCI_REG8(0x3731), 0x00}, /* undocumented */
> + {CCI_REG8(0x3732), 0x08}, /* undocumented */
> + {CCI_REG8(0x3733), 0x01}, /* undocumented */
> + {CCI_REG8(0x3734), 0xfe}, /* undocumented */
> + {CCI_REG8(0x3735), 0x05}, /* undocumented */
> + {CCI_REG8(0x3740), 0x02}, /* undocumented */
> + {CCI_REG8(0x375d), 0x00}, /* undocumented */
> + {CCI_REG8(0x375e), 0x00}, /* undocumented */
> + {CCI_REG8(0x375f), 0x11}, /* undocumented */
> + {CCI_REG8(0x3760), 0x01}, /* undocumented */
> + {CCI_REG8(0x3768), 0x1b}, /* undocumented */
> + {CCI_REG8(0x3769), 0x1b}, /* undocumented */
> + {CCI_REG8(0x376a), 0x1b}, /* undocumented */
> + {CCI_REG8(0x376b), 0x1b}, /* undocumented */
> + {CCI_REG8(0x376c), 0x1a}, /* undocumented */
> + {CCI_REG8(0x376d), 0x17}, /* undocumented */
> + {CCI_REG8(0x376e), 0x0f}, /* undocumented */
> + {CCI_REG8(0x3776), 0x00}, /* undocumented */
> + {CCI_REG8(0x3777), 0x00}, /* undocumented */
> + {CCI_REG8(0x3778), 0x46}, /* undocumented */
> + {CCI_REG8(0x3779), 0x00}, /* undocumented */
> + {CCI_REG8(0x377a), 0x89}, /* undocumented */
> + {CCI_REG8(0x377b), 0x00}, /* undocumented */
> + {CCI_REG8(0x377c), 0x08}, /* undocumented */
> + {CCI_REG8(0x377d), 0x01}, /* undocumented */
> + {CCI_REG8(0x377e), 0x23}, /* undocumented */
> + {CCI_REG8(0x377f), 0x02}, /* undocumented */
> + {CCI_REG8(0x3780), 0xd9}, /* undocumented */
> + {CCI_REG8(0x3781), 0x03}, /* undocumented */
> + {CCI_REG8(0x3782), 0xf5}, /* undocumented */
> + {CCI_REG8(0x3783), 0x06}, /* undocumented */
> + {CCI_REG8(0x3784), 0xa5}, /* undocumented */
> + {CCI_REG8(0x3788), 0x0f}, /* undocumented */
> + {CCI_REG8(0x378a), 0xd9}, /* undocumented */
> + {CCI_REG8(0x378b), 0x03}, /* undocumented */
> + {CCI_REG8(0x378c), 0xeb}, /* undocumented */
> + {CCI_REG8(0x378d), 0x05}, /* undocumented */
> + {CCI_REG8(0x378e), 0x87}, /* undocumented */
> + {CCI_REG8(0x378f), 0x06}, /* undocumented */
> + {CCI_REG8(0x3790), 0xf5}, /* undocumented */
> + {CCI_REG8(0x3792), 0x43}, /* undocumented */
> + {CCI_REG8(0x3794), 0x7a}, /* undocumented */
> + {CCI_REG8(0x3796), 0xa1}, /* undocumented */
> + {CCI_REG8(0x37b0), 0x36}, /* undocumented */
> + {CCI_REG8(0x3a00), 0x00}, /* undocumented */
Not much we can do about that at the moment :-(
> };
>
> -static const struct imx335_reg raw10_framefmt_regs[] = {
> - {0x3050, 0x00},
> - {0x319d, 0x00},
> - {0x341c, 0xff},
> - {0x341d, 0x01},
> +static const struct cci_reg_sequence raw10_framefmt_regs[] = {
> + {IMX335_REG_ADBIT, 0x00},
> + {IMX335_REG_MDBIT, 0x00},
> + {IMX335_REG_ADBIT1, 0x1ff},
> };
>
> -static const struct imx335_reg raw12_framefmt_regs[] = {
> - {0x3050, 0x01},
> - {0x319d, 0x01},
> - {0x341c, 0x47},
> - {0x341d, 0x00},
> +static const struct cci_reg_sequence raw12_framefmt_regs[] = {
> + {IMX335_REG_ADBIT, 0x01},
> + {IMX335_REG_MDBIT, 0x01},
> + {IMX335_REG_ADBIT1, 0x47},
> };
>
> -static const struct imx335_reg mipi_data_rate_1188Mbps[] = {
> - {0x300c, 0x3b},
> - {0x300d, 0x2a},
> - {0x314c, 0xc6},
> - {0x314d, 0x00},
> - {0x315a, 0x02},
> - {0x3168, 0xa0},
> - {0x316a, 0x7e},
> - {0x319e, 0x01},
> - {0x3a18, 0x8f},
> - {0x3a1a, 0x4f},
> - {0x3a1c, 0x47},
> - {0x3a1e, 0x37},
> - {0x3a1f, 0x01},
> - {0x3a20, 0x4f},
> - {0x3a22, 0x87},
> - {0x3a24, 0x4f},
> - {0x3a26, 0x7f},
> - {0x3a28, 0x3f},
> +static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
> + {IMX335_REG_BCWAIT_TIME, 0x3b},
> + {IMX335_REG_CPWAIT_TIME, 0x2a},
> + {IMX335_REG_INCLKSEL1, 0x00c6},
> + {IMX335_REG_INCLKSEL2, 0x02},
> + {IMX335_REG_INCLKSEL3, 0xa0},
> + {IMX335_REG_INCLKSEL4, 0x7e},
> + {IMX335_REG_SYSMODE, 0x01},
> + {IMX335_REG_TCLKPOST, 0x8f},
> + {IMX335_REG_TCLKPREPARE, 0x4f},
> + {IMX335_REG_TCLK_TRAIL, 0x47},
> + {IMX335_REG_TCLK_ZERO, 0x0137},
> + {IMX335_REG_THS_PREPARE, 0x4f},
> + {IMX335_REG_THS_ZERO, 0x87},
> + {IMX335_REG_THS_TRAIL, 0x4f},
> + {IMX335_REG_THS_EXIT, 0x7f},
> + {IMX335_REG_TPLX, 0x3f},
> };
>
> -static const struct imx335_reg mipi_data_rate_891Mbps[] = {
> - {0x300c, 0x3b},
> - {0x300d, 0x2a},
> - {0x314c, 0x29},
> - {0x314d, 0x01},
> - {0x315a, 0x06},
> - {0x3168, 0xa0},
> - {0x316a, 0x7e},
> - {0x319e, 0x02},
> - {0x3a18, 0x7f},
> - {0x3a1a, 0x37},
> - {0x3a1c, 0x37},
> - {0x3a1e, 0xf7},
> - {0x3a20, 0x3f},
> - {0x3a22, 0x6f},
> - {0x3a24, 0x3f},
> - {0x3a26, 0x5f},
> - {0x3a28, 0x2f},
> +static const struct cci_reg_sequence mipi_data_rate_891Mbps[] = {
> + {IMX335_REG_BCWAIT_TIME, 0x3b},
> + {IMX335_REG_CPWAIT_TIME, 0x2a},
> + {IMX335_REG_INCLKSEL1, 0x0129},
> + {IMX335_REG_INCLKSEL2, 0x06},
> + {IMX335_REG_INCLKSEL3, 0xa0},
> + {IMX335_REG_INCLKSEL4, 0x7e},
> + {IMX335_REG_SYSMODE, 0x02},
> + {IMX335_REG_TCLKPOST, 0x7f},
> + {IMX335_REG_TCLKPREPARE, 0x37},
> + {IMX335_REG_TCLK_TRAIL, 0x37},
> + {IMX335_REG_TCLK_ZERO, 0xf7},
> + {IMX335_REG_THS_PREPARE, 0x3f},
> + {IMX335_REG_THS_ZERO, 0x6f},
> + {IMX335_REG_THS_TRAIL, 0x3f},
> + {IMX335_REG_THS_EXIT, 0x5f},
> + {IMX335_REG_TPLX, 0x2f},
> };
>
> static const s64 link_freq[] = {
> @@ -397,101 +425,6 @@ static inline struct imx335 *to_imx335(struct v4l2_subdev *subdev)
> return container_of(subdev, struct imx335, sd);
> }
>
> -/**
> - * imx335_read_reg() - Read registers.
> - * @imx335: pointer to imx335 device
> - * @reg: register address
> - * @len: length of bytes to read. Max supported bytes is 4
> - * @val: pointer to register value to be filled.
> - *
> - * Big endian register addresses with little endian values.
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_read_reg(struct imx335 *imx335, u16 reg, u32 len, u32 *val)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
> - struct i2c_msg msgs[2] = {0};
> - u8 addr_buf[2] = {0};
> - u8 data_buf[4] = {0};
> - int ret;
> -
> - if (WARN_ON(len > 4))
> - return -EINVAL;
> -
> - put_unaligned_be16(reg, addr_buf);
> -
> - /* Write register address */
> - msgs[0].addr = client->addr;
> - msgs[0].flags = 0;
> - msgs[0].len = ARRAY_SIZE(addr_buf);
> - msgs[0].buf = addr_buf;
> -
> - /* Read data from register */
> - msgs[1].addr = client->addr;
> - msgs[1].flags = I2C_M_RD;
> - msgs[1].len = len;
> - msgs[1].buf = data_buf;
> -
> - ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> - if (ret != ARRAY_SIZE(msgs))
> - return -EIO;
> -
> - *val = get_unaligned_le32(data_buf);
> -
> - return 0;
> -}
> -
> -/**
> - * imx335_write_reg() - Write register
> - * @imx335: pointer to imx335 device
> - * @reg: register address
> - * @len: length of bytes. Max supported bytes is 4
> - * @val: register value
> - *
> - * Big endian register addresses with little endian values.
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_write_reg(struct imx335 *imx335, u16 reg, u32 len, u32 val)
> -{
> - struct i2c_client *client = v4l2_get_subdevdata(&imx335->sd);
> - u8 buf[6] = {0};
> -
> - if (WARN_ON(len > 4))
> - return -EINVAL;
> -
> - put_unaligned_be16(reg, buf);
> - put_unaligned_le32(val, buf + 2);
> - if (i2c_master_send(client, buf, len + 2) != len + 2)
> - return -EIO;
> -
> - return 0;
> -}
> -
> -/**
> - * imx335_write_regs() - Write a list of registers
> - * @imx335: pointer to imx335 device
> - * @regs: list of registers to be written
> - * @len: length of registers array
> - *
> - * Return: 0 if successful. error code otherwise.
> - */
> -static int imx335_write_regs(struct imx335 *imx335,
> - const struct imx335_reg *regs, u32 len)
> -{
> - unsigned int i;
> - int ret;
> -
> - for (i = 0; i < len; i++) {
> - ret = imx335_write_reg(imx335, regs[i].address, 1, regs[i].val);
> - if (ret)
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> /**
> * imx335_update_controls() - Update control ranges based on streaming mode
> * @imx335: pointer to imx335 device
> @@ -528,7 +461,7 @@ static int imx335_update_controls(struct imx335 *imx335,
> static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
> {
> u32 lpfr, shutter;
> - int ret;
> + int ret = 0;
>
> lpfr = imx335->vblank + imx335->cur_mode->height;
> shutter = lpfr - exposure;
> @@ -536,64 +469,49 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
> dev_dbg(imx335->dev, "Set exp %u, analog gain %u, shutter %u, lpfr %u\n",
> exposure, gain, shutter, lpfr);
>
> - ret = imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 1);
> - if (ret)
> - return ret;
> -
> - ret = imx335_write_reg(imx335, IMX335_REG_LPFR, 3, lpfr);
> - if (ret)
> - goto error_release_group_hold;
> -
> - ret = imx335_write_reg(imx335, IMX335_REG_SHUTTER, 3, shutter);
> - if (ret)
> - goto error_release_group_hold;
> -
> - ret = imx335_write_reg(imx335, IMX335_REG_AGAIN, 2, gain);
> -
> -error_release_group_hold:
> - imx335_write_reg(imx335, IMX335_REG_HOLD, 1, 0);
> + cci_write(imx335->cci, IMX335_REG_HOLD, 1, &ret);
> + cci_write(imx335->cci, IMX335_REG_VMAX, lpfr, &ret);
> + cci_write(imx335->cci, IMX335_REG_SHUTTER, shutter, &ret);
> + cci_write(imx335->cci, IMX335_REG_AGAIN, gain, &ret);
> + cci_write(imx335->cci, IMX335_REG_HOLD, 0, &ret);
Aha, now this bit is more tricky. Even in case of error we need to
unconditionally (attempt) to release the hold. We'll need two ret
values...
and then:
- cci_write(imx335->cci, IMX335_REG_HOLD, 0, &ret);
+ /*
+ * Unconditionally attempt to release the hold, but track the
+ * error if the unhold itself fails.
+ */
+ ret_hold = cci_write(imx335->cci, IMX335_REG_HOLD, 0, NULL);
+ if (ret_hold)
+ ret = ret_hold;
>
> return ret;
> }
>
> static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
> {
> - int ret;
> + int ret = 0;
>
> if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
> return -EINVAL;
>
> if (pattern_index) {
> - const struct imx335_reg tpg_enable_regs[] = {
> - { 0x3148, 0x10 },
> - { 0x3280, 0x00 },
> - { 0x329c, 0x01 },
> - { 0x32a0, 0x11 },
> - { 0x3302, 0x00 },
> - { 0x3303, 0x00 },
> - { 0x336c, 0x00 },
> + const struct cci_reg_sequence tpg_enable_regs[] = {
> + {IMX335_REG_TPG_TESTCLKEN, 0x10},
> + {IMX335_REG_TPG_DIG_CLP_MODE, 0x00},
> + {IMX335_REG_TPG_EN_DUOUT, 0x01},
> + {IMX335_REG_TPG_COLORWIDTH, 0x11},
> + {IMX335_REG_TPG_BLKLEVEL, 0x00},
> + {IMX335_REG_TPG_WRJ_OPEN, 0x00},
> };
>
> - ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1,
> - imx335_tpg_val[pattern_index]);
> - if (ret)
> - return ret;
> + cci_write(imx335->cci, IMX335_REG_TPG,
> + imx335_tpg_val[pattern_index], &ret);
>
> - ret = imx335_write_regs(imx335, tpg_enable_regs,
> - ARRAY_SIZE(tpg_enable_regs));
> + cci_multi_reg_write(imx335->cci, tpg_enable_regs,
> + ARRAY_SIZE(tpg_enable_regs), &ret);
> } else {
> - const struct imx335_reg tpg_disable_regs[] = {
> - { 0x3148, 0x00 },
> - { 0x3280, 0x01 },
> - { 0x329c, 0x00 },
> - { 0x32a0, 0x10 },
> - { 0x3302, 0x32 },
> - { 0x3303, 0x00 },
> - { 0x336c, 0x01 },
> + const struct cci_reg_sequence tpg_disable_regs[] = {
> + {IMX335_REG_TPG_TESTCLKEN, 0x00},
> + {IMX335_REG_TPG_DIG_CLP_MODE, 0x01},
> + {IMX335_REG_TPG_EN_DUOUT, 0x00},
> + {IMX335_REG_TPG_COLORWIDTH, 0x10},
> + {IMX335_REG_TPG_BLKLEVEL, 0x32},
> + {IMX335_REG_TPG_WRJ_OPEN, 0x01},
> };
>
> - ret = imx335_write_regs(imx335, tpg_disable_regs,
> - ARRAY_SIZE(tpg_disable_regs));
> + cci_multi_reg_write(imx335->cci, tpg_disable_regs,
> + ARRAY_SIZE(tpg_disable_regs), &ret);
> }
>
> return ret;
> @@ -892,12 +810,14 @@ static int imx335_set_framefmt(struct imx335 *imx335)
> {
> switch (imx335->cur_mbus_code) {
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> - return imx335_write_regs(imx335, raw10_framefmt_regs,
> - ARRAY_SIZE(raw10_framefmt_regs));
> + return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
> + ARRAY_SIZE(raw10_framefmt_regs),
> + NULL);
>
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> - return imx335_write_regs(imx335, raw12_framefmt_regs,
> - ARRAY_SIZE(raw12_framefmt_regs));
> + return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
> + ARRAY_SIZE(raw12_framefmt_regs),
> + NULL);
> }
>
> return -EINVAL;
> @@ -916,7 +836,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>
> /* Setup PLL */
> reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
> - ret = imx335_write_regs(imx335, reg_list->regs, reg_list->num_of_regs);
> + ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
> + reg_list->num_of_regs, NULL);
> if (ret) {
> dev_err(imx335->dev, "%s failed to set plls\n", __func__);
> return ret;
> @@ -924,8 +845,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
>
> /* Write sensor mode registers */
> reg_list = &imx335->cur_mode->reg_list;
> - ret = imx335_write_regs(imx335, reg_list->regs,
> - reg_list->num_of_regs);
> + ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
> + reg_list->num_of_regs, NULL);
> if (ret) {
> dev_err(imx335->dev, "fail to write initial registers\n");
> return ret;
> @@ -939,7 +860,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
> }
>
> /* Configure lanes */
> - ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
> + ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
> + imx335->lane_mode, NULL);
> if (ret)
> return ret;
>
> @@ -951,8 +873,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
> }
>
> /* Start streaming */
> - ret = imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
> - 1, IMX335_MODE_STREAMING);
> + ret = cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> + IMX335_MODE_STREAMING, NULL);
> if (ret) {
> dev_err(imx335->dev, "fail to start streaming\n");
> return ret;
> @@ -972,8 +894,8 @@ static int imx335_start_streaming(struct imx335 *imx335)
> */
> static int imx335_stop_streaming(struct imx335 *imx335)
> {
> - return imx335_write_reg(imx335, IMX335_REG_MODE_SELECT,
> - 1, IMX335_MODE_STANDBY);
> + return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> + IMX335_MODE_STANDBY, NULL);
> }
>
> /**
> @@ -1024,14 +946,14 @@ static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
> static int imx335_detect(struct imx335 *imx335)
> {
> int ret;
> - u32 val;
> + u64 val;
>
> - ret = imx335_read_reg(imx335, IMX335_REG_ID, 2, &val);
> + ret = cci_read(imx335->cci, IMX335_REG_ID, &val, NULL);
> if (ret)
> return ret;
>
> if (val != IMX335_ID) {
> - dev_err(imx335->dev, "chip id mismatch: %x!=%x\n",
> + dev_err(imx335->dev, "chip id mismatch: %x!=%llx\n",
> IMX335_ID, val);
> return -ENXIO;
> }
> @@ -1345,6 +1267,11 @@ static int imx335_probe(struct i2c_client *client)
> return -ENOMEM;
>
> imx335->dev = &client->dev;
> + imx335->cci = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(imx335->cci)) {
> + dev_err(imx335->dev, "Unable to initialize I2C\n");
> + return -ENODEV;
> + }
>
> /* Initialize subdev */
> v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops);
> --
> 2.43.0
>
Quoting Umang Jain (2024-03-06 08:10:37)
> The imx335 reports a recommended pixel area of - 2592x1944.
> The driver supported mode however limits it to height=1940.
Hrm, I think I would convert widths and sizes to decimal as a patch
before this patch so the effect is clearer in this diff.
> Fix the height discrepency by correctly the value of height
> (with updates to vblank and mode registers).
>
> Signed-off-by: Umang Jain <[email protected]>
> ---
> drivers/media/i2c/imx335.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 6ea09933e47b..c00e0c2be3f3 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -246,13 +246,13 @@ static const int imx335_tpg_val[] = {
> };
>
> /* Sensor mode registers */
> -static const struct cci_reg_sequence mode_2592x1940_regs[] = {
> +static const struct cci_reg_sequence mode_2592x1944_regs[] = {
> {IMX335_REG_MODE_SELECT, 0x01},
> {IMX335_REG_MASTER_MODE, 0x00},
> - {IMX335_REG_WINMODE, 0x04},
> - {IMX335_REG_HTRIMMING_START, 0x0180},
> + {IMX335_REG_WINMODE, 0x00},
What's the distinction of the winmode here. What is 0x04 vs 0x00?
Is this something that could be a defined value? Or is that not worth
the effort?
> + {IMX335_REG_HTRIMMING_START, 0x30},
HTRIMMING_START has moved a lot more than I would expect there.
Is there a visual impact of any concern here?
> {IMX335_REG_HNUM, 0x0a20},
> - {IMX335_REG_Y_OUT_SIZE, 0x0794},
> + {IMX335_REG_Y_OUT_SIZE, 0x0798},
This bit looks expected ;-)
> {IMX335_REG_VCROP_POS, 0x00b0},
> {IMX335_REG_VCROP_SIZE, 0x0f58},
0x0f58 = 3928. Does that correspond to anything on the pixel array size?
We're modifying the vertical size, so I'm curious if the 'vcrop' is or
should be impacted?
> {IMX335_REG_OPB_SIZE_V, 0x00},
> @@ -403,14 +403,14 @@ static const u32 imx335_mbus_codes[] = {
> /* Supported sensor mode configurations */
> static const struct imx335_mode supported_mode = {
> .width = 2592,
> - .height = 1940,
> + .height = 1944,
> .hblank = 342,
> - .vblank = 2560,
> - .vblank_min = 2560,
> + .vblank = 2556,
> + .vblank_min = 2556,
> .vblank_max = 133060,
> .reg_list = {
> - .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
> - .regs = mode_2592x1940_regs,
> + .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> + .regs = mode_2592x1944_regs,
> },
> };
>
> --
> 2.43.0
>
Quoting Umang Jain (2024-03-06 08:10:38)
> The sensor gain (both analog and digital) are controlled by a
> single gain value where:
> - 0dB to 30dB correspond to analog gain
> - 30.3dB to 72dB correspond to digital gain
> (with 0.3dB step)
>
> Hence, limit the analogue gain value to 100.
> For digital gain, support can be added later if needed.
>
> Signed-off-by: Umang Jain <[email protected]>
I think this makes the behaviour more consistent with expectations from
userspace for the ANALOGUE_GAIN.
Reviewed-by: Kieran Bingham <[email protected]>
> ---
> drivers/media/i2c/imx335.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index c00e0c2be3f3..701bd5318b45 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -45,7 +45,7 @@
> /* Analog gain control */
> #define IMX335_REG_AGAIN CCI_REG8(0x30e8)
> #define IMX335_AGAIN_MIN 0
> -#define IMX335_AGAIN_MAX 240
> +#define IMX335_AGAIN_MAX 100
> #define IMX335_AGAIN_STEP 1
> #define IMX335_AGAIN_DEFAULT 0
>
> @@ -1169,6 +1169,14 @@ static int imx335_init_controls(struct imx335 *imx335)
> IMX335_EXPOSURE_STEP,
> IMX335_EXPOSURE_DEFAULT);
>
> + /*
> + * The sensor has an analog gain and a digital gain, both controlled
> + * through a single gain value, expressed in 0.3dB increments. Values
> + * from 0.0dB (0) to 30.0dB (100) apply analog gain only, higher values
> + * up to 72.0dB (240) add further digital gain. Limit the range to
> + * analog gain only, support for digital gain can be added separately
> + * if needed.
> + */
> imx335->again_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> &imx335_ctrl_ops,
> V4L2_CID_ANALOGUE_GAIN,
> --
> 2.43.0
>
Hi Kieran,
On 06/03/24 10:35 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-06 08:10:37)
>> The imx335 reports a recommended pixel area of - 2592x1944.
>> The driver supported mode however limits it to height=1940.
> Hrm, I think I would convert widths and sizes to decimal as a patch
> before this patch so the effect is clearer in this diff.
>
>> Fix the height discrepency by correctly the value of height
>> (with updates to vblank and mode registers).
>>
>> Signed-off-by: Umang Jain <[email protected]>
>> ---
>> drivers/media/i2c/imx335.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>> index 6ea09933e47b..c00e0c2be3f3 100644
>> --- a/drivers/media/i2c/imx335.c
>> +++ b/drivers/media/i2c/imx335.c
>> @@ -246,13 +246,13 @@ static const int imx335_tpg_val[] = {
>> };
>>
>> /* Sensor mode registers */
>> -static const struct cci_reg_sequence mode_2592x1940_regs[] = {
>> +static const struct cci_reg_sequence mode_2592x1944_regs[] = {
>> {IMX335_REG_MODE_SELECT, 0x01},
>> {IMX335_REG_MASTER_MODE, 0x00},
>> - {IMX335_REG_WINMODE, 0x04},
>> - {IMX335_REG_HTRIMMING_START, 0x0180},
>> + {IMX335_REG_WINMODE, 0x00},
> What's the distinction of the winmode here. What is 0x04 vs 0x00?
>
> Is this something that could be a defined value? Or is that not worth
> the effort?
It can be split out as a define in later parts if we introduce more
modes. currently this change fixes the sensor to be in 'all-pixel scan
mode' hence you see the change in value of WINMODE.
>
>> + {IMX335_REG_HTRIMMING_START, 0x30},
> HTRIMMING_START has moved a lot more than I would expect there.
> Is there a visual impact of any concern here?
Value as per mentioned in the datasheet
>
>> {IMX335_REG_HNUM, 0x0a20},
>> - {IMX335_REG_Y_OUT_SIZE, 0x0794},
>> + {IMX335_REG_Y_OUT_SIZE, 0x0798},
> This bit looks expected ;-)
>
>> {IMX335_REG_VCROP_POS, 0x00b0},
>> {IMX335_REG_VCROP_SIZE, 0x0f58},
> 0x0f58 = 3928. Does that correspond to anything on the pixel array size?
> We're modifying the vertical size, so I'm curious if the 'vcrop' is or
> should be impacted?
Probably I have not named this correctly, as VCROP_
This register (0x3076) is denotes the size of cropping rectangle
Named as 'AREA3_WIDTH_1' with explaination 'cropping size designation
(Vertical direction)'
The value 0x0f58 is as per mentioned in the datasheet. I don't find any
relation except 3928 is twice the IMX335_NATIVE_HEIGHT
>
>> {IMX335_REG_OPB_SIZE_V, 0x00},
>> @@ -403,14 +403,14 @@ static const u32 imx335_mbus_codes[] = {
>> /* Supported sensor mode configurations */
>> static const struct imx335_mode supported_mode = {
>> .width = 2592,
>> - .height = 1940,
>> + .height = 1944,
>> .hblank = 342,
>> - .vblank = 2560,
>> - .vblank_min = 2560,
>> + .vblank = 2556,
>> + .vblank_min = 2556,
>> .vblank_max = 133060,
>> .reg_list = {
>> - .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
>> - .regs = mode_2592x1940_regs,
>> + .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
>> + .regs = mode_2592x1944_regs,
>> },
>> };
>>
>> --
>> 2.43.0
>>
Hi Umang,
On Wed, Mar 06, 2024 at 01:40:34PM +0530, Umang Jain wrote:
> From: Kieran Bingham <[email protected]>
>
> The IMX335 can support both 2 and 4 lane configurations.
> Extend the driver to configure the lane mode accordingly.
> Update the pixel rate depending on the number of lanes in use.
>
> Signed-off-by: Kieran Bingham <[email protected]>
> Signed-off-by: Umang Jain <[email protected]>
> ---
> drivers/media/i2c/imx335.c | 46 +++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index dab6d080bc4c..a42f48823515 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -21,6 +21,11 @@
> #define IMX335_MODE_STANDBY 0x01
> #define IMX335_MODE_STREAMING 0x00
>
> +/* Data Lanes */
> +#define IMX335_LANEMODE 0x3a01
> +#define IMX335_2LANE 1
> +#define IMX335_4LANE 3
> +
> /* Lines per frame */
> #define IMX335_REG_LPFR 0x3030
>
> @@ -67,8 +72,6 @@
> #define IMX335_LINK_FREQ_594MHz 594000000LL
> #define IMX335_LINK_FREQ_445MHz 445500000LL
>
> -#define IMX335_NUM_DATA_LANES 4
> -
> #define IMX335_REG_MIN 0x00
> #define IMX335_REG_MAX 0xfffff
>
> @@ -115,7 +118,6 @@ static const char * const imx335_supply_name[] = {
> * @vblank: Vertical blanking in lines
> * @vblank_min: Minimum vertical blanking in lines
> * @vblank_max: Maximum vertical blanking in lines
> - * @pclk: Sensor pixel clock
> * @reg_list: Register list for sensor mode
> */
> struct imx335_mode {
> @@ -126,7 +128,6 @@ struct imx335_mode {
> u32 vblank;
> u32 vblank_min;
> u32 vblank_max;
> - u64 pclk;
> struct imx335_reg_list reg_list;
> };
>
> @@ -147,6 +148,7 @@ struct imx335_mode {
> * @exp_ctrl: Pointer to exposure control
> * @again_ctrl: Pointer to analog gain control
> * @vblank: Vertical blanking in lines
> + * @lane_mode Mode for number of connected data lanes
> * @cur_mode: Pointer to current selected sensor mode
> * @mutex: Mutex for serializing sensor controls
> * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> @@ -171,6 +173,7 @@ struct imx335 {
> struct v4l2_ctrl *again_ctrl;
> };
> u32 vblank;
> + u32 lane_mode;
> const struct imx335_mode *cur_mode;
> struct mutex mutex;
> unsigned long link_freq_bitmap;
> @@ -377,7 +380,6 @@ static const struct imx335_mode supported_mode = {
> .vblank = 2560,
> .vblank_min = 2560,
> .vblank_max = 133060,
> - .pclk = 396000000,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
> .regs = mode_2592x1940_regs,
> @@ -936,6 +938,11 @@ static int imx335_start_streaming(struct imx335 *imx335)
> return ret;
> }
>
> + /* Configure lanes */
> + ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
> + if (ret)
> + return ret;
> +
> /* Setup handler will write actual exposure and gain */
> ret = __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
> if (ret) {
> @@ -1096,7 +1103,14 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
> if (ret)
> return ret;
>
> - if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX335_NUM_DATA_LANES) {
> + switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
> + case 2:
> + imx335->lane_mode = IMX335_2LANE;
> + break;
> + case 4:
> + imx335->lane_mode = IMX335_4LANE;
> + break;
> + default:
> dev_err(imx335->dev,
> "number of CSI2 data lanes %d is not supported\n",
> bus_cfg.bus.mipi_csi2.num_data_lanes);
> @@ -1209,6 +1223,9 @@ static int imx335_init_controls(struct imx335 *imx335)
> struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
> const struct imx335_mode *mode = imx335->cur_mode;
> u32 lpfr;
> + u64 pclk;
> + s64 link_freq_in_use;
> + u8 bpp;
> int ret;
>
> ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
> @@ -1252,11 +1269,24 @@ static int imx335_init_controls(struct imx335 *imx335)
> 0, 0, imx335_tpg_menu);
>
> /* Read only controls */
> +
> + /* pixel rate = link frequency * lanes * 2 / bits_per_pixel */
> + switch (imx335->cur_mbus_code) {
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + bpp = 10;
> + break;
> + case MEDIA_BUS_FMT_SRGGB12_1X12:
> + bpp = 12;
> + break;
> + }
> +
> + link_freq_in_use = link_freq[__ffs(imx335->link_freq_bitmap)];
> + pclk = link_freq_in_use * (imx335->lane_mode + 1) * 2 / bpp;
Please use div_s64() for this.
> imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> &imx335_ctrl_ops,
> V4L2_CID_PIXEL_RATE,
> - mode->pclk, mode->pclk,
> - 1, mode->pclk);
> + pclk, pclk,
> + 1, pclk);
>
> imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> &imx335_ctrl_ops,
--
Regards,
Sakari Ailus
Hi Dave
On 06/03/24 10:12 pm, Dave Stevenson wrote:
> Hi Umang and Kieran
>
> On Wed, 6 Mar 2024 at 08:11, Umang Jain <[email protected]> wrote:
>> From: Kieran Bingham <[email protected]>
>>
>> The IMX335 can support both 2 and 4 lane configurations.
>> Extend the driver to configure the lane mode accordingly.
>> Update the pixel rate depending on the number of lanes in use.
>>
>> Signed-off-by: Kieran Bingham <[email protected]>
>> Signed-off-by: Umang Jain <[email protected]>
>> ---
>> drivers/media/i2c/imx335.c | 46 +++++++++++++++++++++++++++++++-------
>> 1 file changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>> index dab6d080bc4c..a42f48823515 100644
>> --- a/drivers/media/i2c/imx335.c
>> +++ b/drivers/media/i2c/imx335.c
>> @@ -21,6 +21,11 @@
>> #define IMX335_MODE_STANDBY 0x01
>> #define IMX335_MODE_STREAMING 0x00
>>
>> +/* Data Lanes */
>> +#define IMX335_LANEMODE 0x3a01
>> +#define IMX335_2LANE 1
>> +#define IMX335_4LANE 3
>> +
>> /* Lines per frame */
>> #define IMX335_REG_LPFR 0x3030
>>
>> @@ -67,8 +72,6 @@
>> #define IMX335_LINK_FREQ_594MHz 594000000LL
>> #define IMX335_LINK_FREQ_445MHz 445500000LL
>>
>> -#define IMX335_NUM_DATA_LANES 4
>> -
>> #define IMX335_REG_MIN 0x00
>> #define IMX335_REG_MAX 0xfffff
>>
>> @@ -115,7 +118,6 @@ static const char * const imx335_supply_name[] = {
>> * @vblank: Vertical blanking in lines
>> * @vblank_min: Minimum vertical blanking in lines
>> * @vblank_max: Maximum vertical blanking in lines
>> - * @pclk: Sensor pixel clock
>> * @reg_list: Register list for sensor mode
>> */
>> struct imx335_mode {
>> @@ -126,7 +128,6 @@ struct imx335_mode {
>> u32 vblank;
>> u32 vblank_min;
>> u32 vblank_max;
>> - u64 pclk;
>> struct imx335_reg_list reg_list;
>> };
>>
>> @@ -147,6 +148,7 @@ struct imx335_mode {
>> * @exp_ctrl: Pointer to exposure control
>> * @again_ctrl: Pointer to analog gain control
>> * @vblank: Vertical blanking in lines
>> + * @lane_mode Mode for number of connected data lanes
>> * @cur_mode: Pointer to current selected sensor mode
>> * @mutex: Mutex for serializing sensor controls
>> * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
>> @@ -171,6 +173,7 @@ struct imx335 {
>> struct v4l2_ctrl *again_ctrl;
>> };
>> u32 vblank;
>> + u32 lane_mode;
>> const struct imx335_mode *cur_mode;
>> struct mutex mutex;
>> unsigned long link_freq_bitmap;
>> @@ -377,7 +380,6 @@ static const struct imx335_mode supported_mode = {
>> .vblank = 2560,
>> .vblank_min = 2560,
>> .vblank_max = 133060,
>> - .pclk = 396000000,
>> .reg_list = {
>> .num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
>> .regs = mode_2592x1940_regs,
>> @@ -936,6 +938,11 @@ static int imx335_start_streaming(struct imx335 *imx335)
>> return ret;
>> }
>>
>> + /* Configure lanes */
>> + ret = imx335_write_reg(imx335, IMX335_LANEMODE, 1, imx335->lane_mode);
>> + if (ret)
>> + return ret;
>> +
>> /* Setup handler will write actual exposure and gain */
>> ret = __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
>> if (ret) {
>> @@ -1096,7 +1103,14 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>> if (ret)
>> return ret;
>>
>> - if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX335_NUM_DATA_LANES) {
>> + switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
>> + case 2:
>> + imx335->lane_mode = IMX335_2LANE;
>> + break;
>> + case 4:
>> + imx335->lane_mode = IMX335_4LANE;
>> + break;
>> + default:
>> dev_err(imx335->dev,
>> "number of CSI2 data lanes %d is not supported\n",
>> bus_cfg.bus.mipi_csi2.num_data_lanes);
>> @@ -1209,6 +1223,9 @@ static int imx335_init_controls(struct imx335 *imx335)
>> struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
>> const struct imx335_mode *mode = imx335->cur_mode;
>> u32 lpfr;
>> + u64 pclk;
>> + s64 link_freq_in_use;
>> + u8 bpp;
>> int ret;
>>
>> ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
>> @@ -1252,11 +1269,24 @@ static int imx335_init_controls(struct imx335 *imx335)
>> 0, 0, imx335_tpg_menu);
>>
>> /* Read only controls */
>> +
>> + /* pixel rate = link frequency * lanes * 2 / bits_per_pixel */
>> + switch (imx335->cur_mbus_code) {
>> + case MEDIA_BUS_FMT_SRGGB10_1X10:
>> + bpp = 10;
>> + break;
>> + case MEDIA_BUS_FMT_SRGGB12_1X12:
>> + bpp = 12;
>> + break;
>> + }
>> +
>> + link_freq_in_use = link_freq[__ffs(imx335->link_freq_bitmap)];
>> + pclk = link_freq_in_use * (imx335->lane_mode + 1) * 2 / bpp;
>> imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
>> &imx335_ctrl_ops,
>> V4L2_CID_PIXEL_RATE,
>> - mode->pclk, mode->pclk,
>> - 1, mode->pclk);
>> + pclk, pclk,
>> + 1, pclk);
> Is this actually correct?
> A fair number of the sensors I've encountered have 2 PLL paths - one
> for the pixel array, and one for the CSI block. The bpp will generally
> be fed into the CSI block PLL path, but not into the pixel array one.
> The link frequency will therefore vary with bit depth, but
> V4L2_CID_PIXEL_RATE doesn't change.
>
> imx290 certainly has a disjoin between pixel rate and link freq
> (cropping reduces link freq, but not pixel rate), and we run imx477 in
> 2 lane mode with the pixel array at full tilt (840MPix/s) but large
> horizontal blanking to allow CSI2 enough time to send the data.
>
> If you've validated that for a range of frame rates you get the
> correct output from the sensor in both 10 and 12 bit modes, then I
I've not validated in such cases. Computing from pixel rate from the
link_freq and lane mode came out to be the same as the value currently
hardcoded in the mode structure hence I introduced this change. However,
I am inclined to dropping it after reading your review ;-)
> don't object. I just have an instinctive tick whenever I see drivers
> computing PIXEL_RATE from LINK_FREQ or vice versa :)
Having said that, I do know, the reporting is not perfect since the bpp
can be changed and it would change the link-frequency.
> If you get the right frame rate it may also imply that the link
> frequency isn't as configured, but that rarely has any negative
Indeed ;-)
> effects. You need a reasonably good oscilloscope to be able to measure
> the link frequency.
AH, I see.
>
> Dave
>
>> imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>> &imx335_ctrl_ops,
>> --
>> 2.43.0
>>
>>
Hi Umang,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linuxtv-media-stage/master]
[cannot apply to media-tree/master sailus-media-tree/streams linus/master v6.8-rc7 next-20240308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Umang-Jain/media-imx335-Support-2-or-4-lane-operation-modes/20240306-161903
base: https://git.linuxtv.org/media_stage.git master
patch link: https://lore.kernel.org/r/20240306081038.212412-2-umang.jain%40ideasonboard.com
patch subject: [PATCH 1/5] media: imx335: Support 2 or 4 lane operation modes
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20240309/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240309/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/media/i2c/imx335.c:181: warning: Function parameter or struct member 'lane_mode' not described in 'imx335'
vim +181 drivers/media/i2c/imx335.c
45d19b5fb9aeab Martina Krasteva 2021-05-27 133
45d19b5fb9aeab Martina Krasteva 2021-05-27 134 /**
45d19b5fb9aeab Martina Krasteva 2021-05-27 135 * struct imx335 - imx335 sensor device structure
45d19b5fb9aeab Martina Krasteva 2021-05-27 136 * @dev: Pointer to generic device
45d19b5fb9aeab Martina Krasteva 2021-05-27 137 * @client: Pointer to i2c client
45d19b5fb9aeab Martina Krasteva 2021-05-27 138 * @sd: V4L2 sub-device
45d19b5fb9aeab Martina Krasteva 2021-05-27 139 * @pad: Media pad. Only one pad supported
45d19b5fb9aeab Martina Krasteva 2021-05-27 140 * @reset_gpio: Sensor reset gpio
fea91ee73b7cd1 Kieran Bingham 2023-12-11 141 * @supplies: Regulator supplies to handle power control
45d19b5fb9aeab Martina Krasteva 2021-05-27 142 * @inclk: Sensor input clock
45d19b5fb9aeab Martina Krasteva 2021-05-27 143 * @ctrl_handler: V4L2 control handler
45d19b5fb9aeab Martina Krasteva 2021-05-27 144 * @link_freq_ctrl: Pointer to link frequency control
45d19b5fb9aeab Martina Krasteva 2021-05-27 145 * @pclk_ctrl: Pointer to pixel clock control
45d19b5fb9aeab Martina Krasteva 2021-05-27 146 * @hblank_ctrl: Pointer to horizontal blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27 147 * @vblank_ctrl: Pointer to vertical blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27 148 * @exp_ctrl: Pointer to exposure control
45d19b5fb9aeab Martina Krasteva 2021-05-27 149 * @again_ctrl: Pointer to analog gain control
45d19b5fb9aeab Martina Krasteva 2021-05-27 150 * @vblank: Vertical blanking in lines
8c48b2175e7d73 Kieran Bingham 2024-03-06 151 * @lane_mode Mode for number of connected data lanes
45d19b5fb9aeab Martina Krasteva 2021-05-27 152 * @cur_mode: Pointer to current selected sensor mode
45d19b5fb9aeab Martina Krasteva 2021-05-27 153 * @mutex: Mutex for serializing sensor controls
0862582b5239a6 Umang Jain 2024-02-20 154 * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
cfa49ff0558a32 Umang Jain 2023-12-11 155 * @cur_mbus_code: Currently selected media bus format code
45d19b5fb9aeab Martina Krasteva 2021-05-27 156 */
45d19b5fb9aeab Martina Krasteva 2021-05-27 157 struct imx335 {
45d19b5fb9aeab Martina Krasteva 2021-05-27 158 struct device *dev;
45d19b5fb9aeab Martina Krasteva 2021-05-27 159 struct i2c_client *client;
45d19b5fb9aeab Martina Krasteva 2021-05-27 160 struct v4l2_subdev sd;
45d19b5fb9aeab Martina Krasteva 2021-05-27 161 struct media_pad pad;
45d19b5fb9aeab Martina Krasteva 2021-05-27 162 struct gpio_desc *reset_gpio;
fea91ee73b7cd1 Kieran Bingham 2023-12-11 163 struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
fea91ee73b7cd1 Kieran Bingham 2023-12-11 164
45d19b5fb9aeab Martina Krasteva 2021-05-27 165 struct clk *inclk;
45d19b5fb9aeab Martina Krasteva 2021-05-27 166 struct v4l2_ctrl_handler ctrl_handler;
45d19b5fb9aeab Martina Krasteva 2021-05-27 167 struct v4l2_ctrl *link_freq_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 168 struct v4l2_ctrl *pclk_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 169 struct v4l2_ctrl *hblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 170 struct v4l2_ctrl *vblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 171 struct {
45d19b5fb9aeab Martina Krasteva 2021-05-27 172 struct v4l2_ctrl *exp_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 173 struct v4l2_ctrl *again_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 174 };
45d19b5fb9aeab Martina Krasteva 2021-05-27 175 u32 vblank;
8c48b2175e7d73 Kieran Bingham 2024-03-06 176 u32 lane_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27 177 const struct imx335_mode *cur_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27 178 struct mutex mutex;
0862582b5239a6 Umang Jain 2024-02-20 179 unsigned long link_freq_bitmap;
cfa49ff0558a32 Umang Jain 2023-12-11 180 u32 cur_mbus_code;
45d19b5fb9aeab Martina Krasteva 2021-05-27 @181 };
45d19b5fb9aeab Martina Krasteva 2021-05-27 182
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Umang,
kernel test robot noticed the following build errors:
[auto build test ERROR on linuxtv-media-stage/master]
[cannot apply to media-tree/master sailus-media-tree/streams linus/master v6.8-rc7 next-20240308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Umang-Jain/media-imx335-Support-2-or-4-lane-operation-modes/20240306-161903
base: https://git.linuxtv.org/media_stage.git master
patch link: https://lore.kernel.org/r/20240306081038.212412-2-umang.jain%40ideasonboard.com
patch subject: [PATCH 1/5] media: imx335: Support 2 or 4 lane operation modes
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20240309/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240309/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
m68k-linux-ld: drivers/media/i2c/imx335.o: in function `imx335_init_controls.constprop.0':
>> imx335.c:(.text+0x33c): undefined reference to `__divdi3'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Umang,
kernel test robot noticed the following build errors:
[auto build test ERROR on linuxtv-media-stage/master]
[cannot apply to media-tree/master sailus-media-tree/streams linus/master v6.8-rc7 next-20240308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Umang-Jain/media-imx335-Support-2-or-4-lane-operation-modes/20240306-161903
base: https://git.linuxtv.org/media_stage.git master
patch link: https://lore.kernel.org/r/20240306081038.212412-2-umang.jain%40ideasonboard.com
patch subject: [PATCH 1/5] media: imx335: Support 2 or 4 lane operation modes
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240309/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240309/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/parsers/tplink_safeloader.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_util.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_cmdset_0020.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/maps/map_funcs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/hisi-spmi-controller.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/spmi-pmic-arb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_pruss.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/pcmcia_rsrc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/pcmcia/i82365.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/mr75203.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/vhost/vringh.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/greybus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/greybus/gb-es2.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/rpmsg/rpmsg_char.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/ingenic-adc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/xilinx-ams.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/buffer/kfifo_buf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-hub.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-ast-cf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/siox/siox-bus-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/counter/ftm-quaddec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/oss/dmasound/dmasound_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/oss/dmasound/dmasound_atari.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/oss/dmasound/dmasound_paula.o
WARNING: modpost: sound/oss/dmasound/dmasound_paula: section mismatch in reference: amiga_audio_driver+0x8 (section: .data) -> amiga_audio_remove (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/core/snd-pcm-dmaengine.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/drivers/snd-pcmtest.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/pci/hda/snd-hda-cirrus-scodec-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/soc-topology-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-ab8500-codec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-sigmadsp.o
WARNING: modpost: sound/soc/codecs/snd-soc-tlv320adc3xxx: section mismatch in reference: adc3xxx_i2c_driver+0x8 (section: .data) -> adc3xxx_i2c_remove (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/codecs/snd-soc-wm-adsp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/fsl/imx-pcm-dma.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/mxs/snd-soc-mxs-pcm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/qcom/snd-soc-qcom-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/qcom/snd-soc-qcom-sdw.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/qcom/qdsp6/snd-q6dsp-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/intel/snd-sof-intel-atom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/intel/snd-sof-acpi-intel-byt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/intel/snd-sof-acpi-intel-bdw.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/imx/snd-sof-imx8.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/imx/snd-sof-imx8m.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/imx/snd-sof-imx8ulp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/imx/imx-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/mediatek/mtk-adsp-common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/mediatek/mt8195/snd-sof-mt8195.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/mediatek/mt8186/snd-sof-mt8186.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/snd-sof-utils.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/snd-sof-acpi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/sof/snd-sof-of.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/xilinx/snd-soc-xlnx-i2s.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/soc/xilinx/snd-soc-xlnx-formatter-pcm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in sound/ac97_bus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mtty.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mdpy-fb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/vfio-mdev/mbochs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/configfs/configfs_sample.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/bytestream-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/dma-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/inttype-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kfifo/record-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kobject/kobject-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in samples/kobject/kset-example.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_cmp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_nbyte.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_u32.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_meta.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_text.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_canid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ip_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ipip.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ip_gre.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/udp_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ip_vti.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/ah4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/esp4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/xfrm4_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv4/tunnel4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/xfrm/xfrm_algo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/xfrm/xfrm_user.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/ah6.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/esp6.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/xfrm6_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/tunnel6.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/mip6.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/sit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ipv6/ip6_udp_tunnel.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/key/af_key.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/atm/mpoa.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/6lowpan/6lowpan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/6lowpan/ieee802154_6lowpan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/ieee802154/ieee802154_socket.o
>> ERROR: modpost: "__divdi3" [drivers/media/i2c/imx335.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Umang,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linuxtv-media-stage/master]
[cannot apply to media-tree/master sailus-media-tree/streams linus/master v6.8-rc7 next-20240308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Umang-Jain/media-imx335-Support-2-or-4-lane-operation-modes/20240306-161903
base: https://git.linuxtv.org/media_stage.git master
patch link: https://lore.kernel.org/r/20240306081038.212412-4-umang.jain%40ideasonboard.com
patch subject: [PATCH 3/5] media: imx335: Use V4L2 CCI for accessing sensor registers
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20240309/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240309/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/media/i2c/imx335.c:214: warning: Function parameter or struct member 'cci' not described in 'imx335'
drivers/media/i2c/imx335.c:214: warning: Function parameter or struct member 'lane_mode' not described in 'imx335'
vim +214 drivers/media/i2c/imx335.c
45d19b5fb9aeab Martina Krasteva 2021-05-27 165
45d19b5fb9aeab Martina Krasteva 2021-05-27 166 /**
45d19b5fb9aeab Martina Krasteva 2021-05-27 167 * struct imx335 - imx335 sensor device structure
45d19b5fb9aeab Martina Krasteva 2021-05-27 168 * @dev: Pointer to generic device
45d19b5fb9aeab Martina Krasteva 2021-05-27 169 * @client: Pointer to i2c client
45d19b5fb9aeab Martina Krasteva 2021-05-27 170 * @sd: V4L2 sub-device
45d19b5fb9aeab Martina Krasteva 2021-05-27 171 * @pad: Media pad. Only one pad supported
45d19b5fb9aeab Martina Krasteva 2021-05-27 172 * @reset_gpio: Sensor reset gpio
fea91ee73b7cd1 Kieran Bingham 2023-12-11 173 * @supplies: Regulator supplies to handle power control
45d19b5fb9aeab Martina Krasteva 2021-05-27 174 * @inclk: Sensor input clock
45d19b5fb9aeab Martina Krasteva 2021-05-27 175 * @ctrl_handler: V4L2 control handler
45d19b5fb9aeab Martina Krasteva 2021-05-27 176 * @link_freq_ctrl: Pointer to link frequency control
45d19b5fb9aeab Martina Krasteva 2021-05-27 177 * @pclk_ctrl: Pointer to pixel clock control
45d19b5fb9aeab Martina Krasteva 2021-05-27 178 * @hblank_ctrl: Pointer to horizontal blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27 179 * @vblank_ctrl: Pointer to vertical blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27 180 * @exp_ctrl: Pointer to exposure control
45d19b5fb9aeab Martina Krasteva 2021-05-27 181 * @again_ctrl: Pointer to analog gain control
45d19b5fb9aeab Martina Krasteva 2021-05-27 182 * @vblank: Vertical blanking in lines
8c48b2175e7d73 Kieran Bingham 2024-03-06 183 * @lane_mode Mode for number of connected data lanes
45d19b5fb9aeab Martina Krasteva 2021-05-27 184 * @cur_mode: Pointer to current selected sensor mode
45d19b5fb9aeab Martina Krasteva 2021-05-27 185 * @mutex: Mutex for serializing sensor controls
0862582b5239a6 Umang Jain 2024-02-20 186 * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
cfa49ff0558a32 Umang Jain 2023-12-11 187 * @cur_mbus_code: Currently selected media bus format code
45d19b5fb9aeab Martina Krasteva 2021-05-27 188 */
45d19b5fb9aeab Martina Krasteva 2021-05-27 189 struct imx335 {
45d19b5fb9aeab Martina Krasteva 2021-05-27 190 struct device *dev;
45d19b5fb9aeab Martina Krasteva 2021-05-27 191 struct i2c_client *client;
45d19b5fb9aeab Martina Krasteva 2021-05-27 192 struct v4l2_subdev sd;
45d19b5fb9aeab Martina Krasteva 2021-05-27 193 struct media_pad pad;
45d19b5fb9aeab Martina Krasteva 2021-05-27 194 struct gpio_desc *reset_gpio;
fea91ee73b7cd1 Kieran Bingham 2023-12-11 195 struct regulator_bulk_data supplies[ARRAY_SIZE(imx335_supply_name)];
fa7ceacf9c0af6 Umang Jain 2024-03-06 196 struct regmap *cci;
fea91ee73b7cd1 Kieran Bingham 2023-12-11 197
45d19b5fb9aeab Martina Krasteva 2021-05-27 198 struct clk *inclk;
45d19b5fb9aeab Martina Krasteva 2021-05-27 199 struct v4l2_ctrl_handler ctrl_handler;
45d19b5fb9aeab Martina Krasteva 2021-05-27 200 struct v4l2_ctrl *link_freq_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 201 struct v4l2_ctrl *pclk_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 202 struct v4l2_ctrl *hblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 203 struct v4l2_ctrl *vblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 204 struct {
45d19b5fb9aeab Martina Krasteva 2021-05-27 205 struct v4l2_ctrl *exp_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 206 struct v4l2_ctrl *again_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27 207 };
45d19b5fb9aeab Martina Krasteva 2021-05-27 208 u32 vblank;
8c48b2175e7d73 Kieran Bingham 2024-03-06 209 u32 lane_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27 210 const struct imx335_mode *cur_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27 211 struct mutex mutex;
0862582b5239a6 Umang Jain 2024-02-20 212 unsigned long link_freq_bitmap;
cfa49ff0558a32 Umang Jain 2023-12-11 213 u32 cur_mbus_code;
45d19b5fb9aeab Martina Krasteva 2021-05-27 @214 };
45d19b5fb9aeab Martina Krasteva 2021-05-27 215
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki