[PATCH v6 0/4] LVDS Controller Support for SAM9X75 SoC

[PATCH v6 0/4] LVDS Controller Support for SAM9X75 SoC

[Dharma Balasubiramani]

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

Patch series Changelog:
- Include configs: at91: Enable LVDS serializer
- include all necessary To/Cc entries.
The Individual Changelogs are available on the respective patches.

Dharma Balasubiramani (4):
  dt-bindings: display: bridge: add sam9x75-lvds binding
  drm/bridge: add lvds controller support for sam9x7
  MAINTAINERS: add SAM9X7 SoC's LVDS controller
  ARM: configs: at91: Enable LVDS serializer support

 .../bridge/microchip,sam9x75-lvds.yaml        |  55 +++++
 MAINTAINERS                                   |   8 +
 arch/arm/configs/at91_dt_defconfig            |   1 +
 drivers/gpu/drm/bridge/Kconfig                |   7 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/microchip-lvds.c       | 228 ++++++++++++++++++
 6 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x75-lvds.yaml
 create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c

-- 
2.25.1

[PATCH v6 1/4] dt-bindings: display: bridge: add sam9x75-lvds binding

[Dharma Balasubiramani]

Add the 'sam9x75-lvds' compatible binding, which describes the Low Voltage
Differential Signaling (LVDS) Controller found on some 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changelog
v5 -> v6
- No Changes.
v4 -> v5
- No changes.
v3 -> v4
- Rephrase the commit subject.
v2 -> v3
- No changes.
v1 -> v2
- Remove '|' in description, as there is no formatting to preserve.
- Remove 'gclk' from clock-names as there is only one clock(pclk).
- Remove the unused headers and include only used ones.
- Change the compatible name specific to SoC (sam9x75) instead of entire series.
- Change file name to match the compatible name.
---
 .../bridge/microchip,sam9x75-lvds.yaml        | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x75-lvds.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/microchip,sam9x75-lvds.yaml b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x75-lvds.yaml
new file mode 100644
index 000000000000..862ef441ac9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/microchip,sam9x75-lvds.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/microchip,sam9x75-lvds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip SAM9X75 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,sam9x75-lvds
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Peripheral Bus Clock
+
+  clock-names:
+    items:
+      - const: pclk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/at91.h>
+    lvds-controller@f8060000 {
+      compatible = "microchip,sam9x75-lvds";
+      reg = <0xf8060000 0x100>;
+      interrupts = <56 IRQ_TYPE_LEVEL_HIGH 0>;
+      clocks = <&pmc PMC_TYPE_PERIPHERAL 56>;
+      clock-names = "pclk";
+    };
-- 
2.25.1

[PATCH v6 2/4] 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>
---
Changelog
v5 -> v6
- No Changes.
v4 -> v5
- Drop the unused variable 'format'.
- Use DRM wrapper for dev_err() to maintain uniformity.
- return -ENODEV instead of -EINVAL to maintain consistency with other DRM
  bridge drivers.
v3 -> v4
- No changes.
v2 ->v3
- Correct Typo error "serializer".
- Consolidate get() and prepare() functions and use devm_clk_get_prepared().
- Remove unused variable 'ret' in probe().
- Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
v1 -> v2
- Drop 'res' variable and combine two lines into one.
- Handle deferred probe properly, use dev_err_probe().
- Don't print anything on deferred probe. Dropped print.
- Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
- symbol 'mchp_lvds_driver' was not declared. It should be static.
---
 drivers/gpu/drm/bridge/Kconfig          |   7 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
 3 files changed, 236 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 efd996f6c138..889098e2d65f 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -190,6 +190,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 serializer 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 017b5832733b..7df87b582dca 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -13,6 +13,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..149704f498a6
