[PATCH 0/3] LVDS Controller Support for SAM9X7 SoC

[PATCH 0/3] LVDS Controller Support for SAM9X7 SoC

[Dharma Balasubiramani]

This patch series introduces LVDS controller support for the SAM9X7 SoC. The
LVDS controller is designed to work with Microchip's sam9x7 series
System-on-Chip (SoC) devices, providing Low Voltage Differential Signaling
capabilities.

Dharma Balasubiramani (3):
  dt-bindings: display: bridge: add sam9x7-lvds compatible
  drm/bridge: add lvds controller support for sam9x7
  MAINTAINERS: add SAM9X7 SoC's LVDS controller

 .../display/bridge/microchip,sam9x7-lvds.yaml |  59 +++++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/bridge/Kconfig                |   7 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/microchip-lvds.c       | 250 ++++++++++++++++++
 5 files changed, 325 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
 create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c

-- 
2.25.1

[PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Dharma Balasubiramani]

Add the 'sam9x7-lvds' compatible binding, which describes the
Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
define the properties and configuration for the LVDS Controller in DT.

Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
 .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
new file mode 100644
index 000000000000..8c2c5b858c85
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip SAM9X7 LVDS Controller
+
+maintainers:
+  - Dharma Balasubiramani <dharma.b@microchip.com>
+
+description: |
+  The Low Voltage Differential Signaling Controller (LVDSC) manages data
+  format conversion from the LCD Controller internal DPI bus to OpenLDI
+  LVDS output signals. LVDSC functions include bit mapping, balanced mode
+  management, and serializer.
+
+properties:
+  compatible:
+    const: microchip,sam9x7-lvds
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Peripheral Bus Clock
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: gclk
+    minItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/at91.h>
+    #include <dt-bindings/dma/at91.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    lvds-controller@f8060000 {
+      compatible = "microchip,sam9x7-lvds";
+      reg = <0xf8060000 0x100>;
+      interrupts = <56 IRQ_TYPE_LEVEL_HIGH 0>;
+      clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
+      clock-names = "pclk";
+    };
-- 
2.25.1

[PATCH 2/3] drm/bridge: add lvds controller support for sam9x7

[Dharma Balasubiramani]

Add a new LVDS controller driver for sam9x7 which does the following:
- Prepares and enables the LVDS Peripheral clock
- Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
to the global bridge list.
- Identifies its output endpoint as panel and adds it to the encoder
display pipeline
- Enables the LVDS serializer

Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
 drivers/gpu/drm/bridge/Kconfig          |   7 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/microchip-lvds.c | 250 ++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3e6a4e2044c0..200afb36e421 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -173,6 +173,13 @@ config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
 	  to DP++. This is used with the i.MX6 imx-ldb
 	  driver. You are likely to say N here.
 
+config DRM_MICROCHIP_LVDS_SERIALIZER
+	tristate "Microchip LVDS serailzer support"
+	depends on OF
+	depends on DRM_ATMEL_HLCDC
+	help
+	  Support for Microchip's LVDS serializer.
+
 config DRM_NWL_MIPI_DSI
 	tristate "Northwest Logic MIPI DSI Host controller"
 	depends on DRM
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 2b892b7ed59e..e3804e93d324 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
 obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
 obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
+obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o
diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c b/drivers/gpu/drm/bridge/microchip-lvds.c
new file mode 100644
index 000000000000..297f5ae514f6
--- /dev/null
+++ b/drivers/gpu/drm/bridge/microchip-lvds.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Manikandan Muralidharan <manikandan.m@microchip.com>
+ * Author: Dharma Balasubiramani <dharma.b@microchip.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_graph.h>
+#include <linux/pinctrl/devinfo.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#define LVDS_POLL_TIMEOUT_MS 1000
+
+/* LVDSC register offsets */
+#define LVDSC_CR	0x00
+#define LVDSC_CFGR	0x04
+#define LVDSC_SR	0x0C
+#define LVDSC_WPMR	0xE4
+
+/* Bitfields in LVDSC_CR (Control Register) */
+#define LVDSC_CR_SER_EN	BIT(0)
+
+/* Bitfields in LVDSC_CFGR (Configuration Register) */
+#define LVDSC_CFGR_PIXSIZE_24BITS	0
+#define LVDSC_CFGR_DEN_POL_HIGH		0
+#define LVDSC_CFGR_DC_UNBALANCED	0
+#define LVDSC_CFGR_MAPPING_JEIDA	BIT(6)
+
+/*Bitfields in LVDSC_SR */
+#define LVDSC_SR_CS	BIT(0)
+
+/* Bitfields in LVDSC_WPMR (Write Protection Mode Register) */
+#define LVDSC_WPMR_WPKEY_MASK	GENMASK(31, 8)
+#define LVDSC_WPMR_WPKEY_PSSWD	0x4C5644
+
+struct mchp_lvds {
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *pclk;
+	int format; /* vesa or jeida format */
+	struct drm_panel *panel;
+	struct drm_bridge bridge;
+	struct drm_bridge *panel_bridge;
+};
+
+static inline struct mchp_lvds *bridge_to_lvds(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct mchp_lvds, bridge);
+}
+
+static inline u32 lvds_readl(struct mchp_lvds *lvds, u32 offset)
+{
+	return readl_relaxed(lvds->regs + offset);
+}
+
+static inline void lvds_writel(struct mchp_lvds *lvds, u32 offset, u32 val)
+{
+	writel_relaxed(val, lvds->regs + offset);
+}
+
+static void lvds_serialiser_on(struct mchp_lvds *lvds)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
+
+	/* The LVDSC registers can only be written if WPEN is cleared */
+	lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
+				LVDSC_WPMR_WPKEY_MASK));
+
+	/* Wait for the status of configuration registers to be changed */
+	while (lvds_readl(lvds, LVDSC_SR) & LVDSC_SR_CS) {
+		if (time_after(jiffies, timeout)) {
+			dev_err(lvds->dev, "%s: timeout error\n", __func__);
+			return;
+		}
+		usleep_range(1000, 2000);
+	}
+
+	/* Configure the LVDSC */
+	lvds_writel(lvds, LVDSC_CFGR, (LVDSC_CFGR_MAPPING_JEIDA |
+				LVDSC_CFGR_DC_UNBALANCED |
+				LVDSC_CFGR_DEN_POL_HIGH |
+				LVDSC_CFGR_PIXSIZE_24BITS));
+
+	/* Enable the LVDS serializer */
+	lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
+}
+
+static int mchp_lvds_attach(struct drm_bridge *bridge,
+			    enum drm_bridge_attach_flags flags)
+{
+	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+
+	bridge->encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
+
+	return drm_bridge_attach(bridge->encoder, lvds->panel_bridge,
+				 bridge, flags);
+}
+
+static void mchp_lvds_enable(struct drm_bridge *bridge)
+{
+	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+	int ret;
+
+	ret = clk_enable(lvds->pclk);
+	if (ret < 0) {
+		DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
+		return;
+	}
+
+	ret = pm_runtime_get_sync(lvds->dev);
+	if (ret < 0) {
+		DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
+		clk_disable(lvds->pclk);
+		return;
+	}
+
+	lvds_serialiser_on(lvds);
+}
+
+static void mchp_lvds_disable(struct drm_bridge *bridge)
+{
+	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+
+	pm_runtime_put(lvds->dev);
+	clk_disable(lvds->pclk);
+}
+
+static const struct drm_bridge_funcs mchp_lvds_bridge_funcs = {
+	.attach = mchp_lvds_attach,
+	.enable = mchp_lvds_enable,
+	.disable = mchp_lvds_disable,
+};
+
+static int mchp_lvds_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mchp_lvds *lvds;
+	struct resource *res;
+	struct device_node *port;
+	int ret;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
+	if (!lvds)
+		return -ENOMEM;
+
+	lvds->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lvds->regs = devm_ioremap_resource(lvds->dev, res);
+	if (IS_ERR(lvds->regs))
+		return PTR_ERR(lvds->regs);
+
+	lvds->pclk = devm_clk_get(lvds->dev, "pclk");
+	if (IS_ERR(lvds->pclk)) {
+		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
+		return PTR_ERR(lvds->pclk);
+	}
+
+	ret = clk_prepare(lvds->pclk);
+	if (ret < 0) {
+		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
+		return ret;
+	}
+
+	port = of_graph_get_remote_node(dev->of_node, 1, 0);
+	if (!port) {
+		DRM_DEV_ERROR(dev,
+			      "can't find port point, please init lvds panel port!\n");
+		return -EINVAL;
+	}
+
+	lvds->panel = of_drm_find_panel(port);
+	of_node_put(port);
+
+	if (IS_ERR(lvds->panel)) {
+		DRM_DEV_ERROR(dev, "failed to find panel node\n");
+		return -EPROBE_DEFER;
+	}
+
+	lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
+
+	if (IS_ERR(lvds->panel_bridge))
+		return PTR_ERR(lvds->panel_bridge);
+
+	lvds->bridge.of_node = dev->of_node;
+	lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
+	lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
+
+	dev_set_drvdata(dev, lvds);
+	pm_runtime_enable(dev);
+
+	drm_bridge_add(&lvds->bridge);
+
+	return 0;
+}
+
+static int mchp_lvds_remove(struct platform_device *pdev)
+{
+	struct mchp_lvds *lvds = platform_get_drvdata(pdev);
+
+	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(lvds->pclk);
+
+	return 0;
+}
+
+static const struct of_device_id mchp_lvds_dt_ids[] = {
+	{
+		.compatible = "microchip,sam9x7-lvds",
+	},
+	{},
+};
+
+struct platform_driver mchp_lvds_driver = {
+	.probe = mchp_lvds_probe,
+	.remove = mchp_lvds_remove,
+	.driver = {
+		   .name = "microchip-lvds",
+		   .of_match_table = mchp_lvds_dt_ids,
+	},
+};
+module_platform_driver(mchp_lvds_driver);
+
+MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
+MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
+MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:microchip-lvds");
-- 
2.25.1

