Re: [PATCH v6] drm/panel: Add a driver for the TPO TPG110

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3669 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel