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