[PATCH 3/3] MAINTAINERS: add SAM9X7 SoC's LVDS controller

[Dharma Balasubiramani]

Add the newly added LVDS controller for the SAM9X7 SoC to the existing
MAINTAINERS entry.

Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a7c4cf8201e0..24a266d20df6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14230,6 +14230,14 @@ S:	Supported
 F:	Documentation/devicetree/bindings/power/reset/atmel,sama5d2-shdwc.yaml
 F:	drivers/power/reset/at91-sama5d2_shdwc.c
 
+MICROCHIP SAM9x7-COMPATIBLE LVDS CONTROLLER
+M:	Manikandan Muralidharan <manikandan.m@microchip.com>
+M:	Dharma Balasubiramani <dharma.b@microchip.com>
+L:	dri-devel@lists.freedesktop.org
+S:	Supported
+F:	Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
+F:	drivers/gpu/drm/bridge/microchip-lvds.c
+
 MICROCHIP SOC DRIVERS
 M:	Conor Dooley <conor@kernel.org>
 S:	Supported
-- 
2.25.1

Re: [PATCH 2/3] drm/bridge: add lvds controller support for sam9x7

[Krzysztof Kozlowski]

On 22/01/2024 09:29, Dharma Balasubiramani wrote:
> Add a new LVDS controller driver for sam9x7 which does the following:
> - Prepares and enables the LVDS Peripheral clock
> - Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
> to the global bridge list.
> - Identifies its output endpoint as panel and adds it to the encoder
> display pipeline
> - Enables the LVDS serializer
> 
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---

...

> +
> +static int mchp_lvds_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mchp_lvds *lvds;
> +	struct resource *res;
> +	struct device_node *port;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	lvds->regs = devm_ioremap_resource(lvds->dev, res);

Why not combining these two?

