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;
 
 /* 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*/
-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];
 
-- 
2.43.0

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

On Fri, Mar 27, 2026 at 4:54 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> 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

Hi Sakari,
Thank you for your reply.
> 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.
As a student, I don't currently have access to the hardware needed to
do the bigger refactor safely. However I'm happy to fix the comment
formatting and language.
In earlier versions, I had merged the split declaration of
GP_TIMER_BASE in place in system_local.c before later moving it to
gp_timer.c.
Link: https://lore.kernel.org/linux-media/20260325132434.55775-1-anushkabadhe@gmail.com/
Would it be okay to proceed with the simpler in-place fix for
GP_TIMER_BASE declaration instead?

Thank you,
Anushka

Hi Anushka,

On Mon, Mar 30, 2026 at 07:02:21AM +0530, Anushka B wrote:
> > 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.
> As a student, I don't currently have access to the hardware needed to
> do the bigger refactor safely. However I'm happy to fix the comment
> formatting and language.
> In earlier versions, I had merged the split declaration of
> GP_TIMER_BASE in place in system_local.c before later moving it to
> gp_timer.c.
> Link: https://lore.kernel.org/linux-media/20260325132434.55775-1-anushkabadhe@gmail.com/
> Would it be okay to proceed with the simpler in-place fix for
> GP_TIMER_BASE declaration instead?

I'm fine with v4.

-- 
Regards,

Sakari Ailus

Hi Sakari,

On Wed, Apr 1, 2026 at 12:25 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Mon, Mar 30, 2026 at 07:02:21AM +0530, Anushka B wrote:
> > > 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.
> > As a student, I don't currently have access to the hardware needed to
> > do the bigger refactor safely. However I'm happy to fix the comment
> > formatting and language.
> > In earlier versions, I had merged the split declaration of
> > GP_TIMER_BASE in place in system_local.c before later moving it to
> > gp_timer.c.
> > Link: https://lore.kernel.org/linux-media/20260325132434.55775-1-anushkabadhe@gmail.com/
> > Would it be okay to proceed with the simpler in-place fix for
> > GP_TIMER_BASE declaration instead?
>
> I'm fine with v4.

Should I send a v7 similar to v4, where I merged the split declaration
of GP_TIMER_BASE, and a separate patch for the comment fixes?

Thanks,
Anushka

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.

...

> ---
> Changes in v6:
> - Mark scope of GP_TIMER_BASE static

Good catch, but read my comment against v5. And slow down with new versions,
no new version within 24h, please!

-- 
With Best Regards,
Andy Shevchenko

On Fri, Mar 27, 2026 at 3:13 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> 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.
>
> ...
>
> > ---
> > Changes in v6:
> > - Mark scope of GP_TIMER_BASE static
>
> Good catch, but read my comment against v5. And slow down with new versions,
> no new version within 24h, please!
>
> --
> With Best Regards,
> Andy Shevchenko
>

Hi Andy,
I saw your comment on v5 regarding the GP TIMER comment style, thank
you for the example.
I also received feedback from Sakari on this file:
> Please don't move the defition here. There's a reason for keeping it in the
> same location with the rest of the offsets.
Would it be acceptable to go back to the simpler in-place merge in
system_local.c, joining the split
declaration of GP_TIMER_BASE? I can follow up with the comment
formatting as a separate patch.

I'll make sure to discuss changes before sending new revisions going forward.

Thank you,
Anushka

On Mon, Mar 30, 2026 at 4:19 AM Anushka B <anushkabadhe@gmail.com> wrote:
> On Fri, Mar 27, 2026 at 3:13 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Fri, Mar 27, 2026 at 08:41:06AM +0530, Anushka Badhe wrote:

...

> > > ---
> > > Changes in v6:
> > > - Mark scope of GP_TIMER_BASE static
> >
> > Good catch, but read my comment against v5. And slow down with new versions,
> > no new version within 24h, please!

> I saw your comment on v5 regarding the GP TIMER comment style, thank
> you for the example.
> I also received feedback from Sakari on this file:
> > Please don't move the defition here. There's a reason for keeping it in the
> > same location with the rest of the offsets.
> Would it be acceptable to go back to the simpler in-place merge in
> system_local.c, joining the split
> declaration of GP_TIMER_BASE? I can follow up with the comment
> formatting as a separate patch.

Sakari is the maintainer, follow what he says.

-- 
With Best Regards,
Andy Shevchenko