All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] drm: bridge: add ssd2825 RGB/DSI bridge support
@ 2025-02-13 13:56 Svyatoslav Ryhel
  2025-02-13 13:56 ` [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825 Svyatoslav Ryhel
  2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
  0 siblings, 2 replies; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-13 13:56 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-kernel

Solomon SSD2825 is a RGB to MIPI DSI bridge used in LG Optimus 4D P880
and LG Optimus Vu P895

Svyatoslav Ryhel (2):
  dt-bindings: display: bridge: Document Solomon SSD2825
  drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge

 .../display/bridge/solomon,ssd2825.yaml       | 140 ++++
 drivers/gpu/drm/bridge/Kconfig                |  13 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/ssd2825.c              | 741 ++++++++++++++++++
 4 files changed, 895 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
 create mode 100644 drivers/gpu/drm/bridge/ssd2825.c

-- 
2.43.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825
  2025-02-13 13:56 [PATCH v1 0/2] drm: bridge: add ssd2825 RGB/DSI bridge support Svyatoslav Ryhel
@ 2025-02-13 13:56 ` Svyatoslav Ryhel
  2025-02-13 20:34   ` Conor Dooley
  2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
  1 sibling, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-13 13:56 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-kernel

Add bindings for Solomon SSD2825 MIPI master bridge chip that connects an
application processor with traditional parallel LCD interface and an LCD
driver with MIPI slave interface. The SSD2825 supports both parallel RGB
interface and serial SPI interface.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../display/bridge/solomon,ssd2825.yaml       | 140 ++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
new file mode 100644
index 000000000000..cd7ff971495c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/solomon,ssd2825.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Solomon SSD2825 RGB to MIPI-DSI bridge
+
+maintainers:
+  - Svyatoslav Ryhel <clamor95@gmail.com>
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: solomon,ssd2825
+
+  reg:
+    maxItems: 1
+
+  reset-gpios: true
+
+  dvdd-supply:
+    description: Regulator for 1.2V digital power supply.
+
+  avdd-supply:
+    description: Regulator for 1.2V analog power supply.
+
+  vddio-supply:
+    description: Regulator for 1.8V IO power supply.
+
+  spi-max-frequency:
+    maximum: 1000000
+
+  spi-cpha: true
+  spi-cpol: true
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: tx_clk
+
+  solomon,hs-zero-delay-ns:
+    description:
+      HS zero delay period
+    default: 133
+
+  solomon,hs-prep-delay-ns:
+    description:
+      HS prep delay period
+    default: 40
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Video port for RGB input
+
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/$defs/endpoint-base
+            unevaluatedProperties: false
+
+            properties:
+              bus-width:
+                enum: [ 16, 18, 24 ]
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Video port for DSI output (panel or connector)
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dsi@2 {
+            compatible = "solomon,ssd2825";
+            reg = <2>;
+
+            spi-max-frequency = <1000000>;
+
+            spi-cpha;
+            spi-cpol;
+
+            reset-gpios = <&gpio 114 GPIO_ACTIVE_LOW>;
+
+            dvdd-supply = <&vdd_1v2>;
+            avdd-supply = <&vdd_1v2>;
+            vddio-supply = <&vdd_1v8_io>;
+
+            solomon,hs-zero-delay-ns = <300>;
+            solomon,hs-prep-delay-ns = <65>;
+
+            clocks = <&ssd2825_tx_clk>;
+            clock-names = "tx_clk";
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    bridge_input: endpoint {
+                        remote-endpoint = <&dpi_output>;
+                        bus-width = <24>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+
+                    bridge_output: endpoint {
+                        remote-endpoint = <&panel_input>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
  2025-02-13 13:56 [PATCH v1 0/2] drm: bridge: add ssd2825 RGB/DSI bridge support Svyatoslav Ryhel
  2025-02-13 13:56 ` [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825 Svyatoslav Ryhel
@ 2025-02-13 13:56 ` Svyatoslav Ryhel
  2025-02-13 16:05   ` Dmitry Baryshkov
                     ` (2 more replies)
  1 sibling, 3 replies; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-13 13:56 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-kernel

SSD2825 is an innovative and cost-effective MIPI Bridge Chip solution
targeting high resolution smartphones. It can convert 24bit RGB
interface into 4-lane MIPI-DSI interface to drive extremely high
resolution display modules of up to 800 x 1366, while supporting AMOLED,
a-si LCD or LTPS advanced panel technologies for smartphone applications.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/gpu/drm/bridge/Kconfig   |  13 +
 drivers/gpu/drm/bridge/Makefile  |   1 +
 drivers/gpu/drm/bridge/ssd2825.c | 741 +++++++++++++++++++++++++++++++
 3 files changed, 755 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ssd2825.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 6b4664d91faa..74aec32d0f3b 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -306,6 +306,19 @@ config DRM_SIMPLE_BRIDGE
 	  Support for non-programmable DRM bridges, such as ADI ADV7123, TI
 	  THS8134 and THS8135 or passive resistor ladder DACs.
 
+config DRM_SOLOMON_SSD2825
+	tristate "SSD2825 RGB/DSI bridge"
+	depends on OF
+	select DRM_MIPI_DSI
+	select DRM_KMS_HELPER
+	select DRM_PANEL
+	help
+	  Say Y here if you want support for the Solomon SSD2825 RGB/DSI
+	  SPI bridge driver.
+
+	  Say M here if you want to support this hardware as a module.
+	  The module will be named "solomon-ssd2825".
+
 config DRM_THINE_THC63LVD1024
 	tristate "Thine THC63LVD1024 LVDS decoder bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 97304b429a53..c621ab3fa3a9 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
 obj-$(CONFIG_DRM_SIMPLE_BRIDGE) += simple-bridge.o
+obj-$(CONFIG_DRM_SOLOMON_SSD2825) += ssd2825.o
 obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358762) += tc358762.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
diff --git a/drivers/gpu/drm/bridge/ssd2825.c b/drivers/gpu/drm/bridge/ssd2825.c
new file mode 100644
index 000000000000..823f6dc023a5
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ssd2825.c
@@ -0,0 +1,741 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <video/mipi_display.h>
+#include <video/videomode.h>
+
+#define SSD2825_DEVICE_ID_REG			0xb0
+#define SSD2825_RGB_INTERFACE_CTRL_REG_1	0xb1
+#define SSD2825_RGB_INTERFACE_CTRL_REG_2	0xb2
+#define SSD2825_RGB_INTERFACE_CTRL_REG_3	0xb3
+#define SSD2825_RGB_INTERFACE_CTRL_REG_4	0xb4
+#define SSD2825_RGB_INTERFACE_CTRL_REG_5	0xb5
+#define SSD2825_RGB_INTERFACE_CTRL_REG_6	0xb6
+#define   SSD2825_NON_BURST			BIT(2)
+#define   SSD2825_BURST				BIT(3)
+#define   SSD2825_PCKL_HIGH			BIT(13)
+#define   SSD2825_HSYNC_HIGH			BIT(14)
+#define   SSD2825_VSYNC_HIGH			BIT(15)
+#define SSD2825_CONFIGURATION_REG		0xb7
+#define   SSD2825_CONF_REG_HS			BIT(0)
+#define   SSD2825_CONF_REG_CKE			BIT(1)
+#define   SSD2825_CONF_REG_SLP			BIT(2)
+#define   SSD2825_CONF_REG_VEN			BIT(3)
+#define   SSD2825_CONF_REG_HCLK			BIT(4)
+#define   SSD2825_CONF_REG_CSS			BIT(5)
+#define   SSD2825_CONF_REG_DCS			BIT(6)
+#define   SSD2825_CONF_REG_REN			BIT(7)
+#define   SSD2825_CONF_REG_ECD			BIT(8)
+#define   SSD2825_CONF_REG_EOT			BIT(9)
+#define   SSD2825_CONF_REG_LPE			BIT(10)
+#define SSD2825_VC_CTRL_REG			0xb8
+#define SSD2825_PLL_CTRL_REG			0xb9
+#define SSD2825_PLL_CONFIGURATION_REG		0xba
+#define SSD2825_CLOCK_CTRL_REG			0xbb
+#define SSD2825_PACKET_SIZE_CTRL_REG_1		0xbc
+#define SSD2825_PACKET_SIZE_CTRL_REG_2		0xbd
+#define SSD2825_PACKET_SIZE_CTRL_REG_3		0xbe
+#define SSD2825_PACKET_DROP_REG			0xbf
+#define SSD2825_OPERATION_CTRL_REG		0xc0
+#define SSD2825_MAX_RETURN_SIZE_REG		0xc1
+#define SSD2825_RETURN_DATA_COUNT_REG		0xc2
+#define SSD2825_ACK_RESPONSE_REG		0xc3
+#define SSD2825_LINE_CTRL_REG			0xc4
+#define SSD2825_INTERRUPT_CTRL_REG		0xc5
+#define SSD2825_INTERRUPT_STATUS_REG		0xc6
+#define SSD2825_ERROR_STATUS_REG		0xc7
+#define SSD2825_DATA_FORMAT_REG			0xc8
+#define SSD2825_DELAY_ADJ_REG_1			0xc9
+#define SSD2825_DELAY_ADJ_REG_2			0xca
+#define SSD2825_DELAY_ADJ_REG_3			0xcb
+#define SSD2825_DELAY_ADJ_REG_4			0xcc
+#define SSD2825_DELAY_ADJ_REG_5			0xcd
+#define SSD2825_DELAY_ADJ_REG_6			0xce
+#define SSD2825_HS_TX_TIMER_REG_1		0xcf
+#define SSD2825_HS_TX_TIMER_REG_2		0xd0
+#define SSD2825_LP_RX_TIMER_REG_1		0xd1
+#define SSD2825_LP_RX_TIMER_REG_2		0xd2
+#define SSD2825_TE_STATUS_REG			0xd3
+#define SSD2825_SPI_READ_REG			0xd4
+#define   SSD2825_SPI_READ_REG_RESET		0xfa
+#define SSD2825_PLL_LOCK_REG			0xd5
+#define SSD2825_TEST_REG			0xd6
+#define SSD2825_TE_COUNT_REG			0xd7
+#define SSD2825_ANALOG_CTRL_REG_1		0xd8
+#define SSD2825_ANALOG_CTRL_REG_2		0xd9
+#define SSD2825_ANALOG_CTRL_REG_3		0xda
+#define SSD2825_ANALOG_CTRL_REG_4		0xdb
+#define SSD2825_INTERRUPT_OUT_CTRL_REG		0xdc
+#define SSD2825_RGB_INTERFACE_CTRL_REG_7	0xdd
+#define SSD2825_LANE_CONFIGURATION_REG		0xde
+#define SSD2825_DELAY_ADJ_REG_7			0xdf
+#define SSD2825_INPUT_PIN_CTRL_REG_1		0xe0
+#define SSD2825_INPUT_PIN_CTRL_REG_2		0xe1
+#define SSD2825_BIDIR_PIN_CTRL_REG_1		0xe2
+#define SSD2825_BIDIR_PIN_CTRL_REG_2		0xe3
+#define SSD2825_BIDIR_PIN_CTRL_REG_3		0xe4
+#define SSD2825_BIDIR_PIN_CTRL_REG_4		0xe5
+#define SSD2825_BIDIR_PIN_CTRL_REG_5		0xe6
+#define SSD2825_BIDIR_PIN_CTRL_REG_6		0xe7
+#define SSD2825_BIDIR_PIN_CTRL_REG_7		0xe8
+#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_1	0xe9
+#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_2	0xea
+#define SSD2825_CABC_BRIGHTNESS_STATUS_REG	0xeb
+#define SSD2825_READ_REG			0xff
+
+#define SSD2825_COM_BYTE			0x00
+#define SSD2825_DAT_BYTE			0x01
+
+#define	SSD2828_LP_CLOCK_DIVIDER(n)		(((n) - 1) & 0x3f)
+#define SSD2825_LP_MIN_CLK			5000 /* KHz */
+#define SSD2825_REF_MIN_CLK			2000 /* KHz */
+
+static const char * const ssd2825_supplies[] = {
+	"dvdd", "avdd", "vddio"
+};
+
+struct ssd2825_dsi_output {
+	struct mipi_dsi_device *dev;
+	struct drm_panel *panel;
+	struct drm_bridge *bridge;
+};
+
+struct ssd2825_priv {
+	struct spi_device *spi;
+	struct device *dev;
+
+	struct gpio_desc *reset_gpio;
+
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ssd2825_supplies)];
+
+	struct clk *tx_clk;
+
+	int enabled;
+
+	struct mipi_dsi_host dsi_host;
+	struct drm_bridge bridge;
+	struct ssd2825_dsi_output output;
+
+	u32 pd_lines;		/* number of Parallel Port Input Data Lines */
+	u32 dsi_lanes;		/* number of DSI Lanes */
+
+	/* Parameters for PLL programming */
+	u32 pll_freq_kbps;	/* PLL in kbps */
+	u32 nibble_freq_khz;	/* PLL div by 4 */
+
+	u32 hzd;		/* HS Zero Delay in ns*/
+	u32 hpd;		/* HS Prepare Delay is ns */
+};
+
+static inline struct ssd2825_priv *dsi_host_to_ssd2825(struct mipi_dsi_host
+							 *host)
+{
+	return container_of(host, struct ssd2825_priv, dsi_host);
+}
+
+static inline struct ssd2825_priv *bridge_to_ssd2825(struct drm_bridge
+						     *bridge)
+{
+	return container_of(bridge, struct ssd2825_priv, bridge);
+}
+
+static int ssd2825_write_raw(struct ssd2825_priv *priv, u8 high_byte, u8 low_byte)
+{
+	struct spi_device *spi = priv->spi;
+	u8 tx_buf[2];
+
+	/*
+	 * Low byte is the value, high byte defines type of
+	 * write cycle, 0 for command and 1 for data.
+	 */
+	tx_buf[0] = low_byte;
+	tx_buf[1] = high_byte;
+
+	return spi_write(spi, tx_buf, 2);
+}
+
+static int ssd2825_write_reg(struct ssd2825_priv *priv, u8 reg, u16 command)
+{
+	u8 datal = (command & 0x00FF);
+	u8 datah = (command & 0xFF00) >> 8;
+	int ret;
+
+	/* Command write cycle */
+	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
+	if (ret)
+		return ret;
+
+	/* Data write cycle bits 7-0 */
+	ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datal);
+	if (ret)
+		return ret;
+
+	/* Data write cycle bits 15-8 */
+	ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datah);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ssd2825_write_dsi(struct ssd2825_priv *priv, const u8 *command, int len)
+{
+	int ret, i;
+
+	ret = ssd2825_write_reg(priv, SSD2825_PACKET_SIZE_CTRL_REG_1, len);
+	if (ret)
+		return ret;
+
+	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, SSD2825_PACKET_DROP_REG);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < len; i++) {
+		ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, command[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ssd2825_read_raw(struct ssd2825_priv *priv, u8 cmd, u16 *data)
+{
+	struct spi_device *spi = priv->spi;
+	struct spi_message msg;
+	struct spi_transfer xfer[2];
+	u8 tx_buf[2];
+	u8 rx_buf[2];
+	int ret;
+
+	memset(&xfer, 0, sizeof(xfer));
+
+	tx_buf[1] = (cmd & 0xFF00) >> 8;
+	tx_buf[0] = (cmd & 0x00FF);
+
+	xfer[0].tx_buf = tx_buf;
+	xfer[0].bits_per_word = 9;
+	xfer[0].len = 2;
+
+	xfer[1].rx_buf = rx_buf;
+	xfer[1].bits_per_word = 16;
+	xfer[1].len = 2;
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer[0], &msg);
+	spi_message_add_tail(&xfer[1], &msg);
+
+	ret = spi_sync(spi, &msg);
+	if (ret)
+		dev_err(&spi->dev, "spi_sync_read failed %d\n", ret);
+
+	*data = rx_buf[1] | (rx_buf[0] << 8);
+
+	return 0;
+}
+
+static int ssd2825_read_reg(struct ssd2825_priv *priv, u8 reg, u16 *data)
+{
+	int ret;
+
+	/* Reset the read register */
+	ret = ssd2825_write_reg(priv, SSD2825_SPI_READ_REG, SSD2825_SPI_READ_REG_RESET);
+	if (ret)
+		return ret;
+
+	/* Push the address to read */
+	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
+	if (ret)
+		return ret;
+
+	/* Perform a reading cycle */
+	ret = ssd2825_read_raw(priv, SSD2825_SPI_READ_REG_RESET, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ssd2825_dsi_host_attach(struct mipi_dsi_host *host,
+				   struct mipi_dsi_device *dev)
+{
+	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	struct device_node *ep;
+	int ret;
+
+	if (dev->lanes > 4) {
+		dev_err(priv->dev, "unsupported number of data lanes(%u)\n",
+			dev->lanes);
+		return -EINVAL;
+	}
+
+	/*
+	 * ssd2825 supports both Video and Pulse mode, but the driver only
+	 * implements Video (event) mode currently
+	 */
+	if (!(dev->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+		dev_err(priv->dev, "Only MIPI_DSI_MODE_VIDEO is supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
+					  &bridge);
+	if (ret)
+		return ret;
+
+	if (panel) {
+		bridge = drm_panel_bridge_add_typed(panel,
+						    DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(bridge))
+			return PTR_ERR(bridge);
+	}
+
+	priv->output.dev = dev;
+	priv->output.bridge = bridge;
+	priv->output.panel = panel;
+
+	priv->dsi_lanes = dev->lanes;
+
+	/* get input ep (port0/endpoint0) */
+	ret = -EINVAL;
+	ep = of_graph_get_endpoint_by_regs(host->dev->of_node, 0, 0);
+	if (ep) {
+		ret = of_property_read_u32(ep, "bus-width", &priv->pd_lines);
+
+		of_node_put(ep);
+	}
+
+	if (ret)
+		priv->pd_lines = mipi_dsi_pixel_format_to_bpp(dev->format);
+
+	drm_bridge_add(&priv->bridge);
+
+	return 0;
+}
+
+static int ssd2825_dsi_host_detach(struct mipi_dsi_host *host,
+				   struct mipi_dsi_device *dev)
+{
+	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
+
+	drm_bridge_remove(&priv->bridge);
+	if (priv->output.panel)
+		drm_panel_bridge_remove(priv->output.bridge);
+
+	return 0;
+}
+
+static ssize_t ssd2825_dsi_host_transfer(struct mipi_dsi_host *host,
+					 const struct mipi_dsi_msg *msg)
+{
+	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
+	u8 buf = *(u8 *)msg->tx_buf;
+	u16 config;
+	int ret;
+
+	if (!priv->enabled) {
+		dev_err(priv->dev, "Bridge is not enabled\n");
+		return -ENODEV;
+	}
+
+	if (msg->rx_len) {
+		dev_warn(priv->dev, "MIPI rx is not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	ret = ssd2825_read_reg(priv, SSD2825_CONFIGURATION_REG, &config);
+	if (ret)
+		return ret;
+
+	switch (msg->type) {
+	case MIPI_DSI_DCS_SHORT_WRITE:
+	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
+	case MIPI_DSI_DCS_LONG_WRITE:
+		config |= SSD2825_CONF_REG_DCS;
+		break;
+	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
+	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
+	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
+	case MIPI_DSI_GENERIC_LONG_WRITE:
+		config &= ~SSD2825_CONF_REG_DCS;
+		break;
+	case MIPI_DSI_DCS_READ:
+	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
+	default:
+		return 0;
+	}
+
+	ret = ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG, config);
+	if (ret)
+		return ret;
+
+	ret = ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
+	if (ret)
+		return ret;
+
+	ret = ssd2825_write_dsi(priv, msg->tx_buf, msg->tx_len);
+	if (ret)
+		return ret;
+
+	if (buf == MIPI_DCS_SET_DISPLAY_ON) {
+		ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
+				  SSD2825_CONF_REG_HS | SSD2825_CONF_REG_VEN |
+				  SSD2825_CONF_REG_DCS | SSD2825_CONF_REG_ECD |
+				  SSD2825_CONF_REG_EOT);
+		ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
+		ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
+	}
+
+	return 0;
+}
+
+static const struct mipi_dsi_host_ops ssd2825_dsi_host_ops = {
+	.attach = ssd2825_dsi_host_attach,
+	.detach = ssd2825_dsi_host_detach,
+	.transfer = ssd2825_dsi_host_transfer,
+};
+
+static void ssd2825_hw_reset(struct ssd2825_priv *priv)
+{
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(priv->reset_gpio, 0);
+	usleep_range(5000, 6000);
+}
+
+/*
+ * PLL configuration register settings.
+ *
+ * See the "PLL Configuration Register Description" in the SSD2825 datasheet.
+ */
+static u16 construct_pll_config(struct ssd2825_priv *priv,
+				u32 desired_pll_freq_kbps, u32 reference_freq_khz)
+{
+	u32 div_factor = 1, mul_factor, fr = 0;
+
+	while (reference_freq_khz / (div_factor + 1) >= SSD2825_REF_MIN_CLK)
+		div_factor++;
+	if (div_factor > 31)
+		div_factor = 31;
+
+	mul_factor = DIV_ROUND_UP(desired_pll_freq_kbps * div_factor,
+				  reference_freq_khz);
+
+	priv->pll_freq_kbps = reference_freq_khz * mul_factor / div_factor;
+	priv->nibble_freq_khz = priv->pll_freq_kbps / 4;
+
+	if (priv->pll_freq_kbps >= 501000)
+		fr = 3;
+	else if (priv->pll_freq_kbps >= 251000)
+		fr = 2;
+	else if (priv->pll_freq_kbps >= 126000)
+		fr = 1;
+
+	return (fr << 14) | (div_factor << 8) | mul_factor;
+}
+
+static u32 ssd2825_to_ns(u32 khz)
+{
+	return (1000 * 1000 / khz);
+}
+
+static int ssd2825_setup_pll(struct ssd2825_priv *priv,
+			     const struct drm_display_mode *mode)
+{
+	u16 pll_config, lp_div;
+	u32 nibble_delay, pclk_mult, tx_freq_khz;
+	u8 hzd, hpd;
+
+	tx_freq_khz = clk_get_rate(priv->tx_clk) / 1000;
+	pclk_mult = priv->pd_lines / priv->dsi_lanes + 1;
+	pll_config = construct_pll_config(priv, pclk_mult * mode->clock,
+					  tx_freq_khz);
+
+	lp_div = priv->pll_freq_kbps / (SSD2825_LP_MIN_CLK * 8);
+
+	nibble_delay = ssd2825_to_ns(priv->nibble_freq_khz);
+
+	hzd = priv->hzd / nibble_delay;
+	hpd = (priv->hpd - 4 * nibble_delay) / nibble_delay;
+
+	/* Disable PLL */
+	ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0000);
+	ssd2825_write_reg(priv, SSD2825_LINE_CTRL_REG, 0x0001);
+
+	/* Set delays */
+	dev_dbg(priv->dev, "SSD2825_DELAY_ADJ_REG_1 0x%x\n", (hzd << 8) | hpd);
+	ssd2825_write_reg(priv, SSD2825_DELAY_ADJ_REG_1, (hzd << 8) | hpd);
+
+	/* Set PLL coeficients */
+	dev_dbg(priv->dev, "SSD2825_PLL_CONFIGURATION_REG 0x%x\n", pll_config);
+	ssd2825_write_reg(priv, SSD2825_PLL_CONFIGURATION_REG, pll_config);
+
+	/* Clock Control Register */
+	dev_dbg(priv->dev, "SSD2825_CLOCK_CTRL_REG 0x%x\n",
+		SSD2828_LP_CLOCK_DIVIDER(lp_div));
+	ssd2825_write_reg(priv, SSD2825_CLOCK_CTRL_REG,
+			  SSD2828_LP_CLOCK_DIVIDER(lp_div));
+
+	/* Enable PLL */
+	ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
+	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
+
+	return 0;
+}
+
+static void ssd2825_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
+	int ret;
+
+	if (priv->enabled)
+		return;
+
+	/* Power Sequence */
+	ret = clk_prepare_enable(priv->tx_clk);
+	if (ret < 0)
+		dev_err(priv->dev, "error enabling tx_clk (%d)\n", ret);
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), priv->supplies);
+	if (ret < 0)
+		dev_err(priv->dev, "error enabling regulators (%d)\n", ret);
+
+	usleep_range(1000, 2000);
+
+	ssd2825_hw_reset(priv);
+
+	priv->enabled = true;
+}
+
+static void ssd2825_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
+	struct mipi_dsi_device *dsi_dev = priv->output.dev;
+	unsigned long mode_flags = dsi_dev->mode_flags;
+	const struct drm_display_mode *mode =
+		&bridge->encoder->crtc->state->adjusted_mode;
+	struct device *dev = priv->dev;
+	u8 pixel_format;
+
+	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
+		dev_warn_once(dev,
+			      "Non-continuous mode unimplemented, falling back to continuous\n");
+		mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
+	}
+
+	if (mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+		dev_warn_once(dev,
+			      "Burst mode unimplemented, falling back to simple\n");
+		mode_flags &= ~MIPI_DSI_MODE_VIDEO_BURST;
+	}
+
+	/* Perform SW reset */
+	ssd2825_write_reg(priv, SSD2825_OPERATION_CTRL_REG, 0x0100);
+
+	switch (dsi_dev->format) {
+	case MIPI_DSI_FMT_RGB565:
+		pixel_format = 0x00;
+		break;
+	case MIPI_DSI_FMT_RGB666_PACKED:
+		pixel_format = 0x01;
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		pixel_format = 0x02;
+		break;
+	case MIPI_DSI_FMT_RGB888:
+	default:
+		pixel_format = 0x03;
+		break;
+	}
+
+	/* Set panel timings */
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_1,
+			  ((mode->vtotal - mode->vsync_end) << 8) |
+			  (mode->htotal - mode->hsync_end));
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_2,
+			  ((mode->vtotal - mode->vsync_start) << 8) |
+			  (mode->htotal - mode->hsync_start));
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_3,
+			  ((mode->vsync_start - mode->vdisplay) << 8) |
+			  (mode->hsync_start - mode->hdisplay));
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_4, mode->hdisplay);
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_5, mode->vdisplay);
+	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_6,
+			  SSD2825_HSYNC_HIGH | SSD2825_VSYNC_HIGH |
+			  SSD2825_PCKL_HIGH | SSD2825_NON_BURST |
+			  pixel_format);
+
+	ssd2825_write_reg(priv, SSD2825_LANE_CONFIGURATION_REG, dsi_dev->lanes - 1);
+	ssd2825_write_reg(priv, SSD2825_TEST_REG, 0x0004);
+
+	/* Call PLL configuration */
+	ssd2825_setup_pll(priv, mode);
+
+	usleep_range(10000, 11000);
+
+	/* Initial DSI configuration register set */
+	ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
+			  SSD2825_CONF_REG_CKE | SSD2825_CONF_REG_DCS |
+			  SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
+	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
+}
+
+static void ssd2825_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
+	int ret;
+
+	if (!priv->enabled)
+		return;
+
+	msleep(100);
+
+	/* Exit DSI configuration register set */
+	ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
+			  SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
+	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
+
+	/* HW disable */
+	gpiod_set_value_cansleep(priv->reset_gpio, 1);
+	usleep_range(5000, 6000);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(priv->supplies),
+				     priv->supplies);
+	if (ret < 0)
+		dev_err(priv->dev, "error disabling regulators (%d)\n", ret);
+
+	clk_disable_unprepare(priv->tx_clk);
+
+	priv->enabled = false;
+}
+
+static int ssd2825_bridge_attach(struct drm_bridge *bridge,
+				 enum drm_bridge_attach_flags flags)
+{
+	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
+
+	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
+				 flags);
+}
+
+static enum drm_mode_status
+ssd2825_bridge_mode_valid(struct drm_bridge *bridge,
+			  const struct drm_display_info *info,
+			  const struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static const struct drm_bridge_funcs ssd2825_bridge_funcs = {
+	.attach = ssd2825_bridge_attach,
+	.mode_valid = ssd2825_bridge_mode_valid,
+	.pre_enable = ssd2825_bridge_pre_enable,
+	.enable = ssd2825_bridge_enable,
+	.disable = ssd2825_bridge_disable,
+};
+
+static int ssd2825_probe(struct spi_device *spi)
+{
+	struct ssd2825_priv *priv;
+	struct device *dev = &spi->dev;
+	struct device_node *np = dev->of_node;
+	int i, ret;
+
+	/* Driver supports only this mode for now */
+	spi->bits_per_word = 9;
+
+	ret = spi_setup(spi);
+	if (ret)
+		return ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, priv);
+	priv->spi = spi;
+
+	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
+
+	priv->tx_clk = devm_clk_get_optional(dev, "tx_clk");
+	if (IS_ERR(priv->tx_clk))
+		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
+				     "can't retrieve bridge tx_clk\n");
+
+	priv->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
+				     "failed to get reset GPIO\n");
+
+	for (i = 0; i < ARRAY_SIZE(priv->supplies); ++i)
+		priv->supplies[i].supply = ssd2825_supplies[i];
+
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(priv->supplies),
+				      priv->supplies);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
+	ret = device_property_read_u32(dev, "solomon,hs-zero-delay-ns", &priv->hzd);
+	if (ret)
+		/* If no hs-zero-delay is set by device tree, use the default 133 ns */
+		priv->hzd = 133;
+
+	ret = device_property_read_u32(dev, "solomon,hs-prep-delay-ns", &priv->hpd);
+	if (ret)
+		/* If no hs-prep-delay is set by device tree, use the default 40 ns */
+		priv->hzd = 40;
+
+	priv->dsi_host.dev = dev;
+	priv->dsi_host.ops = &ssd2825_dsi_host_ops;
+
+	priv->bridge.funcs = &ssd2825_bridge_funcs;
+	priv->bridge.of_node = np;
+
+	return mipi_dsi_host_register(&priv->dsi_host);
+}
+
+static void ssd2825_remove(struct spi_device *spi)
+{
+	struct ssd2825_priv *priv = spi_get_drvdata(spi);
+
+	mipi_dsi_host_unregister(&priv->dsi_host);
+}
+
+static const struct of_device_id ssd2825_of_match[] = {
+	{ .compatible = "solomon,ssd2825" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ssd2825_of_match);
+
+static struct spi_driver ssd2825_driver = {
+	.driver = {
+		.name = "ssd2825",
+		.of_match_table = ssd2825_of_match,
+	},
+	.probe = ssd2825_probe,
+	.remove = ssd2825_remove,
+};
+module_spi_driver(ssd2825_driver);
+
+MODULE_DESCRIPTION("Solomon SSD2825 RGB to MIPI-DSI bridge driver SPI");
+MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
  2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
@ 2025-02-13 16:05   ` Dmitry Baryshkov
  2025-02-13 16:08     ` Svyatoslav Ryhel
  2025-02-15  3:48   ` kernel test robot
  2025-02-15  8:31   ` kernel test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Dmitry Baryshkov @ 2025-02-13 16:05 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

On Thu, Feb 13, 2025 at 03:56:05PM +0200, Svyatoslav Ryhel wrote:
> SSD2825 is an innovative and cost-effective MIPI Bridge Chip solution
> targeting high resolution smartphones. It can convert 24bit RGB
> interface into 4-lane MIPI-DSI interface to drive extremely high
> resolution display modules of up to 800 x 1366, while supporting AMOLED,
> a-si LCD or LTPS advanced panel technologies for smartphone applications.

Nit: I don't think that 800x1366 is considered high-res nowadays. Please
consider rewriting the text in a non-marketing way.

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig   |  13 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/ssd2825.c | 741 +++++++++++++++++++++++++++++++
>  3 files changed, 755 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ssd2825.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 6b4664d91faa..74aec32d0f3b 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -306,6 +306,19 @@ config DRM_SIMPLE_BRIDGE
>  	  Support for non-programmable DRM bridges, such as ADI ADV7123, TI
>  	  THS8134 and THS8135 or passive resistor ladder DACs.
>  
> +config DRM_SOLOMON_SSD2825
> +	tristate "SSD2825 RGB/DSI bridge"
> +	depends on OF
> +	select DRM_MIPI_DSI
> +	select DRM_KMS_HELPER
> +	select DRM_PANEL
> +	help
> +	  Say Y here if you want support for the Solomon SSD2825 RGB/DSI
> +	  SPI bridge driver.
> +
> +	  Say M here if you want to support this hardware as a module.
> +	  The module will be named "solomon-ssd2825".
> +
>  config DRM_THINE_THC63LVD1024
>  	tristate "Thine THC63LVD1024 LVDS decoder bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 97304b429a53..c621ab3fa3a9 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>  obj-$(CONFIG_DRM_SIMPLE_BRIDGE) += simple-bridge.o
> +obj-$(CONFIG_DRM_SOLOMON_SSD2825) += ssd2825.o
>  obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358762) += tc358762.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
> diff --git a/drivers/gpu/drm/bridge/ssd2825.c b/drivers/gpu/drm/bridge/ssd2825.c
> new file mode 100644
> index 000000000000..823f6dc023a5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ssd2825.c
> @@ -0,0 +1,741 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <video/mipi_display.h>
> +#include <video/videomode.h>
> +
> +#define SSD2825_DEVICE_ID_REG			0xb0
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_1	0xb1
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_2	0xb2
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_3	0xb3
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_4	0xb4
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_5	0xb5
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_6	0xb6
> +#define   SSD2825_NON_BURST			BIT(2)
> +#define   SSD2825_BURST				BIT(3)
> +#define   SSD2825_PCKL_HIGH			BIT(13)
> +#define   SSD2825_HSYNC_HIGH			BIT(14)
> +#define   SSD2825_VSYNC_HIGH			BIT(15)
> +#define SSD2825_CONFIGURATION_REG		0xb7
> +#define   SSD2825_CONF_REG_HS			BIT(0)
> +#define   SSD2825_CONF_REG_CKE			BIT(1)
> +#define   SSD2825_CONF_REG_SLP			BIT(2)
> +#define   SSD2825_CONF_REG_VEN			BIT(3)
> +#define   SSD2825_CONF_REG_HCLK			BIT(4)
> +#define   SSD2825_CONF_REG_CSS			BIT(5)
> +#define   SSD2825_CONF_REG_DCS			BIT(6)
> +#define   SSD2825_CONF_REG_REN			BIT(7)
> +#define   SSD2825_CONF_REG_ECD			BIT(8)
> +#define   SSD2825_CONF_REG_EOT			BIT(9)
> +#define   SSD2825_CONF_REG_LPE			BIT(10)
> +#define SSD2825_VC_CTRL_REG			0xb8
> +#define SSD2825_PLL_CTRL_REG			0xb9
> +#define SSD2825_PLL_CONFIGURATION_REG		0xba
> +#define SSD2825_CLOCK_CTRL_REG			0xbb
> +#define SSD2825_PACKET_SIZE_CTRL_REG_1		0xbc
> +#define SSD2825_PACKET_SIZE_CTRL_REG_2		0xbd
> +#define SSD2825_PACKET_SIZE_CTRL_REG_3		0xbe
> +#define SSD2825_PACKET_DROP_REG			0xbf
> +#define SSD2825_OPERATION_CTRL_REG		0xc0
> +#define SSD2825_MAX_RETURN_SIZE_REG		0xc1
> +#define SSD2825_RETURN_DATA_COUNT_REG		0xc2
> +#define SSD2825_ACK_RESPONSE_REG		0xc3
> +#define SSD2825_LINE_CTRL_REG			0xc4
> +#define SSD2825_INTERRUPT_CTRL_REG		0xc5
> +#define SSD2825_INTERRUPT_STATUS_REG		0xc6
> +#define SSD2825_ERROR_STATUS_REG		0xc7
> +#define SSD2825_DATA_FORMAT_REG			0xc8
> +#define SSD2825_DELAY_ADJ_REG_1			0xc9
> +#define SSD2825_DELAY_ADJ_REG_2			0xca
> +#define SSD2825_DELAY_ADJ_REG_3			0xcb
> +#define SSD2825_DELAY_ADJ_REG_4			0xcc
> +#define SSD2825_DELAY_ADJ_REG_5			0xcd
> +#define SSD2825_DELAY_ADJ_REG_6			0xce
> +#define SSD2825_HS_TX_TIMER_REG_1		0xcf
> +#define SSD2825_HS_TX_TIMER_REG_2		0xd0
> +#define SSD2825_LP_RX_TIMER_REG_1		0xd1
> +#define SSD2825_LP_RX_TIMER_REG_2		0xd2
> +#define SSD2825_TE_STATUS_REG			0xd3
> +#define SSD2825_SPI_READ_REG			0xd4
> +#define   SSD2825_SPI_READ_REG_RESET		0xfa
> +#define SSD2825_PLL_LOCK_REG			0xd5
> +#define SSD2825_TEST_REG			0xd6
> +#define SSD2825_TE_COUNT_REG			0xd7
> +#define SSD2825_ANALOG_CTRL_REG_1		0xd8
> +#define SSD2825_ANALOG_CTRL_REG_2		0xd9
> +#define SSD2825_ANALOG_CTRL_REG_3		0xda
> +#define SSD2825_ANALOG_CTRL_REG_4		0xdb
> +#define SSD2825_INTERRUPT_OUT_CTRL_REG		0xdc
> +#define SSD2825_RGB_INTERFACE_CTRL_REG_7	0xdd
> +#define SSD2825_LANE_CONFIGURATION_REG		0xde
> +#define SSD2825_DELAY_ADJ_REG_7			0xdf
> +#define SSD2825_INPUT_PIN_CTRL_REG_1		0xe0
> +#define SSD2825_INPUT_PIN_CTRL_REG_2		0xe1
> +#define SSD2825_BIDIR_PIN_CTRL_REG_1		0xe2
> +#define SSD2825_BIDIR_PIN_CTRL_REG_2		0xe3
> +#define SSD2825_BIDIR_PIN_CTRL_REG_3		0xe4
> +#define SSD2825_BIDIR_PIN_CTRL_REG_4		0xe5
> +#define SSD2825_BIDIR_PIN_CTRL_REG_5		0xe6
> +#define SSD2825_BIDIR_PIN_CTRL_REG_6		0xe7
> +#define SSD2825_BIDIR_PIN_CTRL_REG_7		0xe8
> +#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_1	0xe9
> +#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_2	0xea
> +#define SSD2825_CABC_BRIGHTNESS_STATUS_REG	0xeb
> +#define SSD2825_READ_REG			0xff
> +
> +#define SSD2825_COM_BYTE			0x00
> +#define SSD2825_DAT_BYTE			0x01
> +
> +#define	SSD2828_LP_CLOCK_DIVIDER(n)		(((n) - 1) & 0x3f)
> +#define SSD2825_LP_MIN_CLK			5000 /* KHz */
> +#define SSD2825_REF_MIN_CLK			2000 /* KHz */
> +
> +static const char * const ssd2825_supplies[] = {
> +	"dvdd", "avdd", "vddio"
> +};
> +
> +struct ssd2825_dsi_output {
> +	struct mipi_dsi_device *dev;
> +	struct drm_panel *panel;
> +	struct drm_bridge *bridge;
> +};
> +
> +struct ssd2825_priv {
> +	struct spi_device *spi;
> +	struct device *dev;
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(ssd2825_supplies)];
> +
> +	struct clk *tx_clk;
> +
> +	int enabled;
> +
> +	struct mipi_dsi_host dsi_host;
> +	struct drm_bridge bridge;
> +	struct ssd2825_dsi_output output;
> +
> +	u32 pd_lines;		/* number of Parallel Port Input Data Lines */
> +	u32 dsi_lanes;		/* number of DSI Lanes */
> +
> +	/* Parameters for PLL programming */
> +	u32 pll_freq_kbps;	/* PLL in kbps */
> +	u32 nibble_freq_khz;	/* PLL div by 4 */
> +
> +	u32 hzd;		/* HS Zero Delay in ns*/
> +	u32 hpd;		/* HS Prepare Delay is ns */
> +};
> +
> +static inline struct ssd2825_priv *dsi_host_to_ssd2825(struct mipi_dsi_host
> +							 *host)
> +{
> +	return container_of(host, struct ssd2825_priv, dsi_host);
> +}
> +
> +static inline struct ssd2825_priv *bridge_to_ssd2825(struct drm_bridge
> +						     *bridge)
> +{
> +	return container_of(bridge, struct ssd2825_priv, bridge);
> +}
> +
> +static int ssd2825_write_raw(struct ssd2825_priv *priv, u8 high_byte, u8 low_byte)
> +{
> +	struct spi_device *spi = priv->spi;
> +	u8 tx_buf[2];
> +
> +	/*
> +	 * Low byte is the value, high byte defines type of
> +	 * write cycle, 0 for command and 1 for data.
> +	 */
> +	tx_buf[0] = low_byte;
> +	tx_buf[1] = high_byte;
> +
> +	return spi_write(spi, tx_buf, 2);
> +}
> +
> +static int ssd2825_write_reg(struct ssd2825_priv *priv, u8 reg, u16 command)
> +{
> +	u8 datal = (command & 0x00FF);
> +	u8 datah = (command & 0xFF00) >> 8;
> +	int ret;
> +
> +	/* Command write cycle */
> +	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
> +	if (ret)
> +		return ret;
> +
> +	/* Data write cycle bits 7-0 */
> +	ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datal);
> +	if (ret)
> +		return ret;
> +
> +	/* Data write cycle bits 15-8 */
> +	ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datah);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ssd2825_write_dsi(struct ssd2825_priv *priv, const u8 *command, int len)
> +{
> +	int ret, i;
> +
> +	ret = ssd2825_write_reg(priv, SSD2825_PACKET_SIZE_CTRL_REG_1, len);
> +	if (ret)
> +		return ret;
> +
> +	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, SSD2825_PACKET_DROP_REG);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < len; i++) {
> +		ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, command[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ssd2825_read_raw(struct ssd2825_priv *priv, u8 cmd, u16 *data)
> +{
> +	struct spi_device *spi = priv->spi;
> +	struct spi_message msg;
> +	struct spi_transfer xfer[2];
> +	u8 tx_buf[2];
> +	u8 rx_buf[2];
> +	int ret;
> +
> +	memset(&xfer, 0, sizeof(xfer));
> +
> +	tx_buf[1] = (cmd & 0xFF00) >> 8;
> +	tx_buf[0] = (cmd & 0x00FF);
> +
> +	xfer[0].tx_buf = tx_buf;
> +	xfer[0].bits_per_word = 9;
> +	xfer[0].len = 2;
> +
> +	xfer[1].rx_buf = rx_buf;
> +	xfer[1].bits_per_word = 16;
> +	xfer[1].len = 2;
> +
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer[0], &msg);
> +	spi_message_add_tail(&xfer[1], &msg);
> +
> +	ret = spi_sync(spi, &msg);
> +	if (ret)
> +		dev_err(&spi->dev, "spi_sync_read failed %d\n", ret);
> +
> +	*data = rx_buf[1] | (rx_buf[0] << 8);
> +
> +	return 0;
> +}
> +
> +static int ssd2825_read_reg(struct ssd2825_priv *priv, u8 reg, u16 *data)
> +{
> +	int ret;
> +
> +	/* Reset the read register */
> +	ret = ssd2825_write_reg(priv, SSD2825_SPI_READ_REG, SSD2825_SPI_READ_REG_RESET);
> +	if (ret)
> +		return ret;
> +
> +	/* Push the address to read */
> +	ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
> +	if (ret)
> +		return ret;
> +
> +	/* Perform a reading cycle */
> +	ret = ssd2825_read_raw(priv, SSD2825_SPI_READ_REG_RESET, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ssd2825_dsi_host_attach(struct mipi_dsi_host *host,
> +				   struct mipi_dsi_device *dev)
> +{
> +	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	struct device_node *ep;
> +	int ret;
> +
> +	if (dev->lanes > 4) {
> +		dev_err(priv->dev, "unsupported number of data lanes(%u)\n",
> +			dev->lanes);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * ssd2825 supports both Video and Pulse mode, but the driver only
> +	 * implements Video (event) mode currently
> +	 */
> +	if (!(dev->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> +		dev_err(priv->dev, "Only MIPI_DSI_MODE_VIDEO is supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> +					  &bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		bridge = drm_panel_bridge_add_typed(panel,
> +						    DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(bridge))
> +			return PTR_ERR(bridge);
> +	}
> +
> +	priv->output.dev = dev;
> +	priv->output.bridge = bridge;
> +	priv->output.panel = panel;
> +
> +	priv->dsi_lanes = dev->lanes;
> +
> +	/* get input ep (port0/endpoint0) */
> +	ret = -EINVAL;
> +	ep = of_graph_get_endpoint_by_regs(host->dev->of_node, 0, 0);
> +	if (ep) {
> +		ret = of_property_read_u32(ep, "bus-width", &priv->pd_lines);
> +
> +		of_node_put(ep);
> +	}
> +
> +	if (ret)
> +		priv->pd_lines = mipi_dsi_pixel_format_to_bpp(dev->format);
> +
> +	drm_bridge_add(&priv->bridge);
> +
> +	return 0;
> +}
> +
> +static int ssd2825_dsi_host_detach(struct mipi_dsi_host *host,
> +				   struct mipi_dsi_device *dev)
> +{
> +	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> +
> +	drm_bridge_remove(&priv->bridge);
> +	if (priv->output.panel)
> +		drm_panel_bridge_remove(priv->output.bridge);
> +
> +	return 0;
> +}
> +
> +static ssize_t ssd2825_dsi_host_transfer(struct mipi_dsi_host *host,
> +					 const struct mipi_dsi_msg *msg)
> +{
> +	struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> +	u8 buf = *(u8 *)msg->tx_buf;
> +	u16 config;
> +	int ret;
> +
> +	if (!priv->enabled) {
> +		dev_err(priv->dev, "Bridge is not enabled\n");
> +		return -ENODEV;
> +	}

No. See include/drm/drm_mipi_dsi.h:

 * Also note that those callbacks can be called no matter the state the
 * host is in. Drivers that need the underlying device to be powered to
 * perform these operations will first need to make sure it's been
 * properly enabled.


> +
> +	if (msg->rx_len) {
> +		dev_warn(priv->dev, "MIPI rx is not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = ssd2825_read_reg(priv, SSD2825_CONFIGURATION_REG, &config);
> +	if (ret)
> +		return ret;
> +
> +	switch (msg->type) {
> +	case MIPI_DSI_DCS_SHORT_WRITE:
> +	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> +	case MIPI_DSI_DCS_LONG_WRITE:
> +		config |= SSD2825_CONF_REG_DCS;
> +		break;
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> +	case MIPI_DSI_GENERIC_LONG_WRITE:
> +		config &= ~SSD2825_CONF_REG_DCS;
> +		break;
> +	case MIPI_DSI_DCS_READ:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> +	default:
> +		return 0;
> +	}
> +
> +	ret = ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG, config);
> +	if (ret)
> +		return ret;
> +
> +	ret = ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
> +	if (ret)
> +		return ret;
> +
> +	ret = ssd2825_write_dsi(priv, msg->tx_buf, msg->tx_len);
> +	if (ret)
> +		return ret;

Do you need any kind of locking in case there are two simultaneous
transfer operations?

> +
> +	if (buf == MIPI_DCS_SET_DISPLAY_ON) {
> +		ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> +				  SSD2825_CONF_REG_HS | SSD2825_CONF_REG_VEN |
> +				  SSD2825_CONF_REG_DCS | SSD2825_CONF_REG_ECD |
> +				  SSD2825_CONF_REG_EOT);

Most of these flags should be set depending on the
mipi_dsi_device.mode_flags.

> +		ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
> +		ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);

It looks like this should be a part of the .atomic_enable() callback.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops ssd2825_dsi_host_ops = {
> +	.attach = ssd2825_dsi_host_attach,
> +	.detach = ssd2825_dsi_host_detach,
> +	.transfer = ssd2825_dsi_host_transfer,
> +};
> +
> +static void ssd2825_hw_reset(struct ssd2825_priv *priv)
> +{
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	usleep_range(5000, 6000);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +	usleep_range(5000, 6000);
> +}
> +
> +/*
> + * PLL configuration register settings.
> + *
> + * See the "PLL Configuration Register Description" in the SSD2825 datasheet.
> + */
> +static u16 construct_pll_config(struct ssd2825_priv *priv,
> +				u32 desired_pll_freq_kbps, u32 reference_freq_khz)
> +{
> +	u32 div_factor = 1, mul_factor, fr = 0;
> +
> +	while (reference_freq_khz / (div_factor + 1) >= SSD2825_REF_MIN_CLK)
> +		div_factor++;
> +	if (div_factor > 31)
> +		div_factor = 31;
> +
> +	mul_factor = DIV_ROUND_UP(desired_pll_freq_kbps * div_factor,
> +				  reference_freq_khz);
> +
> +	priv->pll_freq_kbps = reference_freq_khz * mul_factor / div_factor;
> +	priv->nibble_freq_khz = priv->pll_freq_kbps / 4;
> +
> +	if (priv->pll_freq_kbps >= 501000)
> +		fr = 3;
> +	else if (priv->pll_freq_kbps >= 251000)
> +		fr = 2;
> +	else if (priv->pll_freq_kbps >= 126000)
> +		fr = 1;
> +
> +	return (fr << 14) | (div_factor << 8) | mul_factor;
> +}
> +
> +static u32 ssd2825_to_ns(u32 khz)
> +{
> +	return (1000 * 1000 / khz);
> +}
> +
> +static int ssd2825_setup_pll(struct ssd2825_priv *priv,
> +			     const struct drm_display_mode *mode)
> +{
> +	u16 pll_config, lp_div;
> +	u32 nibble_delay, pclk_mult, tx_freq_khz;
> +	u8 hzd, hpd;
> +
> +	tx_freq_khz = clk_get_rate(priv->tx_clk) / 1000;
> +	pclk_mult = priv->pd_lines / priv->dsi_lanes + 1;
> +	pll_config = construct_pll_config(priv, pclk_mult * mode->clock,
> +					  tx_freq_khz);
> +
> +	lp_div = priv->pll_freq_kbps / (SSD2825_LP_MIN_CLK * 8);
> +
> +	nibble_delay = ssd2825_to_ns(priv->nibble_freq_khz);
> +
> +	hzd = priv->hzd / nibble_delay;
> +	hpd = (priv->hpd - 4 * nibble_delay) / nibble_delay;
> +
> +	/* Disable PLL */
> +	ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0000);
> +	ssd2825_write_reg(priv, SSD2825_LINE_CTRL_REG, 0x0001);
> +
> +	/* Set delays */
> +	dev_dbg(priv->dev, "SSD2825_DELAY_ADJ_REG_1 0x%x\n", (hzd << 8) | hpd);

drm_dbg_driver() ?

> +	ssd2825_write_reg(priv, SSD2825_DELAY_ADJ_REG_1, (hzd << 8) | hpd);
> +
> +	/* Set PLL coeficients */
> +	dev_dbg(priv->dev, "SSD2825_PLL_CONFIGURATION_REG 0x%x\n", pll_config);
> +	ssd2825_write_reg(priv, SSD2825_PLL_CONFIGURATION_REG, pll_config);
> +
> +	/* Clock Control Register */
> +	dev_dbg(priv->dev, "SSD2825_CLOCK_CTRL_REG 0x%x\n",
> +		SSD2828_LP_CLOCK_DIVIDER(lp_div));
> +	ssd2825_write_reg(priv, SSD2825_CLOCK_CTRL_REG,
> +			  SSD2828_LP_CLOCK_DIVIDER(lp_div));
> +
> +	/* Enable PLL */
> +	ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
> +	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> +
> +	return 0;
> +}
> +
> +static void ssd2825_bridge_pre_enable(struct drm_bridge *bridge)

Is there a reason not to use the .atomic versions?

> +{
> +	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> +	int ret;
> +
> +	if (priv->enabled)
> +		return;
> +
> +	/* Power Sequence */
> +	ret = clk_prepare_enable(priv->tx_clk);
> +	if (ret < 0)
> +		dev_err(priv->dev, "error enabling tx_clk (%d)\n", ret);
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), priv->supplies);
> +	if (ret < 0)
> +		dev_err(priv->dev, "error enabling regulators (%d)\n", ret);
> +
> +	usleep_range(1000, 2000);
> +
> +	ssd2825_hw_reset(priv);
> +
> +	priv->enabled = true;
> +}
> +
> +static void ssd2825_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> +	struct mipi_dsi_device *dsi_dev = priv->output.dev;
> +	unsigned long mode_flags = dsi_dev->mode_flags;
> +	const struct drm_display_mode *mode =
> +		&bridge->encoder->crtc->state->adjusted_mode;

If you'd used atomic callbacks, you could have gone through the state to
get corresponding data. Your main driver can be atomic, in which case
you should not be using encoder->crtc and crtc->state.

> +	struct device *dev = priv->dev;
> +	u8 pixel_format;
> +
> +	if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> +		dev_warn_once(dev,
> +			      "Non-continuous mode unimplemented, falling back to continuous\n");
> +		mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +	}
> +
> +	if (mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +		dev_warn_once(dev,
> +			      "Burst mode unimplemented, falling back to simple\n");
> +		mode_flags &= ~MIPI_DSI_MODE_VIDEO_BURST;

This should be declined earlier, e.g. in the dsi_attach.

> +	}
> +
> +	/* Perform SW reset */
> +	ssd2825_write_reg(priv, SSD2825_OPERATION_CTRL_REG, 0x0100);
> +
> +	switch (dsi_dev->format) {
> +	case MIPI_DSI_FMT_RGB565:
> +		pixel_format = 0x00;
> +		break;
> +	case MIPI_DSI_FMT_RGB666_PACKED:
> +		pixel_format = 0x01;
> +		break;
> +	case MIPI_DSI_FMT_RGB666:
> +		pixel_format = 0x02;
> +		break;
> +	case MIPI_DSI_FMT_RGB888:
> +	default:
> +		pixel_format = 0x03;
> +		break;
> +	}
> +
> +	/* Set panel timings */
> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_1,
> +			  ((mode->vtotal - mode->vsync_end) << 8) |
> +			  (mode->htotal - mode->hsync_end));
> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_2,
> +			  ((mode->vtotal - mode->vsync_start) << 8) |
> +			  (mode->htotal - mode->hsync_start));
> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_3,
> +			  ((mode->vsync_start - mode->vdisplay) << 8) |
> +			  (mode->hsync_start - mode->hdisplay));
> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_4, mode->hdisplay);
> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_5, mode->vdisplay);

Please check that the mode fits into corresponding bit fields in
.mode_valid().

> +	ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_6,
> +			  SSD2825_HSYNC_HIGH | SSD2825_VSYNC_HIGH |
> +			  SSD2825_PCKL_HIGH | SSD2825_NON_BURST |

These flags should come from drm_display_mode

> +			  pixel_format);
> +
> +	ssd2825_write_reg(priv, SSD2825_LANE_CONFIGURATION_REG, dsi_dev->lanes - 1);
> +	ssd2825_write_reg(priv, SSD2825_TEST_REG, 0x0004);
> +
> +	/* Call PLL configuration */
> +	ssd2825_setup_pll(priv, mode);
> +
> +	usleep_range(10000, 11000);
> +
> +	/* Initial DSI configuration register set */
> +	ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> +			  SSD2825_CONF_REG_CKE | SSD2825_CONF_REG_DCS |
> +			  SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
> +	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> +}
> +
> +static void ssd2825_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> +	int ret;
> +
> +	if (!priv->enabled)
> +		return;
> +
> +	msleep(100);
> +
> +	/* Exit DSI configuration register set */
> +	ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> +			  SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
> +	ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> +
> +	/* HW disable */
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	usleep_range(5000, 6000);
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(priv->supplies),
> +				     priv->supplies);
> +	if (ret < 0)
> +		dev_err(priv->dev, "error disabling regulators (%d)\n", ret);
> +
> +	clk_disable_unprepare(priv->tx_clk);
> +
> +	priv->enabled = false;
> +}
> +
> +static int ssd2825_bridge_attach(struct drm_bridge *bridge,
> +				 enum drm_bridge_attach_flags flags)
> +{
> +	struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> +				 flags);
> +}
> +
> +static enum drm_mode_status
> +ssd2825_bridge_mode_valid(struct drm_bridge *bridge,
> +			  const struct drm_display_info *info,
> +			  const struct drm_display_mode *mode)
> +{

No resulution / clock limitations?

> +	return MODE_OK;
> +}
> +
> +static const struct drm_bridge_funcs ssd2825_bridge_funcs = {
> +	.attach = ssd2825_bridge_attach,
> +	.mode_valid = ssd2825_bridge_mode_valid,
> +	.pre_enable = ssd2825_bridge_pre_enable,
> +	.enable = ssd2825_bridge_enable,
> +	.disable = ssd2825_bridge_disable,
> +};
> +
> +static int ssd2825_probe(struct spi_device *spi)
> +{
> +	struct ssd2825_priv *priv;
> +	struct device *dev = &spi->dev;
> +	struct device_node *np = dev->of_node;
> +	int i, ret;
> +
> +	/* Driver supports only this mode for now */
> +	spi->bits_per_word = 9;
> +
> +	ret = spi_setup(spi);
> +	if (ret)
> +		return ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, priv);
> +	priv->spi = spi;
> +
> +	dev_set_drvdata(dev, priv);
> +	priv->dev = dev;
> +
> +	priv->tx_clk = devm_clk_get_optional(dev, "tx_clk");
> +	if (IS_ERR(priv->tx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
> +				     "can't retrieve bridge tx_clk\n");
> +
> +	priv->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
> +				     "failed to get reset GPIO\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->supplies); ++i)
> +		priv->supplies[i].supply = ssd2825_supplies[i];

You can use devm_regulator_bulk_get_const() instead.

> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(priv->supplies),
> +				      priv->supplies);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
> +	ret = device_property_read_u32(dev, "solomon,hs-zero-delay-ns", &priv->hzd);
> +	if (ret)
> +		/* If no hs-zero-delay is set by device tree, use the default 133 ns */
> +		priv->hzd = 133;
> +
> +	ret = device_property_read_u32(dev, "solomon,hs-prep-delay-ns", &priv->hpd);
> +	if (ret)
> +		/* If no hs-prep-delay is set by device tree, use the default 40 ns */
> +		priv->hzd = 40;
> +
> +	priv->dsi_host.dev = dev;
> +	priv->dsi_host.ops = &ssd2825_dsi_host_ops;
> +
> +	priv->bridge.funcs = &ssd2825_bridge_funcs;
> +	priv->bridge.of_node = np;
> +
> +	return mipi_dsi_host_register(&priv->dsi_host);
> +}
> +
> +static void ssd2825_remove(struct spi_device *spi)
> +{
> +	struct ssd2825_priv *priv = spi_get_drvdata(spi);
> +
> +	mipi_dsi_host_unregister(&priv->dsi_host);
> +}
> +
> +static const struct of_device_id ssd2825_of_match[] = {
> +	{ .compatible = "solomon,ssd2825" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ssd2825_of_match);
> +
> +static struct spi_driver ssd2825_driver = {
> +	.driver = {
> +		.name = "ssd2825",
> +		.of_match_table = ssd2825_of_match,
> +	},
> +	.probe = ssd2825_probe,
> +	.remove = ssd2825_remove,
> +};
> +module_spi_driver(ssd2825_driver);
> +
> +MODULE_DESCRIPTION("Solomon SSD2825 RGB to MIPI-DSI bridge driver SPI");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
  2025-02-13 16:05   ` Dmitry Baryshkov
@ 2025-02-13 16:08     ` Svyatoslav Ryhel
  0 siblings, 0 replies; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-13 16:08 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

чт, 13 лют. 2025 р. о 18:05 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> пише:
>
> On Thu, Feb 13, 2025 at 03:56:05PM +0200, Svyatoslav Ryhel wrote:
> > SSD2825 is an innovative and cost-effective MIPI Bridge Chip solution
> > targeting high resolution smartphones. It can convert 24bit RGB
> > interface into 4-lane MIPI-DSI interface to drive extremely high
> > resolution display modules of up to 800 x 1366, while supporting AMOLED,
> > a-si LCD or LTPS advanced panel technologies for smartphone applications.
>
> Nit: I don't think that 800x1366 is considered high-res nowadays. Please
> consider rewriting the text in a non-marketing way.
>
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig   |  13 +
> >  drivers/gpu/drm/bridge/Makefile  |   1 +
> >  drivers/gpu/drm/bridge/ssd2825.c | 741 +++++++++++++++++++++++++++++++
> >  3 files changed, 755 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/ssd2825.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 6b4664d91faa..74aec32d0f3b 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -306,6 +306,19 @@ config DRM_SIMPLE_BRIDGE
> >         Support for non-programmable DRM bridges, such as ADI ADV7123, TI
> >         THS8134 and THS8135 or passive resistor ladder DACs.
> >
> > +config DRM_SOLOMON_SSD2825
> > +     tristate "SSD2825 RGB/DSI bridge"
> > +     depends on OF
> > +     select DRM_MIPI_DSI
> > +     select DRM_KMS_HELPER
> > +     select DRM_PANEL
> > +     help
> > +       Say Y here if you want support for the Solomon SSD2825 RGB/DSI
> > +       SPI bridge driver.
> > +
> > +       Say M here if you want to support this hardware as a module.
> > +       The module will be named "solomon-ssd2825".
> > +
> >  config DRM_THINE_THC63LVD1024
> >       tristate "Thine THC63LVD1024 LVDS decoder bridge"
> >       depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 97304b429a53..c621ab3fa3a9 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> >  obj-$(CONFIG_DRM_SII902X) += sii902x.o
> >  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> >  obj-$(CONFIG_DRM_SIMPLE_BRIDGE) += simple-bridge.o
> > +obj-$(CONFIG_DRM_SOLOMON_SSD2825) += ssd2825.o
> >  obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> >  obj-$(CONFIG_DRM_TOSHIBA_TC358762) += tc358762.o
> >  obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o
> > diff --git a/drivers/gpu/drm/bridge/ssd2825.c b/drivers/gpu/drm/bridge/ssd2825.c
> > new file mode 100644
> > index 000000000000..823f6dc023a5
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/ssd2825.c
> > @@ -0,0 +1,741 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/of.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <video/mipi_display.h>
> > +#include <video/videomode.h>
> > +
> > +#define SSD2825_DEVICE_ID_REG                        0xb0
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_1     0xb1
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_2     0xb2
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_3     0xb3
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_4     0xb4
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_5     0xb5
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_6     0xb6
> > +#define   SSD2825_NON_BURST                  BIT(2)
> > +#define   SSD2825_BURST                              BIT(3)
> > +#define   SSD2825_PCKL_HIGH                  BIT(13)
> > +#define   SSD2825_HSYNC_HIGH                 BIT(14)
> > +#define   SSD2825_VSYNC_HIGH                 BIT(15)
> > +#define SSD2825_CONFIGURATION_REG            0xb7
> > +#define   SSD2825_CONF_REG_HS                        BIT(0)
> > +#define   SSD2825_CONF_REG_CKE                       BIT(1)
> > +#define   SSD2825_CONF_REG_SLP                       BIT(2)
> > +#define   SSD2825_CONF_REG_VEN                       BIT(3)
> > +#define   SSD2825_CONF_REG_HCLK                      BIT(4)
> > +#define   SSD2825_CONF_REG_CSS                       BIT(5)
> > +#define   SSD2825_CONF_REG_DCS                       BIT(6)
> > +#define   SSD2825_CONF_REG_REN                       BIT(7)
> > +#define   SSD2825_CONF_REG_ECD                       BIT(8)
> > +#define   SSD2825_CONF_REG_EOT                       BIT(9)
> > +#define   SSD2825_CONF_REG_LPE                       BIT(10)
> > +#define SSD2825_VC_CTRL_REG                  0xb8
> > +#define SSD2825_PLL_CTRL_REG                 0xb9
> > +#define SSD2825_PLL_CONFIGURATION_REG                0xba
> > +#define SSD2825_CLOCK_CTRL_REG                       0xbb
> > +#define SSD2825_PACKET_SIZE_CTRL_REG_1               0xbc
> > +#define SSD2825_PACKET_SIZE_CTRL_REG_2               0xbd
> > +#define SSD2825_PACKET_SIZE_CTRL_REG_3               0xbe
> > +#define SSD2825_PACKET_DROP_REG                      0xbf
> > +#define SSD2825_OPERATION_CTRL_REG           0xc0
> > +#define SSD2825_MAX_RETURN_SIZE_REG          0xc1
> > +#define SSD2825_RETURN_DATA_COUNT_REG                0xc2
> > +#define SSD2825_ACK_RESPONSE_REG             0xc3
> > +#define SSD2825_LINE_CTRL_REG                        0xc4
> > +#define SSD2825_INTERRUPT_CTRL_REG           0xc5
> > +#define SSD2825_INTERRUPT_STATUS_REG         0xc6
> > +#define SSD2825_ERROR_STATUS_REG             0xc7
> > +#define SSD2825_DATA_FORMAT_REG                      0xc8
> > +#define SSD2825_DELAY_ADJ_REG_1                      0xc9
> > +#define SSD2825_DELAY_ADJ_REG_2                      0xca
> > +#define SSD2825_DELAY_ADJ_REG_3                      0xcb
> > +#define SSD2825_DELAY_ADJ_REG_4                      0xcc
> > +#define SSD2825_DELAY_ADJ_REG_5                      0xcd
> > +#define SSD2825_DELAY_ADJ_REG_6                      0xce
> > +#define SSD2825_HS_TX_TIMER_REG_1            0xcf
> > +#define SSD2825_HS_TX_TIMER_REG_2            0xd0
> > +#define SSD2825_LP_RX_TIMER_REG_1            0xd1
> > +#define SSD2825_LP_RX_TIMER_REG_2            0xd2
> > +#define SSD2825_TE_STATUS_REG                        0xd3
> > +#define SSD2825_SPI_READ_REG                 0xd4
> > +#define   SSD2825_SPI_READ_REG_RESET         0xfa
> > +#define SSD2825_PLL_LOCK_REG                 0xd5
> > +#define SSD2825_TEST_REG                     0xd6
> > +#define SSD2825_TE_COUNT_REG                 0xd7
> > +#define SSD2825_ANALOG_CTRL_REG_1            0xd8
> > +#define SSD2825_ANALOG_CTRL_REG_2            0xd9
> > +#define SSD2825_ANALOG_CTRL_REG_3            0xda
> > +#define SSD2825_ANALOG_CTRL_REG_4            0xdb
> > +#define SSD2825_INTERRUPT_OUT_CTRL_REG               0xdc
> > +#define SSD2825_RGB_INTERFACE_CTRL_REG_7     0xdd
> > +#define SSD2825_LANE_CONFIGURATION_REG               0xde
> > +#define SSD2825_DELAY_ADJ_REG_7                      0xdf
> > +#define SSD2825_INPUT_PIN_CTRL_REG_1         0xe0
> > +#define SSD2825_INPUT_PIN_CTRL_REG_2         0xe1
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_1         0xe2
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_2         0xe3
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_3         0xe4
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_4         0xe5
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_5         0xe6
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_6         0xe7
> > +#define SSD2825_BIDIR_PIN_CTRL_REG_7         0xe8
> > +#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_1   0xe9
> > +#define SSD2825_CABC_BRIGHTNESS_CTRL_REG_2   0xea
> > +#define SSD2825_CABC_BRIGHTNESS_STATUS_REG   0xeb
> > +#define SSD2825_READ_REG                     0xff
> > +
> > +#define SSD2825_COM_BYTE                     0x00
> > +#define SSD2825_DAT_BYTE                     0x01
> > +
> > +#define      SSD2828_LP_CLOCK_DIVIDER(n)             (((n) - 1) & 0x3f)
> > +#define SSD2825_LP_MIN_CLK                   5000 /* KHz */
> > +#define SSD2825_REF_MIN_CLK                  2000 /* KHz */
> > +
> > +static const char * const ssd2825_supplies[] = {
> > +     "dvdd", "avdd", "vddio"
> > +};
> > +
> > +struct ssd2825_dsi_output {
> > +     struct mipi_dsi_device *dev;
> > +     struct drm_panel *panel;
> > +     struct drm_bridge *bridge;
> > +};
> > +
> > +struct ssd2825_priv {
> > +     struct spi_device *spi;
> > +     struct device *dev;
> > +
> > +     struct gpio_desc *reset_gpio;
> > +
> > +     struct regulator_bulk_data supplies[ARRAY_SIZE(ssd2825_supplies)];
> > +
> > +     struct clk *tx_clk;
> > +
> > +     int enabled;
> > +
> > +     struct mipi_dsi_host dsi_host;
> > +     struct drm_bridge bridge;
> > +     struct ssd2825_dsi_output output;
> > +
> > +     u32 pd_lines;           /* number of Parallel Port Input Data Lines */
> > +     u32 dsi_lanes;          /* number of DSI Lanes */
> > +
> > +     /* Parameters for PLL programming */
> > +     u32 pll_freq_kbps;      /* PLL in kbps */
> > +     u32 nibble_freq_khz;    /* PLL div by 4 */
> > +
> > +     u32 hzd;                /* HS Zero Delay in ns*/
> > +     u32 hpd;                /* HS Prepare Delay is ns */
> > +};
> > +
> > +static inline struct ssd2825_priv *dsi_host_to_ssd2825(struct mipi_dsi_host
> > +                                                      *host)
> > +{
> > +     return container_of(host, struct ssd2825_priv, dsi_host);
> > +}
> > +
> > +static inline struct ssd2825_priv *bridge_to_ssd2825(struct drm_bridge
> > +                                                  *bridge)
> > +{
> > +     return container_of(bridge, struct ssd2825_priv, bridge);
> > +}
> > +
> > +static int ssd2825_write_raw(struct ssd2825_priv *priv, u8 high_byte, u8 low_byte)
> > +{
> > +     struct spi_device *spi = priv->spi;
> > +     u8 tx_buf[2];
> > +
> > +     /*
> > +      * Low byte is the value, high byte defines type of
> > +      * write cycle, 0 for command and 1 for data.
> > +      */
> > +     tx_buf[0] = low_byte;
> > +     tx_buf[1] = high_byte;
> > +
> > +     return spi_write(spi, tx_buf, 2);
> > +}
> > +
> > +static int ssd2825_write_reg(struct ssd2825_priv *priv, u8 reg, u16 command)
> > +{
> > +     u8 datal = (command & 0x00FF);
> > +     u8 datah = (command & 0xFF00) >> 8;
> > +     int ret;
> > +
> > +     /* Command write cycle */
> > +     ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Data write cycle bits 7-0 */
> > +     ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datal);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Data write cycle bits 15-8 */
> > +     ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, datah);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int ssd2825_write_dsi(struct ssd2825_priv *priv, const u8 *command, int len)
> > +{
> > +     int ret, i;
> > +
> > +     ret = ssd2825_write_reg(priv, SSD2825_PACKET_SIZE_CTRL_REG_1, len);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, SSD2825_PACKET_DROP_REG);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (i = 0; i < len; i++) {
> > +             ret = ssd2825_write_raw(priv, SSD2825_DAT_BYTE, command[i]);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int ssd2825_read_raw(struct ssd2825_priv *priv, u8 cmd, u16 *data)
> > +{
> > +     struct spi_device *spi = priv->spi;
> > +     struct spi_message msg;
> > +     struct spi_transfer xfer[2];
> > +     u8 tx_buf[2];
> > +     u8 rx_buf[2];
> > +     int ret;
> > +
> > +     memset(&xfer, 0, sizeof(xfer));
> > +
> > +     tx_buf[1] = (cmd & 0xFF00) >> 8;
> > +     tx_buf[0] = (cmd & 0x00FF);
> > +
> > +     xfer[0].tx_buf = tx_buf;
> > +     xfer[0].bits_per_word = 9;
> > +     xfer[0].len = 2;
> > +
> > +     xfer[1].rx_buf = rx_buf;
> > +     xfer[1].bits_per_word = 16;
> > +     xfer[1].len = 2;
> > +
> > +     spi_message_init(&msg);
> > +     spi_message_add_tail(&xfer[0], &msg);
> > +     spi_message_add_tail(&xfer[1], &msg);
> > +
> > +     ret = spi_sync(spi, &msg);
> > +     if (ret)
> > +             dev_err(&spi->dev, "spi_sync_read failed %d\n", ret);
> > +
> > +     *data = rx_buf[1] | (rx_buf[0] << 8);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ssd2825_read_reg(struct ssd2825_priv *priv, u8 reg, u16 *data)
> > +{
> > +     int ret;
> > +
> > +     /* Reset the read register */
> > +     ret = ssd2825_write_reg(priv, SSD2825_SPI_READ_REG, SSD2825_SPI_READ_REG_RESET);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Push the address to read */
> > +     ret = ssd2825_write_raw(priv, SSD2825_COM_BYTE, reg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Perform a reading cycle */
> > +     ret = ssd2825_read_raw(priv, SSD2825_SPI_READ_REG_RESET, data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int ssd2825_dsi_host_attach(struct mipi_dsi_host *host,
> > +                                struct mipi_dsi_device *dev)
> > +{
> > +     struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> > +     struct drm_bridge *bridge;
> > +     struct drm_panel *panel;
> > +     struct device_node *ep;
> > +     int ret;
> > +
> > +     if (dev->lanes > 4) {
> > +             dev_err(priv->dev, "unsupported number of data lanes(%u)\n",
> > +                     dev->lanes);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /*
> > +      * ssd2825 supports both Video and Pulse mode, but the driver only
> > +      * implements Video (event) mode currently
> > +      */
> > +     if (!(dev->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> > +             dev_err(priv->dev, "Only MIPI_DSI_MODE_VIDEO is supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     ret = drm_of_find_panel_or_bridge(host->dev->of_node, 1, 0, &panel,
> > +                                       &bridge);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (panel) {
> > +             bridge = drm_panel_bridge_add_typed(panel,
> > +                                                 DRM_MODE_CONNECTOR_DSI);
> > +             if (IS_ERR(bridge))
> > +                     return PTR_ERR(bridge);
> > +     }
> > +
> > +     priv->output.dev = dev;
> > +     priv->output.bridge = bridge;
> > +     priv->output.panel = panel;
> > +
> > +     priv->dsi_lanes = dev->lanes;
> > +
> > +     /* get input ep (port0/endpoint0) */
> > +     ret = -EINVAL;
> > +     ep = of_graph_get_endpoint_by_regs(host->dev->of_node, 0, 0);
> > +     if (ep) {
> > +             ret = of_property_read_u32(ep, "bus-width", &priv->pd_lines);
> > +
> > +             of_node_put(ep);
> > +     }
> > +
> > +     if (ret)
> > +             priv->pd_lines = mipi_dsi_pixel_format_to_bpp(dev->format);
> > +
> > +     drm_bridge_add(&priv->bridge);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ssd2825_dsi_host_detach(struct mipi_dsi_host *host,
> > +                                struct mipi_dsi_device *dev)
> > +{
> > +     struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> > +
> > +     drm_bridge_remove(&priv->bridge);
> > +     if (priv->output.panel)
> > +             drm_panel_bridge_remove(priv->output.bridge);
> > +
> > +     return 0;
> > +}
> > +
> > +static ssize_t ssd2825_dsi_host_transfer(struct mipi_dsi_host *host,
> > +                                      const struct mipi_dsi_msg *msg)
> > +{
> > +     struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> > +     u8 buf = *(u8 *)msg->tx_buf;
> > +     u16 config;
> > +     int ret;
> > +
> > +     if (!priv->enabled) {
> > +             dev_err(priv->dev, "Bridge is not enabled\n");
> > +             return -ENODEV;
> > +     }
>
> No. See include/drm/drm_mipi_dsi.h:
>
>  * Also note that those callbacks can be called no matter the state the
>  * host is in. Drivers that need the underlying device to be powered to
>  * perform these operations will first need to make sure it's been
>  * properly enabled.
>
>
> > +
> > +     if (msg->rx_len) {
> > +             dev_warn(priv->dev, "MIPI rx is not supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     ret = ssd2825_read_reg(priv, SSD2825_CONFIGURATION_REG, &config);
> > +     if (ret)
> > +             return ret;
> > +
> > +     switch (msg->type) {
> > +     case MIPI_DSI_DCS_SHORT_WRITE:
> > +     case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> > +     case MIPI_DSI_DCS_LONG_WRITE:
> > +             config |= SSD2825_CONF_REG_DCS;
> > +             break;
> > +     case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> > +     case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> > +     case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> > +     case MIPI_DSI_GENERIC_LONG_WRITE:
> > +             config &= ~SSD2825_CONF_REG_DCS;
> > +             break;
> > +     case MIPI_DSI_DCS_READ:
> > +     case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> > +     case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> > +     case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> > +     default:
> > +             return 0;
> > +     }
> > +
> > +     ret = ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG, config);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ssd2825_write_dsi(priv, msg->tx_buf, msg->tx_len);
> > +     if (ret)
> > +             return ret;
>
> Do you need any kind of locking in case there are two simultaneous
> transfer operations?
>
> > +
> > +     if (buf == MIPI_DCS_SET_DISPLAY_ON) {
> > +             ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> > +                               SSD2825_CONF_REG_HS | SSD2825_CONF_REG_VEN |
> > +                               SSD2825_CONF_REG_DCS | SSD2825_CONF_REG_ECD |
> > +                               SSD2825_CONF_REG_EOT);
>
> Most of these flags should be set depending on the
> mipi_dsi_device.mode_flags.
>
> > +             ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
> > +             ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
>
> It looks like this should be a part of the .atomic_enable() callback.
>
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct mipi_dsi_host_ops ssd2825_dsi_host_ops = {
> > +     .attach = ssd2825_dsi_host_attach,
> > +     .detach = ssd2825_dsi_host_detach,
> > +     .transfer = ssd2825_dsi_host_transfer,
> > +};
> > +
> > +static void ssd2825_hw_reset(struct ssd2825_priv *priv)
> > +{
> > +     gpiod_set_value_cansleep(priv->reset_gpio, 1);
> > +     usleep_range(5000, 6000);
> > +     gpiod_set_value_cansleep(priv->reset_gpio, 0);
> > +     usleep_range(5000, 6000);
> > +}
> > +
> > +/*
> > + * PLL configuration register settings.
> > + *
> > + * See the "PLL Configuration Register Description" in the SSD2825 datasheet.
> > + */
> > +static u16 construct_pll_config(struct ssd2825_priv *priv,
> > +                             u32 desired_pll_freq_kbps, u32 reference_freq_khz)
> > +{
> > +     u32 div_factor = 1, mul_factor, fr = 0;
> > +
> > +     while (reference_freq_khz / (div_factor + 1) >= SSD2825_REF_MIN_CLK)
> > +             div_factor++;
> > +     if (div_factor > 31)
> > +             div_factor = 31;
> > +
> > +     mul_factor = DIV_ROUND_UP(desired_pll_freq_kbps * div_factor,
> > +                               reference_freq_khz);
> > +
> > +     priv->pll_freq_kbps = reference_freq_khz * mul_factor / div_factor;
> > +     priv->nibble_freq_khz = priv->pll_freq_kbps / 4;
> > +
> > +     if (priv->pll_freq_kbps >= 501000)
> > +             fr = 3;
> > +     else if (priv->pll_freq_kbps >= 251000)
> > +             fr = 2;
> > +     else if (priv->pll_freq_kbps >= 126000)
> > +             fr = 1;
> > +
> > +     return (fr << 14) | (div_factor << 8) | mul_factor;
> > +}
> > +
> > +static u32 ssd2825_to_ns(u32 khz)
> > +{
> > +     return (1000 * 1000 / khz);
> > +}
> > +
> > +static int ssd2825_setup_pll(struct ssd2825_priv *priv,
> > +                          const struct drm_display_mode *mode)
> > +{
> > +     u16 pll_config, lp_div;
> > +     u32 nibble_delay, pclk_mult, tx_freq_khz;
> > +     u8 hzd, hpd;
> > +
> > +     tx_freq_khz = clk_get_rate(priv->tx_clk) / 1000;
> > +     pclk_mult = priv->pd_lines / priv->dsi_lanes + 1;
> > +     pll_config = construct_pll_config(priv, pclk_mult * mode->clock,
> > +                                       tx_freq_khz);
> > +
> > +     lp_div = priv->pll_freq_kbps / (SSD2825_LP_MIN_CLK * 8);
> > +
> > +     nibble_delay = ssd2825_to_ns(priv->nibble_freq_khz);
> > +
> > +     hzd = priv->hzd / nibble_delay;
> > +     hpd = (priv->hpd - 4 * nibble_delay) / nibble_delay;
> > +
> > +     /* Disable PLL */
> > +     ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0000);
> > +     ssd2825_write_reg(priv, SSD2825_LINE_CTRL_REG, 0x0001);
> > +
> > +     /* Set delays */
> > +     dev_dbg(priv->dev, "SSD2825_DELAY_ADJ_REG_1 0x%x\n", (hzd << 8) | hpd);
>
> drm_dbg_driver() ?
>
> > +     ssd2825_write_reg(priv, SSD2825_DELAY_ADJ_REG_1, (hzd << 8) | hpd);
> > +
> > +     /* Set PLL coeficients */
> > +     dev_dbg(priv->dev, "SSD2825_PLL_CONFIGURATION_REG 0x%x\n", pll_config);
> > +     ssd2825_write_reg(priv, SSD2825_PLL_CONFIGURATION_REG, pll_config);
> > +
> > +     /* Clock Control Register */
> > +     dev_dbg(priv->dev, "SSD2825_CLOCK_CTRL_REG 0x%x\n",
> > +             SSD2828_LP_CLOCK_DIVIDER(lp_div));
> > +     ssd2825_write_reg(priv, SSD2825_CLOCK_CTRL_REG,
> > +                       SSD2828_LP_CLOCK_DIVIDER(lp_div));
> > +
> > +     /* Enable PLL */
> > +     ssd2825_write_reg(priv, SSD2825_PLL_CTRL_REG, 0x0001);
> > +     ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> > +
> > +     return 0;
> > +}
> > +
> > +static void ssd2825_bridge_pre_enable(struct drm_bridge *bridge)
>
> Is there a reason not to use the .atomic versions?
>
> > +{
> > +     struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> > +     int ret;
> > +
> > +     if (priv->enabled)
> > +             return;
> > +
> > +     /* Power Sequence */
> > +     ret = clk_prepare_enable(priv->tx_clk);
> > +     if (ret < 0)
> > +             dev_err(priv->dev, "error enabling tx_clk (%d)\n", ret);
> > +
> > +     ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), priv->supplies);
> > +     if (ret < 0)
> > +             dev_err(priv->dev, "error enabling regulators (%d)\n", ret);
> > +
> > +     usleep_range(1000, 2000);
> > +
> > +     ssd2825_hw_reset(priv);
> > +
> > +     priv->enabled = true;
> > +}
> > +
> > +static void ssd2825_bridge_enable(struct drm_bridge *bridge)
> > +{
> > +     struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> > +     struct mipi_dsi_device *dsi_dev = priv->output.dev;
> > +     unsigned long mode_flags = dsi_dev->mode_flags;
> > +     const struct drm_display_mode *mode =
> > +             &bridge->encoder->crtc->state->adjusted_mode;
>
> If you'd used atomic callbacks, you could have gone through the state to
> get corresponding data. Your main driver can be atomic, in which case
> you should not be using encoder->crtc and crtc->state.
>
> > +     struct device *dev = priv->dev;
> > +     u8 pixel_format;
> > +
> > +     if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> > +             dev_warn_once(dev,
> > +                           "Non-continuous mode unimplemented, falling back to continuous\n");
> > +             mode_flags &= ~MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > +     }
> > +
> > +     if (mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> > +             dev_warn_once(dev,
> > +                           "Burst mode unimplemented, falling back to simple\n");
> > +             mode_flags &= ~MIPI_DSI_MODE_VIDEO_BURST;
>
> This should be declined earlier, e.g. in the dsi_attach.
>
> > +     }
> > +
> > +     /* Perform SW reset */
> > +     ssd2825_write_reg(priv, SSD2825_OPERATION_CTRL_REG, 0x0100);
> > +
> > +     switch (dsi_dev->format) {
> > +     case MIPI_DSI_FMT_RGB565:
> > +             pixel_format = 0x00;
> > +             break;
> > +     case MIPI_DSI_FMT_RGB666_PACKED:
> > +             pixel_format = 0x01;
> > +             break;
> > +     case MIPI_DSI_FMT_RGB666:
> > +             pixel_format = 0x02;
> > +             break;
> > +     case MIPI_DSI_FMT_RGB888:
> > +     default:
> > +             pixel_format = 0x03;
> > +             break;
> > +     }
> > +
> > +     /* Set panel timings */
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_1,
> > +                       ((mode->vtotal - mode->vsync_end) << 8) |
> > +                       (mode->htotal - mode->hsync_end));
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_2,
> > +                       ((mode->vtotal - mode->vsync_start) << 8) |
> > +                       (mode->htotal - mode->hsync_start));
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_3,
> > +                       ((mode->vsync_start - mode->vdisplay) << 8) |
> > +                       (mode->hsync_start - mode->hdisplay));
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_4, mode->hdisplay);
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_5, mode->vdisplay);
>
> Please check that the mode fits into corresponding bit fields in
> .mode_valid().
>
> > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_6,
> > +                       SSD2825_HSYNC_HIGH | SSD2825_VSYNC_HIGH |
> > +                       SSD2825_PCKL_HIGH | SSD2825_NON_BURST |
>
> These flags should come from drm_display_mode
>
> > +                       pixel_format);
> > +
> > +     ssd2825_write_reg(priv, SSD2825_LANE_CONFIGURATION_REG, dsi_dev->lanes - 1);
> > +     ssd2825_write_reg(priv, SSD2825_TEST_REG, 0x0004);
> > +
> > +     /* Call PLL configuration */
> > +     ssd2825_setup_pll(priv, mode);
> > +
> > +     usleep_range(10000, 11000);
> > +
> > +     /* Initial DSI configuration register set */
> > +     ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> > +                       SSD2825_CONF_REG_CKE | SSD2825_CONF_REG_DCS |
> > +                       SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
> > +     ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> > +}
> > +
> > +static void ssd2825_bridge_disable(struct drm_bridge *bridge)
> > +{
> > +     struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> > +     int ret;
> > +
> > +     if (!priv->enabled)
> > +             return;
> > +
> > +     msleep(100);
> > +
> > +     /* Exit DSI configuration register set */
> > +     ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG,
> > +                       SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT);
> > +     ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> > +
> > +     /* HW disable */
> > +     gpiod_set_value_cansleep(priv->reset_gpio, 1);
> > +     usleep_range(5000, 6000);
> > +
> > +     ret = regulator_bulk_disable(ARRAY_SIZE(priv->supplies),
> > +                                  priv->supplies);
> > +     if (ret < 0)
> > +             dev_err(priv->dev, "error disabling regulators (%d)\n", ret);
> > +
> > +     clk_disable_unprepare(priv->tx_clk);
> > +
> > +     priv->enabled = false;
> > +}
> > +
> > +static int ssd2825_bridge_attach(struct drm_bridge *bridge,
> > +                              enum drm_bridge_attach_flags flags)
> > +{
> > +     struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> > +
> > +     return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> > +                              flags);
> > +}
> > +
> > +static enum drm_mode_status
> > +ssd2825_bridge_mode_valid(struct drm_bridge *bridge,
> > +                       const struct drm_display_info *info,
> > +                       const struct drm_display_mode *mode)
> > +{
>
> No resulution / clock limitations?
>
> > +     return MODE_OK;
> > +}
> > +
> > +static const struct drm_bridge_funcs ssd2825_bridge_funcs = {
> > +     .attach = ssd2825_bridge_attach,
> > +     .mode_valid = ssd2825_bridge_mode_valid,
> > +     .pre_enable = ssd2825_bridge_pre_enable,
> > +     .enable = ssd2825_bridge_enable,
> > +     .disable = ssd2825_bridge_disable,
> > +};
> > +
> > +static int ssd2825_probe(struct spi_device *spi)
> > +{
> > +     struct ssd2825_priv *priv;
> > +     struct device *dev = &spi->dev;
> > +     struct device_node *np = dev->of_node;
> > +     int i, ret;
> > +
> > +     /* Driver supports only this mode for now */
> > +     spi->bits_per_word = 9;
> > +
> > +     ret = spi_setup(spi);
> > +     if (ret)
> > +             return ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     spi_set_drvdata(spi, priv);
> > +     priv->spi = spi;
> > +
> > +     dev_set_drvdata(dev, priv);
> > +     priv->dev = dev;
> > +
> > +     priv->tx_clk = devm_clk_get_optional(dev, "tx_clk");
> > +     if (IS_ERR(priv->tx_clk))
> > +             return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
> > +                                  "can't retrieve bridge tx_clk\n");
> > +
> > +     priv->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +                                                GPIOD_OUT_HIGH);
> > +     if (IS_ERR(priv->reset_gpio))
> > +             return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
> > +                                  "failed to get reset GPIO\n");
> > +
> > +     for (i = 0; i < ARRAY_SIZE(priv->supplies); ++i)
> > +             priv->supplies[i].supply = ssd2825_supplies[i];
>
> You can use devm_regulator_bulk_get_const() instead.
>
> > +
> > +     ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(priv->supplies),
> > +                                   priv->supplies);
> > +     if (ret < 0)
> > +             return dev_err_probe(dev, ret, "failed to get regulators\n");
> > +
> > +     ret = device_property_read_u32(dev, "solomon,hs-zero-delay-ns", &priv->hzd);
> > +     if (ret)
> > +             /* If no hs-zero-delay is set by device tree, use the default 133 ns */
> > +             priv->hzd = 133;
> > +
> > +     ret = device_property_read_u32(dev, "solomon,hs-prep-delay-ns", &priv->hpd);
> > +     if (ret)
> > +             /* If no hs-prep-delay is set by device tree, use the default 40 ns */
> > +             priv->hzd = 40;
> > +
> > +     priv->dsi_host.dev = dev;
> > +     priv->dsi_host.ops = &ssd2825_dsi_host_ops;
> > +
> > +     priv->bridge.funcs = &ssd2825_bridge_funcs;
> > +     priv->bridge.of_node = np;
> > +
> > +     return mipi_dsi_host_register(&priv->dsi_host);
> > +}
> > +
> > +static void ssd2825_remove(struct spi_device *spi)
> > +{
> > +     struct ssd2825_priv *priv = spi_get_drvdata(spi);
> > +
> > +     mipi_dsi_host_unregister(&priv->dsi_host);
> > +}
> > +
> > +static const struct of_device_id ssd2825_of_match[] = {
> > +     { .compatible = "solomon,ssd2825" },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ssd2825_of_match);
> > +
> > +static struct spi_driver ssd2825_driver = {
> > +     .driver = {
> > +             .name = "ssd2825",
> > +             .of_match_table = ssd2825_of_match,
> > +     },
> > +     .probe = ssd2825_probe,
> > +     .remove = ssd2825_remove,
> > +};
> > +module_spi_driver(ssd2825_driver);
> > +
> > +MODULE_DESCRIPTION("Solomon SSD2825 RGB to MIPI-DSI bridge driver SPI");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.43.0
> >
>
> --
> With best wishes
> Dmitry

Acknowledged, thank you.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825
  2025-02-13 13:56 ` [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825 Svyatoslav Ryhel
@ 2025-02-13 20:34   ` Conor Dooley
  2025-02-14  6:29     ` Svyatoslav Ryhel
  0 siblings, 1 reply; 10+ messages in thread
From: Conor Dooley @ 2025-02-13 20:34 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 4820 bytes --]

On Thu, Feb 13, 2025 at 03:56:04PM +0200, Svyatoslav Ryhel wrote:
> Add bindings for Solomon SSD2825 MIPI master bridge chip that connects an
> application processor with traditional parallel LCD interface and an LCD
> driver with MIPI slave interface. The SSD2825 supports both parallel RGB
> interface and serial SPI interface.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  .../display/bridge/solomon,ssd2825.yaml       | 140 ++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> new file mode 100644
> index 000000000000..cd7ff971495c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/solomon,ssd2825.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Solomon SSD2825 RGB to MIPI-DSI bridge
> +
> +maintainers:
> +  - Svyatoslav Ryhel <clamor95@gmail.com>
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: solomon,ssd2825
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios: true
> +
> +  dvdd-supply:
> +    description: Regulator for 1.2V digital power supply.
> +
> +  avdd-supply:
> +    description: Regulator for 1.2V analog power supply.
> +
> +  vddio-supply:
> +    description: Regulator for 1.8V IO power supply.
> +
> +  spi-max-frequency:
> +    maximum: 1000000
> +
> +  spi-cpha: true
> +  spi-cpol: true

Should these be required? Supplies should really be required too, since
the device probably cannot function without them?

> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: tx_clk

Drop the _clk, since this cannot be anything else! clock-names isn't
really useful when you have just one, so I'd be inclined to say remove
it entirely...

> +  solomon,hs-zero-delay-ns:
> +    description:
> +      HS zero delay period
> +    default: 133
> +
> +  solomon,hs-prep-delay-ns:
> +    description:
> +      HS prep delay period
> +    default: 40

Do these two have limits? Use maximum/minimum to set them if so.
Cheers,
Conor.

> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Video port for RGB input
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +            unevaluatedProperties: false
> +
> +            properties:
> +              bus-width:
> +                enum: [ 16, 18, 24 ]
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Video port for DSI output (panel or connector)
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dsi@2 {
> +            compatible = "solomon,ssd2825";
> +            reg = <2>;
> +
> +            spi-max-frequency = <1000000>;
> +
> +            spi-cpha;
> +            spi-cpol;
> +
> +            reset-gpios = <&gpio 114 GPIO_ACTIVE_LOW>;
> +
> +            dvdd-supply = <&vdd_1v2>;
> +            avdd-supply = <&vdd_1v2>;
> +            vddio-supply = <&vdd_1v8_io>;
> +
> +            solomon,hs-zero-delay-ns = <300>;
> +            solomon,hs-prep-delay-ns = <65>;
> +
> +            clocks = <&ssd2825_tx_clk>;
> +            clock-names = "tx_clk";
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +
> +                    bridge_input: endpoint {
> +                        remote-endpoint = <&dpi_output>;
> +                        bus-width = <24>;
> +                    };
> +                };
> +
> +                port@1 {
> +                    reg = <1>;
> +
> +                    bridge_output: endpoint {
> +                        remote-endpoint = <&panel_input>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.43.0
> 

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825
  2025-02-13 20:34   ` Conor Dooley
@ 2025-02-14  6:29     ` Svyatoslav Ryhel
  2025-02-18 17:09       ` Conor Dooley
  0 siblings, 1 reply; 10+ messages in thread
From: Svyatoslav Ryhel @ 2025-02-14  6:29 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

чт, 13 лют. 2025 р. о 22:34 Conor Dooley <conor@kernel.org> пише:
>
> On Thu, Feb 13, 2025 at 03:56:04PM +0200, Svyatoslav Ryhel wrote:
> > Add bindings for Solomon SSD2825 MIPI master bridge chip that connects an
> > application processor with traditional parallel LCD interface and an LCD
> > driver with MIPI slave interface. The SSD2825 supports both parallel RGB
> > interface and serial SPI interface.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  .../display/bridge/solomon,ssd2825.yaml       | 140 ++++++++++++++++++
> >  1 file changed, 140 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> > new file mode 100644
> > index 000000000000..cd7ff971495c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/solomon,ssd2825.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Solomon SSD2825 RGB to MIPI-DSI bridge
> > +
> > +maintainers:
> > +  - Svyatoslav Ryhel <clamor95@gmail.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: solomon,ssd2825
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios: true
> > +
> > +  dvdd-supply:
> > +    description: Regulator for 1.2V digital power supply.
> > +
> > +  avdd-supply:
> > +    description: Regulator for 1.2V analog power supply.
> > +
> > +  vddio-supply:
> > +    description: Regulator for 1.8V IO power supply.
> > +
> > +  spi-max-frequency:
> > +    maximum: 1000000
> > +
> > +  spi-cpha: true
> > +  spi-cpol: true
>
> Should these be required? Supplies should really be required too, since
> the device probably cannot function without them?
>

No, since spi-* define mode in which device works. If both are present
it is mode 3, if none it is mode 0.

About supplies, device cannot work without power supply obviously, but
often exact supplies are not known and I would like to not enforce
someone to add random regulators just because they are mandatory.

> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: tx_clk
>
> Drop the _clk, since this cannot be anything else! clock-names isn't
> really useful when you have just one, so I'd be inclined to say remove
> it entirely...
>

TX_CLK is the name which datasheet refers to hence I have included
clock name solely to have clear link between datasheet clock
references and clock used here.

> > +  solomon,hs-zero-delay-ns:
> > +    description:
> > +      HS zero delay period
> > +    default: 133
> > +
> > +  solomon,hs-prep-delay-ns:
> > +    description:
> > +      HS prep delay period
> > +    default: 40
>
> Do these two have limits? Use maximum/minimum to set them if so.
> Cheers,
> Conor.
>

Datasheet does not specify limits actually, only defaults. I will try
to calculate boundaries.

> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        unevaluatedProperties: false
> > +        description:
> > +          Video port for RGB input
> > +
> > +        properties:
> > +          endpoint:
> > +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +            unevaluatedProperties: false
> > +
> > +            properties:
> > +              bus-width:
> > +                enum: [ 16, 18, 24 ]
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description:
> > +          Video port for DSI output (panel or connector)
> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        dsi@2 {
> > +            compatible = "solomon,ssd2825";
> > +            reg = <2>;
> > +
> > +            spi-max-frequency = <1000000>;
> > +
> > +            spi-cpha;
> > +            spi-cpol;
> > +
> > +            reset-gpios = <&gpio 114 GPIO_ACTIVE_LOW>;
> > +
> > +            dvdd-supply = <&vdd_1v2>;
> > +            avdd-supply = <&vdd_1v2>;
> > +            vddio-supply = <&vdd_1v8_io>;
> > +
> > +            solomon,hs-zero-delay-ns = <300>;
> > +            solomon,hs-prep-delay-ns = <65>;
> > +
> > +            clocks = <&ssd2825_tx_clk>;
> > +            clock-names = "tx_clk";
> > +
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +
> > +                    bridge_input: endpoint {
> > +                        remote-endpoint = <&dpi_output>;
> > +                        bus-width = <24>;
> > +                    };
> > +                };
> > +
> > +                port@1 {
> > +                    reg = <1>;
> > +
> > +                    bridge_output: endpoint {
> > +                        remote-endpoint = <&panel_input>;
> > +                    };
> > +                };
> > +            };
> > +        };
> > +    };
> > --
> > 2.43.0
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
  2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
  2025-02-13 16:05   ` Dmitry Baryshkov
@ 2025-02-15  3:48   ` kernel test robot
  2025-02-15  8:31   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-02-15  3:48 UTC (permalink / raw)
  To: Svyatoslav Ryhel, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: oe-kbuild-all, dri-devel, devicetree, linux-kernel

Hi Svyatoslav,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.14-rc2 next-20250214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Svyatoslav-Ryhel/dt-bindings-display-bridge-Document-Solomon-SSD2825/20250213-215821
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250213135605.157650-3-clamor95%40gmail.com
patch subject: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
config: x86_64-randconfig-077-20250215 (https://download.01.org/0day-ci/archive/20250215/202502151123.DwXjsR05-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250215/202502151123.DwXjsR05-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502151123.DwXjsR05-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: vmlinux.o: in function `spi_sync_transfer':
>> include/linux/spi/spi.h:1404: undefined reference to `spi_sync'
   ld: vmlinux.o: in function `ssd2825_read_raw':
>> drivers/gpu/drm/bridge/ssd2825.c:241: undefined reference to `spi_sync'
   ld: vmlinux.o: in function `ssd2825_probe':
>> drivers/gpu/drm/bridge/ssd2825.c:664: undefined reference to `spi_setup'
   ld: vmlinux.o: in function `ssd2825_driver_init':
>> drivers/gpu/drm/bridge/ssd2825.c:737: undefined reference to `__spi_register_driver'


vim +1404 include/linux/spi/spi.h

8ae12a0d85987d David Brownell     2006-01-08  1382  
323117ab60156d Geert Uytterhoeven 2016-09-13  1383  /**
323117ab60156d Geert Uytterhoeven 2016-09-13  1384   * spi_sync_transfer - synchronous SPI data transfer
323117ab60156d Geert Uytterhoeven 2016-09-13  1385   * @spi: device with which data will be exchanged
323117ab60156d Geert Uytterhoeven 2016-09-13  1386   * @xfers: An array of spi_transfers
323117ab60156d Geert Uytterhoeven 2016-09-13  1387   * @num_xfers: Number of items in the xfer array
323117ab60156d Geert Uytterhoeven 2016-09-13  1388   * Context: can sleep
323117ab60156d Geert Uytterhoeven 2016-09-13  1389   *
323117ab60156d Geert Uytterhoeven 2016-09-13  1390   * Does a synchronous SPI data transfer of the given spi_transfer array.
323117ab60156d Geert Uytterhoeven 2016-09-13  1391   *
323117ab60156d Geert Uytterhoeven 2016-09-13  1392   * For more specific semantics see spi_sync().
323117ab60156d Geert Uytterhoeven 2016-09-13  1393   *
2ae3de10abfe0b Randy Dunlap       2020-07-15  1394   * Return: zero on success, else a negative error code.
323117ab60156d Geert Uytterhoeven 2016-09-13  1395   */
323117ab60156d Geert Uytterhoeven 2016-09-13  1396  static inline int
323117ab60156d Geert Uytterhoeven 2016-09-13  1397  spi_sync_transfer(struct spi_device *spi, struct spi_transfer *xfers,
323117ab60156d Geert Uytterhoeven 2016-09-13  1398  	unsigned int num_xfers)
323117ab60156d Geert Uytterhoeven 2016-09-13  1399  {
323117ab60156d Geert Uytterhoeven 2016-09-13  1400  	struct spi_message msg;
323117ab60156d Geert Uytterhoeven 2016-09-13  1401  
323117ab60156d Geert Uytterhoeven 2016-09-13  1402  	spi_message_init_with_transfers(&msg, xfers, num_xfers);
323117ab60156d Geert Uytterhoeven 2016-09-13  1403  
323117ab60156d Geert Uytterhoeven 2016-09-13 @1404  	return spi_sync(spi, &msg);
323117ab60156d Geert Uytterhoeven 2016-09-13  1405  }
323117ab60156d Geert Uytterhoeven 2016-09-13  1406  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
  2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
  2025-02-13 16:05   ` Dmitry Baryshkov
  2025-02-15  3:48   ` kernel test robot
@ 2025-02-15  8:31   ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2025-02-15  8:31 UTC (permalink / raw)
  To: Svyatoslav Ryhel, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley
  Cc: oe-kbuild-all, dri-devel, devicetree, linux-kernel

Hi Svyatoslav,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master drm-misc/drm-misc-next v6.14-rc2 next-20250214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Svyatoslav-Ryhel/dt-bindings-display-bridge-Document-Solomon-SSD2825/20250213-215821
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250213135605.157650-3-clamor95%40gmail.com
patch subject: [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge
config: openrisc-randconfig-r063-20250215 (https://download.01.org/0day-ci/archive/20250215/202502151636.fgFKMZoI-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250215/202502151636.fgFKMZoI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502151636.fgFKMZoI-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in lib/asn1_decoder.o
>> ERROR: modpost: "__spi_register_driver" [drivers/gpu/drm/bridge/ssd2825.ko] undefined!
>> ERROR: modpost: "spi_setup" [drivers/gpu/drm/bridge/ssd2825.ko] undefined!
>> ERROR: modpost: "spi_sync" [drivers/gpu/drm/bridge/ssd2825.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825
  2025-02-14  6:29     ` Svyatoslav Ryhel
@ 2025-02-18 17:09       ` Conor Dooley
  0 siblings, 0 replies; 10+ messages in thread
From: Conor Dooley @ 2025-02-18 17:09 UTC (permalink / raw)
  To: Svyatoslav Ryhel
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3960 bytes --]

On Fri, Feb 14, 2025 at 08:29:22AM +0200, Svyatoslav Ryhel wrote:
> чт, 13 лют. 2025 р. о 22:34 Conor Dooley <conor@kernel.org> пише:
> >
> > On Thu, Feb 13, 2025 at 03:56:04PM +0200, Svyatoslav Ryhel wrote:
> > > Add bindings for Solomon SSD2825 MIPI master bridge chip that connects an
> > > application processor with traditional parallel LCD interface and an LCD
> > > driver with MIPI slave interface. The SSD2825 supports both parallel RGB
> > > interface and serial SPI interface.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  .../display/bridge/solomon,ssd2825.yaml       | 140 ++++++++++++++++++
> > >  1 file changed, 140 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> > > new file mode 100644
> > > index 000000000000..cd7ff971495c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/bridge/solomon,ssd2825.yaml
> > > @@ -0,0 +1,140 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/bridge/solomon,ssd2825.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Solomon SSD2825 RGB to MIPI-DSI bridge
> > > +
> > > +maintainers:
> > > +  - Svyatoslav Ryhel <clamor95@gmail.com>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: solomon,ssd2825
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios: true
> > > +
> > > +  dvdd-supply:
> > > +    description: Regulator for 1.2V digital power supply.
> > > +
> > > +  avdd-supply:
> > > +    description: Regulator for 1.2V analog power supply.
> > > +
> > > +  vddio-supply:
> > > +    description: Regulator for 1.8V IO power supply.
> > > +
> > > +  spi-max-frequency:
> > > +    maximum: 1000000
> > > +
> > > +  spi-cpha: true
> > > +  spi-cpol: true
> >
> > Should these be required? Supplies should really be required too, since
> > the device probably cannot function without them?
> >
> 
> No, since spi-* define mode in which device works. If both are present
> it is mode 3, if none it is mode 0.
> 
> About supplies, device cannot work without power supply obviously, but
> often exact supplies are not known and I would like to not enforce
> someone to add random regulators just because they are mandatory.

If the device doesn't work without the supplies, the supplies should be
mandatory in the binding.

> 
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    const: tx_clk
> >
> > Drop the _clk, since this cannot be anything else! clock-names isn't
> > really useful when you have just one, so I'd be inclined to say remove
> > it entirely...
> >
> 
> TX_CLK is the name which datasheet refers to hence I have included
> clock name solely to have clear link between datasheet clock
> references and clock used here.

I think people will be able to make the leap between "tx_clk" and "tx".

> 
> > > +  solomon,hs-zero-delay-ns:
> > > +    description:
> > > +      HS zero delay period
> > > +    default: 133
> > > +
> > > +  solomon,hs-prep-delay-ns:
> > > +    description:
> > > +      HS prep delay period
> > > +    default: 40
> >
> > Do these two have limits? Use maximum/minimum to set them if so.
> > Cheers,
> > Conor.
> >
> 
> Datasheet does not specify limits actually, only defaults. I will try
> to calculate boundaries.

Hmm, that's interesting. I see in your driver that the eventual value
gets converted down to a u8? That should at least impose some sort of
limits I guess.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-02-18 17:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-13 13:56 [PATCH v1 0/2] drm: bridge: add ssd2825 RGB/DSI bridge support Svyatoslav Ryhel
2025-02-13 13:56 ` [PATCH v1 1/2] dt-bindings: display: bridge: Document Solomon SSD2825 Svyatoslav Ryhel
2025-02-13 20:34   ` Conor Dooley
2025-02-14  6:29     ` Svyatoslav Ryhel
2025-02-18 17:09       ` Conor Dooley
2025-02-13 13:56 ` [PATCH v1 2/2] drm: bridge: Add support for Solomon SSD2825 RGB/DSI bridge Svyatoslav Ryhel
2025-02-13 16:05   ` Dmitry Baryshkov
2025-02-13 16:08     ` Svyatoslav Ryhel
2025-02-15  3:48   ` kernel test robot
2025-02-15  8:31   ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.