media: Add Sony IMX585 image sensor support
Subsystems (3)
v3 cover media: Add Sony IMX585 image sensor support
Hi all,
This is v3 of the IMX585 driver.
Changes in v3:
- Remove ClearHDR support.
- Remove HCG/LCG switch support.
- Use full parts number as compatible string.
- dt-bindings: fixing sync mode values.
Mainly the change is to remove the need for driver specific V4L2 Controls given the review comments in v2.
Features
==========
* 4-lane or 2-lane MIPI-CSI-2 up to 2079 Mbps/lane
* 4 K and 1080p @ 60 fps 12-bit mode, monochrome variant.
* Blacklevel adjustments through V4L2_CID_BRIGHTNESS.
* Multi Camera synchronization mode support.
Testing
==========
- Platform: Raspberry Pi 5 and Raspberry Pi 4B, 4/2 lanes
Series layout
=============
1. **dt-bindings: media: Add Sony IMX585 CMOS image sensor**
3. **media: i2c: imx585: Add Sony IMX585 image-sensor driver**
v2 Link: https://lore.kernel.org/linux-media/[email protected]/
v1 Link: https://lore.kernel.org/linux-media/[email protected]/
Thanks for reviewing!
Will Whang (2):
dt-bindings: media: Add Sony IMX585 CMOS image sensor
media: i2c: imx585: Add Sony IMX585 image-sensor driver
.../bindings/media/i2c/sony,imx585.yaml | 114 ++ MAINTAINERS | 7 + drivers/media/i2c/Kconfig | 9 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx585.c | 1358 +++++++++++++++++ 5 files changed, 1489 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml create mode 100644 drivers/media/i2c/imx585.c
Signature · 1 line
2.39.5
PATCH v3 1/2 dt-bindings: media: Add Sony IMX585 CMOS image sensor
Document the devicetree binding for the Sony IMX585. The schema covers the CSI-2 data-lanes,
and the synchronization mode properties used by the driver.
Compatible strings are now using the full parts number given the review comments from patch v2.
--- .../bindings/media/i2c/sony,imx585.yaml | 114 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 120 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml
Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +114 -0
@@ -0,0 +1,114 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/sony,imx585.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sony IMX585 CMOS image sensor + +maintainers: + - Will Whang <[email protected]> + +description: + IMX585 sensor is a Sony CMOS sensor with 4K and FHD outputs. + The driver supports both imx585-aaqj1 and imx585-aamj1. + +properties: + compatible: + enum: + - sony,imx585-aamj1 + - sony,imx585-aaqj1 + + reg: + maxItems: 1 + + clocks: + description: Clock frequency 74.25MHz, 37.125MHz, 72MHz, 27MHz, 24MHz + maxItems: 1 + + vana-supply: + description: Analog power supply (3.3V) + + vddl-supply: + description: Interface power supply (1.8V) + + vdig-supply: + description: Digital power supply (1.1V) + + reset-gpios: + description: Sensor reset (XCLR) GPIO + maxItems: 1 + + sony,sync-mode: + description: | + Select the sensor synchronisation mode. + - internal-leader (sensor drives XVS/XHS) [default] + - internal-follower (internal clock, external XVS input) + - external (sensor follows external XVS/XHS) + enum: + - internal-leader + - internal-follower + - external + default: internal-leader + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + oneOf: + - items: + - const: 1 + - const: 2 + - items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - port + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + imx585@1a { + compatible = "sony,imx585-aaqj1"; + reg = <0x1a>; + clocks = <&imx585_clk>; + + assigned-clocks = <&imx585_clk>; + assigned-clock-rates = <24000000>; + + vana-supply = <&camera_vadd_3v3>; + vdig-supply = <&camera_vdd1_1v8>; + vddl-supply = <&camera_vdd2_1v1>; + + port { + imx585: endpoint { + remote-endpoint = <&cam>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <720000000>; + }; + }; + }; + }; +...
MAINTAINERS +6 -0
@@ -23535,6 +23535,12 @@ T: git git://linuxtv.org/media.git F: Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml F: drivers/media/i2c/imx415.c +SONY IMX585 SENSOR DRIVER +M: Will Whang <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml + SONY MEMORYSTICK SUBSYSTEM M: Maxim Levitsky <[email protected]> M: Alex Dubov <[email protected]>
Signature · 1 line
2.39.5
Re: [PATCH v3 1/2] dt-bindings: media: Add Sony IMX585 CMOS image sensor
Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml line 93
Quoted 7 lines · Will Whang
Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml new file +7
-0+examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + imx585@1a {
You keep ignoring v1 review, then v2 review and sending the same.
Respond to entire feedback you received instead of ignoring it.
Best regards,
Krzysztof
PATCH v3 2/2 media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Will WhangImplements support for:
* 4-lane / 2-lane CSI-2
* 12-bit linear, 4K/FHD mode.
* Monochrome variant.
* Tested on Raspberry Pi 4/5 with 24 MHz clock.
--- MAINTAINERS | 1 + drivers/media/i2c/Kconfig | 9 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx585.c | 1358 ++++++++++++++++++++++++++++++++++++ 4 files changed, 1369 insertions(+) create mode 100644 drivers/media/i2c/imx585.c
MAINTAINERS +1 -0
@@ -23540,6 +23540,7 @@ M: Will Whang <[email protected]> L: [email protected] S: Maintained F: Documentation/devicetree/bindings/media/i2c/sony,imx585.yaml +F: drivers/media/i2c/imx585.c SONY MEMORYSTICK SUBSYSTEM M: Maxim Levitsky <[email protected]>
drivers/media/i2c/Kconfig +9 -0
@@ -267,6 +267,15 @@ config VIDEO_IMX415 To compile this driver as a module, choose M here: the module will be called imx415. +config VIDEO_IMX585 + tristate "Sony IMX585 sensor support" + help + This is a Video4Linux2 sensor driver for the Sony + IMX585 camera. + + To compile this driver as a module, choose M here: the + module will be called imx585. + config VIDEO_MAX9271_LIB tristate
drivers/media/i2c/Makefile +1 -0
@@ -59,6 +59,7 @@ obj-$(CONFIG_VIDEO_IMX335) += imx335.o obj-$(CONFIG_VIDEO_IMX355) += imx355.o obj-$(CONFIG_VIDEO_IMX412) += imx412.o obj-$(CONFIG_VIDEO_IMX415) += imx415.o +obj-$(CONFIG_VIDEO_IMX585) += imx585.o obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o obj-$(CONFIG_VIDEO_ISL7998X) += isl7998x.o obj-$(CONFIG_VIDEO_KS0127) += ks0127.o
drivers/media/i2c/imx585.c new file +1358 -0
1,358 lines not on this page · raw message
Signature · 1 line
2.39.5
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +
drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);
Not much improved. Don't debug V4L2 calls.
I already commented on this and you just send simialr code. Drop this completely.
Quoted 386 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;
drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );
NAK, I wrote you this is broken and you just ignored and sending the same.
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it to dev_dbg instead of removing them, if you think this is too noisy, then just don't turn on debugging.
Quoted 400 lines
Quoted 387 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how imx219.c works, ask Sony don't ask me.
That is why I ignore your comments on this.
Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.
Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.
So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 475 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
Quoted 8 lines
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
What on earth are you talking about?
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
imx274.c:
/*
* imx274_reset - Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.
*
*/
static void imx274_reset(struct stimx274 *priv, int rst)
{gpiod_set_value_cansleep(priv->reset_gpio, 0); usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); gpiod_set_value_cansleep(priv->reset_gpio, !!rst); usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
}
imx283.c:
/* Power/clock management functions */ static int imx283_power_on(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx283 *imx283 = to_imx283(sd); int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(imx283_supply_name),
imx283->supplies);
if (ret) {dev_err(imx283->dev, "failed to enable regulators\n"); return ret;
}
ret = clk_prepare_enable(imx283->xclk); if (ret) {dev_err(imx283->dev, "failed to enable clock\n"); goto reg_off;
}
gpiod_set_value_cansleep(imx283->reset_gpio, 0);
usleep_range(IMX283_XCLR_MIN_DELAY_US,
IMX283_XCLR_MIN_DELAY_US + IMX283_XCLR_DELAY_RANGE_US);
return 0;
reg_off:regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies); return ret;
}
static int imx283_power_off(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx283 *imx283 = to_imx283(sd);
gpiod_set_value_cansleep(imx283->reset_gpio, 1); regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies); clk_disable_unprepare(imx283->xclk);
return 0;
}
imx334.c:
/**
* imx334_power_on() - Sensor power on sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx334_power_on(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx334 *imx334 = to_imx334(sd); int ret;
gpiod_set_value_cansleep(imx334->reset_gpio, 1);
ret = clk_prepare_enable(imx334->inclk); if (ret) {dev_err(imx334->dev, "fail to enable inclk\n"); goto error_reset;
}
usleep_range(18000, 20000);
return 0;
error_reset:gpiod_set_value_cansleep(imx334->reset_gpio, 0);
return ret;
}
/**
* imx334_power_off() - Sensor power off sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx334_power_off(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx334 *imx334 = to_imx334(sd);
gpiod_set_value_cansleep(imx334->reset_gpio, 0);
clk_disable_unprepare(imx334->inclk);
return 0;
}
imx335.c:
/**
* imx335_power_on() - Sensor power on sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx335_power_on(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx335 *imx335 = to_imx335(sd); int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(imx335_supply_name),
imx335->supplies);
if (ret) {dev_err(dev, "%s: failed to enable regulators\n", __func__);
return ret;
}
usleep_range(500, 550); /* Tlow */
gpiod_set_value_cansleep(imx335->reset_gpio, 0);
ret = clk_prepare_enable(imx335->inclk); if (ret) {dev_err(imx335->dev, "fail to enable inclk\n"); goto error_reset;
}
usleep_range(20, 22); /* T4 */
return 0;
error_reset:gpiod_set_value_cansleep(imx335->reset_gpio, 1); regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
return ret;
}
/**
* imx335_power_off() - Sensor power off sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx335_power_off(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx335 *imx335 = to_imx335(sd);
gpiod_set_value_cansleep(imx335->reset_gpio, 1); clk_disable_unprepare(imx335->inclk); regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
return 0;
}
imx412.c
/**
* imx412_power_on() - Sensor power on sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx412_power_on(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx412 *imx412 = to_imx412(sd); int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(imx412_supply_names),
imx412->supplies);
if (ret < 0) {dev_err(dev, "failed to enable regulators\n"); return ret;
}
gpiod_set_value_cansleep(imx412->reset_gpio, 0);
ret = clk_prepare_enable(imx412->inclk); if (ret) {dev_err(imx412->dev, "fail to enable inclk\n"); goto error_reset;
}
usleep_range(1000, 1200);
return 0;
error_reset:gpiod_set_value_cansleep(imx412->reset_gpio, 1); regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
imx412->supplies);
return ret;
}
/**
* imx412_power_off() - Sensor power off sequence
* @dev: pointer to i2c device
*
* Return: 0 if successful, error code otherwise. */
static int imx412_power_off(struct device *dev) {
struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx412 *imx412 = to_imx412(sd);
clk_disable_unprepare(imx412->inclk);
gpiod_set_value_cansleep(imx412->reset_gpio, 1);
regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
imx412->supplies);
return 0;
}
imx415.c:
static int imx415_power_on(struct imx415 *sensor) {
int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
sensor->supplies);
if (ret < 0)
return ret;
gpiod_set_value_cansleep(sensor->reset, 0);
udelay(1);
ret = clk_prepare_enable(sensor->clk); if (ret < 0)
goto err_reset;
/*
* Data sheet states that 20 us are required before communication start,
* but this doesn't work in all cases. Use 100 us to be on the safe
* side.
*/usleep_range(100, 200);
return 0;
err_reset:gpiod_set_value_cansleep(sensor->reset, 1); regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies); return ret;
}
static void imx415_power_off(struct imx415 *sensor)
{clk_disable_unprepare(sensor->clk); gpiod_set_value_cansleep(sensor->reset, 1); regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies); }
Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 479 lines · Will Whang
Quoted 478 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Quoted 7 lines
Quoted 5 lines
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Do you understand the difference betweeen logical level and line level?
Quoted 5 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.
Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }
Quoted 2 lines
imx283.c:
Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 538 lines · Krzysztof Kozlowski
Quoted 479 lines · Will Whang
Quoted 478 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
drivers/media/i2c/imx585.c new file +386
-0+ ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : { +u16 blacklevel =min_t (u32, ctrl->val,4095 ); + + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = +v4l2_ctrl_new_int_menu (hdl, &imx585_ctrl_ops,V4L2_CID_LINK_FREQ , +0 ,0 , &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_BRIGHTNESS ,0 ,0xFFFF ,1 , +IMX585_BLKLEVEL_DEFAULT ); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_mbus_code_enum *code) +{ +struct imx585 *imx585 =to_imx585 (sd); +unsigned int entries; +const u32 *tbl; + +if (imx585->mono) { +if (code->index) +return -EINVAL ; + code->code =MEDIA_BUS_FMT_Y12_1X12 ; +return 0 ; + } + + tbl = color_codes; + entries =ARRAY_SIZE (color_codes) /4 ; + +if (code->index >= entries) +return -EINVAL ; + + code->code =imx585_get_format_code (imx585, tbl[code->index *4 ]); +return 0 ; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_frame_size_enum *fse) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list; +unsigned int num_modes; + +get_mode_table (imx585, fse->code, &mode_list, &num_modes); +if (fse->index >= num_modes) +return -EINVAL ; +if (fse->code !=imx585_get_format_code (imx585, fse->code)) +return -EINVAL ; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + +return 0 ; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, +struct v4l2_subdev_state *sd_state, +struct v4l2_subdev_format *fmt) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +unsigned int num_modes; +struct v4l2_mbus_framefmt *format; + +get_mode_table (imx585, fmt->format.code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field =V4L2_FIELD_NONE ; + fmt->format.colorspace =V4L2_COLORSPACE_RAW ; + fmt->format.ycbcr_enc =V4L2_YCBCR_ENC_601 ; + fmt->format.quantization =V4L2_QUANTIZATION_FULL_RANGE ; + fmt->format.xfer_func =V4L2_XFER_FUNC_NONE ; + + format =v4l2_subdev_state_get_format (sd_state,0 ); + +if (fmt->which ==V4L2_SUBDEV_FORMAT_ACTIVE ) +imx585_set_framing_limits (imx585, mode); + + *format = fmt->format; +return 0 ; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +const struct imx585_mode *mode_list, *mode; +struct v4l2_subdev_state *st; +struct v4l2_mbus_framefmt *fmt; +unsigned int n_modes; +int ret; + + ret =pm_runtime_get_sync (imx585->clientdev); +if (ret <0 ) { +pm_runtime_put_noidle (imx585->clientdev); +return ret; + } + + ret =cci_multi_reg_write (imx585->regmap, common_regs, +ARRAY_SIZE (common_regs),NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write common settings\n" ); + goto err_rpm_put; + } + + ret =cci_write (imx585->regmap,IMX585_INCK_SEL , imx585->inck_sel_val,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL ,IMX585_BLKLEVEL_DEFAULT ,NULL ); +if (!ret) + ret =cci_write (imx585->regmap,IMX585_DATARATE_SEL , + link_freqs_reg_value[imx585->link_freq_idx],NULL ); +if (ret) + goto err_rpm_put; + + ret =cci_write (imx585->regmap,IMX585_LANEMODE , + (imx585->lane_count ==2 ) ?0x01 :0x03 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret =cci_write (imx585->regmap,IMX585_BIN_MODE , imx585->mono ?0x01 :0x00 ,NULL ); +if (ret) + goto err_rpm_put; + +/* Sync configuration */ +if (imx585->sync_mode ==SYNC_INT_FOLLOWER ) { +dev_dbg (imx585->clientdev,"Internal sync follower: XVS input\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x01 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x03 ,NULL );/* XHS out, XVS in */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x08 ,NULL );/* disable XVS OUT */ + }else if (imx585->sync_mode ==SYNC_INT_LEADER ) { +dev_dbg (imx585->clientdev,"Internal sync leader: XVS/XHS output\n" ); +cci_write (imx585->regmap,IMX585_REG_EXTMODE ,0x00 ,NULL ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x00 ,NULL );/* XHS/XVS out */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x0A ,NULL ); + }else { +dev_dbg (imx585->clientdev,"Follower: XVS/XHS input\n" ); +cci_write (imx585->regmap,IMX585_REG_XXS_DRV ,0x0F ,NULL );/* inputs */ +cci_write (imx585->regmap,IMX585_REG_XXS_OUTSEL ,0x00 ,NULL ); + } + + imx585->common_regs_written = true; + +/* Select mode */ + st =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (st,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &n_modes); + mode =v4l2_find_nearest_size (mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret =cci_multi_reg_write (imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs,NULL ); +if (ret) { +dev_err (imx585->clientdev,"Failed to write mode registers\n" ); + goto err_rpm_put; + } + +/* Disable digital clamp */ +cci_write (imx585->regmap,IMX585_REG_DIGITAL_CLAMP ,0x00 ,NULL ); + +/* Apply user controls after writing the base tables */ + ret =__v4l2_ctrl_handler_setup (imx585->sd.ctrl_handler); +if (ret) { +dev_err (imx585->clientdev,"Control handler setup failed\n" ); + goto err_rpm_put; + } + +if (imx585->sync_mode !=SYNC_EXTERNAL ) +cci_write (imx585->regmap,IMX585_REG_XMSTA ,0x00 ,NULL ); + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STREAMING ,NULL ); +if (ret) + goto err_rpm_put; + +dev_dbg (imx585->clientdev,"Streaming started\n" ); +usleep_range (IMX585_STREAM_DELAY_US , +IMX585_STREAM_DELAY_US +IMX585_STREAM_DELAY_RANGE_US ); + +/* vflip, hflip cannot change during streaming */ +__v4l2_ctrl_grab (imx585->vflip, true); +__v4l2_ctrl_grab (imx585->hflip, true); + +return 0 ; + +err_rpm_put: +pm_runtime_put_autosuspend (imx585->clientdev); +return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, +struct v4l2_subdev_state *state,u32 pad, +u64 streams_mask) +{ +struct imx585 *imx585 =to_imx585 (sd); +int ret; + + ret =cci_write (imx585->regmap,IMX585_REG_MODE_SELECT ,IMX585_MODE_STANDBY ,NULL ); +if (ret) +dev_err (imx585->clientdev,"Failed to stop streaming\n" ); + +__v4l2_ctrl_grab (imx585->vflip, false); +__v4l2_ctrl_grab (imx585->hflip, false); + +pm_runtime_put_autosuspend (imx585->clientdev); + +return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); +int ret; + +dev_dbg (imx585->clientdev,"power_on\n" ); + + ret =regulator_bulk_enable (IMX585_NUM_SUPPLIES , imx585->supplies); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable regulators\n" ); +return ret; + } + + ret =clk_prepare_enable (imx585->xclk); +if (ret) { +dev_err (imx585->clientdev,"Failed to enable clock\n" ); + goto reg_off; + } + +gpiod_set_value_cansleep (imx585->reset_gpio,1 ); +usleep_range (IMX585_XCLR_MIN_DELAY_US , +IMX585_XCLR_MIN_DELAY_US +IMX585_XCLR_DELAY_RANGE_US ); +return 0 ; + +reg_off: +regulator_bulk_disable (IMX585_NUM_SUPPLIES , imx585->supplies); +return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ +struct v4l2_subdev *sd =dev_get_drvdata (dev); +struct imx585 *imx585 =to_imx585 (sd); + +dev_dbg (imx585->clientdev,"power_off\n" ); + +gpiod_set_value_cansleep (imx585->reset_gpio,0 );NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Quoted 7 lines
Quoted 5 lines
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Do you understand the difference betweeen logical level and line level?
Quoted 5 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }Quoted 2 lines
imx283.c:Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);
Quoted 5 lines
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
I can apply the same logic and say this is buggy and wrong.
Do you understand this is writing the GPIO directly and has nothing to do with what you think it should be?
Ask Sony why they use logic high = normal mode and logic low = reset.
Quote your previous comments:
Quoted 5 lines
This is not how resets work. Logical reset value high means it is asserted and device does not work.
Read carefully your datasheet. Because if you claim above this is not a reset line, but some other.
imx283.c is the latest one landed in 2024, can you read it carefully and reply again?
Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 484 lines · Will Whang
Quoted 483 lines · Krzysztof Kozlowski
Quoted 481 lines · Will Whang
Quoted 480 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
(quoted diff) +25
-0+ +static int imx585_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); + const struct imx585_mode *mode, *mode_list; + struct v4l2_subdev_state *state; + struct v4l2_mbus_framefmt *fmt; + unsigned int num_modes; + int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(state, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->width, fmt->height); + + /* Apply control only when powered (runtime active). */ + if (!pm_runtime_get_if_active(imx585->clientdev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: { + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */ +Review
Quoted 10 lines
(quoted diff) +10
-0+ dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n", + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret); + break; + } + case V4L2_CID_ANALOGUE_GAIN: + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
(quoted diff) +386
-0+ ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret); + break; + case V4L2_CID_VBLANK: { + u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U; + + current_exposure = clamp_t(u32, current_exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN); + __v4l2_ctrl_modify_range(imx585->exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1, + current_exposure); + + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax); + + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HBLANK: { + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE; + u64 hmax; + + do_div(pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE; + do_div(hmax, pixel_rate); + imx585->hmax = (u32)hmax; + + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_VFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_BRIGHTNESS: { + u16 blacklevel = min_t(u32, ctrl->val, 4095); + + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret); + break; + } + default: + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n", + ctrl->name, ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(imx585->clientdev); + return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls(struct imx585 *imx585) +{ + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; + struct v4l2_fwnode_device_properties props; + int ret; + + ret = v4l2_ctrl_handler_init(hdl, 16); + + /* Read-only, updated per mode */ + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_PIXEL_RATE, + 1, UINT_MAX, 1, 1); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ, + 0, 0, &link_freqs[imx585->link_freq_idx]); + if (imx585->link_freq) + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0); + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1, + IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX, + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT); + + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX, + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT); + + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + + if (hdl->error) { + ret = hdl->error; + dev_err(imx585->clientdev, "control init failed (%d)\n", ret); + goto err_free; + } + + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props); + if (ret) + goto err_free; + + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props); + if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; + return 0; + +err_free: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static void imx585_free_controls(struct imx585 *imx585) +{ + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- + * Pad ops / formats + * -------------------------------------------------------------------------- + */ + +static int imx585_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx585 *imx585 = to_imx585(sd); + unsigned int entries; + const u32 *tbl; + + if (imx585->mono) { + if (code->index) + return -EINVAL; + code->code = MEDIA_BUS_FMT_Y12_1X12; + return 0; + } + + tbl = color_codes; + entries = ARRAY_SIZE(color_codes) / 4; + + if (code->index >= entries) + return -EINVAL; + + code->code = imx585_get_format_code(imx585, tbl[code->index * 4]); + return 0; +} + +static int imx585_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list; + unsigned int num_modes; + + get_mode_table(imx585, fse->code, &mode_list, &num_modes); + if (fse->index >= num_modes) + return -EINVAL; + if (fse->code != imx585_get_format_code(imx585, fse->code)) + return -EINVAL; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int imx585_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + unsigned int num_modes; + struct v4l2_mbus_framefmt *format; + + get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601; + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; + + format = v4l2_subdev_state_get_format(sd_state, 0); + + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) + imx585_set_framing_limits(imx585, mode); + + *format = fmt->format; + return 0; +} + +/* -------------------------------------------------------------------------- + * Stream on/off + * -------------------------------------------------------------------------- + */ + +static int imx585_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + struct v4l2_subdev_state *st; + struct v4l2_mbus_framefmt *fmt; + unsigned int n_modes; + int ret; + + ret = pm_runtime_get_sync(imx585->clientdev); + if (ret < 0) { + pm_runtime_put_noidle(imx585->clientdev); + return ret; + } + + ret = cci_multi_reg_write(imx585->regmap, common_regs, + ARRAY_SIZE(common_regs), NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write common settings\n"); + goto err_rpm_put; + } + + ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL, + link_freqs_reg_value[imx585->link_freq_idx], NULL); + if (ret) + goto err_rpm_put; + + ret = cci_write(imx585->regmap, IMX585_LANEMODE, + (imx585->lane_count == 2) ? 0x01 : 0x03, NULL); + if (ret) + goto err_rpm_put; + + /* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL); + if (ret) + goto err_rpm_put; + + /* Sync configuration */ + if (imx585->sync_mode == SYNC_INT_FOLLOWER) { + dev_dbg(imx585->clientdev, "Internal sync follower: XVS input\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */ + } else if (imx585->sync_mode == SYNC_INT_LEADER) { + dev_dbg(imx585->clientdev, "Internal sync leader: XVS/XHS output\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL); + } else { + dev_dbg(imx585->clientdev, "Follower: XVS/XHS input\n"); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL); + } + + imx585->common_regs_written = true; + + /* Select mode */ + st = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(st, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &n_modes); + mode = v4l2_find_nearest_size(mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs, NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write mode registers\n"); + goto err_rpm_put; + } + + /* Disable digital clamp */ + cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL); + + /* Apply user controls after writing the base tables */ + ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler); + if (ret) { + dev_err(imx585->clientdev, "Control handler setup failed\n"); + goto err_rpm_put; + } + + if (imx585->sync_mode != SYNC_EXTERNAL) + cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL); + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL); + if (ret) + goto err_rpm_put; + + dev_dbg(imx585->clientdev, "Streaming started\n"); + usleep_range(IMX585_STREAM_DELAY_US, + IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US); + + /* vflip, hflip cannot change during streaming */ + __v4l2_ctrl_grab(imx585->vflip, true); + __v4l2_ctrl_grab(imx585->hflip, true); + + return 0; + +err_rpm_put: + pm_runtime_put_autosuspend(imx585->clientdev); + return ret; +} + +static int imx585_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + int ret; + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL); + if (ret) + dev_err(imx585->clientdev, "Failed to stop streaming\n"); + + __v4l2_ctrl_grab(imx585->vflip, false); + __v4l2_ctrl_grab(imx585->hflip, false); + + pm_runtime_put_autosuspend(imx585->clientdev); + + return ret; +} + +/* -------------------------------------------------------------------------- + * Power / runtime PM + * -------------------------------------------------------------------------- + */ + +static int imx585_power_on(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + int ret; + + dev_dbg(imx585->clientdev, "power_on\n"); + + ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable regulators\n"); + return ret; + } + + ret = clk_prepare_enable(imx585->xclk); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable clock\n"); + goto reg_off; + } + + gpiod_set_value_cansleep(imx585->reset_gpio, 1); + usleep_range(IMX585_XCLR_MIN_DELAY_US, + IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US); + return 0; + +reg_off: + regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies); + return ret;Review
Quoted 10 lines
(quoted diff) +10
-0+} + +static int imx585_power_off(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + + dev_dbg(imx585->clientdev, "power_off\n"); + + gpiod_set_value_cansleep(imx585->reset_gpio, 0);NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Respond here
Quoted 2 lines
Quoted 2 lines
Do you understand the difference betweeen logical level and line level?
Respond here
Quoted 46 lines
Quoted 46 lines
Quoted 6 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }Quoted 2 lines
imx283.c:Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);
Quoted 5 lines
Quoted 5 lines
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
So you do not understand above and yet you keep arguing with maintainer.
Quoted 3 lines
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
And? All other drivers, camera sensors, hwmon, iio, codecs and whatnot?
Quoted 1 line
I can apply the same logic and say this is buggy and wrong.
We are not going to talk imaginary things.
Quoted 2 lines
Do you understand this is writing the GPIO directly and has nothing to
It is not. Again, you are mixing logical level with line level. You never responded to that part, you never used actual arguments except some vague statements like above.
You do not write GPIO directly.
Each driver is supposed to use logical level.
Quoted 2 lines
do with what you think it should be?
Ask Sony why they use logic high = normal mode and logic low = reset.
Again you are mixing knowledge. Line level is completely irrelevant here. 99% of devices has active low reset line.
Quoted 10 lines
Quote your previous comments:
Quoted 5 lines
This is not how resets work. Logical reset value high means it is asserted and device does not work.
Read carefully your datasheet. Because if you claim above this is not a reset line, but some other.
imx283.cis the latest one landed in 2024, can you read it carefully and reply again?
I will not because arguments "I found such code somewhere, so I will not respond to actual arguments just use that code" are invalid.
You cannot make reset asserted and claim "this is operating stage". I explained why: because it does not work with correct DTS. If it works for you, then because your DTS is not correct.
Apparently you do not understand what is assertion and what is logical state of GPIO (I asked this three times), but you keep disagreeing. That's basic knowledge, so please kindly go to Wikipedia or stackoverflow, because you are now wasting reviewers time. You do not respond to reviewers arguments.
As I said: code is wrong so NAK.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 601 lines · Krzysztof Kozlowski
Quoted 484 lines · Will Whang
Quoted 483 lines · Krzysztof Kozlowski
Quoted 481 lines · Will Whang
Quoted 480 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
(quoted diff) +25
-0+ +static int imx585_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); + const struct imx585_mode *mode, *mode_list; + struct v4l2_subdev_state *state; + struct v4l2_mbus_framefmt *fmt; + unsigned int num_modes; + int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(state, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->width, fmt->height); + + /* Apply control only when powered (runtime active). */ + if (!pm_runtime_get_if_active(imx585->clientdev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: { + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */ +Review
Quoted 10 lines
(quoted diff) +10
-0+ dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n", + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret); + break; + } + case V4L2_CID_ANALOGUE_GAIN: + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
(quoted diff) +386
-0+ ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret); + break; + case V4L2_CID_VBLANK: { + u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U; + + current_exposure = clamp_t(u32, current_exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN); + __v4l2_ctrl_modify_range(imx585->exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1, + current_exposure); + + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax); + + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HBLANK: { + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE; + u64 hmax; + + do_div(pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE; + do_div(hmax, pixel_rate); + imx585->hmax = (u32)hmax; + + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_VFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_BRIGHTNESS: { + u16 blacklevel = min_t(u32, ctrl->val, 4095); + + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret); + break; + } + default: + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n", + ctrl->name, ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(imx585->clientdev); + return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls(struct imx585 *imx585) +{ + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; + struct v4l2_fwnode_device_properties props; + int ret; + + ret = v4l2_ctrl_handler_init(hdl, 16); + + /* Read-only, updated per mode */ + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_PIXEL_RATE, + 1, UINT_MAX, 1, 1); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ, + 0, 0, &link_freqs[imx585->link_freq_idx]); + if (imx585->link_freq) + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0); + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1, + IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX, + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT); + + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX, + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT); + + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + + if (hdl->error) { + ret = hdl->error; + dev_err(imx585->clientdev, "control init failed (%d)\n", ret); + goto err_free; + } + + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props); + if (ret) + goto err_free; + + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props); + if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; + return 0; + +err_free: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static void imx585_free_controls(struct imx585 *imx585) +{ + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- + * Pad ops / formats + * -------------------------------------------------------------------------- + */ + +static int imx585_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx585 *imx585 = to_imx585(sd); + unsigned int entries; + const u32 *tbl; + + if (imx585->mono) { + if (code->index) + return -EINVAL; + code->code = MEDIA_BUS_FMT_Y12_1X12; + return 0; + } + + tbl = color_codes; + entries = ARRAY_SIZE(color_codes) / 4; + + if (code->index >= entries) + return -EINVAL; + + code->code = imx585_get_format_code(imx585, tbl[code->index * 4]); + return 0; +} + +static int imx585_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list; + unsigned int num_modes; + + get_mode_table(imx585, fse->code, &mode_list, &num_modes); + if (fse->index >= num_modes) + return -EINVAL; + if (fse->code != imx585_get_format_code(imx585, fse->code)) + return -EINVAL; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int imx585_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + unsigned int num_modes; + struct v4l2_mbus_framefmt *format; + + get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601; + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; + + format = v4l2_subdev_state_get_format(sd_state, 0); + + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) + imx585_set_framing_limits(imx585, mode); + + *format = fmt->format; + return 0; +} + +/* -------------------------------------------------------------------------- + * Stream on/off + * -------------------------------------------------------------------------- + */ + +static int imx585_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + struct v4l2_subdev_state *st; + struct v4l2_mbus_framefmt *fmt; + unsigned int n_modes; + int ret; + + ret = pm_runtime_get_sync(imx585->clientdev); + if (ret < 0) { + pm_runtime_put_noidle(imx585->clientdev); + return ret; + } + + ret = cci_multi_reg_write(imx585->regmap, common_regs, + ARRAY_SIZE(common_regs), NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write common settings\n"); + goto err_rpm_put; + } + + ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL, + link_freqs_reg_value[imx585->link_freq_idx], NULL); + if (ret) + goto err_rpm_put; + + ret = cci_write(imx585->regmap, IMX585_LANEMODE, + (imx585->lane_count == 2) ? 0x01 : 0x03, NULL); + if (ret) + goto err_rpm_put; + + /* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL); + if (ret) + goto err_rpm_put; + + /* Sync configuration */ + if (imx585->sync_mode == SYNC_INT_FOLLOWER) { + dev_dbg(imx585->clientdev, "Internal sync follower: XVS input\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */ + } else if (imx585->sync_mode == SYNC_INT_LEADER) { + dev_dbg(imx585->clientdev, "Internal sync leader: XVS/XHS output\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL); + } else { + dev_dbg(imx585->clientdev, "Follower: XVS/XHS input\n"); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL); + } + + imx585->common_regs_written = true; + + /* Select mode */ + st = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(st, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &n_modes); + mode = v4l2_find_nearest_size(mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs, NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write mode registers\n"); + goto err_rpm_put; + } + + /* Disable digital clamp */ + cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL); + + /* Apply user controls after writing the base tables */ + ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler); + if (ret) { + dev_err(imx585->clientdev, "Control handler setup failed\n"); + goto err_rpm_put; + } + + if (imx585->sync_mode != SYNC_EXTERNAL) + cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL); + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL); + if (ret) + goto err_rpm_put; + + dev_dbg(imx585->clientdev, "Streaming started\n"); + usleep_range(IMX585_STREAM_DELAY_US, + IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US); + + /* vflip, hflip cannot change during streaming */ + __v4l2_ctrl_grab(imx585->vflip, true); + __v4l2_ctrl_grab(imx585->hflip, true); + + return 0; + +err_rpm_put: + pm_runtime_put_autosuspend(imx585->clientdev); + return ret; +} + +static int imx585_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + int ret; + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL); + if (ret) + dev_err(imx585->clientdev, "Failed to stop streaming\n"); + + __v4l2_ctrl_grab(imx585->vflip, false); + __v4l2_ctrl_grab(imx585->hflip, false); + + pm_runtime_put_autosuspend(imx585->clientdev); + + return ret; +} + +/* -------------------------------------------------------------------------- + * Power / runtime PM + * -------------------------------------------------------------------------- + */ + +static int imx585_power_on(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + int ret; + + dev_dbg(imx585->clientdev, "power_on\n"); + + ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable regulators\n"); + return ret; + } + + ret = clk_prepare_enable(imx585->xclk); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable clock\n"); + goto reg_off; + } + + gpiod_set_value_cansleep(imx585->reset_gpio, 1); + usleep_range(IMX585_XCLR_MIN_DELAY_US, + IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US); + return 0; + +reg_off: + regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies); + return ret;Review
Quoted 10 lines
(quoted diff) +10
-0+} + +static int imx585_power_off(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + + dev_dbg(imx585->clientdev, "power_off\n"); + + gpiod_set_value_cansleep(imx585->reset_gpio, 0);NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
But regardless, this is still buggy and this is still NAK.
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Respond here
Quoted 2 lines
Quoted 2 lines
Do you understand the difference betweeen logical level and line level?
Respond here
Quoted 46 lines
Quoted 46 lines
Quoted 6 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }Quoted 2 lines
imx283.c:Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);Quoted 5 lines
Quoted 5 lines
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
So you do not understand above and yet you keep arguing with maintainer.
Quoted 3 lines
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
And? All other drivers, camera sensors, hwmon, iio, codecs and whatnot?
Quoted 1 line
I can apply the same logic and say this is buggy and wrong.
We are not going to talk imaginary things.
Quoted 2 lines
Do you understand this is writing the GPIO directly and has nothing to
It is not. Again, you are mixing logical level with line level. You never responded to that part, you never used actual arguments except some vague statements like above.
You do not write GPIO directly.
Each driver is supposed to use logical level.
Quoted 2 lines
do with what you think it should be?
Ask Sony why they use logic high = normal mode and logic low = reset.Again you are mixing knowledge. Line level is completely irrelevant here. 99% of devices has active low reset line.
Quoted 10 lines
Quote your previous comments:
Quoted 5 lines
This is not how resets work. Logical reset value high means it is asserted and device does not work.
Read carefully your datasheet. Because if you claim above this is not a reset line, but some other.
imx283.cis the latest one landed in 2024, can you read it carefully and reply again?I will not because arguments "I found such code somewhere, so I will not respond to actual arguments just use that code" are invalid.
You cannot make reset asserted and claim "this is operating stage". I explained why: because it does not work with correct DTS. If it works for you, then because your DTS is not correct.
Quoted 7 lines
Apparently you do not understand what is assertion and what is logical state of GPIO (I asked this three times), but you keep disagreeing. That's basic knowledge, so please kindly go to Wikipedia or stackoverflow, because you are now wasting reviewers time. You do not respond to reviewers arguments.
As I said: code is wrong so NAK.
BTW,
I asked some emails ago:
"Do you understand the difference betweeen logical level and line level?"
The question was on purpose, so you will think on the answer. If you thought about this and responded to this after that thinking, that process would lead you to the correct answer and correct code.
You avoid doing research and learning this stuff, which is your right. Just like our right is not to accept buggy code. If you do not want to learn, then implement it like maintainer asks.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Krzysztof KozlowskiQuoted 488 lines · Krzysztof Kozlowski
Quoted 486 lines · Will Whang
Quoted 485 lines · Krzysztof Kozlowski
Quoted 483 lines · Will Whang
Quoted 482 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
(quoted diff) +25
-0+ +static int imx585_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); + const struct imx585_mode *mode, *mode_list; + struct v4l2_subdev_state *state; + struct v4l2_mbus_framefmt *fmt; + unsigned int num_modes; + int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(state, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->width, fmt->height); + + /* Apply control only when powered (runtime active). */ + if (!pm_runtime_get_if_active(imx585->clientdev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: { + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */ +Review
Quoted 10 lines
(quoted diff) +10
-0+ dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n", + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret); + break; + } + case V4L2_CID_ANALOGUE_GAIN: + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
(quoted diff) +386
-0+ ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret); + break; + case V4L2_CID_VBLANK: { + u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U; + + current_exposure = clamp_t(u32, current_exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN); + __v4l2_ctrl_modify_range(imx585->exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1, + current_exposure); + + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax); + + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HBLANK: { + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE; + u64 hmax; + + do_div(pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE; + do_div(hmax, pixel_rate); + imx585->hmax = (u32)hmax; + + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_VFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_BRIGHTNESS: { + u16 blacklevel = min_t(u32, ctrl->val, 4095); + + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret); + break; + } + default: + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n", + ctrl->name, ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(imx585->clientdev); + return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls(struct imx585 *imx585) +{ + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; + struct v4l2_fwnode_device_properties props; + int ret; + + ret = v4l2_ctrl_handler_init(hdl, 16); + + /* Read-only, updated per mode */ + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_PIXEL_RATE, + 1, UINT_MAX, 1, 1); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ, + 0, 0, &link_freqs[imx585->link_freq_idx]); + if (imx585->link_freq) + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0); + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1, + IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX, + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT); + + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX, + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT); + + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + + if (hdl->error) { + ret = hdl->error; + dev_err(imx585->clientdev, "control init failed (%d)\n", ret); + goto err_free; + } + + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props); + if (ret) + goto err_free; + + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props); + if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; + return 0; + +err_free: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static void imx585_free_controls(struct imx585 *imx585) +{ + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- + * Pad ops / formats + * -------------------------------------------------------------------------- + */ + +static int imx585_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx585 *imx585 = to_imx585(sd); + unsigned int entries; + const u32 *tbl; + + if (imx585->mono) { + if (code->index) + return -EINVAL; + code->code = MEDIA_BUS_FMT_Y12_1X12; + return 0; + } + + tbl = color_codes; + entries = ARRAY_SIZE(color_codes) / 4; + + if (code->index >= entries) + return -EINVAL; + + code->code = imx585_get_format_code(imx585, tbl[code->index * 4]); + return 0; +} + +static int imx585_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list; + unsigned int num_modes; + + get_mode_table(imx585, fse->code, &mode_list, &num_modes); + if (fse->index >= num_modes) + return -EINVAL; + if (fse->code != imx585_get_format_code(imx585, fse->code)) + return -EINVAL; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int imx585_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + unsigned int num_modes; + struct v4l2_mbus_framefmt *format; + + get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601; + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; + + format = v4l2_subdev_state_get_format(sd_state, 0); + + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) + imx585_set_framing_limits(imx585, mode); + + *format = fmt->format; + return 0; +} + +/* -------------------------------------------------------------------------- + * Stream on/off + * -------------------------------------------------------------------------- + */ + +static int imx585_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + struct v4l2_subdev_state *st; + struct v4l2_mbus_framefmt *fmt; + unsigned int n_modes; + int ret; + + ret = pm_runtime_get_sync(imx585->clientdev); + if (ret < 0) { + pm_runtime_put_noidle(imx585->clientdev); + return ret; + } + + ret = cci_multi_reg_write(imx585->regmap, common_regs, + ARRAY_SIZE(common_regs), NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write common settings\n"); + goto err_rpm_put; + } + + ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL, + link_freqs_reg_value[imx585->link_freq_idx], NULL); + if (ret) + goto err_rpm_put; + + ret = cci_write(imx585->regmap, IMX585_LANEMODE, + (imx585->lane_count == 2) ? 0x01 : 0x03, NULL); + if (ret) + goto err_rpm_put; + + /* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL); + if (ret) + goto err_rpm_put; + + /* Sync configuration */ + if (imx585->sync_mode == SYNC_INT_FOLLOWER) { + dev_dbg(imx585->clientdev, "Internal sync follower: XVS input\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */ + } else if (imx585->sync_mode == SYNC_INT_LEADER) { + dev_dbg(imx585->clientdev, "Internal sync leader: XVS/XHS output\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL); + } else { + dev_dbg(imx585->clientdev, "Follower: XVS/XHS input\n"); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL); + } + + imx585->common_regs_written = true; + + /* Select mode */ + st = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(st, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &n_modes); + mode = v4l2_find_nearest_size(mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs, NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write mode registers\n"); + goto err_rpm_put; + } + + /* Disable digital clamp */ + cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL); + + /* Apply user controls after writing the base tables */ + ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler); + if (ret) { + dev_err(imx585->clientdev, "Control handler setup failed\n"); + goto err_rpm_put; + } + + if (imx585->sync_mode != SYNC_EXTERNAL) + cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL); + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL); + if (ret) + goto err_rpm_put; + + dev_dbg(imx585->clientdev, "Streaming started\n"); + usleep_range(IMX585_STREAM_DELAY_US, + IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US); + + /* vflip, hflip cannot change during streaming */ + __v4l2_ctrl_grab(imx585->vflip, true); + __v4l2_ctrl_grab(imx585->hflip, true); + + return 0; + +err_rpm_put: + pm_runtime_put_autosuspend(imx585->clientdev); + return ret; +} + +static int imx585_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + int ret; + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL); + if (ret) + dev_err(imx585->clientdev, "Failed to stop streaming\n"); + + __v4l2_ctrl_grab(imx585->vflip, false); + __v4l2_ctrl_grab(imx585->hflip, false); + + pm_runtime_put_autosuspend(imx585->clientdev); + + return ret; +} + +/* -------------------------------------------------------------------------- + * Power / runtime PM + * -------------------------------------------------------------------------- + */ + +static int imx585_power_on(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + int ret; + + dev_dbg(imx585->clientdev, "power_on\n"); + + ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable regulators\n"); + return ret; + } + + ret = clk_prepare_enable(imx585->xclk); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable clock\n"); + goto reg_off; + } + + gpiod_set_value_cansleep(imx585->reset_gpio, 1); + usleep_range(IMX585_XCLR_MIN_DELAY_US, + IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US); + return 0; + +reg_off: + regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies); + return ret;Review
Quoted 10 lines
(quoted diff) +10
-0+} + +static int imx585_power_off(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + + dev_dbg(imx585->clientdev, "power_off\n"); + + gpiod_set_value_cansleep(imx585->reset_gpio, 0);NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Respond here
You send the reply to both V3 and V2, so double reply.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Do you understand the difference betweeen logical level and line level?
Respond here
Do you understand this is writing GPIO? This has never been related to the discussion.
I have kept telling you you got this sensor usage wrong.
Quoted 53 lines
Quoted 47 lines
Quoted 47 lines
Quoted 7 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }Quoted 2 lines
imx283.c:Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);Quoted 5 lines
Quoted 5 lines
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
Quoted 8 lines
So you do not understand above and yet you keep arguing with maintainer.
Quoted 3 lines
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
And? All other drivers, camera sensors, hwmon, iio, codecs and whatnot?
Are those Sony image sensors?
Quoted 5 lines
Quoted 2 lines
I can apply the same logic and say this is buggy and wrong.
We are not going to talk imaginary things.
So you can imagine this code is buggy even though I tell you this is correct? Of course I tested it already.
Quoted 10 lines
Quoted 1 line
Do you understand this is writing the GPIO directly and has nothing to
It is not. Again, you are mixing logical level with line level. You never responded to that part, you never used actual arguments except some vague statements like above.
You do not write GPIO directly.
Each driver is supposed to use logical level.
hmm? Code in the tree disagree with you? Are you going to arguing that all the 90% of Sony image sensor drivers are old and buggy?
For writing GPIO directly, what is devm_gpiod_get_optional for?
Quoted 25 lines
Quoted 3 lines
do with what you think it should be?
Ask Sony why they use logic high = normal mode and logic low = reset.Again you are mixing knowledge. Line level is completely irrelevant here. 99% of devices has active low reset line.
Quoted 10 lines
Quote your previous comments:
Quoted 5 lines
This is not how resets work. Logical reset value high means it is asserted and device does not work.
Read carefully your datasheet. Because if you claim above this is not a reset line, but some other.
imx283.cis the latest one landed in 2024, can you read it carefully and reply again?I will not because arguments "I found such code somewhere, so I will not respond to actual arguments just use that code" are invalid.
You cannot make reset asserted and claim "this is operating stage". I explained why: because it does not work with correct DTS. If it works for you, then because your DTS is not correct.
Quoted 6 lines
Apparently you do not understand what is assertion and what is logical state of GPIO (I asked this three times), but you keep disagreeing. That's basic knowledge, so please kindly go to Wikipedia or stackoverflow, because you are now wasting reviewers time. You do not respond to reviewers arguments.
You are asking me to code a bug in the driver and the arguments don't make sense.
As much as I appreciate your feedback, I want a working driver upstream and will have to point to the existing code base to prove that it works.
I understand your points that you want me to code a bug (from my point of view), I will take that feedback to consider but the only way forward is to remove the optional reset gpio I guess.
Quoted 5 lines
As I said: code is wrong so NAK.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
Quoted 489 lines · Will Whang
Quoted 488 lines · Krzysztof Kozlowski
Quoted 486 lines · Will Whang
Quoted 485 lines · Krzysztof Kozlowski
Quoted 483 lines · Will Whang
Quoted 482 lines · Krzysztof Kozlowski
Quoted 45 lines · Will Whang
Quoted 40 lines · Krzysztof Kozlowski
Quoted 25 lines · Will Whang
(quoted diff) +25
-0+ +static int imx585_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); + const struct imx585_mode *mode, *mode_list; + struct v4l2_subdev_state *state; + struct v4l2_mbus_framefmt *fmt; + unsigned int num_modes; + int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(state, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->width, fmt->height); + + /* Apply control only when powered (runtime active). */ + if (!pm_runtime_get_if_active(imx585->clientdev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: { + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */ +Review
Quoted 10 lines
(quoted diff) +10
-0+ dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n", + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret); + break; + } + case V4L2_CID_ANALOGUE_GAIN: + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 8 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.Because you do not debug useful parts of the driver, but only invocation of v4l2 controls.
Quoted 400 lines
Quoted 400 lines
Quoted 387 lines
(quoted diff) +386
-0+ ret = cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret); + break; + case V4L2_CID_VBLANK: { + u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U; + + current_exposure = clamp_t(u32, current_exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN); + __v4l2_ctrl_modify_range(imx585->exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1, + current_exposure); + + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax); + + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HBLANK: { + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE; + u64 hmax; + + do_div(pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE; + do_div(hmax, pixel_rate); + imx585->hmax = (u32)hmax; + + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_VFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_BRIGHTNESS: { + u16 blacklevel = min_t(u32, ctrl->val, 4095); + + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret); + break; + } + default: + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n", + ctrl->name, ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(imx585->clientdev); + return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls(struct imx585 *imx585) +{ + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; + struct v4l2_fwnode_device_properties props; + int ret; + + ret = v4l2_ctrl_handler_init(hdl, 16); + + /* Read-only, updated per mode */ + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_PIXEL_RATE, + 1, UINT_MAX, 1, 1); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ, + 0, 0, &link_freqs[imx585->link_freq_idx]); + if (imx585->link_freq) + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0); + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1, + IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX, + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT); + + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX, + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT); + + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + + if (hdl->error) { + ret = hdl->error; + dev_err(imx585->clientdev, "control init failed (%d)\n", ret); + goto err_free; + } + + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props); + if (ret) + goto err_free; + + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props); + if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; + return 0; + +err_free: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static void imx585_free_controls(struct imx585 *imx585) +{ + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- + * Pad ops / formats + * -------------------------------------------------------------------------- + */ + +static int imx585_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx585 *imx585 = to_imx585(sd); + unsigned int entries; + const u32 *tbl; + + if (imx585->mono) { + if (code->index) + return -EINVAL; + code->code = MEDIA_BUS_FMT_Y12_1X12; + return 0; + } + + tbl = color_codes; + entries = ARRAY_SIZE(color_codes) / 4; + + if (code->index >= entries) + return -EINVAL; + + code->code = imx585_get_format_code(imx585, tbl[code->index * 4]); + return 0; +} + +static int imx585_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list; + unsigned int num_modes; + + get_mode_table(imx585, fse->code, &mode_list, &num_modes); + if (fse->index >= num_modes) + return -EINVAL; + if (fse->code != imx585_get_format_code(imx585, fse->code)) + return -EINVAL; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int imx585_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + unsigned int num_modes; + struct v4l2_mbus_framefmt *format; + + get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601; + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; + + format = v4l2_subdev_state_get_format(sd_state, 0); + + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) + imx585_set_framing_limits(imx585, mode); + + *format = fmt->format; + return 0; +} + +/* -------------------------------------------------------------------------- + * Stream on/off + * -------------------------------------------------------------------------- + */ + +static int imx585_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + struct v4l2_subdev_state *st; + struct v4l2_mbus_framefmt *fmt; + unsigned int n_modes; + int ret; + + ret = pm_runtime_get_sync(imx585->clientdev); + if (ret < 0) { + pm_runtime_put_noidle(imx585->clientdev); + return ret; + } + + ret = cci_multi_reg_write(imx585->regmap, common_regs, + ARRAY_SIZE(common_regs), NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write common settings\n"); + goto err_rpm_put; + } + + ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL, + link_freqs_reg_value[imx585->link_freq_idx], NULL); + if (ret) + goto err_rpm_put; + + ret = cci_write(imx585->regmap, IMX585_LANEMODE, + (imx585->lane_count == 2) ? 0x01 : 0x03, NULL); + if (ret) + goto err_rpm_put; + + /* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL); + if (ret) + goto err_rpm_put; + + /* Sync configuration */ + if (imx585->sync_mode == SYNC_INT_FOLLOWER) { + dev_dbg(imx585->clientdev, "Internal sync follower: XVS input\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */ + } else if (imx585->sync_mode == SYNC_INT_LEADER) { + dev_dbg(imx585->clientdev, "Internal sync leader: XVS/XHS output\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL); + } else { + dev_dbg(imx585->clientdev, "Follower: XVS/XHS input\n"); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL); + } + + imx585->common_regs_written = true; + + /* Select mode */ + st = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(st, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &n_modes); + mode = v4l2_find_nearest_size(mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs, NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write mode registers\n"); + goto err_rpm_put; + } + + /* Disable digital clamp */ + cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL); + + /* Apply user controls after writing the base tables */ + ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler); + if (ret) { + dev_err(imx585->clientdev, "Control handler setup failed\n"); + goto err_rpm_put; + } + + if (imx585->sync_mode != SYNC_EXTERNAL) + cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL); + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL); + if (ret) + goto err_rpm_put; + + dev_dbg(imx585->clientdev, "Streaming started\n"); + usleep_range(IMX585_STREAM_DELAY_US, + IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US); + + /* vflip, hflip cannot change during streaming */ + __v4l2_ctrl_grab(imx585->vflip, true); + __v4l2_ctrl_grab(imx585->hflip, true); + + return 0; + +err_rpm_put: + pm_runtime_put_autosuspend(imx585->clientdev); + return ret; +} + +static int imx585_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + int ret; + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL); + if (ret) + dev_err(imx585->clientdev, "Failed to stop streaming\n"); + + __v4l2_ctrl_grab(imx585->vflip, false); + __v4l2_ctrl_grab(imx585->hflip, false); + + pm_runtime_put_autosuspend(imx585->clientdev); + + return ret; +} + +/* -------------------------------------------------------------------------- + * Power / runtime PM + * -------------------------------------------------------------------------- + */ + +static int imx585_power_on(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + int ret; + + dev_dbg(imx585->clientdev, "power_on\n"); + + ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable regulators\n"); + return ret; + } + + ret = clk_prepare_enable(imx585->xclk); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable clock\n"); + goto reg_off; + } + + gpiod_set_value_cansleep(imx585->reset_gpio, 1); + usleep_range(IMX585_XCLR_MIN_DELAY_US, + IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US); + return 0; + +reg_off: + regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies); + return ret;Review
Quoted 10 lines
(quoted diff) +10
-0+} + +static int imx585_power_off(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + + dev_dbg(imx585->clientdev, "power_off\n"); + + gpiod_set_value_cansleep(imx585->reset_gpio, 0);NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 7 lines
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.So there is a bug, you claim that you may do the same bug and then say:
Quoted 1 line
That is why I ignore your comments on this.
and ignoring comments that your code is buggy. Great!
If you ever decide to not follow reviewer's opinion, you MUST respond and you MUST say WHY in the changelog.
Nothing that happened.
But regardless, this is still buggy and this is still NAK.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
NAK means: Don't send the same code.
What on earth are you talking about?
Why are you sending me this in two copies?
Respond here
You send the reply to both V3 and V2, so double reply.
You send messages to me in private and then later you send them in public. Don't. I am speaking about SAME messages.
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Do you understand the difference betweeen logical level and line level?
Respond here
Do you understand this is writing GPIO? This has never been related to
Your comments are really not helping... Imagine that I know this code a lot. You can check it easily.
Quoted 2 lines
the discussion.
I have kept telling you you got this sensor usage wrong.
Bring arguments.
Explain why asserting GPIO line is making the device operational.
Quoted 57 lines
Quoted 57 lines
Quoted 48 lines
Quoted 48 lines
Quoted 8 lines
See imx274.c,imx283.c,imx334.c,imx335.c,imx412.c,imx415.c. Your claim that this is buggy doesn't make sense when all other Sony imx drivers are using the same logic.
Quoted 9 lines
imx274.c:
/*
*imx274_reset- Function called to reset the sensor
* @priv: Pointer to device structure
* @rst: Input value for determining the sensor's end state after reset *
* Set the senor in reset and then
* if rst = 0, keep it in reset;
* if rst = 1, bring it out of reset.Buggy driver, another old poor code.
Quoted 11 lines
*
*/
static voidimx274_reset(struct stimx274 *priv, int rst){gpiod_set_value_cansleep(priv->reset_gpio, 0);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);gpiod_set_value_cansleep(priv->reset_gpio, !!rst);usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); }Quoted 2 lines
imx283.c:Way you paste code makes it very unreadable. It's easier to point web.git references.
Anyway, look also here:
Quoted 5 lines
static void
imx415_power_off(struct imx415 *sensor){clk_disable_unprepare(sensor->clk);gpiod_set_value_cansleep(sensor->reset, 1);Quoted 5 lines
Quoted 5 lines
But if you claim that reset has to be asserted for this device to work - and that's what your code is doing - then this is not a reset line.
Do you understand what is the meaning of asserted reset (or to assert reset line)?
So you do not understand above and yet you keep arguing with maintainer.
Quoted 1 line
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
And? All other drivers, camera sensors, hwmon, iio, codecs and whatnot?
Are those Sony image sensors?
It does not matter. Hardware electronic engineers in Sony are not a different specie than other engineers...
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
I can apply the same logic and say this is buggy and wrong.
We are not going to talk imaginary things.
So you can imagine this code is buggy even though I tell you this is correct? Of course I tested it already.
With buggy DTS...
...
Quoted 5 lines
Quoted 5 lines
I will not because arguments "I found such code somewhere, so I will not respond to actual arguments just use that code" are invalid.
You cannot make reset asserted and claim "this is operating stage". I
Do you understand this?
Quoted 5 lines
Quoted 5 lines
explained why: because it does not work with correct DTS. If it works for you, then because your DTS is not correct.
Apparently you do not understand what is assertion and what is logical state of GPIO (I asked this three times), but you keep disagreeing.
Do you understand this?
Quoted 6 lines
Quoted 4 lines
That's basic knowledge, so please kindly go to Wikipedia or stackoverflow, because you are now wasting reviewers time. You do not respond to reviewers arguments.
You are asking me to code a bug in the driver and the arguments don't make sense.
No, I asked you to learn basics of this and what is assertion of reset line.
Quoted 7 lines
As much as I appreciate your feedback, I want a working driver upstream and will have to point to the existing code base to prove that it works.
I understand your points that you want me to code a bug (from my point of view), I will take that feedback to consider but the only way forward is to remove the optional reset gpio I guess.
No, you do not want to understand the problem at all. You do not want to even listen even though I gave you multiple guidances and multiple arguments.
You actually never responded to the actual arguments.
When experienced people tell you this is wrong and give you arguments, you should consider they might be right...
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Will WhangQuoted 21 lines · Will Whang
Quoted 6 lines
Quoted 3 lines
And in all the examples I provided to you, this is the only IMX415 that has the logic inverted.
And? All other drivers, camera sensors, hwmon, iio, codecs and whatnot?
Are those Sony image sensors?
Quoted 5 lines
Quoted 2 lines
I can apply the same logic and say this is buggy and wrong.
We are not going to talk imaginary things.
So you can imagine this code is buggy even though I tell you this is correct? Of course I tested it already.
Quoted 6 lines
Quoted 1 line
Do you understand this is writing the GPIO directly and has nothing to
It is not. Again, you are mixing logical level with line level. You never responded to that part, you never used actual arguments except some vague statements like above.
Quoted 6 lines
Quoted 4 lines
You do not write GPIO directly.
Each driver is supposed to use logical level.
hmm? Code in the tree disagree with you? Are you going to arguing that all the 90% of Sony image sensor drivers are old and buggy?
I checked now and both driver and all users of imx219 are wrong. Don't ever use that as an example.
And you doing THE SAME as imx219 is also proof that your code is wrong.
Best regards,
Krzysztof
Sakari Ailus
maintainerRe: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Will WhangHi Will,
Quoted 5 lines · Will Whang
You are asking me to code a bug in the driver and the arguments don't make sense.
As much as I appreciate your feedback, I want a working driver upstream and will have to point to the existing code base to prove that it works.
Please do address Krysztof's comments, otherwise there won't be progress in upstreaming this driver. I can recommend reading section "The active low and open drain semantics" in Documentation/driver-api/gpio/consumer.rst .
Signature · 3 lines
Regards, Sakari Ailus
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Will WhangLe samedi 16 août 2025 à 12:58 -0700, Will Whang a écrit :
Quoted 45 lines · Krzysztof Kozlowski
Quoted 40 lines · Will Whang
Quoted 25 lines
drivers/media/i2c/imx585.c new file +25
-0+ +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); + const struct imx585_mode *mode, *mode_list; + struct v4l2_subdev_state *state; + struct v4l2_mbus_framefmt *fmt; + unsigned int num_modes; + int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(state, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->width, fmt->height); + + /* Apply control only when powered (runtime active). */ + if (!pm_runtime_get_if_active(imx585->clientdev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: { + u32 shr = (imx585->vmax - ctrl->val) & ~1U; /* SHR always a multiple of 2 */ +drivers/media/i2c/imx585.c line 626
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+ dev_dbg(imx585->clientdev, "EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n", + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_SHR, shr, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "SHR write failed (%d)\n", ret); + break; + } + case V4L2_CID_ANALOGUE_GAIN: + dev_dbg(imx585->clientdev, "ANALOG_GAIN=%u\n", ctrl->val);Not much improved. Don't debug V4L2 calls.
Quoted 3 lines
I already commented on this and you just send simialr code. Drop this completely.
Quoted 7 lines
I need to debug V4L2 calls for image quality debugging. I don't understand why I can not have
dev_dbg().
What I read from your comments on the previous patch is that you don't want to have a noisy driver and I sorta agree with that but for debug purposes this is not an issue.
That is why I move it todev_dbginstead of removing them, if you think this is too noisy, then just don't turn on debugging.
V4L2 calls have generic trace already, see instruction to activate them:
On top of which, you can use strace and v4l2-tracer from userspace.
regards,
Nicolas
Quoted 412 lines
Quoted 401 lines
Quoted 388 lines
+ ret =
cci_write(imx585->regmap, IMX585_REG_ANALOG_GAIN, ctrl->val, NULL); + if (ret)
+dev_err_ratelimited(imx585->clientdev, "Gain write failed (%d)\n", ret); + break;
+ caseV4L2_CID_VBLANK: {
+ u32current_exposure=imx585->exposure->cur.val;drivers/media/i2c/imx585.c new file +380
-0+ + imx585->vmax = (mode->height + ctrl->val) & ~1U; + + current_exposure = clamp_t(u32, current_exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN); + __v4l2_ctrl_modify_range(imx585->exposure, + IMX585_EXPOSURE_MIN, imx585->vmax - IMX585_SHR_MIN, 1, + current_exposure); + + dev_dbg(imx585->clientdev, "VBLANK=%u -> VMAX=%u\n", ctrl->val, imx585->vmax); + + ret = cci_write(imx585->regmap, IMX585_REG_VMAX, imx585->vmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HBLANK: { + u64 pixel_rate = (u64)mode->width * IMX585_PIXEL_RATE; + u64 hmax; + + do_div(pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) * IMX585_PIXEL_RATE; + do_div(hmax, pixel_rate); + imx585->hmax = (u32)hmax; + + dev_dbg(imx585->clientdev, "HBLANK=%u -> HMAX=%u\n", ctrl->val, imx585->hmax); + + ret = cci_write(imx585->regmap, IMX585_REG_HMAX, imx585->hmax, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HMAX write failed (%d)\n", ret); + break; + } + case V4L2_CID_HFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEH, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "HFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_VFLIP: + ret = cci_write(imx585->regmap, IMX585_FLIP_WINMODEV, ctrl->val, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "VFLIP write failed (%d)\n", ret); + break; + case V4L2_CID_BRIGHTNESS: { + u16 blacklevel = min_t(u32, ctrl->val, 4095); + + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, blacklevel, NULL); + if (ret) + dev_err_ratelimited(imx585->clientdev, "BLKLEVEL write failed (%d)\n", ret); + break; + } + default: + dev_dbg(imx585->clientdev, "Unhandled ctrl %s: id=0x%x, val=0x%x\n", + ctrl->name, ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(imx585->clientdev); + return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ + struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; + struct v4l2_fwnode_device_properties props; + int ret; + + ret = v4l2_ctrl_handler_init(hdl, 16); + + /* Read-only, updated per mode */ + imx585->pixel_rate = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_PIXEL_RATE, + 1, UINT_MAX, 1, 1); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ, + 0, 0, &link_freqs[imx585->link_freq_idx]); + if (imx585->link_freq) + imx585->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx585->vblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VBLANK, 0, 0xFFFFF, 1, 0); + imx585->hblank = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HBLANK, 0, 0xFFFF, 1, 0); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1, + IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_EXPOSURE, + IMX585_EXPOSURE_MIN, IMX585_EXPOSURE_MAX, + IMX585_EXPOSURE_STEP, IMX585_EXPOSURE_DEFAULT); + + imx585->gain = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + IMX585_ANA_GAIN_MIN, IMX585_ANA_GAIN_MAX, + IMX585_ANA_GAIN_STEP, IMX585_ANA_GAIN_DEFAULT); + + imx585->hflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + imx585->vflip = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + + if (hdl->error) { + ret = hdl->error; + dev_err(imx585->clientdev, "control init failed (%d)\n", ret); + goto err_free; + } + + ret = v4l2_fwnode_device_parse(imx585->clientdev, &props); + if (ret) + goto err_free; + + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx585_ctrl_ops, &props); + if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; + return 0; + +err_free: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ + v4l2_ctrl_handler_free(imx585->sd.ctrl_handler); +} + +/* -------------------------------------------------------------------------- +* Pad ops / formats +* -------------------------------------------------------------------------- +*/ + +static int imx585_enum_mbus_code (struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx585 *imx585 = to_imx585(sd); + unsigned int entries; + const u32 *tbl; + + if (imx585->mono) { + if (code->index) + return -EINVAL; + code->code = MEDIA_BUS_FMT_Y12_1X12; + return 0; + } + + tbl = color_codes; + entries = ARRAY_SIZE(color_codes) / 4; + + if (code->index >= entries) + return -EINVAL; + + code->code = imx585_get_format_code(imx585, tbl[code->index * 4]); + return 0; +} + +static int imx585_enum_frame_size (struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list; + unsigned int num_modes; + + get_mode_table(imx585, fse->code, &mode_list, &num_modes); + if (fse->index >= num_modes) + return -EINVAL; + if (fse->code != imx585_get_format_code(imx585, fse->code)) + return -EINVAL; + + fse->min_width = mode_list[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = mode_list[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int imx585_set_pad_format (struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + unsigned int num_modes; + struct v4l2_mbus_framefmt *format; + + get_mode_table(imx585, fmt->format.code, &mode_list, &num_modes); + mode = v4l2_find_nearest_size(mode_list, num_modes, width, height, + fmt->format.width, fmt->format.height); + + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.field = V4L2_FIELD_NONE; + fmt->format.colorspace = V4L2_COLORSPACE_RAW; + fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_601; + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; + + format = v4l2_subdev_state_get_format(sd_state, 0); + + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) + imx585_set_framing_limits(imx585, mode); + + *format = fmt->format; + return 0; +} + +/* -------------------------------------------------------------------------- +* Stream on/off +* -------------------------------------------------------------------------- +*/ + +static int imx585_enable_streams (struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + const struct imx585_mode *mode_list, *mode; + struct v4l2_subdev_state *st; + struct v4l2_mbus_framefmt *fmt; + unsigned int n_modes; + int ret; + + ret = pm_runtime_get_sync(imx585->clientdev); + if (ret < 0) { + pm_runtime_put_noidle(imx585->clientdev); + return ret; + } + + ret = cci_multi_reg_write(imx585->regmap, common_regs, + ARRAY_SIZE(common_regs), NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write common settings\n"); + goto err_rpm_put; + } + + ret = cci_write(imx585->regmap, IMX585_INCK_SEL, imx585->inck_sel_val, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_REG_BLKLEVEL, IMX585_BLKLEVEL_DEFAULT, NULL); + if (!ret) + ret = cci_write(imx585->regmap, IMX585_DATARATE_SEL, + link_freqs_reg_value[imx585->link_freq_idx], NULL); + if (ret) + goto err_rpm_put; + + ret = cci_write(imx585->regmap, IMX585_LANEMODE, + (imx585->lane_count == 2) ? 0x01 : 0x03, NULL); + if (ret) + goto err_rpm_put; + + /* Mono bin flag (datasheet: 0x01 mono, 0x00 color) */ + ret = cci_write(imx585->regmap, IMX585_BIN_MODE, imx585->mono ? 0x01 : 0x00, NULL); + if (ret) + goto err_rpm_put; + + /* Sync configuration */ + if (imx585->sync_mode == SYNC_INT_FOLLOWER) { + dev_dbg(imx585->clientdev, "Internal sync follower: XVS input\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x01, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x03, NULL); /* XHS out, XVS in */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x08, NULL); /* disable XVS OUT */ + } else if (imx585->sync_mode == SYNC_INT_LEADER) { + dev_dbg(imx585->clientdev, "Internal sync leader: XVS/XHS output\n"); + cci_write(imx585->regmap, IMX585_REG_EXTMODE, 0x00, NULL); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x00, NULL); /* XHS/XVS out */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x0A, NULL); + } else { + dev_dbg(imx585->clientdev, "Follower: XVS/XHS input\n"); + cci_write(imx585->regmap, IMX585_REG_XXS_DRV, 0x0F, NULL); /* inputs */ + cci_write(imx585->regmap, IMX585_REG_XXS_OUTSEL, 0x00, NULL); + } + + imx585->common_regs_written = true; + + /* Select mode */ + st = v4l2_subdev_get_locked_active_state(&imx585->sd); + fmt = v4l2_subdev_state_get_format(st, 0); + + get_mode_table(imx585, fmt->code, &mode_list, &n_modes); + mode = v4l2_find_nearest_size(mode_list, n_modes, width, height, + fmt->width, fmt->height); + + ret = cci_multi_reg_write(imx585->regmap, mode->reg_list.regs, + mode->reg_list.num_of_regs, NULL); + if (ret) { + dev_err(imx585->clientdev, "Failed to write mode registers\n"); + goto err_rpm_put; + } + + /* Disable digital clamp */ + cci_write(imx585->regmap, IMX585_REG_DIGITAL_CLAMP, 0x00, NULL); + + /* Apply user controls after writing the base tables */ + ret = __v4l2_ctrl_handler_setup(imx585->sd.ctrl_handler); + if (ret) { + dev_err(imx585->clientdev, "Control handler setup failed\n"); + goto err_rpm_put; + } + + if (imx585->sync_mode != SYNC_EXTERNAL) + cci_write(imx585->regmap, IMX585_REG_XMSTA, 0x00, NULL); + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STREAMING, NULL); + if (ret) + goto err_rpm_put; + + dev_dbg(imx585->clientdev, "Streaming started\n"); + usleep_range(IMX585_STREAM_DELAY_US, + IMX585_STREAM_DELAY_US + IMX585_STREAM_DELAY_RANGE_US); + + /* vflip, hflip cannot change during streaming */ + __v4l2_ctrl_grab(imx585->vflip, true); + __v4l2_ctrl_grab(imx585->hflip, true); + + return 0; + +err_rpm_put: + pm_runtime_put_autosuspend(imx585->clientdev); + return ret; +} + +static int imx585_disable_streams (struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct imx585 *imx585 = to_imx585(sd); + int ret; + + ret = cci_write(imx585->regmap, IMX585_REG_MODE_SELECT, IMX585_MODE_STANDBY, NULL); + if (ret) + dev_err(imx585->clientdev, "Failed to stop streaming\n"); + + __v4l2_ctrl_grab(imx585->vflip, false); + __v4l2_ctrl_grab(imx585->hflip, false); + + pm_runtime_put_autosuspend(imx585->clientdev); + + return ret; +} + +/* -------------------------------------------------------------------------- +* Power / runtime PM +* -------------------------------------------------------------------------- +*/ + +static int imx585_power_on (struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + int ret; + + dev_dbg(imx585->clientdev, "power_on\n"); + + ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable regulators\n"); + return ret; + } + + ret = clk_prepare_enable(imx585->xclk); + if (ret) { + dev_err(imx585->clientdev, "Failed to enable clock\n"); + goto reg_off; + } + + gpiod_set_value_cansleep(imx585->reset_gpio, 1); + usleep_range(IMX585_XCLR_MIN_DELAY_US, + IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US); + return 0; + +reg_off: + regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies); + return ret;drivers/media/i2c/imx585.c line 1022
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+} + +static int imx585_power_off (struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct imx585 *imx585 = to_imx585(sd); + + dev_dbg(imx585->clientdev, "power_off\n"); + + gpiod_set_value_cansleep(imx585->reset_gpio, 0);NAK, I wrote you this is broken and you just ignored and sending the same.
Quoted 5 lines
You are mixing line level with logical level.
There is no way your code actually works, unless you have broken DTS. Test your patches correctly (with proper, fixed DTS) and don't send the same completely ignoring reviewers.
See how
imx219.cworks, ask Sony don't ask me.
That is why I ignore your comments on this.Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
In reply to Will WhangHi Will,
Thanks for working on getting this upstream! I'm always happy to see more cameras supported for all platforms!
Please post the IMX585 camera sensor helpers to libcamera too - they're eligible for merge when a driver is posted publicly to the kernel mailing list.
I'm only tackling the black level in this review at the moment as I noticied it so trimming out other parts for the review:
Quoting Will Whang (2025-08-16 06:54:32)
Quoted 8 lines
Implements support for:
* 4-lane / 2-lane CSI-2
* 12-bit linear, 4K/FHD mode.
* Monochrome variant.
* Tested on Raspberry Pi 4/5 with 24 MHz clock.Signed-off-by Will Whang <[email protected]>---
<snip>
drivers/media/i2c/imx585.c line 87
Quoted 4 lines
drivers/media/i2c/imx585.c new file +4
-0+ +/* Black level control */ +#define IMX585_REG_BLKLEVEL CCI_REG16_LE(0x30dc) +#define IMX585_BLKLEVEL_DEFAULT 50
50 ... seems surprisingly low to me ... Is that a 10 bit value or a 12 bit value ? You only have 12 bit modes ...
Oh - fortunately I can see a datasheet on this one.
"""
When the output data length is 10-bit output, increasing the register setting value by 1h increases the black level by 1 LSB. When the output data length is 12-bit output, increasing the register setting value by 1h increases the black level by 4 LSB.
Use with values shown below is recommended.
10-bit output: 032h (50d in LSB units)
12-bit output: 032h (200d in LSB units)
"""
Ok - so the value written is always a 10 bit number regardless of the mode, which gives 3200 as a 16 bit value which is more in the range I would have expected, so now it makes sense.
Quoted 4 lines
drivers/media/i2c/imx585.c new file +4
-0+ +/* Digital Clamp */ +#define IMX585_REG_DIGITAL_CLAMP CCI_REG8(0x3458) +
<snip>
Quoted 78 lines
drivers/media/i2c/imx585.c new file +78
-0+/* -------------------------------------------------------------------------- +* Controls +* -------------------------------------------------------------------------- +*/ + +static int imx585_set_ctrl (struct v4l2_ctrl *ctrl) +{ + struct imx585 *imx585 = container_of(ctrl->handler, struct imx585, ctrl_handler); +const struct imx585_mode *mode, *mode_list; +struct v4l2_subdev_state *state; +struct v4l2_mbus_framefmt *fmt; +unsigned int num_modes; +int ret =0 ; + + state =v4l2_subdev_get_locked_active_state (&imx585->sd); + fmt =v4l2_subdev_state_get_format (state,0 ); + +get_mode_table (imx585, fmt->code, &mode_list, &num_modes); + mode =v4l2_find_nearest_size (mode_list, num_modes, width, height, + fmt->width, fmt->height); + +/* Apply control only when powered (runtime active). */ +if (!pm_runtime_get_if_active (imx585->clientdev)) +return 0 ; + +switch (ctrl->id) { +case V4L2_CID_EXPOSURE : { +u32 shr = (imx585->vmax - ctrl->val) & ~1U ;/* SHR always a multiple of 2 */ + +dev_dbg (imx585->clientdev,"EXPOSURE=%u -> SHR=%u (VMAX=%u HMAX=%u)\n" , + ctrl->val, shr, imx585->vmax, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_SHR , shr,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"SHR write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_ANALOGUE_GAIN : +dev_dbg (imx585->clientdev,"ANALOG_GAIN=%u\n" , ctrl->val); + ret =cci_write (imx585->regmap,IMX585_REG_ANALOG_GAIN , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"Gain write failed (%d)\n" , ret); +break ; +case V4L2_CID_VBLANK : { +u32 current_exposure = imx585->exposure->cur.val; + + imx585->vmax = (mode->height + ctrl->val) & ~1U ; + + current_exposure =clamp_t (u32, current_exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ); +__v4l2_ctrl_modify_range (imx585->exposure, +IMX585_EXPOSURE_MIN , imx585->vmax -IMX585_SHR_MIN ,1 , + current_exposure); + +dev_dbg (imx585->clientdev,"VBLANK=%u -> VMAX=%u\n" , ctrl->val, imx585->vmax); + + ret =cci_write (imx585->regmap,IMX585_REG_VMAX , imx585->vmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HBLANK : { +u64 pixel_rate = (u64 )mode->width *IMX585_PIXEL_RATE ; +u64 hmax; + +do_div (pixel_rate, mode->min_hmax); + hmax = (u64)(mode->width + ctrl->val) *IMX585_PIXEL_RATE ; +do_div (hmax, pixel_rate); + imx585->hmax = (u32 )hmax; + +dev_dbg (imx585->clientdev,"HBLANK=%u -> HMAX=%u\n" , ctrl->val, imx585->hmax); + + ret =cci_write (imx585->regmap,IMX585_REG_HMAX , imx585->hmax,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HMAX write failed (%d)\n" , ret); +break ; + } +case V4L2_CID_HFLIP :
drivers/media/i2c/imx585.c line 675
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+ ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEH , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"HFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_VFLIP : + ret =cci_write (imx585->regmap,IMX585_FLIP_WINMODEV , ctrl->val,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"VFLIP write failed (%d)\n" , ret); +break ; +case V4L2_CID_BRIGHTNESS : {
This is the wrong control to set blacklevel.
Please use V4L2_CID_BLACK_LEVEL
Quoted 1 line
+ u16 blacklevel =
min_t(u32, ctrl->val, 4095);
do you know if the value is specific to which mode the sensor is in ?
I assume this is a 10 bit value if the sensor is in a 10 bit mode and a 12 bit mode if the sensor is in 12 bit mode for example.
Edit: Nope - now I've found a datasheet - this is /always/ a 10 bit value. Can you document that with a comment please? Especially as this driver only currently outputs 12 bit modes but uses a 10 bit black level that could be confusing otherwise.
If we want to make that adjustable from libcamera we'll have to be careful to always use the correct units.
Quoted 35 lines
drivers/media/i2c/imx585.c new file +35
-0+ + ret =cci_write (imx585->regmap,IMX585_REG_BLKLEVEL , blacklevel,NULL ); +if (ret) +dev_err_ratelimited (imx585->clientdev,"BLKLEVEL write failed (%d)\n" , ret); +break ; + } +default : +dev_dbg (imx585->clientdev,"Unhandled ctrl %s: id=0x%x, val=0x%x\n" , + ctrl->name, ctrl->id, ctrl->val); +break ; + } + +pm_runtime_put (imx585->clientdev); +return ret; +} + +static const struct v4l2_ctrl_ops imx585_ctrl_ops = { + .s_ctrl = imx585_set_ctrl, +}; + +static int imx585_init_controls (struct imx585 *imx585) +{ +struct v4l2_ctrl_handler *hdl = &imx585->ctrl_handler; +struct v4l2_fwnode_device_properties props; +int ret; + + ret =v4l2_ctrl_handler_init (hdl,16 ); + +/* Read-only, updated per mode */ + imx585->pixel_rate =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_PIXEL_RATE , +1 ,UINT_MAX ,1 ,1 ); + + imx585->link_freq = + v4l2_ctrl_new_int_menu(hdl, &imx585_ctrl_ops, V4L2_CID_LINK_FREQ,
drivers/media/i2c/imx585.c line 721
Quoted 10 lines
drivers/media/i2c/imx585.c new file +10
-0+ 0, 0, &link_freqs[imx585->link_freq_idx]); +if (imx585->link_freq) + imx585->link_freq->flags |=V4L2_CTRL_FLAG_READ_ONLY ; + + imx585->vblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VBLANK ,0 ,0xFFFFF ,1 ,0 ); + imx585->hblank =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HBLANK ,0 ,0xFFFF ,1 ,0 ); + imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops, + V4L2_CID_BRIGHTNESS, 0, 0xFFFF, 1,
You're setting max to 0xFFFF but in the code you clamp it to 4095 (0xFFF)?
I'd use readable decimal values for the black level range rather than hex, as I think of black level in decimal values.
If you set this to
imx585->blacklevel = v4l2_ctrl_new_std(hdl, &imx585_ctrl_ops,
V4L2_CID_BLACK_LEVEL, 0, 4095, 1,
IMX585_BLKLEVEL_DEFAULT);you can remove the clamp from the set controls call as v4l2 will restrict the values.
Oh ... I just looked at the datasheet again:
Setting value
Initial value 032h
Setting value 000h to 3FFh
So the actual maximum is 1023. Maybe hex (0x3ff) or decimal is fine either way depending on if you want to closely match the datasheet.
Quoted 43 lines
drivers/media/i2c/imx585.c new file +43
-0+ IMX585_BLKLEVEL_DEFAULT); + + imx585->exposure =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_EXPOSURE , +IMX585_EXPOSURE_MIN ,IMX585_EXPOSURE_MAX , +IMX585_EXPOSURE_STEP ,IMX585_EXPOSURE_DEFAULT ); + + imx585->gain =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops,V4L2_CID_ANALOGUE_GAIN , +IMX585_ANA_GAIN_MIN ,IMX585_ANA_GAIN_MAX , +IMX585_ANA_GAIN_STEP ,IMX585_ANA_GAIN_DEFAULT ); + + imx585->hflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_HFLIP ,0 ,1 ,1 ,0 ); + imx585->vflip =v4l2_ctrl_new_std (hdl, &imx585_ctrl_ops, +V4L2_CID_VFLIP ,0 ,1 ,1 ,0 ); + +if (hdl->error) { + ret = hdl->error; +dev_err (imx585->clientdev,"control init failed (%d)\n" , ret); + goto err_free; + } + + ret =v4l2_fwnode_device_parse (imx585->clientdev, &props); +if (ret) + goto err_free; + + ret =v4l2_ctrl_new_fwnode_properties (hdl, &imx585_ctrl_ops, &props); +if (ret) + goto err_free; + + imx585->sd.ctrl_handler = hdl; +return 0 ; + +err_free: +v4l2_ctrl_handler_free (hdl); +return ret; +} + +static void imx585_free_controls (struct imx585 *imx585) +{ +v4l2_ctrl_handler_free (imx585->sd.ctrl_handler); +} +};
<snip>