Hi Joel.
Replying to Noralf's mail as I lost the original mail.
> > diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > new file mode 100644
> > index 000000000000..e2d1d7497352
> > --- /dev/null
> > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > @@ -0,0 +1,248 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +// Copyright 2018 IBM Corporation
> > +
> > +#include <linux/clk.h>
> > +#include <linux/reset.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_fb_cma_helper.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +#include <drm/drm_simple_kms_helper.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_panel.h>
>
> I prefer includes sorted alphabetically, but no requirement.
Please sort them as Noralf suggest, as this makes it much more obvious
when one is adding a duplicate header.
> > +
> > + priv->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(priv->rst)) {
> > + dev_err(&pdev->dev,
> > + "missing or invalid reset controller device tree entry");
Add error code to your dev_err() calls, so one later better
can tell what was wrong if river fails to load.
Same goes for other dev_xxx calls in this function / dirver.
(Most dev_xxx have the return code, only a few seems to miss it)
> > +static const struct of_device_id aspeed_gfx_match[] = {
> > + { .compatible = "aspeed,ast2400-gfx" },
> > + { .compatible = "aspeed,ast2500-gfx" },
> > + { }
Many drivers put a /* sentinel */ comment inside the empty {}
With the few things suggested above considered this is
Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
Sam
