[PATCH] drm/panel: ws2401: Add driver for WideChips WS2401

[Noralf Trønnes]

Den 25.06.2021 00.44, skrev Linus Walleij:
> This adds a driver for panels based on the WideChips WS2401 display
> controller. This display controller is used in the Samsung LMS380KF01
> display found in the Samsung GT-I8160 (Codina) mobile phone and
> possibly others.
> 
> As is common with Samsung displays manufacturer commands are necessary
> to configure the display to a working state.
> 
> The display optionally supports internal backlight control, but can
> also use an external backlight.
> 
> This driver re-uses the DBI infrastructure to communicate with the
> display.
> 
> Cc: phone-devel at vger.kernel.org
> Cc: Douglas Anderson <dianders at chromium.org>
> Cc: Noralf Trønnes <noralf at tronnes.org>
> Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
> ---

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 4894913936e9..f4fe1dba9912 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -552,6 +552,15 @@ config DRM_PANEL_VISIONOX_RM69299
>  	  Say Y here if you want to enable support for Visionox
>  	  RM69299  DSI Video Mode panel.
>  
> +config DRM_PANEL_WIDECHIPS_WS2401
> +	tristate "Widechips WS2401 DPI panel driver"
> +	depends on OF && SPI && GPIOLIB

I couldn't find any OF dependency in the driver?

> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_MIPI_DBI
> +	help
> +	  Say Y here if you want to enable support for the Widechips
> +	  WS2401 DPI 480x800 display controller.
> +
>  config DRM_PANEL_XINPENG_XPP055C272
>  	tristate "Xinpeng XPP055C272 panel driver"
>  	depends on OF

> diff --git a/drivers/gpu/drm/panel/panel-widechips-ws2401.c b/drivers/gpu/drm/panel/panel-widechips-ws2401.c
> new file mode 100644
> index 000000000000..d15870301174
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-widechips-ws2401.c
> @@ -0,0 +1,404 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panel driver for the WideChips WS2401 480x800 DPI RGB panel, used in
> + * the Samsung Mobile Display (SMD) LMS380KF01.
> + * Found in the Samsung Galaxy Ace 2 GT-I8160 mobile phone.
> + * Linus Walleij <linus.walleij at linaro.org>
> + * Inspired by code and know-how in the vendor driver by Gareth Phillips.
> + */
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

AFAICS there are no users of this header.

> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +

> +static const u8 ws2401_dbi_read_commands[] = {
> +	WS2401_READ_ID1,
> +	WS2401_READ_ID2,
> +	WS2401_READ_ID3,
> +	0, /* sentinel */
> +};
> +

> +static void ws2401_read_mtp_id(struct ws2401 *ws)
> +{
> +	struct mipi_dbi *dbi = &ws->dbi;
> +	u8 id1, id2, id3;
> +	int ret;
> +
> +	ret = mipi_dbi_command_read(dbi, WS2401_READ_ID1, &id1);
> +	if (ret) {
> +		dev_err(ws->dev, "unable to read MTP ID 1\n");
> +		return;
> +	}
> +	ret = mipi_dbi_command_read(dbi, WS2401_READ_ID2, &id1);
> +	if (ret) {
> +		dev_err(ws->dev, "unable to read MTP ID 2\n");
> +		return;
> +	}
> +	ret = mipi_dbi_command_read(dbi, WS2401_READ_ID3, &id1);
> +	if (ret) {
> +		dev_err(ws->dev, "unable to read MTP ID 3\n");
> +		return;
> +	}
> +	dev_info(ws->dev, "MTP ID: %02x %02x %02x\n", id1, id2, id3);
> +}

Why do you read these id's on every power on, it doesn't look like you
use them?

If they're just informational, they should be available through debugfs,
see mipi_dbi_debugfs_init().

> +static int ws2401_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ws2401 *ws;
> +	int ret;
> +
> +	ws = devm_kzalloc(dev, sizeof(*ws), GFP_KERNEL);
> +	if (!ws)
> +		return -ENOMEM;
> +	ws->dev = dev;
> +
> +	/*
> +	 * VCI   is the analog voltage supply
> +	 * VCCIO is the digital I/O voltage supply
> +	 */
> +	ws->regulators[0].supply = "vci";
> +	ws->regulators[1].supply = "vccio";
> +	ret = devm_regulator_bulk_get(dev,
> +				      ARRAY_SIZE(ws->regulators),
> +				      ws->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
> +	ws->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ws->reset)) {
> +		ret = PTR_ERR(ws->reset);
> +		return dev_err_probe(dev, ret, "no RESET GPIO\n");
> +	}
> +
> +	ret = mipi_dbi_spi_init(spi, &ws->dbi, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "MIPI DBI init failed\n");
> +	ws->dbi.read_commands = ws2401_dbi_read_commands;
> +
> +	drm_panel_init(&ws->panel, dev, &ws2401_drm_funcs,
> +		       DRM_MODE_CONNECTOR_DPI);
> +
> +	ret = drm_panel_of_backlight(&ws->panel);
> +	if (ret) {

I fail to understand how to use internal backlight. If there's no
backlight DT property, ret will be zero, right?

of_find_backlight() can return -EPROBE_DEFER which will also end up here...

> +		dev_info(dev, "no external backlight, using internal backlight\n");

Like Doug I'm not a fan of these backlight info messages, things like
these clutter up the boot log. It should be possible to glean this info
from the backlight sysfs node should it be important for debugging.

Noralf.

> +		ws->bl = devm_backlight_device_register(dev, "ws2401", dev, ws,
> +							&ws2401_bl_ops, &ws2401_bl_props);
> +		if (IS_ERR(ws->bl)) {
> +			ret = PTR_ERR(ws->bl);
> +			return dev_err_probe(dev, ret,
> +					     "failed to register backlight device\n");
> +		}
> +		ws->panel.backlight = ws->bl;
> +	} else {
> +		dev_info(dev, "using external backlight\n");
> +	}
> +
> +	spi_set_drvdata(spi, ws);
> +
> +	drm_panel_add(&ws->panel);
> +	dev_dbg(dev, "added panel\n");
> +
> +	return 0;
> +}