media: imx258: Add imx258 camera sensor driver
PATCH 1/1 media: imx258: Add imx258 camera sensor driver
From: Jason Chen <[email protected]>
Add a V4L2 sub-device driver for the Sony IMX258 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data.
---
since v2:
-- Update the streaming function to remove SW_STANDBY in the beginning. -- Adjust the delay time from 1ms to 12ms before set stream-on register. since v3:
-- fix the sd.entity to make code be compiled on the mainline kernel. since v4:
-- Enabled AG, DG, and Exposure time control correctly.
since v5:
-- Sensor vendor provided a new setting to fix different CLK issue -- Add one more resolution for 1048x780, used for VGA streaming since v6:
-- improved i2c read/write function to support writing 2 registers -- modified i2c reg read/write function with a more portable way -- utilized v4l2_find_nearest_size instead of the local find_best_fit function -- defined an enum for the link freq entries for explicit indexing since v7:
-- Removed usleep due to sufficient delay implemented in coreboot -- Added handling for VBLANK control that auto frame-line-control is enabled since v8:
-- Fix some error return and intents
since v9:
-- Fix a typo (fmr -> fmt)
MAINTAINERS | 7 + drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx258.c | 1296 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1315 insertions(+) create mode 100644 drivers/media/i2c/imx258.c
MAINTAINERS +7 -0
@@ -12646,6 +12646,13 @@ S: Maintained F: drivers/ssb/ F: include/linux/ssb/ +SONY IMX258 SENSOR DRIVER +M: Sakari Ailus <[email protected]> +L: [email protected] +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/imx258.c + SONY IMX274 SENSOR DRIVER M: Leon Luo <[email protected]> L: [email protected]
drivers/media/i2c/Kconfig +11 -0
@@ -565,6 +565,17 @@ config VIDEO_APTINA_PLL config VIDEO_SMIAPP_PLL tristate +config VIDEO_IMX258 + tristate "Sony IMX258 sensor support" + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on MEDIA_CAMERA_SUPPORT + ---help--- + This is a Video4Linux2 sensor-level driver for the Sony + IMX258 camera. + + To compile this driver as a module, choose M here: the + module will be called imx258. + config VIDEO_IMX274 tristate "Sony IMX274 sensor support" depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
drivers/media/i2c/Makefile +1 -0
@@ -94,6 +94,7 @@ obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o obj-$(CONFIG_VIDEO_OV2659) += ov2659.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o +obj-$(CONFIG_VIDEO_IMX258) += imx258.o obj-$(CONFIG_VIDEO_IMX274) += imx274.o obj-$(CONFIG_SDR_MAX2175) += max2175.o
drivers/media/i2c/imx258.c new file +1296 -0
1,296 lines not on this page · raw message
Signature · 1 line
2.7.4
kbuild test robot
build botThank you for the patch! Yet something to improve:
Hi Jason,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andy-Yeh/media-imx258-Add-imx258-camera-sensor-driver/20180319-082217
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce:
# save the attached .config to linux build tree
make ARCH=i386 All errors (new ones prefixed by >>):
drivers/media//i2c/imx258.c: In function 'imx258_set_pad_format':
drivers/media//i2c/imx258.c:870:9: error: implicit declaration of function 'v4l2_find_nearest_size'; did you mean 'v4l2_find_nearest_format'? [-Werror=implicit-function-declaration]
mode = v4l2_find_nearest_size(
^~~~~~~~~~~~~~~~~~~~~~
v4l2_find_nearest_format
drivers/media//i2c/imx258.c:871:200: error: 'width' undeclared (first use in this function)
supported_modes, ARRAY_SIZE(supported_modes), width, height,
^
drivers/media//i2c/imx258.c:871:200: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/media//i2c/imx258.c:871:207: error: 'height' undeclared (first use in this function); did you mean '__iget'?
supported_modes, ARRAY_SIZE(supported_modes), width, height,
^
__igetcc1: some warnings being treated as errors
vim +871 drivers/media//i2c/imx258.c
851
852 static int imx258_set_pad_format(struct v4l2_subdev *sd,
853 struct v4l2_subdev_pad_config *cfg,
854 struct v4l2_subdev_format *fmt)
855 {
856 struct imx258 *imx258 = to_imx258(sd);
857 const struct imx258_mode *mode;
858 struct v4l2_mbus_framefmt *framefmt;
859 s32 vblank_def;
860 s32 vblank_min;
861 s64 h_blank;
862 s64 pixel_rate;
863 s64 link_freq;
864
865 mutex_lock(&imx258->mutex);
866
867 /* Only one raw bayer(GBRG) order is supported */
868 fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10;
869
> 870 mode = v4l2_find_nearest_size( > 871 supported_modes, ARRAY_SIZE(supported_modes), width, height,
872 fmt->format.width, fmt->format.height);
873 imx258_update_pad_format(mode, fmt);
874 if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
875 framefmt = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
876 *framefmt = fmt->format;
877 } else {
878 imx258->cur_mode = mode;
879 __v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
880
881 link_freq = link_freq_menu_items[mode->link_freq_index];
882 pixel_rate = link_freq_to_pixel_rate(link_freq);
883 __v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
884 /* Update limits and set FPS to default */
885 vblank_def = imx258->cur_mode->vts_def -
886 imx258->cur_mode->height;
887 vblank_min = imx258->cur_mode->vts_min -
888 imx258->cur_mode->height;
889 __v4l2_ctrl_modify_range(
890 imx258->vblank, vblank_min,
891 IMX258_VTS_MAX - imx258->cur_mode->height, 1,
892 vblank_def);
893 __v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
894 h_blank =
895 link_freq_configs[mode->link_freq_index].pixels_per_line
896 - imx258->cur_mode->width;
897 __v4l2_ctrl_modify_range(imx258->hblank, h_blank,
898 h_blank, 1, h_blank);
899 }
900
901 mutex_unlock(&imx258->mutex);
902
903 return 0;
904 }
905
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Andy,
Some issues found when reviewing cherry pick of this patch to Chrome OS kernel. Please see inline.
On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <[email protected]> wrote:
[snip]
drivers/media/i2c/imx258.c line 766
Quoted 5 lines
drivers/media/i2c/imx258.c new file +5
-0+case V4L2_CID_VBLANK : +/* +* Auto Frame Length Line Control is enabled by default. +* Not need control Vblank Register. +*/
What is the meaning of this control then? Should it be read-only?
Quoted 13 lines
drivers/media/i2c/imx258.c new file +13
-0+break ; +default : +dev_info (&client->dev, +"ctrl(id:0x%x,val:0x%x) is not handled\n" , + ctrl->id, ctrl->val); + ret = -EINVAL ; +break ; + } + +pm_runtime_put (&client->dev); + +return ret; +
[snip]
drivers/media/i2c/imx258.c line 1165
Quoted 4 lines
drivers/media/i2c/imx258.c new file +4
-0+v4l2_ctrl_new_std_menu_items (ctrl_hdlr, &imx258_ctrl_ops, +V4L2_CID_TEST_PATTERN , +ARRAY_SIZE (imx258_test_pattern_menu) -1 , +0 ,0 , imx258_test_pattern_menu);
There is no code for handling this control in imx258_s_ctrl(). It's not a correct behavior to register a control, which isn't handled by the driver. Please either implement the control completely or remove it.
drivers/media/i2c/imx258.c line 1171
Quoted 6 lines
drivers/media/i2c/imx258.c new file +6
-0+ +if (ctrl_hdlr->error) { + ret = ctrl_hdlr->error; +dev_err (&client->dev,"%s control init failed (%d)\n" , + __func__, ret); + goto error;
Something strange happening here with indentation.
Best regards,
Tomasz
Sakari Ailus
maintainerHi Tomasz,
Quoted 9 lines · Tomasz Figa
Hi Andy,
Some issues found when reviewing cherry pick of this patch to Chrome OS kernel. Please see inline.
On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <[email protected]> wrote:
[snip]
Quoted 7 lines
drivers/media/i2c/imx258.c line 766
Quoted 5 lines
drivers/media/i2c/imx258.c new file +5
-0+case V4L2_CID_VBLANK : +/* +* Auto Frame Length Line Control is enabled by default. +* Not need control Vblank Register. +*/ What is the meaning of this control then? Should it be read-only?
The read-only flag is for the uAPI; the control framework still passes through changes to the control value done using kAPI to the driver.
Quoted 22 lines
Quoted 14 lines
drivers/media/i2c/imx258.c new file +13
-0+break ; +default : +dev_info (&client->dev, +"ctrl(id:0x%x,val:0x%x) is not handled\n" , + ctrl->id, ctrl->val); + ret = -EINVAL ; +break ; + } + +pm_runtime_put (&client->dev); + +return ret; +[snip]
drivers/media/i2c/imx258.c line 1165
Quoted 4 lines
drivers/media/i2c/imx258.c new file +4
-0+v4l2_ctrl_new_std_menu_items (ctrl_hdlr, &imx258_ctrl_ops, +V4L2_CID_TEST_PATTERN , +ARRAY_SIZE (imx258_test_pattern_menu) -1 , +0 ,0 , imx258_test_pattern_menu);
Quoted 4 lines
There is no code for handling this control in
imx258_s_ctrl(). It's not a correct behavior to register a control, which isn't handled by the driver. Please either implement the control completely or remove it.
Indeed.
Signature · 4 lines
Kind regards, Sakari Ailus [email protected]
Quoted 18 lines · Sakari Ailus
Hi Tomasz,
Quoted 15 lines · Tomasz Figa
Hi Andy,
Some issues found when reviewing cherry pick of this patch to Chrome OS kernel. Please see inline.
On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <[email protected]> wrote:
[snip]
Quoted 5 lines
drivers/media/i2c/imx258.c new file +5
-0+case V4L2_CID_VBLANK : +/* +* Auto Frame Length Line Control is enabled by default. +* Not need control Vblank Register. +*/
Quoted 4 lines
Quoted 1 line
What is the meaning of this control then? Should it be read-only?
The read-only flag is for the uAPI; the control framework still passes through changes to the control value done using kAPI to the driver.
The read-only flag is not even set in current code.
Also, I'm not sure about the control framework setting read-only control. According to the code, it doesn't:
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ctrls.c#L2477
Best regards,
Tomasz
Quoted 20 lines · Tomasz Figa
Quoted 18 lines · Sakari Ailus
Hi Tomasz,
Quoted 15 lines · Tomasz Figa
Hi Andy,
Some issues found when reviewing cherry pick of this patch to Chrome OS kernel. Please see inline.
On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <[email protected]> wrote:
[snip]
Quoted 5 lines
drivers/media/i2c/imx258.c new file +5
-0+case V4L2_CID_VBLANK : +/* +* Auto Frame Length Line Control is enabled by default. +* Not need control Vblank Register. +*/
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
What is the meaning of this control then? Should it be read-only?
The read-only flag is for the uAPI; the control framework still passes through changes to the control value done using kAPI to the driver.
The read-only flag is not even set in current code.
Ah, you're right, it's just hblank... but if the driver doesn't support setting this control, then it should most likely be read-only. It would seem like that the driver just updates the control to convey the value to the user.
Quoted 4 lines
Also, I'm not sure about the control framework setting read-only control. According to the code, it doesn't:
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ctrls.c#L2477
If you set the control using e.g. v4l2_ctrl_s_ctrl(), it should end up to the driver's s_ctrl callback.
Signature · 4 lines
Regards, Sakari Ailus e-mail: [email protected]
Hi Tomasz, Sakari,
Thanks for your kindly comments. We will have an internal discussion on VBLANK control implementation to let it be read-only.
And for test pattern, we will definitely implement it but will remove the item from v4l2 menu first.
However, since in the early stage, we found an issue that if not register TEST_PATTERN V4L2 item in kernel, HAL will crash soon when open camera. We would like to resolve the issue both in HAL and kernel (removing test pattern) first.
For test pattern implementation on imx258, it must be needed due to cros-camera-test demands it. Will complete and submit it after full internal verification.
Regards, Andy
-----Original Message-----
From: Sakari Ailus [mailto:[email protected]]
Sent: Friday, March 23, 2018 10:30 PM
To: Tomasz Figa <[email protected]>
Cc: Sakari Ailus <[email protected]>; Yeh, Andy <[email protected]>; Linux Media Mailing List <[email protected]>; Chen, JasonX Z <[email protected]>; Chiang, AlanX <[email protected]>; Lai, Jim <[email protected]>
Subject: Re: [PATCH v9.1] media: imx258: Add imx258 camera sensor driver
Quoted 20 lines · Tomasz Figa
Quoted 18 lines · Sakari Ailus
Hi Tomasz,
Quoted 15 lines · Tomasz Figa
Hi Andy,
Some issues found when reviewing cherry pick of this patch to Chrome OS kernel. Please see inline.
On Sat, Mar 17, 2018 at 1:38 AM, Andy Yeh <[email protected]> wrote:
[snip]
Quoted 5 lines
drivers/media/i2c/imx258.c new file +5
-0+case V4L2_CID_VBLANK : +/* +* Auto Frame Length Line Control is enabled by default. +* Not need control Vblank Register. +*/
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
What is the meaning of this control then? Should it be read-only?
The read-only flag is for the uAPI; the control framework still passes through changes to the control value done using kAPI to the driver.
The read-only flag is not even set in current code.
Ah, you're right, it's just hblank... but if the driver doesn't support setting this control, then it should most likely be read-only. It would seem like that the driver just updates the control to convey the value to the user.
Quoted 5 lines
Also, I'm not sure about the control framework setting read-only control. According to the code, it doesn't:
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core
/v4l2-ctrls.c#L2477
If you set the control using e.g. v4l2_ctrl_s_ctrl(), it should end up to the driver's s_ctrl callback.
--
Regards,
Sakari Ailus
e-mail: [email protected]