Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Uwe,

just a few nitpicks, see below.

On Fri, Feb 10, 2023 at 07:00:14PM +0100, Uwe Kleine-König wrote:
> From: Marian Cichy <m.cichy@pengutronix.de>
> 
> Add support for the LCD Controller found on i.MX21 and i.MX25.
> 
> It targets to be a drop in replacement for the imx-fb driver.
> 
> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
> [ukl: Rebase to a newer kernel version, various smaller fixes and
> improvements]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/Kconfig         |   1 +
>  drivers/gpu/drm/imx/Makefile        |   1 +
>  drivers/gpu/drm/imx/lcdc/Kconfig    |   7 +
>  drivers/gpu/drm/imx/lcdc/Makefile   |   1 +
>  drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 553 ++++++++++++++++++++++++++++
>  5 files changed, 563 insertions(+)
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
>  create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> 
[...]
> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> new file mode 100644
> index 000000000000..c2197fc50306
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
> +
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_fourcc.h>

Choose one, remove the other.

[...]
> +struct imx_lcdc {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	const struct drm_display_mode *mode;

The mode pointer appears to be unused.

> +	struct drm_bridge *bridge;

The bridge could be a local variable in _probe().

> +	struct drm_connector *connector;
> +	void __iomem *base;
> +
> +	struct clk *clk_ipg;
> +	struct clk *clk_ahb;
> +	struct clk *clk_per;
> +};
> +
> +static const u32 imx_lcdc_formats[] = {
> +	DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888,
> +};
> +
> +static inline struct imx_lcdc *imx_lcdc_from_drmdev(struct drm_device *drm)
> +{
> +	return container_of(drm, struct imx_lcdc, drm);
> +}
> +
> +static unsigned int imx_lcdc_get_format(unsigned int drm_format)
> +{
> +	unsigned int bpp;
> +
> +	switch (drm_format) {
> +	default:
> +		DRM_WARN("Format not supported - fallback to XRGB8888\n");
> +		fallthrough;
> +
> +	case DRM_FORMAT_XRGB8888:
> +		bpp = BPP_XRGB8888;
> +		break;

This could just return directly, no need for the local bpp variable.

> +
> +	case DRM_FORMAT_RGB565:
> +		bpp = BPP_RGB565;
> +		break;
> +	}
> +
> +	return bpp;
> +}
> +
[...]
> +
> +static int imx_lcdc_probe(struct platform_device *pdev)
> +{
> +	struct imx_lcdc *lcdc;
> +	struct drm_device *drm;
> +	int irq;
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	lcdc = devm_drm_dev_alloc(dev, &imx_lcdc_drm_driver,
> +				  struct imx_lcdc, drm);
> +	if (!lcdc)
> +		return -ENOMEM;
> +
> +	drm = &lcdc->drm;
> +
> +	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lcdc->base))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->base), "Cannot get IO memory\n");
> +
> +	lcdc->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> +	if (IS_ERR(lcdc->bridge))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->bridge), "Failed to find bridge\n");
[...]
> +	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");

The bridge could be a local variable.

With those addressed,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel