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

[<Dharma.B@microchip.com>]

Hi Krzysztof,

On 22/01/24 9:19 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 22/01/2024 09:29, 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>
>> ---
> 
> ...
> 
>> +
>> +static int mchp_lvds_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct mchp_lvds *lvds;
>> +     struct resource *res;
>> +     struct device_node *port;
>> +     int ret;
>> +
>> +     if (!dev->of_node)
>> +             return -ENODEV;
>> +
>> +     lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>> +     if (!lvds)
>> +             return -ENOMEM;
>> +
>> +     lvds->dev = dev;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     lvds->regs = devm_ioremap_resource(lvds->dev, res);
> 
> Why not combining these two?

It seems reasonable to combine these two lines since the resource 
variable (res) is only used at this point. I'll proceed with 
consolidating these lines for simplicity.

> 
>> +     if (IS_ERR(lvds->regs))
>> +             return PTR_ERR(lvds->regs);
>> +
>> +     lvds->pclk = devm_clk_get(lvds->dev, "pclk");
>> +     if (IS_ERR(lvds->pclk)) {
>> +             DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
> 
> Handle properly deferred probe. What's DRM wrapper over dev_err_probe()?
Sure, I will use dev_err_probe()

return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), "could not get 
pclk_lvds\n");

> 
>> +             return PTR_ERR(lvds->pclk);
>> +     }
>> +
>> +     ret = clk_prepare(lvds->pclk);
>> +     if (ret < 0) {
>> +             DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
>> +             return ret;
>> +     }
>> +
>> +     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 -EINVAL;
>> +     }
>> +
>> +     lvds->panel = of_drm_find_panel(port);
>> +     of_node_put(port);
>> +
>> +     if (IS_ERR(lvds->panel)) {
>> +             DRM_DEV_ERROR(dev, "failed to find panel node\n");
>> +             return -EPROBE_DEFER;
> 
> OK, that's for sure wrong. Don't print anything on deferred probe.
Sure, I will drop the print here.
> 
>> +     }
>> +
>> +     lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
>> +
>> +     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);
>> +     pm_runtime_enable(dev);
>> +
>> +     drm_bridge_add(&lvds->bridge);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mchp_lvds_remove(struct platform_device *pdev)
>> +{
>> +     struct mchp_lvds *lvds = platform_get_drvdata(pdev);
>> +
>> +     pm_runtime_disable(&pdev->dev);
>> +     clk_unprepare(lvds->pclk);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id mchp_lvds_dt_ids[] = {
>> +     {
>> +             .compatible = "microchip,sam9x7-lvds",
>> +     },
>> +     {},
>> +};
>> +
>> +struct platform_driver mchp_lvds_driver = {
>> +     .probe = mchp_lvds_probe,
>> +     .remove = mchp_lvds_remove,
>> +     .driver = {
>> +                .name = "microchip-lvds",
>> +                .of_match_table = mchp_lvds_dt_ids,
>> +     },
>> +};
>> +module_platform_driver(mchp_lvds_driver);
>> +
>> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
>> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
>> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:microchip-lvds");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.

Okay, I will remove the MODULE_ALIAS and update the mchp_lvds_dt_ids[] 
as below along with MODULE_DEVICE_TABLE()

static const struct of_device_id mchp_lvds_dt_ids[] = {
         {
                 .compatible = "microchip,sam9x72-lvds",
         },
         {
                 .compatible = "microchip,sam9x75-lvds",
         },
         {},
};
MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);

-- 
Thanks,
Dharma B.
> 
> 
> Best regards,
> Krzysztof
>