| Message ID | 20240311005432.1752853-1-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: Fixes SPI support for BMP3xx devices | expand |
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.
On Mon, 11 Mar 2024 12:05:07 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 >
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@linux.intel.com> 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 > > >
On Mon, 11 Mar 2024 01:54:32 +0100 Vasileios Amoiridis <vassilisamir@gmail.com> 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. > > Fixes: a9dd9ba32311 ("iio: pressure: Fixes BMP38x and BMP390 SPI support") > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> Other than switching to a bool as Andy suggested, this looks fine to me. Jonathan > --- > drivers/iio/pressure/bmp280-core.c | 1 + > drivers/iio/pressure/bmp280-spi.c | 9 ++------- > drivers/iio/pressure/bmp280.h | 1 + > 3 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index fe8734468ed3..5ea9039caf75 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -1233,6 +1233,7 @@ const struct bmp280_chip_info bmp380_chip_info = { > .chip_id = bmp380_chip_ids, > .num_chip_id = ARRAY_SIZE(bmp380_chip_ids), > .regmap_config = &bmp380_regmap_config, > + .spi_read_extra_byte = 1, > .start_up_time = 2000, > .channels = bmp380_channels, > .num_channels = 2, > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c > index a444d4b2978b..3a5fec5d47fd 100644 > --- a/drivers/iio/pressure/bmp280-spi.c > +++ b/drivers/iio/pressure/bmp280-spi.c > @@ -96,15 +96,10 @@ static int bmp280_spi_probe(struct spi_device *spi) > > chip_info = spi_get_device_match_data(spi); > > - switch (chip_info->chip_id[0]) { > - case BMP380_CHIP_ID: > - case BMP390_CHIP_ID: > + if (chip_info->spi_read_extra_byte) > bmp_regmap_bus = &bmp380_regmap_bus; > - break; > - default: > + else > bmp_regmap_bus = &bmp280_regmap_bus; > - break; > - } > > regmap = devm_regmap_init(&spi->dev, > bmp_regmap_bus, > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h > index 4012387d7956..70bceaccf447 100644 > --- a/drivers/iio/pressure/bmp280.h > +++ b/drivers/iio/pressure/bmp280.h > @@ -423,6 +423,7 @@ struct bmp280_chip_info { > int num_chip_id; > > const struct regmap_config *regmap_config; > + int spi_read_extra_byte; > > const struct iio_chan_spec *channels; > int num_channels;
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index fe8734468ed3..5ea9039caf75 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -1233,6 +1233,7 @@ const struct bmp280_chip_info bmp380_chip_info = { .chip_id = bmp380_chip_ids, .num_chip_id = ARRAY_SIZE(bmp380_chip_ids), .regmap_config = &bmp380_regmap_config, + .spi_read_extra_byte = 1, .start_up_time = 2000, .channels = bmp380_channels, .num_channels = 2, diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c index a444d4b2978b..3a5fec5d47fd 100644 --- a/drivers/iio/pressure/bmp280-spi.c +++ b/drivers/iio/pressure/bmp280-spi.c @@ -96,15 +96,10 @@ static int bmp280_spi_probe(struct spi_device *spi) chip_info = spi_get_device_match_data(spi); - switch (chip_info->chip_id[0]) { - case BMP380_CHIP_ID: - case BMP390_CHIP_ID: + if (chip_info->spi_read_extra_byte) bmp_regmap_bus = &bmp380_regmap_bus; - break; - default: + else bmp_regmap_bus = &bmp280_regmap_bus; - break; - } regmap = devm_regmap_init(&spi->dev, bmp_regmap_bus, diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index 4012387d7956..70bceaccf447 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -423,6 +423,7 @@ struct bmp280_chip_info { int num_chip_id; const struct regmap_config *regmap_config; + int spi_read_extra_byte; const struct iio_chan_spec *channels; int num_channels;
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. Fixes: a9dd9ba32311 ("iio: pressure: Fixes BMP38x and BMP390 SPI support") Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 1 + drivers/iio/pressure/bmp280-spi.c | 9 ++------- drivers/iio/pressure/bmp280.h | 1 + 3 files changed, 4 insertions(+), 7 deletions(-)