| Message ID | 20240527-ad4111-v3-6-7e9eddbbd3eb@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for AD411x | expand |
On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > Reduce the size used by the device info struct by packing the bool > fields within the same byte. This reduces the struct size from 52 bytes > to 44 bytes. > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > --- > drivers/iio/adc/ad7173.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > index 328685ce25e0..e8357a21d513 100644 > --- a/drivers/iio/adc/ad7173.c > +++ b/drivers/iio/adc/ad7173.c > @@ -179,15 +179,15 @@ struct ad7173_device_info { > unsigned int clock; > unsigned int id; > char *name; > - bool has_current_inputs; > - bool has_vcom_input; > - bool has_temp; > + bool has_current_inputs :1; > + bool has_vcom_input :1; > + bool has_temp :1; > /* ((AVDD1 − AVSS)/5) */ > - bool has_common_input; > - bool has_input_buf; > - bool has_int_ref; > - bool has_ref2; > - bool higher_gpio_bits; > + bool has_common_input :1; > + bool has_input_buf :1; > + bool has_int_ref :1; > + bool has_ref2 :1; > + bool higher_gpio_bits :1; > u8 num_gpios; > }; > > This is really a very micro optimization... I would drop it tbh but no strong feelings about it. - Nuno Sá
On 5/29/24 7:23 AM, Nuno Sá wrote: > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >> >> Reduce the size used by the device info struct by packing the bool >> fields within the same byte. This reduces the struct size from 52 bytes >> to 44 bytes. >> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> >> --- >> drivers/iio/adc/ad7173.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c >> index 328685ce25e0..e8357a21d513 100644 >> --- a/drivers/iio/adc/ad7173.c >> +++ b/drivers/iio/adc/ad7173.c >> @@ -179,15 +179,15 @@ struct ad7173_device_info { >> unsigned int clock; >> unsigned int id; >> char *name; >> - bool has_current_inputs; >> - bool has_vcom_input; >> - bool has_temp; >> + bool has_current_inputs :1; >> + bool has_vcom_input :1; >> + bool has_temp :1; >> /* ((AVDD1 − AVSS)/5) */ >> - bool has_common_input; >> - bool has_input_buf; >> - bool has_int_ref; >> - bool has_ref2; >> - bool higher_gpio_bits; >> + bool has_common_input :1; >> + bool has_input_buf :1; >> + bool has_int_ref :1; >> + bool has_ref2 :1; >> + bool higher_gpio_bits :1; >> u8 num_gpios; >> }; >> >> > > This is really a very micro optimization... I would drop it tbh but no strong > feelings about it. > > - Nuno Sá This only considers RAM size and not code size too. At least on ARM arch every time we read or write to one of these fields, the code is now implicitly `((field & 0x1) >> bits)` so two extra assembly instructions for each read and write. This could be bigger than the size saved in the structs.
On Wed, 2024-05-29 at 15:32 -0500, David Lechner wrote: > On 5/29/24 7:23 AM, Nuno Sá wrote: > > On Mon, 2024-05-27 at 20:02 +0300, Dumitru Ceclan via B4 Relay wrote: > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > > > Reduce the size used by the device info struct by packing the bool > > > fields within the same byte. This reduces the struct size from 52 bytes > > > to 44 bytes. > > > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > --- > > > drivers/iio/adc/ad7173.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > > > index 328685ce25e0..e8357a21d513 100644 > > > --- a/drivers/iio/adc/ad7173.c > > > +++ b/drivers/iio/adc/ad7173.c > > > @@ -179,15 +179,15 @@ struct ad7173_device_info { > > > unsigned int clock; > > > unsigned int id; > > > char *name; > > > - bool has_current_inputs; > > > - bool has_vcom_input; > > > - bool has_temp; > > > + bool has_current_inputs :1; > > > + bool has_vcom_input :1; > > > + bool has_temp :1; > > > /* ((AVDD1 − AVSS)/5) */ > > > - bool has_common_input; > > > - bool has_input_buf; > > > - bool has_int_ref; > > > - bool has_ref2; > > > - bool higher_gpio_bits; > > > + bool has_common_input :1; > > > + bool has_input_buf :1; > > > + bool has_int_ref :1; > > > + bool has_ref2 :1; > > > + bool higher_gpio_bits :1; > > > u8 num_gpios; > > > }; > > > > > > > > > > This is really a very micro optimization... I would drop it tbh but no strong > > feelings about it. > > > > - Nuno Sá > > This only considers RAM size and not code size too. At least on ARM arch > every time we read or write to one of these fields, the code is now > implicitly `((field & 0x1) >> bits)` so two extra assembly instructions > for each read and write. This could be bigger than the size saved in > the structs. > > very good point... - Nuno Sá
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c index 328685ce25e0..e8357a21d513 100644 --- a/drivers/iio/adc/ad7173.c +++ b/drivers/iio/adc/ad7173.c @@ -179,15 +179,15 @@ struct ad7173_device_info { unsigned int clock; unsigned int id; char *name; - bool has_current_inputs; - bool has_vcom_input; - bool has_temp; + bool has_current_inputs :1; + bool has_vcom_input :1; + bool has_temp :1; /* ((AVDD1 − AVSS)/5) */ - bool has_common_input; - bool has_input_buf; - bool has_int_ref; - bool has_ref2; - bool higher_gpio_bits; + bool has_common_input :1; + bool has_input_buf :1; + bool has_int_ref :1; + bool has_ref2 :1; + bool higher_gpio_bits :1; u8 num_gpios; };