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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vf-zdsh6CP3XX6jyjVutch9Z_iH78zrpaFkt9WkP=qz4w@mail.gmail.com>
Date: Tue, 14 Jan 2025 11:43:11 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, andrej.skvortzov@...il.com, neil.armstrong@...aro.org, 
	icenowy@...c.io, megi@....cz, danila@...xyga.com, 
	javier.carrasco.cruz@...il.com, andy@...nel.org, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210

On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski
<apokusinski01@...il.com> wrote:

> Thanks for the review. I'm currently implementing some changes in the
> driver according to the review, however I have some doubts regarding
> removal of the `i2c_client` from `si7210_data`.

...

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

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