Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 12 Jan 2019 16:59:38 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Himanshu Jha <himanshujha199640@gmail.com>
Cc:     Matt Ranostay <matt.ranostay@konsulko.com>,
        Amir Mahdi Ghorbanian <indigoomega021@gmail.com>,
        lars@metafoo.de, Michael.Hennerich@analog.com, knaack.h@gmx.de,
        pmeerw@pmeerw.net, gregkh@linuxfoundation.org,
        linux-iio@vger.kernel.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
Message-ID: <20190112165938.0a7baa50@archlinux>
In-Reply-To: <20190107175748.GA29220@himanshu-Vostro-3559>
References: <1545434786-15220-1-git-send-email-indigoomega021@gmail.com>
        <20181224095823.GA17061@himanshu-Vostro-3559>
        <8E720BF2-7AFC-4FFA-8FA2-5A403FD5DD1D@konsulko.com>
        <20190107175748.GA29220@himanshu-Vostro-3559>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 7 Jan 2019 23:27:48 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
> >  =20
> > > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@gmail.com>=
 wrote:
> > >  =20
> > >> On Fri, Dec 21, 2018 at 03:26:26PM -0800, Amir Mahdi Ghorbanian wrot=
e:
> > >> Replaced bool in struct with unsigned int bitfield to conserve space=
 and
> > >> more clearly define size of varibales =20
> >=20
> > Important thing to note is depending on padding, alignment, and positio=
n of field it probably won=E2=80=99t save any space. =20
>=20
> Well, then what do you say about the following results ;-)
>=20
> Before:
> ------
>=20
> himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_dat=
a drivers/staging/iio/adc/ad7192.o
> struct ad7192_platform_data {
> 	u16                        vref_mv;              /*     0     2 */
> 	u8                         clock_source_sel;     /*     2     1 */
>=20
> 	/* XXX 1 byte hole, try to pack */
>=20
> 	u32                        ext_clk_hz;           /*     4     4 */
> 	bool                       refin2_en;            /*     8     1 */
> 	bool                       rej60_en;             /*     9     1 */
> 	bool                       sinc3_en;             /*    10     1 */
> 	bool                       chop_en;              /*    11     1 */
> 	bool                       buf_en;               /*    12     1 */
> 	bool                       unipolar_en;          /*    13     1 */
> 	bool                       burnout_curr_en;      /*    14     1 */
>=20
> 	/* size: 16, cachelines: 1, members: 10 */
> 	/* sum members: 14, holes: 1, sum holes: 1 */
> 	/* padding: 1 */
> 	/* last cacheline: 16 bytes */
> };
>=20
> After:
> -----
>=20
> himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_dat=
a drivers/staging/iio/adc/ad7192.o
> struct ad7192_platform_data {
> 	u16                        vref_mv;              /*     0     2 */
> 	u8                         clock_source_sel;     /*     2     1 */
>=20
> 	/* XXX 1 byte hole, try to pack */
>=20
> 	u32                        ext_clk_hz;           /*     4     4 */
> 	unsigned int               refin2_en:1;          /*     8:31  4 */
> 	unsigned int               rej60_en:1;           /*     8:30  4 */
> 	unsigned int               sinc3_en:1;           /*     8:29  4 */
> 	unsigned int               chop_en:1;            /*     8:28  4 */
> 	unsigned int               buf_en:1;             /*     8:27  4 */
> 	unsigned int               unipolar_en:1;        /*     8:26  4 */
> 	unsigned int               burnout_curr_en:1;    /*     8:25  4 */
>=20
> 	/* size: 12, cachelines: 1, members: 10 */
> 	/* sum members: 11, holes: 1, sum holes: 1 */
> 	/* bit_padding: 25 bits */
> 	/* last cacheline: 12 bytes */
> };
>=20
> > Also for internal unpacked structures it really makes little sense to s=
ave a few bytes of data. Don=E2=80=99t read into that packed structures are=
 good.. they usually aren=E2=80=99t :) =20
>=20
> Yes, agreed, but I often see patches to save few bytes by marking
> array as static.
>=20
> There is some effort in this direction:
> https://lore.kernel.org/lkml/20181221235230.GC13168@ziepe.ca/
>=20
> Very likely to get more patches if the above patch gets merged.
>=20
The one additional thing that is relevant here is that we will probably
drop the whole structure anyway as part of moving it out of staging.
There might be some equivalent elements stored elsewhere, but quite
likely it won't be all of them.  As a result I'm not particularly keen
on patches to clean platform data up.

+ we really are dealing with trivial amounts of data here..

J