linux-kernel - Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

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: <20160614112527.GA30021@ulmo.ba.sec>
Date:	Tue, 14 Jun 2016 13:25:27 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Vinay Simha <simhavcs@...il.com>
Cc:	Archit Taneja <archit.taneja@...il.com>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	John Stultz <john.stultz@...aro.org>,
	Rob Clark <robdclark@...il.com>,
	David Airlie <airlied@...ux.ie>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:DRM PANEL DRIVERS" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote:
[...]
> On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding
> <thierry.reding@...il.com> wrote:
> > On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
[...]
> >> +const char *regs[] = {
> >> +     "vddp",
> >> +     "dcdc_en",
> >> +     "vcc"
> >> +};
> >
> > This should be static. Also use a more sensible name, such as
> > regulator_names, please.
> it kept as regs, to keep constant names as used in
> the dsi_cfg file
> drivers/gpu/drm/msm/dsi/dsi_cfg.c

That's a completely different driver, no need to be consistent.

> >> +static int jdi_panel_init(struct jdi_panel *jdi)
> >> +{
> > [...]
> >> +     struct mipi_dsi_device *dsi = jdi->dsi;
> >> +     int ret;
> >> +
> >> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> >> +
> >> +     ret = mipi_dsi_dcs_soft_reset(dsi);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     usleep_range(10000, 20000);
> >> +
> >> +     ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);
> >> +     if (ret < 0)
> >> +             return ret;
> >
> > Please use the existing symbolic constants for this.
> i am not clear on the symbolic constants for pixel_format ?

See include/video/mipi_display.h

> >> +
> >> +     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> >> +                              (u8[]){ 0x24 }, 1);
> >> +     if (ret < 0)
> >> +             return ret;
> brightness control setting, to enable pwm/backlight
> >> +
> >> +     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
> >> +                              (u8[]){ 0x00 }, 1);
> >> +     if (ret < 0)
> >> +             return ret;
> >
> this is to set cabc off/on

Can you point me to the specification of these? I'd like to investigate
whether or not we can turn these into more sensible commands. As-is, it
is completely obfuscated and I'd like to avoid that where possible.

> >> +     ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +     mdelay(10);
> >
> > Same here. This also needs at least a comment, though ideally you'd use
> > symbolic names for those magic numbers.
> i do not have the datasheet to give more description.
> this is for interface setting, either command mode/video mode

Okay, please add a comment on what this is supposed to do, then.

> >> +
> >> +     jdi->mode = &default_mode;
> >> +
> >> +     for (i = 0; i < num; i++)
> >> +             s[i].supply = regs[i];
> >> +
> >> +     ret = devm_regulator_bulk_get(dev, num, s);
> >> +     if (ret < 0) {
> >> +             dev_err(dev, "%s: failed to init regulator, ret=%d\n",
> >> +                     __func__, ret);
> >> +             return ret;
> >> +     }
> >> +
> >> +     jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >> +     if (IS_ERR(jdi->reset_gpio)) {
> >> +             dev_err(dev, "cannot get reset-gpios %ld\n",
> >> +                     PTR_ERR(jdi->reset_gpio));
> >
> > This is a third variant of error reporting. Please stick to one.
> for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting
> cannot be changed to ret,
> others error reporting incorporated consistently.

PTR_ERR() returns signed long, not unsigned.

You can still use the same format for the message and substitute the
%ld printk specifier to match the type.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)