On 14/10/2025 10:43, Karunika Choo wrote:
> Introduce architecture-specific function pointers to support
> architecture-dependent behaviours. This patch adds the following
> function pointers and updates their usage accordingly:
>
> - soft_reset
> - l2_power_on
> - l2_power_off
>
> Signed-off-by: Karunika Choo <[email protected]>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 4 ++--
> drivers/gpu/drm/panthor/panthor_fw.c | 5 +++--
> drivers/gpu/drm/panthor/panthor_gpu.c | 13 ++++++++++---
> drivers/gpu/drm/panthor/panthor_gpu.h | 1 +
> drivers/gpu/drm/panthor/panthor_hw.c | 9 ++++++++-
> drivers/gpu/drm/panthor/panthor_hw.h | 23 +++++++++++++++++++++++
> 6 files changed, 47 insertions(+), 8 deletions(-)
>
<snip>
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h
> b/drivers/gpu/drm/panthor/panthor_hw.h
> index 7a191e76aeec..5a4e4aad9099 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
> };
>
>
> +/**
> + * struct panthor_hw_ops - HW operations that are specific to a GPU
> + */
> +struct panthor_hw_ops {
> + /** @soft_reset: Soft reset function pointer */
> + int (*soft_reset)(struct panthor_device *ptdev);
> +#define panthor_hw_soft_reset(__ptdev) \
> + ((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev)
> : 0)
> +
> + /** @l2_power_off: L2 power off function pointer */
> + int (*l2_power_off)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_off(__ptdev) \
> + ((__ptdev)->hw->ops.l2_power_off ?
> (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
> +
> + /** @l2_power_on: L2 power on function pointer */
> + int (*l2_power_on)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_on(__ptdev) \
> + ((__ptdev)->hw->ops.l2_power_on ?
> (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
> +};
Minor comments:
* You are defining these to have a return value, but you haven't
updated any of the call-sites to deal with a failure (the return value
is ignored). This is actually an existing problem, but AFAICT the new
_pwr_ versions have more error codes which are simply getting thrown away.
* Is there a good reason why we need to support these functions being
NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
a dummy (empty) function in those cases.
* A static inline function would be neater and would avoid any
potential issues from the multiple evaluation of __ptdev.
Thanks,
Steve
> +
> /**
> * struct panthor_hw - GPU specific register mapping and functions
> */
> struct panthor_hw {
> /** @features: Bitmap containing panthor_hw_feature */
> DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
> +
> + /** @ops: Panthor HW specific operations */
> + struct panthor_hw_ops ops;
> };
>
> int panthor_hw_init(struct panthor_device *ptdev);
