Re: [PATCH v2 1/2] i2c: rtl9300: add realtek,rtl9607c-i2c compatible

[Michael Zavertkin]

On Sat, Oct 10, 2026 at 06:45:14AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] i2c: rtl9300: device_is_compatible check breaks DT fallback patterns
> --
> 
> commit 96f11e2144ef16398bba64fc512f5b346ab89571
> Author: Michael Zavertkin <misha.zavertkin@xxxxxxx>
> 
> i2c: rtl9300: add realtek,rtl9607c-i2c compatible
> 
> This commit adds support for the realtek,rtl9607c-i2c compatible string to 
> distinguish it from other RTL9607 variants. It avoids using the IO_MODE_EN 
> register on the new compatible by restricting the select_scl call and emits 
> a deprecation warning for the old compatible string.
> 
> > diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> > index 3a8225b0666c0..22aa3b38458c0 100644
> > --- a/drivers/i2c/busses/i2c-rtl9300.c
> > +++ b/drivers/i2c/busses/i2c-rtl9300.c
> > @@ -200,10 +200,12 @@ static int rtl9607_i2c_config_chan(struct rtl9300_i2c *i2c, struct rtl9300_i2c_c
> >  	if (ret)
> >  		return ret;
> >  
> > -	drv_data = device_get_match_data(i2c->dev);
> > -	ret = drv_data->select_scl(i2c, i2c->scl_num);
> > -	if (ret)
> > -		return ret;
> > +	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c")) {
> > +		drv_data = device_get_match_data(i2c->dev);
> > +		ret = drv_data->select_scl(i2c, i2c->scl_num);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> [Severity: Medium]
> Will this check break device tree fallback patterns?
> 
> If a new device tree provides a fallback compatible list like 
> compatible = "realtek,rtl9607c-i2c", "realtek,rtl9607-i2c"; 
> device_is_compatible() will return true because the legacy string is still 
> present in the node.
> 
> If this happens, the driver will incorrectly execute the legacy select_scl 
> hardware configuration (modifying the IO_MODE_EN register) on RTL9607C 
> hardware, instead of delegating it to the pinctrl driver as intended.
> 
> Would it be safer to differentiate hardware capabilities based on the matched 
> driver data (for instance, pointing the new compatible to a struct without 
> a select_scl function) instead of dynamically searching the OF node for 
> deprecated strings?
> 
> >  	i2c->sda_num = chan->sda_num;
> >  	return 0;
> > @@ -494,6 +496,10 @@ static int rtl9300_i2c_probe(struct platform_device *pdev)
> >  		scl_num = 0;
> >  	i2c->scl_num = (u8)scl_num;
> >  
> > +	if (device_is_compatible(i2c->dev, "realtek,rtl9607-i2c"))
> > +		dev_warn(dev, "Compatible 'realtek,rtl9607-i2c' is deprecated. "
> > +			      "Use 'realtek,rtl9607c-i2c' instead\n");
> > +
> 
> [Severity: Medium]
> Does this also trigger an incorrect deprecation warning when fallback 
> compatibles are used?
> 
> Because device_is_compatible() matches any string in the list, providing a 
> valid fallback to support older kernels will falsely trigger this warning 
> even if "realtek,rtl9607c-i2c" is properly specified as the primary compatible 
> string.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791611999.git.misha.zavertkin@xxxxxxx?part=1

There is no device-embedded device tree files with this compatibles,
only embedded in kernel during build (OpenWRT approach), so fallback
compatibles isn't expected, and proper fallback support would look like
overkill right now.