[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240924225515.GQ7165@pendragon.ideasonboard.com>
Date: Wed, 25 Sep 2024 01:55:15 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 08/11] media: i2c: ov5645: Switch to
{enable,disable}_streams
Hi Prabhakar,
Thank you for the patch.
On Tue, Sep 10, 2024 at 06:06:07PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Switch from s_stream to enable_streams and disable_streams callbacks.
Here too you should explain why. The "why" is the most important part of
any commit message.
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> ---
> drivers/media/i2c/ov5645.c | 90 +++++++++++++++++++-------------------
> 1 file changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index 9497ec737cb7..dc93514608ee 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -923,71 +923,71 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
> +static int ov5645_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> {
> - struct ov5645 *ov5645 = to_ov5645(subdev);
> - struct v4l2_subdev_state *state;
> + struct ov5645 *ov5645 = to_ov5645(sd);
> int ret;
>
> - state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
> -
> - if (enable) {
> - ret = pm_runtime_resume_and_get(ov5645->dev);
> - if (ret < 0) {
> - v4l2_subdev_unlock_state(state);
> - return ret;
> - }
> + ret = pm_runtime_resume_and_get(ov5645->dev);
> + if (ret < 0)
> + return ret;
>
> - ret = ov5645_set_register_array(ov5645,
> + ret = ov5645_set_register_array(ov5645,
> ov5645->current_mode->data,
> ov5645->current_mode->data_size);
> - if (ret < 0) {
> - dev_err(ov5645->dev, "could not set mode %dx%d\n",
> - ov5645->current_mode->width,
> - ov5645->current_mode->height);
> - goto err_rpm_put;
> - }
> - ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
> - if (ret < 0) {
> - dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> - goto err_rpm_put;
> - }
> -
> - ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> - if (ret < 0)
> - goto err_rpm_put;
> -
> - ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> - OV5645_SYSTEM_CTRL0_START);
> - if (ret < 0)
> - goto err_rpm_put;
> - } else {
> - ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> - if (ret < 0)
> - goto stream_off_rpm_put;
> + if (ret < 0) {
> + dev_err(ov5645->dev, "could not set mode %dx%d\n",
> + ov5645->current_mode->width,
> + ov5645->current_mode->height);
> + goto err_rpm_put;
> + }
> + ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
> + if (ret < 0) {
> + dev_err(ov5645->dev, "could not sync v4l2 controls\n");
> + goto err_rpm_put;
> + }
>
> - ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> - OV5645_SYSTEM_CTRL0_STOP);
> + ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
> + if (ret < 0)
> + goto err_rpm_put;
>
> - goto stream_off_rpm_put;
> - }
> + ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> + OV5645_SYSTEM_CTRL0_START);
> + if (ret < 0)
> + goto err_rpm_put;
>
> - v4l2_subdev_unlock_state(state);
> return 0;
>
> err_rpm_put:
> pm_runtime_put_sync(ov5645->dev);
> return ret;
> +}
> +
> +static int ov5645_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct ov5645 *ov5645 = to_ov5645(sd);
> + int ret;
> +
> + ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
> + if (ret < 0)
> + goto rpm_put;
> +
> + ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
> + OV5645_SYSTEM_CTRL0_STOP);
>
> -stream_off_rpm_put:
> +rpm_put:
> pm_runtime_mark_last_busy(ov5645->dev);
> pm_runtime_put_autosuspend(ov5645->dev);
> - v4l2_subdev_unlock_state(state);
> +
> return ret;
> }
>
> static const struct v4l2_subdev_video_ops ov5645_video_ops = {
> - .s_stream = ov5645_s_stream,
> + .s_stream = v4l2_subdev_s_stream_helper,
> };
>
> static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> @@ -996,6 +996,8 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
> .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = ov5645_set_format,
> .get_selection = ov5645_get_selection,
> + .enable_streams = ov5645_enable_streams,
> + .disable_streams = ov5645_disable_streams,
> };
>
> static const struct v4l2_subdev_core_ops ov5645_core_ops = {
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists