| 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: <04fd00bb-beb4-4f35-88fb-bf1cc7691505@kernel.org>
Date: Sun, 17 Aug 2025 09:02:25 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Will Whang <will@...lwhang.com>
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 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?
Do you understand the difference betweeen logical level and line level?
> 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)?
Best regards,
Krzysztof
Powered by blists - more mailing lists