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

[<Dharma.B@microchip.com>]

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.