Re: [PATCH v2 04/56] drm/drm_scdc_helper: Add SCDC helper funcs for FRL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/display: Unnecessary I2C read in drm_scdc_disable_frl()
--

commit 4f7a237d2d408c021d966e908effa3af04d82b79
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/drm_scdc_helper: Add SCDC helper funcs for FRL

This commit introduces SCDC register definitions and helper functions
to configure and monitor FRL (Fixed Rate Link) link training between
the source and sink.

> diff --git a/drivers/gpu/drm/display/drm_scdc_helper.c b/drivers/gpu/drm/display/drm_scdc_helper.c
> --- a/drivers/gpu/drm/display/drm_scdc_helper.c
> +++ b/drivers/gpu/drm/display/drm_scdc_helper.c
[ ... ]
> +int drm_scdc_disable_frl(struct i2c_adapter *adapter)
> +{
> +	u8 config;
> +	int ret;
> +
> +	ret = drm_scdc_readb(adapter, SCDC_CONFIG_1, &config);
> +	if (ret)
> +		return ret;
> +
> +	config &= ~SCDC_FRL_RATE;
> +
> +	return drm_scdc_writeb(adapter, SCDC_CONFIG_1, config);

[Severity: Low]
Is this read-modify-write sequence strictly necessary here?

The SCDC_CONFIG_1 register contains only SCDC_FRL_RATE (bits 0-3) and
SCDC_FFE_LEVELS (bits 4-7). Since FFE levels are only meaningful when FRL is
enabled, preserving them during disable is likely unnecessary.

Given that reading over the slow I2C DDC bus adds measurable latency, and
drm_scdc_config_frl() successfully treats the register as fully known by
issuing a direct write without a preceding read, would it be better to just
issue a direct write of 0 to clear the register entirely?

> +}
> +EXPORT_SYMBOL(drm_scdc_disable_frl);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012035806.3361439-1-ankit.k.nautiyal@intel.com?part=4