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

[Eugen Hristev]

On 6/22/26 14:32, Balakrishnan.S@xxxxxxxxxxxxx wrote:
> 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 ?

Try it out, and if it's better, yes, send it in the next version. If
it's not looking good, reply with a snippet/reason why

Eugen
> 
> If so will fix both in next version.
> 
> Thanks,
> Balakrishnan
> 
>>
>> Eugen
>>
>>>
>>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>>> ---
>>>   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.
>>>    */
>>>
>>>
>>
>