| Message ID | 1545434786-15220-1-git-send-email-indigoomega021@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Staging: iio: ad7192: replaced bool in struct | expand |
On Fri, 21 Dec 2018 15:26:26 -0800 Amir Mahdi Ghorbanian <indigoomega021@gmail.com> wrote: > Replaced bool in struct with unsigned int bitfield to conserve space and > more clearly define size of varibales > > Signed-off-by: Amir Mahdi Ghorbanian <indigoomega021@gmail.com> Hi Amir, I'm a bit in two minds on this one. It's not a size critical structure and the advantage of bools is they make it clear the thing really is true or false. Another element here is that this is a platform data structure and unless we have users in the kernel tree, the chances that we'll maintain it's existence at all as we look to move this driver out of staging is very small! So slightly marginal advantage to the change on a structure that I certainly hope is shortly going away! Sorry, but I'm not convinced and won't be applying it. If you want to work on this driver though that would be great and I'm happy to do a review of it to highlight what other elements need cleaning up. Just say on the list and either I'll take a look or one of our other reviewers will. Thanks, Jonathan > --- > drivers/staging/iio/adc/ad7192.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h > index 7433a43..7d3e62f 100644 > --- a/drivers/staging/iio/adc/ad7192.h > +++ b/drivers/staging/iio/adc/ad7192.h > @@ -35,13 +35,13 @@ struct ad7192_platform_data { > u16 vref_mv; > u8 clock_source_sel; > u32 ext_clk_hz; > - bool refin2_en; > - bool rej60_en; > - bool sinc3_en; > - bool chop_en; > - bool buf_en; > - bool unipolar_en; > - bool burnout_curr_en; > + unsigned int refin2_en : 1; > + unsigned int rej60_en : 1; > + unsigned int sinc3_en : 1; > + unsigned int chop_en : 1; > + unsigned int buf_en : 1; > + unsigned int unipolar_en : 1; > + unsigned int burnout_curr_en : 1; > }; > > #endif /* IIO_ADC_AD7192_H_ */
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 > > Signed-off-by: Amir Mahdi Ghorbanian <indigoomega021@gmail.com> > --- There was some discussion on this at Outreachy list: https://groups.google.com/d/msg/outreachy-kernel/xpQAl-Gn8HA/yqcQRG_qBgAJ I think unless you post some statistics about 'conserving' space, it is unlikely that maintainers will apply it. This idea was originally given by Linus and that thread of discussion is worth reading too. > drivers/staging/iio/adc/ad7192.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h > index 7433a43..7d3e62f 100644 > --- a/drivers/staging/iio/adc/ad7192.h > +++ b/drivers/staging/iio/adc/ad7192.h > @@ -35,13 +35,13 @@ struct ad7192_platform_data { > u16 vref_mv; > u8 clock_source_sel; > u32 ext_clk_hz; > - bool refin2_en; > - bool rej60_en; > - bool sinc3_en; > - bool chop_en; > - bool buf_en; > - bool unipolar_en; > - bool burnout_curr_en; > + unsigned int refin2_en : 1; > + unsigned int rej60_en : 1; > + unsigned int sinc3_en : 1; > + unsigned int chop_en : 1; > + unsigned int buf_en : 1; > + unsigned int unipolar_en : 1; > + unsigned int burnout_curr_en : 1; > }; > > #endif /* IIO_ADC_AD7192_H_ */ > -- > 2.7.4 > Goodluck!
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 > > Signed-off-by: Amir Mahdi Ghorbanian <indigoomega021@gmail.com> > --- > drivers/staging/iio/adc/ad7192.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h > index 7433a43..7d3e62f 100644 > --- a/drivers/staging/iio/adc/ad7192.h > +++ b/drivers/staging/iio/adc/ad7192.h > @@ -35,13 +35,13 @@ struct ad7192_platform_data { > u16 vref_mv; > u8 clock_source_sel; > u32 ext_clk_hz; > - bool refin2_en; > - bool rej60_en; > - bool sinc3_en; > - bool chop_en; > - bool buf_en; > - bool unipolar_en; > - bool burnout_curr_en; > + unsigned int refin2_en : 1; > + unsigned int rej60_en : 1; > + unsigned int sinc3_en : 1; > + unsigned int chop_en : 1; > + unsigned int buf_en : 1; > + unsigned int unipolar_en : 1; > + unsigned int burnout_curr_en : 1; Don't put spaces around the :. Just do: unsigned int burnout_curr_en:1; Otherwise it looks like part of a select statement or something. regards, dan carpenter
> On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@gmail.com> 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. 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 :) - Matt >> >> Signed-off-by: Amir Mahdi Ghorbanian <indigoomega021@gmail.com> >> --- > > There was some discussion on this at Outreachy list: > https://groups.google.com/d/msg/outreachy-kernel/xpQAl-Gn8HA/yqcQRG_qBgAJ > > I think unless you post some statistics about 'conserving' space, > it is unlikely that maintainers will apply it. > > This idea was originally given by Linus and that thread of discussion > is worth reading too. > >> drivers/staging/iio/adc/ad7192.h | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h >> index 7433a43..7d3e62f 100644 >> --- a/drivers/staging/iio/adc/ad7192.h >> +++ b/drivers/staging/iio/adc/ad7192.h >> @@ -35,13 +35,13 @@ struct ad7192_platform_data { >> u16 vref_mv; >> u8 clock_source_sel; >> u32 ext_clk_hz; >> - bool refin2_en; >> - bool rej60_en; >> - bool sinc3_en; >> - bool chop_en; >> - bool buf_en; >> - bool unipolar_en; >> - bool burnout_curr_en; >> + unsigned int refin2_en : 1; >> + unsigned int rej60_en : 1; >> + unsigned int sinc3_en : 1; >> + unsigned int chop_en : 1; >> + unsigned int buf_en : 1; >> + unsigned int unipolar_en : 1; >> + unsigned int burnout_curr_en : 1; >> }; >> >> #endif /* IIO_ADC_AD7192_H_ */ >> -- >> 2.7.4 >> > > Goodluck! > > -- > Himanshu Jha > Undergraduate Student > Department of Electronics & Communication > Guru Tegh Bahadur Institute of Technology
On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote: > > > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@gmail.com> 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@ziepe.ca/ Very likely to get more patches if the above patch gets merged.
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: > > > > > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@gmail.com> 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@ziepe.ca/ > > 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
diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h index 7433a43..7d3e62f 100644 --- a/drivers/staging/iio/adc/ad7192.h +++ b/drivers/staging/iio/adc/ad7192.h @@ -35,13 +35,13 @@ struct ad7192_platform_data { u16 vref_mv; u8 clock_source_sel; u32 ext_clk_hz; - bool refin2_en; - bool rej60_en; - bool sinc3_en; - bool chop_en; - bool buf_en; - bool unipolar_en; - bool burnout_curr_en; + unsigned int refin2_en : 1; + unsigned int rej60_en : 1; + unsigned int sinc3_en : 1; + unsigned int chop_en : 1; + unsigned int buf_en : 1; + unsigned int unipolar_en : 1; + unsigned int burnout_curr_en : 1; }; #endif /* IIO_ADC_AD7192_H_ */
Replaced bool in struct with unsigned int bitfield to conserve space and more clearly define size of varibales Signed-off-by: Amir Mahdi Ghorbanian <indigoomega021@gmail.com> --- drivers/staging/iio/adc/ad7192.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)