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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxx>
>> + * Author: Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>
>> + *
>> + */
>> +
>> +#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@xxxxxxxxxxxxx>");
>> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1
>>
> 
> --
> With best wishes
> Dmitry

-- 
With Best Regards,
Dharma B.





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux