Re: [PATCH 1/2] drm/komeda: Fix bits parsing of GLB_CORE_ID

[Liviu Dudau <liviu.dudau@arm.com>]

On Tue, Jul 21, 2026 at 02:52:26PM +0100, Raveendra Talabattula wrote:
> GLB_CORE_ID contains the product and version information returned by the
> hardware. The current macros parses VERSION_MINOR as a 4-bit field and
> VERSION_STATUS as an 8-bit field, which does not match the register
> layout. As a result, the driver reports an incorrect version number.
> 
> Fix the parsing macros to match the hardware specification:
> - VERSION_MINOR is 8 bits at [11:4]
> - VERSION_STATUS is 4 bits at [3:0]
> 
> Fixes: bd628c1bed79 ("drm/komeda: komeda_dev/pipeline/component definition and initialzation")
> Signed-off-by: Asad Malik <asad.malik@arm.com>
> Signed-off-by: Raveendra Talabattula <raveendra.talabattula@arm.com>
> ---
>  drivers/gpu/drm/arm/display/include/malidp_product.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> index 6f954bcdf40e..bf63c4e05c3e 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> @@ -8,14 +8,18 @@
>  #define _MALIDP_PRODUCT_H_
>  
>  /* Product identification */
> +/* GLB_CORE_ID fields as per HW specification:
> + * MINOR is 8 bits ([11:4]) and STATUS is 4 bits ([3:0]).
> + * Update masks/shifts accordingly.
> + */
>  #define MALIDP_CORE_ID(__product, __major, __minor, __status) \
>  	((((__product) & 0xFFFF) << 16) | (((__major) & 0xF) << 12) | \
> -	(((__minor) & 0xF) << 8) | ((__status) & 0xFF))
> +	(((__minor) & 0xFF) << 4) | ((__status) & 0xF))
>  
> -#define MALIDP_CORE_ID_PRODUCT_ID(__core_id) ((__u32)(__core_id) >> 16)
> +#define MALIDP_CORE_ID_PRODUCT_ID(__core_id) (((__u32)(__core_id) >> 16) & 0xFFFF)

This change is redundant.

>  #define MALIDP_CORE_ID_MAJOR(__core_id)      (((__u32)(__core_id) >> 12) & 0xF)
> -#define MALIDP_CORE_ID_MINOR(__core_id)      (((__u32)(__core_id) >> 8) & 0xF)
> -#define MALIDP_CORE_ID_STATUS(__core_id)     (((__u32)(__core_id)) & 0xFF)
> +#define MALIDP_CORE_ID_MINOR(__core_id)      (((__u32)(__core_id) >>  4) & 0xFF)
> +#define MALIDP_CORE_ID_STATUS(__core_id)     (((__u32)(__core_id) >>  0) & 0xF)

Shift by zero is unnecessary. Simple bitwise AND is enough.


Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

>  
>  /* Mali-display product IDs */
>  #define MALIDP_D71_PRODUCT_ID	0x0071
> -- 
> 2.43.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