Re: [PATCH v2 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs
On Sat, Mar 16, 2019 at 10:59:44PM +0100, Sam Ravnborg wrote:
> > + 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.
Drive-through comment: Totally ok to require this, failing here usually
means you have a driver bug somewhere. fbdev is optional as in Kconfig
optional, but people who enable it generally need it.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 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
Tested-by: Artur Rojek
+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 = >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,
+_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, ,
);
+ 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",
>lcd_mode);
+
+ priv->framedesc = dma_alloc_coherent(dev,
sizeof(*priv->framedesc),
+ >framedesc_phys, GFP_KERNEL);
+ if (!priv->framedesc)
+ return -ENOMEM;
+
+ priv->framedesc->next = priv->framedesc_phys;
+ priv->framedesc->id = 0xdeafbead;
+
+ drm = drm_dev_alloc(_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 = _drm_mode_config_funcs;
+
+ drm_plane_helper_add(>primary,
_drm_plane_helper_funcs);
+
+ ret = drm_universal_plane_init(drm, >primary,
+
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 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
Tested-by: Artur Rojek
+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 = >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,
+_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, ,
);
+ 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",
>lcd_mode);
+
+ priv->framedesc = dma_alloc_coherent(dev,
sizeof(*priv->framedesc),
+ >framedesc_phys, GFP_KERNEL);
+ if (!priv->framedesc)
+ return -ENOMEM;
+
+ priv->framedesc->next = priv->framedesc_phys;
+ priv->framedesc->id = 0xdeafbead;
+
+ drm = drm_dev_alloc(_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 = _drm_mode_config_funcs;
+
+
Re: [PATCH v2 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs
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
> Tested-by: Artur Rojek
> +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 = >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,
> + _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, , );
> + 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", >lcd_mode);
> +
> + priv->framedesc = dma_alloc_coherent(dev, sizeof(*priv->framedesc),
> + >framedesc_phys, GFP_KERNEL);
> + if (!priv->framedesc)
> + return -ENOMEM;
> +
> + priv->framedesc->next = priv->framedesc_phys;
> + priv->framedesc->id = 0xdeafbead;
> +
> + drm = drm_dev_alloc(_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 = _drm_mode_config_funcs;
> +
> + drm_plane_helper_add(>primary, _drm_plane_helper_funcs);
> +
> + ret = drm_universal_plane_init(drm, >primary,
> +0, _drm_primary_plane_funcs,
> +ingenic_drm_primary_formats,
> +
