[PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[Vasileios Amoiridis]

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(-)

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;
-- 
2.25.1

Re: [PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[Jonathan Cameron]

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

> 

Re: [PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[Vasileios Amoiridis]

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
> > 
> 

Re: [PATCH] iio: pressure: Fixes SPI support for BMP3xx devices

[Jonathan Cameron]

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;