Re: [PATCH v6] staging: media: atomisp: fix GP_TIMER_BASE scope in gp_timer.c

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Anushka,

Thanks for the update.

On Fri, Mar 27, 2026 at 08:41:06AM +0530, Anushka Badhe wrote:
> GP_TIMER_BASE is only used in gp_timer.c and it does not need to be
> globally visible.
> 
> Move its declaration from system_local.c to gp_timer.c and make it file
> local by marking it static. Remove external declaration from system_local.h
> and its usage in gp_timer.h
> 
> This fixes a sparse warning about global visibility and cleans up
> unnecessary global exposure.
> 
> Signed-off-by: Anushka Badhe <anushkabadhe@gmail.com>
> ---
> Changes in v6:
> - Mark scope of GP_TIMER_BASE static
> 
>  Changes in v5:
> - Move GP_TIMER_BASE definition to gp_timer.c
> - Remove extern from system_local.h
> - Remove include of system_local.h from gp_timer.h
> 
> Changes in v4:
> - Remove unrelated block comment style fixes
> 
> Changes in v3:
> - Add commit description
> - Fix subject prefix to staging: media: atomisp:
> 
> Changes in v2:
> - Fix block comment style (move closing */ to its own line)
> - Merge split GP_TIMER_BASE declaration onto a single line
> 
> Note:
> * This patch is part of the GSoC2026 application process for device tree
> binding
> s conversions
> * https://github.com/LinuxFoundationGSoC/ProjectIdeas/wiki/GSoC-2026-Device-Tree-Bindings
> 
>  .../media/atomisp/pci/hive_isp_css_common/host/gp_timer.c  | 7 ++++++-
>  .../media/atomisp/pci/hive_isp_css_include/gp_timer.h      | 1 -
>  drivers/staging/media/atomisp/pci/system_local.c           | 6 ------
>  drivers/staging/media/atomisp/pci/system_local.h           | 5 -----
>  4 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c
> index d04c179a5ecd..0c1b67988dd9 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/gp_timer.c
> @@ -11,7 +11,12 @@
>  #ifndef __INLINE_GP_TIMER__
>  #include "gp_timer_private.h"  /*device_access.h*/
>  #endif /* __INLINE_GP_TIMER__ */
> -#include "system_local.h"
> +
> +/*GP TIMER , all timer registers are inter-twined,
> + * so, having multiple base addresses for
> + * different timers does not help
> + */
> +static const hrt_address GP_TIMER_BASE = (hrt_address)0x0000000000000600ULL;

Please don't move the defition here. There's a reason for keeping it in the
same location with the rest of the offsets. There's a lot to cleanup here
but what should be done is roughly:

- Make these constants macros (with IPU2_ or ATOMISP2_ prefix?) and move
  them into a separate header (perhaps with register definitions?).

- Remove my_env and make struct device (or maybe struct atomisp_device?) as
  a parameter for register access functions.

This may get a bit complicated due to the amount of cleanup needed so
having the hardware for testing would be rather essential.

>  
>  /* FIXME: not sure if reg_load(), reg_store() should be API.
>   */
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_include/gp_timer.h b/drivers/staging/media/atomisp/pci/hive_isp_css_include/gp_timer.h
> index 94f81af70007..e651d9ef1114 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_include/gp_timer.h
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_include/gp_timer.h
> @@ -21,7 +21,6 @@
>   *	- local:   system and cell specific constants and identifiers
>   */
>  
> -#include "system_local.h"    /*GP_TIMER_BASE address */
>  #include "gp_timer_local.h"  /*GP_TIMER register offsets */
>  
>  #ifndef __INLINE_GP_TIMER__
> diff --git a/drivers/staging/media/atomisp/pci/system_local.c b/drivers/staging/media/atomisp/pci/system_local.c
> index a8a93760d5b1..8d4fd80f8984 100644
> --- a/drivers/staging/media/atomisp/pci/system_local.c
> +++ b/drivers/staging/media/atomisp/pci/system_local.c
> @@ -83,12 +83,6 @@ const hrt_address GP_DEVICE_BASE[N_GP_DEVICE_ID] = {
>  	0x0000000000000000ULL
>  };
>  
> -/*GP TIMER , all timer registers are inter-twined,
> - * so, having multiple base addresses for
> - * different timers does not help*/

This comment could benefit from fixing, regarding both formatting and
language.

> -const hrt_address GP_TIMER_BASE =
> -    (hrt_address)0x0000000000000600ULL;
> -
>  /* GPIO */
>  const hrt_address GPIO_BASE[N_GPIO_ID] = {
>  	0x0000000000000400ULL
> diff --git a/drivers/staging/media/atomisp/pci/system_local.h b/drivers/staging/media/atomisp/pci/system_local.h
> index 970f4ef990ec..2bd46f5123fb 100644
> --- a/drivers/staging/media/atomisp/pci/system_local.h
> +++ b/drivers/staging/media/atomisp/pci/system_local.h
> @@ -53,11 +53,6 @@ extern const hrt_address FIFO_MONITOR_BASE[N_FIFO_MONITOR_ID];
>  /* GP_DEVICE (single base for all separate GP_REG instances) */
>  extern const hrt_address GP_DEVICE_BASE[N_GP_DEVICE_ID];
>  
> -/*GP TIMER , all timer registers are inter-twined,
> - * so, having multiple base addresses for
> - * different timers does not help*/
> -extern const hrt_address GP_TIMER_BASE;
> -
>  /* GPIO */
>  extern const hrt_address GPIO_BASE[N_GPIO_ID];
>  

-- 
Kind regards,

Sakari Ailus