Re: [PATCH v8 05/15] drm/imx: dc: Flag CRTC status in GERNERALPURPOSE register

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] drm/imx: dc: missing max_register_is_0 in dc_gp_regmap_config
- [Low] drm/imx: dc: commit message uses stale helper names and register typo
--

commit 376ae6ee1d9ffb7f4790013466f30007db36afe6
Author: Liu Ying <victor.liu@nxp.com>

drm/imx: dc: Flag CRTC status in GERNERALPURPOSE register

This commit maps the Display Controller's GENERALPURPOSE register via an MMIO
regmap and adds helpers to set, clear, and query the CRTC enablement status in
its 2 LSB bits.

>     drm/imx: dc: Flag CRTC status in GERNERALPURPOSE register
>     
>     Map Display Controller's register GERNERALPURPOSE at offset 0x100 and
>     create an MMIO regmap to access the register.  Create helpers
>     dc_drv_flag_crtc_{enable,disable}() with the 2 LSB bits in the register
>     to flag the CRTC enablement/disablement status.  Also, create a helper
>     dc_drv_crtc_was_enabled() so that the CRTC driver can use it to check
>     if the CRTC was enabled by a previous boot or not.  This is a preparation
>     for an upcoming patch to disable display controller at boot if the CRTC
>     was enabled by the driver during a previous boot instead of a bootloader.
>     
>     Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
>     Signed-off-by: Liu Ying <victor.liu@nxp.com>

[Severity: Low]
This isn't a bug in the code, but should the commit message be updated to
match the current symbol names?

In drivers/gpu/drm/imx/dc/dc-drv.c and drivers/gpu/drm/imx/dc/dc-drv.h, the
helpers are named dc_drv_set_crtc_enable(), dc_drv_set_crtc_disable(), and
dc_drv_crtc_is_enabled(), rather than dc_drv_flag_crtc_{enable,disable}() and
dc_drv_crtc_was_enabled().

Also, the commit subject and body spell the register name as GERNERALPURPOSE,
whereas the macro in drivers/gpu/drm/imx/dc/dc-drv.c is defined as
GENERALPURPOSE.

> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
> index 13795a2ad7350..9629beaed43c1 100644
> --- a/drivers/gpu/drm/imx/dc/dc-drv.c
> +++ b/drivers/gpu/drm/imx/dc/dc-drv.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
>  
>  #include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -28,9 +29,21 @@
>  #include "dc-drv.h"
>  #include "dc-pe.h"
>  
> +/* General Purpose register offset used to flag driver usage. */
> +#define GENERALPURPOSE		0x100
> +#define  CRTC_ENABLED(x)	BIT(x)
> +
>  struct dc_priv {
>  	struct drm_device *drm;
>  	struct clk *clk_cfg;
> +	struct regmap *gp_regmap;
> +};
> +
> +static const struct regmap_config dc_gp_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = 0,
> +};

[Severity: Low]
Should dc_gp_regmap_config also set .max_register_is_0 = true here?

In dc_probe(), only a 4-byte MMIO region at r->start + GENERALPURPOSE is
mapped and passed to devm_regmap_init_mmio() for priv->gp_regmap, so offset 0
is the only valid register.

In __regmap_init(), map->max_register_is_set is initialized as:

    map->max_register_is_set = map->max_register ?: config->max_register_is_0;

Without .max_register_is_0 = true, regmap treats a zero .max_register as
unspecified and leaves map->max_register_is_set false, so regmap_writeable()
and regmap_readable() do not enforce the offset 0 bound.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-imx8-dc-prefetch-v8-0-c4f81afd4533@nxp.com?part=5