> +	if (IS_ERR(lvds->regs))
> +		return PTR_ERR(lvds->regs);
> +
> +	lvds->pclk = devm_clk_get(lvds->dev, "pclk");
> +	if (IS_ERR(lvds->pclk)) {
> +		DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");

Handle properly deferred probe. What's DRM wrapper over dev_err_probe()?

> +		return PTR_ERR(lvds->pclk);
> +	}
> +
> +	ret = clk_prepare(lvds->pclk);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
> +		return ret;
> +	}
> +
> +	port = of_graph_get_remote_node(dev->of_node, 1, 0);
> +	if (!port) {
> +		DRM_DEV_ERROR(dev,
> +			      "can't find port point, please init lvds panel port!\n");
> +		return -EINVAL;
> +	}
> +
> +	lvds->panel = of_drm_find_panel(port);
> +	of_node_put(port);
> +
> +	if (IS_ERR(lvds->panel)) {
> +		DRM_DEV_ERROR(dev, "failed to find panel node\n");
> +		return -EPROBE_DEFER;

OK, that's for sure wrong. Don't print anything on deferred probe.

> +	}
> +
> +	lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
> +
> +	if (IS_ERR(lvds->panel_bridge))
> +		return PTR_ERR(lvds->panel_bridge);
> +
> +	lvds->bridge.of_node = dev->of_node;
> +	lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
> +	lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
> +
> +	dev_set_drvdata(dev, lvds);
> +	pm_runtime_enable(dev);
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static int mchp_lvds_remove(struct platform_device *pdev)
> +{
> +	struct mchp_lvds *lvds = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	clk_unprepare(lvds->pclk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mchp_lvds_dt_ids[] = {
> +	{
> +		.compatible = "microchip,sam9x7-lvds",
> +	},
> +	{},
> +};
> +
> +struct platform_driver mchp_lvds_driver = {
> +	.probe = mchp_lvds_probe,
> +	.remove = mchp_lvds_remove,
> +	.driver = {
> +		   .name = "microchip-lvds",
> +		   .of_match_table = mchp_lvds_dt_ids,
> +	},
> +};
> +module_platform_driver(mchp_lvds_driver);
> +
> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:microchip-lvds");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Krzysztof Kozlowski]

On 22/01/2024 09:29, Dharma Balasubiramani wrote:
> Add the 'sam9x7-lvds' compatible binding, which describes the
> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
> define the properties and configuration for the LVDS Controller in DT.
> 
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
>  .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> new file mode 100644
> index 000000000000..8c2c5b858c85
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip SAM9X7 LVDS Controller

What is the "X"?

> +
> +maintainers:
> +  - Dharma Balasubiramani <dharma.b@microchip.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
> +  management, and serializer.
> +
> +properties:
> +  compatible:
> +    const: microchip,sam9x7-lvds

What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
version number.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Peripheral Bus Clock
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: gclk
> +    minItems: 1

No, you just said you have one clock.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/dma/at91.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>

This header is not used. Include only used ones (and missing interrupt).

> +
> +    lvds-controller@f8060000 {
> +      compatible = "microchip,sam9x7-lvds";
> +      reg = <0xf8060000 0x100>;
> +      interrupts = <56 IRQ_TYPE_LEVEL_HIGH 0>;

What is "0"?

> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
> +      clock-names = "pclk";
> +    };

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Conor Dooley]

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

On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
> > Add the 'sam9x7-lvds' compatible binding, which describes the
> > Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
> > sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
> > define the properties and configuration for the LVDS Controller in DT.
> > 
> > Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> > ---
> >  .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> > new file mode 100644
> > index 000000000000..8c2c5b858c85
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip SAM9X7 LVDS Controller
> 
> What is the "X"?
> 
> > +
> > +maintainers:
> > +  - Dharma Balasubiramani <dharma.b@microchip.com>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
> > +  format conversion from the LCD Controller internal DPI bus to OpenLDI
> > +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
> > +  management, and serializer.
> > +
> > +properties:
> > +  compatible:
> > +    const: microchip,sam9x7-lvds
> 
> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
> version number.

These SoCs actually do have an x in their name. However, and I do always
get confused here, the sam9x7 is a series of SoCs (the cover letter does
say this) rather than a specific device.
I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
The devices are largely similar, but I am not sure if the sam9x70
supports LVDS at all. Having a compatible for the series does not seem
correct to me.

Cheers,
Conor.

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

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Dharma.B]

Hi Krzysztof,

On 22/01/24 9:21 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>> Add the 'sam9x7-lvds' compatible binding, which describes the
>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
>> define the properties and configuration for the LVDS Controller in DT.
>>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
>>   .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>> new file mode 100644
>> index 000000000000..8c2c5b858c85
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>> @@ -0,0 +1,59 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip SAM9X7 LVDS Controller
> 
> What is the "X
Answered below
> 
>> +
>> +maintainers:
>> +  - Dharma Balasubiramani <dharma.b@microchip.com>
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.
Sure, I will drop it.
> 
>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
>> +  management, and serializer.
>> +
>> +properties:
>> +  compatible:
>> +    const: microchip,sam9x7-lvds
> 
> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
> version number.
The term 'X' doesn't serve as a wildcard; rather, it directly represents 
the name of the SoC series, I should use sam9x75,sam9x72 instead of sam9x7.
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Peripheral Bus Clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: pclk
>> +      - const: gclk
>> +    minItems: 1
> 
> No, you just said you have one clock.
Certainly, I need to exclude the gclk. Thanks.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/at91.h>
>> +    #include <dt-bindings/dma/at91.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> 
> This header is not used. Include only used ones (and missing interrupt).
> 
>> +
>> +    lvds-controller@f8060000 {
>> +      compatible = "microchip,sam9x7-lvds";
>> +      reg = <0xf8060000 0x100>;
>> +      interrupts = <56 IRQ_TYPE_LEVEL_HIGH 0>;
> 
> What is "0"?

Please refer 
"Documentation/devicetree/bindings/interrupt-controller/atmel,aic.txt"
The third cell is used to specify the irq priority from 0 (lowest) to 
7(highest).

-- 
With Best Regards,
Dharma B.
> 
>> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
>> +      clock-names = "pclk";
>> +    };
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Dharma.B]

