Re: [PATCH v2 2/4] media: platform: rzg2l-cru: Use v4l2_get_link_freq()

[Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>]

On 12/11/2024 12:46, Daniel Scally wrote:
> From: Daniel Scally <dan.scally+renesas@xxxxxxxxxxxxxxxx>
>
> The rzg2l_csi2_calc_mbps() function currently tries to calculate the
> link frequency for a CSI2 bus using the V4L2_CID_PIXEL_RATE control
> of the remote subdevice. Switch the function to v4l2_get_link_freq()
> which correctly targets V4L2_CID_LINK_FREQ before falling back on
> V4L2_CID_PIXEL_RATE if the former is unavailable.
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Signed-off-by: Daniel Scally <dan.scally+renesas@xxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
>
> 	- None
>
>   .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 25 ++++++++-----------
>   1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index 881e910dce02..cbefd43de09b 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -242,28 +242,23 @@ static int rzg2l_csi2_calc_mbps(struct rzg2l_csi2 *csi2)
>   	const struct rzg2l_csi2_format *format;
>   	const struct v4l2_mbus_framefmt *fmt;
>   	struct v4l2_subdev_state *state;
> -	struct v4l2_ctrl *ctrl;
>   	u64 mbps;
>   
> -	/* Read the pixel rate control from remote. */
> -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> -	if (!ctrl) {
> -		dev_err(csi2->dev, "no pixel rate control in subdev %s\n",
> -			source->name);
> -		return -EINVAL;
> -	}
> -
>   	state = v4l2_subdev_lock_and_get_active_state(&csi2->subdev);
>   	fmt = v4l2_subdev_state_get_format(state, RZG2L_CSI2_SINK);
>   	format = rzg2l_csi2_code_to_fmt(fmt->code);
>   	v4l2_subdev_unlock_state(state);
>   
> -	/*
> -	 * Calculate hsfreq in Mbps
> -	 * hsfreq = (pixel_rate * bits_per_sample) / number_of_lanes
> -	 */
> -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
> -	do_div(mbps, csi2->lanes * 1000000);
> +	/* Read the link frequency from remote subdevice. */
> +	mbps = v4l2_get_link_freq(source->ctrl_handler, format->bpp,
> +				  csi2->lanes);
> +	if (mbps < 0) {
CI points out to me that mbps is unsigned and so this check is meaningless - I'll fix that in a v3
> +		dev_err(csi2->dev, "can't retrieve link freq from subdev %s\n",
> +			source->name);
> +		return -EINVAL;
> +	}
> +
> +	do_div(mbps, 1000000);
>   
>   	return mbps;
>   }