* [RFC PATCH 0/3] v4l2 api changes for imx378 driver
@ 2020-04-14 20:01 Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw)
To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez
Hi all,
I would like to discuss with you guys the next two following topics:
* VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL:
I'm working on a driver for the imx378 sensor but the current v4l2-subdev API
(VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL) doesn't allow you to set up a range of frame
intervals. However, this is supported in the v4l2 device API level. My idea is
to follow the same approach as the VIDIOC_SUBDEV_ENUM_FRAME_SIZE by setting a
min and max intervals in the VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL to solve this
missing support.
This is the current output of VIDIOC_ENUM_FRAMEINTERVALS in continous mode:
v4l2-ctl --list-frameintervals width=1920,height=1080,pixelformat=pRAA \
-d /dev/video0
ioctl: VIDIOC_ENUM_FRAMEINTERVALS
Interval: Continuous 0.029s - 0.607s (1.648-34.797 fps)
This is the current output of VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL:
v4l2-ctl --list-subdev-frameintervals code=0x300f,width=1920,height=1080 \
-d /dev/v4l-subdev19
ioctl: VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (pad=0)
Interval: 0.029s (34.797 fps)
So, the idea would be to return the interval range from the v4l2-subdev level
to the device level. Besides that, it would also be necessary to adapt the
v4l-utils tools (compliance and ctl).
What do you think guys?
Please, follow the RFC patch series to see my suggestion.
* V4L2_CID_TEMPERATURE:
In addition to this, the driver is able to report as a v4l2 control the
temperature of the sensor. Since is quite 'general' control I also included the
v4l2 temperature control as part of the common v4l2 control list.
Would it be also possible?
In the RFC patch series you will find the the modified code for the
VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL and V4L2_CID_TEMPERATURE topics as well as
the imx378 driver using the above.
Daniel Gomez (3):
media: v4l2-subdev.h: Add min and max enum
media: v4l2: Add v4l2 control IDs for temperature
media: imx378: Add imx378 camera sensor driver
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 4 +-
include/uapi/linux/v4l2-subdev.h | 6 +-
6 files changed, 1854 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/i2c/imx378.c
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez
@ 2020-04-14 20:01 ` Daniel Gomez
2020-04-30 9:42 ` Sakari Ailus
2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez
2 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw)
To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez
Add min and max structures to the v4l2-subdev callback in order to allow
the subdev to return a range of valid frame intervals.
This would operate similar to the struct v4l2_subdev_frame_size_enum and
its max and min values for the width and the height. In this case, the
possibility to return a frame interval range is added to the v4l2-subdev level
whenever the v4l2 device operates in step-wise or continuous mode.
Signed-off-by: Daniel Gomez <daniel@qtec.com>
---
include/uapi/linux/v4l2-subdev.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index 03970ce30741..ee15393c58cd 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
* @code: format code (MEDIA_BUS_FMT_ definitions)
* @width: frame width in pixels
* @height: frame height in pixels
+ * @min_interval: min frame interval in seconds
+ * @max_interval: max frame interval in seconds
* @interval: frame interval in seconds
* @which: format type (from enum v4l2_subdev_format_whence)
*/
@@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
__u32 code;
__u32 width;
__u32 height;
+ struct v4l2_fract min_interval;
+ struct v4l2_fract max_interval;
struct v4l2_fract interval;
__u32 which;
- __u32 reserved[8];
+ __u32 reserved[4];
};
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature
2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez
@ 2020-04-14 20:01 ` Daniel Gomez
2020-04-28 20:15 ` Sakari Ailus
2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez
2 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw)
To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez
Add a v4l2 control ID to handle the temperature.
Signed-off-by: Daniel Gomez <daniel@qtec.com>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++
include/uapi/linux/v4l2-controls.h | 4 +++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 93d33d1db4e8..17b93111baa8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers";
case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component";
case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr";
+ case V4L2_CID_TEMPERATURE: return "Temperature";
/* Codec controls */
/* The MPEG controls are applicable to all codec controls
@@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
*type = V4L2_CTRL_TYPE_INTEGER;
*flags |= V4L2_CTRL_FLAG_READ_ONLY;
break;
+ case V4L2_CID_TEMPERATURE:
+ *type = V4L2_CTRL_TYPE_INTEGER;
+ *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
+ break;
case V4L2_CID_MPEG_VIDEO_DEC_PTS:
*type = V4L2_CTRL_TYPE_INTEGER64;
*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 1a58d7cc4ccc..76ec0a6da8d5 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -143,8 +143,10 @@ enum v4l2_colorfx {
#define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41)
#define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42)
+#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43)
+
/* last CID + 1 */
-#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44)
/* USER-class private control IDs */
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver
2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez
@ 2020-04-14 20:01 ` Daniel Gomez
2020-04-15 0:37 ` kbuild test robot
2020-04-28 21:02 ` Sakari Ailus
2 siblings, 2 replies; 15+ messages in thread
From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw)
To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez
Add a V4L2 sub-device driver for the Sony IMX378 image sensor.
This is a camera sensor using the I2C bus for control and the
CSI-2 bus for data.
Signed-off-by: Daniel Gomez <daniel@qtec.com>
---
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++++++++++++
3 files changed, 1841 insertions(+)
create mode 100644 drivers/media/i2c/imx378.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index efd12bf4f8eb..8ea630275faf 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -796,6 +796,17 @@ config VIDEO_IMX355
To compile this driver as a module, choose M here: the
module will be called imx355.
+config VIDEO_IMX378
+ tristate "Sony IMX378 sensor support"
+ depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+ depends on V4L2_FWNODE
+ help
+ This is a Video4Linux2 sensor driver for the Sony
+ IMX378 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx378.
+
config VIDEO_OV2640
tristate "OmniVision OV2640 sensor support"
depends on VIDEO_V4L2 && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 77bf7d0b691f..dff1c9ec444f 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o
obj-$(CONFIG_VIDEO_IMX290) += imx290.o
obj-$(CONFIG_VIDEO_IMX319) += imx319.o
obj-$(CONFIG_VIDEO_IMX355) += imx355.o
+obj-$(CONFIG_VIDEO_IMX378) += imx378.o
obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
obj-$(CONFIG_SDR_MAX2175) += max2175.o
diff --git a/drivers/media/i2c/imx378.c b/drivers/media/i2c/imx378.c
new file mode 100644
index 000000000000..6e22c3b45f72
--- /dev/null
+++ b/drivers/media/i2c/imx378.c
@@ -0,0 +1,1829 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * imx378.c - imx378 sensor driver
+ *
+ * Copyright 2019 Qtechnology A/S
+ *
+ * Daniel Gomez <daniel@qtec.com>
+ *
+ * Based on imx214.c and imx355.c
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define IMX378_DEFAULT_CLK_FREQ 24000000
+#define IMX378_DEFAULT_LINK_FREQ 480000000
+#define IMX378_DEFAULT_LINK_MFREQ (IMX378_DEFAULT_LINK_FREQ / 1000000)
+#define IMX378_DEFAULT_PIXEL_RATE ((IMX378_DEFAULT_LINK_FREQ * 8LL) / 10)
+#define IMX378_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
+#define IMX378_DEFAULT_FIVAL {1, 30}
+#define IMX378_LINE_LENGTH 4444
+#define IMX378_MIN_WIDTH 4
+#define IMX378_MIN_HEIGHT 4
+#define IMX378_MAX_DEFAULT_WIDTH 4032
+#define IMX378_MAX_DEFAULT_HEIGHT 3040
+#define IMX378_MAX_BOUNDS_WIDTH 4032
+#define IMX378_MAX_BOUNDS_HEIGHT 3104
+#define IMX378_CID_CUSTOM_BASE (V4L2_CID_USER_BASE | 0xf000)
+#define IMX378_CID_GREEN_BALANCE (IMX378_CID_CUSTOM_BASE + 0)
+
+static const char * const imx378_supply_name[] = {
+ "vdda",
+ "vddd",
+ "vdddo",
+};
+
+#define IMX378_NUM_SUPPLIES ARRAY_SIZE(imx378_supply_name)
+
+struct imx378 {
+ struct device *dev;
+ struct clk *xclk;
+ struct regmap *regmap;
+
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct v4l2_mbus_framefmt fmt;
+ struct v4l2_rect crop;
+
+ struct v4l2_ctrl_handler ctrls;
+ struct v4l2_ctrl *pixel_rate;
+ struct v4l2_ctrl *link_freq;
+ struct v4l2_ctrl *exposure;
+ struct v4l2_ctrl *vflip;
+ struct v4l2_ctrl *hflip;
+ struct v4l2_ctrl *analogue_gain;
+ struct v4l2_ctrl *digital_gain;
+ struct v4l2_ctrl *red_balance;
+ struct v4l2_ctrl *blue_balance;
+ struct v4l2_ctrl *green_balance;
+ struct v4l2_ctrl *temperature;
+
+ struct regulator_bulk_data supplies[IMX378_NUM_SUPPLIES];
+
+ struct gpio_desc *enable_gpio;
+
+ struct mutex mutex;
+
+ struct v4l2_fract fival;
+
+ bool streaming;
+};
+
+struct reg_8 {
+ u16 addr;
+ u8 val;
+};
+
+enum {
+ IMX378_TABLE_WAIT_MS = 0,
+ IMX378_TABLE_END,
+ IMX378_MAX_RETRIES,
+ IMX378_WAIT_MS
+};
+
+static const struct reg_8 mode_table_common[] = {
+ /* software standby settings */
+ {0x0100, 0x00},
+
+ /* software reset */
+ {0x0103, 0x01},
+
+ /* CSI */
+ {0x0112, 0x0A},
+ {0x0113, 0x0A},
+ {0x0114, 0x03},
+
+ /* global settings */
+ {0x0136, 0x18},
+ {0x0137, 0x00},
+
+ {0x0202, 0x20},
+ {0x0203, 0xa0},
+ {0x0204, 0x03},
+ {0x0205, 0xd2},
+ {0x020e, 0x01},
+ {0x020f, 0x00},
+ {0x0210, 0x02},
+ {0x0211, 0x21},
+ {0x0212, 0x01},
+ {0x0213, 0xee},
+ {0x0214, 0x01},
+ {0x0215, 0x00},
+ {0x0220, 0x00},
+ {0x0221, 0x11},
+
+ {0x0301, 0x05},
+ {0x0303, 0x02},
+ {0x0305, 0x03},
+ {0x0306, 0x00},
+ {0x0307, 0x96},
+
+ {0x0309, 0x0a},
+ {0x030b, 0x01},
+ {0x030d, 0x02},
+ {0x030e, 0x00},
+ {0x030f, 0xf6},
+ {0x0310, 0x00},
+ {0x0340, 0x04},
+ {0x0341, 0xb0},
+ {0x0342, 0x20},
+ {0x0343, 0x08},
+ {0x0344, 0x00},
+ {0x0345, 0x00},
+ {0x0346, 0x01},
+ {0x0347, 0x78},
+ {0x0348, 0x0f},
+ {0x0349, 0xd7},
+ {0x0350, 0x01},
+ {0x034a, 0x0a},
+ {0x034b, 0x67},
+ {0x034c, 0x07},
+ {0x034d, 0x80},
+ {0x034e, 0x04},
+ {0x034f, 0x38},
+ {0x0381, 0x01},
+ {0x0383, 0x01},
+ {0x0385, 0x01},
+ {0x0387, 0x01},
+
+ {0x0401, 0x00},
+ {0x0404, 0x00},
+ {0x0405, 0x10},
+ {0x0408, 0x00},
+ {0x0409, 0x6c},
+ {0x040a, 0x00},
+ {0x040b, 0x40},
+ {0x040c, 0x07},
+ {0x040d, 0x80},
+ {0x040e, 0x04},
+ {0x040f, 0x38},
+
+ {0x0900, 0x00},
+ {0x0901, 0x11},
+ {0x0902, 0x02},
+
+ {0x0c1a, 0x01},
+ {0x0c1b, 0x01},
+
+ {0x3030, 0x01},
+ {0x3032, 0x00},
+ {0x3033, 0x40},
+ {0x3140, 0x02},
+ {0x38a8, 0x1f},
+ {0x38a9, 0xff},
+ {0x38aa, 0x1f},
+ {0x38ab, 0xff},
+ {0x3c00, 0x00},
+ {0x3c01, 0x03},
+ {0x3c02, 0xdc},
+ {0x3d8a, 0x01},
+ {0x3e20, 0x01},
+ {0x3e37, 0x01},
+ {0x3f0d, 0x00},
+ {0x3f50, 0x00},
+ {0x3f56, 0x00},
+ {0x3f57, 0x01},
+ {0x3ff9, 0x00},
+
+ {0x4421, 0x08},
+ {0x4ae9, 0x18},
+ {0x4ae9, 0x21},
+ {0x4aea, 0x08},
+ {0x4aea, 0x80},
+
+ {0x55d4, 0x00},
+ {0x55d5, 0x00},
+ {0x55d6, 0x07},
+ {0x55d7, 0xff},
+ {0x55e8, 0x07},
+ {0x55e9, 0xff},
+ {0x55ea, 0x00},
+ {0x55eb, 0x00},
+ {0x5748, 0x07},
+ {0x5749, 0xff},
+ {0x574a, 0x00},
+ {0x574b, 0x00},
+ {0x574c, 0x07},
+ {0x574d, 0xff},
+ {0x574e, 0x00},
+ {0x574f, 0x00},
+ {0x5754, 0x00},
+ {0x5755, 0x00},
+ {0x5756, 0x07},
+ {0x5757, 0xff},
+ {0x5973, 0x04},
+ {0x5974, 0x01},
+ {0x5d13, 0xc3},
+ {0x5d14, 0x58},
+ {0x5d15, 0xa3},
+ {0x5d16, 0x1d},
+ {0x5d17, 0x65},
+ {0x5d18, 0x8c},
+ {0x5d1a, 0x06},
+ {0x5d1b, 0xa9},
+ {0x5d1c, 0x45},
+ {0x5d1d, 0x3a},
+ {0x5d1e, 0xab},
+ {0x5d1f, 0x15},
+ {0x5d21, 0x0e},
+ {0x5d22, 0x52},
+ {0x5d23, 0xaa},
+ {0x5d24, 0x7d},
+ {0x5d25, 0x57},
+ {0x5d26, 0xa8},
+ {0x5d37, 0x5a},
+ {0x5d38, 0x5a},
+ {0x5d77, 0x7f},
+
+ {0x7b3b, 0x01},
+ {0x7b4c, 0x00},
+ {0x7b53, 0x01},
+ {0x7b75, 0x0e},
+ {0x7b76, 0x0b},
+ {0x7b77, 0x08},
+ {0x7b78, 0x0a},
+ {0x7b79, 0x47},
+ {0x7b7c, 0x00},
+ {0x7b7d, 0x00},
+
+ {0x8d1f, 0x00},
+ {0x8d27, 0x00},
+
+ {0x9004, 0x03},
+ {0x9200, 0x50},
+ {0x9201, 0x6c},
+ {0x9202, 0x71},
+ {0x9203, 0x00},
+ {0x9204, 0x71},
+ {0x9205, 0x01},
+ {0x9304, 0x03},
+ {0x9305, 0x00},
+ {0x9369, 0x5a},
+ {0x936b, 0x55},
+ {0x936d, 0x28},
+ {0x9371, 0x6a},
+ {0x9373, 0x6a},
+ {0x9375, 0x64},
+ {0x9905, 0x00},
+ {0x9907, 0x00},
+ {0x9909, 0x00},
+ {0x990b, 0x00},
+ {0x991a, 0x00},
+ {0x9944, 0x3c},
+ {0x9947, 0x3c},
+ {0x994a, 0x8c},
+ {0x994b, 0x50},
+ {0x994c, 0x1b},
+ {0x994d, 0x8c},
+ {0x994e, 0x50},
+ {0x994f, 0x1b},
+ {0x9950, 0x8c},
+ {0x9951, 0x1b},
+ {0x9952, 0x0a},
+ {0x9953, 0x8c},
+ {0x9954, 0x1b},
+ {0x9955, 0x0a},
+ {0x996b, 0x8c},
+ {0x996c, 0x64},
+ {0x996d, 0x50},
+ {0x9a13, 0x04},
+ {0x9a14, 0x04},
+ {0x9a19, 0x00},
+ {0x9a1c, 0x04},
+ {0x9a1d, 0x04},
+ {0x9a26, 0x05},
+ {0x9a27, 0x05},
+ {0x9a2c, 0x01},
+ {0x9a2d, 0x03},
+ {0x9a2f, 0x05},
+ {0x9a30, 0x05},
+ {0x9a41, 0x00},
+ {0x9a46, 0x00},
+ {0x9a47, 0x00},
+ {0x9a4c, 0x0d},
+ {0x9a4d, 0x0d},
+ {0x9c17, 0x35},
+ {0x9c1d, 0x31},
+ {0x9c29, 0x50},
+ {0x9c3b, 0x2f},
+ {0x9c41, 0x6b},
+ {0x9c47, 0x2d},
+ {0x9c4d, 0x40},
+ {0x9c6b, 0x00},
+ {0x9c71, 0xc8},
+ {0x9c73, 0x32},
+ {0x9c75, 0x04},
+ {0x9c7d, 0x2d},
+ {0x9c83, 0x40},
+ {0x9c94, 0x3f},
+ {0x9c95, 0x3f},
+ {0x9c96, 0x3f},
+ {0x9c97, 0x00},
+ {0x9c98, 0x00},
+ {0x9c99, 0x00},
+ {0x9c9a, 0x3f},
+ {0x9c9b, 0x3f},
+ {0x9c9c, 0x3f},
+ {0x9ca0, 0x0f},
+ {0x9ca1, 0x0f},
+ {0x9ca2, 0x0f},
+ {0x9ca3, 0x00},
+ {0x9ca4, 0x00},
+ {0x9ca5, 0x00},
+ {0x9ca6, 0x1e},
+ {0x9ca7, 0x1e},
+ {0x9ca8, 0x1e},
+ {0x9ca9, 0x00},
+ {0x9caa, 0x00},
+ {0x9cab, 0x00},
+ {0x9cac, 0x09},
+ {0x9cad, 0x09},
+ {0x9cae, 0x09},
+ {0x9cbd, 0x50},
+ {0x9cbf, 0x50},
+ {0x9cc1, 0x50},
+ {0x9cc3, 0x40},
+ {0x9cc5, 0x40},
+ {0x9cc7, 0x40},
+ {0x9cc9, 0x0a},
+ {0x9ccb, 0x0a},
+ {0x9ccd, 0x0a},
+ {0x9d17, 0x35},
+ {0x9d1d, 0x31},
+ {0x9d29, 0x50},
+ {0x9d3b, 0x2f},
+ {0x9d41, 0x6b},
+ {0x9d47, 0x42},
+ {0x9d4d, 0x5a},
+ {0x9d6b, 0x00},
+ {0x9d71, 0xc8},
+ {0x9d73, 0x32},
+ {0x9d75, 0x04},
+ {0x9d7d, 0x42},
+ {0x9d83, 0x5a},
+ {0x9d94, 0x3f},
+ {0x9d95, 0x3f},
+ {0x9d96, 0x3f},
+ {0x9d97, 0x00},
+ {0x9d98, 0x00},
+ {0x9d99, 0x00},
+ {0x9d9a, 0x3f},
+ {0x9d9b, 0x3f},
+ {0x9d9c, 0x3f},
+ {0x9d9d, 0x1f},
+ {0x9d9e, 0x1f},
+ {0x9d9f, 0x1f},
+ {0x9da0, 0x0f},
+ {0x9da1, 0x0f},
+ {0x9da2, 0x0f},
+ {0x9da3, 0x00},
+ {0x9da4, 0x00},
+ {0x9da5, 0x00},
+ {0x9da6, 0x1e},
+ {0x9da7, 0x1e},
+ {0x9da8, 0x1e},
+ {0x9da9, 0x00},
+ {0x9daa, 0x00},
+ {0x9dab, 0x00},
+ {0x9dac, 0x09},
+ {0x9dad, 0x09},
+ {0x9dae, 0x09},
+ {0x9dc9, 0x0a},
+ {0x9dcb, 0x0a},
+ {0x9dcd, 0x0a},
+ {0x9e17, 0x35},
+ {0x9e1d, 0x31},
+ {0x9e29, 0x50},
+ {0x9e3b, 0x2f},
+ {0x9e41, 0x6b},
+ {0x9e47, 0x2d},
+ {0x9e4d, 0x40},
+ {0x9e6b, 0x00},
+ {0x9e71, 0xc8},
+ {0x9e73, 0x32},
+ {0x9e75, 0x04},
+ {0x9e94, 0x0f},
+ {0x9e95, 0x0f},
+ {0x9e96, 0x0f},
+ {0x9e97, 0x00},
+ {0x9e98, 0x00},
+ {0x9e99, 0x00},
+ {0x9e9a, 0x2f},
+ {0x9e9b, 0x2f},
+ {0x9e9c, 0x2f},
+ {0x9e9d, 0x00},
+ {0x9e9e, 0x00},
+ {0x9e9f, 0x00},
+ {0x9ea0, 0x0f},
+ {0x9ea1, 0x0f},
+ {0x9ea2, 0x0f},
+ {0x9ea3, 0x00},
+ {0x9ea4, 0x00},
+ {0x9ea5, 0x00},
+ {0x9ea6, 0x3f},
+ {0x9ea7, 0x3f},
+ {0x9ea8, 0x3f},
+ {0x9ea9, 0x00},
+ {0x9eaa, 0x00},
+ {0x9eab, 0x00},
+ {0x9eac, 0x09},
+ {0x9ead, 0x09},
+ {0x9eae, 0x09},
+ {0x9ec9, 0x0a},
+ {0x9ecb, 0x0a},
+ {0x9ecd, 0x0a},
+ {0x9f17, 0x35},
+ {0x9f1d, 0x31},
+ {0x9f29, 0x50},
+ {0x9f3b, 0x2f},
+ {0x9f41, 0x6b},
+ {0x9f47, 0x42},
+ {0x9f4d, 0x5a},
+ {0x9f6b, 0x00},
+ {0x9f71, 0xc8},
+ {0x9f73, 0x32},
+ {0x9f75, 0x04},
+ {0x9f94, 0x0f},
+ {0x9f95, 0x0f},
+ {0x9f96, 0x0f},
+ {0x9f97, 0x00},
+ {0x9f98, 0x00},
+ {0x9f99, 0x00},
+ {0x9f9a, 0x2f},
+ {0x9f9b, 0x2f},
+ {0x9f9c, 0x2f},
+ {0x9f9d, 0x00},
+ {0x9f9e, 0x00},
+ {0x9f9f, 0x00},
+ {0x9fa0, 0x0f},
+ {0x9fa1, 0x0f},
+ {0x9fa2, 0x0f},
+ {0x9fa3, 0x00},
+ {0x9fa4, 0x00},
+ {0x9fa5, 0x00},
+ {0x9fa6, 0x1e},
+ {0x9fa7, 0x1e},
+ {0x9fa8, 0x1e},
+ {0x9fa9, 0x00},
+ {0x9faa, 0x00},
+ {0x9fab, 0x00},
+ {0x9fac, 0x09},
+ {0x9fad, 0x09},
+ {0x9fae, 0x09},
+ {0x9fc9, 0x0a},
+ {0x9fcb, 0x0a},
+ {0x9fcd, 0x0a},
+
+ {0xa001, 0x0a},
+ {0xa003, 0x0a},
+ {0xa005, 0x0a},
+ {0xa006, 0x01},
+ {0xa007, 0xc0},
+ {0xa009, 0xc0},
+ {0xa14b, 0xff},
+ {0xa151, 0x0c},
+ {0xa153, 0x50},
+ {0xa155, 0x02},
+ {0xa157, 0x00},
+ {0xa1ad, 0xff},
+ {0xa1b3, 0x0c},
+ {0xa1b5, 0x50},
+ {0xa1b9, 0x00},
+ {0xa24b, 0xff},
+ {0xa257, 0x00},
+ {0xa2a9, 0x60},
+ {0xa2ad, 0xff},
+ {0xa2b7, 0x00},
+ {0xa2b9, 0x00},
+
+ {0xb21f, 0x04},
+ {0xb35c, 0x00},
+ {0xb35e, 0x08},
+ {0xbcf1, 0x02},
+ {0xe000, 0x00},
+ {0xf61c, 0x04},
+ {0xf61e, 0x04},
+
+ {IMX378_TABLE_END, 0x00}
+};
+
+static inline struct imx378 *to_imx378(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct imx378, sd);
+}
+
+static int imx378_set_flip_mode(struct imx378 *imx378, u32 code)
+{
+ /* WORKAROUND to update controls */
+ imx378->vflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY;
+ imx378->hflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY;
+
+ switch (code) {
+ case MEDIA_BUS_FMT_SBGGR10_1X10:
+ v4l2_ctrl_s_ctrl(imx378->vflip, 1);
+ v4l2_ctrl_s_ctrl(imx378->hflip, 1);
+ break;
+ case MEDIA_BUS_FMT_SGBRG10_1X10:
+ v4l2_ctrl_s_ctrl(imx378->vflip, 1);
+ v4l2_ctrl_s_ctrl(imx378->hflip, 0);
+ break;
+ case MEDIA_BUS_FMT_SGRBG10_1X10:
+ v4l2_ctrl_s_ctrl(imx378->vflip, 0);
+ v4l2_ctrl_s_ctrl(imx378->hflip, 1);
+ break;
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ v4l2_ctrl_s_ctrl(imx378->vflip, 0);
+ v4l2_ctrl_s_ctrl(imx378->hflip, 0);
+ break;
+ default:
+ break;
+ }
+
+ /* WORKAROUND to update controls */
+ imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ return 0;
+}
+
+static u32 imx378_get_format_code(struct imx378 *imx378)
+{
+ /*
+ * Only one bayer order is supported.
+ * It depends on the flip settings.
+ */
+ u32 code;
+ static const u32 codes[2][2] = {
+ { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
+ { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
+ };
+
+ lockdep_assert_held(&imx378->mutex);
+ code = codes[imx378->vflip->val][imx378->hflip->val];
+
+ return code;
+}
+
+static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val)
+{
+ int ret;
+ u32 h_val = 0, l_val = 0;
+
+ ret = regmap_read(imx378->regmap, addr + 1, &l_val);
+ if (ret) {
+ pr_err("%s:i2c read failed, %x\n", __func__, addr + 1);
+ return ret;
+ }
+
+ *val = l_val & 0xFF;
+
+ ret = regmap_read(imx378->regmap, addr, &h_val);
+ if (ret) {
+ pr_err("%s:i2c read failed, %x\n", __func__, addr);
+ return ret;
+ }
+
+ *val |= (h_val & 0xFF) << 8;
+
+ return ret;
+}
+
+static int imx378_write_reg16(struct imx378 *imx378, u16 addr, u16 val)
+{
+ int ret;
+
+ ret = regmap_write(imx378->regmap, addr + 1, val);
+ if (ret) {
+ pr_err("%s:i2c write failed, %x = %x\n",
+ __func__, addr + 1, val);
+ return ret;
+ }
+
+ ret = regmap_write(imx378->regmap, addr, val >> 8);
+ if (ret) {
+ pr_err("%s:i2c write failed, %x = %x\n",
+ __func__, addr, val >> 8);
+ return ret;
+ }
+
+ return ret;
+}
+
+static int __maybe_unused imx378_power_on(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx378 *imx378 = to_imx378(sd);
+ int ret;
+
+ gpiod_set_value_cansleep(imx378->enable_gpio, 0);
+
+ ret = regulator_bulk_enable(IMX378_NUM_SUPPLIES, imx378->supplies);
+ if (ret < 0) {
+ dev_err(imx378->dev, "failed to enable regulators: %d\n", ret);
+ return ret;
+ }
+
+ usleep_range(2000, 3000);
+
+ ret = clk_prepare_enable(imx378->xclk);
+ if (ret < 0) {
+ regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies);
+ dev_err(imx378->dev, "clk prepare enable failed\n");
+ return ret;
+ }
+
+ gpiod_set_value_cansleep(imx378->enable_gpio, 1);
+ usleep_range(12000, 15000);
+
+ return 0;
+}
+
+static int __maybe_unused imx378_power_off(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx378 *imx378 = to_imx378(sd);
+
+ gpiod_set_value_cansleep(imx378->enable_gpio, 0);
+
+ clk_disable_unprepare(imx378->xclk);
+
+ regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies);
+ usleep_range(10, 20);
+
+ return 0;
+}
+
+static int imx378_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct imx378 *imx378 = container_of(sd, struct imx378, sd);
+
+ if (code->index != 0 || code->pad != 0)
+ return -EINVAL;
+
+ code->code = imx378_get_format_code(imx378);
+
+ return 0;
+}
+
+static int imx378_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ struct imx378 *imx378 = container_of(sd, struct imx378, sd);
+
+ if (fse->index != 0)
+ return -EINVAL;
+
+ mutex_lock(&imx378->mutex);
+ if (fse->code != imx378_get_format_code(imx378)) {
+ mutex_unlock(&imx378->mutex);
+ return -EINVAL;
+ }
+ mutex_unlock(&imx378->mutex);
+
+ fse->min_width = IMX378_MIN_WIDTH;
+ fse->max_width = IMX378_MAX_BOUNDS_WIDTH;
+ fse->min_height = IMX378_MIN_HEIGHT;
+ fse->max_height = IMX378_MAX_BOUNDS_HEIGHT;
+
+ return 0;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int imx378_s_register(struct v4l2_subdev *sd,
+ const struct v4l2_dbg_register *reg)
+{
+ struct imx378 *imx378 = container_of(sd, struct imx378, sd);
+
+ return regmap_write(imx378->regmap, reg->reg, reg->val);
+}
+
+static int imx378_g_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ struct imx378 *imx378 = container_of(sd, struct imx378, sd);
+ unsigned int aux;
+ int ret;
+
+ reg->size = 1;
+ ret = regmap_read(imx378->regmap, reg->reg, &aux);
+ reg->val = aux;
+
+ return ret;
+}
+#endif
+
+static const struct v4l2_subdev_core_ops imx378_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .g_register = imx378_g_register,
+ .s_register = imx378_s_register,
+#endif
+};
+
+static int imx378_set_img_size(struct imx378 *imx378)
+{
+ u32 frame_length;
+ int ret;
+
+ ret = imx378_write_reg16(imx378, 0x34c, imx378->fmt.width);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x34e, imx378->fmt.height);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x40c, imx378->fmt.width);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x40e, imx378->fmt.height);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x342, IMX378_LINE_LENGTH);
+ if (ret < 0)
+ goto error;
+
+ frame_length = IMX378_DEFAULT_LINK_FREQ /
+ ((imx378->fival.denominator / imx378->fival.numerator)
+ * IMX378_LINE_LENGTH);
+
+ ret = imx378_write_reg16(imx378, 0x340, frame_length);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x344, imx378->crop.left);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x348, imx378->crop.left +
+ imx378->fmt.width - 1);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x346, imx378->crop.top);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x34a, imx378->crop.top +
+ imx378->fmt.height - 1);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x408, 0);
+ if (ret < 0)
+ goto error;
+
+ ret = imx378_write_reg16(imx378, 0x40a, 0);
+ if (ret < 0)
+ goto error;
+
+ ret = regmap_write(imx378->regmap, 0xb04, 1);
+ if (ret < 0)
+ goto error;
+
+ return ret;
+
+error:
+ dev_err(imx378->dev, "could not set image size %d\n", ret);
+ return ret;
+}
+
+static struct v4l2_mbus_framefmt *
+__imx378_get_pad_format(struct imx378 *imx378,
+ struct v4l2_subdev_pad_config *cfg,
+ unsigned int pad,
+ enum v4l2_subdev_format_whence which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(&imx378->sd, cfg, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &imx378->fmt;
+ default:
+ return NULL;
+ }
+}
+
+static int imx378_get_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+
+ mutex_lock(&imx378->mutex);
+ fmt->format = *__imx378_get_pad_format(imx378, cfg, fmt->pad,
+ fmt->which);
+ mutex_unlock(&imx378->mutex);
+
+ return 0;
+}
+
+static struct v4l2_rect *
+__imx378_get_pad_crop(struct imx378 *imx378, struct v4l2_subdev_pad_config *cfg,
+ unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(&imx378->sd, cfg, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &imx378->crop;
+ default:
+ return NULL;
+ }
+}
+
+static int imx378_min_interval(struct imx378 *imx378,
+ struct v4l2_fract *fival)
+{
+ fival->numerator = IMX378_LINE_LENGTH * (imx378->fmt.height);
+ fival->denominator = IMX378_DEFAULT_LINK_FREQ;
+
+ return 0;
+}
+
+static int imx378_max_interval(struct v4l2_fract *fival)
+{
+ fival->numerator = IMX378_LINE_LENGTH * 0xffff;
+ fival->denominator = IMX378_DEFAULT_LINK_FREQ;
+
+ return 0;
+}
+
+#define FRACT_CMP(a, OP, b) \
+ ((u64)(a).numerator * (b).denominator OP \
+ (u64)(b).numerator * (a).denominator)
+static int imx378_clamp_interval(struct imx378 *imx378,
+ struct v4l2_fract *fival)
+{
+ struct v4l2_fract interval;
+ int ret;
+
+ ret = imx378_min_interval(imx378, &interval);
+ if (ret)
+ return ret;
+ if (FRACT_CMP(*fival, <, interval))
+ *fival = interval;
+
+ ret = imx378_max_interval(&interval);
+ if (ret)
+ return ret;
+ if (FRACT_CMP(*fival, >, interval))
+ *fival = interval;
+
+ return 0;
+}
+
+static int imx378_g_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fival)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+
+ fival->interval = imx378->fival;
+
+ return 0;
+}
+
+static int imx378_s_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_frame_interval *fival)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+ u64 max_exposure;
+
+ if (fival->interval.denominator == 0)
+ fival->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
+
+ imx378_clamp_interval(imx378, &fival->interval);
+
+ imx378->fival = fival->interval;
+
+ /* Update exposure limits */
+ max_exposure = (IMX378_DEFAULT_LINK_FREQ /
+ ((imx378->fival.denominator / imx378->fival.numerator)
+ * IMX378_LINE_LENGTH)) - 20;
+
+ __v4l2_ctrl_modify_range(imx378->exposure, imx378->exposure->minimum,
+ max_exposure, imx378->exposure->step,
+ max_exposure);
+
+ return 0;
+}
+
+static int
+imx378_enum_frame_interval(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_interval_enum *fie)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+ int ret;
+
+ if (fie->index != 0)
+ return -EINVAL;
+
+ if (fie->width > IMX378_MAX_BOUNDS_WIDTH ||
+ fie->width < IMX378_MIN_WIDTH)
+ return -EINVAL;
+
+ if (fie->height > IMX378_MAX_BOUNDS_HEIGHT ||
+ fie->height < IMX378_MIN_HEIGHT)
+ return -EINVAL;
+
+ mutex_lock(&imx378->mutex);
+ if (fie->code != imx378_get_format_code(imx378)) {
+ mutex_unlock(&imx378->mutex);
+ return -EINVAL;
+ }
+ mutex_unlock(&imx378->mutex);
+
+ ret = imx378_min_interval(imx378, &fie->min_interval);
+ if (ret)
+ return ret;
+
+ ret = imx378_max_interval(&fie->max_interval);
+ if (ret)
+ return ret;
+
+ fie->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
+
+ return 0;
+}
+
+static int imx378_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_format *fmt)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+ struct v4l2_mbus_framefmt *__fmt;
+ struct v4l2_rect *__crop;
+ struct v4l2_subdev_frame_interval fival = { };
+
+ mutex_lock(&imx378->mutex);
+
+ if (imx378->streaming && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
+ return -EBUSY;
+
+ __crop = __imx378_get_pad_crop(imx378, cfg, fmt->pad, fmt->which);
+
+ v4l_bound_align_image(&fmt->format.width, IMX378_MIN_WIDTH,
+ IMX378_MAX_BOUNDS_WIDTH, ilog2(4),
+ &fmt->format.height, IMX378_MIN_HEIGHT,
+ IMX378_MAX_BOUNDS_HEIGHT, ilog2(8), 0);
+
+ __crop->width = fmt->format.width;
+ __crop->height = fmt->format.height;
+
+ v4l_bound_align_image(&__crop->left, 0,
+ IMX378_MAX_BOUNDS_WIDTH - __crop->width, ilog2(4),
+ &__crop->top, 0,
+ IMX378_MAX_BOUNDS_HEIGHT - __crop->height,
+ ilog2(8), 0);
+
+ __fmt = __imx378_get_pad_format(imx378, cfg, fmt->pad, fmt->which);
+
+ __fmt->width = __crop->width;
+ __fmt->height = __crop->height;
+
+ if (fmt->format.code && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
+ imx378_set_flip_mode(imx378, fmt->format.code);
+
+ __fmt->code = imx378_get_format_code(imx378);
+
+ __fmt->field = V4L2_FIELD_NONE;
+ __fmt->colorspace = V4L2_COLORSPACE_SRGB;
+ __fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace);
+ __fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+ __fmt->colorspace, __fmt->ycbcr_enc);
+ __fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__fmt->colorspace);
+
+ fmt->format = *__fmt;
+
+ /* Frame interval depends on the format so, update it accordingly */
+ if (fmt->which != V4L2_SUBDEV_FORMAT_TRY) {
+ fival.interval = imx378->fival;
+ imx378_s_frame_interval(sd, &fival);
+ }
+
+ mutex_unlock(&imx378->mutex);
+
+ return 0;
+}
+
+static int imx378_get_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_selection *s)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_COMPOSE:
+ s->r = imx378->crop;
+ return 0;
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ s->r.width = IMX378_MAX_DEFAULT_WIDTH;
+ s->r.height = IMX378_MAX_DEFAULT_HEIGHT;
+ s->r.left = 0;
+ s->r.top = 0;
+ return 0;
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ s->r.width = IMX378_MAX_BOUNDS_WIDTH;
+ s->r.height = IMX378_MAX_BOUNDS_HEIGHT;
+ s->r.left = 0;
+ s->r.top = 0;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int imx378_set_selection(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_selection *s)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+ int ret;
+
+ if (s->target == V4L2_SEL_TGT_COMPOSE)
+ return 0;
+
+ if (s->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
+
+ mutex_lock(&imx378->mutex);
+
+ s->r.width = imx378->crop.width;
+ s->r.height = imx378->crop.height;
+
+ v4l_bound_align_image(&s->r.left, 0,
+ IMX378_MAX_BOUNDS_WIDTH - s->r.width, ilog2(4),
+ &s->r.top, 0,
+ IMX378_MAX_BOUNDS_HEIGHT - s->r.height, ilog2(8),
+ 0);
+
+ imx378->crop.left = s->r.left;
+ imx378->crop.top = s->r.top;
+
+ if (imx378->streaming) {
+ ret = imx378_set_img_size(imx378);
+ if (ret < 0) {
+ dev_err(imx378->dev, "could not set image size\n");
+ mutex_unlock(&imx378->mutex);
+ return ret;
+ }
+ }
+
+ mutex_unlock(&imx378->mutex);
+ return 0;
+}
+
+static int imx378_set_exposure(struct imx378 *imx378)
+{
+ int ret;
+
+ /* Set group hold */
+ ret = regmap_write(imx378->regmap, 0x104, 1);
+ if (ret < 0)
+ return ret;
+
+ ret = imx378_write_reg16(imx378, 0x202, imx378->exposure->val);
+ if (ret < 0)
+ return ret;
+
+ /* Release group hold */
+ return regmap_write(imx378->regmap, 0x104, 0);
+}
+
+static int imx378_set_analogue_gain(struct imx378 *imx378)
+{
+ int ret;
+
+ /* Set group hold */
+ ret = regmap_write(imx378->regmap, 0x104, 1);
+ if (ret < 0)
+ return ret;
+
+ ret = imx378_write_reg16(imx378, 0x204, imx378->analogue_gain->val);
+ if (ret < 0)
+ return ret;
+
+ /* Release group hold */
+ return regmap_write(imx378->regmap, 0x104, 0);
+}
+
+static int imx378_set_digital_gain_global(struct imx378 *imx378)
+{
+ int ret;
+
+ /* Set group hold */
+ ret = regmap_write(imx378->regmap, 0x104, 1);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain control mode all color */
+ ret = regmap_write(imx378->regmap, 0x3ff9, 1);
+ if (ret < 0)
+ return ret;
+
+ /* Set digital gain globally */
+ ret = imx378_write_reg16(imx378, 0x20e, imx378->digital_gain->val);
+ if (ret < 0)
+ return ret;
+
+ /* Release group hold */
+ return regmap_write(imx378->regmap, 0x104, 0);
+}
+
+static int imx378_set_digital_gain_by_color(struct imx378 *imx378)
+{
+ int ret;
+
+ /* Set group hold */
+ ret = regmap_write(imx378->regmap, 0x104, 1);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain control mode by color */
+ ret = regmap_write(imx378->regmap, 0x3ff9, 0);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain R */
+ ret = imx378_write_reg16(imx378, 0x210, imx378->red_balance->val);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain B */
+ ret = imx378_write_reg16(imx378, 0x212, imx378->blue_balance->val);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain GR */
+ ret = imx378_write_reg16(imx378, 0x20e, imx378->green_balance->val);
+ if (ret < 0)
+ return ret;
+
+ /* Digital gain GB */
+ ret = imx378_write_reg16(imx378, 0x214, imx378->green_balance->val);
+ if (ret < 0)
+ return ret;
+
+ /* Release group hold */
+ return regmap_write(imx378->regmap, 0x104, 0);
+}
+
+static int imx378_g_temperature(struct imx378 *imx378, s32 *temperature)
+{
+ unsigned int aux;
+ int ret;
+
+ if (imx378->streaming) {
+ ret = regmap_read(imx378->regmap, 0x13a, &aux);
+ if (ret < 0)
+ return ret;
+
+ if (aux >= 0xec)
+ *temperature = (aux - 0xec - 20) * 1000;
+ else
+ *temperature = (aux * 1000);
+ }
+
+ dev_err(imx378->dev,
+ "Sensor is powered off, returning invalid temperature\n");
+
+ return 0;
+}
+
+static int imx378_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct imx378 *imx378 = container_of(ctrl->handler,
+ struct imx378, ctrls);
+ int ret;
+
+ /*
+ * Applying V4L2 control value only happens
+ * when power is up for streaming
+ */
+ if (!pm_runtime_get_if_in_use(imx378->dev))
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_TEMPERATURE:
+ ret = imx378_g_temperature(imx378, &ctrl->val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ pm_runtime_put(imx378->dev);
+
+ return ret;
+}
+
+static int imx378_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct imx378 *imx378 = container_of(ctrl->handler,
+ struct imx378, ctrls);
+ int ret;
+
+ /*
+ * Applying V4L2 control value only happens
+ * when power is up for streaming
+ */
+ if (!pm_runtime_get_if_in_use(imx378->dev))
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_EXPOSURE_ABSOLUTE:
+ ret = imx378_set_exposure(imx378);
+ break;
+ case V4L2_CID_ANALOGUE_GAIN:
+ ret = imx378_set_analogue_gain(imx378);
+ break;
+ case V4L2_CID_DIGITAL_GAIN:
+ ret = imx378_set_digital_gain_global(imx378);
+ break;
+ case V4L2_CID_RED_BALANCE:
+ case V4L2_CID_BLUE_BALANCE:
+ case IMX378_CID_GREEN_BALANCE:
+ ret = imx378_set_digital_gain_by_color(imx378);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ pm_runtime_put(imx378->dev);
+
+ return ret;
+}
+
+static const struct v4l2_ctrl_ops imx378_ctrl_ops = {
+ .g_volatile_ctrl = imx378_g_volatile_ctrl,
+ .s_ctrl = imx378_set_ctrl,
+};
+
+#define MAX_CMD 4
+static int imx378_write_table(struct imx378 *imx378,
+ const struct reg_8 table[])
+{
+ u8 vals[MAX_CMD];
+ int i, ret;
+
+ for (table = table; table->addr != IMX378_TABLE_END ; table++) {
+ if (table->addr == IMX378_TABLE_WAIT_MS) {
+ usleep_range(table->val * 1000,
+ table->val * 1000 + 500);
+ continue;
+ }
+
+ for (i = 0; i < MAX_CMD; i++) {
+ if (table[i].addr != (table[0].addr + i))
+ break;
+ vals[i] = table[i].val;
+ }
+
+ ret = regmap_bulk_write(imx378->regmap, table->addr, vals, i);
+ usleep_range(80, 120);
+
+ if (ret) {
+ dev_err(imx378->dev, "write_table error: %d\n", ret);
+ return ret;
+ }
+
+ table += i - 1;
+ }
+
+ return 0;
+}
+
+static int imx378_start_streaming(struct imx378 *imx378)
+{
+ int ret;
+
+ ret = imx378_write_table(imx378, mode_table_common);
+ if (ret < 0) {
+ dev_err(imx378->dev, "could not send common table %d\n", ret);
+ goto error;
+ }
+
+ ret = __v4l2_ctrl_handler_setup(&imx378->ctrls);
+ if (ret < 0) {
+ dev_err(imx378->dev, "could not sync v4l2 controls\n");
+ goto error;
+ }
+
+ ret = imx378_set_img_size(imx378);
+ if (ret < 0) {
+ dev_err(imx378->dev, "could not set image size\n");
+ goto error;
+ }
+
+ /* set orientation */
+ ret = regmap_write(imx378->regmap, 0x101,
+ imx378->hflip->val | imx378->vflip->val << 1);
+ if (ret < 0)
+ goto error;
+
+ /* set temperature control */
+ ret = regmap_write(imx378->regmap, 0x138, 1);
+ if (ret < 0)
+ goto error;
+
+ ret = regmap_write(imx378->regmap, 0x100, 1);
+ if (ret < 0) {
+ dev_err(imx378->dev, "could not send start table %d\n", ret);
+ goto error;
+ }
+
+ usleep_range(4000, 6000);
+
+ return 0;
+
+error:
+ mutex_unlock(&imx378->mutex);
+ return ret;
+}
+
+static int imx378_stop_streaming(struct imx378 *imx378)
+{
+ int ret;
+
+ ret = regmap_write(imx378->regmap, 0x100, 0);
+ if (ret < 0)
+ dev_err(imx378->dev, "could not send stop table %d\n", ret);
+
+ return ret;
+}
+
+static int imx378_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct imx378 *imx378 = to_imx378(sd);
+ int ret;
+
+ mutex_lock(&imx378->mutex);
+
+ if (imx378->streaming == enable) {
+ mutex_unlock(&imx378->mutex);
+ return 0;
+ }
+
+ if (enable) {
+ ret = pm_runtime_get_sync(imx378->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(imx378->dev);
+ goto err_unlock;
+ }
+
+ ret = imx378_start_streaming(imx378);
+ if (ret < 0)
+ goto err_rpm_put;
+ } else {
+ ret = imx378_stop_streaming(imx378);
+ if (ret < 0)
+ goto err_rpm_put;
+ pm_runtime_put(imx378->dev);
+ }
+
+ imx378->streaming = enable;
+
+ mutex_unlock(&imx378->mutex);
+
+ return 0;
+
+err_rpm_put:
+ pm_runtime_put(imx378->dev);
+err_unlock:
+ mutex_unlock(&imx378->mutex);
+
+ return ret;
+}
+
+static int imx378_entity_init_cfg(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg)
+{
+ struct v4l2_subdev_format fmt = { };
+ struct v4l2_subdev_frame_interval fival = { };
+
+ if (!cfg) {
+ fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY :
+ V4L2_SUBDEV_FORMAT_ACTIVE;
+ fmt.format.width = IMX378_MAX_BOUNDS_WIDTH;
+ fmt.format.height = IMX378_MAX_BOUNDS_HEIGHT;
+
+ imx378_set_fmt(sd, cfg, &fmt);
+
+ fival.interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
+ imx378_s_frame_interval(sd, &fival);
+ }
+
+ return 0;
+}
+
+static const struct v4l2_subdev_video_ops imx378_video_ops = {
+ .s_stream = imx378_s_stream,
+ .g_frame_interval = imx378_g_frame_interval,
+ .s_frame_interval = imx378_s_frame_interval,
+};
+
+static const struct v4l2_subdev_pad_ops imx378_subdev_pad_ops = {
+ .enum_mbus_code = imx378_enum_mbus_code,
+ .enum_frame_size = imx378_enum_frame_size,
+ .enum_frame_interval = imx378_enum_frame_interval,
+ .get_fmt = imx378_get_fmt,
+ .set_fmt = imx378_set_fmt,
+ .get_selection = imx378_get_selection,
+ .set_selection = imx378_set_selection,
+ .init_cfg = imx378_entity_init_cfg,
+};
+
+static const struct v4l2_subdev_ops imx378_subdev_ops = {
+ .core = &imx378_core_ops,
+ .video = &imx378_video_ops,
+ .pad = &imx378_subdev_pad_ops,
+};
+
+static const struct regmap_config sensor_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static int imx378_get_regulators(struct device *dev, struct imx378 *imx378)
+{
+ unsigned int i;
+
+ for (i = 0; i < IMX378_NUM_SUPPLIES; i++)
+ imx378->supplies[i].supply = imx378_supply_name[i];
+
+ return devm_regulator_bulk_get(dev, IMX378_NUM_SUPPLIES,
+ imx378->supplies);
+}
+
+struct v4l2_ctrl
+*imx378_ctrl_new_str_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name,
+ s64 min, s64 max, u64 step, s64 def)
+{
+ static struct v4l2_ctrl_config ctrl = {
+ .ops = &imx378_ctrl_ops,
+ .type = V4L2_CTRL_TYPE_STRING,
+ };
+
+ ctrl.id = id;
+ ctrl.name = name;
+ ctrl.min = min;
+ ctrl.max = max;
+ ctrl.step = step;
+ ctrl.def = def;
+
+ return v4l2_ctrl_new_custom(hdl, &ctrl, NULL);
+}
+
+struct v4l2_ctrl
+*imx378_ctrl_new_int_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name,
+ s64 min, s64 max, u64 step, s64 def)
+{
+ static struct v4l2_ctrl_config ctrl = {
+ .ops = &imx378_ctrl_ops,
+ .type = V4L2_CTRL_TYPE_INTEGER,
+ };
+
+ ctrl.id = id;
+ ctrl.name = name;
+ ctrl.min = min;
+ ctrl.max = max;
+ ctrl.step = step;
+ ctrl.def = def;
+
+ return v4l2_ctrl_new_custom(hdl, &ctrl, NULL);
+}
+
+static int imx378_init_controls(struct imx378 *imx378)
+{
+ static const s64 link_freq[] = {
+ IMX378_DEFAULT_LINK_FREQ,
+ };
+ int ret;
+
+ mutex_init(&imx378->mutex);
+ imx378->ctrls.lock = &imx378->mutex;
+
+ v4l2_ctrl_handler_init(&imx378->ctrls, 12);
+
+ imx378->pixel_rate = v4l2_ctrl_new_std(&imx378->ctrls, NULL,
+ V4L2_CID_PIXEL_RATE, 0,
+ IMX378_DEFAULT_PIXEL_RATE, 1,
+ IMX378_DEFAULT_PIXEL_RATE);
+
+ imx378->link_freq = v4l2_ctrl_new_int_menu(&imx378->ctrls, NULL,
+ V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(link_freq) - 1,
+ 0, link_freq);
+ if (imx378->link_freq)
+ imx378->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ imx378->exposure = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
+ V4L2_CID_EXPOSURE_ABSOLUTE,
+ 0x8, 0xffff, 1, 0xc70);
+
+ imx378->hflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ if (imx378->hflip)
+ imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ imx378->vflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ if (imx378->vflip)
+ imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ imx378->analogue_gain = v4l2_ctrl_new_std(&imx378->ctrls,
+ &imx378_ctrl_ops,
+ V4L2_CID_ANALOGUE_GAIN,
+ 0, 978, 1, 0);
+
+ imx378->digital_gain = v4l2_ctrl_new_std(&imx378->ctrls,
+ &imx378_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+ 0x100, 0xfff, 1, 0x100);
+
+ imx378->red_balance = v4l2_ctrl_new_std(&imx378->ctrls,
+ &imx378_ctrl_ops, V4L2_CID_RED_BALANCE,
+ 0x100, 0xfff, 1, 0x221);
+
+ imx378->blue_balance = v4l2_ctrl_new_std(&imx378->ctrls,
+ &imx378_ctrl_ops, V4L2_CID_BLUE_BALANCE,
+ 0x100, 0xfff, 1, 0x1ee);
+
+ imx378->green_balance = imx378_ctrl_new_int_custom(&imx378->ctrls,
+ IMX378_CID_GREEN_BALANCE,
+ "Green Balance",
+ 0x100, 0xfff, 1, 0x100);
+ if (imx378->green_balance)
+ imx378->green_balance->flags = V4L2_CTRL_FLAG_SLIDER;
+
+ imx378->temperature = v4l2_ctrl_new_std(&imx378->ctrls,
+ &imx378_ctrl_ops, V4L2_CID_TEMPERATURE,
+ -200000, 800000, 1, 250000);
+ if (imx378->temperature)
+ imx378->temperature->name = "Sensor Temperature";
+
+ ret = imx378->ctrls.error;
+ if (ret) {
+ dev_err(imx378->dev, "%s control init failed (%d)\n",
+ __func__, ret);
+ goto error;
+ }
+
+ imx378->sd.ctrl_handler = &imx378->ctrls;
+
+ return 0;
+
+error:
+ v4l2_ctrl_handler_free(&imx378->ctrls);
+ return ret;
+}
+
+static int imx378_parse_fwnode(struct device *dev)
+{
+ struct fwnode_handle *endpoint;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY,
+ };
+ unsigned int i;
+ int ret;
+
+ endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
+ if (!endpoint) {
+ dev_err(dev, "endpoint node not found\n");
+ return -EINVAL;
+ }
+
+ ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+ if (ret) {
+ dev_err(dev, "parsing endpoint node failed\n");
+ goto done;
+ }
+
+ for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
+ if (bus_cfg.link_frequencies[i] == IMX378_DEFAULT_LINK_FREQ)
+ break;
+
+ if (i == bus_cfg.nr_of_link_frequencies) {
+ dev_err(dev, "link-frequencies %d not supported, Please review your DT\n",
+ IMX378_DEFAULT_LINK_FREQ);
+ ret = -EINVAL;
+ goto done;
+ }
+
+done:
+ v4l2_fwnode_endpoint_free(&bus_cfg);
+ fwnode_handle_put(endpoint);
+ return ret;
+}
+
+static int __maybe_unused imx378_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx378 *imx378 = to_imx378(sd);
+
+ if (imx378->streaming)
+ imx378_stop_streaming(imx378);
+
+ return 0;
+}
+
+static int __maybe_unused imx378_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx378 *imx378 = to_imx378(sd);
+ int ret;
+
+ if (imx378->streaming) {
+ ret = imx378_start_streaming(imx378);
+ if (ret)
+ goto error;
+ }
+
+ return 0;
+
+error:
+ imx378_stop_streaming(imx378);
+ imx378->streaming = 0;
+ return ret;
+}
+
+static int imx378_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct imx378 *imx378;
+ int ret;
+
+ ret = imx378_parse_fwnode(dev);
+ if (ret)
+ return ret;
+
+ imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL);
+ if (!imx378)
+ return -ENOMEM;
+
+ imx378->dev = dev;
+
+ imx378->xclk = devm_clk_get(dev, NULL);
+ if (IS_ERR(imx378->xclk)) {
+ dev_err(dev, "could not get xclk");
+ return PTR_ERR(imx378->xclk);
+ }
+
+ ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ);
+ if (ret) {
+ dev_err(dev, "could not set xclk frequency\n");
+ return ret;
+ }
+
+ ret = imx378_get_regulators(dev, imx378);
+ if (ret < 0) {
+ dev_err(dev, "could not get regulators\n");
+ return ret;
+ }
+
+ imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(imx378->enable_gpio)) {
+ dev_err(dev, "could get enable gpio\n");
+ return PTR_ERR(imx378->enable_gpio);
+ }
+
+ imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config);
+ if (IS_ERR(imx378->regmap)) {
+ dev_err(dev, "regmap init failed\n");
+ return PTR_ERR(imx378->regmap);
+ }
+
+ v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops);
+
+ imx378_power_on(imx378->dev);
+
+ pm_runtime_set_active(imx378->dev);
+ pm_runtime_enable(imx378->dev);
+ pm_runtime_idle(imx378->dev);
+
+ ret = imx378_init_controls(imx378);
+ if (ret) {
+ dev_err(dev, "failed to init controls: %d", ret);
+ goto error_probe;
+ }
+
+ imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ imx378->pad.flags = MEDIA_PAD_FL_SOURCE;
+ imx378->sd.dev = &client->dev;
+ imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+ ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad);
+ if (ret < 0) {
+ dev_err(dev, "could not register media entity\n");
+ goto free_ctrl;
+ }
+
+ imx378_entity_init_cfg(&imx378->sd, NULL);
+
+ ret = v4l2_async_register_subdev_sensor_common(&imx378->sd);
+ if (ret < 0) {
+ dev_err(dev, "could not register v4l2 device\n");
+ goto free_entity;
+ }
+
+ ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd);
+ if (ret < 0) {
+ dev_err(dev, "could not register v4l2 sd\n");
+ goto free_entity;
+ }
+
+ ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev);
+ if (ret) {
+ dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+
+free_entity:
+ media_entity_cleanup(&imx378->sd.entity);
+free_ctrl:
+ v4l2_ctrl_handler_free(&imx378->ctrls);
+error_probe:
+ mutex_destroy(&imx378->mutex);
+ pm_runtime_disable(imx378->dev);
+
+ return ret;
+}
+
+static int imx378_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct imx378 *imx378 = to_imx378(sd);
+
+ v4l2_async_unregister_subdev(&imx378->sd);
+ media_entity_cleanup(&imx378->sd.entity);
+ v4l2_ctrl_handler_free(&imx378->ctrls);
+
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+
+ mutex_destroy(&imx378->mutex);
+
+ return 0;
+}
+
+static const struct of_device_id imx378_of_match[] = {
+ { .compatible = "sony,imx378" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, imx378_of_match);
+
+static const struct i2c_device_id imx378_id[] = {
+ {"imx378", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, imx378_id);
+
+static const struct dev_pm_ops imx378_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(imx378_suspend, imx378_resume)
+ SET_RUNTIME_PM_OPS(imx378_power_off, imx378_power_on, NULL)
+};
+
+static struct i2c_driver imx378_i2c_driver = {
+ .driver = {
+ .of_match_table = imx378_of_match,
+ .pm = &imx378_pm_ops,
+ .name = "imx378",
+ },
+ .probe_new = imx378_probe,
+ .remove = imx378_remove,
+ .id_table = imx378_id,
+};
+
+module_i2c_driver(imx378_i2c_driver);
+
+MODULE_DESCRIPTION("Sony IMX378 Camera driver");
+MODULE_AUTHOR("Daniel Gomez <daniel@qtec.com>");
+MODULE_LICENSE("GPL v2");
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver
2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez
@ 2020-04-15 0:37 ` kbuild test robot
2020-04-28 21:02 ` Sakari Ailus
1 sibling, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2020-04-15 0:37 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6122 bytes --]
Hi Daniel,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.7-rc1 next-20200414]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Daniel-Gomez/v4l2-api-changes-for-imx378-driver/20200415-053427
base: git://linuxtv.org/media_tree.git master
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/media/i2c/imx378.c: In function 'imx378_probe':
>> drivers/media/i2c/imx378.c:1754:8: error: implicit declaration of function 'v4l2_device_register_subdev'; did you mean 'v4l2_async_register_subdev'? [-Werror=implicit-function-declaration]
1754 | ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
| v4l2_async_register_subdev
>> drivers/media/i2c/imx378.c:1754:43: error: 'struct imx378' has no member named 'v4l2_dev'
1754 | ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd);
| ^~
>> drivers/media/i2c/imx378.c:1760:8: error: implicit declaration of function 'v4l2_device_register_subdev_nodes'; did you mean 'v4l2_async_register_subdev'? [-Werror=implicit-function-declaration]
1760 | ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| v4l2_async_register_subdev
drivers/media/i2c/imx378.c:1760:49: error: 'struct imx378' has no member named 'v4l2_dev'
1760 | ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev);
| ^~
At top level:
drivers/media/i2c/imx378.c:577:12: warning: 'imx378_read_reg16' defined but not used [-Wunused-function]
577 | static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val)
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +1754 drivers/media/i2c/imx378.c
1674
1675 static int imx378_probe(struct i2c_client *client)
1676 {
1677 struct device *dev = &client->dev;
1678 struct imx378 *imx378;
1679 int ret;
1680
1681 ret = imx378_parse_fwnode(dev);
1682 if (ret)
1683 return ret;
1684
1685 imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL);
1686 if (!imx378)
1687 return -ENOMEM;
1688
1689 imx378->dev = dev;
1690
1691 imx378->xclk = devm_clk_get(dev, NULL);
1692 if (IS_ERR(imx378->xclk)) {
1693 dev_err(dev, "could not get xclk");
1694 return PTR_ERR(imx378->xclk);
1695 }
1696
1697 ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ);
1698 if (ret) {
1699 dev_err(dev, "could not set xclk frequency\n");
1700 return ret;
1701 }
1702
1703 ret = imx378_get_regulators(dev, imx378);
1704 if (ret < 0) {
1705 dev_err(dev, "could not get regulators\n");
1706 return ret;
1707 }
1708
1709 imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
1710 if (IS_ERR(imx378->enable_gpio)) {
1711 dev_err(dev, "could get enable gpio\n");
1712 return PTR_ERR(imx378->enable_gpio);
1713 }
1714
1715 imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config);
1716 if (IS_ERR(imx378->regmap)) {
1717 dev_err(dev, "regmap init failed\n");
1718 return PTR_ERR(imx378->regmap);
1719 }
1720
1721 v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops);
1722
1723 imx378_power_on(imx378->dev);
1724
1725 pm_runtime_set_active(imx378->dev);
1726 pm_runtime_enable(imx378->dev);
1727 pm_runtime_idle(imx378->dev);
1728
1729 ret = imx378_init_controls(imx378);
1730 if (ret) {
1731 dev_err(dev, "failed to init controls: %d", ret);
1732 goto error_probe;
1733 }
1734
1735 imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
1736 imx378->pad.flags = MEDIA_PAD_FL_SOURCE;
1737 imx378->sd.dev = &client->dev;
1738 imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
1739
1740 ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad);
1741 if (ret < 0) {
1742 dev_err(dev, "could not register media entity\n");
1743 goto free_ctrl;
1744 }
1745
1746 imx378_entity_init_cfg(&imx378->sd, NULL);
1747
1748 ret = v4l2_async_register_subdev_sensor_common(&imx378->sd);
1749 if (ret < 0) {
1750 dev_err(dev, "could not register v4l2 device\n");
1751 goto free_entity;
1752 }
1753
> 1754 ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd);
1755 if (ret < 0) {
1756 dev_err(dev, "could not register v4l2 sd\n");
1757 goto free_entity;
1758 }
1759
> 1760 ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev);
1761 if (ret) {
1762 dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n",
1763 ret);
1764 return ret;
1765 }
1766
1767 return 0;
1768
1769 free_entity:
1770 media_entity_cleanup(&imx378->sd.entity);
1771 free_ctrl:
1772 v4l2_ctrl_handler_free(&imx378->ctrls);
1773 error_probe:
1774 mutex_destroy(&imx378->mutex);
1775 pm_runtime_disable(imx378->dev);
1776
1777 return ret;
1778 }
1779
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 61726 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature
2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez
@ 2020-04-28 20:15 ` Sakari Ailus
0 siblings, 0 replies; 15+ messages in thread
From: Sakari Ailus @ 2020-04-28 20:15 UTC (permalink / raw)
To: Daniel Gomez; +Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Daniel,
On Tue, Apr 14, 2020 at 10:01:50PM +0200, Daniel Gomez wrote:
> Add a v4l2 control ID to handle the temperature.
>
> Signed-off-by: Daniel Gomez <daniel@qtec.com>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++
> include/uapi/linux/v4l2-controls.h | 4 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 93d33d1db4e8..17b93111baa8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers";
> case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component";
> case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr";
> + case V4L2_CID_TEMPERATURE: return "Temperature";
What's the unit of this control? I think it should have one.
As Hans pointed out, documentation is needed.
>
> /* Codec controls */
> /* The MPEG controls are applicable to all codec controls
> @@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> *type = V4L2_CTRL_TYPE_INTEGER;
> *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> break;
> + case V4L2_CID_TEMPERATURE:
> + *type = V4L2_CTRL_TYPE_INTEGER;
> + *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> + break;
> case V4L2_CID_MPEG_VIDEO_DEC_PTS:
> *type = V4L2_CTRL_TYPE_INTEGER64;
> *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 1a58d7cc4ccc..76ec0a6da8d5 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -143,8 +143,10 @@ enum v4l2_colorfx {
> #define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41)
> #define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42)
>
> +#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43)
> +
> /* last CID + 1 */
> -#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43)
> +#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44)
>
> /* USER-class private control IDs */
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver
2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez
2020-04-15 0:37 ` kbuild test robot
@ 2020-04-28 21:02 ` Sakari Ailus
1 sibling, 0 replies; 15+ messages in thread
From: Sakari Ailus @ 2020-04-28 21:02 UTC (permalink / raw)
To: Daniel Gomez; +Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Daniel,
On Tue, Apr 14, 2020 at 10:01:51PM +0200, Daniel Gomez wrote:
> Add a V4L2 sub-device driver for the Sony IMX378 image sensor.
> This is a camera sensor using the I2C bus for control and the
> CSI-2 bus for data.
>
> Signed-off-by: Daniel Gomez <daniel@qtec.com>
> ---
> drivers/media/i2c/Kconfig | 11 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 1841 insertions(+)
> create mode 100644 drivers/media/i2c/imx378.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index efd12bf4f8eb..8ea630275faf 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -796,6 +796,17 @@ config VIDEO_IMX355
> To compile this driver as a module, choose M here: the
> module will be called imx355.
>
> +config VIDEO_IMX378
> + tristate "Sony IMX378 sensor support"
> + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on V4L2_FWNODE
You'll need to use select on some of these things nowadays. Please see the
current Kconfig in the media tree master.
> + help
> + This is a Video4Linux2 sensor driver for the Sony
> + IMX378 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx378.
> +
> config VIDEO_OV2640
> tristate "OmniVision OV2640 sensor support"
> depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 77bf7d0b691f..dff1c9ec444f 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o
> obj-$(CONFIG_VIDEO_IMX290) += imx290.o
> obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> +obj-$(CONFIG_VIDEO_IMX378) += imx378.o
> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>
> obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/imx378.c b/drivers/media/i2c/imx378.c
> new file mode 100644
> index 000000000000..6e22c3b45f72
> --- /dev/null
> +++ b/drivers/media/i2c/imx378.c
> @@ -0,0 +1,1829 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * imx378.c - imx378 sensor driver
> + *
> + * Copyright 2019 Qtechnology A/S
> + *
> + * Daniel Gomez <daniel@qtec.com>
> + *
> + * Based on imx214.c and imx355.c
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define IMX378_DEFAULT_CLK_FREQ 24000000
> +#define IMX378_DEFAULT_LINK_FREQ 480000000
> +#define IMX378_DEFAULT_LINK_MFREQ (IMX378_DEFAULT_LINK_FREQ / 1000000)
> +#define IMX378_DEFAULT_PIXEL_RATE ((IMX378_DEFAULT_LINK_FREQ * 8LL) / 10)
> +#define IMX378_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
> +#define IMX378_DEFAULT_FIVAL {1, 30}
> +#define IMX378_LINE_LENGTH 4444
> +#define IMX378_MIN_WIDTH 4
> +#define IMX378_MIN_HEIGHT 4
> +#define IMX378_MAX_DEFAULT_WIDTH 4032
> +#define IMX378_MAX_DEFAULT_HEIGHT 3040
> +#define IMX378_MAX_BOUNDS_WIDTH 4032
> +#define IMX378_MAX_BOUNDS_HEIGHT 3104
> +#define IMX378_CID_CUSTOM_BASE (V4L2_CID_USER_BASE | 0xf000)
> +#define IMX378_CID_GREEN_BALANCE (IMX378_CID_CUSTOM_BASE + 0)
> +
> +static const char * const imx378_supply_name[] = {
> + "vdda",
> + "vddd",
> + "vdddo",
> +};
> +
> +#define IMX378_NUM_SUPPLIES ARRAY_SIZE(imx378_supply_name)
> +
> +struct imx378 {
> + struct device *dev;
> + struct clk *xclk;
> + struct regmap *regmap;
> +
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct v4l2_mbus_framefmt fmt;
> + struct v4l2_rect crop;
> +
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *pixel_rate;
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *hflip;
> + struct v4l2_ctrl *analogue_gain;
> + struct v4l2_ctrl *digital_gain;
> + struct v4l2_ctrl *red_balance;
> + struct v4l2_ctrl *blue_balance;
> + struct v4l2_ctrl *green_balance;
> + struct v4l2_ctrl *temperature;
> +
> + struct regulator_bulk_data supplies[IMX378_NUM_SUPPLIES];
> +
> + struct gpio_desc *enable_gpio;
> +
> + struct mutex mutex;
> +
> + struct v4l2_fract fival;
> +
> + bool streaming;
> +};
> +
> +struct reg_8 {
> + u16 addr;
> + u8 val;
> +};
> +
> +enum {
> + IMX378_TABLE_WAIT_MS = 0,
> + IMX378_TABLE_END,
> + IMX378_MAX_RETRIES,
> + IMX378_WAIT_MS
> +};
> +
> +static const struct reg_8 mode_table_common[] = {
> + /* software standby settings */
> + {0x0100, 0x00},
> +
> + /* software reset */
> + {0x0103, 0x01},
> +
> + /* CSI */
> + {0x0112, 0x0A},
> + {0x0113, 0x0A},
> + {0x0114, 0x03},
> +
> + /* global settings */
> + {0x0136, 0x18},
> + {0x0137, 0x00},
> +
> + {0x0202, 0x20},
> + {0x0203, 0xa0},
> + {0x0204, 0x03},
> + {0x0205, 0xd2},
> + {0x020e, 0x01},
> + {0x020f, 0x00},
> + {0x0210, 0x02},
> + {0x0211, 0x21},
> + {0x0212, 0x01},
> + {0x0213, 0xee},
> + {0x0214, 0x01},
> + {0x0215, 0x00},
> + {0x0220, 0x00},
> + {0x0221, 0x11},
> +
> + {0x0301, 0x05},
> + {0x0303, 0x02},
> + {0x0305, 0x03},
> + {0x0306, 0x00},
> + {0x0307, 0x96},
> +
> + {0x0309, 0x0a},
> + {0x030b, 0x01},
> + {0x030d, 0x02},
> + {0x030e, 0x00},
> + {0x030f, 0xf6},
> + {0x0310, 0x00},
> + {0x0340, 0x04},
> + {0x0341, 0xb0},
> + {0x0342, 0x20},
> + {0x0343, 0x08},
> + {0x0344, 0x00},
> + {0x0345, 0x00},
> + {0x0346, 0x01},
> + {0x0347, 0x78},
> + {0x0348, 0x0f},
> + {0x0349, 0xd7},
> + {0x0350, 0x01},
> + {0x034a, 0x0a},
> + {0x034b, 0x67},
> + {0x034c, 0x07},
> + {0x034d, 0x80},
> + {0x034e, 0x04},
> + {0x034f, 0x38},
> + {0x0381, 0x01},
> + {0x0383, 0x01},
> + {0x0385, 0x01},
> + {0x0387, 0x01},
> +
> + {0x0401, 0x00},
> + {0x0404, 0x00},
> + {0x0405, 0x10},
> + {0x0408, 0x00},
> + {0x0409, 0x6c},
> + {0x040a, 0x00},
> + {0x040b, 0x40},
> + {0x040c, 0x07},
> + {0x040d, 0x80},
> + {0x040e, 0x04},
> + {0x040f, 0x38},
> +
> + {0x0900, 0x00},
> + {0x0901, 0x11},
> + {0x0902, 0x02},
> +
> + {0x0c1a, 0x01},
> + {0x0c1b, 0x01},
> +
> + {0x3030, 0x01},
> + {0x3032, 0x00},
> + {0x3033, 0x40},
> + {0x3140, 0x02},
> + {0x38a8, 0x1f},
> + {0x38a9, 0xff},
> + {0x38aa, 0x1f},
> + {0x38ab, 0xff},
> + {0x3c00, 0x00},
> + {0x3c01, 0x03},
> + {0x3c02, 0xdc},
> + {0x3d8a, 0x01},
> + {0x3e20, 0x01},
> + {0x3e37, 0x01},
> + {0x3f0d, 0x00},
> + {0x3f50, 0x00},
> + {0x3f56, 0x00},
> + {0x3f57, 0x01},
> + {0x3ff9, 0x00},
> +
> + {0x4421, 0x08},
> + {0x4ae9, 0x18},
> + {0x4ae9, 0x21},
> + {0x4aea, 0x08},
> + {0x4aea, 0x80},
> +
> + {0x55d4, 0x00},
> + {0x55d5, 0x00},
> + {0x55d6, 0x07},
> + {0x55d7, 0xff},
> + {0x55e8, 0x07},
> + {0x55e9, 0xff},
> + {0x55ea, 0x00},
> + {0x55eb, 0x00},
> + {0x5748, 0x07},
> + {0x5749, 0xff},
> + {0x574a, 0x00},
> + {0x574b, 0x00},
> + {0x574c, 0x07},
> + {0x574d, 0xff},
> + {0x574e, 0x00},
> + {0x574f, 0x00},
> + {0x5754, 0x00},
> + {0x5755, 0x00},
> + {0x5756, 0x07},
> + {0x5757, 0xff},
> + {0x5973, 0x04},
> + {0x5974, 0x01},
> + {0x5d13, 0xc3},
> + {0x5d14, 0x58},
> + {0x5d15, 0xa3},
> + {0x5d16, 0x1d},
> + {0x5d17, 0x65},
> + {0x5d18, 0x8c},
> + {0x5d1a, 0x06},
> + {0x5d1b, 0xa9},
> + {0x5d1c, 0x45},
> + {0x5d1d, 0x3a},
> + {0x5d1e, 0xab},
> + {0x5d1f, 0x15},
> + {0x5d21, 0x0e},
> + {0x5d22, 0x52},
> + {0x5d23, 0xaa},
> + {0x5d24, 0x7d},
> + {0x5d25, 0x57},
> + {0x5d26, 0xa8},
> + {0x5d37, 0x5a},
> + {0x5d38, 0x5a},
> + {0x5d77, 0x7f},
> +
> + {0x7b3b, 0x01},
> + {0x7b4c, 0x00},
> + {0x7b53, 0x01},
> + {0x7b75, 0x0e},
> + {0x7b76, 0x0b},
> + {0x7b77, 0x08},
> + {0x7b78, 0x0a},
> + {0x7b79, 0x47},
> + {0x7b7c, 0x00},
> + {0x7b7d, 0x00},
> +
> + {0x8d1f, 0x00},
> + {0x8d27, 0x00},
> +
> + {0x9004, 0x03},
> + {0x9200, 0x50},
> + {0x9201, 0x6c},
> + {0x9202, 0x71},
> + {0x9203, 0x00},
> + {0x9204, 0x71},
> + {0x9205, 0x01},
> + {0x9304, 0x03},
> + {0x9305, 0x00},
> + {0x9369, 0x5a},
> + {0x936b, 0x55},
> + {0x936d, 0x28},
> + {0x9371, 0x6a},
> + {0x9373, 0x6a},
> + {0x9375, 0x64},
> + {0x9905, 0x00},
> + {0x9907, 0x00},
> + {0x9909, 0x00},
> + {0x990b, 0x00},
> + {0x991a, 0x00},
> + {0x9944, 0x3c},
> + {0x9947, 0x3c},
> + {0x994a, 0x8c},
> + {0x994b, 0x50},
> + {0x994c, 0x1b},
> + {0x994d, 0x8c},
> + {0x994e, 0x50},
> + {0x994f, 0x1b},
> + {0x9950, 0x8c},
> + {0x9951, 0x1b},
> + {0x9952, 0x0a},
> + {0x9953, 0x8c},
> + {0x9954, 0x1b},
> + {0x9955, 0x0a},
> + {0x996b, 0x8c},
> + {0x996c, 0x64},
> + {0x996d, 0x50},
> + {0x9a13, 0x04},
> + {0x9a14, 0x04},
> + {0x9a19, 0x00},
> + {0x9a1c, 0x04},
> + {0x9a1d, 0x04},
> + {0x9a26, 0x05},
> + {0x9a27, 0x05},
> + {0x9a2c, 0x01},
> + {0x9a2d, 0x03},
> + {0x9a2f, 0x05},
> + {0x9a30, 0x05},
> + {0x9a41, 0x00},
> + {0x9a46, 0x00},
> + {0x9a47, 0x00},
> + {0x9a4c, 0x0d},
> + {0x9a4d, 0x0d},
> + {0x9c17, 0x35},
> + {0x9c1d, 0x31},
> + {0x9c29, 0x50},
> + {0x9c3b, 0x2f},
> + {0x9c41, 0x6b},
> + {0x9c47, 0x2d},
> + {0x9c4d, 0x40},
> + {0x9c6b, 0x00},
> + {0x9c71, 0xc8},
> + {0x9c73, 0x32},
> + {0x9c75, 0x04},
> + {0x9c7d, 0x2d},
> + {0x9c83, 0x40},
> + {0x9c94, 0x3f},
> + {0x9c95, 0x3f},
> + {0x9c96, 0x3f},
> + {0x9c97, 0x00},
> + {0x9c98, 0x00},
> + {0x9c99, 0x00},
> + {0x9c9a, 0x3f},
> + {0x9c9b, 0x3f},
> + {0x9c9c, 0x3f},
> + {0x9ca0, 0x0f},
> + {0x9ca1, 0x0f},
> + {0x9ca2, 0x0f},
> + {0x9ca3, 0x00},
> + {0x9ca4, 0x00},
> + {0x9ca5, 0x00},
> + {0x9ca6, 0x1e},
> + {0x9ca7, 0x1e},
> + {0x9ca8, 0x1e},
> + {0x9ca9, 0x00},
> + {0x9caa, 0x00},
> + {0x9cab, 0x00},
> + {0x9cac, 0x09},
> + {0x9cad, 0x09},
> + {0x9cae, 0x09},
> + {0x9cbd, 0x50},
> + {0x9cbf, 0x50},
> + {0x9cc1, 0x50},
> + {0x9cc3, 0x40},
> + {0x9cc5, 0x40},
> + {0x9cc7, 0x40},
> + {0x9cc9, 0x0a},
> + {0x9ccb, 0x0a},
> + {0x9ccd, 0x0a},
> + {0x9d17, 0x35},
> + {0x9d1d, 0x31},
> + {0x9d29, 0x50},
> + {0x9d3b, 0x2f},
> + {0x9d41, 0x6b},
> + {0x9d47, 0x42},
> + {0x9d4d, 0x5a},
> + {0x9d6b, 0x00},
> + {0x9d71, 0xc8},
> + {0x9d73, 0x32},
> + {0x9d75, 0x04},
> + {0x9d7d, 0x42},
> + {0x9d83, 0x5a},
> + {0x9d94, 0x3f},
> + {0x9d95, 0x3f},
> + {0x9d96, 0x3f},
> + {0x9d97, 0x00},
> + {0x9d98, 0x00},
> + {0x9d99, 0x00},
> + {0x9d9a, 0x3f},
> + {0x9d9b, 0x3f},
> + {0x9d9c, 0x3f},
> + {0x9d9d, 0x1f},
> + {0x9d9e, 0x1f},
> + {0x9d9f, 0x1f},
> + {0x9da0, 0x0f},
> + {0x9da1, 0x0f},
> + {0x9da2, 0x0f},
> + {0x9da3, 0x00},
> + {0x9da4, 0x00},
> + {0x9da5, 0x00},
> + {0x9da6, 0x1e},
> + {0x9da7, 0x1e},
> + {0x9da8, 0x1e},
> + {0x9da9, 0x00},
> + {0x9daa, 0x00},
> + {0x9dab, 0x00},
> + {0x9dac, 0x09},
> + {0x9dad, 0x09},
> + {0x9dae, 0x09},
> + {0x9dc9, 0x0a},
> + {0x9dcb, 0x0a},
> + {0x9dcd, 0x0a},
> + {0x9e17, 0x35},
> + {0x9e1d, 0x31},
> + {0x9e29, 0x50},
> + {0x9e3b, 0x2f},
> + {0x9e41, 0x6b},
> + {0x9e47, 0x2d},
> + {0x9e4d, 0x40},
> + {0x9e6b, 0x00},
> + {0x9e71, 0xc8},
> + {0x9e73, 0x32},
> + {0x9e75, 0x04},
> + {0x9e94, 0x0f},
> + {0x9e95, 0x0f},
> + {0x9e96, 0x0f},
> + {0x9e97, 0x00},
> + {0x9e98, 0x00},
> + {0x9e99, 0x00},
> + {0x9e9a, 0x2f},
> + {0x9e9b, 0x2f},
> + {0x9e9c, 0x2f},
> + {0x9e9d, 0x00},
> + {0x9e9e, 0x00},
> + {0x9e9f, 0x00},
> + {0x9ea0, 0x0f},
> + {0x9ea1, 0x0f},
> + {0x9ea2, 0x0f},
> + {0x9ea3, 0x00},
> + {0x9ea4, 0x00},
> + {0x9ea5, 0x00},
> + {0x9ea6, 0x3f},
> + {0x9ea7, 0x3f},
> + {0x9ea8, 0x3f},
> + {0x9ea9, 0x00},
> + {0x9eaa, 0x00},
> + {0x9eab, 0x00},
> + {0x9eac, 0x09},
> + {0x9ead, 0x09},
> + {0x9eae, 0x09},
> + {0x9ec9, 0x0a},
> + {0x9ecb, 0x0a},
> + {0x9ecd, 0x0a},
> + {0x9f17, 0x35},
> + {0x9f1d, 0x31},
> + {0x9f29, 0x50},
> + {0x9f3b, 0x2f},
> + {0x9f41, 0x6b},
> + {0x9f47, 0x42},
> + {0x9f4d, 0x5a},
> + {0x9f6b, 0x00},
> + {0x9f71, 0xc8},
> + {0x9f73, 0x32},
> + {0x9f75, 0x04},
> + {0x9f94, 0x0f},
> + {0x9f95, 0x0f},
> + {0x9f96, 0x0f},
> + {0x9f97, 0x00},
> + {0x9f98, 0x00},
> + {0x9f99, 0x00},
> + {0x9f9a, 0x2f},
> + {0x9f9b, 0x2f},
> + {0x9f9c, 0x2f},
> + {0x9f9d, 0x00},
> + {0x9f9e, 0x00},
> + {0x9f9f, 0x00},
> + {0x9fa0, 0x0f},
> + {0x9fa1, 0x0f},
> + {0x9fa2, 0x0f},
> + {0x9fa3, 0x00},
> + {0x9fa4, 0x00},
> + {0x9fa5, 0x00},
> + {0x9fa6, 0x1e},
> + {0x9fa7, 0x1e},
> + {0x9fa8, 0x1e},
> + {0x9fa9, 0x00},
> + {0x9faa, 0x00},
> + {0x9fab, 0x00},
> + {0x9fac, 0x09},
> + {0x9fad, 0x09},
> + {0x9fae, 0x09},
> + {0x9fc9, 0x0a},
> + {0x9fcb, 0x0a},
> + {0x9fcd, 0x0a},
> +
> + {0xa001, 0x0a},
> + {0xa003, 0x0a},
> + {0xa005, 0x0a},
> + {0xa006, 0x01},
> + {0xa007, 0xc0},
> + {0xa009, 0xc0},
> + {0xa14b, 0xff},
> + {0xa151, 0x0c},
> + {0xa153, 0x50},
> + {0xa155, 0x02},
> + {0xa157, 0x00},
> + {0xa1ad, 0xff},
> + {0xa1b3, 0x0c},
> + {0xa1b5, 0x50},
> + {0xa1b9, 0x00},
> + {0xa24b, 0xff},
> + {0xa257, 0x00},
> + {0xa2a9, 0x60},
> + {0xa2ad, 0xff},
> + {0xa2b7, 0x00},
> + {0xa2b9, 0x00},
> +
> + {0xb21f, 0x04},
> + {0xb35c, 0x00},
> + {0xb35e, 0x08},
> + {0xbcf1, 0x02},
> + {0xe000, 0x00},
> + {0xf61c, 0x04},
> + {0xf61e, 0x04},
> +
> + {IMX378_TABLE_END, 0x00}
> +};
> +
> +static inline struct imx378 *to_imx378(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct imx378, sd);
> +}
> +
> +static int imx378_set_flip_mode(struct imx378 *imx378, u32 code)
> +{
> + /* WORKAROUND to update controls */
> + imx378->vflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY;
> + imx378->hflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY;
> +
> + switch (code) {
> + case MEDIA_BUS_FMT_SBGGR10_1X10:
> + v4l2_ctrl_s_ctrl(imx378->vflip, 1);
> + v4l2_ctrl_s_ctrl(imx378->hflip, 1);
Other raw sensor drivers which support flip controls change the format
based on those controls, not the other way around. Could you do the same in
this one, please?
> + break;
> + case MEDIA_BUS_FMT_SGBRG10_1X10:
> + v4l2_ctrl_s_ctrl(imx378->vflip, 1);
> + v4l2_ctrl_s_ctrl(imx378->hflip, 0);
> + break;
> + case MEDIA_BUS_FMT_SGRBG10_1X10:
> + v4l2_ctrl_s_ctrl(imx378->vflip, 0);
> + v4l2_ctrl_s_ctrl(imx378->hflip, 1);
> + break;
> + case MEDIA_BUS_FMT_SRGGB10_1X10:
> + v4l2_ctrl_s_ctrl(imx378->vflip, 0);
> + v4l2_ctrl_s_ctrl(imx378->hflip, 0);
> + break;
> + default:
> + break;
> + }
> +
> + /* WORKAROUND to update controls */
> + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + return 0;
> +}
> +
> +static u32 imx378_get_format_code(struct imx378 *imx378)
> +{
> + /*
> + * Only one bayer order is supported.
> + * It depends on the flip settings.
> + */
> + u32 code;
> + static const u32 codes[2][2] = {
> + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, },
> + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, },
> + };
> +
> + lockdep_assert_held(&imx378->mutex);
> + code = codes[imx378->vflip->val][imx378->hflip->val];
> +
> + return code;
> +}
> +
> +static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val)
> +{
> + int ret;
> + u32 h_val = 0, l_val = 0;
> +
> + ret = regmap_read(imx378->regmap, addr + 1, &l_val);
Is there a reason to read just 8 bits at a time? Same for writing.
> + if (ret) {
> + pr_err("%s:i2c read failed, %x\n", __func__, addr + 1);
> + return ret;
> + }
> +
> + *val = l_val & 0xFF;
> +
> + ret = regmap_read(imx378->regmap, addr, &h_val);
> + if (ret) {
> + pr_err("%s:i2c read failed, %x\n", __func__, addr);
> + return ret;
> + }
> +
> + *val |= (h_val & 0xFF) << 8;
> +
> + return ret;
> +}
> +
> +static int imx378_write_reg16(struct imx378 *imx378, u16 addr, u16 val)
> +{
> + int ret;
> +
> + ret = regmap_write(imx378->regmap, addr + 1, val);
> + if (ret) {
> + pr_err("%s:i2c write failed, %x = %x\n",
> + __func__, addr + 1, val);
> + return ret;
> + }
> +
> + ret = regmap_write(imx378->regmap, addr, val >> 8);
> + if (ret) {
> + pr_err("%s:i2c write failed, %x = %x\n",
> + __func__, addr, val >> 8);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int __maybe_unused imx378_power_on(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx378 *imx378 = to_imx378(sd);
> + int ret;
> +
> + gpiod_set_value_cansleep(imx378->enable_gpio, 0);
> +
> + ret = regulator_bulk_enable(IMX378_NUM_SUPPLIES, imx378->supplies);
> + if (ret < 0) {
> + dev_err(imx378->dev, "failed to enable regulators: %d\n", ret);
> + return ret;
> + }
> +
> + usleep_range(2000, 3000);
> +
> + ret = clk_prepare_enable(imx378->xclk);
> + if (ret < 0) {
> + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies);
> + dev_err(imx378->dev, "clk prepare enable failed\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(imx378->enable_gpio, 1);
> + usleep_range(12000, 15000);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx378_power_off(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + gpiod_set_value_cansleep(imx378->enable_gpio, 0);
> +
> + clk_disable_unprepare(imx378->xclk);
> +
> + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies);
> + usleep_range(10, 20);
> +
> + return 0;
> +}
> +
> +static int imx378_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct imx378 *imx378 = container_of(sd, struct imx378, sd);
> +
> + if (code->index != 0 || code->pad != 0)
> + return -EINVAL;
> +
> + code->code = imx378_get_format_code(imx378);
> +
> + return 0;
> +}
> +
> +static int imx378_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct imx378 *imx378 = container_of(sd, struct imx378, sd);
> +
> + if (fse->index != 0)
> + return -EINVAL;
> +
> + mutex_lock(&imx378->mutex);
> + if (fse->code != imx378_get_format_code(imx378)) {
> + mutex_unlock(&imx378->mutex);
> + return -EINVAL;
> + }
> + mutex_unlock(&imx378->mutex);
> +
> + fse->min_width = IMX378_MIN_WIDTH;
> + fse->max_width = IMX378_MAX_BOUNDS_WIDTH;
> + fse->min_height = IMX378_MIN_HEIGHT;
> + fse->max_height = IMX378_MAX_BOUNDS_HEIGHT;
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int imx378_s_register(struct v4l2_subdev *sd,
> + const struct v4l2_dbg_register *reg)
> +{
> + struct imx378 *imx378 = container_of(sd, struct imx378, sd);
> +
> + return regmap_write(imx378->regmap, reg->reg, reg->val);
> +}
> +
> +static int imx378_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct imx378 *imx378 = container_of(sd, struct imx378, sd);
> + unsigned int aux;
> + int ret;
> +
> + reg->size = 1;
> + ret = regmap_read(imx378->regmap, reg->reg, &aux);
> + reg->val = aux;
> +
> + return ret;
> +}
> +#endif
> +
> +static const struct v4l2_subdev_core_ops imx378_core_ops = {
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> + .g_register = imx378_g_register,
> + .s_register = imx378_s_register,
> +#endif
> +};
> +
> +static int imx378_set_img_size(struct imx378 *imx378)
> +{
> + u32 frame_length;
> + int ret;
> +
> + ret = imx378_write_reg16(imx378, 0x34c, imx378->fmt.width);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x34e, imx378->fmt.height);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x40c, imx378->fmt.width);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x40e, imx378->fmt.height);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x342, IMX378_LINE_LENGTH);
> + if (ret < 0)
> + goto error;
> +
> + frame_length = IMX378_DEFAULT_LINK_FREQ /
> + ((imx378->fival.denominator / imx378->fival.numerator)
> + * IMX378_LINE_LENGTH);
Do you ensure somewhere you won't be dividing by zero here?
> +
> + ret = imx378_write_reg16(imx378, 0x340, frame_length);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x344, imx378->crop.left);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x348, imx378->crop.left +
> + imx378->fmt.width - 1);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x346, imx378->crop.top);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x34a, imx378->crop.top +
> + imx378->fmt.height - 1);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x408, 0);
> + if (ret < 0)
> + goto error;
> +
> + ret = imx378_write_reg16(imx378, 0x40a, 0);
> + if (ret < 0)
> + goto error;
> +
> + ret = regmap_write(imx378->regmap, 0xb04, 1);
> + if (ret < 0)
> + goto error;
> +
> + return ret;
> +
> +error:
> + dev_err(imx378->dev, "could not set image size %d\n", ret);
> + return ret;
> +}
> +
> +static struct v4l2_mbus_framefmt *
> +__imx378_get_pad_format(struct imx378 *imx378,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad,
> + enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(&imx378->sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &imx378->fmt;
> + default:
> + return NULL;
> + }
> +}
> +
> +static int imx378_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + mutex_lock(&imx378->mutex);
> + fmt->format = *__imx378_get_pad_format(imx378, cfg, fmt->pad,
> + fmt->which);
> + mutex_unlock(&imx378->mutex);
> +
> + return 0;
> +}
> +
> +static struct v4l2_rect *
> +__imx378_get_pad_crop(struct imx378 *imx378, struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_crop(&imx378->sd, cfg, pad);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &imx378->crop;
> + default:
> + return NULL;
> + }
> +}
> +
> +static int imx378_min_interval(struct imx378 *imx378,
> + struct v4l2_fract *fival)
> +{
> + fival->numerator = IMX378_LINE_LENGTH * (imx378->fmt.height);
> + fival->denominator = IMX378_DEFAULT_LINK_FREQ;
> +
> + return 0;
> +}
> +
> +static int imx378_max_interval(struct v4l2_fract *fival)
> +{
> + fival->numerator = IMX378_LINE_LENGTH * 0xffff;
> + fival->denominator = IMX378_DEFAULT_LINK_FREQ;
> +
> + return 0;
> +}
> +
> +#define FRACT_CMP(a, OP, b) \
> + ((u64)(a).numerator * (b).denominator OP \
> + (u64)(b).numerator * (a).denominator)
> +static int imx378_clamp_interval(struct imx378 *imx378,
> + struct v4l2_fract *fival)
> +{
> + struct v4l2_fract interval;
> + int ret;
> +
> + ret = imx378_min_interval(imx378, &interval);
> + if (ret)
> + return ret;
> + if (FRACT_CMP(*fival, <, interval))
> + *fival = interval;
> +
> + ret = imx378_max_interval(&interval);
> + if (ret)
> + return ret;
> + if (FRACT_CMP(*fival, >, interval))
> + *fival = interval;
> +
> + return 0;
> +}
> +
> +static int imx378_g_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *fival)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + fival->interval = imx378->fival;
> +
> + return 0;
> +}
> +
> +static int imx378_s_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_frame_interval *fival)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> + u64 max_exposure;
> +
> + if (fival->interval.denominator == 0)
> + fival->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
> +
> + imx378_clamp_interval(imx378, &fival->interval);
> +
> + imx378->fival = fival->interval;
> +
> + /* Update exposure limits */
> + max_exposure = (IMX378_DEFAULT_LINK_FREQ /
> + ((imx378->fival.denominator / imx378->fival.numerator)
> + * IMX378_LINE_LENGTH)) - 20;
Ditto.
> +
> + __v4l2_ctrl_modify_range(imx378->exposure, imx378->exposure->minimum,
> + max_exposure, imx378->exposure->step,
> + max_exposure);
I believe you should be acquiring the control handler lock here, i.e. use
the locked variant. This function is called by the V4L2 subdev framework,
including through the IOCTL handler.
> +
> + return 0;
> +}
> +
> +static int
> +imx378_enum_frame_interval(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_interval_enum *fie)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> + int ret;
> +
> + if (fie->index != 0)
> + return -EINVAL;
> +
> + if (fie->width > IMX378_MAX_BOUNDS_WIDTH ||
> + fie->width < IMX378_MIN_WIDTH)
> + return -EINVAL;
> +
> + if (fie->height > IMX378_MAX_BOUNDS_HEIGHT ||
> + fie->height < IMX378_MIN_HEIGHT)
> + return -EINVAL;
The frame interval should be specific to a given size, i.e. you can return
an error if this is being enumerated for something else than an enumerated
size.
> +
> + mutex_lock(&imx378->mutex);
> + if (fie->code != imx378_get_format_code(imx378)) {
> + mutex_unlock(&imx378->mutex);
> + return -EINVAL;
> + }
> + mutex_unlock(&imx378->mutex);
> +
> + ret = imx378_min_interval(imx378, &fie->min_interval);
> + if (ret)
> + return ret;
> +
> + ret = imx378_max_interval(&fie->max_interval);
> + if (ret)
> + return ret;
> +
> + fie->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
> +
> + return 0;
> +}
> +
> +static int imx378_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> + struct v4l2_mbus_framefmt *__fmt;
> + struct v4l2_rect *__crop;
> + struct v4l2_subdev_frame_interval fival = { };
> +
> + mutex_lock(&imx378->mutex);
> +
> + if (imx378->streaming && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
> + return -EBUSY;
Oops!
> +
> + __crop = __imx378_get_pad_crop(imx378, cfg, fmt->pad, fmt->which);
> +
> + v4l_bound_align_image(&fmt->format.width, IMX378_MIN_WIDTH,
> + IMX378_MAX_BOUNDS_WIDTH, ilog2(4),
> + &fmt->format.height, IMX378_MIN_HEIGHT,
> + IMX378_MAX_BOUNDS_HEIGHT, ilog2(8), 0);
> +
> + __crop->width = fmt->format.width;
> + __crop->height = fmt->format.height;
> +
> + v4l_bound_align_image(&__crop->left, 0,
> + IMX378_MAX_BOUNDS_WIDTH - __crop->width, ilog2(4),
> + &__crop->top, 0,
> + IMX378_MAX_BOUNDS_HEIGHT - __crop->height,
> + ilog2(8), 0);
> +
> + __fmt = __imx378_get_pad_format(imx378, cfg, fmt->pad, fmt->which);
> +
> + __fmt->width = __crop->width;
> + __fmt->height = __crop->height;
> +
> + if (fmt->format.code && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
> + imx378_set_flip_mode(imx378, fmt->format.code);
> +
> + __fmt->code = imx378_get_format_code(imx378);
> +
> + __fmt->field = V4L2_FIELD_NONE;
> + __fmt->colorspace = V4L2_COLORSPACE_SRGB;
> + __fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace);
> + __fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> + __fmt->colorspace, __fmt->ycbcr_enc);
> + __fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__fmt->colorspace);
> +
> + fmt->format = *__fmt;
> +
> + /* Frame interval depends on the format so, update it accordingly */
> + if (fmt->which != V4L2_SUBDEV_FORMAT_TRY) {
> + fival.interval = imx378->fival;
> + imx378_s_frame_interval(sd, &fival);
> + }
> +
> + mutex_unlock(&imx378->mutex);
> +
> + return 0;
> +}
> +
> +static int imx378_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_selection *s)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_CROP:
> + case V4L2_SEL_TGT_COMPOSE:
> + s->r = imx378->crop;
s->which should be taken into account here, just as in set_fmt above.
> + return 0;
> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + s->r.width = IMX378_MAX_DEFAULT_WIDTH;
> + s->r.height = IMX378_MAX_DEFAULT_HEIGHT;
> + s->r.left = 0;
> + s->r.top = 0;
> + return 0;
> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + s->r.width = IMX378_MAX_BOUNDS_WIDTH;
> + s->r.height = IMX378_MAX_BOUNDS_HEIGHT;
> + s->r.left = 0;
> + s->r.top = 0;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int imx378_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_selection *s)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> + int ret;
> +
> + if (s->target == V4L2_SEL_TGT_COMPOSE)
> + return 0;
> +
> + if (s->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
> + mutex_lock(&imx378->mutex);
> +
> + s->r.width = imx378->crop.width;
> + s->r.height = imx378->crop.height;
> +
> + v4l_bound_align_image(&s->r.left, 0,
> + IMX378_MAX_BOUNDS_WIDTH - s->r.width, ilog2(4),
> + &s->r.top, 0,
> + IMX378_MAX_BOUNDS_HEIGHT - s->r.height, ilog2(8),
> + 0);
> +
> + imx378->crop.left = s->r.left;
> + imx378->crop.top = s->r.top;
> +
> + if (imx378->streaming) {
> + ret = imx378_set_img_size(imx378);
> + if (ret < 0) {
> + dev_err(imx378->dev, "could not set image size\n");
> + mutex_unlock(&imx378->mutex);
> + return ret;
> + }
> + }
> +
> + mutex_unlock(&imx378->mutex);
> + return 0;
> +}
> +
> +static int imx378_set_exposure(struct imx378 *imx378)
> +{
> + int ret;
> +
> + /* Set group hold */
> + ret = regmap_write(imx378->regmap, 0x104, 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx378_write_reg16(imx378, 0x202, imx378->exposure->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Release group hold */
> + return regmap_write(imx378->regmap, 0x104, 0);
> +}
> +
> +static int imx378_set_analogue_gain(struct imx378 *imx378)
> +{
> + int ret;
> +
> + /* Set group hold */
> + ret = regmap_write(imx378->regmap, 0x104, 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = imx378_write_reg16(imx378, 0x204, imx378->analogue_gain->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Release group hold */
> + return regmap_write(imx378->regmap, 0x104, 0);
> +}
> +
> +static int imx378_set_digital_gain_global(struct imx378 *imx378)
> +{
> + int ret;
> +
> + /* Set group hold */
> + ret = regmap_write(imx378->regmap, 0x104, 1);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain control mode all color */
> + ret = regmap_write(imx378->regmap, 0x3ff9, 1);
> + if (ret < 0)
> + return ret;
> +
> + /* Set digital gain globally */
> + ret = imx378_write_reg16(imx378, 0x20e, imx378->digital_gain->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Release group hold */
> + return regmap_write(imx378->regmap, 0x104, 0);
> +}
> +
> +static int imx378_set_digital_gain_by_color(struct imx378 *imx378)
> +{
> + int ret;
> +
> + /* Set group hold */
> + ret = regmap_write(imx378->regmap, 0x104, 1);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain control mode by color */
> + ret = regmap_write(imx378->regmap, 0x3ff9, 0);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain R */
> + ret = imx378_write_reg16(imx378, 0x210, imx378->red_balance->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain B */
> + ret = imx378_write_reg16(imx378, 0x212, imx378->blue_balance->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain GR */
> + ret = imx378_write_reg16(imx378, 0x20e, imx378->green_balance->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Digital gain GB */
> + ret = imx378_write_reg16(imx378, 0x214, imx378->green_balance->val);
> + if (ret < 0)
> + return ret;
> +
> + /* Release group hold */
> + return regmap_write(imx378->regmap, 0x104, 0);
> +}
> +
> +static int imx378_g_temperature(struct imx378 *imx378, s32 *temperature)
> +{
> + unsigned int aux;
> + int ret;
> +
> + if (imx378->streaming) {
> + ret = regmap_read(imx378->regmap, 0x13a, &aux);
> + if (ret < 0)
> + return ret;
> +
> + if (aux >= 0xec)
> + *temperature = (aux - 0xec - 20) * 1000;
> + else
> + *temperature = (aux * 1000);
> + }
> +
> + dev_err(imx378->dev,
> + "Sensor is powered off, returning invalid temperature\n");
You could simply return an error here. No kernel messages should be emitted
unless there's a non-uAPI related problem.
> +
> + return 0;
> +}
> +
> +static int imx378_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx378 *imx378 = container_of(ctrl->handler,
> + struct imx378, ctrls);
> + int ret;
> +
> + /*
> + * Applying V4L2 control value only happens
> + * when power is up for streaming
> + */
> + if (!pm_runtime_get_if_in_use(imx378->dev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_TEMPERATURE:
> + ret = imx378_g_temperature(imx378, &ctrl->val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + pm_runtime_put(imx378->dev);
> +
> + return ret;
> +}
> +
> +static int imx378_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct imx378 *imx378 = container_of(ctrl->handler,
> + struct imx378, ctrls);
> + int ret;
> +
> + /*
> + * Applying V4L2 control value only happens
> + * when power is up for streaming
> + */
> + if (!pm_runtime_get_if_in_use(imx378->dev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE_ABSOLUTE:
> + ret = imx378_set_exposure(imx378);
> + break;
> + case V4L2_CID_ANALOGUE_GAIN:
> + ret = imx378_set_analogue_gain(imx378);
> + break;
> + case V4L2_CID_DIGITAL_GAIN:
> + ret = imx378_set_digital_gain_global(imx378);
> + break;
> + case V4L2_CID_RED_BALANCE:
> + case V4L2_CID_BLUE_BALANCE:
> + case IMX378_CID_GREEN_BALANCE:
Perhaps Hans can correct me, but my understanding is the red and blue
balances are meant to control the balance of the two while green stays
constant.
The difference is that the gains that the sensor implements are
independent. I wouldn't use the existing controls for them. Also, the green
gain is different for those pixels that are adjacent to blue and red pixels
horizontally. There's usually no need to set them separately though, but I
think it could still make sense to have that in the control API for
completeness.
I'd add four standard controls for these if you need this feature.
> + ret = imx378_set_digital_gain_by_color(imx378);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + pm_runtime_put(imx378->dev);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops imx378_ctrl_ops = {
> + .g_volatile_ctrl = imx378_g_volatile_ctrl,
> + .s_ctrl = imx378_set_ctrl,
> +};
> +
> +#define MAX_CMD 4
> +static int imx378_write_table(struct imx378 *imx378,
> + const struct reg_8 table[])
> +{
> + u8 vals[MAX_CMD];
> + int i, ret;
> +
> + for (table = table; table->addr != IMX378_TABLE_END ; table++) {
> + if (table->addr == IMX378_TABLE_WAIT_MS) {
> + usleep_range(table->val * 1000,
> + table->val * 1000 + 500);
> + continue;
> + }
> +
> + for (i = 0; i < MAX_CMD; i++) {
> + if (table[i].addr != (table[0].addr + i))
> + break;
> + vals[i] = table[i].val;
> + }
> +
> + ret = regmap_bulk_write(imx378->regmap, table->addr, vals, i);
> + usleep_range(80, 120);
> +
> + if (ret) {
> + dev_err(imx378->dev, "write_table error: %d\n", ret);
> + return ret;
> + }
> +
> + table += i - 1;
> + }
> +
> + return 0;
> +}
> +
> +static int imx378_start_streaming(struct imx378 *imx378)
> +{
> + int ret;
> +
> + ret = imx378_write_table(imx378, mode_table_common);
> + if (ret < 0) {
> + dev_err(imx378->dev, "could not send common table %d\n", ret);
> + goto error;
> + }
> +
> + ret = __v4l2_ctrl_handler_setup(&imx378->ctrls);
> + if (ret < 0) {
> + dev_err(imx378->dev, "could not sync v4l2 controls\n");
> + goto error;
> + }
> +
> + ret = imx378_set_img_size(imx378);
> + if (ret < 0) {
> + dev_err(imx378->dev, "could not set image size\n");
> + goto error;
> + }
> +
> + /* set orientation */
> + ret = regmap_write(imx378->regmap, 0x101,
> + imx378->hflip->val | imx378->vflip->val << 1);
> + if (ret < 0)
> + goto error;
> +
> + /* set temperature control */
> + ret = regmap_write(imx378->regmap, 0x138, 1);
> + if (ret < 0)
> + goto error;
Could you use v4l2_ctrl_handler_setup() for doing this instead?
Also, in general, using descriptive macros instead of raw register
addresses is preferred.
> +
> + ret = regmap_write(imx378->regmap, 0x100, 1);
> + if (ret < 0) {
> + dev_err(imx378->dev, "could not send start table %d\n", ret);
> + goto error;
> + }
> +
> + usleep_range(4000, 6000);
> +
> + return 0;
> +
> +error:
> + mutex_unlock(&imx378->mutex);
> + return ret;
> +}
> +
> +static int imx378_stop_streaming(struct imx378 *imx378)
> +{
> + int ret;
> +
> + ret = regmap_write(imx378->regmap, 0x100, 0);
> + if (ret < 0)
> + dev_err(imx378->dev, "could not send stop table %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int imx378_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct imx378 *imx378 = to_imx378(sd);
> + int ret;
> +
> + mutex_lock(&imx378->mutex);
> +
> + if (imx378->streaming == enable) {
> + mutex_unlock(&imx378->mutex);
> + return 0;
> + }
> +
> + if (enable) {
> + ret = pm_runtime_get_sync(imx378->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(imx378->dev);
> + goto err_unlock;
> + }
> +
> + ret = imx378_start_streaming(imx378);
> + if (ret < 0)
> + goto err_rpm_put;
> + } else {
> + ret = imx378_stop_streaming(imx378);
> + if (ret < 0)
> + goto err_rpm_put;
> + pm_runtime_put(imx378->dev);
> + }
> +
> + imx378->streaming = enable;
> +
> + mutex_unlock(&imx378->mutex);
> +
> + return 0;
> +
> +err_rpm_put:
> + pm_runtime_put(imx378->dev);
> +err_unlock:
> + mutex_unlock(&imx378->mutex);
> +
> + return ret;
> +}
> +
> +static int imx378_entity_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg)
> +{
> + struct v4l2_subdev_format fmt = { };
> + struct v4l2_subdev_frame_interval fival = { };
> +
> + if (!cfg) {
This function is never called with cfg being NULL.
> + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY :
> + V4L2_SUBDEV_FORMAT_ACTIVE;
cfg would be always NULL here.
> + fmt.format.width = IMX378_MAX_BOUNDS_WIDTH;
> + fmt.format.height = IMX378_MAX_BOUNDS_HEIGHT;
> +
> + imx378_set_fmt(sd, cfg, &fmt);
> +
> + fival.interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL;
> + imx378_s_frame_interval(sd, &fival);
> + }
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops imx378_video_ops = {
> + .s_stream = imx378_s_stream,
> + .g_frame_interval = imx378_g_frame_interval,
> + .s_frame_interval = imx378_s_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx378_subdev_pad_ops = {
> + .enum_mbus_code = imx378_enum_mbus_code,
> + .enum_frame_size = imx378_enum_frame_size,
> + .enum_frame_interval = imx378_enum_frame_interval,
> + .get_fmt = imx378_get_fmt,
> + .set_fmt = imx378_set_fmt,
> + .get_selection = imx378_get_selection,
> + .set_selection = imx378_set_selection,
> + .init_cfg = imx378_entity_init_cfg,
> +};
> +
> +static const struct v4l2_subdev_ops imx378_subdev_ops = {
> + .core = &imx378_core_ops,
> + .video = &imx378_video_ops,
> + .pad = &imx378_subdev_pad_ops,
> +};
> +
> +static const struct regmap_config sensor_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int imx378_get_regulators(struct device *dev, struct imx378 *imx378)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < IMX378_NUM_SUPPLIES; i++)
> + imx378->supplies[i].supply = imx378_supply_name[i];
> +
> + return devm_regulator_bulk_get(dev, IMX378_NUM_SUPPLIES,
> + imx378->supplies);
> +}
> +
> +struct v4l2_ctrl
> +*imx378_ctrl_new_str_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name,
> + s64 min, s64 max, u64 step, s64 def)
> +{
> + static struct v4l2_ctrl_config ctrl = {
> + .ops = &imx378_ctrl_ops,
> + .type = V4L2_CTRL_TYPE_STRING,
> + };
> +
> + ctrl.id = id;
> + ctrl.name = name;
> + ctrl.min = min;
> + ctrl.max = max;
> + ctrl.step = step;
> + ctrl.def = def;
> +
> + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL);
> +}
> +
> +struct v4l2_ctrl
> +*imx378_ctrl_new_int_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name,
> + s64 min, s64 max, u64 step, s64 def)
> +{
> + static struct v4l2_ctrl_config ctrl = {
> + .ops = &imx378_ctrl_ops,
> + .type = V4L2_CTRL_TYPE_INTEGER,
> + };
> +
> + ctrl.id = id;
> + ctrl.name = name;
> + ctrl.min = min;
> + ctrl.max = max;
> + ctrl.step = step;
> + ctrl.def = def;
> +
> + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL);
> +}
> +
> +static int imx378_init_controls(struct imx378 *imx378)
> +{
> + static const s64 link_freq[] = {
> + IMX378_DEFAULT_LINK_FREQ,
> + };
> + int ret;
> +
> + mutex_init(&imx378->mutex);
> + imx378->ctrls.lock = &imx378->mutex;
> +
> + v4l2_ctrl_handler_init(&imx378->ctrls, 12);
> +
> + imx378->pixel_rate = v4l2_ctrl_new_std(&imx378->ctrls, NULL,
> + V4L2_CID_PIXEL_RATE, 0,
> + IMX378_DEFAULT_PIXEL_RATE, 1,
> + IMX378_DEFAULT_PIXEL_RATE);
> +
> + imx378->link_freq = v4l2_ctrl_new_int_menu(&imx378->ctrls, NULL,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(link_freq) - 1,
> + 0, link_freq);
> + if (imx378->link_freq)
> + imx378->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + imx378->exposure = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
> + V4L2_CID_EXPOSURE_ABSOLUTE,
> + 0x8, 0xffff, 1, 0xc70);
> +
> + imx378->hflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + if (imx378->hflip)
> + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + imx378->vflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + if (imx378->vflip)
> + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + imx378->analogue_gain = v4l2_ctrl_new_std(&imx378->ctrls,
> + &imx378_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN,
> + 0, 978, 1, 0);
> +
> + imx378->digital_gain = v4l2_ctrl_new_std(&imx378->ctrls,
> + &imx378_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> + 0x100, 0xfff, 1, 0x100);
> +
> + imx378->red_balance = v4l2_ctrl_new_std(&imx378->ctrls,
> + &imx378_ctrl_ops, V4L2_CID_RED_BALANCE,
> + 0x100, 0xfff, 1, 0x221);
> +
> + imx378->blue_balance = v4l2_ctrl_new_std(&imx378->ctrls,
> + &imx378_ctrl_ops, V4L2_CID_BLUE_BALANCE,
> + 0x100, 0xfff, 1, 0x1ee);
> +
> + imx378->green_balance = imx378_ctrl_new_int_custom(&imx378->ctrls,
> + IMX378_CID_GREEN_BALANCE,
> + "Green Balance",
> + 0x100, 0xfff, 1, 0x100);
> + if (imx378->green_balance)
> + imx378->green_balance->flags = V4L2_CTRL_FLAG_SLIDER;
> +
> + imx378->temperature = v4l2_ctrl_new_std(&imx378->ctrls,
> + &imx378_ctrl_ops, V4L2_CID_TEMPERATURE,
> + -200000, 800000, 1, 250000);
Those are big numbers.
> + if (imx378->temperature)
> + imx378->temperature->name = "Sensor Temperature";
Standard controls do not need to be renamed by drivers.
> +
> + ret = imx378->ctrls.error;
> + if (ret) {
> + dev_err(imx378->dev, "%s control init failed (%d)\n",
> + __func__, ret);
> + goto error;
> + }
> +
> + imx378->sd.ctrl_handler = &imx378->ctrls;
> +
> + return 0;
> +
> +error:
> + v4l2_ctrl_handler_free(&imx378->ctrls);
> + return ret;
> +}
> +
> +static int imx378_parse_fwnode(struct device *dev)
> +{
> + struct fwnode_handle *endpoint;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + unsigned int i;
> + int ret;
> +
> + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> + if (!endpoint) {
> + dev_err(dev, "endpoint node not found\n");
> + return -EINVAL;
> + }
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> + if (ret) {
> + dev_err(dev, "parsing endpoint node failed\n");
> + goto done;
> + }
> +
> + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> + if (bus_cfg.link_frequencies[i] == IMX378_DEFAULT_LINK_FREQ)
> + break;
> +
> + if (i == bus_cfg.nr_of_link_frequencies) {
> + dev_err(dev, "link-frequencies %d not supported, Please review your DT\n",
> + IMX378_DEFAULT_LINK_FREQ);
> + ret = -EINVAL;
> + goto done;
> + }
> +
> +done:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> + fwnode_handle_put(endpoint);
> + return ret;
> +}
> +
> +static int __maybe_unused imx378_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + if (imx378->streaming)
> + imx378_stop_streaming(imx378);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx378_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx378 *imx378 = to_imx378(sd);
> + int ret;
> +
> + if (imx378->streaming) {
> + ret = imx378_start_streaming(imx378);
> + if (ret)
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + imx378_stop_streaming(imx378);
> + imx378->streaming = 0;
> + return ret;
> +}
> +
> +static int imx378_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct imx378 *imx378;
> + int ret;
> +
> + ret = imx378_parse_fwnode(dev);
> + if (ret)
> + return ret;
> +
> + imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL);
> + if (!imx378)
> + return -ENOMEM;
> +
> + imx378->dev = dev;
> +
> + imx378->xclk = devm_clk_get(dev, NULL);
> + if (IS_ERR(imx378->xclk)) {
> + dev_err(dev, "could not get xclk");
> + return PTR_ERR(imx378->xclk);
> + }
> +
> + ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ);
> + if (ret) {
> + dev_err(dev, "could not set xclk frequency\n");
> + return ret;
> + }
> +
> + ret = imx378_get_regulators(dev, imx378);
> + if (ret < 0) {
> + dev_err(dev, "could not get regulators\n");
> + return ret;
> + }
> +
> + imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(imx378->enable_gpio)) {
> + dev_err(dev, "could get enable gpio\n");
> + return PTR_ERR(imx378->enable_gpio);
> + }
> +
> + imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config);
> + if (IS_ERR(imx378->regmap)) {
> + dev_err(dev, "regmap init failed\n");
> + return PTR_ERR(imx378->regmap);
> + }
> +
> + v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops);
> +
> + imx378_power_on(imx378->dev);
This could fail. Please add error handling.
> +
> + pm_runtime_set_active(imx378->dev);
> + pm_runtime_enable(imx378->dev);
> + pm_runtime_idle(imx378->dev);
Nice. But I'd make them the last operation in probe. You'll need to power
off the sensor if the rest fails --- think what happens when CONFIG_PM
disabled.
> +
> + ret = imx378_init_controls(imx378);
> + if (ret) {
> + dev_err(dev, "failed to init controls: %d", ret);
> + goto error_probe;
> + }
> +
> + imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + imx378->pad.flags = MEDIA_PAD_FL_SOURCE;
> + imx378->sd.dev = &client->dev;
> + imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> + ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad);
> + if (ret < 0) {
> + dev_err(dev, "could not register media entity\n");
> + goto free_ctrl;
> + }
> +
> + imx378_entity_init_cfg(&imx378->sd, NULL);
> +
> + ret = v4l2_async_register_subdev_sensor_common(&imx378->sd);
> + if (ret < 0) {
> + dev_err(dev, "could not register v4l2 device\n");
> + goto free_entity;
> + }
> +
> + ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd);
This isn't needed here; it takes place when the async subdevice is bound.
> + if (ret < 0) {
> + dev_err(dev, "could not register v4l2 sd\n");
> + goto free_entity;
> + }
> +
> + ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev);
This shouldn't be called here, but from the bridge driver.
> + if (ret) {
> + dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +
> +free_entity:
> + media_entity_cleanup(&imx378->sd.entity);
> +free_ctrl:
> + v4l2_ctrl_handler_free(&imx378->ctrls);
> +error_probe:
> + mutex_destroy(&imx378->mutex);
> + pm_runtime_disable(imx378->dev);
> +
> + return ret;
> +}
> +
> +static int imx378_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx378 *imx378 = to_imx378(sd);
> +
> + v4l2_async_unregister_subdev(&imx378->sd);
> + media_entity_cleanup(&imx378->sd.entity);
> + v4l2_ctrl_handler_free(&imx378->ctrls);
> +
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
imx378_power_off(imx378);
> +
> + mutex_destroy(&imx378->mutex);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id imx378_of_match[] = {
> + { .compatible = "sony,imx378" },
Please add DT bindings for the device.
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, imx378_of_match);
> +
> +static const struct i2c_device_id imx378_id[] = {
> + {"imx378", 0},
Do you really need i2c device table? If not, please use probe_new and omit
it.
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, imx378_id);
> +
> +static const struct dev_pm_ops imx378_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(imx378_suspend, imx378_resume)
> + SET_RUNTIME_PM_OPS(imx378_power_off, imx378_power_on, NULL)
> +};
> +
> +static struct i2c_driver imx378_i2c_driver = {
> + .driver = {
> + .of_match_table = imx378_of_match,
> + .pm = &imx378_pm_ops,
> + .name = "imx378",
> + },
> + .probe_new = imx378_probe,
> + .remove = imx378_remove,
> + .id_table = imx378_id,
> +};
> +
> +module_i2c_driver(imx378_i2c_driver);
> +
> +MODULE_DESCRIPTION("Sony IMX378 Camera driver");
> +MODULE_AUTHOR("Daniel Gomez <daniel@qtec.com>");
> +MODULE_LICENSE("GPL v2");
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez
@ 2020-04-30 9:42 ` Sakari Ailus
2020-04-30 11:10 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Sakari Ailus @ 2020-04-30 9:42 UTC (permalink / raw)
To: Daniel Gomez
Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel, Laurent Pinchart
Hi Daniel,
Thanks for the patchset.
I'm also cc'ing Laurent who I think could be interested in this one.
On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> Add min and max structures to the v4l2-subdev callback in order to allow
> the subdev to return a range of valid frame intervals.
>
> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> its max and min values for the width and the height. In this case, the
> possibility to return a frame interval range is added to the v4l2-subdev level
> whenever the v4l2 device operates in step-wise or continuous mode.
The current API only allows providing a list of enumerated values. That is
limiting indeed, especially on register list based sensor drivers where
vertical blanking is configurable.
I guess this could be extended to cover what V4L2, more or less. If we tell
it's a range, is it assumed to be contiguous? We don't have try operation
for the frame interval, but I guess set is good enough. The fraction is
probably best for TV standards but it's not what camera sensors natively
use. (But for a register list based driver, the established practice
remains to use frame interval.)
I'm also wondering the effect on existing user space; if a driver gives a
range, how will the existing programs work with such a driver?
I'd add an anonymous union with the interval field, the other field being
min_interval. Then the current applications would get the minimum interval
and still continue to function. I guess compilers are modern enough these
days we can have an anonymous union in the uAPI?
>
> Signed-off-by: Daniel Gomez <daniel@qtec.com>
> ---
> include/uapi/linux/v4l2-subdev.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 03970ce30741..ee15393c58cd 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> * @code: format code (MEDIA_BUS_FMT_ definitions)
> * @width: frame width in pixels
> * @height: frame height in pixels
> + * @min_interval: min frame interval in seconds
> + * @max_interval: max frame interval in seconds
> * @interval: frame interval in seconds
> * @which: format type (from enum v4l2_subdev_format_whence)
> */
> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> __u32 code;
> __u32 width;
> __u32 height;
> + struct v4l2_fract min_interval;
> + struct v4l2_fract max_interval;
This changes the memory layout of the struct and breaks the ABI. Any new
fields in the struct may only replace reserved fields while the rest must
stay unchanged.
> struct v4l2_fract interval;
> __u32 which;
> - __u32 reserved[8];
> + __u32 reserved[4];
> };
>
> /**
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 9:42 ` Sakari Ailus
@ 2020-04-30 11:10 ` Laurent Pinchart
2020-04-30 13:31 ` Sakari Ailus
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2020-04-30 11:10 UTC (permalink / raw)
To: Sakari Ailus
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hello,
On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the patchset.
>
> I'm also cc'ing Laurent who I think could be interested in this one.
>
> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > Add min and max structures to the v4l2-subdev callback in order to allow
> > the subdev to return a range of valid frame intervals.
> >
> > This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > its max and min values for the width and the height. In this case, the
> > possibility to return a frame interval range is added to the v4l2-subdev level
> > whenever the v4l2 device operates in step-wise or continuous mode.
>
> The current API only allows providing a list of enumerated values. That is
> limiting indeed, especially on register list based sensor drivers where
> vertical blanking is configurable.
>
> I guess this could be extended to cover what V4L2, more or less. If we tell
> it's a range, is it assumed to be contiguous? We don't have try operation
> for the frame interval, but I guess set is good enough. The fraction is
> probably best for TV standards but it's not what camera sensors natively
> use. (But for a register list based driver, the established practice
> remains to use frame interval.)
>
> I'm also wondering the effect on existing user space; if a driver gives a
> range, how will the existing programs work with such a driver?
>
> I'd add an anonymous union with the interval field, the other field being
> min_interval. Then the current applications would get the minimum interval
> and still continue to function. I guess compilers are modern enough these
> days we can have an anonymous union in the uAPI?
We can discuss all this, but given patch 3/3 in this series, I think
this isn't the right API :-) The sensor driver should not expose the
frame interval enumeration API. It should instead expose control of the
frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
V4L2_CID_VBLANK.
> > Signed-off-by: Daniel Gomez <daniel@qtec.com>
> > ---
> > include/uapi/linux/v4l2-subdev.h | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > index 03970ce30741..ee15393c58cd 100644
> > --- a/include/uapi/linux/v4l2-subdev.h
> > +++ b/include/uapi/linux/v4l2-subdev.h
> > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> > * @code: format code (MEDIA_BUS_FMT_ definitions)
> > * @width: frame width in pixels
> > * @height: frame height in pixels
> > + * @min_interval: min frame interval in seconds
> > + * @max_interval: max frame interval in seconds
> > * @interval: frame interval in seconds
> > * @which: format type (from enum v4l2_subdev_format_whence)
> > */
> > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> > __u32 code;
> > __u32 width;
> > __u32 height;
> > + struct v4l2_fract min_interval;
> > + struct v4l2_fract max_interval;
>
> This changes the memory layout of the struct and breaks the ABI. Any new
> fields in the struct may only replace reserved fields while the rest must
> stay unchanged.
>
> > struct v4l2_fract interval;
> > __u32 which;
> > - __u32 reserved[8];
> > + __u32 reserved[4];
> > };
> >
> > /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 11:10 ` Laurent Pinchart
@ 2020-04-30 13:31 ` Sakari Ailus
2020-04-30 13:59 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Sakari Ailus @ 2020-04-30 13:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Laurent,
On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > Thanks for the patchset.
> >
> > I'm also cc'ing Laurent who I think could be interested in this one.
> >
> > On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > Add min and max structures to the v4l2-subdev callback in order to allow
> > > the subdev to return a range of valid frame intervals.
> > >
> > > This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > its max and min values for the width and the height. In this case, the
> > > possibility to return a frame interval range is added to the v4l2-subdev level
> > > whenever the v4l2 device operates in step-wise or continuous mode.
> >
> > The current API only allows providing a list of enumerated values. That is
> > limiting indeed, especially on register list based sensor drivers where
> > vertical blanking is configurable.
> >
> > I guess this could be extended to cover what V4L2, more or less. If we tell
> > it's a range, is it assumed to be contiguous? We don't have try operation
> > for the frame interval, but I guess set is good enough. The fraction is
> > probably best for TV standards but it's not what camera sensors natively
> > use. (But for a register list based driver, the established practice
> > remains to use frame interval.)
> >
> > I'm also wondering the effect on existing user space; if a driver gives a
> > range, how will the existing programs work with such a driver?
> >
> > I'd add an anonymous union with the interval field, the other field being
> > min_interval. Then the current applications would get the minimum interval
> > and still continue to function. I guess compilers are modern enough these
> > days we can have an anonymous union in the uAPI?
>
> We can discuss all this, but given patch 3/3 in this series, I think
> this isn't the right API :-) The sensor driver should not expose the
> frame interval enumeration API. It should instead expose control of the
> frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> V4L2_CID_VBLANK.
>
That would require also exposing the size of the pixel array (and the
analogue crop), in order to provide all the necessary information to
calculate the frame rate. No objections there; this is a new driver.
There are however existing drivers that implement s_frame_interval subdev
ioctl; those might benefit from this one. Or would you implement the pixel
rate based control as well, and effectively deprecate the s_frame_interval
on those?
> > > Signed-off-by: Daniel Gomez <daniel@qtec.com>
> > > ---
> > > include/uapi/linux/v4l2-subdev.h | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > > index 03970ce30741..ee15393c58cd 100644
> > > --- a/include/uapi/linux/v4l2-subdev.h
> > > +++ b/include/uapi/linux/v4l2-subdev.h
> > > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> > > * @code: format code (MEDIA_BUS_FMT_ definitions)
> > > * @width: frame width in pixels
> > > * @height: frame height in pixels
> > > + * @min_interval: min frame interval in seconds
> > > + * @max_interval: max frame interval in seconds
> > > * @interval: frame interval in seconds
> > > * @which: format type (from enum v4l2_subdev_format_whence)
> > > */
> > > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> > > __u32 code;
> > > __u32 width;
> > > __u32 height;
> > > + struct v4l2_fract min_interval;
> > > + struct v4l2_fract max_interval;
> >
> > This changes the memory layout of the struct and breaks the ABI. Any new
> > fields in the struct may only replace reserved fields while the rest must
> > stay unchanged.
> >
> > > struct v4l2_fract interval;
> > > __u32 which;
> > > - __u32 reserved[8];
> > > + __u32 reserved[4];
> > > };
> > >
> > > /**
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 13:31 ` Sakari Ailus
@ 2020-04-30 13:59 ` Laurent Pinchart
2020-04-30 14:15 ` Sakari Ailus
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2020-04-30 13:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Sakari,
On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> >>> Add min and max structures to the v4l2-subdev callback in order to allow
> >>> the subdev to return a range of valid frame intervals.
> >>>
> >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> >>> its max and min values for the width and the height. In this case, the
> >>> possibility to return a frame interval range is added to the v4l2-subdev level
> >>> whenever the v4l2 device operates in step-wise or continuous mode.
> >>
> >> The current API only allows providing a list of enumerated values. That is
> >> limiting indeed, especially on register list based sensor drivers where
> >> vertical blanking is configurable.
> >>
> >> I guess this could be extended to cover what V4L2, more or less. If we tell
> >> it's a range, is it assumed to be contiguous? We don't have try operation
> >> for the frame interval, but I guess set is good enough. The fraction is
> >> probably best for TV standards but it's not what camera sensors natively
> >> use. (But for a register list based driver, the established practice
> >> remains to use frame interval.)
> >>
> >> I'm also wondering the effect on existing user space; if a driver gives a
> >> range, how will the existing programs work with such a driver?
> >>
> >> I'd add an anonymous union with the interval field, the other field being
> >> min_interval. Then the current applications would get the minimum interval
> >> and still continue to function. I guess compilers are modern enough these
> >> days we can have an anonymous union in the uAPI?
> >
> > We can discuss all this, but given patch 3/3 in this series, I think
> > this isn't the right API :-) The sensor driver should not expose the
> > frame interval enumeration API. It should instead expose control of the
> > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > V4L2_CID_VBLANK.
> >
>
> That would require also exposing the size of the pixel array (and the
> analogue crop), in order to provide all the necessary information to
> calculate the frame rate. No objections there; this is a new driver.
>
> There are however existing drivers that implement s_frame_interval subdev
> ioctl; those might benefit from this one. Or would you implement the pixel
> rate based control as well, and effectively deprecate the s_frame_interval
> on those?
That's what I would recommend, yes. I would only keep
.s_frame_interval() for sensors that expose that concept at the hardware
level (for instance with an integrated ISP whose firmware exposes a
frame interval or frame rate control).
> >>> Signed-off-by: Daniel Gomez <daniel@qtec.com>
> >>> ---
> >>> include/uapi/linux/v4l2-subdev.h | 6 +++++-
> >>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> >>> index 03970ce30741..ee15393c58cd 100644
> >>> --- a/include/uapi/linux/v4l2-subdev.h
> >>> +++ b/include/uapi/linux/v4l2-subdev.h
> >>> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> >>> * @code: format code (MEDIA_BUS_FMT_ definitions)
> >>> * @width: frame width in pixels
> >>> * @height: frame height in pixels
> >>> + * @min_interval: min frame interval in seconds
> >>> + * @max_interval: max frame interval in seconds
> >>> * @interval: frame interval in seconds
> >>> * @which: format type (from enum v4l2_subdev_format_whence)
> >>> */
> >>> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> >>> __u32 code;
> >>> __u32 width;
> >>> __u32 height;
> >>> + struct v4l2_fract min_interval;
> >>> + struct v4l2_fract max_interval;
> >>
> >> This changes the memory layout of the struct and breaks the ABI. Any new
> >> fields in the struct may only replace reserved fields while the rest must
> >> stay unchanged.
> >>
> >>> struct v4l2_fract interval;
> >>> __u32 which;
> >>> - __u32 reserved[8];
> >>> + __u32 reserved[4];
> >>> };
> >>>
> >>> /**
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 13:59 ` Laurent Pinchart
@ 2020-04-30 14:15 ` Sakari Ailus
2020-04-30 14:17 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Sakari Ailus @ 2020-04-30 14:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Laurent,
On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > >>> the subdev to return a range of valid frame intervals.
> > >>>
> > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > >>> its max and min values for the width and the height. In this case, the
> > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > >>
> > >> The current API only allows providing a list of enumerated values. That is
> > >> limiting indeed, especially on register list based sensor drivers where
> > >> vertical blanking is configurable.
> > >>
> > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > >> for the frame interval, but I guess set is good enough. The fraction is
> > >> probably best for TV standards but it's not what camera sensors natively
> > >> use. (But for a register list based driver, the established practice
> > >> remains to use frame interval.)
> > >>
> > >> I'm also wondering the effect on existing user space; if a driver gives a
> > >> range, how will the existing programs work with such a driver?
> > >>
> > >> I'd add an anonymous union with the interval field, the other field being
> > >> min_interval. Then the current applications would get the minimum interval
> > >> and still continue to function. I guess compilers are modern enough these
> > >> days we can have an anonymous union in the uAPI?
> > >
> > > We can discuss all this, but given patch 3/3 in this series, I think
> > > this isn't the right API :-) The sensor driver should not expose the
> > > frame interval enumeration API. It should instead expose control of the
> > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > V4L2_CID_VBLANK.
> > >
> >
> > That would require also exposing the size of the pixel array (and the
> > analogue crop), in order to provide all the necessary information to
> > calculate the frame rate. No objections there; this is a new driver.
> >
> > There are however existing drivers that implement s_frame_interval subdev
> > ioctl; those might benefit from this one. Or would you implement the pixel
> > rate based control as well, and effectively deprecate the s_frame_interval
> > on those?
>
> That's what I would recommend, yes. I would only keep
> .s_frame_interval() for sensors that expose that concept at the hardware
> level (for instance with an integrated ISP whose firmware exposes a
> frame interval or frame rate control).
Sounds good to me.
Jacopo's set exposing read-only subdevs completes the puzzle so the user
space should have all it needs, right?
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 14:15 ` Sakari Ailus
@ 2020-04-30 14:17 ` Laurent Pinchart
2020-04-30 14:18 ` Sakari Ailus
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2020-04-30 14:17 UTC (permalink / raw)
To: Sakari Ailus
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Sakari,
On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > > >>> the subdev to return a range of valid frame intervals.
> > > >>>
> > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > >>> its max and min values for the width and the height. In this case, the
> > > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > > >>
> > > >> The current API only allows providing a list of enumerated values. That is
> > > >> limiting indeed, especially on register list based sensor drivers where
> > > >> vertical blanking is configurable.
> > > >>
> > > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > > >> for the frame interval, but I guess set is good enough. The fraction is
> > > >> probably best for TV standards but it's not what camera sensors natively
> > > >> use. (But for a register list based driver, the established practice
> > > >> remains to use frame interval.)
> > > >>
> > > >> I'm also wondering the effect on existing user space; if a driver gives a
> > > >> range, how will the existing programs work with such a driver?
> > > >>
> > > >> I'd add an anonymous union with the interval field, the other field being
> > > >> min_interval. Then the current applications would get the minimum interval
> > > >> and still continue to function. I guess compilers are modern enough these
> > > >> days we can have an anonymous union in the uAPI?
> > > >
> > > > We can discuss all this, but given patch 3/3 in this series, I think
> > > > this isn't the right API :-) The sensor driver should not expose the
> > > > frame interval enumeration API. It should instead expose control of the
> > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > > V4L2_CID_VBLANK.
> > > >
> > >
> > > That would require also exposing the size of the pixel array (and the
> > > analogue crop), in order to provide all the necessary information to
> > > calculate the frame rate. No objections there; this is a new driver.
> > >
> > > There are however existing drivers that implement s_frame_interval subdev
> > > ioctl; those might benefit from this one. Or would you implement the pixel
> > > rate based control as well, and effectively deprecate the s_frame_interval
> > > on those?
> >
> > That's what I would recommend, yes. I would only keep
> > .s_frame_interval() for sensors that expose that concept at the hardware
> > level (for instance with an integrated ISP whose firmware exposes a
> > frame interval or frame rate control).
>
> Sounds good to me.
>
> Jacopo's set exposing read-only subdevs completes the puzzle so the user
> space should have all it needs, right?
Until we run into the next missing piece :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 14:17 ` Laurent Pinchart
@ 2020-04-30 14:18 ` Sakari Ailus
2020-04-30 14:20 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Sakari Ailus @ 2020-04-30 14:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > > > >>> the subdev to return a range of valid frame intervals.
> > > > >>>
> > > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > > >>> its max and min values for the width and the height. In this case, the
> > > > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > > > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > > > >>
> > > > >> The current API only allows providing a list of enumerated values. That is
> > > > >> limiting indeed, especially on register list based sensor drivers where
> > > > >> vertical blanking is configurable.
> > > > >>
> > > > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > > > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > > > >> for the frame interval, but I guess set is good enough. The fraction is
> > > > >> probably best for TV standards but it's not what camera sensors natively
> > > > >> use. (But for a register list based driver, the established practice
> > > > >> remains to use frame interval.)
> > > > >>
> > > > >> I'm also wondering the effect on existing user space; if a driver gives a
> > > > >> range, how will the existing programs work with such a driver?
> > > > >>
> > > > >> I'd add an anonymous union with the interval field, the other field being
> > > > >> min_interval. Then the current applications would get the minimum interval
> > > > >> and still continue to function. I guess compilers are modern enough these
> > > > >> days we can have an anonymous union in the uAPI?
> > > > >
> > > > > We can discuss all this, but given patch 3/3 in this series, I think
> > > > > this isn't the right API :-) The sensor driver should not expose the
> > > > > frame interval enumeration API. It should instead expose control of the
> > > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > > > V4L2_CID_VBLANK.
> > > > >
> > > >
> > > > That would require also exposing the size of the pixel array (and the
> > > > analogue crop), in order to provide all the necessary information to
> > > > calculate the frame rate. No objections there; this is a new driver.
> > > >
> > > > There are however existing drivers that implement s_frame_interval subdev
> > > > ioctl; those might benefit from this one. Or would you implement the pixel
> > > > rate based control as well, and effectively deprecate the s_frame_interval
> > > > on those?
> > >
> > > That's what I would recommend, yes. I would only keep
> > > .s_frame_interval() for sensors that expose that concept at the hardware
> > > level (for instance with an integrated ISP whose firmware exposes a
> > > frame interval or frame rate control).
> >
> > Sounds good to me.
> >
> > Jacopo's set exposing read-only subdevs completes the puzzle so the user
> > space should have all it needs, right?
>
> Until we run into the next missing piece :-)
I was thinking of the frame rate configuration. Can you confirm that?
--
Sakari Ailus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum
2020-04-30 14:18 ` Sakari Ailus
@ 2020-04-30 14:20 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2020-04-30 14:20 UTC (permalink / raw)
To: Sakari Ailus
Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel
Hi Sakari,
On Thu, Apr 30, 2020 at 05:18:49PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> >> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> >>>> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> >>>>>> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> >>>>>>> Add min and max structures to the v4l2-subdev callback in order to allow
> >>>>>>> the subdev to return a range of valid frame intervals.
> >>>>>>>
> >>>>>>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> >>>>>>> its max and min values for the width and the height. In this case, the
> >>>>>>> possibility to return a frame interval range is added to the v4l2-subdev level
> >>>>>>> whenever the v4l2 device operates in step-wise or continuous mode.
> >>>>>>
> >>>>>> The current API only allows providing a list of enumerated values. That is
> >>>>>> limiting indeed, especially on register list based sensor drivers where
> >>>>>> vertical blanking is configurable.
> >>>>>>
> >>>>>> I guess this could be extended to cover what V4L2, more or less. If we tell
> >>>>>> it's a range, is it assumed to be contiguous? We don't have try operation
> >>>>>> for the frame interval, but I guess set is good enough. The fraction is
> >>>>>> probably best for TV standards but it's not what camera sensors natively
> >>>>>> use. (But for a register list based driver, the established practice
> >>>>>> remains to use frame interval.)
> >>>>>>
> >>>>>> I'm also wondering the effect on existing user space; if a driver gives a
> >>>>>> range, how will the existing programs work with such a driver?
> >>>>>>
> >>>>>> I'd add an anonymous union with the interval field, the other field being
> >>>>>> min_interval. Then the current applications would get the minimum interval
> >>>>>> and still continue to function. I guess compilers are modern enough these
> >>>>>> days we can have an anonymous union in the uAPI?
> >>>>>
> >>>>> We can discuss all this, but given patch 3/3 in this series, I think
> >>>>> this isn't the right API :-) The sensor driver should not expose the
> >>>>> frame interval enumeration API. It should instead expose control of the
> >>>>> frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> >>>>> V4L2_CID_VBLANK.
> >>>>>
> >>>>
> >>>> That would require also exposing the size of the pixel array (and the
> >>>> analogue crop), in order to provide all the necessary information to
> >>>> calculate the frame rate. No objections there; this is a new driver.
> >>>>
> >>>> There are however existing drivers that implement s_frame_interval subdev
> >>>> ioctl; those might benefit from this one. Or would you implement the pixel
> >>>> rate based control as well, and effectively deprecate the s_frame_interval
> >>>> on those?
> >>>
> >>> That's what I would recommend, yes. I would only keep
> >>> .s_frame_interval() for sensors that expose that concept at the hardware
> >>> level (for instance with an integrated ISP whose firmware exposes a
> >>> frame interval or frame rate control).
> >>
> >> Sounds good to me.
> >>
> >> Jacopo's set exposing read-only subdevs completes the puzzle so the user
> >> space should have all it needs, right?
> >
> > Until we run into the next missing piece :-)
>
> I was thinking of the frame rate configuration. Can you confirm that?
I believe so, yes.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-04-30 14:20 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez
2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez
2020-04-30 9:42 ` Sakari Ailus
2020-04-30 11:10 ` Laurent Pinchart
2020-04-30 13:31 ` Sakari Ailus
2020-04-30 13:59 ` Laurent Pinchart
2020-04-30 14:15 ` Sakari Ailus
2020-04-30 14:17 ` Laurent Pinchart
2020-04-30 14:18 ` Sakari Ailus
2020-04-30 14:20 ` Laurent Pinchart
2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez
2020-04-28 20:15 ` Sakari Ailus
2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez
2020-04-15 0:37 ` kbuild test robot
2020-04-28 21:02 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.