mbox series

[0/3] LVDS Controller Support for SAM9X7 SoC

Message ID 20240122082947.21645-1-dharma.b@microchip.com
Headers show
Series LVDS Controller Support for SAM9X7 SoC | expand

Message

Dharma Balasubiramani Jan. 22, 2024, 8:29 a.m. UTC 
This patch series introduces LVDS controller support for the SAM9X7 SoC. The
LVDS controller is designed to work with Microchip's sam9x7 series
System-on-Chip (SoC) devices, providing Low Voltage Differential Signaling
capabilities.

Dharma Balasubiramani (3):
  dt-bindings: display: bridge: add sam9x7-lvds compatible
  drm/bridge: add lvds controller support for sam9x7
  MAINTAINERS: add SAM9X7 SoC's LVDS controller

 .../display/bridge/microchip,sam9x7-lvds.yaml |  59 +++++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/bridge/Kconfig                |   7 +
 drivers/gpu/drm/bridge/Makefile               |   1 +
 drivers/gpu/drm/bridge/microchip-lvds.c       | 250 ++++++++++++++++++
 5 files changed, 325 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/microchip,sam9x7-lvds.yaml
 create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c

Comments

Krzysztof Kozlowski Jan. 22, 2024, 3:49 p.m. UTC | #1 
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?

> +	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()?

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

> +	}
> +
> +	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.


Best regards,
Krzysztof
Dharma Balasubiramani Jan. 23, 2024, 8:26 a.m. UTC | #2 
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);