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

Den 30.06.2021 20.28, 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@vger.kernel.org Cc: Douglas Anderson dianders@chromium.org Cc: Noralf Trønnes noralf@tronnes.org Signed-off-by: Linus Walleij linus.walleij@linaro.org

---

diff --git a/drivers/gpu/drm/panel/panel-widechips-ws2401.c b/drivers/gpu/drm/panel/panel-widechips-ws2401.c

+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);

Didn't spot this earlier, but you're reading ID2 and ID3 into 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);

+}

+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;
• ws2401_power_on(ws);
• ws2401_read_mtp_id(ws);
• ws2401_power_off(ws);
• drm_panel_init(&ws->panel, dev, &ws2401_drm_funcs,

•        DRM_MODE_CONNECTOR_DPI);
  

• ret = drm_panel_of_backlight(&ws->panel);

I still don't see how internal backlight should work, have you tried it?

Tracking down the call chain, drm_panel_of_backlight() can only return 0, -EINVAL, -ENOMEM and -EPROBE_DEFER. It returns 0 whether the backlight property exists or not.

I would do something like this:

if (ret) return dev_err_probe(dev, ret, "failed to get backlight");

if (!ws->panel.backlight) {

• dev_dbg(dev, "no external backlight, using internal backlight\n");
  

• ws->bl = devm_backlight_device_register(dev, "ws2401", dev, ws,
  

• 					&ws2401_bl_ops, &ws2401_bl_props);
  

• if (IS_ERR(ws->bl))
  

• 	return dev_err_probe(dev, PTR_ERR(ws->bl),
  

• 			     "failed to register backlight device\n");
  

• ws->panel.backlight = ws->bl;
  

• } else {

• dev_dbg(dev, "using external backlight\n");
  

• }
• spi_set_drvdata(spi, ws);
• drm_panel_add(&ws->panel);
• dev_dbg(dev, "added panel\n");
• return 0;

+}

Noralf.