Re: [PATCH 2/2] media: i2c: dw9719: Add DW9714VA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] media: dw9719: silent truncation of DW9714VA default SAC mode
--

commit 4fa9a1b834161c5e052c734eb425a78a00978e18
Author: Yibo Wang <wyibo6109@gmail.com>

    media: i2c: dw9719: Add DW9714VA support
    
    The dongwoon,dw9714va compatible names the DW9714V revision found on
    the OnePlus Pad 4 ("iceland") rear camera module.  DW9714VA is a
    coined name, not an official part number; the revision shares its
    IC INFO id (0xFE) with the other DW9714V revisions.

> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> index 3b7ba88fd67c0..2afe37361bff3 100644
> --- a/drivers/media/i2c/dw9719.c
> +++ b/drivers/media/i2c/dw9719.c

[ ... ]

> @@ -153,6 +163,16 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  			return ret;
>  
>  		switch (val) {
> +		case DW9714V_ID:
> +			dw9719->model = DW9714VA;
> +			/*
> +			 * No SAC tuning data exists for DW9714VA modules in
> +			 * general; start from the family defaults and let DT
> +			 * provide the module-specific values.
> +			 */
> +			dw9719->sac_mode = DW9719_DEFAULT_SAC;
> +			dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
> +			break;
>  		case DW9719_ID:
>  			dw9719->model = DW9719;

[ ... ]

> @@ -188,6 +208,7 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  	}
>  
>  	switch (dw9719->model) {
> +	case DW9714VA:
>  	case DW9800K:
>  		cci_write(dw9719->regmap, DW9719_CONTROL, DW9719_ENABLE_RINGING, &ret);
>  		cci_write(dw9719->regmap, DW9719_MODE,
>  			  dw9719->sac_mode << DW9800K_MODE_SAC_SHIFT, &ret);

[Severity: Medium]
When the device tree does not provide a module-specific SAC tuning via the
dongwoon,sac-mode property, the driver defaults dw9719->sac_mode to
DW9719_DEFAULT_SAC (4) for the DW9714VA model.

Does writing this default value to the DW9719_MODE register cause a truncation?

When dw9719->sac_mode (4) is shifted by DW9800K_MODE_SAC_SHIFT (6), the result
is 256 (0x100). Because DW9719_MODE is an 8-bit register, the value will be
silently truncated to 0 when cci_write() copies it to an 8-bit buffer:

drivers/media/v4l2-core/v4l2-cci.c:cci_write() {
    ...
	switch (len) {
	case 1:
		buf[0] = val;
		break;
    ...
}

Could this result in an unintended hardware configuration being applied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016090930.947830-1-wyibo6109@gmail.com?part=2