Hi Conor,

On 22/01/24 10:07 pm, Conor Dooley wrote:
> On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
>> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>>> Add the 'sam9x7-lvds' compatible binding, which describes the
>>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
>>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
>>> define the properties and configuration for the LVDS Controller in DT.
>>>
>>> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
>>> ---
>>>   .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>>>   1 file changed, 59 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>> new file mode 100644
>>> index 000000000000..8c2c5b858c85
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>> @@ -0,0 +1,59 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Microchip SAM9X7 LVDS Controller
>> What is the "X"?
>>
>>> +
>>> +maintainers:
>>> +  - Dharma Balasubiramani<dharma.b@microchip.com>
>>> +
>>> +description: |
>> Do not need '|' unless you need to preserve formatting.
>>
>>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
>>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
>>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
>>> +  management, and serializer.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: microchip,sam9x7-lvds
>> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
>> version number.
> These SoCs actually do have an x in their name. However, and I do always
> get confused here, the sam9x7 is a series of SoCs (the cover letter does
> say this) rather than a specific device.
> I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
> The devices are largely similar, but I am not sure if the sam9x70
> supports LVDS at all. Having a compatible for the series does not seem
> correct to me.
Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while 
sam9x70 does not. I will revise the compatibility to include both 
sam9x72 and sam9x75, as outlined below:

properties:
   compatible:
     enum:
       - microchip,sam9x72-lvds
       - microchip,sam9x75-lvds

Additionally, I will update the driver accordingly. Thank you.

-- 
With Best Regards,
Dharma B.
> 
> Cheers,
> Conor.

Re: [PATCH 2/3] drm/bridge: add lvds controller support for sam9x7

[Dharma.B]

Hi Krzysztof,

On 22/01/24 9:19 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>> Add a new LVDS controller driver for sam9x7 which does the following:
>> - Prepares and enables the LVDS Peripheral clock
>> - Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
>> to the global bridge list.
>> - Identifies its output endpoint as panel and adds it to the encoder
>> display pipeline
>> - Enables the LVDS serializer
>>
>> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
> 
> ...
> 
>> +
>> +static int mchp_lvds_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct mchp_lvds *lvds;
>> +     struct resource *res;
>> +     struct device_node *port;
>> +     int ret;
>> +
>> +     if (!dev->of_node)
>> +             return -ENODEV;
>> +
>> +     lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>> +     if (!lvds)
>> +             return -ENOMEM;
>> +
>> +     lvds->dev = dev;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     lvds->regs = devm_ioremap_resource(lvds->dev, res);
> 
> Why not combining these two?

It seems reasonable to combine these two lines since the resource 
variable (res) is only used at this point. I'll proceed with 
consolidating these lines for simplicity.

> 
>> +     if (IS_ERR(lvds->regs))
>> +             return PTR_ERR(lvds->regs);
>> +
>> +     lvds->pclk = devm_clk_get(lvds->dev, "pclk");
>> +     if (IS_ERR(lvds->pclk)) {
>> +             DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
> 
> Handle properly deferred probe. What's DRM wrapper over dev_err_probe()?
Sure, I will use dev_err_probe()

return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), "could not get 
pclk_lvds\n");

