Re: [PATCH 07/10] media: microchip-isc: fix pfe_cfg0_bps comment

[<Balakrishnan.S@microchip.com>]

Hi Eugen,

On 21/06/26 11:57 am, Eugen Hristev wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/16/26 14:51, Balakrishnan Sambath wrote:
>> The @pfe_cfg0_bps comment claimed the field holds the "number of
>> hardware data lines connected to the ISC". It does not. The field
>> stores the pre-shifted PFE_CFG0 BPS value (e.g. ISC_PFE_CFG0_BPS_EIGHT,
>> which is 0x4 << 28) and is ORed straight into the PFE_CFG0 register
>> word in microchip-isc-base.c.
>>
>> The old wording invites a reader to treat it as a small bit-depth
>> integer (8, 10, 12) and compare or do arithmetic on it directly, which
>> silently breaks since the value is shifted into bits 30:28. Document
>> what the field really holds and how to read the bit-depth back out with
>> FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...).
> 
> In this commit message I would focus on the correct meaning of
> pfe_cfg0_bps and stop inferring what a reader *might* have understood.
> Let's just fix it to be right, explain the right way, and forget the
> wrong way.

Sure. Will fix the comment to just say what the field holds.

> 
> But it looks like we could improve this to hold an actual meaningful
> data here, instead of a preshifted value, and rather shift it to the
> register when the hardware needs it.

Yes that seems better. Will store the actual bps value and shift it at 
the PFE_CFG0 write where its needed, will that work ?

If so will fix both in next version.

Thanks,
Balakrishnan

> 
> Eugen
> 
>>
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/media/platform/microchip/microchip-isc.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/microchip/microchip-isc.h b/drivers/media/platform/microchip/microchip-isc.h
>> index f5e322c2e36b..b084459f4583 100644
>> --- a/drivers/media/platform/microchip/microchip-isc.h
>> +++ b/drivers/media/platform/microchip/microchip-isc.h
>> @@ -62,7 +62,11 @@ struct isc_subdev_entity {
>>    * @mbus_code:               V4L2 media bus format code.
>>    * @cfa_baycfg:              If this format is RAW BAYER, indicate the type of bayer.
>>                        this is either BGBG, RGRG, etc.
>> - * @pfe_cfg0_bps:    Number of hardware data lines connected to the ISC
>> + * @pfe_cfg0_bps:    Pre-shifted ISC_PFE_CFG0 BPS field value (e.g.
>> +                     ISC_PFE_CFG0_BPS_EIGHT), not a plain bit-depth integer.
>> +                     OR it directly into the PFE_CFG0 register word, or use
>> +                     FIELD_GET(ISC_PFE_CFG0_BPS_MASK, ...) to obtain the
>> +                     3-bit BPS field value.
>>    * @raw:             If the format is raw bayer.
>>    */
>>
>>
>