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 <[email protected]> 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 <[email protected]> > Signed-off-by: Liu Ying <[email protected]> [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/[email protected]?part=5