> 
>> +             return PTR_ERR(lvds->pclk);
>> +     }
>> +
>> +     ret = clk_prepare(lvds->pclk);
>> +     if (ret < 0) {
>> +             DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
>> +             return ret;
>> +     }
>> +
>> +     port = of_graph_get_remote_node(dev->of_node, 1, 0);
>> +     if (!port) {
>> +             DRM_DEV_ERROR(dev,
>> +                           "can't find port point, please init lvds panel port!\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     lvds->panel = of_drm_find_panel(port);
>> +     of_node_put(port);
>> +
>> +     if (IS_ERR(lvds->panel)) {
>> +             DRM_DEV_ERROR(dev, "failed to find panel node\n");
>> +             return -EPROBE_DEFER;
> 
> OK, that's for sure wrong. Don't print anything on deferred probe.
Sure, I will drop the print here.
> 
>> +     }
>> +
>> +     lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
>> +
>> +     if (IS_ERR(lvds->panel_bridge))
>> +             return PTR_ERR(lvds->panel_bridge);
>> +
>> +     lvds->bridge.of_node = dev->of_node;
>> +     lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>> +     lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
>> +
>> +     dev_set_drvdata(dev, lvds);
>> +     pm_runtime_enable(dev);
>> +
>> +     drm_bridge_add(&lvds->bridge);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_lvds_remove(struct platform_device *pdev)
>> +{
>> +     struct mchp_lvds *lvds = platform_get_drvdata(pdev);
>> +
>> +     pm_runtime_disable(&pdev->dev);
>> +     clk_unprepare(lvds->pclk);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id mchp_lvds_dt_ids[] = {
>> +     {
>> +             .compatible = "microchip,sam9x7-lvds",
>> +     },
>> +     {},
>> +};
>> +
>> +struct platform_driver mchp_lvds_driver = {
>> +     .probe = mchp_lvds_probe,
>> +     .remove = mchp_lvds_remove,
>> +     .driver = {
>> +                .name = "microchip-lvds",
>> +                .of_match_table = mchp_lvds_dt_ids,
>> +     },
>> +};
>> +module_platform_driver(mchp_lvds_driver);
>> +
>> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
>> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
>> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:microchip-lvds");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.

Okay, I will remove the MODULE_ALIAS and update the mchp_lvds_dt_ids[] 
as below along with MODULE_DEVICE_TABLE()

static const struct of_device_id mchp_lvds_dt_ids[] = {
         {
                 .compatible = "microchip,sam9x72-lvds",
         },
         {
                 .compatible = "microchip,sam9x75-lvds",
         },
         {},
};
MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);

-- 
Thanks,
Dharma B.
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Rob Herring]

On Tue, Jan 23, 2024 at 03:39:13AM +0000, Dharma.B@microchip.com wrote:
> Hi Conor,
> 
> On 22/01/24 10:07 pm, Conor Dooley wrote:
> > On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
> >> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
> >>> Add the 'sam9x7-lvds' compatible binding, which describes the
> >>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
> >>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
> >>> define the properties and configuration for the LVDS Controller in DT.
> >>>
> >>> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
> >>> ---
> >>>   .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
> >>>   1 file changed, 59 insertions(+)
> >>>   create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> >>> new file mode 100644
> >>> index 000000000000..8c2c5b858c85
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
> >>> @@ -0,0 +1,59 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
> >>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Microchip SAM9X7 LVDS Controller
> >> What is the "X"?
> >>
> >>> +
> >>> +maintainers:
> >>> +  - Dharma Balasubiramani<dharma.b@microchip.com>
> >>> +
> >>> +description: |
> >> Do not need '|' unless you need to preserve formatting.
> >>
> >>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
> >>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
> >>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
> >>> +  management, and serializer.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: microchip,sam9x7-lvds
> >> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
> >> version number.
> > These SoCs actually do have an x in their name. However, and I do always
> > get confused here, the sam9x7 is a series of SoCs (the cover letter does
> > say this) rather than a specific device.
> > I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
> > The devices are largely similar, but I am not sure if the sam9x70
> > supports LVDS at all. Having a compatible for the series does not seem
> > correct to me.
> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while 
> sam9x70 does not. I will revise the compatibility to include both 
> sam9x72 and sam9x75, as outlined below:
> 
> properties:
>    compatible:
>      enum:
>        - microchip,sam9x72-lvds
>        - microchip,sam9x75-lvds

I would presume these 2 are the same, but the above implies they 
aren't. I think what you had is fine assuming these are all 
fundamentally the same part with just packaging or fused off h/w 
differences.

Rob

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Dharma.B]

