On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski
<apokusinski01@xxxxxxxxx> 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
