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

[Andy Shevchenko]

On Wed, Jun 03, 2026 at 06:09:24AM +0000, Lianfeng Ouyang wrote:
> > -----邮件原件-----
> > 发件人: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > 发送时间: 2026年6月3日 13:49
> > 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?
> 
> 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

So, it's not a DW per se? What the DW databook reflects the register layout and
other bits? I.o.w. which version of DW i2c IP is this?

> 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

This can be done in the custom regmap functions. You know that you may
redefine the regmap IO accessors and do whatever you want there based on
the registers. Yes, what you are probably talking about is a workflow.

But we basically need to see a patch-per-workflow change for each case
to understand the differences better. I.o.w. if you need, for instance,
separate or special IRQ handling part, create a patch to move existing
code to a callback with explanation why it's needed. All the same for
the timings and so on. It would be nice to have a datasheet at hand to
actually read and see the differences. Without that it's even hard to
propose better solutions. But current state is for sure no go.

> 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

This feature should be left at last, but altogether it smells like either
a completely new i2c DW design (like they did for DMA and SSI at some
point) and most likely will need a brand new driver explaining all of this
with a link to a datasheet.

> 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

These sounds like candidates for i2c-designware-lib.c, indeed.
But the rest most likely shouldn't be intervened at all. I.o.w.
please provide a detailed analysis (something like [1]) in
the cover letter, or just send an RFC on the driver design
before even starting considering actual coding.

> Hope I can answer your question, thank you

[1]: e539f435cb9c ("spi: dw: Add support for DesignWare DWC_ssi")


-- 
With Best Regards,
Andy Shevchenko