On 31/01/24 12:42 am, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jan 23, 2024 at 03:39:13AM +0000, Dharma.B@microchip.com wrote:
>> Hi Conor,
>>
>> On 22/01/24 10:07 pm, Conor Dooley wrote:
>>> On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
>>>> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>>>>> Add the 'sam9x7-lvds' compatible binding, which describes the
>>>>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
>>>>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
>>>>> define the properties and configuration for the LVDS Controller in DT.
>>>>>
>>>>> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
>>>>> ---
>>>>>    .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>>>>>    1 file changed, 59 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..8c2c5b858c85
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>> @@ -0,0 +1,59 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
>>>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Microchip SAM9X7 LVDS Controller
>>>> What is the "X"?
>>>>
>>>>> +
>>>>> +maintainers:
>>>>> +  - Dharma Balasubiramani<dharma.b@microchip.com>
>>>>> +
>>>>> +description: |
>>>> Do not need '|' unless you need to preserve formatting.
>>>>
>>>>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
>>>>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
>>>>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
>>>>> +  management, and serializer.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: microchip,sam9x7-lvds
>>>> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
>>>> version number.
>>> These SoCs actually do have an x in their name. However, and I do always
>>> get confused here, the sam9x7 is a series of SoCs (the cover letter does
>>> say this) rather than a specific device.
>>> I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
>>> The devices are largely similar, but I am not sure if the sam9x70
>>> supports LVDS at all. Having a compatible for the series does not seem
>>> correct to me.
>> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while
>> sam9x70 does not. I will revise the compatibility to include both
>> sam9x72 and sam9x75, as outlined below:
>>
>> properties:
>>     compatible:
>>       enum:
>>         - microchip,sam9x72-lvds
>>         - microchip,sam9x75-lvds
> 
> I would presume these 2 are the same, but the above implies they
> aren't. I think what you had is fine assuming these are all
> fundamentally the same part with just packaging or fused off h/w
> differences.

Yes, so is it okay to have compatible for a series? Shall I go ahead with
"
   compatible:
     const: microchip,sam9x7-lvds
"
itself?

-- 
Thanks,
Dharma B.
> 
> Rob

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Krzysztof Kozlowski]

On 01/02/2024 05:10, Dharma.B@microchip.com wrote:
> On 31/01/24 12:42 am, Rob Herring wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On Tue, Jan 23, 2024 at 03:39:13AM +0000, Dharma.B@microchip.com wrote:
>>> Hi Conor,
>>>
>>> On 22/01/24 10:07 pm, Conor Dooley wrote:
>>>> On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
>>>>> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>>>>>> Add the 'sam9x7-lvds' compatible binding, which describes the
>>>>>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
>>>>>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
>>>>>> define the properties and configuration for the LVDS Controller in DT.
>>>>>>
>>>>>> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
>>>>>> ---
>>>>>>    .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>>>>>>    1 file changed, 59 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..8c2c5b858c85
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>> @@ -0,0 +1,59 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
>>>>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Microchip SAM9X7 LVDS Controller
>>>>> What is the "X"?
>>>>>
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Dharma Balasubiramani<dharma.b@microchip.com>
>>>>>> +
>>>>>> +description: |
>>>>> Do not need '|' unless you need to preserve formatting.
>>>>>
>>>>>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
>>>>>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
>>>>>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
>>>>>> +  management, and serializer.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: microchip,sam9x7-lvds
>>>>> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
>>>>> version number.
>>>> These SoCs actually do have an x in their name. However, and I do always
>>>> get confused here, the sam9x7 is a series of SoCs (the cover letter does
>>>> say this) rather than a specific device.
>>>> I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
>>>> The devices are largely similar, but I am not sure if the sam9x70
>>>> supports LVDS at all. Having a compatible for the series does not seem
>>>> correct to me.
>>> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while
>>> sam9x70 does not. I will revise the compatibility to include both
>>> sam9x72 and sam9x75, as outlined below:
>>>
>>> properties:
>>>     compatible:
>>>       enum:
>>>         - microchip,sam9x72-lvds
>>>         - microchip,sam9x75-lvds
>>
>> I would presume these 2 are the same, but the above implies they
>> aren't. I think what you had is fine assuming these are all
>> fundamentally the same part with just packaging or fused off h/w
>> differences.
> 
> Yes, so is it okay to have compatible for a series? Shall I go ahead with
> "
>    compatible:
>      const: microchip,sam9x7-lvds

You said 9x70, which would match such 9x7 "series", is different, so I
still think series should not be used. I don't know much about Microchip
naming scheme, so this x is always confusing. However if these are the
same, maybe just use sam9x72?

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: display: bridge: add sam9x7-lvds compatible

[Dharma.B]

