lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 02 Apr 2019 18:16:23 +0200 From: Paul Cercueil <paul@...pouillou.net> To: Sam Ravnborg <sam@...nborg.org> Cc: David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>, Rob Herring <robh+dt@...nel.org>, Mark Rutland <mark.rutland@....com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <maxime.ripard@...tlin.com>, Sean Paul <sean@...rly.run>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org Subject: Re: [PATCH v2 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs Hi Sam, Le sam. 16 mars 2019 à 22:59, Sam Ravnborg <sam@...nborg.org> a écrit : > 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 <paul@...pouillou.net> >> Tested-by: Artur Rojek <contact@...ur-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) If I have a request for deferred probe after devm_drm_dev_init(), I get a kernel oops in drm_mode_config_cleanup()... >> + 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. >
Powered by blists - more mailing lists