Re: [PATCH v2 09/16] drm/tidss: Add external data and sync signal edge configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect error check for missing syscon property causes driver probe failure on platforms without it.
- [Medium] Using regmap_write() on a syscon MMR register overwrites the entire register, potentially clearing reserved or unhandled bits.
- [Low] Evaluating PTR_ERR() on a potentially valid pointer without a preceding IS_ERR() check violates API semantics.
--

commit 6ff2a14021d4cf3eead5f27440ae73dbed474911
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

drm/tidss: Add external data and sync signal edge configuration

This commit adds support to configure the DPI output data and sync signal
edges via a syscon register. It looks up the syscon regmap using the
ti,dpi-io-ctrl property and writes the required clock inversion bits
during the video port preparation phase.

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c

[ ... ]

> @@ -1201,6 +1203,13 @@ void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport,
>  				  mode->crtc_hdisplay - 1) |
>  		       FIELD_PREP(DISPC_VP_SIZE_SCREEN_VDISPLAY_MASK,
>  				  mode->crtc_vdisplay - 1));
> +
> +	if (dispc->vp_data[hw_videoport].dpi_output && dispc->syscon_dpi_io_ctrl) {
> +		regmap_write(dispc->syscon_dpi_io_ctrl,
> +			     dispc->syscon_dpi_io_ctrl_offset + 0x0,
> +			     (!ipc ? DPI0_CLK_CTRL_DATA_CLK_INVDIS : 0) |
> +			     (rf ? DPI0_CLK_CTRL_SYNC_CLK_INVDIS : 0));
> +	}
>  }

Could this regmap_write() overwrite other important fields in the syscon MMR
register? 

Using regmap_write() unconditionally writes 0 to all other bits. Should this
be a read-modify-write using regmap_update_bits() with an explicit mask for
bits 8 and 9 to preserve the rest of the register state?

[ ... ]

> @@ -2989,6 +2998,18 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	dispc_init_errata(dispc);
>  
> +	dispc->syscon_dpi_io_ctrl =
> +		syscon_regmap_lookup_by_phandle_args(tidss->dev->of_node,
> +						     "ti,dpi-io-ctrl", 1,
> +						     &dispc->syscon_dpi_io_ctrl_offset);
> +
> +	if (PTR_ERR(dispc->syscon_dpi_io_ctrl) == -ENODEV)

Is it safe to evaluate PTR_ERR() before verifying that the pointer is actually
an error using IS_ERR()?

Also, when the ti,dpi-io-ctrl property is absent from the device tree,
syscon_regmap_lookup_by_phandle_args() returns -ENOENT, not -ENODEV.

Will this cause the driver to evaluate the missing property case as false,
fall through to the IS_ERR() block, and trigger dev_err_probe(), breaking
display initialization on platforms that do not provide this optional
configuration?

> +		dispc->syscon_dpi_io_ctrl = NULL;
> +	else if (IS_ERR(dispc->syscon_dpi_io_ctrl))
> +		return dev_err_probe(dispc->dev,
> +				     PTR_ERR(dispc->syscon_dpi_io_ctrl),
> +				     "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
> +
>  	dispc->fourccs = devm_kcalloc(dev, ARRAY_SIZE(dispc_color_formats),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-beagley-ai-display-v2-0-9e9bcefde6bc@ideasonboard.com?part=9