Re: [PATCH v2 2/3] media: i2c: dw9719: Update DW9800K driver

[bod]

On 2026-09-14 12:25 +0200, Griffin Kroah-Hartman wrote:
> The previous DW9800K driver code was created without access to a
> datasheet for the part, now that we have one we can clean up some
> misunderstandings and add some features for further expandability.
> 
> Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@xxxxxxxxxxxxx>

1. Be more explicit about which misunderstandings and features you mean.
2. Generally speaking for each thing you might write in the commit log
   a) Misunderstanding X
   b) Thing fixed y

   I'd recommend making those standalone patches.
3. Since you appear to be describing Fixes: here you should
   c) Have a Fixes: tag
   d) Cc: stable@xxxxxxxxxxxxxxx

>  drivers/media/i2c/dw9719.c | 71 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> index 3b7ba88fd67c044ef9005a0aac6a5d26ab939a90..982be59b9ee17a12e8c25ab790ccb8351454c91c 100644
> --- a/drivers/media/i2c/dw9719.c
> +++ b/drivers/media/i2c/dw9719.c
> @@ -68,9 +68,18 @@
>  #define DW9761_VCM_PRELOAD		CCI_REG8(8)
>  #define DW9761_DEFAULT_VCM_PRELOAD	0x73
>  
> -#define DW9800K_DEFAULT_SAC		1
> +#define DW9800K_ID			0xEB
> +#define DW9800K_DEFAULT_SAC		3
>  #define DW9800K_MODE_SAC_SHIFT		6
> -#define DW9800K_DEFAULT_VCM_FREQ		0x10
> +
> +#define DW9800K_RESONANCE		CCI_REG8(7)
> +
> +#define DW9800K_DEFAULT_DIV		0x01
> +#define DW9800K_DIV_SHIFT		6
> +#define DW9800K_DIV_LOW_SHIFT		2
> +
> +#define DW9800K_DEFAULT_SACT		0x20
> +
>  
>  #define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
>  
> @@ -90,6 +99,8 @@ struct dw9719_device {
>  	u32 mode_low_bits;
>  	u32 sac_mode;
>  	u32 vcm_freq;
> +	u32 div;
> +	u32 sac_time;
>  
>  	struct dw9719_v4l2_ctrls {
>  		struct v4l2_ctrl_handler handler;
> @@ -140,10 +151,6 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  			dw9719->sac_mode = DW9718S_DEFAULT_SAC;
>  			dw9719->vcm_freq = DW9718S_DEFAULT_VCM_FREQ;
>  			goto props;
> -		case DW9800K:
> -			dw9719->sac_mode = DW9800K_DEFAULT_SAC;
> -			dw9719->vcm_freq = DW9800K_DEFAULT_VCM_FREQ;
> -			goto props;

As an example this change should be argued on its merits - you need to lead
your reviewers to water and then see how they'd like to drink.

>  		default:
>  			break;
>  		}
> @@ -165,6 +172,12 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  			dw9719->sac_mode = DW9761_DEFAULT_SAC;
>  			dw9719->vcm_freq = DW9761_DEFAULT_VCM_FREQ;
>  			break;
> +		case DW9800K_ID:
> +			dw9719->sac_mode = DW9800K_DEFAULT_SAC;
> +			dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
> +			dw9719->div = DW9800K_DEFAULT_DIV;
> +			dw9719->sac_time = DW9800K_DEFAULT_SACT;
> +			break;
>  		default:
>  			dev_err(dw9719->dev,
>  				"Error unknown device id 0x%02llx\n", val);
> @@ -185,14 +198,54 @@ static int dw9719_power_up(struct dw9719_device *dw9719, bool detect)
>  		/* Optional indication of VCM prescale */
>  		device_property_read_u32(dw9719->dev, "dongwoon,vcm-prescale",
>  					 &dw9719->vcm_freq);
> +
> +		/* Optional indication of clock pre-scale select */
> +		device_property_read_u32(dw9719->dev, "dongwoon,clock-presc",
> +					 &dw9719->div);
> +
> +		/* Optional indication of SAC timing */
> +		device_property_read_u32(dw9719->dev, "dongwoon,timing-control",
> +					 &dw9719->sac_time);
>  	}
>  
>  	switch (dw9719->model) {
>  	case DW9800K:
> -		cci_write(dw9719->regmap, DW9719_CONTROL, DW9719_ENABLE_RINGING, &ret);
> +		int ring, sac;
> +
> +		/*
> +		 * Enable ringing for SAC3-SAC5
> +		 *
> +		 * sac_mode	reg	Mode
> +		 * ===========================
> +		 *	0	0X	Direct
> +		 *	1	1X	LSC
> +		 *	2	00	SAC2
> +		 *	3	01	SAC3
> +		 *	4	10	SAC4
> +		 *	5	11	SAC5
> +		 */
> +		switch (dw9719->sac_mode) {
> +		case 0:
> +			ring = 0;
> +			sac = 0;
> +			break;
> +		case 1:
> +			ring = 0;
> +			sac = 2;
> +			break;
> +		default:
> +			ring = DW9719_ENABLE_RINGING;
> +			sac = dw9719->sac_mode - 2;
> +			break;
> +		}

For example what is ringing - a patch should cover "enable ringing it does
thing X" and then the code should be a straight-forward reading of thing X,
matching the commit log.
---
bod