linux-kernel - Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOMZO5DS2v15h9E=qKg2vKuFkBSQQwdBHA5Th5mZ+ca6DWgQsw@mail.gmail.com>
Date:   Fri, 21 Jun 2019 10:59:19 -0300
From:   Fabio Estevam <festevam@...il.com>
To:     Robert Chiras <robert.chiras@....com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        DRI mailing list <dri-devel@...ts.freedesktop.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        NXP Linux Team <linux-imx@....com>
Subject: Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver

Hi Robert,

On Thu, Jun 20, 2019 at 10:31 AM Robert Chiras <robert.chiras@....com> wrote:

> +fail:
> +       if (rad->reset)
> +               gpiod_set_value_cansleep(rad->reset, 1);

gpiod_set_value_cansleep() can handle NULL, so no need for the if() check.

> +static const struct display_timing rad_default_timing = {
> +       .pixelclock = { 132000000, 132000000, 132000000 },

Having the same information listed three times does not seem useful.

You could use a drm_display_mode structure with a single entry instead.

> +       videomode_from_timing(&rad_default_timing, &panel->vm);
> +
> +       of_property_read_u32(np, "width-mm", &panel->width_mm);
> +       of_property_read_u32(np, "height-mm", &panel->height_mm);
> +
> +       panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

Since this is optional it would be better to use
devm_gpiod_get_optional() instead.


> +
> +       if (IS_ERR(panel->reset))
> +               panel->reset = NULL;
> +       else
> +               gpiod_set_value_cansleep(panel->reset, 1);

This is not handling defer probing, so it would be better to do like this:

panel->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(panel->reset))
      return  PTR_ERR(panel->reset);

> +       memset(&bl_props, 0, sizeof(bl_props));
> +       bl_props.type = BACKLIGHT_RAW;
> +       bl_props.brightness = 255;
> +       bl_props.max_brightness = 255;
> +
> +       panel->backlight = devm_backlight_device_register(dev, dev_name(dev),
> +                                                         dev, dsi,
> +                                                         &rad_bl_ops,
> +                                                         &bl_props);

Could you put more parameters into the same line?

Using 4 lines seems excessive.

> +       if (IS_ERR(panel->backlight)) {
> +               ret = PTR_ERR(panel->backlight);
> +               dev_err(dev, "Failed to register backlight (%d)\n", ret);
> +               return ret;
> +       }
> +
> +       drm_panel_init(&panel->panel);
> +       panel->panel.funcs = &rad_panel_funcs;
> +       panel->panel.dev = dev;
> +       dev_set_drvdata(dev, panel);
> +
> +       ret = drm_panel_add(&panel->panel);
> +

Unneeded blank line

> +       if (ret < 0)
> +               return ret;
> +
> +       ret = mipi_dsi_attach(dsi);
> +       if (ret < 0)
> +               drm_panel_remove(&panel->panel);
> +
> +       return ret;

You did not handle the "power" regulator.

> +static int __maybe_unused rad_panel_suspend(struct device *dev)
> +{
> +       struct rad_panel *rad = dev_get_drvdata(dev);
> +
> +       if (!rad->reset)
> +               return 0;
> +
> +       devm_gpiod_put(dev, rad->reset);
> +       rad->reset = NULL;
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused rad_panel_resume(struct device *dev)
> +{
> +       struct rad_panel *rad = dev_get_drvdata(dev);
> +
> +       if (rad->reset)
> +               return 0;
> +
> +       rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

Why do you call devm_gpiod_get() once again?

I am having a hard time to understand the need for this suspend/resume hooks.

Can't you simply remove them?