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

[Liu Ying <victor.liu@nxp.com>]

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@nxp.com> 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@nxp.com>
> > ---
> > 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