[PATCH 1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[PATCH 1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[Linus Walleij]

This adds device tree bindings for the Samsung LMS397KF04
RGB DPI display panel.

Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../display/panel/samsung,lms397kf04.yaml     | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml
new file mode 100644
index 000000000000..4cb75a5f2e3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/samsung,lms397kf04.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung LMS397KF04 display panel
+
+description: The datasheet claims this is based around a display controller
+  named DB7430 with a separate backlight controller.
+
+maintainers:
+  - Linus Walleij <linus.walleij@linaro.org>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: samsung,lms397kf04
+
+  reg: true
+
+  reset-gpios: true
+
+  vci-supply:
+    description: regulator that supplies the VCI analog voltage
+      usually around 3.0 V
+
+  vccio-supply:
+    description: regulator that supplies the VCCIO voltage usually
+      around 1.8 V
+
+  backlight: true
+
+  spi-max-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: inherited as a SPI client node, the datasheet specifies
+      maximum 300 ns minimum cycle which gives around 3 MHz max frequency
+    maximum: 3000000
+
+  port: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      panel@0 {
+        compatible = "samsung,lms397kf04";
+        spi-max-frequency = <3000000>;
+        reg = <0>;
+        vci-supply = <&lcd_3v0_reg>;
+        vccio-supply = <&lcd_1v8_reg>;
+        reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
+        backlight = <&ktd259>;
+
+        port {
+          panel_in: endpoint {
+            remote-endpoint = <&display_out>;
+          };
+        };
+      };
+    };
+
+...
-- 
2.29.2

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

[PATCH 2/2] drm/panel: lms397kf04: Add driver for LMS397KF04

[Linus Walleij]

This adds a new driver for the Samsung LMS397KF04
DPI display controlled over SPI.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/panel/Kconfig                 |   8 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-samsung-lms397kf04.c  | 421 ++++++++++++++++++
 4 files changed, 437 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-lms397kf04.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..6d32432adebe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5731,6 +5731,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/display/panel/raydium,rm67191.yaml
 F:	drivers/gpu/drm/panel/panel-raydium-rm67191.c
 
+DRM DRIVER FOR SAMSUNG LMS397KF04 PANELS
+M:	Linus Walleij <linus.walleij@linaro.org>
+S:	Maintained
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml
+F:	drivers/gpu/drm/panel/panel-samsung-lms397kf04.c
+
 DRM DRIVER FOR SITRONIX ST7703 PANELS
 M:	Guido Günther <agx@sigxcpu.org>
 R:	Purism Kernel Team <kernel@puri.sm>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4894913936e9..8e24a92ad9ce 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -348,6 +348,14 @@ config DRM_PANEL_SAMSUNG_S6D16D0
 	depends on DRM_MIPI_DSI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_SAMSUNG_LMS397KF04
+	tristate "Samsung LMS397KF04 DPI panel"
+	depends on OF && SPI && GPIOLIB
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for the Samsung
+	  LMS397KF04 480x800 DPI panel.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index cae4d976c069..de23d00937db 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
 obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_LMS397KF04) += panel-samsung-lms397kf04.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c b/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c
