Re: [PATCH v3 10/10] media: microchip-isc: fix WB offset and gain register field masking

[Eugen Hristev <ehristev@kernel.org>]

On 7/21/26 13:29, Balakrishnan Sambath wrote:
> ISC_WB_O_* and ISC_WB_G_* each pack two 13-bit fields. A negative offset
> sign-extends and corrupts the adjacent field. Add masks for the two
> fields and write them with FIELD_PREP(), which masks each value into its
> field, so sign extension can no longer bleed across.
> 
> Fixes: 91b4e487b0c6 ("media: microchip: add ISC driver as Microchip ISC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  .../media/platform/microchip/microchip-isc-base.c   | 21 +++++++++++++--------
>  .../media/platform/microchip/microchip-isc-regs.h   |  6 ++++++
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 1a9b97edfa32..3d25d7d28652 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -62,18 +62,23 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
>  
>  	/* In here we set our actual hw pipeline config */
>  
> +	/*
> +	 * Each register packs two 13-bit fields. FIELD_PREP() masks every
> +	 * value into its field, so sign extension of a negative offset can
> +	 * no longer bleed into the adjacent field.
> +	 */

I guess this comment does not make sense. It refers to a bad situation
in the past that is no longer valid. In time, it will be forgotten and
it does not make sense to mention it here.
At least my opinion on it.


>  	regmap_write(isc->regmap, ISC_WB_O_RGR,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
> +		     FIELD_PREP(ISC_WB_O_LO, ctrls->offset[ISC_HIS_CFG_MODE_R]) |
> +		     FIELD_PREP(ISC_WB_O_HI, ctrls->offset[ISC_HIS_CFG_MODE_GR]));
>  	regmap_write(isc->regmap, ISC_WB_O_BGB,
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
> -		     ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
> +		     FIELD_PREP(ISC_WB_O_LO, ctrls->offset[ISC_HIS_CFG_MODE_B]) |
> +		     FIELD_PREP(ISC_WB_O_HI, ctrls->offset[ISC_HIS_CFG_MODE_GB]));
>  	regmap_write(isc->regmap, ISC_WB_G_RGR,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_R] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
> +		     FIELD_PREP(ISC_WB_G_LO, ctrls->gain[ISC_HIS_CFG_MODE_R]) |
> +		     FIELD_PREP(ISC_WB_G_HI, ctrls->gain[ISC_HIS_CFG_MODE_GR]));
>  	regmap_write(isc->regmap, ISC_WB_G_BGB,
> -		     ctrls->gain[ISC_HIS_CFG_MODE_B] |
> -		     (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
> +		     FIELD_PREP(ISC_WB_G_LO, ctrls->gain[ISC_HIS_CFG_MODE_B]) |
> +		     FIELD_PREP(ISC_WB_G_HI, ctrls->gain[ISC_HIS_CFG_MODE_GB]));
>  }
>  
>  static inline void isc_reset_awb_ctrls(struct isc_device *isc)
> diff --git a/drivers/media/platform/microchip/microchip-isc-regs.h b/drivers/media/platform/microchip/microchip-isc-regs.h
> index 9ddbbb6dd68b..fe145b142b82 100644
> --- a/drivers/media/platform/microchip/microchip-isc-regs.h
> +++ b/drivers/media/platform/microchip/microchip-isc-regs.h
> @@ -149,6 +149,12 @@
>  /* ISC White Balance Gain for B, GB Register */
>  #define ISC_WB_G_BGB	0x0000006c
>  
> +/* Each WB offset/gain register packs two 13-bit fields, low and high */
> +#define ISC_WB_O_LO	GENMASK(12, 0)	/* R or B  offset [12:0]  */
> +#define ISC_WB_O_HI	GENMASK(28, 16)	/* GR or GB offset [28:16] */
> +#define ISC_WB_G_LO	GENMASK(12, 0)	/* R or B  gain [12:0]  */
> +#define ISC_WB_G_HI	GENMASK(28, 16)	/* GR or GB gain [28:16] */
> +
>  /* ISC Color Filter Array Control Register */
>  #define ISC_CFA_CTRL    0x00000070
>  
>