diff mbox series

drivers: iio: pressure: Fixes BMP38x and BMP390 SPI support

Message ID 20240219191359.18367-1-vassilisamir@gmail.com
State New
Headers
Series drivers: iio: pressure: Fixes BMP38x and BMP390 SPI support |

Commit Message

Vasileios Amoiridis Feb. 19, 2024, 7:13 p.m. UTC 
  According to the datasheet of BMP38x and BMP390 devices, for an SPI
read operation the first byte that is returned needs to be dropped,
and the rest of the bytes are the actual data returned from the
sensor.

Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-spi.c | 49 ++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron Feb. 19, 2024, 7:42 p.m. UTC | #1 
On Mon, 19 Feb 2024 20:13:59 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> According to the datasheet of BMP38x and BMP390 devices, for an SPI
> read operation the first byte that is returned needs to be dropped,
> and the rest of the bytes are the actual data returned from the
> sensor.
> 
> Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")
No line break. The fixes tag is part of the main tag block.
I can fix that whilst applying.
Otherwise this looks fine to me.

I'll leave it on list a little longer though as both Linus and Angel
are still about and might have time to also take a look at this.

> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-spi.c | 49 ++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index e8a5fed07e88..1972014dca93 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -8,6 +8,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/err.h>
>  #include <linux/regmap.h>
> +#include <linux/bits.h>
>  
>  #include "bmp280.h"
>  
> @@ -35,6 +36,33 @@ static int bmp280_regmap_spi_read(void *context, const void *reg,
>  	return spi_write_then_read(spi, reg, reg_size, val, val_size);
>  }
>  
> +static int bmp380_regmap_spi_read(void *context, const void *reg,
> +				  size_t reg_size, void *val, size_t val_size)
> +{
> +	struct spi_device *spi = to_spi_device(context);
> +	u8 rx_buf[4];
> +	ssize_t status;
> +
> +	/*
> +	 * Maximum number of consecutive bytes read for a temperature or
> +	 * pressure measurement is 3.
> +	 */
> +	if (val_size > 3)
> +		return -EINVAL;
> +	/*
> +	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
> +	 * the first byte needs to be dropped and the rest are the requested
> +	 * data.
> +	 */
> +	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
> +	if (status)
> +		return status;
> +
> +	memcpy(val, rx_buf + 1, val_size);
> +
> +	return 0;
> +}
> +
>  static struct regmap_bus bmp280_regmap_bus = {
>  	.write = bmp280_regmap_spi_write,
>  	.read = bmp280_regmap_spi_read,
> @@ -42,10 +70,19 @@ static struct regmap_bus bmp280_regmap_bus = {
>  	.val_format_endian_default = REGMAP_ENDIAN_BIG,
>  };
>  
> +static struct regmap_bus bmp380_regmap_bus = {
> +	.write = bmp280_regmap_spi_write,
> +	.read = bmp380_regmap_spi_read,
> +	.read_flag_mask = BIT(7),
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
>  static int bmp280_spi_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  	const struct bmp280_chip_info *chip_info;
> +	struct regmap_bus *bmp_regmap_bus;
>  	struct regmap *regmap;
>  	int ret;
>  
> @@ -58,8 +95,18 @@ 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:
> +		bmp_regmap_bus = &bmp380_regmap_bus;
> +		break;
> +	default:
> +		bmp_regmap_bus = &bmp280_regmap_bus;
> +		break;
> +	}
> +
>  	regmap = devm_regmap_init(&spi->dev,
> -				  &bmp280_regmap_bus,
> +				  bmp_regmap_bus,
>  				  &spi->dev,
>  				  chip_info->regmap_config);
>  	if (IS_ERR(regmap)) {
Vasileios Amoiridis Feb. 19, 2024, 7:51 p.m. UTC | #2 
On Mon, Feb 19, 2024 at 07:42:36PM +0000, Jonathan Cameron wrote:
> On Mon, 19 Feb 2024 20:13:59 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > According to the datasheet of BMP38x and BMP390 devices, for an SPI
> > read operation the first byte that is returned needs to be dropped,
> > and the rest of the bytes are the actual data returned from the
> > sensor.
> > 
> > Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")
> No line break. The fixes tag is part of the main tag block.
> I can fix that whilst applying.
> Otherwise this looks fine to me.
> 
> I'll leave it on list a little longer though as both Linus and Angel
> are still about and might have time to also take a look at this.
>

Thank you very much again for the quick answer. Since you can fix it while applying,
I should not annoy the mailing list with more e-mails. Just for future reference,
when you mean no line break you mean between the main block and the "Fixes" line
or between the "Fixes" line and the "Signed-off-by: ...." ? 
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/pressure/bmp280-spi.c | 49 ++++++++++++++++++++++++++++++-
> >  1 file changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> > index e8a5fed07e88..1972014dca93 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/spi/spi.h>
> >  #include <linux/err.h>
> >  #include <linux/regmap.h>
> > +#include <linux/bits.h>
> >  
> >  #include "bmp280.h"
> >  
> > @@ -35,6 +36,33 @@ static int bmp280_regmap_spi_read(void *context, const void *reg,
> >  	return spi_write_then_read(spi, reg, reg_size, val, val_size);
> >  }
> >  
> > +static int bmp380_regmap_spi_read(void *context, const void *reg,
> > +				  size_t reg_size, void *val, size_t val_size)
> > +{
> > +	struct spi_device *spi = to_spi_device(context);
> > +	u8 rx_buf[4];
> > +	ssize_t status;
> > +
> > +	/*
> > +	 * Maximum number of consecutive bytes read for a temperature or
> > +	 * pressure measurement is 3.
> > +	 */
> > +	if (val_size > 3)
> > +		return -EINVAL;
> > +	/*
> > +	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
> > +	 * the first byte needs to be dropped and the rest are the requested
> > +	 * data.
> > +	 */
> > +	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
> > +	if (status)
> > +		return status;
> > +
> > +	memcpy(val, rx_buf + 1, val_size);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct regmap_bus bmp280_regmap_bus = {
> >  	.write = bmp280_regmap_spi_write,
> >  	.read = bmp280_regmap_spi_read,
> > @@ -42,10 +70,19 @@ static struct regmap_bus bmp280_regmap_bus = {
> >  	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> >  };
> >  
> > +static struct regmap_bus bmp380_regmap_bus = {
> > +	.write = bmp280_regmap_spi_write,
> > +	.read = bmp380_regmap_spi_read,
> > +	.read_flag_mask = BIT(7),
> > +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> > +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> > +};
> > +
> >  static int bmp280_spi_probe(struct spi_device *spi)
> >  {
> >  	const struct spi_device_id *id = spi_get_device_id(spi);
> >  	const struct bmp280_chip_info *chip_info;
> > +	struct regmap_bus *bmp_regmap_bus;
> >  	struct regmap *regmap;
> >  	int ret;
> >  
> > @@ -58,8 +95,18 @@ 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:
> > +		bmp_regmap_bus = &bmp380_regmap_bus;
> > +		break;
> > +	default:
> > +		bmp_regmap_bus = &bmp280_regmap_bus;
> > +		break;
> > +	}
> > +
> >  	regmap = devm_regmap_init(&spi->dev,
> > -				  &bmp280_regmap_bus,
> > +				  bmp_regmap_bus,
> >  				  &spi->dev,
> >  				  chip_info->regmap_config);
> >  	if (IS_ERR(regmap)) {
> 

Best regards,
Vasileios Amoiridis
Angel Iglesias Feb. 19, 2024, 11:30 p.m. UTC | #3 
On Mon, 2024-02-19 at 20:13 +0100, Vasileios Amoiridis wrote:
> According to the datasheet of BMP38x and BMP390 devices, for an SPI
> read operation the first byte that is returned needs to be dropped,
> and the rest of the bytes are the actual data returned from the
> sensor.
> 
Looks good to me :)

Acked-by: Angel Iglesias <ang.iglesiasg@gmail.com>

Kind regards,
Angel
Andy Shevchenko Feb. 20, 2024, 1:51 p.m. UTC | #4 
On Mon, Feb 19, 2024 at 08:51:45PM +0100, Vasileios Amoiridis wrote:
> On Mon, Feb 19, 2024 at 07:42:36PM +0000, Jonathan Cameron wrote:
> > On Mon, 19 Feb 2024 20:13:59 +0100
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > 
> > > According to the datasheet of BMP38x and BMP390 devices, for an SPI
> > > read operation the first byte that is returned needs to be dropped,
> > > and the rest of the bytes are the actual data returned from the
> > > sensor.
> > > 
> > > Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")
> > No line break. The fixes tag is part of the main tag block.
> > I can fix that whilst applying.
> > Otherwise this looks fine to me.
> > 
> > I'll leave it on list a little longer though as both Linus and Angel
> > are still about and might have time to also take a look at this.
> >
> 
> Thank you very much again for the quick answer. Since you can fix it while applying,
> I should not annoy the mailing list with more e-mails. Just for future reference,
> when you mean no line break you mean between the main block and the "Fixes" line
> or between the "Fixes" line and the "Signed-off-by: ...." ? 
> > > 
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

The standard pattern for the patch (as it is seen in the `git log`):

---8<---

iio: ...Subject line, summary of the change...
BLANK LINE
..Multi-line description, may contain blank lines...
BLANK LINE
..Tag block: SoB, Rb, etc. tags (may NOT contain blank lines)...

---8<---
Andy Shevchenko Feb. 20, 2024, 2:09 p.m. UTC | #5 
On Mon, Feb 19, 2024 at 08:13:59PM +0100, Vasileios Amoiridis wrote:
> According to the datasheet of BMP38x and BMP390 devices, for an SPI
> read operation the first byte that is returned needs to be dropped,
> and the rest of the bytes are the actual data returned from the
> sensor.

..

>  #include <linux/spi/spi.h>
>  #include <linux/err.h>
>  #include <linux/regmap.h>
> +#include <linux/bits.h>

I see that it's unsorted, but try to squeeze a new header to the better place
where more will be kept sorted. With given context, it should go before all
others, but it might be even better location.

..

> +static int bmp380_regmap_spi_read(void *context, const void *reg,
> +				  size_t reg_size, void *val, size_t val_size)
> +{
> +	struct spi_device *spi = to_spi_device(context);
> +	u8 rx_buf[4];
> +	ssize_t status;
> +
> +	/*
> +	 * Maximum number of consecutive bytes read for a temperature or
> +	 * pressure measurement is 3.
> +	 */
> +	if (val_size > 3)
> +		return -EINVAL;
> +	/*
> +	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
> +	 * the first byte needs to be dropped and the rest are the requested
> +	 * data.
> +	 */
> +	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
> +	if (status)
> +		return status;

> +	memcpy(val, rx_buf + 1, val_size);

Okay, endianess is defined in the regmap_bus below...

> +	return 0;
> +}
Vasileios Amoiridis Feb. 20, 2024, 5:46 p.m. UTC | #6 
On Tue, Feb 20, 2024 at 04:09:26PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 19, 2024 at 08:13:59PM +0100, Vasileios Amoiridis wrote:
> > According to the datasheet of BMP38x and BMP390 devices, for an SPI
> > read operation the first byte that is returned needs to be dropped,
> > and the rest of the bytes are the actual data returned from the
> > sensor.
> 
> ...
> 
> >  #include <linux/spi/spi.h>
> >  #include <linux/err.h>
> >  #include <linux/regmap.h>
> > +#include <linux/bits.h>
> 
> I see that it's unsorted, but try to squeeze a new header to the better place
> where more will be kept sorted. With given context, it should go before all
> others, but it might be even better location.
> 
> ...
> 

So you would suggest a re-ordering of the headers with a reverse
christmas-tree? I don't see it used in the other drivers of the IIO
subsystem but I can do it as well

> > +static int bmp380_regmap_spi_read(void *context, const void *reg,
> > +				  size_t reg_size, void *val, size_t val_size)
> > +{
> > +	struct spi_device *spi = to_spi_device(context);
> > +	u8 rx_buf[4];
> > +	ssize_t status;
> > +
> > +	/*
> > +	 * Maximum number of consecutive bytes read for a temperature or
> > +	 * pressure measurement is 3.
> > +	 */
> > +	if (val_size > 3)
> > +		return -EINVAL;
> > +	/*
> > +	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
> > +	 * the first byte needs to be dropped and the rest are the requested
> > +	 * data.
> > +	 */
> > +	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
> > +	if (status)
> > +		return status;
> 
> > +	memcpy(val, rx_buf + 1, val_size);
> 
> Okay, endianess is defined in the regmap_bus below...
> 
> > +	return 0;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Jonathan Cameron Feb. 24, 2024, 4:30 p.m. UTC | #7 
On Mon, 19 Feb 2024 20:13:59 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> According to the datasheet of BMP38x and BMP390 devices, for an SPI
> read operation the first byte that is returned needs to be dropped,
> and the rest of the bytes are the actual data returned from the
> sensor.
> 
> Fixes: 8d329309184d ("iio: pressure: bmp280: Add support for BMP380 sensor family")
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>

Applied and marked for stable - with tweaks to header order and the space above
as per discussion.
> ---
>  drivers/iio/pressure/bmp280-spi.c | 49 ++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index e8a5fed07e88..1972014dca93 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -8,6 +8,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/err.h>
>  #include <linux/regmap.h>
> +#include <linux/bits.h>
>  
>  #include "bmp280.h"
>  
> @@ -35,6 +36,33 @@ static int bmp280_regmap_spi_read(void *context, const void *reg,
>  	return spi_write_then_read(spi, reg, reg_size, val, val_size);
>  }
>  
> +static int bmp380_regmap_spi_read(void *context, const void *reg,
> +				  size_t reg_size, void *val, size_t val_size)
> +{
> +	struct spi_device *spi = to_spi_device(context);
> +	u8 rx_buf[4];
> +	ssize_t status;
> +
> +	/*
> +	 * Maximum number of consecutive bytes read for a temperature or
> +	 * pressure measurement is 3.
> +	 */
> +	if (val_size > 3)
> +		return -EINVAL;
> +	/*
> +	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
> +	 * the first byte needs to be dropped and the rest are the requested
> +	 * data.
> +	 */
> +	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
> +	if (status)
> +		return status;
> +
> +	memcpy(val, rx_buf + 1, val_size);
> +
> +	return 0;
> +}
> +
>  static struct regmap_bus bmp280_regmap_bus = {
>  	.write = bmp280_regmap_spi_write,
>  	.read = bmp280_regmap_spi_read,
> @@ -42,10 +70,19 @@ static struct regmap_bus bmp280_regmap_bus = {
>  	.val_format_endian_default = REGMAP_ENDIAN_BIG,
>  };
>  
> +static struct regmap_bus bmp380_regmap_bus = {
> +	.write = bmp280_regmap_spi_write,
> +	.read = bmp380_regmap_spi_read,
> +	.read_flag_mask = BIT(7),
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
>  static int bmp280_spi_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  	const struct bmp280_chip_info *chip_info;
> +	struct regmap_bus *bmp_regmap_bus;
>  	struct regmap *regmap;
>  	int ret;
>  
> @@ -58,8 +95,18 @@ 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:
> +		bmp_regmap_bus = &bmp380_regmap_bus;
> +		break;
> +	default:
> +		bmp_regmap_bus = &bmp280_regmap_bus;
> +		break;
> +	}
> +
>  	regmap = devm_regmap_init(&spi->dev,
> -				  &bmp280_regmap_bus,
> +				  bmp_regmap_bus,
>  				  &spi->dev,
>  				  chip_info->regmap_config);
>  	if (IS_ERR(regmap)) {
diff mbox series

Patch

diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index e8a5fed07e88..1972014dca93 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -8,6 +8,7 @@ 
 #include <linux/spi/spi.h>
 #include <linux/err.h>
 #include <linux/regmap.h>
+#include <linux/bits.h>
 
 #include "bmp280.h"
 
@@ -35,6 +36,33 @@  static int bmp280_regmap_spi_read(void *context, const void *reg,
 	return spi_write_then_read(spi, reg, reg_size, val, val_size);
 }
 
+static int bmp380_regmap_spi_read(void *context, const void *reg,
+				  size_t reg_size, void *val, size_t val_size)
+{
+	struct spi_device *spi = to_spi_device(context);
+	u8 rx_buf[4];
+	ssize_t status;
+
+	/*
+	 * Maximum number of consecutive bytes read for a temperature or
+	 * pressure measurement is 3.
+	 */
+	if (val_size > 3)
+		return -EINVAL;
+	/*
+	 * According to the BMP3xx datasheets, for a basic SPI read opertion,
+	 * the first byte needs to be dropped and the rest are the requested
+	 * data.
+	 */
+	status = spi_write_then_read(spi, reg, 1, rx_buf, val_size + 1);
+	if (status)
+		return status;
+
+	memcpy(val, rx_buf + 1, val_size);
+
+	return 0;
+}
+
 static struct regmap_bus bmp280_regmap_bus = {
 	.write = bmp280_regmap_spi_write,
 	.read = bmp280_regmap_spi_read,
@@ -42,10 +70,19 @@  static struct regmap_bus bmp280_regmap_bus = {
 	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
+static struct regmap_bus bmp380_regmap_bus = {
+	.write = bmp280_regmap_spi_write,
+	.read = bmp380_regmap_spi_read,
+	.read_flag_mask = BIT(7),
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
 static int bmp280_spi_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
 	const struct bmp280_chip_info *chip_info;
+	struct regmap_bus *bmp_regmap_bus;
 	struct regmap *regmap;
 	int ret;
 
@@ -58,8 +95,18 @@  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:
+		bmp_regmap_bus = &bmp380_regmap_bus;
+		break;
+	default:
+		bmp_regmap_bus = &bmp280_regmap_bus;
+		break;
+	}
+
 	regmap = devm_regmap_init(&spi->dev,
-				  &bmp280_regmap_bus,
+				  bmp_regmap_bus,
 				  &spi->dev,
 				  chip_info->regmap_config);
 	if (IS_ERR(regmap)) {