Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Mon, 7 Jan 2019 23:27:48 +0530
Himanshu Jha wrote:
> On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
> >
> > > On Dec 24, 2018, at 01:58, Himanshu Jha
> > > 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 {
> u16vref_mv; /* 0 2 */
> u8 clock_source_sel; /* 2 1 */
>
> /* XXX 1 byte hole, try to pack */
>
> u32ext_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 {
> u16vref_mv; /* 0 2 */
> u8 clock_source_sel; /* 2 1 */
>
> /* XXX 1 byte hole, try to pack */
>
> u32ext_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.gc13...@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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
>
> > On Dec 24, 2018, at 01:58, Himanshu Jha 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 {
u16vref_mv; /* 0 2 */
u8 clock_source_sel; /* 2 1 */
/* XXX 1 byte hole, try to pack */
u32ext_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 {
u16vref_mv; /* 0 2 */
u8 clock_source_sel; /* 2 1 */
/* XXX 1 byte hole, try to pack */
u32ext_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.gc13...@ziepe.ca/
Very likely to get more patches if the above patch gets merged.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Sat, 22 Dec 2018 21:58:25 -0500 indigo omega wrote: > Hello Jonathan, > > I'd be happy to help cleanup this driver, and I'd greatly appreciate some > direction as to what needs to be done. > > Thank you, > > Amir Ghorbanian Sure, I'll give the current state a quick review... Major items are that it needs devicetree bindings instead of platform data and there are a few unusual sysfs interfaces that may need documenting. Comments inline. > /* > * AD7190 AD7192 AD7193 AD7195 SPI ADC driver > * > * Copyright 2011-2015 Analog Devices Inc. > * > * Licensed under the GPL-2. > */ > > #include > #include > #include > #include > #include > #include > #include > #include > #include > #include > > #include > #include > #include > #include > #include > #include > #include > > #include "ad7192.h" > > /* Registers */ > #define AD7192_REG_COMM 0 /* Communications Register (WO, > 8-bit) */ > #define AD7192_REG_STAT 0 /* Status Register (RO, > 8-bit) */ > #define AD7192_REG_MODE 1 /* Mode Register (RW, > 24-bit */ > #define AD7192_REG_CONF 2 /* Configuration Register (RW, > 24-bit) */ > #define AD7192_REG_DATA 3 /* Data Register (RO, > 24/32-bit) */ > #define AD7192_REG_ID 4 /* ID Register (RO, 8-bit) */ > #define AD7192_REG_GPOCON 5 /* GPOCON Register (RO, 8-bit) */ > #define AD7192_REG_OFFSET 6 /* Offset Register (RW, 16-bit */ > /* (AD7792)/24-bit (AD7192)) */ > #define AD7192_REG_FULLSALE 7 /* Full-Scale Register */ > /* (RW, 16-bit (AD7792)/24-bit (AD7192)) */ > > /* Communications Register Bit Designations (AD7192_REG_COMM) */ > #define AD7192_COMM_WEN BIT(7) /* Write Enable */ > #define AD7192_COMM_WRITE 0 /* Write Operation */ > #define AD7192_COMM_READ BIT(6) /* Read Operation */ > #define AD7192_COMM_ADDR(x) (((x) & 0x7) << 3) /* Register Address */ > #define AD7192_COMM_CREAD BIT(2) /* Continuous Read of Data Register */ > > /* Status Register Bit Designations (AD7192_REG_STAT) */ > #define AD7192_STAT_RDY BIT(7) /* Ready */ > #define AD7192_STAT_ERR BIT(6) /* Error (Overrange, Underrange) > */ > #define AD7192_STAT_NOREF BIT(5) /* Error no external reference */ > #define AD7192_STAT_PARITYBIT(4) /* Parity */ > #define AD7192_STAT_CH3 BIT(2) /* Channel 3 */ > #define AD7192_STAT_CH2 BIT(1) /* Channel 2 */ > #define AD7192_STAT_CH1 BIT(0) /* Channel 1 */ > > /* Mode Register Bit Designations (AD7192_REG_MODE) */ > #define AD7192_MODE_SEL(x)(((x) & 0x7) << 21) /* Operation Mode Select */ > #define AD7192_MODE_SEL_MASK (0x7 << 21) /* Operation Mode Select Mask */ > #define AD7192_MODE_DAT_STA BIT(20) /* Status Register transmission */ > #define AD7192_MODE_CLKSRC(x) (((x) & 0x3) << 18) /* Clock Source Select */ > #define AD7192_MODE_SINC3 BIT(15) /* SINC3 Filter Select */ > #define AD7192_MODE_ACX BIT(14) /* AC excitation enable(AD7195 > only)*/ > #define AD7192_MODE_ENPAR BIT(13) /* Parity Enable */ > #define AD7192_MODE_CLKDIVBIT(12) /* Clock divide by 2 (AD7190/2 only)*/ > #define AD7192_MODE_SCYCLEBIT(11) /* Single cycle conversion */ > #define AD7192_MODE_REJ60 BIT(10) /* 50/60Hz notch filter */ > #define AD7192_MODE_RATE(x) ((x) & 0x3FF) /* Filter Update Rate Select */ > > /* Mode Register: AD7192_MODE_SEL options */ > #define AD7192_MODE_CONT 0 /* Continuous Conversion Mode */ > #define AD7192_MODE_SINGLE1 /* Single Conversion Mode */ > #define AD7192_MODE_IDLE 2 /* Idle Mode */ > #define AD7192_MODE_PWRDN 3 /* Power-Down Mode */ > #define AD7192_MODE_CAL_INT_ZERO 4 /* Internal Zero-Scale Calibration */ > #define AD7192_MODE_CAL_INT_FULL 5 /* Internal Full-Scale Calibration */ > #define AD7192_MODE_CAL_SYS_ZERO 6 /* System Zero-Scale Calibration */ > #define AD7192_MODE_CAL_SYS_FULL 7 /* System Full-Scale Calibration */ > > /* Mode Register: AD7192_MODE_CLKSRC options */ > #define AD7192_CLK_EXT_MCLK1_20 /* External 4.92 MHz Clock > connected*/ > /* from MCLK1 to MCLK2 */ > #define AD7192_CLK_EXT_MCLK2 1 /* External Clock applied to MCLK2 */ > #define AD7192_CLK_INT2 /* Internal 4.92 MHz Clock > not */ > /* available at the MCLK2 pin */ > #define AD7192_CLK_INT_CO 3 /* Internal 4.92 MHz Clock available*/ > /* at the MCLK2 pin */ > > /* Configuration Register Bit Designations (AD7192_REG_CONF) */ > > #define AD7192_CONF_CHOP BIT(23) /* CHOP enable */ > #define AD7192_CONF_REFSELBIT(20) /* REFIN1/REFIN2 Reference Select */ > #define
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
> On Dec 24, 2018, at 01:58, Himanshu Jha 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
>> ---
>
> 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 {
>>u16vref_mv;
>>u8clock_source_sel;
>>u32ext_clk_hz;
>> -boolrefin2_en;
>> -boolrej60_en;
>> -boolsinc3_en;
>> -boolchop_en;
>> -boolbuf_en;
>> -boolunipolar_en;
>> -boolburnout_curr_en;
>> +unsigned intrefin2_en : 1;
>> +unsigned intrej60_en : 1;
>> +unsigned intsinc3_en : 1;
>> +unsigned intchop_en : 1;
>> +unsigned intbuf_en : 1;
>> +unsigned intunipolar_en : 1;
>> +unsigned intburnout_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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
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
> ---
> 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;
> - boolrefin2_en;
> - boolrej60_en;
> - boolsinc3_en;
> - boolchop_en;
> - boolbuf_en;
> - boolunipolar_en;
> - boolburnout_curr_en;
> + unsigned intrefin2_en : 1;
> + unsigned intrej60_en : 1;
> + unsigned intsinc3_en : 1;
> + unsigned intchop_en : 1;
> + unsigned intbuf_en : 1;
> + unsigned intunipolar_en : 1;
> + unsigned intburnout_curr_en : 1;
Don't put spaces around the :. Just do:
unsigned intburnout_curr_en:1;
Otherwise it looks like part of a select statement or something.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
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
> ---
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;
> - boolrefin2_en;
> - boolrej60_en;
> - boolsinc3_en;
> - boolchop_en;
> - boolbuf_en;
> - boolunipolar_en;
> - boolburnout_curr_en;
> + unsigned intrefin2_en : 1;
> + unsigned intrej60_en : 1;
> + unsigned intsinc3_en : 1;
> + unsigned intchop_en : 1;
> + unsigned intbuf_en : 1;
> + unsigned intunipolar_en : 1;
> + unsigned intburnout_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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Fri, 21 Dec 2018 15:26:26 -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
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;
> - boolrefin2_en;
> - boolrej60_en;
> - boolsinc3_en;
> - boolchop_en;
> - boolbuf_en;
> - boolunipolar_en;
> - boolburnout_curr_en;
> + unsigned intrefin2_en : 1;
> + unsigned intrej60_en : 1;
> + unsigned intsinc3_en : 1;
> + unsigned intchop_en : 1;
> + unsigned intbuf_en : 1;
> + unsigned intunipolar_en : 1;
> + unsigned intburnout_curr_en : 1;
> };
>
> #endif /* IIO_ADC_AD7192_H_ */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: iio: ad7192: replaced bool in struct
Replaced bool in struct with unsigned int bitfield to conserve space and
more clearly define size of varibales
Signed-off-by: Amir Mahdi Ghorbanian
---
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;
- boolrefin2_en;
- boolrej60_en;
- boolsinc3_en;
- boolchop_en;
- boolbuf_en;
- boolunipolar_en;
- boolburnout_curr_en;
+ unsigned intrefin2_en : 1;
+ unsigned intrej60_en : 1;
+ unsigned intsinc3_en : 1;
+ unsigned intchop_en : 1;
+ unsigned intbuf_en : 1;
+ unsigned intunipolar_en : 1;
+ unsigned intburnout_curr_en : 1;
};
#endif /* IIO_ADC_AD7192_H_ */
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
