Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210

[Andy Shevchenko]

On Wed, Jan 15, 2025 at 1:53 AM Antoni Pokusinski
<apokusinski01@xxxxxxxxx> wrote:
> On Tue, Jan 14, 2025 at 11:43:11AM +0200, Andy Shevchenko wrote:
> > On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski
> > <apokusinski01@xxxxxxxxx> wrote:

...

> > > > > +struct si7210_data {
> > > > > +       struct i2c_client *client;
> > > >
> > > > Do we really need a room for that? Isn't it derivable from the below
> > > > regmap? Also note the frequency of use of client vs. regmap. The
> > > > result in the object file can be much better if regmap becomes the
> > > > first member here. Check it (with bloat-o-meter, for example).
> > >
> > > I used arm-linux-nm and the bloat-o-meter to compare the sizes and it
> > > turned out that the version which contains the `i2c_client` has
> > > slightly smaller size actually. Here are the results:
> > >
> > > $ ./scripts/bloat-o-meter -p arm-linux-  ./old_si7210.ko  ./new_si7210.ko
> > > add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
> > > Function                                     old     new   delta
> > > si7210_probe                                 556     560      +4
> > > Total: Before=4021, After=4025, chg +0.10%
> > >
> > > Here is the diff (shortened for better readability) between
> > > the old_si7210.ko (uses `si7210_data->i2c_client`) and
> > > new_si7210.ko (does not use `si7210_data->i2c_client`):
> > >
> > >  struct si7210_data {
> > > -       struct i2c_client *client;
> > >         struct regmap *regmap;
> > > ...
> > >  static int si7210_device_wake(struct si7210_data *data)
> > >  {
> > > +       struct device *dev = regmap_get_device(data->regmap);
> > >         int ret;
> > >
> > > -       ret = i2c_smbus_read_byte(data->client);
> > > +       ret = i2c_smbus_read_byte(to_i2c_client(dev));
> > > ...
> > > static int si7210_probe(struct i2c_client *client)
> > >         data = iio_priv(indio_dev);
> > > -       data->client = client;
> > >
> > > Hence, I guess that it's actually better to leave the `i2c_client` as it is.
> >
> > I don't think you have tested all that I was talking about, i.e. have
> > you tried to swap the positions of client and regmap? What I expect is
> > that when you swap them you will see a good size reduction due to
> > pointer arithmetics becoming no-op for the regmap pointer. And then
> > the dropping of the client might waste all that beneficial size.
> >
>
> Ok, so I've tried to swap the `i2c_client` and `regmap` pointers and...
> there was no change shown by the bloat-o-meter. The only improvement was
> that the new object file (that is after moving the `regmap` to the
> beginning of the struct) was 8 bytes smaller in file size.
>
> Out of curiosity I've also tried moving
> the `regmap` further away in the structure (e.g. I placed it after the
> regulator and mutex) but there was still no change. I am a bit confused,
> since this behavior is different from what you described that it should
> be.

I haven't told "should" I have told "expected". So this means that for
the compiler in use and platform (architecture) there are no
differences. Good to have a confirmation for that. Nevertheless, I
would keep them swapped, because it's more natural to see the most
used member first at the structure.

> > > > > +       struct regmap *regmap;
> > > > > +       struct regulator *vdd;
> > > > > +       struct mutex fetch_lock; /* lock for a single measurement fetch */
> > > > > +       s8 temp_offset;
> > > > > +       s8 temp_gain;
> > > > > +       s8 scale_20_a[A_REGS_COUNT];
> > > > > +       s8 scale_200_a[A_REGS_COUNT];
> > > > > +       u8 curr_scale;
> > > > > +};


-- 
With Best Regards,
Andy Shevchenko