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

[sashiko-bot@kernel.org]

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@mail.ru>

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@mail.ru?part=1