new file mode 100644
index 000000000000..41290cadc351
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Panel driver for the Samsung LMS397KF04 480x800 DPI RGB panel.
+ * According to the data sheet the display controller is called DB7430
+ * Linus Walleij <linus.walleij@linaro.org>
+ */
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.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>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+
+#define LMS397_MANUFACTURER_CMD		0xb0
+#define LMS397_UNKNOWN_B4		0xb4
+#define LMS397_USER_SELECT		0xb5
+#define LMS397_UNKNOWN_B7		0xb7
+#define LMS397_UNKNOWN_B8		0xb8
+#define LMS397_PANEL_DRIVING		0xc0
+#define LMS397_SOURCE_CONTROL		0xc1
+#define LMS397_GATE_INTERFACE		0xc4
+#define LMS397_DISPLAY_H_TIMING		0xc5
+#define LMS397_RGB_SYNC_OPTION		0xc6
+#define LMS397_GAMMA_SET_RED		0xc8
+#define LMS397_GAMMA_SET_GREEN		0xc9
+#define LMS397_GAMMA_SET_BLUE		0xca
+#define LMS397_BIAS_CURRENT_CTRL	0xd1
+#define LMS397_DDV_CTRL			0xd2
+#define LMS397_GAMMA_CTRL_REF		0xd3
+#define LMS397_UNKNOWN_D4		0xd4
+#define LMS397_DCDC_CTRL		0xd5
+#define LMS397_VCL_CTRL			0xd6
+#define LMS397_UNKNOWN_F8		0xf8
+#define LMS397_UNKNOWN_FC		0xfc
+
+#define DATA_MASK	0x100
+
+/**
+ * struct lms397kf04 - state container for the LMS397kf04 panel
+ */
+struct lms397kf04 {
+	/**
+	 * @dev: the container device
+	 */
+	struct device *dev;
+	/**
+	 * @spi: the corresponding SPI device
+	 */
+	struct spi_device *spi;
+	/**
+	 * @panel: the DRM panel instance for this device
+	 */
+	struct drm_panel panel;
+	/**
+	 * @width: the width of this panel in mm
+	 */
+	u32 width;
+	/**
+	 * @height: the height of this panel in mm
+	 */
+	u32 height;
+	/**
+	 * @reset: reset GPIO line
+	 */
+	struct gpio_desc *reset;
+	/**
+	 * @regulators: VCCIO and VIO supply regulators
+	 */
+	struct regulator_bulk_data regulators[2];
+};
+
+static const struct drm_display_mode lms397kf04_mode = {
+	/*
+	 * 31 ns period min (htotal*vtotal*vrefresh)/1000
+	 * gives a Vrefresh of ~71 Hz.
+	 */
+	.clock = 32258,
+	.hdisplay = 480,
+	.hsync_start = 480 + 10,
+	.hsync_end = 480 + 10 + 4,
+	.htotal = 480 + 10 + 4 + 40,
+	.vdisplay = 800,
+	.vsync_start = 800 + 6,
+	.vsync_end = 800 + 6 + 1,
+	.vtotal = 800 + 6 + 1 + 7,
+	.width_mm = 53,
+	.height_mm = 87,
+	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static inline struct lms397kf04 *to_lms397kf04(struct drm_panel *panel)
+{
+	return container_of(panel, struct lms397kf04, panel);
+}
+
+static int lms397kf04_write_word(struct lms397kf04 *lms, u16 data)
+{
+	/* SPI buffers are always in CPU order */
+	return spi_write(lms->spi, &data, 2);
+}
+
+static int lms397kf04_dcs_write(struct lms397kf04 *lms, const u8 *data, size_t len)
+{
+	int ret;
+
+	dev_dbg(lms->dev, "SPI writing dcs seq: %*ph\n", (int)len, data);
+
+	/*
+	 * This sends 9 bits with the first bit (bit 8) set to 0
+	 * This indicates that this is a command. Anything after the
+	 * command is data.
+	 */
+	ret = lms397kf04_write_word(lms, *data);
+
+	while (!ret && --len) {
+		++data;
+		/* This sends 9 bits with the first bit (bit 8) set to 1 */
+		ret = lms397kf04_write_word(lms, *data | DATA_MASK);
+	}
+
+	if (ret) {
+		dev_err(lms->dev, "SPI error %d writing dcs seq: %*ph\n", ret,
+			(int)len, data);
+	}
+
+	return ret;
+}
+
+#define lms397kf04_dcs_write_seq_static(ctx, seq ...) \
+	({ \
+		static const u8 d[] = { seq }; \
+		lms397kf04_dcs_write(ctx, d, ARRAY_SIZE(d)); \
+	})
+
+static int lms397kf04_power_on(struct lms397kf04 *lms)
+{
+	int ret;
+
+	/* Power up */
+	ret = regulator_bulk_enable(ARRAY_SIZE(lms->regulators),
+				    lms->regulators);
+	if (ret) {
+		dev_err(lms->dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+	msleep(50);
+
+	/* Assert reset >=1 ms */
+	gpiod_set_value_cansleep(lms->reset, 1);
+	msleep(1);
+	/* De-assert reset */
+	gpiod_set_value_cansleep(lms->reset, 0);
+	/* Wait >= 10 ms */
+	msleep(10);
+	dev_info(lms->dev, "de-asserted RESET\n");
+
+	/*
+	 * This is set to 0x0a (RGB/BGR order + horizontal flip) in order
+	 * to make the display behave normally. If this is not set the displays
+	 * normal output behaviour is horizontally flipped and BGR ordered. Do
+	 * it twice because the first message doesn't always "take".
+	 */
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);
+	/* Called "Access protection off" in vendor code */
+	lms397kf04_dcs_write_seq_static(lms, LMS397_MANUFACTURER_CMD, 0x00);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_PANEL_DRIVING, 0x28, 0x08);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_SOURCE_CONTROL,
+					0x01, 0x30, 0x15, 0x05, 0x22);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_GATE_INTERFACE,
+					0x10, 0x01, 0x00);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_DISPLAY_H_TIMING,
+					0x06, 0x55, 0x03, 0x07, 0x0b,
+					0x33, 0x00, 0x01, 0x03);
+	/*
+	 * 0x00 in datasheet 0x01 in vendor code 0x00, it seems 0x01 means
+	 * DE active high and 0x00 means DE active low.
+	 */
+	lms397kf04_dcs_write_seq_static(lms, LMS397_RGB_SYNC_OPTION, 0x01);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_RED,
+		/* R positive gamma */ 0x00,
+		0x0A, 0x31, 0x3B, 0x4E, 0x58, 0x59, 0x5B, 0x58, 0x5E, 0x62,
+		0x60, 0x61, 0x5E, 0x62, 0x55, 0x55, 0x7F, 0x08,
+		/* R negative gamma */ 0x00,
+		0x0A, 0x31, 0x3B, 0x4E, 0x58, 0x59, 0x5B, 0x58, 0x5E, 0x62,
+		0x60, 0x61, 0x5E, 0x62, 0x55, 0x55, 0x7F, 0x08);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_GREEN,
+		/* G positive gamma */ 0x00,
+		0x25, 0x15, 0x28, 0x3D, 0x4A, 0x48, 0x4C, 0x4A, 0x52, 0x59,
+		0x59, 0x5B, 0x56, 0x60, 0x5D, 0x55, 0x7F, 0x0A,
+		/* G negative gamma */ 0x00,
+		0x25, 0x15, 0x28, 0x3D, 0x4A, 0x48, 0x4C, 0x4A, 0x52, 0x59,
+		0x59, 0x5B, 0x56, 0x60, 0x5D, 0x55, 0x7F, 0x0A);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_BLUE,
+		/* B positive gamma */ 0x00,
+		0x48, 0x10, 0x1F, 0x2F, 0x35, 0x38, 0x3D, 0x3C, 0x45, 0x4D,
+		0x4E, 0x52, 0x51, 0x60, 0x7F, 0x7E, 0x7F, 0x0C,
+		/* B negative gamma */ 0x00,
+		0x48, 0x10, 0x1F, 0x2F, 0x35, 0x38, 0x3D, 0x3C, 0x45, 0x4D,
+		0x4E, 0x52, 0x51, 0x60, 0x7F, 0x7E, 0x7F, 0x0C);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_BIAS_CURRENT_CTRL,
+					0x33, 0x13);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_DDV_CTRL,
+					0x11, 0x00, 0x00);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_CTRL_REF,
+					0x50, 0x50);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_DCDC_CTRL,
+					0x2f, 0x11, 0x1e, 0x46);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_VCL_CTRL,
+					0x11, 0x0a);
+
+	return 0;
+}
+
+static void lms397kf04_power_off(struct lms397kf04 *lms)
+{
+	/* Go into RESET and disable regulators */
+	gpiod_set_value_cansleep(lms->reset, 1);
+	regulator_bulk_disable(ARRAY_SIZE(lms->regulators),
+			       lms->regulators);
+}
+
+static int lms397kf04_unprepare(struct drm_panel *panel)
+{
+	struct lms397kf04 *lms = to_lms397kf04(panel);
+
+	lms397kf04_power_off(lms);
+
+	return 0;
+}
+
+static int lms397kf04_disable(struct drm_panel *panel)
+{
+	struct lms397kf04 *lms = to_lms397kf04(panel);
+
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_DISPLAY_OFF);
+	msleep(25);
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_ENTER_SLEEP_MODE);
+	msleep(120);
+
+	return 0;
+}
+
+static int lms397kf04_prepare(struct drm_panel *panel)
+{
+	struct lms397kf04 *lms = to_lms397kf04(panel);
+	int ret;
+
+	ret = lms397kf04_power_on(lms);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int lms397kf04_enable(struct drm_panel *panel)
+{
+	struct lms397kf04 *lms = to_lms397kf04(panel);
+
+	/* Exit sleep mode */
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(20);
+
+	/* NVM (non-volatile memory) load sequence */
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_D4,
+					0x52, 0x5e);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_F8,
+					0x01, 0xf5, 0xf2, 0x71, 0x44);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_FC,
+					0x00, 0x08);
+	msleep(150);
+
+	/* CABC turn on sequence (BC = backlight control) */
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B4,
+					0x0f, 0x00, 0x50);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_USER_SELECT, 0x80);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B7, 0x24);
+	lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B8, 0x01);
+
+	/* Turn on display */
+	lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_DISPLAY_ON);
+
+	/* Update brightness */
+
+	return 0;
+}
+
+/**
+ * lms397kf04_get_modes() - return the mode
+ * @panel: the panel to get the mode for
+ * @connector: reference to the central DRM connector control structure
+ */
+static int lms397kf04_get_modes(struct drm_panel *panel,
+			    struct drm_connector *connector)
+{
+	struct lms397kf04 *lms = to_lms397kf04(panel);
+	struct drm_display_mode *mode;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	mode = drm_mode_duplicate(connector->dev, &lms397kf04_mode);
+	if (!mode) {
+		dev_err(lms->dev, "failed to add mode\n");
+		return -ENOMEM;
+	}
+
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	connector->display_info.bus_flags =
+		DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	drm_mode_set_name(mode);
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs lms397kf04_drm_funcs = {
+	.disable = lms397kf04_disable,
+	.unprepare = lms397kf04_unprepare,
+	.prepare = lms397kf04_prepare,
+	.enable = lms397kf04_enable,
+	.get_modes = lms397kf04_get_modes,
+};
+
+static int lms397kf04_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct lms397kf04 *lms;
+	int ret;
+
+	lms = devm_kzalloc(dev, sizeof(*lms), GFP_KERNEL);
+	if (!lms)
+		return -ENOMEM;
+	lms->dev = dev;
+
+	/*
+	 * VCI   is the analog voltage supply
+	 * VCCIO is the digital I/O voltage supply
+	 */
+	lms->regulators[0].supply = "vci";
+	lms->regulators[1].supply = "vccio";
+	ret = devm_regulator_bulk_get(dev,
+				      ARRAY_SIZE(lms->regulators),
+				      lms->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
+	/* This asserts the RESET signal, putting the display into reset */
+	lms->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(lms->reset)) {
+		dev_err(dev, "no RESET GPIO\n");
+		return -ENODEV;
+	}
+
+	spi->bits_per_word = 9;
+	/* Preserve e.g. SPI_3WIRE setting */
+	spi->mode |= SPI_MODE_3;
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		dev_err(dev, "spi setup failed.\n");
+		return ret;
+	}
+	lms->spi = spi;
+
+	drm_panel_init(&lms->panel, dev, &lms397kf04_drm_funcs,
+		       DRM_MODE_CONNECTOR_DPI);
+
+	/* FIXME: if no external backlight, use internal backlight */
+	ret = drm_panel_of_backlight(&lms->panel);
+	if (ret) {
+		dev_info(dev, "failed to add backlight\n");
+		return ret;
+	}
+
+	spi_set_drvdata(spi, lms);
+
+	drm_panel_add(&lms->panel);
+	dev_info(dev, "added panel\n");
+
+	return 0;
+}
+
+static int lms397kf04_remove(struct spi_device *spi)
+{
+	struct lms397kf04 *lms = spi_get_drvdata(spi);
+
+	drm_panel_remove(&lms->panel);
+	return 0;
+}
+
+static const struct of_device_id lms397kf04_match[] = {
+	{ .compatible = "samsung,lms397kf04", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, lms397kf04_match);
+
+static struct spi_driver lms397kf04_driver = {
+	.probe		= lms397kf04_probe,
+	.remove		= lms397kf04_remove,
+	.driver		= {
+		.name	= "lms397kf04-panel",
+		.of_match_table = lms397kf04_match,
+	},
+};
+module_spi_driver(lms397kf04_driver);
+
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("Samsung LMS397KF04 panel driver");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

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

Re: [PATCH 1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[Rob Herring]

On Tue, 06 Apr 2021 01:47:12 +0200, Linus Walleij wrote:
> This adds device tree bindings for the Samsung LMS397KF04
> RGB DPI display panel.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../display/panel/samsung,lms397kf04.yaml     | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[Linus Walleij]

On Tue, Apr 6, 2021 at 1:47 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds device tree bindings for the Samsung LMS397KF04
> RGB DPI display panel.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Someone on DRM misc up to review and/or apply these two
patches?

Doug? I bet you have some patch(es) you can toss back at me to
review in return.

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

Re: [2/2] drm/panel: lms397kf04: Add driver for LMS397KF04

[Doug Anderson]

Hi,

On Thu, Apr 29, 2021 at 8:40 AM Linus Walleij <linus.walleij@linaro.org wrote:
>
> @@ -33,6 +33,7 @@ obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen
>  obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o
>  obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
>  obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o
> +obj-$(CONFIG_DRM_PANEL_SAMSUNG_LMS397KF04) += panel-samsung-lms397kf04.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o

nit: sort ordering is off by one. D comes before M.


> diff --git a/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c b/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c
> new file mode 100644
> index 000000000000..41290cadc351
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-samsung-lms397kf04.c
> @@ -0,0 +1,421 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panel driver for the Samsung LMS397KF04 480x800 DPI RGB panel.
> + * According to the data sheet the display controller is called DB7430
> + * Linus Walleij <linus.walleij@linaro.org>
> + */
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.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>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +
> +#define LMS397_MANUFACTURER_CMD                0xb0
> +#define LMS397_UNKNOWN_B4              0xb4
> +#define LMS397_USER_SELECT             0xb5
> +#define LMS397_UNKNOWN_B7              0xb7
> +#define LMS397_UNKNOWN_B8              0xb8
> +#define LMS397_PANEL_DRIVING           0xc0
> +#define LMS397_SOURCE_CONTROL          0xc1
> +#define LMS397_GATE_INTERFACE          0xc4
> +#define LMS397_DISPLAY_H_TIMING                0xc5
> +#define LMS397_RGB_SYNC_OPTION         0xc6
> +#define LMS397_GAMMA_SET_RED           0xc8
> +#define LMS397_GAMMA_SET_GREEN         0xc9
> +#define LMS397_GAMMA_SET_BLUE          0xca
> +#define LMS397_BIAS_CURRENT_CTRL       0xd1
> +#define LMS397_DDV_CTRL                        0xd2
> +#define LMS397_GAMMA_CTRL_REF          0xd3
> +#define LMS397_UNKNOWN_D4              0xd4
> +#define LMS397_DCDC_CTRL               0xd5
> +#define LMS397_VCL_CTRL                        0xd6
> +#define LMS397_UNKNOWN_F8              0xf8
> +#define LMS397_UNKNOWN_FC              0xfc

I managed to dig up a copy of the DCS spec. It says that all these
0xb0 - 0xfc are specific to each manufacturer, so that makes sense
that they're all defined in this file instead of somewhere common.
...but it also says that, at least the way they're intended to be
used, these commands are all supposed to be used only in the factory
and then disabled. I guess maybe the manufacturer of this panel
ignored that and requires these things to be configured each time the
panel is powered up?

Also: should these #defines have a "DB7430" prefix instead of "LMS397"
prefix? Presumably all of these commands are defined by the display
controller and the only "LMS397" specific stuff is what you need to
program into them for your display. I'm trying to think forward to
when the second panel comes along that also has a DB7430 display
controller.


> +
> +#define DATA_MASK      0x100
> +
> +/**
> + * struct lms397kf04 - state container for the LMS397kf04 panel
> + */
> +struct lms397kf04 {
> +       /**
> +        * @dev: the container device
> +        */
> +       struct device *dev;

optional nit: these are all small descriptions. Why not move them all
to the single-line format, like:

/** @dev: the container device */


> +       /**
> +        * @spi: the corresponding SPI device
> +        */
> +       struct spi_device *spi;
> +       /**
> +        * @panel: the DRM panel instance for this device
> +        */
> +       struct drm_panel panel;
> +       /**
> +        * @width: the width of this panel in mm
> +        */
> +       u32 width;
> +       /**
> +        * @height: the height of this panel in mm
> +        */
> +       u32 height;
> +       /**
> +        * @reset: reset GPIO line
> +        */
> +       struct gpio_desc *reset;
> +       /**
> +        * @regulators: VCCIO and VIO supply regulators
> +        */
> +       struct regulator_bulk_data regulators[2];
> +};
> +
> +static const struct drm_display_mode lms397kf04_mode = {
> +       /*
> +        * 31 ns period min (htotal*vtotal*vrefresh)/1000
> +        * gives a Vrefresh of ~71 Hz.
> +        */
> +       .clock = 32258,
> +       .hdisplay = 480,
> +       .hsync_start = 480 + 10,
> +       .hsync_end = 480 + 10 + 4,
> +       .htotal = 480 + 10 + 4 + 40,
> +       .vdisplay = 800,
> +       .vsync_start = 800 + 6,
> +       .vsync_end = 800 + 6 + 1,
> +       .vtotal = 800 + 6 + 1 + 7,
> +       .width_mm = 53,
> +       .height_mm = 87,
> +       .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};
> +
> +static inline struct lms397kf04 *to_lms397kf04(struct drm_panel *panel)
> +{
> +       return container_of(panel, struct lms397kf04, panel);
> +}
> +
> +static int lms397kf04_write_word(struct lms397kf04 *lms, u16 data)
> +{
> +       /* SPI buffers are always in CPU order */
> +       return spi_write(lms->spi, &data, 2);
> +}
> +
> +static int lms397kf04_dcs_write(struct lms397kf04 *lms, const u8 *data, size_t len)
> +{
> +       int ret;
> +
> +       dev_dbg(lms->dev, "SPI writing dcs seq: %*ph\n", (int)len, data);
> +
> +       /*
> +        * This sends 9 bits with the first bit (bit 8) set to 0
> +        * This indicates that this is a command. Anything after the
> +        * command is data.
> +        */
> +       ret = lms397kf04_write_word(lms, *data);
> +
> +       while (!ret && --len) {
> +               ++data;
> +               /* This sends 9 bits with the first bit (bit 8) set to 1 */
> +               ret = lms397kf04_write_word(lms, *data | DATA_MASK);
> +       }
> +
> +       if (ret) {
> +               dev_err(lms->dev, "SPI error %d writing dcs seq: %*ph\n", ret,
> +                       (int)len, data);
> +       }
> +
> +       return ret;

So I've never looked at MIPI stuff at all before. ...but, as far as I
can tell your panel is implementing MIPI DBI Type C Option 1 (3-line
SPI). It feels like you should be using the functions for dealing with
MIPI DBI, like mipi_dbi_spi_init(), mipi_dbi_command(). If I'm reading
the code correctly, I think that will have the benefit of making your
panel more flexible in terms of the capabilities of the SPI
controller. It seems to handle the case when the SPI controller
doesn't support 9-bits-per-word transfers, for instance.

If you have a good reason for not using the MIPI DBI functions then
let me know and I'll look over your functions more closely.


> +}
> +
> +#define lms397kf04_dcs_write_seq_static(ctx, seq ...) \
> +       ({ \
> +               static const u8 d[] = { seq }; \
> +               lms397kf04_dcs_write(ctx, d, ARRAY_SIZE(d)); \
> +       })
> +
> +static int lms397kf04_power_on(struct lms397kf04 *lms)
> +{
> +       int ret;
> +
> +       /* Power up */
> +       ret = regulator_bulk_enable(ARRAY_SIZE(lms->regulators),
> +                                   lms->regulators);
> +       if (ret) {
> +               dev_err(lms->dev, "failed to enable regulators: %d\n", ret);
> +               return ret;
> +       }
> +       msleep(50);
> +
> +       /* Assert reset >=1 ms */
> +       gpiod_set_value_cansleep(lms->reset, 1);
> +       msleep(1);

I dunno if it's still conventional wisdom, but
"Documentation/timers/timers-howto.rst" still says that you should be
using usleep() for such short sleeps.


> +       /* De-assert reset */
> +       gpiod_set_value_cansleep(lms->reset, 0);
> +       /* Wait >= 10 ms */
> +       msleep(10);
> +       dev_info(lms->dev, "de-asserted RESET\n");

Can this be something less than dev_info()? dev_dbg() maybe?


> +
> +       /*
> +        * This is set to 0x0a (RGB/BGR order + horizontal flip) in order
> +        * to make the display behave normally. If this is not set the displays
> +        * normal output behaviour is horizontally flipped and BGR ordered. Do
> +        * it twice because the first message doesn't always "take".
> +        */
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);

This actually looks like a somewhat standard property, but I guess
exactly how it's interpreted is somewhat subject to the panel. Seems
fine to just hardcode something (like you're doing) for now and
someone can later figure out how to make it fancier.

Also: should we be error-checking lms397kf04_dcs_write_seq_static()
return values in this function? spi_write() can fail...


> +       /* Called "Access protection off" in vendor code */
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_MANUFACTURER_CMD, 0x00);

Should you change the #define to LMS397_ACCESS_PROTECTION or something, then?

I don't have the vendor code, but I would suspect that
"MANUFACTURER_CMD" is 0xb0 because the MIPI DCS document describes
0xb0 as the ID of the first custom manufacturer-specific command, but
really all the commands 0xb0 - 0xff are manufacturer-specific
commands.


> +       lms397kf04_dcs_write_seq_static(lms, LMS397_PANEL_DRIVING, 0x28, 0x08);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_SOURCE_CONTROL,
> +                                       0x01, 0x30, 0x15, 0x05, 0x22);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_GATE_INTERFACE,
> +                                       0x10, 0x01, 0x00);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_DISPLAY_H_TIMING,
> +                                       0x06, 0x55, 0x03, 0x07, 0x0b,
> +                                       0x33, 0x00, 0x01, 0x03);
> +       /*
> +        * 0x00 in datasheet 0x01 in vendor code 0x00, it seems 0x01 means
> +        * DE active high and 0x00 means DE active low.
> +        */
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_RGB_SYNC_OPTION, 0x01);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_RED,
> +               /* R positive gamma */ 0x00,
> +               0x0A, 0x31, 0x3B, 0x4E, 0x58, 0x59, 0x5B, 0x58, 0x5E, 0x62,
> +               0x60, 0x61, 0x5E, 0x62, 0x55, 0x55, 0x7F, 0x08,
> +               /* R negative gamma */ 0x00,
> +               0x0A, 0x31, 0x3B, 0x4E, 0x58, 0x59, 0x5B, 0x58, 0x5E, 0x62,
> +               0x60, 0x61, 0x5E, 0x62, 0x55, 0x55, 0x7F, 0x08);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_GREEN,
> +               /* G positive gamma */ 0x00,
> +               0x25, 0x15, 0x28, 0x3D, 0x4A, 0x48, 0x4C, 0x4A, 0x52, 0x59,
> +               0x59, 0x5B, 0x56, 0x60, 0x5D, 0x55, 0x7F, 0x0A,
> +               /* G negative gamma */ 0x00,
> +               0x25, 0x15, 0x28, 0x3D, 0x4A, 0x48, 0x4C, 0x4A, 0x52, 0x59,
> +               0x59, 0x5B, 0x56, 0x60, 0x5D, 0x55, 0x7F, 0x0A);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_SET_BLUE,
> +               /* B positive gamma */ 0x00,
> +               0x48, 0x10, 0x1F, 0x2F, 0x35, 0x38, 0x3D, 0x3C, 0x45, 0x4D,
> +               0x4E, 0x52, 0x51, 0x60, 0x7F, 0x7E, 0x7F, 0x0C,
> +               /* B negative gamma */ 0x00,
> +               0x48, 0x10, 0x1F, 0x2F, 0x35, 0x38, 0x3D, 0x3C, 0x45, 0x4D,
> +               0x4E, 0x52, 0x51, 0x60, 0x7F, 0x7E, 0x7F, 0x0C);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_BIAS_CURRENT_CTRL,
> +                                       0x33, 0x13);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_DDV_CTRL,
> +                                       0x11, 0x00, 0x00);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_GAMMA_CTRL_REF,
> +                                       0x50, 0x50);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_DCDC_CTRL,
> +                                       0x2f, 0x11, 0x1e, 0x46);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_VCL_CTRL,
> +                                       0x11, 0x0a);
> +
> +       return 0;
> +}
> +
> +static void lms397kf04_power_off(struct lms397kf04 *lms)
> +{
> +       /* Go into RESET and disable regulators */
> +       gpiod_set_value_cansleep(lms->reset, 1);
> +       regulator_bulk_disable(ARRAY_SIZE(lms->regulators),
> +                              lms->regulators);

In theory regulator_bulk_disable() could fail. Since unprepare() also
has an "error" return maybe return it?


> +}
> +
> +static int lms397kf04_unprepare(struct drm_panel *panel)
> +{
> +       struct lms397kf04 *lms = to_lms397kf04(panel);
> +
> +       lms397kf04_power_off(lms);
> +
> +       return 0;
> +}

