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
