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

[Anushka B]

On Fri, Mar 27, 2026 at 4:54 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
Would it be okay to proceed with the simpler in-place fix for
GP_TIMER_BASE declaration instead?

Thank you,
Anushka