It could be shorter. Assuming you add a return value to lms397kf04_power_off():

static int lms397kf04_unprepare(struct drm_panel *panel)
{
  return lms397kf04_power_off(to_lms397kf04(panel));
}


> +
> +static int lms397kf04_disable(struct drm_panel *panel)
> +{
> +       struct lms397kf04 *lms = to_lms397kf04(panel);
> +
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_DISPLAY_OFF);
> +       msleep(25);
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_ENTER_SLEEP_MODE);
> +       msleep(120);
> +
> +       return 0;
> +}
> +
> +static int lms397kf04_prepare(struct drm_panel *panel)
> +{
> +       struct lms397kf04 *lms = to_lms397kf04(panel);
> +       int ret;
> +
> +       ret = lms397kf04_power_on(lms);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

It could be shorter:

static int lms397kf04_prepare(struct drm_panel *panel)
{
  return lms397kf04_power_on(to_lms397kf04(panel));
}

> +}
> +
> +static int lms397kf04_enable(struct drm_panel *panel)
> +{
> +       struct lms397kf04 *lms = to_lms397kf04(panel);
> +
> +       /* Exit sleep mode */
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_EXIT_SLEEP_MODE);
> +       msleep(20);
> +
> +       /* NVM (non-volatile memory) load sequence */
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_D4,
> +                                       0x52, 0x5e);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_F8,
> +                                       0x01, 0xf5, 0xf2, 0x71, 0x44);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_FC,
> +                                       0x00, 0x08);
> +       msleep(150);

