diff mbox series

drivers: iio: pressure: Add SPI support for BMP38x and BMP390

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

Commit Message

Vasileios Amoiridis Feb. 15, 2024, 4:43 p.m. UTC 
  According to the datasheet of BMP38x and BMP390 devices, in SPI
operation, the first byte that returns after a read operation is
garbage and it needs to be dropped and return the rest of the
bytes.

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

Comments

Andy Shevchenko Feb. 15, 2024, 5:03 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 05:43:32PM +0100, Vasileios Amoiridis wrote:
> According to the datasheet of BMP38x and BMP390 devices, in SPI
> operation, the first byte that returns after a read operation is
> garbage and it needs to be dropped and return the rest of the
> bytes.

Thank you for the patch, my comments below.

..

> +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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
> +	ssize_t status;
> +	u8 buf;

AFAIU this buffer is not DMA-capable.

> +	memcpy(&buf, reg, reg_size);

I prefer to see a switch case with cases based on allowed sizes and proper
endianess accessors.

> +	buf |= 0x80;

This is done by regmap, no?

> +	/*
> +	 * According to the BMP380, BMP388, BMP390 datasheets, for a basic
> +	 * read operation, after the write is done, 2 bytes are received and
> +	 * the first one has to be dropped. The 2nd one is the requested
> +	 * value.
> +	 */
> +	status = spi_write_then_read(spi, &buf, 1, ret, val_size + 1);

sizeof() ?

> +	if (status)
> +		return status;

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

As per above.

> +	return 0;
> +}
Angel Iglesias Feb. 15, 2024, 5:20 p.m. UTC | #2 
On Thu, 2024-02-15 at 17:43 +0100, Vasileios Amoiridis wrote:
> According to the datasheet of BMP38x and BMP390 devices, in SPI
> operation, the first byte that returns after a read operation is
> garbage and it needs to be dropped and return the rest of the
> bytes.

Hey good catch! It flew past me when I added this devices because I tested only
on i2c at the time.

Kind regards,
Angel

> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-spi.c | 47 ++++++++++++++++++++++++++++++-
>  drivers/iio/pressure/bmp280.h     |  2 ++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-
> spi.c
> index 433d6fac83c4..c4b4a5d67f94 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -35,6 +35,32 @@ 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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
> +	ssize_t status;
> +	u8 buf;
> +
> +	memcpy(&buf, reg, reg_size);
> +	buf |= 0x80;
> +
> +	/*
> +	 * According to the BMP380, BMP388, BMP390 datasheets, for a basic
> +	 * read operation, after the write is done, 2 bytes are received and
> +	 * the first one has to be dropped. The 2nd one is the requested
> +	 * value.
> +	 */
> +	status = spi_write_then_read(spi, &buf, 1, ret, val_size + 1);
> +	if (status)
> +		return status;
> +
> +	memcpy(val, ret + 1, val_size);
> +
> +	return 0;
> +}
> +
>  static struct regmap_bus bmp280_regmap_bus = {
>  	.write = bmp280_regmap_spi_write,
>  	.read = bmp280_regmap_spi_read,
> @@ -42,10 +68,18 @@ 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,
> +	.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 +92,19 @@ 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 --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index 4012387d7956..ca482b7e4295 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -191,6 +191,8 @@
>  #define BMP380_TEMP_SKIPPED		0x800000
>  #define BMP380_PRESS_SKIPPED		0x800000
>  
> +#define BMP380_SPI_MAX_REG_COUNT_READ   3
> +
>  /* BMP280 specific registers */
>  #define BMP280_REG_HUMIDITY_LSB		0xFE
>  #define BMP280_REG_HUMIDITY_MSB		0xFD
Jonathan Cameron Feb. 16, 2024, 11:05 a.m. UTC | #3 
On Thu, 15 Feb 2024 19:03:27 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Feb 15, 2024 at 05:43:32PM +0100, Vasileios Amoiridis wrote:
> > According to the datasheet of BMP38x and BMP390 devices, in SPI
> > operation, the first byte that returns after a read operation is
> > garbage and it needs to be dropped and return the rest of the
> > bytes.  
> 
> Thank you for the patch, my comments below.
> 
> ...
> 
> > +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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
> > +	ssize_t status;
> > +	u8 buf;  
> 
> AFAIU this buffer is not DMA-capable.

Doesn't matter in this case as spi_write_then_read() bounces anyway so you don't need
to provide it with a dma safe buffer. It's in the docs, so we can rely
on this not changing.

https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L4391
Jonathan Cameron Feb. 16, 2024, 11:18 a.m. UTC | #4 
On Thu, 15 Feb 2024 17:43:32 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> According to the datasheet of BMP38x and BMP390 devices, in SPI
> operation, the first byte that returns after a read operation is
> garbage and it needs to be dropped and return the rest of the
> bytes.

Make it clear in the patch title that this is a fix and add a fixes tag.

> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/pressure/bmp280-spi.c | 47 ++++++++++++++++++++++++++++++-
>  drivers/iio/pressure/bmp280.h     |  2 ++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> index 433d6fac83c4..c4b4a5d67f94 100644
> --- a/drivers/iio/pressure/bmp280-spi.c
> +++ b/drivers/iio/pressure/bmp280-spi.c
> @@ -35,6 +35,32 @@ 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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];

