Hi Paul.
Thanks for the v2 submission.
Did you analyze the possibility to utilize drm_simple_display_pipe_init()
and the related infrastructure?
If this fits it should simplify the driver.
If it does not fit please let us know why.
As this is a one crtc / one connector / one panel the drm_simple_*
infrastructure is supposed to be a good fit.
Some smaller comments in the following.
Most are suggestion, do not follow these blindly.
Sam
> Add a KMS driver for the Ingenic JZ47xx family of SoCs.
> This driver is meant to replace the aging jz4740-fb driver.
>
> Signed-off-by: Paul Cercueil <p...@crapouillou.net>
> Tested-by: Artur Rojek <cont...@artur-rojek.eu>
> +struct ingenic_drm {
> + struct device *dev;
> + void __iomem *base;
> + struct regmap *map;
> + struct clk *lcd_clk, *pix_clk;
> +
> + u32 lcd_mode;
> +
> + struct ingenic_dma_hwdesc *framedesc;
Consider the name "dma_hwdesc" for this.
The struct is named so, which give a good indication
this is a more descriptive name.
That said, the current solution looks much cleaner than the
previous one.
> + dma_addr_t framedesc_phys;
Likewise.
> +
> + struct drm_device *drm;
If drm is embedded you can use devm_drm_dev_init()
recently added to drm-misc.
See the very nice example in drivers/gu/drm/drm_drv.c
(only in drm-misc-next for now)
> + struct drm_plane primary;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> +};
> +
> +
> +static int ingenic_drm_probe(struct platform_device *pdev)
> +{
> + const struct jz_soc_info *soc_info;
> + struct device *dev = &pdev->dev;
> + struct ingenic_drm *priv;
> + struct clk *parent_clk;
> + struct drm_bridge *bridge;
> + struct drm_panel *panel;
> + struct drm_device *drm;
> + struct resource *mem;
> + void __iomem *base;
> + long parent_rate;
> + int ret, irq;
> +
> + soc_info = device_get_match_data(dev);
Everyone else uses of_device_... here. You should most
likely do the same.
> + if (!soc_info)
> + return -EINVAL;
Also, consider to print an error here.
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
Use of devm_kzalloc() here is not good. See driver example in drm_drv.c
> +
> + priv->dev = dev;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = base = devm_ioremap_resource(dev, mem);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "Failed to get platform irq\n");
> + return -ENOENT;
> + }
> +
> + priv->map = devm_regmap_init_mmio(dev, base,
> + &ingenic_drm_regmap_config);
> + if (IS_ERR(priv->map)) {
> + dev_err(dev, "Failed to create regmap\n");
> + return PTR_ERR(priv->map);
> + }
> +
> + if (soc_info->needs_dev_clk) {
> + priv->lcd_clk = devm_clk_get(dev, "lcd");
> + if (IS_ERR(priv->lcd_clk)) {
> + dev_err(dev, "Failed to get lcd clock\n");
> + return PTR_ERR(priv->lcd_clk);
> + }
> + }
> +
> + priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
> + if (IS_ERR(priv->pix_clk)) {
> + dev_err(dev, "Failed to get pixel clock\n");
> + return PTR_ERR(priv->pix_clk);
> + }
> +
> + ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get panel handle\n");
> + return ret;
> + }
> +
> + if (panel) {
> + bridge = devm_drm_panel_bridge_add(dev, panel,
> + DRM_MODE_CONNECTOR_Unknown);
> + }
> +
> + device_property_read_u32(dev, "ingenic,lcd-mode", &priv->lcd_mode);
> +
> + priv->framedesc = dma_alloc_coherent(dev, sizeof(*priv->framedesc),
> + &priv->framedesc_phys, GFP_KERNEL);
> + if (!priv->framedesc)
> + return -ENOMEM;
> +
> + priv->framedesc->next = priv->framedesc_phys;
> + priv->framedesc->id = 0xdeafbead;
> +
> + drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
> + if (IS_ERR(drm)) {
> + ret = PTR_ERR(drm);
> + goto err_free_dma;
> + }
> +
> + priv->drm = drm;
> +
> + drm_mode_config_init(drm);
> + drm->mode_config.min_width = 0;
> + drm->mode_config.min_height = 0;
> + drm->mode_config.max_width = 800;
> + drm->mode_config.max_height = 600;
> + drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
> +
> + drm_plane_helper_add(&priv->primary, &ingenic_drm_plane_helper_funcs);
> +
> + ret = drm_universal_plane_init(drm, &priv->primary,
> + 0, &ingenic_drm_primary_plane_funcs,
> + ingenic_drm_primary_formats,
> + ARRAY_SIZE(ingenic_drm_primary_formats),
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to register primary plane: %i\n", ret);
> + goto err_unref_drm;
> + }
> +
> + drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
> +
> + ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
> + NULL, &ingenic_drm_crtc_funcs, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to init CRTC: %i\n", ret);
> + goto err_cleanup_plane;
> + }
> +
> + priv->encoder.possible_crtcs = 1;
> +
> + drm_encoder_helper_add(&priv->encoder,
> + &ingenic_drm_encoder_helper_funcs);
> +
> + ret = drm_encoder_init(drm, &priv->encoder, &ingenic_drm_encoder_funcs,
> + DRM_MODE_ENCODER_DPI, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to init encoder: %i\n", ret);
> + goto err_cleanup_crtc;
> + }
> +
> + ret = drm_bridge_attach(&priv->encoder, bridge, NULL);
> + if (ret) {
> + dev_err(dev, "Unable to attach bridge\n");
> + goto err_cleanup_encoder;
> + }
> +
> + platform_set_drvdata(pdev, drm);
> + priv->drm = drm;
> + drm->dev_private = priv;
> +
> + ret = drm_irq_install(drm, irq);
> + if (ret) {
> + dev_err(dev, "Unable to install IRQ handler\n");
> + goto err_cleanup_encoder;
> + }
> +
> + ret = drm_vblank_init(drm, 1);
> + if (ret) {
> + dev_err(dev, "Failed calling drm_vblank_init()\n");
> + goto err_uninstall_irq;
> + }
> +
> + drm_mode_config_reset(drm);
> +
> + ret = clk_prepare_enable(priv->pix_clk);
> + if (ret) {
> + dev_err(dev, "Unable to start pixel clock\n");
> + goto err_uninstall_irq;
> + }
> +
> + if (priv->lcd_clk) {
> + parent_clk = clk_get_parent(priv->lcd_clk);
> + parent_rate = clk_get_rate(parent_clk);
> +
> + /* LCD Device clock must be 3x the pixel clock for STN panels,
> + * or 1.5x the pixel clock for TFT panels. To avoid having to
> + * check for the LCD device clock everytime we do a mode change,
> + * we set the LCD device clock to the highest rate possible.
> + */
> + ret = clk_set_rate(priv->lcd_clk, parent_rate);
> + if (ret) {
> + dev_err(dev, "Unable to set LCD clock rate\n");
> + goto err_pixclk_disable;
> + }
> +
> + ret = clk_prepare_enable(priv->lcd_clk);
> + if (ret) {
> + dev_err(dev, "Unable to start lcd clock\n");
> + goto err_pixclk_disable;
> + }
> + }
> +
> + ret = drm_fbdev_generic_setup(drm, 16);
> + if (ret) {
> + dev_err(dev, "Failed to init fbdev\n");
> + goto err_devclk_disable;
> + }
fbdev is usually considered an optionl feature that do not prevent
the display driver from loading.
Consider what to do in the error case.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
