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

[Alex Elder <elder@ieee.org>]

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@gmail.com>
>>>
>>> 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@gmail.com>

. . .

>> 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