I'll believe you if you tell me that it's correct, but something feels
odd to me. As I mentioned above, the MIPI DCS specs say that it's
expected that most of the configuration that you're doing in
lms397kf04_power_on() would happen at manufacturer time and then be
"locked" so you don't need to do it again.

I could sorta believe that maybe some panels wouldn't have any
non-volatile storage and that would explain why you need to program
this stuff on every enable. ...but now the above comment says that
it's loading stuff from non-volatile memory.

Are you sure that all of the magic config commands you have in
lms397kf04_power_on() are actually needed / doing anything? Could they
be leftover from some example code and they're not actually needed
anymore?


> +       /* CABC turn on sequence (BC = backlight control) */

I would expect that the "CA" would mean "Content Adaptive". As I
understand it CABC saves power by dimming the backlight on the screen
(or maybe part of the screen) depending on the contents. I think it's
supposed to be a good power savings for watching full screen movies
where it's common that nothing is using "full white".


> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B4,
> +                                       0x0f, 0x00, 0x50);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_USER_SELECT, 0x80);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B7, 0x24);
> +       lms397kf04_dcs_write_seq_static(lms, LMS397_UNKNOWN_B8, 0x01);
> +
> +       /* Turn on display */
> +       lms397kf04_dcs_write_seq_static(lms, MIPI_DCS_SET_DISPLAY_ON);
> +
> +       /* Update brightness */
> +

