| 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
| ||
|
Message-Id: <1552785272.1562.2@crapouillou.net>
Date: Sun, 17 Mar 2019 02:14:32 +0100
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.
In the current state of the driver it would be possible to use the
drm_simple_display_pipe stuff, yes. However the SoCs support multiple
planes, and multiple outputs, and the plan is to upload the driver
to support these.
> Some smaller comments in the following.
> Most are suggestion, do not follow these blindly.
Ok, thanks.
Regards,
-Paul
> 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)
>
>> + 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