Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Alex Elder]

On 2/13/25 5:38 AM, Troy Mitchell wrote:
> Hi! Alex.
> Thanks for ur detailed review.

Troy, I see you've sent a new version, and I'm going to
provide a review for it today.  But I somehow missed
this message, and it looks like I should have responded.

I know it's late, but I'm doing that now anyway.

> On 2025/2/12 05:39, Alex Elder wrote:
> > On 11/25/24 12:49 AM, Troy Mitchell wrote:
> > > From: Troy Mitchell <troymitchell988@xxxxxxxxx>
> > >
> > > This patch introduces basic I2C support for the SpacemiT K1 SoC,
> > > utilizing interrupts for transfers.
> > >
> > > The driver has been tested using i2c-tools on a Bananapi-F3 board,
> > > and basic I2C read/write operations have been confirmed to work.
> > >
> > > Signed-off-by: Troy Mitchell <TroyMitchell988@xxxxxxxxx>

. . .

> > The descriptions below are also good.  Maybe be consistent
> > with your capitalization in comments?  (Though I suppose
> > the capitalization above is to explain the name of the
> > register.)
> Yes. The above is to explain the full name.
> So I still need to keep them consistent?

You'll never get everything consistent, and even getting
two people to agree on what things should be consistent
might be difficult.

I think what you have is OK, but I might (for example)
capitalize every comment (or *not* capitalize).

Not a big deal.

> > > +#define CR_START        BIT(0)        /* start bit */
> > > +#define CR_STOP         BIT(1)        /* stop bit */

. . .

> > For the *IE symbols that follow, why not use *_IE as the spec
> > does?
> The symbols follow the datasheet of SpacemiT.
> Even if the data sheet doesn't have IE, should I add it myself?

I think it's good to use naming that matches what the hardware
specifications say.  That said, it doesn't always work well.
I worked on a driver where most of the hardware names were
30 or more characters wide.  That leads to unreadable code.

I would *much* rather have code readability than have the
symbol names match the hardware spec.  So I personally
focus on that, and just try to ensure it's easy to map
the name in the code back to what it represents in the
hardware.

> > > +#define CR_ALDIE        BIT(18)        /* enable arbitration interrupt */
> > > +#define CR_DTEIE        BIT(19)        /* enable tx interrupts */
> > > +#define CR_DRFIE        BIT(20)        /* enable rx interrupts */

. . .

> > Is it simply assumed to be already configured that way?  (This
> > driver doesn't set it at initialization time.)  Someday
> > we should set things up explicitly.
> yes. by dts.
> I will handle frequency that is from dts in next version.

In general it is a bad idea to add new features between
versions of a patch series--unless specifically requested
by a reviewer.  Once the series is accepted it should be easy
to add features later.

> > The only place the next symbol is used is in computing a
> > custom timeout period before initiating a transfer.
> Yes, but I wanna explain the freq number is fast mode.
> So I define it here.
> But don't worry, I will drop it in next version and accept freq from dts.

. . .

> > The next function only resets the bus if it's not idle (where
> > "idle" means both SCL and SDA are high).  Based on the name
> > of the function, I'd expect it to always reset.  In other
> > words, I'd rather this be named something slightly different,
> > or have the check of SDA and SCL be done in the caller(s).
> How about `spacemit_i2c_conditional_reset`?

That name is fine, though I think I'll be commenting on it
in my review shortly.

> Because I am worried that in the future, when the fifo or other reset is called,
> SDA and SCL will always be checked.

If you add a feature in the future that makes the current code not
make sense, you can change the code *then* to be more suitable.
Don't get too far ahead of yourself.

Write code that does the job, and is readable and simple.

> > > +static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
> > > +{
> > > +    u32 status;
> > > +
> > > +    /* if bus is locked, reset unit. 0: locked */
> > > +    status = readl(i2c->base + IBMR);
> > > +    if ((status & BMR_SDA) && (status & BMR_SCL))
> > > +        return;
> > > +
> > > +    spacemit_i2c_reset(i2c);
> > > +    usleep_range(10, 20);
> > > +
> > > +    /* check scl status again */
> > > +    status = readl(i2c->base + IBMR);
> > > +    if (!(status & BMR_SCL))
> > > +        dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
> > > +}
> > > +
> > > +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
> > > +{
> > > +    int ret = 0;
> > > +    u32 val;
> > > +
> >
> > I think the next 4 lines can be deleted.  The readl_poll_timeout()
> > immediately does what they do, without delay.
> The register value is always returned directly。
> but it needs to be judged if the bus is busy by !(val & (SR_UB | SR_IBB).

I might be wrong, but my point was that the readl_poll_timeout()
call does exactly what you're doing here.  So you could simply
do the readl_poll_timeout() without doing this readl() call first.

> > > +    val = readl(i2c->base + ISR);
> > > +    if (likely(!(val & (SR_UB | SR_IBB))))
> > > +        return 0;
> > > +
> > > +    ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
> > > +                 1500, I2C_BUS_RECOVER_TIMEOUT);
> > > +    if (unlikely(ret)) {
> > > +        spacemit_i2c_reset(i2c);
> >
> > If readl_poll_timeout() returns non-zero, it is -ETIMEDOUT.
> > Why change it to -EAGAIN?  (It ultimately gets consumed by
> > spacemit_i2c_xfer(), which handles -ETIMEDOUT and -EAGAIN
> > identically.
> I will drop it.

. . .

> > The following function is used in two places, but I don't think
> > it really adds any value.  Just open-code the writel() call in
> > those two spots.
> I want to explain more clearly what is done in the init phase, so I put them
> into a function and assign values step by step.
> Is it not necessary?
> or maybe I can add `inline`.

No.  People accustomed to reading kernel code know exactly
what a call to writel() does.  When you wrap that in a
function like this, it seems like you might be doing
something more complicated than that--and it's distracting.

It is best to simply use writel() (in this case) and get
rid of this trivial wrapper function.  Rely on well-named
variables to make things clearer, and if you think there's
something more, say so in a comment.

Also, much like the likely() and unlikely() calls, it is
almost never warranted to use an inline function in a C
file.  There are some cases where it serves a purpose,
but in general you can depend on the compiler to do the
inlining if it improves the way the code executes.

> > > +static inline void
> > > +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
> > > +{
> > > +    writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
> > > +}

. . .

That's enough.  I'm going to review your new version now.

					-Alex