Was there supposed to be something after "Update brightness"?


> +       return 0;
> +}
> +
> +/**
> + * lms397kf04_get_modes() - return the mode
> + * @panel: the panel to get the mode for
> + * @connector: reference to the central DRM connector control structure
> + */
> +static int lms397kf04_get_modes(struct drm_panel *panel,
> +                           struct drm_connector *connector)

nit: second line in function declaration has odd indentation.


> +{
> +       struct lms397kf04 *lms = to_lms397kf04(panel);
> +       struct drm_display_mode *mode;
> +       static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +
> +       mode = drm_mode_duplicate(connector->dev, &lms397kf04_mode);
> +       if (!mode) {
> +               dev_err(lms->dev, "failed to add mode\n");
> +               return -ENOMEM;
> +       }
> +
> +       connector->display_info.width_mm = mode->width_mm;
> +       connector->display_info.height_mm = mode->height_mm;
> +       connector->display_info.bus_flags =
> +               DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +       drm_display_info_set_bus_formats(&connector->display_info,
> +                                        &bus_format, 1);

panel-simple also sets the bpc in the "display_info". Do you need to?


> +
> +       drm_mode_set_name(mode);
> +       mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +       drm_mode_probed_add(connector, mode);
> +
> +       return 1;
> +}
> +
> +static const struct drm_panel_funcs lms397kf04_drm_funcs = {
> +       .disable = lms397kf04_disable,
> +       .unprepare = lms397kf04_unprepare,
> +       .prepare = lms397kf04_prepare,
> +       .enable = lms397kf04_enable,
> +       .get_modes = lms397kf04_get_modes,
> +};
> +
> +static int lms397kf04_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct lms397kf04 *lms;
> +       int ret;
> +
> +       lms = devm_kzalloc(dev, sizeof(*lms), GFP_KERNEL);
> +       if (!lms)
> +               return -ENOMEM;
> +       lms->dev = dev;
> +
> +       /*
> +        * VCI   is the analog voltage supply
> +        * VCCIO is the digital I/O voltage supply
> +        */
> +       lms->regulators[0].supply = "vci";
> +       lms->regulators[1].supply = "vccio";
> +       ret = devm_regulator_bulk_get(dev,
> +                                     ARRAY_SIZE(lms->regulators),
> +                                     lms->regulators);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
> +       /* This asserts the RESET signal, putting the display into reset */
> +       lms->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(lms->reset)) {
> +               dev_err(dev, "no RESET GPIO\n");
> +               return -ENODEV;

nit: include the error code in the error message? Here and in other
error messages in probe.

Also, you're forcing the error to -ENODEV. What if devm_gpiod_get()
returned -EPROBE_DEFER? That will cause a problem, right? Why not just
return the error code?

...oh, or more broadly use the nifty dev_err_probe() function that you
use above!


> +       }
> +
> +       spi->bits_per_word = 9;
> +       /* Preserve e.g. SPI_3WIRE setting */
> +       spi->mode |= SPI_MODE_3;
> +       ret = spi_setup(spi);
> +       if (ret < 0) {
> +               dev_err(dev, "spi setup failed.\n");
> +               return ret;
> +       }
> +       lms->spi = spi;
> +
> +       drm_panel_init(&lms->panel, dev, &lms397kf04_drm_funcs,
> +                      DRM_MODE_CONNECTOR_DPI);
> +
> +       /* FIXME: if no external backlight, use internal backlight */
> +       ret = drm_panel_of_backlight(&lms->panel);
> +       if (ret) {
> +               dev_info(dev, "failed to add backlight\n");

dev_err() ?


> +               return ret;
> +       }
> +
> +       spi_set_drvdata(spi, lms);
> +
> +       drm_panel_add(&lms->panel);
> +       dev_info(dev, "added panel\n");

