RE: [PATCH v3] media: imx208: Add imx208 camera sensor driver
Hi Tomasz, Please see my comments. -Original Message- >Hi Ping-chung, >On Wed, Aug 1, 2018 at 11:31 AM Ping-chung Chen >wrote: > > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen > --- > since v1: > -- Update the function media_entity_pads_init for upstreaming. > -- Change the structure name mutex as imx208_mx. > -- Refine the control flow of test pattern function. > -- vflip/hflip control support (will impact the output bayer order) > -- support 4 bayer orders output (via change v/hflip) > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > -- Simplify error handling in the set_stream function. > since v2: > -- Refine coding style. > -- Fix the if statement to use pm_runtime_get_if_in_use(). > -- Print more error log during error handling. > -- Remove mutex_destroy() from imx208_free_controls(). > -- Add more comments. >Thanks for addressing the comments. There are still few remaining, though. >Please see inline. [snip] > +enum { > + IMX208_LINK_FREQ_384MHZ_INDEX, > + IMX208_LINK_FREQ_96MHZ_INDEX, > +}; > + > +/* > + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample > + * data rate => double data rate; number of lanes => 2; bits per > +pixel => 10 */ static u64 link_freq_to_pixel_rate(u64 f) { > + f *= IMX208_DATA_RATE_DOUBLE * IMX208_NUM_OF_LANES; > + do_div(f, IMX208_PIXEL_BITS); > + > + return f; > +} > + > +/* Menu items for LINK_FREQ V4L2 control */ static const s64 > +link_freq_menu_items[] = { > + IMX208_LINK_FREQ_384MHZ, > + IMX208_LINK_FREQ_96MHZ, >I asked for having explicit indices using IMX208_LINK_FREQ_*_INDEX enum used >here too. Sorry I forgot this one, fixed. > +}; > + > +/* Link frequency configs */ > +static const struct imx208_link_freq_config link_freq_configs[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_384MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_96MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > +}; > + > +/* Mode configs */ > +static const struct imx208_mode supported_modes[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = { >This is not the right index for this array (even if numerically the same. This >array is just a list of available modes (resolutions) and there is no other >data structure referring to particular entries of it, so it doesn't need >explicit indexing. Done. > + .width = 1936, > + .height = 1096, > + .vts_def = IMX208_VTS_60FPS, > + .vts_min = IMX208_VTS_60FPS_MIN, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1936x1096_60fps_regs), > + .regs = mode_1936x1096_60fps_regs, > + }, > + .link_freq_index = IMX208_LINK_FREQ_384MHZ_INDEX, > + }, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = { >Ditto. Best regards, Tomasz
Re: [PATCH v3] media: imx208: Add imx208 camera sensor driver
Hi Ping-chung, On Wed, Aug 1, 2018 at 11:31 AM Ping-chung Chen wrote: > > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen > --- > since v1: > -- Update the function media_entity_pads_init for upstreaming. > -- Change the structure name mutex as imx208_mx. > -- Refine the control flow of test pattern function. > -- vflip/hflip control support (will impact the output bayer order) > -- support 4 bayer orders output (via change v/hflip) > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > -- Simplify error handling in the set_stream function. > since v2: > -- Refine coding style. > -- Fix the if statement to use pm_runtime_get_if_in_use(). > -- Print more error log during error handling. > -- Remove mutex_destroy() from imx208_free_controls(). > -- Add more comments. Thanks for addressing the comments. There are still few remaining, though. Please see inline. [snip] > +enum { > + IMX208_LINK_FREQ_384MHZ_INDEX, > + IMX208_LINK_FREQ_96MHZ_INDEX, > +}; > + > +/* > + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample > + * data rate => double data rate; number of lanes => 2; bits per pixel => 10 > + */ > +static u64 link_freq_to_pixel_rate(u64 f) > +{ > + f *= IMX208_DATA_RATE_DOUBLE * IMX208_NUM_OF_LANES; > + do_div(f, IMX208_PIXEL_BITS); > + > + return f; > +} > + > +/* Menu items for LINK_FREQ V4L2 control */ > +static const s64 link_freq_menu_items[] = { > + IMX208_LINK_FREQ_384MHZ, > + IMX208_LINK_FREQ_96MHZ, I asked for having explicit indices using IMX208_LINK_FREQ_*_INDEX enum used here too. > +}; > + > +/* Link frequency configs */ > +static const struct imx208_link_freq_config link_freq_configs[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_384MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_96MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > +}; > + > +/* Mode configs */ > +static const struct imx208_mode supported_modes[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = { This is not the right index for this array (even if numerically the same. This array is just a list of available modes (resolutions) and there is no other data structure referring to particular entries of it, so it doesn't need explicit indexing. > + .width = 1936, > + .height = 1096, > + .vts_def = IMX208_VTS_60FPS, > + .vts_min = IMX208_VTS_60FPS_MIN, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1936x1096_60fps_regs), > + .regs = mode_1936x1096_60fps_regs, > + }, > + .link_freq_index = IMX208_LINK_FREQ_384MHZ_INDEX, > + }, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = { Ditto. Best regards, Tomasz
[PATCH v3] media: imx208: Add imx208 camera sensor driver
From: "Chen, Ping-chung" Add a V4L2 sub-device driver for the Sony IMX208 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Ping-Chung Chen --- since v1: -- Update the function media_entity_pads_init for upstreaming. -- Change the structure name mutex as imx208_mx. -- Refine the control flow of test pattern function. -- vflip/hflip control support (will impact the output bayer order) -- support 4 bayer orders output (via change v/hflip) - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 -- Simplify error handling in the set_stream function. since v2: -- Refine coding style. -- Fix the if statement to use pm_runtime_get_if_in_use(). -- Print more error log during error handling. -- Remove mutex_destroy() from imx208_free_controls(). -- Add more comments. MAINTAINERS| 7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx208.c | 995 + 4 files changed, 1014 insertions(+) create mode 100644 drivers/media/i2c/imx208.c diff --git a/MAINTAINERS b/MAINTAINERS index bbd9b9b..896c1df 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13268,6 +13268,13 @@ S: Maintained F: drivers/ssb/ F: include/linux/ssb/ +SONY IMX208 SENSOR DRIVER +M: Sakari Ailus +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx208.c + SONY IMX258 SENSOR DRIVER M: Sakari Ailus L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 8669853..ae11f1e 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL tristate +config VIDEO_IMX208 + tristate "Sony IMX208 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor driver for the Sony + IMX208 camera. + + To compile this driver as a module, choose M here: the + module will be called imx208. + config VIDEO_IMX258 tristate "Sony IMX258 sensor support" depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 837c428..47604c2 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_IMX208) += imx208.o obj-$(CONFIG_VIDEO_IMX258) += imx258.o obj-$(CONFIG_VIDEO_IMX274) += imx274.o diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c new file mode 100644 index 000..e997193 --- /dev/null +++ b/drivers/media/i2c/imx208.c @@ -0,0 +1,995 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Intel Corporation + +#include +#include +#include +#include +#include +#include +#include +#include + +#define IMX208_REG_MODE_SELECT 0x0100 +#define IMX208_MODE_STANDBY0x00 +#define IMX208_MODE_STREAMING 0x01 + +/* Chip ID */ +#define IMX208_REG_CHIP_ID 0x +#define IMX208_CHIP_ID 0x0208 + +/* V_TIMING internal */ +#define IMX208_REG_VTS 0x0340 +#define IMX208_VTS_60FPS 0x0472 +#define IMX208_VTS_BINNING 0x0239 +#define IMX208_VTS_60FPS_MIN 0x0458 +#define IMX208_VTS_BINNING_MIN 0x0230 +#define IMX208_VTS_MAX 0x + +/* HBLANK control - read only */ +#define IMX208_PPL_384MHZ 2248 +#define IMX208_PPL_96MHZ 2248 + +/* Exposure control */ +#define IMX208_REG_EXPOSURE0x0202 +#define IMX208_EXPOSURE_MIN4 +#define IMX208_EXPOSURE_STEP 1 +#define IMX208_EXPOSURE_DEFAULT0x190 +#define IMX208_EXPOSURE_MAX65535 + +/* Analog gain control */ +#define IMX208_REG_ANALOG_GAIN 0x0204 +#define IMX208_ANA_GAIN_MIN0 +#define IMX208_ANA_GAIN_MAX0x00e0 +#define IMX208_ANA_GAIN_STEP 1 +#define IMX208_ANA_GAIN_DEFAULT0x0 + +/* Digital gain control */ +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e +#define IMX208_REG_R_DIGITAL_GAIN 0x0210 +#define IMX208_REG_B_DIGITAL_GAIN 0x0212 +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214 +#define IMX208_DGTL_GAIN_MIN 0 +#define IMX208_DGTL_GAIN_MAX 4096 +#define IMX208_DGTL_GAIN_DEFAULT 0x100 +#define IMX208_DGTL_GAIN_STEP 1 + +/* Orientation */ +#define IMX208_REG_ORIENTATION_CONTROL 0x0101 + +/* Test Pattern Control */ +#define IMX208_REG_TEST_PATTERN_MODE 0x0600 +#define IMX208_TEST_PATTERN_DISABLE0x0 +#define