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

[Sean Paul]

On Mon, Jan 28, 2013 at 12:02 PM, Leela Krishna Amudala
<l.krishna at samsung.com> wrote:
> 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.
>

In that case, you should probably add that assignment of pdata =
pdev->dev.platform_data back ;)

Sean

>> 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