Given you rely on val_size < 3 you should check for that explcitly rather than
potentially overflowing the buffer.
ret is not a good naming choice for this variable as it's commonly used for
integer return values.  Call it read_buf or something like that.

> +	ssize_t status;
> +	u8 buf;
> +
> +	memcpy(&buf, reg, reg_size);
> +	buf |= 0x80;

Can you use regmap_bus read_flag_mask for this?  Seems to apply to 
all devices supported. + that's common for spi regmaps


Mind you I note the bmp280_regmap_spi_write() is masking the bit out which seems
backwards  - all the registers are defined with the bit set for that part
but not the 380.  Ah well - not part of this fix even if it's odd.


> +
> +	/*
> +	 * According to the BMP380, BMP388, BMP390 datasheets, for a basic
> +	 * read operation, after the write is done, 2 bytes are received and
> +	 * the first one has to be dropped. The 2nd one is the requested
> +	 * value.
> +	 */
> +	status = spi_write_then_read(spi, &buf, 1, ret, val_size + 1);
> +	if (status)
> +		return status;
> +
> +	memcpy(val, ret + 1, val_size);
> +
> +	return 0;
> +}
> +
>  static struct regmap_bus bmp280_regmap_bus = {
>  	.write = bmp280_regmap_spi_write,
>  	.read = bmp280_regmap_spi_read,
> @@ -42,10 +68,18 @@ 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,
> +	.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 +92,19 @@ 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 --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> index 4012387d7956..ca482b7e4295 100644
> --- a/drivers/iio/pressure/bmp280.h
> +++ b/drivers/iio/pressure/bmp280.h
> @@ -191,6 +191,8 @@
>  #define BMP380_TEMP_SKIPPED		0x800000
>  #define BMP380_PRESS_SKIPPED		0x800000
>  
> +#define BMP380_SPI_MAX_REG_COUNT_READ   3
This doesn't seem useful as only used in one place.
> +
>  /* BMP280 specific registers */
>  #define BMP280_REG_HUMIDITY_LSB		0xFE
>  #define BMP280_REG_HUMIDITY_MSB		0xFD
Vasileios Amoiridis Feb. 16, 2024, 1:26 p.m. UTC | #5 
On Fri, Feb 16, 2024 at 11:18:34AM +0000, Jonathan Cameron wrote:
> On Thu, 15 Feb 2024 17:43:32 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > According to the datasheet of BMP38x and BMP390 devices, in SPI
> > operation, the first byte that returns after a read operation is
> > garbage and it needs to be dropped and return the rest of the
> > bytes.
> 
> Make it clear in the patch title that this is a fix and add a fixes tag.
> 

The original support for SPI was added 8 years ago. Should I include that commit
of 8 years ago in the fixes tag or just use a the word "fixes" with the rest of the
title?

> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/pressure/bmp280-spi.c | 47 ++++++++++++++++++++++++++++++-
> >  drivers/iio/pressure/bmp280.h     |  2 ++
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> > index 433d6fac83c4..c4b4a5d67f94 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -35,6 +35,32 @@ 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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
> 
> Given you rely on val_size < 3 you should check for that explcitly rather than
> potentially overflowing the buffer.
> ret is not a good naming choice for this variable as it's commonly used for
> integer return values.  Call it read_buf or something like that.
> 

Thanks for pointing this out it makes a lot of sense.

> > +	ssize_t status;
> > +	u8 buf;
> > +
> > +	memcpy(&buf, reg, reg_size);
> > +	buf |= 0x80;
> 
> Can you use regmap_bus read_flag_mask for this?  Seems to apply to 
> all devices supported. + that's common for spi regmaps
>

Yes I noticed it yesterday in my tests that this was missing and it actually
applies to all the devices. So the read_flag_mask should be added to both
regmap_bus structs. 
> 
> Mind you I note the bmp280_regmap_spi_write() is masking the bit out which seems
> backwards  - all the registers are defined with the bit set for that part
> but not the 380.  Ah well - not part of this fix even if it's odd.
> 
> 
> > +
> > +	/*
> > +	 * According to the BMP380, BMP388, BMP390 datasheets, for a basic
> > +	 * read operation, after the write is done, 2 bytes are received and
> > +	 * the first one has to be dropped. The 2nd one is the requested
> > +	 * value.
> > +	 */
> > +	status = spi_write_then_read(spi, &buf, 1, ret, val_size + 1);
> > +	if (status)
> > +		return status;
> > +
> > +	memcpy(val, ret + 1, val_size);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct regmap_bus bmp280_regmap_bus = {
> >  	.write = bmp280_regmap_spi_write,
> >  	.read = bmp280_regmap_spi_read,
> > @@ -42,10 +68,18 @@ 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,
> > +	.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 +92,19 @@ 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 --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index 4012387d7956..ca482b7e4295 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -191,6 +191,8 @@
> >  #define BMP380_TEMP_SKIPPED		0x800000
> >  #define BMP380_PRESS_SKIPPED		0x800000
> >  
> > +#define BMP380_SPI_MAX_REG_COUNT_READ   3
> This doesn't seem useful as only used in one place.

Could this define be moved in the bmp280-spi.c file or to not even use a define?

> > +
> >  /* BMP280 specific registers */
> >  #define BMP280_REG_HUMIDITY_LSB		0xFE
> >  #define BMP280_REG_HUMIDITY_MSB		0xFD
>
Vasileios Amoiridis Feb. 16, 2024, 1:29 p.m. UTC | #6 
On Fri, Feb 16, 2024 at 11:05:43AM +0000, Jonathan Cameron wrote:
> On Thu, 15 Feb 2024 19:03:27 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Thu, Feb 15, 2024 at 05:43:32PM +0100, Vasileios Amoiridis wrote:
> > > According to the datasheet of BMP38x and BMP390 devices, in SPI
> > > operation, the first byte that returns after a read operation is
> > > garbage and it needs to be dropped and return the rest of the
> > > bytes.  
> > 
> > Thank you for the patch, my comments below.
> > 
> > ...
> > 
> > > +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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
> > > +	ssize_t status;
> > > +	u8 buf;  
> > 
> > AFAIU this buffer is not DMA-capable.
> 
> Doesn't matter in this case as spi_write_then_read() bounces anyway so you don't need
> to provide it with a dma safe buffer. It's in the docs, so we can rely
> on this not changing.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi.c#L4391
> 

Since the read_flag_mask can be used in the struct regmap_bus, there is no need for an 
extra u8 buffer to manipulate the bits. Instead, the reg value from the function inputs 
can be used as in any other case.
Jonathan Cameron Feb. 16, 2024, 3:47 p.m. UTC | #7 
On Fri, 16 Feb 2024 14:26:44 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Fri, Feb 16, 2024 at 11:18:34AM +0000, Jonathan Cameron wrote:
> > On Thu, 15 Feb 2024 17:43:32 +0100
> > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> >   
> > > According to the datasheet of BMP38x and BMP390 devices, in SPI
> > > operation, the first byte that returns after a read operation is
> > > garbage and it needs to be dropped and return the rest of the
> > > bytes.  
> > 
> > Make it clear in the patch title that this is a fix and add a fixes tag.
> >   
> 
> The original support for SPI was added 8 years ago. Should I include that commit
> of 8 years ago in the fixes tag or just use a the word "fixes" with the rest of the
> title?
> 
Original git commit for the fixes tag.  Lets us know this wants to go in all stable kernels.
Also fixes in the title.


> > > +	ssize_t status;
> > > +	u8 buf;
> > > +
> > > +	memcpy(&buf, reg, reg_size);
> > > +	buf |= 0x80;  
> > 
> > Can you use regmap_bus read_flag_mask for this?  Seems to apply to 
> > all devices supported. + that's common for spi regmaps
> >  
> 
> Yes I noticed it yesterday in my tests that this was missing and it actually
> applies to all the devices. So the read_flag_mask should be added to both
> regmap_bus structs. 

It's there sort of indirectly for the bmp280 - the register addresses all happen
to include that bit, then it is cleared explicitly for the other direction.



> > 
> > Mind you I note the bmp280_regmap_spi_write() is masking the bit out which seems
> > backwards  - all the registers are defined with the bit set for that part
> > but not the 380.  Ah well - not part of this fix even if it's odd.
> > 

> > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > > index 4012387d7956..ca482b7e4295 100644
> > > --- a/drivers/iio/pressure/bmp280.h
> > > +++ b/drivers/iio/pressure/bmp280.h
> > > @@ -191,6 +191,8 @@
> > >  #define BMP380_TEMP_SKIPPED		0x800000
> > >  #define BMP380_PRESS_SKIPPED		0x800000
> > >  
> > > +#define BMP380_SPI_MAX_REG_COUNT_READ   3  
> > This doesn't seem useful as only used in one place.  
> 
> Could this define be moved in the bmp280-spi.c file or to not even use a define?
Not use it. Don't see how it is helpful. Just check that the
thing will fit in the array using an ARRAY_SIZE()...
> 
> > > +
> > >  /* BMP280 specific registers */
> > >  #define BMP280_REG_HUMIDITY_LSB		0xFE
> > >  #define BMP280_REG_HUMIDITY_MSB		0xFD  
> >
Vasileios Amoiridis Feb. 16, 2024, 4:40 p.m. UTC | #8 
On Fri, Feb 16, 2024 at 03:47:42PM +0000, Jonathan Cameron wrote:
> On Fri, 16 Feb 2024 14:26:44 +0100
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > On Fri, Feb 16, 2024 at 11:18:34AM +0000, Jonathan Cameron wrote:
> > > On Thu, 15 Feb 2024 17:43:32 +0100
> > > Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> > >   
> > > > According to the datasheet of BMP38x and BMP390 devices, in SPI
> > > > operation, the first byte that returns after a read operation is
> > > > garbage and it needs to be dropped and return the rest of the
> > > > bytes.  
> > > 
> > > Make it clear in the patch title that this is a fix and add a fixes tag.
> > >   
> > 
> > The original support for SPI was added 8 years ago. Should I include that commit
> > of 8 years ago in the fixes tag or just use a the word "fixes" with the rest of the
> > title?
> > 
> Original git commit for the fixes tag.  Lets us know this wants to go in all stable kernels.
> Also fixes in the title.
 
Ok, will do that!
>
> 
> > > > +	ssize_t status;
> > > > +	u8 buf;
> > > > +
> > > > +	memcpy(&buf, reg, reg_size);
> > > > +	buf |= 0x80;  
> > > 
> > > Can you use regmap_bus read_flag_mask for this?  Seems to apply to 
> > > all devices supported. + that's common for spi regmaps
> > >  
> > 
> > Yes I noticed it yesterday in my tests that this was missing and it actually
> > applies to all the devices. So the read_flag_mask should be added to both
> > regmap_bus structs. 
> 
> It's there sort of indirectly for the bmp280 - the register addresses all happen
> to include that bit, then it is cleared explicitly for the other direction.

Oh okay, now I understand what you mean. Ok then I can also send a different patch
for this as well just to keep the code consistent.
> 
> 
> 
> > > 
> > > Mind you I note the bmp280_regmap_spi_write() is masking the bit out which seems
> > > backwards  - all the registers are defined with the bit set for that part
> > > but not the 380.  Ah well - not part of this fix even if it's odd.
> > > 
> 
> > > > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > > > index 4012387d7956..ca482b7e4295 100644
> > > > --- a/drivers/iio/pressure/bmp280.h
> > > > +++ b/drivers/iio/pressure/bmp280.h
> > > > @@ -191,6 +191,8 @@
> > > >  #define BMP380_TEMP_SKIPPED		0x800000
> > > >  #define BMP380_PRESS_SKIPPED		0x800000
> > > >  
> > > > +#define BMP380_SPI_MAX_REG_COUNT_READ   3  
> > > This doesn't seem useful as only used in one place.  
> > 
> > Could this define be moved in the bmp280-spi.c file or to not even use a define?
> Not use it. Don't see how it is helpful. Just check that the
> thing will fit in the array using an ARRAY_SIZE()...

Understood.
> > 
> > > > +
> > > >  /* BMP280 specific registers */
> > > >  #define BMP280_REG_HUMIDITY_LSB		0xFE
> > > >  #define BMP280_REG_HUMIDITY_MSB		0xFD  
> > >   
> 

Thank you very much for the feedback, I'll work on the patches and submit them again.

Best regards,
Vasileios Amoiridis
diff mbox series

Patch

diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
index 433d6fac83c4..c4b4a5d67f94 100644
--- a/drivers/iio/pressure/bmp280-spi.c
+++ b/drivers/iio/pressure/bmp280-spi.c
@@ -35,6 +35,32 @@  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 ret[BMP380_SPI_MAX_REG_COUNT_READ + 1];
+	ssize_t status;
+	u8 buf;
+
+	memcpy(&buf, reg, reg_size);
+	buf |= 0x80;
+
+	/*
+	 * According to the BMP380, BMP388, BMP390 datasheets, for a basic
+	 * read operation, after the write is done, 2 bytes are received and
+	 * the first one has to be dropped. The 2nd one is the requested
+	 * value.
+	 */
+	status = spi_write_then_read(spi, &buf, 1, ret, val_size + 1);
+	if (status)
+		return status;
+
+	memcpy(val, ret + 1, val_size);
+
+	return 0;
+}
+
 static struct regmap_bus bmp280_regmap_bus = {
 	.write = bmp280_regmap_spi_write,
 	.read = bmp280_regmap_spi_read,
@@ -42,10 +68,18 @@  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,
+	.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 +92,19 @@  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 --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 4012387d7956..ca482b7e4295 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -191,6 +191,8 @@ 
 #define BMP380_TEMP_SKIPPED		0x800000
 #define BMP380_PRESS_SKIPPED		0x800000
 
+#define BMP380_SPI_MAX_REG_COUNT_READ   3
+
 /* BMP280 specific registers */
 #define BMP280_REG_HUMIDITY_LSB		0xFE
 #define BMP280_REG_HUMIDITY_MSB		0xFD