Re: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave support

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

On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote:
> > -----邮件原件-----
> > 发件人: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 发送时间: 2026年6月3日 6:27
> > On Wed, May 27, 2026 at 04:50:36PM +0800, lianfeng.ouyang wrote:

> > > The Starfive JHB100 I2C controller is a variant of the widely-used
> > > DesignWare I2C IP, with a distinct register layout and enhanced features
> > > such as SMBus Alert and programmable FIFO depths.
> > >
> > > The series is structured as follows:
> > > 1.  Adds the device tree binding document for the starfive,jhb100-i2c
> > >         compatible.
> > > 2.  Prepares the existing i2c-designware-core by exporting and making
> > >         certain key functions overridable, allowing code reuse.
> > > 3.  Introduces the new i2c-starfive-* driver, with separate modules for
> > >         master and slave functionality, based on the 2023-07 revision of
> > > 	the Synopsys IP manual.
> > >
> > > Currently, due to the following differences, i2c designware cannot be
> > > fully reused
> > > 1.  For high and low level counting settings at different rates, i2c
> > >         starfive can use IC_SCL-H/LCNT to set SS, FM, FM+, UFM
> > > 2.  Interrupt clearing is achieved by writing 1 to the corresponding
> > >         bit of INTR_CLR, while designware reads different clearing
> > > 	registers
> > > 3.  Master and slave require separate probe callbacks and cannot rely
> > >         solely on the runtime mode switching provided by
> > i2c_dw_set_mode()
> > > 4.  The value of FIFO depth is not obtained through registers, but
> > >         written through DTS
> > 
> > NAK in this form. We well discourage code duplication and ugly ifdeffery with
> > full of __weak annotations that may not be present in the regular driver. There
> > is not even a tiny bit of justification for this nonsense.
> > 
> > TL;DR: this series needs much more work.
> > 
> > > I have written some poorly styled code to reduce changes to i2c designware
> > > and reuse its functions by keeping aa always true, for example
> > > 1.  the implementation of i2c-d w_probe_master() differs only for the two
> > >         IPs in i2c_dw_set_timits_master(). In order to reuse
> > > 	i2c_dw_probe_master(), i2c_dw_set_timits_master is declared as
> > > 	__weak. A better approach is to use a callback function, but using
> > > 	a callback function requires changing more i2c designware files.
> > > 	I don't know what the attitude of the community is
> > > 2.  For the operation of clearing interrupt flags, i2c designware reads
> > >         and i2c starfive writes. Therefore, in order not to modify the
> > > 	relevant logic of i2c designware, I added a write operation to
> > > 	sf_reg_read()
> > > So I think this version of the code is not allowed to merge, but I don't
> > > know how to handle this situation because if i2c designware is not changed
> > >  at all, we will have to write code that is similar to i2c designware.
> > >  Will this type of IP not be allowed to merge?
> 
> Thanks for the review.
> 
> In the future, the designware will be changed to the form of callback functions, 
> and then callback functions will be passed in i2c starry - * and implemented 
> using designware as a library

Why you can't specify your own regmap as it was done in Baikal case? What are
the obstacles to achieve that?

-- 
With Best Regards,
Andy Shevchenko