Re: [PATCH] Staging: iio: ad7192: replaced bool in struct

[Jonathan Cameron]

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

> On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
> >   
> > > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@xxxxxxxxx> wrote:
> > >   
> > >> On Fri, Dec 21, 2018 at 03:26:26PM -0800, Amir Mahdi Ghorbanian wrote:
> > >> Replaced bool in struct with unsigned int bitfield to conserve space and
> > >> more clearly define size of varibales  
> > 
> > Important thing to note is depending on padding, alignment, and position of field it probably wonât save any space.  
> 
> Well, then what do you say about the following results ;-)
> 
> Before:
> ------
> 
> himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data drivers/staging/iio/adc/ad7192.o
> struct ad7192_platform_data {
> 	u16                        vref_mv;              /*     0     2 */
> 	u8                         clock_source_sel;     /*     2     1 */
> 
> 	/* XXX 1 byte hole, try to pack */
> 
> 	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 */
> 
> 	/* size: 16, cachelines: 1, members: 10 */
> 	/* sum members: 14, holes: 1, sum holes: 1 */
> 	/* padding: 1 */
> 	/* last cacheline: 16 bytes */
> };
> 
> After:
> -----
> 
> himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data drivers/staging/iio/adc/ad7192.o
> struct ad7192_platform_data {
> 	u16                        vref_mv;              /*     0     2 */
> 	u8                         clock_source_sel;     /*     2     1 */
> 
> 	/* XXX 1 byte hole, try to pack */
> 
> 	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 */
> 
> 	/* size: 12, cachelines: 1, members: 10 */
> 	/* sum members: 11, holes: 1, sum holes: 1 */
> 	/* bit_padding: 25 bits */
> 	/* last cacheline: 12 bytes */
> };
> 
> > Also for internal unpacked structures it really makes little sense to save a few bytes of data. Donât read into that packed structures are good.. they usually arenât :)  
> 
> Yes, agreed, but I often see patches to save few bytes by marking
> array as static.
> 
> There is some effort in this direction:
> https://lore.kernel.org/lkml/20181221235230.GC13168@xxxxxxxx/
> 
> Very likely to get more patches if the above patch gets merged.
> 
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