[PATCH v1] media: i2c: alvium: Fix controls for WB/AWB

[PATCH v1] media: i2c: alvium: Fix controls for WB/AWB

[Martin Hecht]

With that patch the controls for red-balance and blue-balance were created
only if the particular camera supports that. Otherwise the pointers on
the control variable are initialized with NULL to prevent side effects for
clustering with AWB control.

Fixes: 0a7af872915e ("media: i2c: Add support for alvium camera")
Signed-off-by: Martin Hecht <mhecht73@gmail.com>
---
 drivers/media/i2c/alvium-csi2.c | 37 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
index b62b45a4f2fc..4c6934e9e177 100644
--- a/drivers/media/i2c/alvium-csi2.c
+++ b/drivers/media/i2c/alvium-csi2.c
@@ -2108,26 +2108,33 @@ static int alvium_ctrl_init(struct alvium_dev *alvium)
 						  0, 0, &alvium->link_freq);
 	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	if (alvium->avail_ft.whiteb) {
+		ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
+							V4L2_CID_BLUE_BALANCE,
+							alvium->min_bbalance,
+							alvium->max_bbalance,
+							alvium->inc_bbalance,
+							alvium->dft_bbalance);
+		ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
+						       V4L2_CID_RED_BALANCE,
+						       alvium->min_rbalance,
+						       alvium->max_rbalance,
+						       alvium->inc_rbalance,
+						       alvium->dft_rbalance);
+	} else {
+		/* set to NULL for v4l2_ctrl_auto_cluster if not existing */
+		ctrls->blue_balance	= NULL;
+		ctrls->red_balance = NULL;
+	}
+
 	/* Auto/manual white balance */
 	if (alvium->avail_ft.auto_whiteb) {
 		ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
 						   V4L2_CID_AUTO_WHITE_BALANCE,
 						   0, 1, 1, 1);
-		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, false);
-	}
-
-	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
-						V4L2_CID_BLUE_BALANCE,
-						alvium->min_bbalance,
-						alvium->max_bbalance,
-						alvium->inc_bbalance,
-						alvium->dft_bbalance);
-	ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
-					       V4L2_CID_RED_BALANCE,
-					       alvium->min_rbalance,
-					       alvium->max_rbalance,
-					       alvium->inc_rbalance,
-					       alvium->dft_rbalance);
+
+		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, true);
+	}
 
 	/* Auto/manual exposure */
 	if (alvium->avail_ft.auto_exp) {
-- 
2.43.0

Re: [PATCH v1] media: i2c: alvium: Fix controls for WB/AWB

[Sakari Ailus]

Hi Martin,

Thanks for the patch.

On Tue, May 05, 2026 at 04:25:10PM +0200, Martin Hecht wrote:
> With that patch the controls for red-balance and blue-balance were created
> only if the particular camera supports that. Otherwise the pointers on
> the control variable are initialized with NULL to prevent side effects for
> clustering with AWB control.
> 
> Fixes: 0a7af872915e ("media: i2c: Add support for alvium camera")
> Signed-off-by: Martin Hecht <mhecht73@gmail.com>
> ---
>  drivers/media/i2c/alvium-csi2.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
> index b62b45a4f2fc..4c6934e9e177 100644
> --- a/drivers/media/i2c/alvium-csi2.c
> +++ b/drivers/media/i2c/alvium-csi2.c
> @@ -2108,26 +2108,33 @@ static int alvium_ctrl_init(struct alvium_dev *alvium)
>  						  0, 0, &alvium->link_freq);
>  	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;

This is a problem. Can you move setting the flags after checking the
handler's error status? The functions adding controls may fail and this is
simply a missing error check.

Can you submit a fix, with a Fixes: tag and this patch should be rebased on
the fix, please?

>  
> +	if (alvium->avail_ft.whiteb) {
> +		ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
> +							V4L2_CID_BLUE_BALANCE,
> +							alvium->min_bbalance,
> +							alvium->max_bbalance,
> +							alvium->inc_bbalance,
> +							alvium->dft_bbalance);
> +		ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
> +						       V4L2_CID_RED_BALANCE,
> +						       alvium->min_rbalance,
> +						       alvium->max_rbalance,
> +						       alvium->inc_rbalance,
> +						       alvium->dft_rbalance);
> +	} else {
> +		/* set to NULL for v4l2_ctrl_auto_cluster if not existing */
> +		ctrls->blue_balance	= NULL;
> +		ctrls->red_balance = NULL;

Aren't the two NULL already before this?

> +	}
> +
>  	/* Auto/manual white balance */
>  	if (alvium->avail_ft.auto_whiteb) {
>  		ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
>  						   V4L2_CID_AUTO_WHITE_BALANCE,
>  						   0, 1, 1, 1);
> -		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, false);
> -	}
> -
> -	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
> -						V4L2_CID_BLUE_BALANCE,
> -						alvium->min_bbalance,
> -						alvium->max_bbalance,
> -						alvium->inc_bbalance,
> -						alvium->dft_bbalance);
> -	ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
> -					       V4L2_CID_RED_BALANCE,
> -					       alvium->min_rbalance,
> -					       alvium->max_rbalance,
> -					       alvium->inc_rbalance,
> -					       alvium->dft_rbalance);
> +
> +		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, true);
> +	}
>  
>  	/* Auto/manual exposure */
>  	if (alvium->avail_ft.auto_exp) {

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1] media: i2c: alvium: Fix controls for WB/AWB

[Martin Hecht]

Hi Sakari,

thank you for the comments.

