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

[Eugen Hristev]

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.

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.

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.
>   */
>  
>