Re: [PATCH 18/39] drm: renesas: shmobile: Embed drm_device in shmob_drm_device

From: Laurent Pinchart
Date: Fri Jun 23 2023 - 11:48:59 EST

Next message: Laurent Pinchart: "Re: [PATCH 19/39] drm: renesas: shmobile: Convert container helpers to static inline functions"
Previous message: Laurent Pinchart: "Re: [PATCH 17/39] drm: renesas: shmobile: Use drmm_universal_plane_alloc()"
In reply to: Geert Uytterhoeven: "[PATCH 18/39] drm: renesas: shmobile: Embed drm_device in shmob_drm_device"
Next in thread: Geert Uytterhoeven: "[PATCH 05/39] drm: renesas: shmobile: Correct encoder/connector types"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Geert,

Thank you for the patch.

On Thu, Jun 22, 2023 at 11:21:30AM +0200, Geert Uytterhoeven wrote:
> Embedding drm_device in shmob_drm_device allows us to use the DRM
> managed API to allocate both structures in one go, simplifying error
> handling.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>

> ---
> .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 12 +++---
> .../gpu/drm/renesas/shmobile/shmob_drm_drv.c | 41 +++++++------------
> .../gpu/drm/renesas/shmobile/shmob_drm_drv.h | 2 +-
> .../gpu/drm/renesas/shmobile/shmob_drm_kms.c | 16 ++++----
> .../drm/renesas/shmobile/shmob_drm_plane.c | 8 ++--
> 5 files changed, 34 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> index bcdebbc9f8a7e299..26611440f7a12715 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> @@ -139,7 +139,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> struct shmob_drm_device *sdev = crtc->dev->dev_private;
> const struct shmob_drm_interface_data *idata = &sdev->pdata->iface;
> const struct shmob_drm_format_info *format;
> - struct drm_device *dev = sdev->ddev;
> + struct drm_device *dev = &sdev->ddev;
> struct drm_plane *plane;
> u32 value;
> int ret;
> @@ -472,7 +472,7 @@ int shmob_drm_crtc_create(struct shmob_drm_device *sdev)
>
> sdev->crtc.dpms = DRM_MODE_DPMS_OFF;
>
> - primary = __drm_universal_plane_alloc(sdev->ddev, sizeof(*primary), 0,
> + primary = __drm_universal_plane_alloc(&sdev->ddev, sizeof(*primary), 0,
> 0, &primary_plane_funcs,
> modeset_formats,
> ARRAY_SIZE(modeset_formats),
> @@ -481,7 +481,7 @@ int shmob_drm_crtc_create(struct shmob_drm_device *sdev)
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> - ret = drm_crtc_init_with_planes(sdev->ddev, crtc, primary, NULL,
> + ret = drm_crtc_init_with_planes(&sdev->ddev, crtc, primary, NULL,
> &crtc_funcs, NULL);
> if (ret < 0) {
> drm_plane_cleanup(primary);
> @@ -557,7 +557,7 @@ int shmob_drm_encoder_create(struct shmob_drm_device *sdev)
>
> encoder->possible_crtcs = 1;
>
> - ret = drm_simple_encoder_init(sdev->ddev, encoder,
> + ret = drm_simple_encoder_init(&sdev->ddev, encoder,
> DRM_MODE_ENCODER_DPI);
> if (ret < 0)
> return ret;
> @@ -637,7 +637,7 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
> connector->display_info.width_mm = sdev->pdata->panel.width_mm;
> connector->display_info.height_mm = sdev->pdata->panel.height_mm;
>
> - ret = drm_connector_init(sdev->ddev, connector, &connector_funcs,
> + ret = drm_connector_init(&sdev->ddev, connector, &connector_funcs,
> DRM_MODE_CONNECTOR_DPI);
> if (ret < 0)
> return ret;
> @@ -650,7 +650,7 @@ int shmob_drm_connector_create(struct shmob_drm_device *sdev,
>
> drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> drm_object_property_set_value(&connector->base,
> - sdev->ddev->mode_config.dpms_property, DRM_MODE_DPMS_OFF);
> + sdev->ddev.mode_config.dpms_property, DRM_MODE_DPMS_OFF);
>
> return 0;
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> index ece9aedde9b662d4..2b77af3a8c97ef8c 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> @@ -137,7 +137,7 @@ static int shmob_drm_pm_suspend(struct device *dev)
> {
> struct shmob_drm_device *sdev = dev_get_drvdata(dev);
>
> - drm_kms_helper_poll_disable(sdev->ddev);
> + drm_kms_helper_poll_disable(&sdev->ddev);
> shmob_drm_crtc_suspend(&sdev->crtc);
>
> return 0;
> @@ -147,11 +147,11 @@ static int shmob_drm_pm_resume(struct device *dev)
> {
> struct shmob_drm_device *sdev = dev_get_drvdata(dev);
>
> - drm_modeset_lock_all(sdev->ddev);
> + drm_modeset_lock_all(&sdev->ddev);
> shmob_drm_crtc_resume(&sdev->crtc);
> - drm_modeset_unlock_all(sdev->ddev);
> + drm_modeset_unlock_all(&sdev->ddev);
>
> - drm_kms_helper_poll_enable(sdev->ddev);
> + drm_kms_helper_poll_enable(&sdev->ddev);
> return 0;
> }
>
> @@ -165,12 +165,10 @@ static DEFINE_SIMPLE_DEV_PM_OPS(shmob_drm_pm_ops,
> static int shmob_drm_remove(struct platform_device *pdev)
> {
> struct shmob_drm_device *sdev = platform_get_drvdata(pdev);
> - struct drm_device *ddev = sdev->ddev;
> + struct drm_device *ddev = &sdev->ddev;
>
> drm_dev_unregister(ddev);
> drm_kms_helper_poll_fini(ddev);
> - drm_dev_put(ddev);
> -
> return 0;
> }
>
> @@ -188,13 +186,15 @@ static int shmob_drm_probe(struct platform_device *pdev)
> }
>
> /*
> - * Allocate and initialize the driver private data, I/O resources and
> - * clocks.
> + * Allocate and initialize the DRM device, driver private data, I/O
> + * resources and clocks.
> */
> - sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
> - if (sdev == NULL)
> - return -ENOMEM;
> + sdev = devm_drm_dev_alloc(&pdev->dev, &shmob_drm_driver,
> + struct shmob_drm_device, ddev);
> + if (IS_ERR(sdev))
> + return PTR_ERR(sdev);
>
> + ddev = &sdev->ddev;
> sdev->dev = &pdev->dev;
> sdev->pdata = pdata;
> spin_lock_init(&sdev->irq_lock);
> @@ -217,20 +217,12 @@ static int shmob_drm_probe(struct platform_device *pdev)
> if (ret < 0)
> return ret;
>
> - /* Allocate and initialize the DRM device. */
> - ddev = drm_dev_alloc(&shmob_drm_driver, &pdev->dev);
> - if (IS_ERR(ddev))
> - return PTR_ERR(ddev);
> -
> - sdev->ddev = ddev;
> ddev->dev_private = sdev;
>
> ret = shmob_drm_modeset_init(sdev);
> - if (ret < 0) {
> - dev_err_probe(&pdev->dev, ret,
> - "failed to initialize mode setting\n");
> - goto err_free_drm_dev;
> - }
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to initialize mode setting\n");
>
> for (i = 0; i < 4; ++i) {
> ret = shmob_drm_plane_create(sdev, i);
> @@ -272,9 +264,6 @@ static int shmob_drm_probe(struct platform_device *pdev)
>
> err_modeset_cleanup:
> drm_kms_helper_poll_fini(ddev);
> -err_free_drm_dev:
> - drm_dev_put(ddev);
> -
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> index 16d830168b2ada21..77bb0da48f37ace8 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> @@ -32,7 +32,7 @@ struct shmob_drm_device {
> unsigned int irq;
> spinlock_t irq_lock; /* Protects hardware LDINTR register */
>
> - struct drm_device *ddev;
> + struct drm_device ddev;
>
> struct shmob_drm_crtc crtc;
> struct drm_encoder encoder;
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_kms.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_kms.c
> index 1a62e7f8a8a9e6df..20316907030b2789 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_kms.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_kms.c
> @@ -153,7 +153,7 @@ int shmob_drm_modeset_init(struct shmob_drm_device *sdev)
> {
> int ret;
>
> - ret = drmm_mode_config_init(sdev->ddev);
> + ret = drmm_mode_config_init(&sdev->ddev);
> if (ret)
> return ret;
>
> @@ -169,15 +169,15 @@ int shmob_drm_modeset_init(struct shmob_drm_device *sdev)
> if (ret < 0)
> return ret;
>
> - drm_kms_helper_poll_init(sdev->ddev);
> + drm_kms_helper_poll_init(&sdev->ddev);
>
> - sdev->ddev->mode_config.min_width = 0;
> - sdev->ddev->mode_config.min_height = 0;
> - sdev->ddev->mode_config.max_width = 4095;
> - sdev->ddev->mode_config.max_height = 4095;
> - sdev->ddev->mode_config.funcs = &shmob_drm_mode_config_funcs;
> + sdev->ddev.mode_config.min_width = 0;
> + sdev->ddev.mode_config.min_height = 0;
> + sdev->ddev.mode_config.max_width = 4095;
> + sdev->ddev.mode_config.max_height = 4095;
> + sdev->ddev.mode_config.funcs = &shmob_drm_mode_config_funcs;
>
> - drm_helper_disable_unused_functions(sdev->ddev);
> + drm_helper_disable_unused_functions(&sdev->ddev);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> index 1fb68b5fe915b8dc..17e66a018689f648 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c
> @@ -68,7 +68,7 @@ static void __shmob_drm_plane_setup(struct shmob_drm_plane *splane,
> splane->format->ldbbsifr;
>
> #define plane_reg_dump(sdev, splane, reg) \
> - dev_dbg(sdev->ddev->dev, "%s(%u): %s 0x%08x 0x%08x\n", __func__, \
> + dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x 0x%08x\n", __func__, \
> splane->index, #reg, \
> lcdc_read(sdev, reg(splane->index)), \
> lcdc_read(sdev, reg(splane->index) + LCDC_SIDE_B_OFFSET))
> @@ -81,7 +81,7 @@ static void __shmob_drm_plane_setup(struct shmob_drm_plane *splane,
> plane_reg_dump(sdev, splane, LDBnBSACR);
>
> lcdc_write(sdev, LDBCR, LDBCR_UPC(splane->index));
> - dev_dbg(sdev->ddev->dev, "%s(%u): %s 0x%08x\n", __func__, splane->index,
> + dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, splane->index,
> "LDBCR", lcdc_read(sdev, LDBCR));
>
> lcdc_write(sdev, LDBnBSIFR(splane->index), format);
> @@ -103,7 +103,7 @@ static void __shmob_drm_plane_setup(struct shmob_drm_plane *splane,
>
> lcdc_write(sdev, LDBCR,
> LDBCR_UPF(splane->index) | LDBCR_UPD(splane->index));
> - dev_dbg(sdev->ddev->dev, "%s(%u): %s 0x%08x\n", __func__, splane->index,
> + dev_dbg(sdev->ddev.dev, "%s(%u): %s 0x%08x\n", __func__, splane->index,
> "LDBCR", lcdc_read(sdev, LDBCR));
>
> plane_reg_dump(sdev, splane, LDBnBSIFR);
> @@ -198,7 +198,7 @@ int shmob_drm_plane_create(struct shmob_drm_device *sdev, unsigned int index)
> {
> struct shmob_drm_plane *splane;
>
> - splane = drmm_universal_plane_alloc(sdev->ddev, struct shmob_drm_plane,
> + splane = drmm_universal_plane_alloc(&sdev->ddev, struct shmob_drm_plane,
> plane, 1, &shmob_drm_plane_funcs,
> formats, ARRAY_SIZE(formats), NULL,
> DRM_PLANE_TYPE_OVERLAY, NULL);

--
Regards,

Laurent Pinchart

Next message: Laurent Pinchart: "Re: [PATCH 19/39] drm: renesas: shmobile: Convert container helpers to static inline functions"
Previous message: Laurent Pinchart: "Re: [PATCH 17/39] drm: renesas: shmobile: Use drmm_universal_plane_alloc()"
In reply to: Geert Uytterhoeven: "[PATCH 18/39] drm: renesas: shmobile: Embed drm_device in shmob_drm_device"
Next in thread: Geert Uytterhoeven: "[PATCH 05/39] drm: renesas: shmobile: Correct encoder/connector types"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]