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

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

On Mon, Feb 02, 2026 at 02:39:28PM +0800, Binbin Zhou wrote:
> On Thu, Jan 29, 2026 at 10:38 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > 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:

...

> > > 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.
> 
> Sorry, I slacked off.
> How about the organized data structure as follows:
> 
> struct loongson2_i2c_msg {
>         u8      *buf;
>         u32     count;
>         int     result;
>         u8      addr;
>         bool    stop;
> };
> 
> struct loongson2_i2c_priv {
>         struct i2c_adapter              adapter;
>         struct completion               complete;
>         struct clk                      *clk;
>         struct regmap                   *regmap;
>         int                             speed;
>         int                             parent_rate;
>         struct loongson2_i2c_msg        msg;
> };

At the first glance looks okay to me.

-- 
With Best Regards,
Andy Shevchenko