| Message ID | 20240122082947.21645-3-dharma.b@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LVDS Controller Support for SAM9X7 SoC | expand |
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
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);
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
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");