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

[Balakrishnan.S]

Hi Eugen,

On 21/07/26 6:49 pm, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>> ---
>>   .../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.

Yeah, it makes sense if no objections let me chop it off.

Thanks,
Balakrishnan

> 
> 
>>        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
>>
>>
>