Re: [PATCH v1 02/27] drm/panthor: Move register access helpers out of panthor_device.h

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/panthor: missing and incorrect header dependencies in panthor_device_io.h
--

commit 4a15e81aefbe1480e274e929d06e5e1ad5234842
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Move register access helpers out of panthor_device.h

Move register access helpers out of panthor_device.h to allow reuse by
other panthor drivers that will be added to enable v15 support.

> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index b55a3f9edd414..54031c5252473 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -6,7 +6,6 @@
>  #ifndef __PANTHOR_DEVICE_H__
>  #define __PANTHOR_DEVICE_H__
>  
> -#include <linux/atomic.h>

[Severity: Low]
Is it okay to remove <linux/atomic.h> here? struct panthor_device (defined
further down in this file) relies heavily on atomic_t, such as for
retry_count and pending reset state. It seems to currently rely on
transitive inclusion from the newly added panthor_device_io.h.

>  #include <linux/io-pgtable.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>

[ ... ]

> diff --git a/drivers/gpu/drm/panthor/panthor_device_io.h b/drivers/gpu/drm/panthor/panthor_device_io.h
> new file mode 100644
> index 0000000000000..2287a0d9f86c2
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_device_io.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> +/* Copyright 2026 ARM Limited. All rights reserved. */
> +
> +#ifndef __PANTHOR_DEVICE_IO_H__
> +#define __PANTHOR_DEVICE_IO_H__
> +
> +#include <linux/atomic.h>

[Severity: Low]
Does this file actually need <linux/atomic.h>? It doesn't appear to use any
atomic types.

> +#include <linux/io.h>
> +
> +
> +static inline void gpu_write(void __iomem *iomem, u32 reg, u32 data)
> +{
> +	writel(data, iomem + reg);
> +}

[ ... ]

> +static inline void gpu_write64(void __iomem *iomem, u32 reg, u64 data)
> +{
> +	gpu_write(iomem, reg, lower_32_bits(data));
> +	gpu_write(iomem, reg + 4, upper_32_bits(data));
> +}

[Severity: Low]
Should <linux/wordpart.h> be included for lower_32_bits() and
upper_32_bits()? Files including this header directly might fail to build if
the dependency isn't met transitively.

> +static inline u64 gpu_read64(void __iomem *iomem, u32 reg)
> +{
> +	return (gpu_read(iomem, reg) | ((u64)gpu_read(iomem, reg + 4) << 32));
> +}

[ ... ]

> +static inline u64 gpu_read64_counter(void __iomem *iomem, u32 reg)
> +{
> +	u32 lo, hi1, hi2;
> +	do {
> +		hi1 = gpu_read(iomem, reg + 4);
> +		lo = gpu_read(iomem, reg);
> +		hi2 = gpu_read(iomem, reg + 4);
> +	} while (hi1 != hi2);
> +	return lo | ((u64)hi2 << 32);
> +}
> +
> +#define gpu_read_poll_timeout(iomem, reg, val, cond, delay_us, timeout_us)	\
> +	read_poll_timeout(gpu_read, val, cond, delay_us, timeout_us, false,	\
> +			  iomem, reg)

[Severity: Low]
Should <linux/iopoll.h> be included here for the read_poll_timeout() macros?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922204535.2850094-1-karunika.choo@arm.com?part=2