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@xxxxxxxxxxxxx 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@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
>
> [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.
