Re: [PATCH v6 12/14] drm/bridge: imx: Add LDB support for i.MX8qxp

From: Liu Ying
Date: Wed Mar 31 2021 - 02:28:36 EST


On Tue, 2021-03-30 at 11:54 +0200, Robert Foss wrote:
> Hey Liu,
>
> checkpatch --strict lists some nit and a warning. With those fixed
> feel free to add my r-b.
>
> On Wed, 17 Mar 2021 at 04:57, Liu Ying <victor.liu@xxxxxxx> wrote:
> > This patch adds a drm bridge driver for i.MX8qxp LVDS display bridge(LDB)
> > which is officially named as pixel mapper. The LDB has two channels.
> > Each of them supports up to 24bpp parallel input color format and can map
> > the input to VESA or JEIDA standards. The two channels cannot be used
> > simultaneously, that is to say, the user should pick one of them to use.
> > Two LDB channels from two LDB instances can work together in LDB split
> > mode to support a dual link LVDS display. The channel indexes have to be
> > different. Channel0 outputs odd pixels and channel1 outputs even pixels.
> > This patch supports the LDB single mode and split mode.
> >
> > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > ---
> > Note that this patch depends on the patch 'phy: Add LVDS configuration options',
> > which has already been sent with the following series to add Mixel combo PHY
> > found in i.MX8qxp:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg879957.html&amp;data=04%7C01%7Cvictor.liu%40nxp.com%7C7c4f1092633c4687fb3108d8f361ddfd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637526948949839014%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=iNr6RU0xrhcW8epwIPPJDpayjWxfmWsSjCju3XIcmPM%3D&amp;reserved=0
> >
> > v5->v6:
> > * No change.
> >
> > v4->v5:
> > * Link with the imx-ldb-helper object. (Robert)
> > * Correspondingly, rename 'imx8qxp-ldb.c' to 'imx8qxp-ldb-drv.c'.
> >
> > v3->v4:
> > * No change.
> >
> > v2->v3:
> > * No change.
> >
> > v1->v2:
> > * Drop unnecessary DT validation.
> > * Use of_graph_get_endpoint_by_regs() and of_graph_get_remote_endpoint() to
> > get the input remote endpoint in imx8qxp_ldb_set_di_id().
> > * Avoid using companion_port OF node after putting it in
> > imx8qxp_ldb_parse_dt_companion().
> > * Mention i.MX8qxp LDB official name 'pixel mapper' in the bridge driver
> > and Kconfig help message.
> >
> > drivers/gpu/drm/bridge/imx/Kconfig | 9 +
> > drivers/gpu/drm/bridge/imx/Makefile | 3 +
> > drivers/gpu/drm/bridge/imx/imx8qxp-ldb-drv.c | 720 +++++++++++++++++++++++++++
> > 3 files changed, 732 insertions(+)
> > create mode 100644 drivers/gpu/drm/bridge/imx/imx8qxp-ldb-drv.c
> >

[...]

> > +
> > +static int imx8qxp_ldb_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct imx8qxp_ldb *imx8qxp_ldb;
> > + struct imx8qxp_ldb_channel *imx8qxp_ldb_ch;
> > + struct ldb *ldb;
> > + struct ldb_channel *ldb_ch;
> > + int ret, i;
> > +
> > + imx8qxp_ldb = devm_kzalloc(dev, sizeof(*imx8qxp_ldb), GFP_KERNEL);
> > + if (!imx8qxp_ldb)
> > + return -ENOMEM;
> > +
> > + imx8qxp_ldb->clk_pixel = devm_clk_get(dev, "pixel");
> > + if (IS_ERR(imx8qxp_ldb->clk_pixel)) {
> > + ret = PTR_ERR(imx8qxp_ldb->clk_pixel);
> > + if (ret != -EPROBE_DEFER)
> > + DRM_DEV_ERROR(dev,
> > + "failed to get pixel clock: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + imx8qxp_ldb->clk_bypass = devm_clk_get(dev, "bypass");
> > + if (IS_ERR(imx8qxp_ldb->clk_bypass)) {
> > + ret = PTR_ERR(imx8qxp_ldb->clk_bypass);
> > + if (ret != -EPROBE_DEFER)
> > + DRM_DEV_ERROR(dev,
> > + "failed to get bypass clock: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + imx8qxp_ldb->dev = dev;
> > +
> > + ldb = &imx8qxp_ldb->base;
> > + ldb->dev = dev;
> > + ldb->ctrl_reg = 0xe0;
> > +
> > + for (i = 0; i < MAX_LDB_CHAN_NUM; i++)
> > + ldb->channel[i] = &imx8qxp_ldb->channel[i].base;
> > +
> > + ret = ldb_init_helper(ldb);
> > + if (ret)
> > + return ret;
> > +
> > + if (ldb->available_ch_cnt == 0) {
> > + DRM_DEV_DEBUG_DRIVER(dev, "no available channel\n");
> > + return 0;
> > + } else if (ldb->available_ch_cnt > 1) {
> > + DRM_DEV_ERROR(dev, "invalid available channel number(%u)\n",
> > + ldb->available_ch_cnt);
> > + return -ENOTSUPP;
> > + }
>
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> #683: FILE: drivers/gpu/drm/bridge/imx/imx8qxp-ldb-drv.c:625:
> + return -ENOTSUPP;
>
> Maybe -EINVAL is a better return value.

Will use -EINVAL in the next version.

Liu Ying