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

[Lianfeng Ouyang]

> -----邮件原件-----
> 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> 发送时间: 2026年6月3日 6:27
> 收件人: 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, 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?
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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

Best Regards,
Lianfeng Ouyang