On 5/6/26 11:28, Sakari Ailus wrote:
> Hi Martin,
> 
> Thanks for the patch.
> 
> On Tue, May 05, 2026 at 04:25:10PM +0200, Martin Hecht wrote:
>> With that patch the controls for red-balance and blue-balance were created
>> only if the particular camera supports that. Otherwise the pointers on
>> the control variable are initialized with NULL to prevent side effects for
>> clustering with AWB control.
>>
>> Fixes: 0a7af872915e ("media: i2c: Add support for alvium camera")
>> Signed-off-by: Martin Hecht <mhecht73@gmail.com>
>> ---
>>   drivers/media/i2c/alvium-csi2.c | 37 ++++++++++++++++++++-------------
>>   1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
>> index b62b45a4f2fc..4c6934e9e177 100644
>> --- a/drivers/media/i2c/alvium-csi2.c
>> +++ b/drivers/media/i2c/alvium-csi2.c
>> @@ -2108,26 +2108,33 @@ static int alvium_ctrl_init(struct alvium_dev *alvium)
>>   						  0, 0, &alvium->link_freq);
>>   	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
> This is a problem. Can you move setting the flags after checking the
> handler's error status? The functions adding controls may fail and this is
> simply a missing error check.
> 
> Can you submit a fix, with a Fixes: tag and this patch should be rebased on
> the fix, please?

I'm preparing a separate fix for that issue. It's the same situation 
also  for some other controls like pixel_rate and link_frequency but not 
only. Can I combine that into one patch for fix only that in 
alvium_ctrl_init?

> 
>>   
>> +	if (alvium->avail_ft.whiteb) {
>> +		ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
>> +							V4L2_CID_BLUE_BALANCE,
>> +							alvium->min_bbalance,
>> +							alvium->max_bbalance,
>> +							alvium->inc_bbalance,
>> +							alvium->dft_bbalance);
>> +		ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
>> +						       V4L2_CID_RED_BALANCE,
>> +						       alvium->min_rbalance,
>> +						       alvium->max_rbalance,
>> +						       alvium->inc_rbalance,
>> +						       alvium->dft_rbalance);
>> +	} else {
>> +		/* set to NULL for v4l2_ctrl_auto_cluster if not existing */
>> +		ctrls->blue_balance	= NULL;
>> +		ctrls->red_balance = NULL;
> 
> Aren't the two NULL already before this?

You are right. It's zeroed before because __GFP_ZERO in devm_kzalloc. 
Will remove that redundant code.

> 
>> +	}
>> +
>>   	/* Auto/manual white balance */
>>   	if (alvium->avail_ft.auto_whiteb) {
>>   		ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
>>   						   V4L2_CID_AUTO_WHITE_BALANCE,
>>   						   0, 1, 1, 1);
>> -		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, false);
>> -	}
>> -
>> -	ctrls->blue_balance = v4l2_ctrl_new_std(hdl, ops,
>> -						V4L2_CID_BLUE_BALANCE,
>> -						alvium->min_bbalance,
>> -						alvium->max_bbalance,
>> -						alvium->inc_bbalance,
>> -						alvium->dft_bbalance);
>> -	ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops,
>> -					       V4L2_CID_RED_BALANCE,
>> -					       alvium->min_rbalance,
>> -					       alvium->max_rbalance,
>> -					       alvium->inc_rbalance,
>> -					       alvium->dft_rbalance);
>> +
>> +		v4l2_ctrl_auto_cluster(3, &ctrls->auto_wb, 0, true);
>> +	}
>>   
>>   	/* Auto/manual exposure */
>>   	if (alvium->avail_ft.auto_exp) {
> 

BR Martin

Re: [PATCH v1] media: i2c: alvium: Fix controls for WB/AWB

[Sakari Ailus]

Hi Martin,

On Wed, May 06, 2026 at 12:16:13PM +0200, Martin Hecht wrote:
> Hi Sakari,
> 
> thank you for the comments.
> 
> On 5/6/26 11:28, Sakari Ailus wrote:
> > Hi Martin,
> > 
> > Thanks for the patch.
> > 
> > On Tue, May 05, 2026 at 04:25:10PM +0200, Martin Hecht wrote:
> > > With that patch the controls for red-balance and blue-balance were created
> > > only if the particular camera supports that. Otherwise the pointers on
> > > the control variable are initialized with NULL to prevent side effects for
> > > clustering with AWB control.
> > > 
> > > Fixes: 0a7af872915e ("media: i2c: Add support for alvium camera")
> > > Signed-off-by: Martin Hecht <mhecht73@gmail.com>
> > > ---
> > >   drivers/media/i2c/alvium-csi2.c | 37 ++++++++++++++++++++-------------
> > >   1 file changed, 22 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c
> > > index b62b45a4f2fc..4c6934e9e177 100644
> > > --- a/drivers/media/i2c/alvium-csi2.c
> > > +++ b/drivers/media/i2c/alvium-csi2.c
> > > @@ -2108,26 +2108,33 @@ static int alvium_ctrl_init(struct alvium_dev *alvium)
> > >   						  0, 0, &alvium->link_freq);
> > >   	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > 
> > This is a problem. Can you move setting the flags after checking the
> > handler's error status? The functions adding controls may fail and this is
> > simply a missing error check.
> > 
> > Can you submit a fix, with a Fixes: tag and this patch should be rebased on
> > the fix, please?
> 
> I'm preparing a separate fix for that issue. It's the same situation also
> for some other controls like pixel_rate and link_frequency but not only. Can
> I combine that into one patch for fix only that in alvium_ctrl_init?

Please do.

-- 
Regards,

Sakari Ailus