[PATCH] video: drm: exynos: Adds display-timing node parsing using video helper function

[Leela Krishna Amudala]

On Mon, Jan 28, 2013 at 9:24 PM, Sean Paul <seanpaul at chromium.org> wrote:
>
> On Mon, Jan 28, 2013 at 12:45 AM, Vikas Sajjan <vikas.sajjan at linaro.org>
> wrote:
> > This patch adds display-timing node parsing using video helper function
> >
> > Signed-off-by: Leela Krishna Amudala <l.krishna at samsung.com>
> > Signed-off-by: Vikas Sajjan <vikas.sajjan at linaro.org>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   35
> > ++++++++++++++++++++++++++++--
> >  1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > index bf0d9ba..975e7f7 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/of_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pinctrl/consumer.h>
> >
> >  #include <video/samsung_fimd.h>
> >  #include <drm/exynos_drm.h>
> > @@ -903,21 +904,51 @@ static int __devinit fimd_probe(struct
> > platform_device *pdev)
> >         struct device *dev = &pdev->dev;
> >         struct fimd_context *ctx;
> >         struct exynos_drm_subdrv *subdrv;
> > -       struct exynos_drm_fimd_pdata *pdata;
> > +       struct exynos_drm_fimd_pdata *pdata = pdev->dev.platform_data;
> >         struct exynos_drm_panel_info *panel;
> > +       struct fb_videomode *fbmode;
> > +       struct device *disp_dev = &pdev->dev;
> > +       struct pinctrl *pctrl;
> >         struct resource *res;
> >         int win;
> >         int ret = -EINVAL;
> >
> >         DRM_DEBUG_KMS("%s\n", __FILE__);
> >
> > -       pdata = pdev->dev.platform_data;
> > +       if (pdev->dev.of_node) {
> > +               pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +               if (!pdata) {
> > +                       dev_err(dev, "memory allocation for pdata
> > failed\n");
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               fbmode = devm_kzalloc(dev, sizeof(*fbmode), GFP_KERNEL);
> > +               if (!fbmode) {
> > +                       dev_err(dev, "memory allocation for fbmode
> > failed\n");
>
> Why dev_err instead of DRM_ERROR?
>

Will change it to DRM_ERROR

> > +                       return -ENOMEM;
> > +               }
> > +
> > +               ret = of_get_fb_videomode(disp_dev->of_node, fbmode,
> > -1);
> > +               if (ret) {
> > +                       dev_err(dev, "failed to get fb_videomode\n");
> > +                       return -EINVAL;
> > +               }
> > +               pdata->panel.timing = (struct fb_videomode) *fbmode;
> > +       }
> > +
> >         if (!pdata) {
>
> This condition is kind of weird, in that it's really checking if
> (!pdev->dev.of_node) implicitly (since you already check the
> allocation of pdata above).
>
Even I thought the same. But kept this check because If DT node is
not available and driver still gets plat data from machine file then the
if (pdev->dev.of_node){} block will be skipped and plat data should be checked.

> Seems like you could make this more clear and save a level of
> indentation by doing the following above:
>
> if (!pdev->dev.of_node) {
>         DRM_ERROR("Device tree node was not found\n");
>         return -EINVAL;
> }
>
If I return -EINVAL here then this driver will become Full DT based one.
and probe will be failed if DT node is not present.

Will correct the if check and post the next version.

/Leela Krishna Amudala
> Then just get rid of this check and the one wrapping the allocations
> above.
>
> Sean
>
> >                 dev_err(dev, "no platform data specified\n");
> >                 return -EINVAL;
> >         }
> >
> > +       pctrl = devm_pinctrl_get_select_default(dev);
> > +       if (IS_ERR(pctrl)) {
> > +               dev_err(dev, "no pinctrl data provided.\n");
> > +               return -EINVAL;
> > +       }
> > +
> >         panel = &pdata->panel;
> > +
> >         if (!panel) {
> >                 dev_err(dev, "panel is null.\n");
> >                 return -EINVAL;
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel