Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/02/2025 20:56, Sebastian LaVine wrote:
> +static int imx728_detect(struct imx728 *imx728)
> +{
> +       int ret = 0;
> +       u64 minor, major;
> +
> +       cci_read(imx728->regmap, IMX728_REG_VMAJOR, &major, &ret);
> +       if (ret != 0) {
> +               dev_err(imx728->dev, "Could not read PARAM_MAJOR_VER!");
> +               return ret;
> +       }
> +       cci_read(imx728->regmap, IMX728_REG_VMINOR, &minor, &ret);
> +       if (ret != 0) {
> +               dev_err(imx728->dev, "Could not read PARAM_MINOR_VER!");
> +               return ret;
> +       }
> +       dev_dbg(imx728->dev, "Got version: %lld.%lld", major, minor);
> +
> +       return 0;
> +}
> +
> +static int imx728_reset(struct imx728 *imx728)
> +{
> +
> +       int ret = 0;
> +
> +       // Prefer hardware reset if available.
> +       if (!IS_ERR_OR_NULL(imx728->reset_gpio)) {

Here can be ERR (although why?) but...

> +               gpiod_set_value_cansleep(imx728->reset_gpio, 1);
> +               usleep_range(1000, 10000);
> +               gpiod_set_value_cansleep(imx728->reset_gpio, 0);
> +               msleep(100);
> +       } else {
> +               // Software reset.
> +               cci_write(imx728->regmap, IMX728_REG_RESET_0, 0xc3, &ret);
> +               cci_update_bits(imx728->regmap, IMX728_REG_RESET_1, 0x1, 0x1, &ret);
> +               msleep(100);
> +       }
> +
> +       return ret;
> +}
> +
> +static int imx728_power_on(struct imx728 *imx728)
> +{
> +       int ret;
> +
> +       ret = clk_prepare_enable(imx728->clk);
> +       if (ret < 0)
> +               return ret;
> +
> +       imx728_reset(imx728);
> +       return 0;
> +}
> +
> +static int imx728_power_off(struct imx728 *imx728)
> +{
> +
> +       if (imx728->reset_gpio) {

Here cannot.

> +               gpiod_set_value_cansleep(imx728->reset_gpio, 1);
> +
> +               usleep_range(1, 10);
> +       }
> +       clk_disable_unprepare(imx728->clk);
> +       return 0;
> +}
> +


...

> +
> +static int imx728_set_stream(struct v4l2_subdev *sd, int enable)
> +{
> +       struct imx728 *imx728 = to_imx728(sd);
> +       int ret;
> +
> +       mutex_lock(&imx728->lock);

Just use guard. That's actually perfect candidate.


> +       if (imx728->streaming == enable) {
> +               mutex_unlock(&imx728->lock);
> +               return 0;
> +       }
> +
> +       if (enable) {
> +               ret = pm_runtime_get_sync(imx728->dev);
> +               if (ret < 0) {
> +                       pm_runtime_put_noidle(imx728->dev);
> +                       goto err_unlock;
> +               }
> +
> +               ret = imx728_start_stream(imx728);
> +               if (ret < 0)
> +                       goto err_runtime_put;
> +       } else {
> +               ret = imx728_stop_stream(imx728);
> +               if (ret < 0)
> +                       goto err_runtime_put;
> +               pm_runtime_mark_last_busy(imx728->dev);
> +               pm_runtime_put_autosuspend(imx728->dev);
> +       }
> +
> +       imx728->streaming = enable;
> +
> +       __v4l2_ctrl_grab(imx728->ctrl.h_flip, enable);
> +       __v4l2_ctrl_grab(imx728->ctrl.v_flip, enable);
> +
> +       mutex_unlock(&imx728->lock);
> +
> +       return 0;
> +
> +err_runtime_put:
> +       pm_runtime_put(imx728->dev);
> +
> +err_unlock:
> +       mutex_unlock(&imx728->lock);
> +       dev_err(imx728->dev,
> +               "%s: failed to setup streaming %d\n", __func__, ret);
> +       return ret;
> +}
> +
> +static const struct v4l2_subdev_core_ops imx728_core_ops = {
> +       .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +       .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
> +static const struct v4l2_subdev_video_ops imx728_subdev_video_ops = {
> +       .s_stream = imx728_set_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops imx728_subdev_pad_ops = {
> +       .enum_frame_size = imx728_enum_frame_sizes,
> +       .enum_mbus_code = imx728_enum_mbus_code,
> +       .get_fmt = v4l2_subdev_get_fmt,
> +       .get_frame_desc = imx728_get_frame_desc,
> +       .get_frame_interval = imx728_get_frame_interval,
> +       .get_selection = imx728_get_selection,
> +       .set_fmt = imx728_set_fmt,
> +       .set_frame_interval = imx728_set_frame_interval,
> +       .set_routing = imx728_set_routing,
> +};
> +
> +static const struct v4l2_subdev_ops imx728_subdev_ops = {
> +       .core  = &imx728_core_ops,
> +       .video = &imx728_subdev_video_ops,
> +       .pad   = &imx728_subdev_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops imx728_internal_ops = {
> +       .init_state = imx728_init_state,
> +};
> +
> +
> +static const struct v4l2_ctrl_ops imx728_ctrl_ops = {
> +       .s_ctrl = imx728_set_ctrl,
> +};
> +
> +static int imx728_probe(struct i2c_client *client)
> +{
> +       struct imx728 *imx728;
> +       struct v4l2_subdev *sd;
> +       struct v4l2_ctrl_handler *ctrl_hdr;
> +       int ret;
> +
> +       imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL);
> +       if (!imx728)
> +               return -ENOMEM;
> +
> +       imx728->dev = &client->dev;
> +
> +       imx728->regmap = devm_cci_regmap_init_i2c(client, 16);
> +       if (IS_ERR(imx728->regmap))
> +               return PTR_ERR(imx728->regmap);
> +
> +       imx728->reset_gpio = devm_gpiod_get_optional(imx728->dev,
> +                                            "reset", GPIOD_OUT_LOW);
> +       if (IS_ERR(imx728->reset_gpio))
> +               return PTR_ERR(imx728->reset_gpio);

So can it be ERR after that point? Looks like not.



Best regards,
Krzysztof