Change to dev_dbg() ?

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

Re: [1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[Doug Anderson]

Hi,

On Thu, Apr 29, 2021 at 8:39 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This adds device tree bindings for the Samsung LMS397KF04
> RGB DPI display panel.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../display/panel/samsung,lms397kf04.yaml     | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)

Probably not needed since you have Rob Herring's review, but:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Samsung LMS397KF04

[Doug Anderson]

Hi,

On Thu, Apr 29, 2021 at 7:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Apr 6, 2021 at 1:47 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > This adds device tree bindings for the Samsung LMS397KF04
> > RGB DPI display panel.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Someone on DRM misc up to review and/or apply these two
> patches?
>
> Doug? I bet you have some patch(es) you can toss back at me to
> review in return.

I know next to nothing about MIPI panels, but I gave it my best shot.
Please take my comments with a grain of salt since I'm still a bit
noobie.

I think I already probably owed you a review anyway for previous
reviews you did for me. I do have my big 20-part series out there and
I certainly won't object to more review but most of the patches are
close to having enough review at the moment. For some of the simple
(dare I say "trivial"?) patches Bjorn was nice enough to provide
review. For the panel patches I got an extra review from Sean Paul. I
think there are only one or two patches in the series that have no
review at the moment because I had to spin them for Bjorn's feedback
and he hasn't gotten back to it yet.

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