On 01/02/24 1:09 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 01/02/2024 05:10, Dharma.B@microchip.com wrote:
>> On 31/01/24 12:42 am, Rob Herring wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Tue, Jan 23, 2024 at 03:39:13AM +0000, Dharma.B@microchip.com wrote:
>>>> Hi Conor,
>>>>
>>>> On 22/01/24 10:07 pm, Conor Dooley wrote:
>>>>> On Mon, Jan 22, 2024 at 04:51:16PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>>>>>>> Add the 'sam9x7-lvds' compatible binding, which describes the
>>>>>>> Low Voltage Differential Signaling (LVDS) Controller found on Microchip's
>>>>>>> sam9x7 series System-on-Chip (SoC) devices. This binding will be used to
>>>>>>> define the properties and configuration for the LVDS Controller in DT.
>>>>>>>
>>>>>>> Signed-off-by: Dharma Balasubiramani<dharma.b@microchip.com>
>>>>>>> ---
>>>>>>>     .../display/bridge/microchip,sam9x7-lvds.yaml | 59 +++++++++++++++++++
>>>>>>>     1 file changed, 59 insertions(+)
>>>>>>>     create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..8c2c5b858c85
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
>>>>>>> @@ -0,0 +1,59 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id:http://devicetree.org/schemas/display/bridge/microchip,sam9x7-lvds.yaml#
>>>>>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Microchip SAM9X7 LVDS Controller
>>>>>> What is the "X"?
>>>>>>
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Dharma Balasubiramani<dharma.b@microchip.com>
>>>>>>> +
>>>>>>> +description: |
>>>>>> Do not need '|' unless you need to preserve formatting.
>>>>>>
>>>>>>> +  The Low Voltage Differential Signaling Controller (LVDSC) manages data
>>>>>>> +  format conversion from the LCD Controller internal DPI bus to OpenLDI
>>>>>>> +  LVDS output signals. LVDSC functions include bit mapping, balanced mode
>>>>>>> +  management, and serializer.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: microchip,sam9x7-lvds
>>>>>> What is "x"? Wildcard? Then no, don't use it and instead use proper SoC
>>>>>> version number.
>>>>> These SoCs actually do have an x in their name. However, and I do always
>>>>> get confused here, the sam9x7 is a series of SoCs (the cover letter does
>>>>> say this) rather than a specific device.
>>>>> I think the series current consists of a sam9x70 sam9x72 and a sam9x75.
>>>>> The devices are largely similar, but I am not sure if the sam9x70
>>>>> supports LVDS at all. Having a compatible for the series does not seem
>>>>> correct to me.
>>>> Yes, you are correct. Only sam9x72 and sam9x75 have LVDS support, while
>>>> sam9x70 does not. I will revise the compatibility to include both
>>>> sam9x72 and sam9x75, as outlined below:
>>>>
>>>> properties:
>>>>      compatible:
>>>>        enum:
>>>>          - microchip,sam9x72-lvds
>>>>          - microchip,sam9x75-lvds
>>>
>>> I would presume these 2 are the same, but the above implies they
>>> aren't. I think what you had is fine assuming these are all
>>> fundamentally the same part with just packaging or fused off h/w
>>> differences.
>>
>> Yes, so is it okay to have compatible for a series? Shall I go ahead with
>> "
>>     compatible:
>>       const: microchip,sam9x7-lvds
> 
> You said 9x70, which would match such 9x7 "series", is different, so I
> still think series should not be used. I don't know much about Microchip
> naming scheme, so this x is always confusing. However if these are the
> same, maybe just use sam9x72?

sam9x75 is the first board to be available publicly; hence, I shall use 
the "microchip,sam9x75-lvds" compatible and reuse the same in other 
boards that (features same IP) will come later.

> 
> Best regards,
> Krzysztof
> 

-- 
With Best Regards,
Dharma B.

Re: [PATCH 2/3] drm/bridge: add lvds controller support for sam9x7

[kernel test robot]

Hi Dharma,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.8-rc2 next-20240202]
[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/Dharma-Balasubiramani/dt-bindings-display-bridge-add-sam9x7-lvds-compatible/20240122-163209
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240122082947.21645-3-dharma.b%40microchip.com
patch subject: [PATCH 2/3] drm/bridge: add lvds controller support for sam9x7
config: arm-randconfig-r112-20240203 (https://download.01.org/0day-ci/archive/20240203/202402032248.6puqAuzM-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240203/202402032248.6puqAuzM-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/202402032248.6puqAuzM-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/bridge/microchip-lvds.c:236:24: sparse: sparse: symbol 'mchp_lvds_driver' was not declared. Should it be static?

vim +/mchp_lvds_driver +236 drivers/gpu/drm/bridge/microchip-lvds.c

   235	
 > 236	struct platform_driver mchp_lvds_driver = {
   237		.probe = mchp_lvds_probe,
   238		.remove = mchp_lvds_remove,
   239		.driver = {
   240			   .name = "microchip-lvds",
   241			   .of_match_table = mchp_lvds_dt_ids,
   242		},
   243	};
   244	module_platform_driver(mchp_lvds_driver);
   245	

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