| 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: <CAFoNnrxbzcF+YranTL8Von3BkROhq8X=RX5sa90M6PYgS_vjkQ@mail.gmail.com>
Date: Sat, 16 Aug 2025 12:58:15 -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 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.
>
> > + 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.
That is why I ignore your comments on this.
> Best regards,
> Krzysztof
Powered by blists - more mailing lists