Re: [2/2] drm/panel: lms397kf04: Add driver for LMS397KF04

[Linus Walleij]

Hi Doug,

just sent out a v2 of this! Hope you can look at it.

Some comment on comments:

On Thu, Apr 29, 2021 at 10:21 PM Doug Anderson <dianders@chromium.org> wrote:

(...)
> > +#define LMS397_UNKNOWN_F8              0xf8
> > +#define LMS397_UNKNOWN_FC              0xfc
>
> I managed to dig up a copy of the DCS spec. It says that all these
> 0xb0 - 0xfc are specific to each manufacturer, so that makes sense
> that they're all defined in this file instead of somewhere common.
> ...but it also says that, at least the way they're intended to be
> used, these commands are all supposed to be used only in the factory
> and then disabled. I guess maybe the manufacturer of this panel
> ignored that and requires these things to be configured each time the
> panel is powered up?
(...)
> I'll believe you if you tell me that it's correct, but something feels
> odd to me. As I mentioned above, the MIPI DCS specs say that it's
> expected that most of the configuration that you're doing in
> lms397kf04_power_on() would happen at manufacturer time and then be
> "locked" so you don't need to do it again.
>
> I could sorta believe that maybe some panels wouldn't have any
> non-volatile storage and that would explain why you need to program
> this stuff on every enable. ...but now the above comment says that
> it's loading stuff from non-volatile memory.
>
> Are you sure that all of the magic config commands you have in
> lms397kf04_power_on() are actually needed / doing anything? Could they
> be leftover from some example code and they're not actually needed
> anymore?

