Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jan 29, 2026 at 04:07:06PM +0800, Binbin Zhou wrote:
> On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:

...

> > > +/*
> >
> > It's not marked as kernel-doc, but looks very much like that. Why?
> > Same Q for *all* cases like this.
> 
> I didn't intend to mark it in kernel-doc; I just wanted to comment on
> the variable.
> Is removing the `@` symbol sufficient?

But why? If you want it being properly documented, make sure it follows
the regular format.

> > > + * struct loongson2_i2c_msg - client specific data
> > > + * @addr: 8-bit slave addr, including r/w bit
> > > + * @count: number of bytes to be transferred
> > > + * @buf: data buffer
> > > + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> > > + * @result: result of the transfer
> > > + */
> > > +struct loongson2_i2c_msg {
> > > +     u8 addr;
> > > +     u32 count;
> > > +     u8 *buf;
> > > +     bool stop;
> > > +     int result;
> >
> > Run `pahole` and amend *all* data types accordingly.
> 
>  Moving 'stop' from after 'buf' to after 'addr'.
> >
> > > +};

...

> After run `pahole`, the structs are reorganized as follow:
> 
> pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
> i2c-ls2x-v2.o
> struct loongson2_i2c_priv {
>         struct i2c_adapter         adapter
> __attribute__((__aligned__(8))); /*     0  1064 */
>         /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
> */
>         struct clk *               clk;                  /*  1064
> 8 */
>         struct completion          complete;             /*  1072
> 32 */
>         /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
> */
>         struct regmap *            regmap;               /*  1104

It's better to keep pointers like clk and regmap next to each other.
They are semantically coupled as "resources".

> 8 */
>         int                        speed;                /*  1112
> 4 */
>         int                        parent_rate;          /*  1116
> 4 */
>         struct loongson2_i2c_msg   msg;                  /*  1120
> 24 */
> 
>         /* XXX last struct has 4 bytes of padding */
> 
>         /* size: 1144, cachelines: 18, members: 7 */
>         /* paddings: 1, sum paddings: 4 */
>         /* forced alignments: 1 */
>         /* last cacheline: 56 bytes */
> } __attribute__((__aligned__(8)));

...

> pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
> i2c-ls2x-v2.o
> struct loongson2_i2c_msg {
>         u8                         addr;                 /*     0
> 1 */
>         bool                       stop;                 /*     1
> 1 */
> 
>         /* XXX 2 bytes hole, try to pack */

>         u32                        count;                /*     4
> 4 */
>         u8 *                       buf;                  /*     8
> 8 */

Also think about it (don't blindly follow the `pahole` automatic mode).
This is much better if you move the pointer to be the first, followed
by a count, result, and others.

>         int                        result;               /*    16
> 4 */
> 
>         /* size: 24, cachelines: 1, members: 5 */
>         /* sum members: 18, holes: 1, sum holes: 2 */
>         /* padding: 4 */
>         /* last cacheline: 24 bytes */
> };

TL;DR: don't use `pahole` blindly. Use the common sense.

...

> > > +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> > > +{
> > > +     u32 status;
> > > +     int ret;
> > > +
> > > +     ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> > > +                                    !(status & LOONGSON2_I2C_SR2_BUSY),
> > > +                                    LOONGSON2_I2C_FREE_SLEEP_US,
> > > +                                    LOONGSON2_I2C_FREE_TIMEOUT_US);
> > > +     if (ret) {
> > > +             dev_dbg(priv->dev, "I2C bus free failed.\n");
> >
> > > +             ret = -EBUSY;
> >
> > Why?! What's wrong with the error code returned in ret?
> 
> I want to indicate the bus busy state if it times out.

This is not an answer. So, why do you remap error code. What's wrong with
the callee's one?

> > > +     }
> > > +
> > > +     return ret;
> > > +}

...

> > Ah, it seems it's a helper. Please, return the error code from it as int and
> > not irqreturn_t, this will make things clearer.
> 
> How about just define it as void:
> 
> static void loongson2_i2c_isr_error(u32 status, void *data)
> {
> .........
>         if (status & LOONGSON2_I2C_SR1_ARLO) {
>          ........
>                 msg->result = -EAGAIN;
>                 goto out;
>         }
>         if (status & LOONGSON2_I2C_SR1_AF) {
>            .......
>                 msg->result = -EIO;
>                 goto out;
>         }
>         if (status & LOONGSON2_I2C_SR1_BERR) {
>          .........
>                 msg->result = -EIO;
>                 goto out;
>         }
> 
> out:
>         loongson2_i2c_disable_irq(priv);
>         complete(&priv->complete);
> }
> 
> and in loongson2_i2c_isr_event(), I reference it as:
> 
>         if (status & LOONGSON2_I2C_SR1_ITERREN_MASK) {
>                 loongson2_i2c_isr_error(status, data);
>                 return IRQ_NONE;
>         }

> Its return value is meaningless for loongson2_i2c_isr_event().

Works for me.

...

> > > +     u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
> >
> > Split assignment...

> > ...to be here, which improves readability (no need to go somewhere up in
> > the code to see what this is about.
> 
> ok, I will put  `possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;` here.
> 
> > > +     /* Update possible_status if buffer interrupt is enabled */
> > > +     if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> > > +             possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;

I expect something like this:

		/* Update possible_status if buffer interrupt is enabled */
		possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
		if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
			possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;

...

> > > +     strscpy(adap->name, pdev->name, sizeof(adap->name));
> >
> > 2-arguments version is even better.
> 
> Sorry, I'm not quite sure what you mean by `2-arguments version.`

Use 2 arguments instead of 3. I.o.w. just drop the third argument.

-- 
With Best Regards,
Andy Shevchenko