Re: [PATCH v4 06/16] media: sony-starvis2: Allow runtime control of link frequency

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Dave,

On Thu, Oct 15, 2026 at 04:57:32PM +0100, Dave Stevenson wrote:
> DT allows configuration of multiple link frequencies.
> V4L2_CID_LINK_FREQ allows read write control.
> 
> Join these up so that link frequency can be configured at run time,
> as long as the sensor isn't streaming.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/sony-starvis2.c | 75 ++++++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/sony-starvis2.c b/drivers/media/i2c/sony-starvis2.c
> index 0edb0b4dd25f..02199336010e 100644
> --- a/drivers/media/i2c/sony-starvis2.c
> +++ b/drivers/media/i2c/sony-starvis2.c
> @@ -725,6 +725,7 @@ struct starvis2 {
>  	struct v4l2_ctrl *exposure;
>  	struct v4l2_ctrl *vblank;
>  	struct v4l2_ctrl *hblank;
> +	struct v4l2_ctrl *link_freq;
>  
>  	/* Track VMAX for exposure updates */
>  	u32 vmax;
> @@ -755,6 +756,7 @@ static int starvis2_set_ctrl(struct v4l2_ctrl *ctrl)
>  	struct starvis2 *starvis2 = container_of_const(ctrl->handler, struct
>  						       starvis2, ctrl_handler);
>  	struct i2c_client *client = v4l2_get_subdevdata(&starvis2->sd);
> +	const struct starvis2_variant *variant = starvis2->variant;
>  	const struct v4l2_mbus_framefmt *format;
>  	struct v4l2_subdev_state *state;
>  	int ret = 0;
> @@ -778,6 +780,25 @@ static int starvis2_set_ctrl(struct v4l2_ctrl *ctrl)
>  			return ret;
>  	}
>  
> +	if (ctrl->id == V4L2_CID_LINK_FREQ) {
> +		u32 hmax = variant->hmax_min_link_freq[ctrl->val];
> +		s32 hblank;
> +
> +		if (starvis2->lane_mode == STARVIS2_LANEMODE_4L)
> +			hmax = max(hmax >> 1, variant->hmax_min_pixel_array);
> +
> +		hblank = hmax * variant->pix_per_clk -
> +					variant->active_area.width;
> +
> +		ret = __v4l2_ctrl_modify_range(starvis2->hblank,
> +					       hblank,
> +					       starvis2->hblank->maximum,
> +					       starvis2->hblank->step,
> +					       hblank);

This fits on fewer lines.

> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * Only apply control values when device is powered on (RPM ACTIVE)
>  	 * and streaming (usage count != 0)
> @@ -824,6 +845,12 @@ static int starvis2_set_ctrl(struct v4l2_ctrl *ctrl)
>  		cci_write(starvis2->cci, STARVIS2_REG_WINMODEV, ctrl->val,
>  			  &ret);
>  		break;
> +	case V4L2_CID_LINK_FREQ:
> +		/* The STARVIS2_LINK_FREQ_ enums match up with the register */
> +		cci_write(starvis2->cci, STARVIS2_REG_DATARATE_SEL, ctrl->val,
> +			  &ret);
> +		break;
> +
>  	default:
>  		dev_warn(&client->dev,
>  			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
> @@ -938,9 +965,6 @@ static int starvis2_write_common(struct starvis2 *starvis2)
>  
>  	cci_write(starvis2->cci, STARVIS2_REG_INCK_SEL, starvis2->inck_sel_val,
>  		  &ret);
> -	cci_write(starvis2->cci, STARVIS2_REG_DATARATE_SEL,
> -		  __fls(starvis2->link_freq_bitmap),
> -		  &ret);
>  	cci_write(starvis2->cci, STARVIS2_REG_LANEMODE, starvis2->lane_mode,
>  		  &ret);
>  
> @@ -1018,6 +1042,7 @@ static int starvis2_enable_streams(struct v4l2_subdev *sd,
>  		goto err_rpm_put;
>  	}
>  
> +	__v4l2_ctrl_grab(starvis2->link_freq, true);

An extra newline would be nice here.

>  	return 0;
>  
>  err_rpm_put:
> @@ -1042,6 +1067,8 @@ static int starvis2_disable_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		dev_err(&client->dev, "%s failed to stop stream\n", __func__);
>  
> +	__v4l2_ctrl_grab(starvis2->link_freq, false);
> +
>  	pm_runtime_put(&client->dev);
>  
>  	return ret;
> @@ -1208,7 +1235,6 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  	struct i2c_client *client = v4l2_get_subdevdata(&starvis2->sd);
>  	const struct starvis2_variant *variant = starvis2->variant;
>  	struct v4l2_fwnode_device_properties props;
> -	struct v4l2_ctrl *link_freq;
>  	s32 hblank, max_hblank, vblank, max_vblank;
>  	u32 hmax;
>  	int ret;
> @@ -1224,25 +1250,26 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  
>  	starvis2->vmax = starvis2->variant->vmax_default;
>  
> -	hmax = variant->hmax_min_link_freq[__fls(starvis2->link_freq_bitmap)];
> -	if (starvis2->lane_mode == STARVIS2_LANEMODE_4L)
> -		hmax = max(hmax >> 1, variant->hmax_min_pixel_array);
> -
>  	/* PIXEL_RATE is fixed and read-only */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &starvis2_ctrl_ops, V4L2_CID_PIXEL_RATE,
>  			  starvis2->variant->pixel_rate,
>  			  starvis2->variant->pixel_rate, 1,
>  			  starvis2->variant->pixel_rate);
>  
> -	/* LINK_FREQ is also read only */
> -	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> -					   V4L2_CID_LINK_FREQ,
> -					   ARRAY_SIZE(link_freqs) - 1,
> -					   __ffs(starvis2->link_freq_bitmap),
> -					   link_freqs);
> -
> -	if (link_freq)
> -		link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	starvis2->link_freq =
> +		v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> +				       V4L2_CID_LINK_FREQ,
> +				       __fls(starvis2->link_freq_bitmap),
> +				       __ffs(starvis2->link_freq_bitmap),
> +				       link_freqs);
> +	if (!starvis2->link_freq)
> +		goto err_free_handler;
> +
> +	v4l2_ctrl_modify_range(starvis2->link_freq,
> +			       __ffs(starvis2->link_freq_bitmap),
> +			       __fls(starvis2->link_freq_bitmap),
> +			       ~(starvis2->link_freq_bitmap),

No need for parentheses.

> +			       __ffs(starvis2->link_freq_bitmap));
>  
>  	vblank = starvis2->vmax - starvis2->variant->active_area.height;
>  	max_vblank = STARVIS2_VMAX_MAX - starvis2->variant->active_area.height;
> @@ -1250,6 +1277,10 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  					     V4L2_CID_VBLANK, vblank, max_vblank,
>  					     2, vblank);
>  
> +	hmax = variant->hmax_min_link_freq[starvis2->link_freq->val];
> +	if (starvis2->lane_mode == STARVIS2_LANEMODE_4L)
> +		hmax = max(hmax >> 1, variant->hmax_min_pixel_array);
> +
>  	hblank = hmax * starvis2->variant->pix_per_clk -
>  					starvis2->variant->active_area.width;
>  	max_hblank = STARVIS2_HMAX_MAX * starvis2->variant->pix_per_clk -
> @@ -1285,16 +1316,20 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &starvis2_ctrl_ops, &props);
>  
>  	if (ctrl_hdlr->error) {
> -		ret = ctrl_hdlr->error;
>  		dev_err(&client->dev, "%s control init failed (%d)\n",
>  			__func__, ret);
> -		v4l2_ctrl_handler_free(ctrl_hdlr);
> -		return ret;
> +		goto err_free_handler;
>  	}
>  
>  	starvis2->sd.ctrl_handler = ctrl_hdlr;
>  
>  	return 0;
> +
> +err_free_handler:
> +	ret = ctrl_hdlr->error;
> +	v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> +	return ret;

The error codes will be ctrl_hdlr->error here so you could omit assigning
to ret. You're actually doing that twice above. :-)

>  }
>  
>  static int starvis2_check_hwcfg(struct device *dev, struct starvis2 *starvis2)
> 

-- 
Regards,

Sakari Ailus