Yes it is absolutely necessary.

This happens for a bit, see for example
drivers/gpu/drm/panel/panel-novatek-nt35510.c

Why manufacturers (Samsung) do this I do not know for sure.
Could be either:

1. The flash memory is an optional external component so they save
  money on the BOM like this
2. Programming the flash in production takes too much time so they
  save manufacturing cost like this
3. Time-to-market, product was stressed out the door without
  time to set up the proper flashing on the production line
4. The panel actually has some settings from production, but oops
  they are wrong and now they can't be fixed.

Case 4 corresponds to e.g. all ACPI quirks we have because
abstracting things away by giving stuff BIOS is so good in theory
and fails so much in practice because bugs.

> So I've never looked at MIPI stuff at all before. ...but, as far as I
> can tell your panel is implementing MIPI DBI Type C Option 1 (3-line
> SPI). It feels like you should be using the functions for dealing with
> MIPI DBI, like mipi_dbi_spi_init(), mipi_dbi_command(). If I'm reading
> the code correctly, I think that will have the benefit of making your
> panel more flexible in terms of the capabilities of the SPI
> controller. It seems to handle the case when the SPI controller
> doesn't support 9-bits-per-word transfers, for instance.
>
> If you have a good reason for not using the MIPI DBI functions then
> let me know and I'll look over your functions more closely.

MIPI DBI nominally requires an extra (GPIO) line called D/C
(data/command) and this display for sure does not have it.
So it would be "type c1" (no extra GPIO).

I will test and see if I can get DBI working... if I do I will
respin v2 to v3.

Yours,
Linus Walleij