Re: [PATCH] media: atomisp: remove private white balance IOCTLs

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

Hi Karthikey,

On Wed, Dec 31, 2025 at 10:54:27AM +0530, Karthikey D Kadati wrote:
> This patch resolves a MUST-FIX graduation blocker identified in the
> atomisp TODO by removing the private ATOMISP_IOC_G_ISP_WHITE_BALANCE
> and ATOMISP_IOC_S_ISP_WHITE_BALANCE and replacing them with standard
> V4L2 control handling.
> 
> The private IOCTLs were used to set white balance parameters. This
> functionality is now mapped to the standard V4L2 controls
> V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE.
> 
> A helper function `atomisp_v4l2_set_wb` is introduced to translate the
> V4L2 control values to the driver's internal configuration format.
> 
> Signed-off-by: Karthikey D Kadati <karthikey3608@gmail.com>
> ---
>  .../media/atomisp/include/linux/atomisp.h     |  5 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 49 ++++++++++++++++---
>  2 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
> index 3c8fa3f58..fcf116cc4 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
> @@ -741,10 +741,7 @@ enum atomisp_burst_capture_options {
>  	_IOW('v', BASE_VIDIOC_PRIVATE + 15, struct atomisp_ctc_table)
>  
>  /* white balance Correction */
> -#define ATOMISP_IOC_G_ISP_WHITE_BALANCE \
> -	_IOR('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config)
> -#define ATOMISP_IOC_S_ISP_WHITE_BALANCE \
> -	_IOW('v', BASE_VIDIOC_PRIVATE + 16, struct atomisp_wb_config)
> +
>  
>  /* fpn table loading */
>  #define ATOMISP_IOC_S_ISP_FPN_TABLE \
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index bb8b2f221..5c0a1d92b 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -1083,6 +1083,38 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
>   * applications initialize the id and value fields of a struct v4l2_control
>   * and call this ioctl.
>   */
> +static int atomisp_v4l2_set_wb(struct atomisp_sub_device *asd, int id,
> +			       int value)
> +{
> +	struct atomisp_device *isp = asd->isp;
> +	struct atomisp_wb_config config;
> +	int ret;
> +
> +	if (atomisp_css_get_wb_config(asd, &config)) {

I'm not sure this makes sense. How will the caller know the value of
integer_bits?

I'd think the IOCTL interface of this driver should probably be largely
removed, to be replaced by the parameter buffer.

> +		dev_err(isp->dev, "%s: can't get wb config\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	switch (id) {
> +	case V4L2_CID_BLUE_BALANCE:
> +		config.b = value << (16 - 8 - config.integer_bits + 1);
> +		break;
> +	case V4L2_CID_RED_BALANCE:
> +		config.r = value << (16 - 8 - config.integer_bits + 1);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = atomisp_white_balance_param(asd, 1, &config);
> +	if (ret) {
> +		dev_err(isp->dev, "%s: set wb config failed\n", __func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int atomisp_s_ctrl(struct file *file, void *fh,
>  			  struct v4l2_control *control)
>  {
> @@ -1122,6 +1154,17 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
>  	case V4L2_CID_ATOMISP_LOW_LIGHT:
>  		ret = atomisp_low_light(asd, 1, &control->value);
>  		break;
> +	case V4L2_CID_AUTO_WHITE_BALANCE:
> +		/*
> +		 * TODO: Auto White Balance is not supported yet.
> +		 * It is currently handled by the ISP.
> +		 */
> +		ret = 0;
> +		break;
> +	case V4L2_CID_RED_BALANCE:
> +	case V4L2_CID_BLUE_BALANCE:
> +		ret = atomisp_v4l2_set_wb(asd, control->id, control->value);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -1484,13 +1527,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
>  		err = atomisp_ctc(asd, 1, arg);
>  		break;
>  
> -	case ATOMISP_IOC_G_ISP_WHITE_BALANCE:
> -		err = atomisp_white_balance_param(asd, 0, arg);
> -		break;
>  
> -	case ATOMISP_IOC_S_ISP_WHITE_BALANCE:
> -		err = atomisp_white_balance_param(asd, 1, arg);
> -		break;
>  
>  	case ATOMISP_IOC_G_3A_CONFIG:
>  		err = atomisp_3a_config_param(asd, 0, arg);

-- 
Regards,

Sakari Ailus