--- /dev/null
+++ b/drivers/gpu/drm/bridge/microchip-lvds.c
@@ -0,0 +1,228 @@
+// 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;
+	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)) {
+			DRM_DEV_ERROR(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 device_node *port;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
+	if (!lvds)
+		return -ENOMEM;
+
+	lvds->dev = dev;
+
+	lvds->regs = devm_ioremap_resource(lvds->dev,
+			platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(lvds->regs))
+		return PTR_ERR(lvds->regs);
+
+	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");
+	if (IS_ERR(lvds->pclk))
+		return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
+				"could not get pclk_lvds prepared\n");
+
+	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 -ENODEV;
+	}
+
+	lvds->panel = of_drm_find_panel(port);
+	of_node_put(port);
+
+	if (IS_ERR(lvds->panel))
+		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);
+	devm_pm_runtime_enable(dev);
+
+	drm_bridge_add(&lvds->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id mchp_lvds_dt_ids[] = {
+	{
+		.compatible = "microchip,sam9x75-lvds",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);
+
+static struct platform_driver mchp_lvds_driver = {
+	.probe = mchp_lvds_probe,
+	.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");
-- 
2.25.1

[PATCH v6 3/4] 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>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changelog
v5 -> v6
- Correct the file name sam9x7-lvds.yaml -> sam9x75-lvds.yaml.
v4 -> v5
v3 -> v4
- No changes.
v2 -> v3
- Move the entry before "MICROCHIP SAMA5D2-COMPATIBLE ADC DRIVER".
v1 -> v2
- No Changes.
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c23fda1aa1f0..e49347eac596 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14563,6 +14563,14 @@ S:	Supported
 F:	Documentation/devicetree/bindings/pwm/atmel,at91sam-pwm.yaml
 F:	drivers/pwm/pwm-atmel.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,sam9x75-lvds.yaml
+F:	drivers/gpu/drm/bridge/microchip-lvds.c
+
 MICROCHIP SAMA5D2-COMPATIBLE ADC DRIVER
 M:	Eugen Hristev <eugen.hristev@microchip.com>
 L:	linux-iio@vger.kernel.org
-- 
2.25.1

[PATCH v6 4/4] ARM: configs: at91: Enable LVDS serializer support

[Dharma Balasubiramani]

Enable LVDS serializer support for display pipeline.

Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
Acked-by: Hari Prasath Gujulan Elango <hari.prasathge@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
Changelog
v4 -> v5
v3 -> v4
v2 -> v3
- No Changes.
---
 arch/arm/configs/at91_dt_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/at91_dt_defconfig b/arch/arm/configs/at91_dt_defconfig
index 1d53aec4c836..6eabe2313c9a 100644
--- a/arch/arm/configs/at91_dt_defconfig
+++ b/arch/arm/configs/at91_dt_defconfig
@@ -143,6 +143,7 @@ CONFIG_VIDEO_OV2640=m
 CONFIG_VIDEO_OV7740=m
 CONFIG_DRM=y
 CONFIG_DRM_ATMEL_HLCDC=y
+CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER=y
 CONFIG_DRM_PANEL_SIMPLE=y
 CONFIG_DRM_PANEL_EDP=y
 CONFIG_FB_ATMEL=y
-- 
2.25.1

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

[Dmitry Baryshkov]

On Wed, Apr 17, 2024 at 08:11:35AM +0530, 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>
> ---
> Changelog
> v5 -> v6
> - No Changes.
> v4 -> v5
> - Drop the unused variable 'format'.
> - Use DRM wrapper for dev_err() to maintain uniformity.
> - return -ENODEV instead of -EINVAL to maintain consistency with other DRM
>   bridge drivers.
> v3 -> v4
> - No changes.
> v2 ->v3
> - Correct Typo error "serializer".
> - Consolidate get() and prepare() functions and use devm_clk_get_prepared().
> - Remove unused variable 'ret' in probe().
> - Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
> v1 -> v2
> - Drop 'res' variable and combine two lines into one.
> - Handle deferred probe properly, use dev_err_probe().
> - Don't print anything on deferred probe. Dropped print.
> - Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
> - symbol 'mchp_lvds_driver' was not declared. It should be static.
> ---
>  drivers/gpu/drm/bridge/Kconfig          |   7 +
>  drivers/gpu/drm/bridge/Makefile         |   1 +
>  drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
>  3 files changed, 236 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 efd996f6c138..889098e2d65f 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -190,6 +190,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 serializer 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 017b5832733b..7df87b582dca 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -13,6 +13,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..149704f498a6
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
> @@ -0,0 +1,228 @@
> +// 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;
> +	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)) {
> +			DRM_DEV_ERROR(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;

Why do you need to touch encoder_type here? It's not your bridge's
responsibility.

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

This can result in unbalanced clk_disable(), if pm_runtime_get_sync()
fails. This function calls clk_disable(), but the framework has no way
to know that .enable() was not successful and calls .disable(), which
also calls clk_disable().

Please consider turning pclk into pm_clk so that its state is managed
automatically (or at least moving clk_enable/disable into pm_ops).

> +		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 device_node *port;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = dev;
> +
> +	lvds->regs = devm_ioremap_resource(lvds->dev,
> +			platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +	if (IS_ERR(lvds->regs))
> +		return PTR_ERR(lvds->regs);
> +
> +	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");

Why do you need _prepared version?

> +	if (IS_ERR(lvds->pclk))
> +		return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
> +				"could not get pclk_lvds prepared\n");
> +
> +	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 -ENODEV;
> +	}
> +
> +	lvds->panel = of_drm_find_panel(port);
> +	of_node_put(port);
> +
> +	if (IS_ERR(lvds->panel))
> +		return -EPROBE_DEFER;
> +
> +	lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);

Please use instead:

devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);

