| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFoNnrxT83nz0qxf8HTapqOXuEQ0Vh+RbxyqRGQy_sJp9nzpAg@mail.gmail.com>
Date: Sun, 17 Aug 2025 00:53:49 -0700
From: Will Whang <will@...lwhang.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] media: i2c: imx585: Add Sony IMX585 image-sensor driver
On Sun, Aug 17, 2025 at 12:35 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 17/08/2025 09:15, Will Whang wrote:
> > On Sun, Aug 17, 2025 at 12:02 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>
> >> On 17/08/2025 08:46, Will Whang wrote:
> >>> On Sat, Aug 16, 2025 at 11:10 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>>>
> >>>> On 16/08/2025 21:58, Will Whang wrote:
> >>>>> On Sat, Aug 16, 2025 at 1:04 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>>>>>
> >>>>>> On 16/08/2025 07:54, Will Whang wrote:
> >>>>>>> +
> >>>>>>> +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);
> >>>>>>
> >>>>>> Not much improved. Don't debug V4L2 calls.
> >>>>>>
> >>>>>> 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.
> >>>>>
> >>>>
> >>>>
> >>>> Because you do not debug useful parts of the driver, but only invocation
> >>>> of v4l2 controls.
> >>>>
> >>>>
> >>>>>>
> >>>>>>> + 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;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +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.
> >>>>>
> >>>>> See how imx219.c works, ask Sony don't ask me.
> >>>>
> >>>> So there is a bug, you claim that you may do the same bug and then say:
> >>>>
> >>>>> 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.
> >>>
> >>> 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.
>
> >>
> >> 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.
>
> >>
> >>> 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.
> >>
> >> Buggy driver, another old poor code.
> >>
> >>
> >>> *
> >>> */
> >>> 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:
> >>>
> >>
> >> Way you paste code makes it very unreadable. It's easier to point
> >> web.git references.
> >>
> >>
> >> Anyway, look also here:
> >>
> >>>
> >>> 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)?
>
> So you do not understand above and yet you keep arguing with maintainer.
>
> >
> > 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?
>
> > 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.
> > 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?
>
> > 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.
>
> >
> > Quote your previous comments:
> >> 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?
>
> 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.
>
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.
> As I said: code is wrong so NAK.
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists