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

[Lianfeng Ouyang]

> -----邮件原件-----
> 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 发送时间: 2026年6月3日 13:49
> 收件人: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> 抄送: Andi Shyti <andi.shyti@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 主题: Re: 回复: [PATCH v2 0/3] i2c: Add Starfive JHB100 I2C master/slave
> support
> 
> On Wed, Jun 03, 2026 at 05:31:38AM +0000, Lianfeng Ouyang wrote:
> > > -----邮件原件-----
> > > 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > 发送时间: 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
> 

Hi Andy,

The main reasons are as follows
1.  For high and low level counting settings at different rates, i2c
starfive just use IC_SCL_H/LCNT to set SS, FM, FM+, UFM, 
====> Therefore, it is not possible to directly use the i2c_dew_set_timits_master() 
      of designware, Because the definition of registers has changed

2.  Interrupt clearing is achieved by writing 1 to the corresponding
bit of INTR_CLR, while designware reads different clearing
registers
====> The way of operating registers is different, so it cannot be 
      distinguished solely by address or offset, and can only be adapted 
      to operations belonging to i2c starfive through callback functions

3.  Master and slave require separate probe callbacks and cannot rely
solely on the runtime mode switching provided by i2c_dw_set_mode()
====> There are host and slave IP addresses separately, unlike designware 
      where one IP supports two roles, because the logic of switching roles 
      cannot be distinguished by address and offset

4.  The value of FIFO depth is not obtained through registers, but
written through DTS
====> The register does not have information on the depth of FIFO, 
      and manual writing of register settings is required

In addition to the above four points, the initialization, transmission, and interrupts 
of i2c Starfive are basically the same as i2c Desirware. Therefore, we hope to reuse 
i2c designware instead of re implementing the i2c Starfive driver, otherwise the code repetition will be high

Hope I can answer your question, thank you