> +
> +	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);
> +	devm_pm_runtime_enable(dev);

Error check is missing.

> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mchp_lvds_dt_ids[] = {
> +	{
> +		.compatible = "microchip,sam9x75-lvds",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);
> +
> +static struct platform_driver mchp_lvds_driver = {
> +	.probe = mchp_lvds_probe,
> +	.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");
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

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

[Dharma.B]

Hi Dmitry,

On 17/04/24 12:05 pm, Dmitry Baryshkov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Apr 17, 2024 at 08:11:35AM +0530, 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>
>> ---
>> Changelog
>> v5 -> v6
>> - No Changes.
>> v4 -> v5
>> - Drop the unused variable 'format'.
>> - Use DRM wrapper for dev_err() to maintain uniformity.
>> - return -ENODEV instead of -EINVAL to maintain consistency with other DRM
>>    bridge drivers.
>> v3 -> v4
>> - No changes.
>> v2 ->v3
>> - Correct Typo error "serializer".
>> - Consolidate get() and prepare() functions and use devm_clk_get_prepared().
>> - Remove unused variable 'ret' in probe().
>> - Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
>> v1 -> v2
>> - Drop 'res' variable and combine two lines into one.
>> - Handle deferred probe properly, use dev_err_probe().
>> - Don't print anything on deferred probe. Dropped print.
>> - Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
>> - symbol 'mchp_lvds_driver' was not declared. It should be static.
>> ---
>>   drivers/gpu/drm/bridge/Kconfig          |   7 +
>>   drivers/gpu/drm/bridge/Makefile         |   1 +
>>   drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
>>   3 files changed, 236 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 efd996f6c138..889098e2d65f 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -190,6 +190,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 serializer 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 017b5832733b..7df87b582dca 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -13,6 +13,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..149704f498a6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
>> @@ -0,0 +1,228 @@
>> +// 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;
>> +     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)) {
>> +                     DRM_DEV_ERROR(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;
> 
> Why do you need to touch encoder_type here? It's not your bridge's
> responsibility.

Indeed, I've verified it across other bridge drivers too. Thank you for 
bringing it to my attention. I will remove setting the encoder type here.

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

I will change this into clk_prepare_enable as we are dropping the 
prepared version in the probe().

>> +     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);
> 
> This can result in unbalanced clk_disable(), if pm_runtime_get_sync()
> fails. This function calls clk_disable(), but the framework has no way
> to know that .enable() was not successful and calls .disable(), which
> also calls clk_disable( >
> Please consider turning pclk into pm_clk so that its state is managed
> automatically (or at least moving clk_enable/disable into pm_ops).

This process appears rather intricate. May I suggest omitting the 
'clk_disable' operation here?

> 
>> +             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 device_node *port;
>> +
>> +     if (!dev->of_node)
>> +             return -ENODEV;
>> +
>> +     lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>> +     if (!lvds)
>> +             return -ENOMEM;
>> +
>> +     lvds->dev = dev;
>> +
>> +     lvds->regs = devm_ioremap_resource(lvds->dev,
>> +                     platform_get_resource(pdev, IORESOURCE_MEM, 0));
>> +     if (IS_ERR(lvds->regs))
>> +             return PTR_ERR(lvds->regs);
>> +
>> +     lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");
> 
> Why do you need _prepared version?

I will change this to just devm_clk_get().

> 
>> +     if (IS_ERR(lvds->pclk))
>> +             return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
>> +                             "could not get pclk_lvds prepared\n");
>> +
>> +     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 -ENODEV;
>> +     }
>> +
>> +     lvds->panel = of_drm_find_panel(port);
>> +     of_node_put(port);
>> +
>> +     if (IS_ERR(lvds->panel))
>> +             return -EPROBE_DEFER;
>> +
>> +     lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
> 
> Please use instead:
> 
> devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);

Sure, Noted. Thanks.
> 
>> +
>> +     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);
>> +     devm_pm_runtime_enable(dev);
> 
> Error check is missing.

