Re: [PATCH v2 2/3] i2c: designware: Export symbols and add __weak for Starfive I2C driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, May 27, 2026 at 04:50:38PM +0800, lianfeng.ouyang wrote:

> Export several key functions (i2c_dw_probe_master, i2c_dw_init,
> i2c_dw_xfer_init, i2c_dw_read_clear_intrbits, etc.) and mark them as
> __weak. This allows the i2c starfive driver to reuse the common
> infrastructure while overriding the implementations where needed.
> 
> Additionally, extend the register map configuration and introduce the
> MODEL_STARFIVE flag to accommodate the starfive i2c  IP's different
> register space.

...

> +#endif

My gosh, no.


>  		.val_bits = 32,
>  		.reg_stride = 4,
>  		.disable_locking = true,
> +#if IS_ENABLED(CONFIG_I2C_STARFIVE)
> +		.reg_read = sf_reg_read,
> +		.reg_write = sf_reg_write,
> +		.max_register = SF_IC_SMBUS_INTR_CLR,
> +#else

No.

Just find a different way. See how Baikal T1 support was implemented
(in v6.18, for example).

>  		.reg_read = dw_reg_read,
>  		.reg_write = dw_reg_write,
>  		.max_register = DW_IC_COMP_TYPE,
> +#endif

...

> +#ifdef CONFIG_OF

Definitely no.

> +	ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-tx-fifo-depth", &tx_fifo_cfg);

No. Why?!

Also do not use OF-centric APIs in the driver that uses fwnode.

> +	if (!ret && (tx_fifo_cfg < 2 || tx_fifo_cfg > 256))
> +		tx_fifo_cfg = 8;
> +
> +	ret = of_property_read_u32(dev->dev->of_node, "starfive,i2c-rx-fifo-depth", &rx_fifo_cfg);
> +	if (!ret && (rx_fifo_cfg < 2 || rx_fifo_cfg > 256))
> +		rx_fifo_cfg = 8;
> +#endif
> +	param = rx_fifo_cfg << 8 | tx_fifo_cfg << 16;
> +#else
>  	ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, &param);
>  	i2c_dw_release_lock(dev);
>  	if (ret)
>  		return ret;
> +#endif

-- 
With Best Regards,
Andy Shevchenko