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@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> [ukl: Rebase to a newer kernel version, various smaller fixes and
> improvements]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx>
> +
> +#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@xxxxxxxxxxxxxx>
regards
Philipp
