On Mon, Mar 11, 2024 at 03:58:29PM +0000, Jonathan Cameron wrote:
> On Mon, 11 Mar 2024 12:05:07 +0200
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > On Mon, Mar 11, 2024 at 01:54:32AM +0100, Vasileios Amoiridis wrote:
> > > Bosch does not use unique BMPxxx_CHIP_ID for the different versions of
> > > the device which leads to misidentification of devices if their ID is
> > > used. Use a new value in the chip_info structure instead of the
> > > BMPxxx_CHIP_ID, in order to choose the regmap_bus to be used.
> >
> > ...
> >
> > > const struct regmap_config *regmap_config;
> > > + int spi_read_extra_byte;
> >
> > Why is it int and not boolean?
> > Also, please run `pahole` to see the best place for a new member.
>
> Whilst that's good in general, there aren't many of these structs (4ish)
> so if the 'cheapest' positioning isn't natural or hurts readability
> ignore what you get from pahole.
>
> Jonathan
>
Hello Andy, hello Jonathan,
Thank you for your feedback! Andy, I already used pahole as you suggested
already in one of my previous patch series for this driver, and the
result looks like it uses only 4 byte values so adding a bool would only
create a 3 byte hole. Apart from that, I noticed that for example, the
num_chip_ids value could have easily been a u8 but instead it's an int,
I guess to satisfy the alignment requirements. Also the id_reg could
easily be a u8 but instead it is an unsigned int. Maybe in the future, if
more values are added it will be good to have a re-organization of those
values. I can look at it, after the fix is done and it is on the mainline.
Finally, I chose this specific position because it's next to the
regmap_config, and the new value affects the regmap_bus so in my opinion
it helps readability.
pahole --class_name=bmp280_chip_info bmp280-core.dwo
struct bmp280_chip_info {
unsigned int id_reg; /* 0 4 */
const u8 * chip_id; /* 4 4 */
int num_chip_id; /* 8 4 */
const struct regmap_config * regmap_config; /* 12 4 */
int spi_read_extra_byte; /* 16 4 */
const struct iio_chan_spec * channels; /* 20 4 */
int num_channels; /* 24 4 */
unsigned int start_up_time; /* 28 4 */
const int * oversampling_temp_avail; /* 32 4 */
int num_oversampling_temp_avail; /* 36 4 */
int oversampling_temp_default; /* 40 4 */
const int * oversampling_press_avail; /* 44 4 */
int num_oversampling_press_avail; /* 48 4 */
int oversampling_press_default; /* 52 4 */
const int * oversampling_humid_avail; /* 56 4 */
int num_oversampling_humid_avail; /* 60 4 */
/* --- cacheline 1 boundary (64 bytes) --- */
int oversampling_humid_default; /* 64 4 */
const int * iir_filter_coeffs_avail; /* 68 4 */
int num_iir_filter_coeffs_avail; /* 72 4 */
int iir_filter_coeff_default; /* 76 4 */
const int * sampling_freq_avail; /* 80 4 */
int num_sampling_freq_avail; /* 84 4 */
int sampling_freq_default; /* 88 4 */
int (*chip_config)(struct bmp280_data *); /* 92 4 */
int (*read_temp)(struct bmp280_data *, int *, int *); /* 96 4 */
int (*read_press)(struct bmp280_data *, int *, int *); /* 100 4 */
int (*read_humid)(struct bmp280_data *, int *, int *); /* 104 4 */
int (*read_calib)(struct bmp280_data *); /* 108 4 */
int (*preinit)(struct bmp280_data *); /* 112 4 */
/* size: 116, cachelines: 2, members: 29 */
/* last cacheline: 52 bytes */
};
Thanks,
Vasilis
> >
>
