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 <[email protected]> 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/[email protected]?part=4
