From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v6] drm/panel: Add a driver for the TPO TPG110 Date: Thu, 10 Jan 2019 09:11:04 +0100 Message-ID: <20190110081104.GB5213@ulmo> References: <20190109135331.14895-1-linus.walleij@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0814702659==" Return-path: Received: from mail-wm1-x343.google.com (mail-wm1-x343.google.com [IPv6:2a00:1450:4864:20::343]) by gabe.freedesktop.org (Postfix) with ESMTPS id B3C576E65D for ; Thu, 10 Jan 2019 08:11:07 +0000 (UTC) Received: by mail-wm1-x343.google.com with SMTP id b11so10275100wmj.1 for ; Thu, 10 Jan 2019 00:11:07 -0800 (PST) In-Reply-To: <20190109135331.14895-1-linus.walleij@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Linus Walleij Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0814702659== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="E39vaYmALEf/7YXx" Content-Disposition: inline --E39vaYmALEf/7YXx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jan 09, 2019 at 02:53:31PM +0100, Linus Walleij wrote: [...] > diff --git a/drivers/gpu/drm/panel/panel-tpo-tpg110.c b/drivers/gpu/drm/panel/panel-tpo-tpg110.c [...] > +static int tpg110_startup(struct tpg110 *tpg) > +{ > + u8 val; > + int i; > + > + /* De-assert the reset signal */ > + gpiod_set_value_cansleep(tpg->grestb, 0); > + mdelay(1); Does this have to be the spinning variant? This seems to be only used in the probe path, so doesn't have to be atomic. > + dev_info(tpg->dev, "de-asserted GRESTB\n"); Maybe turn this into dev_dbg()? > + > + /* Test display communication */ > + tpg110_write_reg(tpg, TPG110_TEST, 0x55); > + val = tpg110_read_reg(tpg, TPG110_TEST); > + if (val != 0x55) { > + dev_err(tpg->dev, "failed communication test\n"); > + return -ENODEV; > + } > + > + val = tpg110_read_reg(tpg, TPG110_CHIPID); > + dev_info(tpg->dev, "TPG110 chip ID: %d version: %d\n", > + val >> 4, val & 0x0f); > + > + /* Show display resolution */ > + val = tpg110_read_reg(tpg, TPG110_CTRL1); > + val &= TPG110_RES_MASK; > + switch (val) { > + case TPG110_RES_400X240_D: > + dev_info(tpg->dev, > + "IN 400x240 RGB -> OUT 800x480 RGB (dual scan)"); > + break; > + case TPG110_RES_480X272_D: > + dev_info(tpg->dev, > + "IN 480x272 RGB -> OUT 800x480 RGB (dual scan)"); > + break; > + case TPG110_RES_480X640: > + dev_info(tpg->dev, "480x640 RGB"); > + break; > + case TPG110_RES_480X272: > + dev_info(tpg->dev, "480x272 RGB"); > + break; > + case TPG110_RES_640X480: > + dev_info(tpg->dev, "640x480 RGB"); > + break; > + case TPG110_RES_800X480: > + dev_info(tpg->dev, "800x480 RGB"); > + break; > + default: > + dev_info(tpg->dev, "ILLEGAL RESOLUTION"); dev_err()? Also, perhaps make this some proper error message and include the invalid value? Also I just noticed that the above informational messages all lack a \n at the end. The above is also very verbose, do we really need all that information in the kernel log? > + break; > + } > + > + /* From the producer side, this is the same resolution */ > + if (val == TPG110_RES_480X272_D) > + val = TPG110_RES_480X272; > + > + for (i = 0; i < ARRAY_SIZE(tpg110_modes); i++) { > + const struct tpg110_panel_mode *pm; > + > + pm = &tpg110_modes[i]; > + if (pm->magic == val) { > + tpg->panel_mode = pm; > + break; > + } > + } > + if (i == ARRAY_SIZE(tpg110_modes)) { > + dev_err(tpg->dev, "unsupported mode (%02x) detected\n", > + val); > + return -ENODEV; > + } > + > + val = tpg110_read_reg(tpg, TPG110_CTRL2); > + dev_info(tpg->dev, "resolution and standby is controlled by %s\n", > + (val & TPG110_CTRL2_RES_PM_CTRL) ? "software" : "hardware"); > + /* Take control over resolution and standby */ > + val |= TPG110_CTRL2_RES_PM_CTRL; > + tpg110_write_reg(tpg, TPG110_CTRL2, val); > + > + return 0; > +} > + > +static int tpg110_disable(struct drm_panel *panel) > +{ > + struct tpg110 *tpg = to_tpg110(panel); > + u8 val; > + > + /* Put chip into standby */ > + val = tpg110_read_reg(tpg, TPG110_CTRL2_PM); > + val &= ~TPG110_CTRL2_PM; > + tpg110_write_reg(tpg, TPG110_CTRL2_PM, val); > + > + if (tpg->backlight) { > + tpg->backlight->props.power = FB_BLANK_POWERDOWN; > + tpg->backlight->props.state |= BL_CORE_FBBLANK; > + backlight_update_status(tpg->backlight); > + } backlight_disable()? > + > + return 0; > +} > + > +static int tpg110_enable(struct drm_panel *panel) > +{ > + struct tpg110 *tpg = to_tpg110(panel); > + u8 val; > + > + if (tpg->backlight) { > + tpg->backlight->props.state &= ~BL_CORE_FBBLANK; > + tpg->backlight->props.power = FB_BLANK_UNBLANK; > + backlight_update_status(tpg->backlight); > + } backlight_enable()? Thierry --E39vaYmALEf/7YXx Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlw2/hQACgkQ3SOs138+ s6E81w/+IxT9M8OcdOm1z9cR0VohyC5h6Hf9JKsjP6tWUAMW5FnYCRpi7/y5mbOn eDnQsG/9Kp/YU8mpX1Im1i2CDROiehprsBThLqccpL84uGGCnyx55CNpxtW+YS1B CUMasNLlgn0jqsFDAJFe20Uj3BnnGsEejm33LNHT99z0+y1qpn7tJvCYGniytXjq /IBCkL9NrYMcVVh9zuqgS4yunm9MSqveERPlKaLf/x7P64lFZApG+bto9ExlTdMR V54EJRqRjzBD8DOziQLmulEh6daOEpYHEd1bT9S+PZ7s0fj899qdxWvA/jZCPHbI ucu3wjIyk2MN2LOQdY8TU4zIps3Dupse1u0+Iq91KJ9OLLpzYtcj5FluVkMi//KD 6RQOXzjBt4JiAt5YjA+eHyPKG9XWwvyZs6nvYnkdcdbUGP7kEhsbSOaw/LCtJLJn wSu3N8emTlcQUvTFGkqLA7HKD/YjMFcoLafq1nynvcmkri1m2k8I51Hqhu/N6EHB 1uycMy6ufwshZxckTJB9rRV71KZzKSjPw1BGsgqxP/HPqXefFY1RVMJR5UAZ3Qqe P1xzL0j63bL1D6MOAkfS8kw2N76Hw6z7/orfgADBPJKnSVVzaZH+nH6BcFV7Jzln IZGChdc9ORqEQKs+TB/9m6Vql20PKf6BRznMZI1EZfj+3mFX6pU= =AEBO -----END PGP SIGNATURE----- --E39vaYmALEf/7YXx-- --===============0814702659== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0814702659==--