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

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

-- 
With best wishes
Dmitry