Re: [PATCH 2/2] drm: Add ASPEED GFX driver

[Sam Ravnborg <sam@ravnborg.org>]

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@ravnborg.org>

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel