| 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: <CAFoNnrw4yRKGL_m0=g14C583o13ptC6e84TN---ABdyeg8jMhg@mail.gmail.com>
Date: Sat, 16 Aug 2025 23:46: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 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?
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);
}
> Best regards,
> Krzysztof
Powered by blists - more mailing lists