Sure, I will add this

         ret = devm_pm_runtime_enable(dev);
         if (ret < 0) {
                 DRM_DEV_ERROR(lvds->dev, "failed to enable pm runtime: 
%d\n", ret);
                 return ret;
         }


> 
>> +
>> +     drm_bridge_add(&lvds->bridge);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id mchp_lvds_dt_ids[] = {
>> +     {
>> +             .compatible = "microchip,sam9x75-lvds",
>> +     },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);
>> +
>> +static struct platform_driver mchp_lvds_driver = {
>> +     .probe = mchp_lvds_probe,
>> +     .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");
>> --
>> 2.25.1
>>
> 
> --
> With best wishes
> Dmitry

-- 
With Best Regards,
Dharma B.

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

[Dmitry Baryshkov]

On Thu, Apr 18, 2024 at 03:54:53AM +0000, Dharma.B@microchip.com wrote:
> Hi Dmitry,
> 
> On 17/04/24 12:05 pm, Dmitry Baryshkov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Apr 17, 2024 at 08:11:35AM +0530, 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>
> >> ---
> >> Changelog
> >> v5 -> v6
> >> - No Changes.
> >> v4 -> v5
> >> - Drop the unused variable 'format'.
> >> - Use DRM wrapper for dev_err() to maintain uniformity.
> >> - return -ENODEV instead of -EINVAL to maintain consistency with other DRM
> >>    bridge drivers.
> >> v3 -> v4
> >> - No changes.
> >> v2 ->v3
> >> - Correct Typo error "serializer".
> >> - Consolidate get() and prepare() functions and use devm_clk_get_prepared().
> >> - Remove unused variable 'ret' in probe().
> >> - Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
> >> v1 -> v2
> >> - Drop 'res' variable and combine two lines into one.
> >> - Handle deferred probe properly, use dev_err_probe().
> >> - Don't print anything on deferred probe. Dropped print.
> >> - Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
> >> - symbol 'mchp_lvds_driver' was not declared. It should be static.
> >> ---
> >>   drivers/gpu/drm/bridge/Kconfig          |   7 +
> >>   drivers/gpu/drm/bridge/Makefile         |   1 +
> >>   drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
> >>   3 files changed, 236 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 efd996f6c138..889098e2d65f 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig

[skipped]

> >> +     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);
> > 
> > This can result in unbalanced clk_disable(), if pm_runtime_get_sync()
> > fails. This function calls clk_disable(), but the framework has no way
> > to know that .enable() was not successful and calls .disable(), which
> > also calls clk_disable( >
> > Please consider turning pclk into pm_clk so that its state is managed
> > automatically (or at least moving clk_enable/disable into pm_ops).
> 
> This process appears rather intricate. May I suggest omitting the 
> 'clk_disable' operation here?

Yes

> 
> > 
> >> +             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 device_node *port;
> >> +
> >> +     if (!dev->of_node)
> >> +             return -ENODEV;
> >> +
> >> +     lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> >> +     if (!lvds)
> >> +             return -ENOMEM;
> >> +
> >> +     lvds->dev = dev;
> >> +
> >> +     lvds->regs = devm_ioremap_resource(lvds->dev,
> >> +                     platform_get_resource(pdev, IORESOURCE_MEM, 0));
> >> +     if (IS_ERR(lvds->regs))
> >> +             return PTR_ERR(lvds->regs);
> >> +
> >> +     lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");
> > 
> > Why do you need _prepared version?
> 
> I will change this to just devm_clk_get().
> 
> > 
> >> +     if (IS_ERR(lvds->pclk))
> >> +             return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
> >> +                             "could not get pclk_lvds prepared\n");
> >> +
> >> +     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 -ENODEV;
> >> +     }
> >> +
> >> +     lvds->panel = of_drm_find_panel(port);
> >> +     of_node_put(port);
> >> +
> >> +     if (IS_ERR(lvds->panel))
> >> +             return -EPROBE_DEFER;
> >> +
> >> +     lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
> > 
> > Please use instead:
> > 
> > devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> 
> Sure, Noted. Thanks.
> > 
> >> +
> >> +     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);
> >> +     devm_pm_runtime_enable(dev);
> > 
> > Error check is missing.
> 
> Sure, I will add this
> 
>          ret = devm_pm_runtime_enable(dev);
>          if (ret < 0) {
>                  DRM_DEV_ERROR(lvds->dev, "failed to enable pm runtime: 

DRM_DEV_ERROR is deprecated, plese use suitable replacement.

> %d\n", ret);
>                  return ret;
>          }
> 
> 

-- 
With best wishes
Dmitry

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

[Dharma.B]

On 18/04/24 11:55 am, Dmitry Baryshkov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Apr 18, 2024 at 03:54:53AM +0000, Dharma.B@microchip.com wrote:
>> Hi Dmitry,
>>
>> On 17/04/24 12:05 pm, Dmitry Baryshkov wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Apr 17, 2024 at 08:11:35AM +0530, 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>
>>>> ---
>>>> Changelog
>>>> v5 -> v6
>>>> - No Changes.
>>>> v4 -> v5
>>>> - Drop the unused variable 'format'.
>>>> - Use DRM wrapper for dev_err() to maintain uniformity.
>>>> - return -ENODEV instead of -EINVAL to maintain consistency with other DRM
>>>>     bridge drivers.
>>>> v3 -> v4
>>>> - No changes.
>>>> v2 ->v3
>>>> - Correct Typo error "serializer".
>>>> - Consolidate get() and prepare() functions and use devm_clk_get_prepared().
>>>> - Remove unused variable 'ret' in probe().
>>>> - Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
>>>> v1 -> v2
>>>> - Drop 'res' variable and combine two lines into one.
>>>> - Handle deferred probe properly, use dev_err_probe().
>>>> - Don't print anything on deferred probe. Dropped print.
>>>> - Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
>>>> - symbol 'mchp_lvds_driver' was not declared. It should be static.
>>>> ---
>>>>    drivers/gpu/drm/bridge/Kconfig          |   7 +
>>>>    drivers/gpu/drm/bridge/Makefile         |   1 +
>>>>    drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
>>>>    3 files changed, 236 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 efd996f6c138..889098e2d65f 100644
>>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>>> +++ b/drivers/gpu/drm/bridge/Kconfig
> 
> [skipped]
> 
>>>> +     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);
>>>
>>> This can result in unbalanced clk_disable(), if pm_runtime_get_sync()
>>> fails. This function calls clk_disable(), but the framework has no way
>>> to know that .enable() was not successful and calls .disable(), which
>>> also calls clk_disable( >
>>> Please consider turning pclk into pm_clk so that its state is managed
>>> automatically (or at least moving clk_enable/disable into pm_ops).
>>
>> This process appears rather intricate. May I suggest omitting the
>> 'clk_disable' operation here?
> 
> Yes
> 
>>
>>>
>>>> +             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 device_node *port;
>>>> +
>>>> +     if (!dev->of_node)
>>>> +             return -ENODEV;
>>>> +
>>>> +     lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>>>> +     if (!lvds)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     lvds->dev = dev;
>>>> +
>>>> +     lvds->regs = devm_ioremap_resource(lvds->dev,
>>>> +                     platform_get_resource(pdev, IORESOURCE_MEM, 0));
>>>> +     if (IS_ERR(lvds->regs))
>>>> +             return PTR_ERR(lvds->regs);
>>>> +
>>>> +     lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");
>>>
>>> Why do you need _prepared version?
>>
>> I will change this to just devm_clk_get().
>>
>>>
>>>> +     if (IS_ERR(lvds->pclk))
>>>> +             return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
>>>> +                             "could not get pclk_lvds prepared\n");
>>>> +
>>>> +     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 -ENODEV;
>>>> +     }
>>>> +
>>>> +     lvds->panel = of_drm_find_panel(port);
>>>> +     of_node_put(port);
>>>> +
>>>> +     if (IS_ERR(lvds->panel))
>>>> +             return -EPROBE_DEFER;
>>>> +
>>>> +     lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
>>>
>>> Please use instead:
>>>
>>> devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>>
>> Sure, Noted. Thanks.
>>>
>>>> +
>>>> +     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);
>>>> +     devm_pm_runtime_enable(dev);
>>>
>>> Error check is missing.
>>
>> Sure, I will add this
>>
>>           ret = devm_pm_runtime_enable(dev);
>>           if (ret < 0) {
>>                   DRM_DEV_ERROR(lvds->dev, "failed to enable pm runtime:
> 
> DRM_DEV_ERROR is deprecated, plese use suitable replacement.

Sure, I will replace DRM_DEV_ERROR() with dev_err().

> 
>> %d\n", ret);
>>                   return ret;
>>           }
>>
>>
> 
> --
> With best wishes
> Dmitry

-- 
With Best Regards,
Dharma B.