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

[Balakrishnan.S]

Hi Eugen,

On 17/07/26 10:59 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/29/26 14:09, Balakrishnan Sambath wrote:
>> ISC_WB_O_* and ISC_WB_G_* pack two 13-bit fields per register. Sign
>> extension from negative offsets corrupts the upper field. Mask both
>> fields to 13 bits before packing.
>>
>> 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  | 22 ++++++++++++++--------
>>   1 file changed, 14 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..79a58efb6333 100644
>> --- a/drivers/media/platform/microchip/microchip-isc-base.c
>> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
>> @@ -62,18 +62,24 @@ static inline void isc_update_awb_ctrls(struct isc_device *isc)
>>
>>        /* In here we set our actual hw pipeline config */
>>
>> +     /*
>> +      * Offsets are 13-bit signed fields [12:0] and [28:16]. Cast to
>> +      * u32 and mask to 13 bits so sign extension of a negative value
>> +      * cannot corrupt the adjacent field.
>> +      */
>>        regmap_write(isc->regmap, ISC_WB_O_RGR,
>> -                  ((ctrls->offset[ISC_HIS_CFG_MODE_R])) |
>> -                  ((ctrls->offset[ISC_HIS_CFG_MODE_GR]) << 16));
>> +                  ((u32)ctrls->offset[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
>> +                  (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>>        regmap_write(isc->regmap, ISC_WB_O_BGB,
>> -                  ((ctrls->offset[ISC_HIS_CFG_MODE_B])) |
>> -                  ((ctrls->offset[ISC_HIS_CFG_MODE_GB]) << 16));
>> +                  ((u32)ctrls->offset[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
>> +                  (((u32)ctrls->offset[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
>> +     /* Gains are 13-bit unsigned fields [12:0] and [28:16] */
>>        regmap_write(isc->regmap, ISC_WB_G_RGR,
>> -                  ctrls->gain[ISC_HIS_CFG_MODE_R] |
>> -                  (ctrls->gain[ISC_HIS_CFG_MODE_GR] << 16));
>> +                  (ctrls->gain[ISC_HIS_CFG_MODE_R] & GENMASK(12, 0)) |
>> +                  ((ctrls->gain[ISC_HIS_CFG_MODE_GR] & GENMASK(12, 0)) << 16));
>>        regmap_write(isc->regmap, ISC_WB_G_BGB,
>> -                  ctrls->gain[ISC_HIS_CFG_MODE_B] |
>> -                  (ctrls->gain[ISC_HIS_CFG_MODE_GB] << 16));
>> +                  (ctrls->gain[ISC_HIS_CFG_MODE_B] & GENMASK(12, 0)) |
>> +                  ((ctrls->gain[ISC_HIS_CFG_MODE_GB] & GENMASK(12, 0)) << 16));
>>   }
>>
> 
> Do you think you can rewrite these using FIELD_PREP ? It's getting a
> little complicated to understand what is being written here.

Makes sense, the WB offset/gain writes read much better that way.
Will rewrite them with FIELD_PREP in v3.

Thanks,
Balakrishnan

> 
> 
>>   static inline void isc_reset_awb_ctrls(struct isc_device *isc)
>>
>