Hi,

[I just noticed I dropped the Cc list, resending]

Thanks for contributing this driver, it's awesome to see it moving
forward.

And congrats on the documentation too, it's not often we see a driver
that well documented on its v3.

I've stripped some parts of the patch that weren't relevant to my
review.

On Tue, Jun 13, 2023 at 03:47:47PM +0100, Sarah Walker wrote:
> +static __always_inline struct pvr_device *
> +to_pvr_device(struct drm_device *drm_dev)
> +{
> +     return container_of(drm_dev, struct pvr_device, base);
> +}

For that kind of helpers, we're slowly transitioning to using a macro
and container_of_const. This allows to work with const-ness context.

> +static int
> +pvr_probe(struct platform_device *plat_dev)
> +{
> +     struct pvr_device *pvr_dev;
> +     struct drm_device *drm_dev;
> +     int err;
> +
> +     pvr_dev = devm_drm_dev_alloc(&plat_dev->dev, &pvr_drm_driver,
> +                                  struct pvr_device, base);
> +     if (IS_ERR(pvr_dev)) {
> +             err = IS_ERR(pvr_dev);

PTR_ERR?

> +             goto err_out;

The general pattern here is to return directly here, it's simpler to
follow.

> +     }
> +     drm_dev = &pvr_dev->base;
> +
> +     platform_set_drvdata(plat_dev, drm_dev);
> +
> +     err = drm_dev_register(drm_dev, 0);
> +     if (err)
> +             goto err_out;

I guess it would be simpler here too, but I think you're going to move
things around anyway?

> +static const struct of_device_id dt_match[] = {
> +     { .compatible = "ti,am62-gpu", .data = NULL },
> +     { .compatible = "img,powervr-seriesaxe", .data = NULL },

Do you really need both? The binding you documented requires both to be
there, so I think you can either match on the more specific one (and
extend that list when needed) or match the more generic one and be done
with it once and for all. Having both is redundant.

> +     {}
> +};
> +MODULE_DEVICE_TABLE(of, dt_match);
> +
> +static struct platform_driver pvr_driver = {
> +     .probe = pvr_probe,
> +     .remove = pvr_remove,
> +     .driver = {
> +             .name = PVR_DRIVER_NAME,
> +             .of_match_table = dt_match,
> +     },
> +};
> +module_platform_driver(pvr_driver);
> +
> +MODULE_AUTHOR("Imagination Technologies Ltd.");
> +MODULE_DESCRIPTION(PVR_DRIVER_DESC);
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_IMPORT_NS(DMA_BUF);
> +MODULE_FIRMWARE("powervr/rogue_4.40.2.51_v1.fw");
> +MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");

That one should probably be moved to the firmware patch?

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
> new file mode 100644
> index 000000000000..8e6f4a4dde3f
> --- /dev/null
> +++ b/drivers/gpu/drm/imagination/pvr_drv.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> +
> +#ifndef PVR_DRV_H
> +#define PVR_DRV_H
> +
> +#include "linux/compiler_attributes.h"
> +#include <uapi/drm/pvr_drm.h>
> +
> +#define PVR_DRIVER_NAME "powervr"
> +#define PVR_DRIVER_DESC "Imagination PowerVR Graphics"

Do you intend to support the SGX and Rogue GPUs with this driver? If
not, mentioning the generation/architecture name somewhere would be
nice.

Maxime