Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640225 > unrolled thread

Re: [PATCH] iio: Add driver for Infineon DPS310

Started byJoel Stanley <joel@jms.id.au>
First post2017-05-12 08:30 +0200
Last post2017-05-14 17:00 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] iio: Add driver for Infineon DPS310 Joel Stanley <joel@jms.id.au> - 2017-05-12 08:30 +0200
    Re: [PATCH] iio: Add driver for Infineon DPS310 Jonathan Cameron <jic23@kernel.org> - 2017-05-14 17:00 +0200

#1640225 — Re: [PATCH] iio: Add driver for Infineon DPS310

FromJoel Stanley <joel@jms.id.au>
Date2017-05-12 08:30 +0200
SubjectRe: [PATCH] iio: Add driver for Infineon DPS310
Message-ID<tGcfo-nE-9@gated-at.bofh.it>
On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>
> comments below
>
>> This driver supports polled measurement of temperature over i2c only.
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>  drivers/iio/pressure/Kconfig  |  11 ++
>>  drivers/iio/pressure/Makefile |   1 +
>>  drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 413 insertions(+)
>>  create mode 100644 drivers/iio/pressure/dps310.c
>>
>> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
>> index bd8d96b96771..50caefc0cd7d 100644
>> --- a/drivers/iio/pressure/Kconfig
>> +++ b/drivers/iio/pressure/Kconfig
>> @@ -42,6 +42,17 @@ config BMP280_SPI
>>       depends on SPI_MASTER
>>       select REGMAP
>>
>> +config DPS310
>> +       tristate "Infineon DPS310 pressure and temperature sensor"
>> +       depends on I2C
>> +       select REGMAP_I2C
>> +       help
>> +      Support for the Infineon DPS310 digital barometric pressure sensor.
>> +      This driver measures temperature only.
>
> does it make sense to propose a driver for it then?

Yes. We are building a system that uses the sensor to measure temperature.

If you would prefer I can put it under drivers/iio/temp?

> is there intention to add pressure soonish?
>
> link to datasheet would be nice

Good idea.

>> +++ b/drivers/iio/pressure/dps310.c
>> @@ -0,0 +1,401 @@
>> +/*
>> + * Copyright 2017 IBM Corporation
>> + *
>> + * Joel Stanley <joel@jms.id.au>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * The DPS310 is a barometric pressure and temperature sensor.
>> + * Currently only reading a single temperature is supported by
>> + * this driver.
>> + *
>
> maybe add a comment with I2C address

The addresses are specified at the bottom of the file. Is it
convention to add them to a comment too?

>
>> + * TODO:
>> + *  - Pressure sensor readings
>> + *  - Optionally support the FIFO
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define PRS_BASE     0x00
>
> prefix everything with DPS310_ or dps310_

Ack.

>
>> +#define TMP_BASE     0x03
>> +#define PRS_CFG              0x06
>> +#define TMP_CFG              0x07
>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>> +#define  TMP_EXT     BIT(7)
>> +#define MEAS_CFG     0x08
>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>> +#define   PRESSURE_EN        BIT(0)
>> +#define   TEMP_EN    BIT(1)
>> +#define   BACKGROUND BIT(2)
>> +#define  PRS_RDY     BIT(4)
>> +#define  TMP_RDY     BIT(5)
>> +#define  SENSOR_RDY  BIT(6)
>> +#define  COEF_RDY    BIT(7)
>> +#define CFG_REG              0x09
>> +#define  INT_HL              BIT(7)
>> +#define  TMP_SHIFT_EN        BIT(3)
>> +#define  PRS_SHIFT_EN        BIT(4)
>> +#define  FIFO_EN     BIT(5)
>> +#define  SPI_EN              BIT(6)
>> +#define RESET                0x0c
>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>> +#define COEF_BASE    0x10
>> +
>> +#define TMP_RATE(_n) ilog2(_n)
>> +#define TMP_PRC(_n)  ilog2(_n)
>> +
>> +#define MCELSIUS_PER_CELSIUS 1000
>
> indeed :)
> questionable usefulness

Other drivers do the same. Perhaps it could go in the iio core?

As a driver author it tells me that the drivers is doing this
conversion, and not that the hardware requires a scaling of 1000.

>> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
>
> use sign_extend32() instead?

Thanks for pointing this out! I was sure it must exist but I couldn't find it.


>> +
>> +     data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
>> +     if (IS_ERR(data->regmap))
>> +             return PTR_ERR(data->regmap);
>> +
>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>> +     if (r < 0)
>> +             return r;
>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /* Turn on temperature measurement in the background */
>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>> +                     TEMP_EN | BACKGROUND);
>
> should be disabled again if _probe fails lateron

Ok.

>
>> +     if (r < 0)
>> +             return r;
>> +
>> +     /*
>> +      * Calibration coefficients required for reporting temperature.
>> +      * They are availalbe 40ms after the device has started
>
> available

Thanks.

>
>> +      */
>> +     r = dps310_get_temp_coef(data);
>> +     if (r == -EAGAIN)
>> +             return -EPROBE_DEFER;
>
> wouldn't it make more sense to wait for the data to become ready, maybe
> with a timeout?

Ok.

>
>> +     if (r < 0)
>> +             return r;
>> +
>> +     r = devm_iio_device_register(&client->dev, iio);
>> +     if (r)
>> +             return r;
>> +
>> +     i2c_set_clientdata(client, iio);
>> +
>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>> +                     client->name);
>
> no extra logging, this adds little extra information

I disagree - lets discuss it in my reply to Jonathan.

>
>> +     return 0;
>> +}
>
> turn off measurement in _remove()?

Ok.

Thanks for the review.

Cheers,

Joel

[toc] | [next] | [standalone]

#1641104

FromJonathan Cameron <jic23@kernel.org>
Date2017-05-14 17:00 +0200
Message-ID<tH3a1-2Q4-1@gated-at.bofh.it>
In reply to#1640225
On 12/05/17 07:28, Joel Stanley wrote:
> On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
> <pmeerw@pmeerw.net> wrote:
>>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>>> i2c and SPI.
>>
>> comments below
>>
>>> This driver supports polled measurement of temperature over i2c only.
>>>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>> ---
>>>   drivers/iio/pressure/Kconfig  |  11 ++
>>>   drivers/iio/pressure/Makefile |   1 +
>>>   drivers/iio/pressure/dps310.c | 401 ++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 413 insertions(+)
>>>   create mode 100644 drivers/iio/pressure/dps310.c
>>>
>>> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
>>> index bd8d96b96771..50caefc0cd7d 100644
>>> --- a/drivers/iio/pressure/Kconfig
>>> +++ b/drivers/iio/pressure/Kconfig
>>> @@ -42,6 +42,17 @@ config BMP280_SPI
>>>        depends on SPI_MASTER
>>>        select REGMAP
>>>
>>> +config DPS310
>>> +       tristate "Infineon DPS310 pressure and temperature sensor"
>>> +       depends on I2C
>>> +       select REGMAP_I2C
>>> +       help
>>> +      Support for the Infineon DPS310 digital barometric pressure sensor.
>>> +      This driver measures temperature only.
>>
>> does it make sense to propose a driver for it then?
> 
> Yes. We are building a system that uses the sensor to measure temperature.
> 
> If you would prefer I can put it under drivers/iio/temp?
No.  You guys are probably the oddity here (it's much easier to make
a temperature chip than a pressure one so primary purpose is normally
going to be pressure.)

No chance at all you could put really basic pressure reading in place
as well?
> 
>> is there intention to add pressure soonish?
>>
>> link to datasheet would be nice
> 
> Good idea.
> 
>>> +++ b/drivers/iio/pressure/dps310.c
>>> @@ -0,0 +1,401 @@
>>> +/*
>>> + * Copyright 2017 IBM Corporation
>>> + *
>>> + * Joel Stanley <joel@jms.id.au>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * as published by the Free Software Foundation; either version
>>> + * 2 of the License, or (at your option) any later version.
>>> + *
>>> + * The DPS310 is a barometric pressure and temperature sensor.
>>> + * Currently only reading a single temperature is supported by
>>> + * this driver.
>>> + *
>>
>> maybe add a comment with I2C address
> 
> The addresses are specified at the bottom of the file. Is it
> convention to add them to a comment too?
> 
>>
>>> + * TODO:
>>> + *  - Pressure sensor readings
>>> + *  - Optionally support the FIFO
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +#define PRS_BASE     0x00
>>
>> prefix everything with DPS310_ or dps310_
> 
> Ack.
> 
>>
>>> +#define TMP_BASE     0x03
>>> +#define PRS_CFG              0x06
>>> +#define TMP_CFG              0x07
>>> +#define  TMP_RATE_BITS       GENMASK(6, 4)
>>> +#define  TMP_PRC_BITS        GENMASK(3, 0)
>>> +#define  TMP_EXT     BIT(7)
>>> +#define MEAS_CFG     0x08
>>> +#define  MEAS_CTRL_BITS      GENMASK(2, 0)
>>> +#define   PRESSURE_EN        BIT(0)
>>> +#define   TEMP_EN    BIT(1)
>>> +#define   BACKGROUND BIT(2)
>>> +#define  PRS_RDY     BIT(4)
>>> +#define  TMP_RDY     BIT(5)
>>> +#define  SENSOR_RDY  BIT(6)
>>> +#define  COEF_RDY    BIT(7)
>>> +#define CFG_REG              0x09
>>> +#define  INT_HL              BIT(7)
>>> +#define  TMP_SHIFT_EN        BIT(3)
>>> +#define  PRS_SHIFT_EN        BIT(4)
>>> +#define  FIFO_EN     BIT(5)
>>> +#define  SPI_EN              BIT(6)
>>> +#define RESET                0x0c
>>> +#define  RESET_MAGIC (BIT(0) | BIT(3))
>>> +#define COEF_BASE    0x10
>>> +
>>> +#define TMP_RATE(_n) ilog2(_n)
>>> +#define TMP_PRC(_n)  ilog2(_n)
>>> +
>>> +#define MCELSIUS_PER_CELSIUS 1000
>>
>> indeed :)
>> questionable usefulness
> 
> Other drivers do the same. Perhaps it could go in the iio core?
I think Peter was advocating just using 1000 where needed rather
than having a define.
> 
> As a driver author it tells me that the drivers is doing this
> conversion, and not that the hardware requires a scaling of 1000.
> 
>>> +static s32 dps310_twos_compliment(u32 raw, size_t num_bits)
>>
>> use sign_extend32() instead?
> 
> Thanks for pointing this out! I was sure it must exist but I couldn't find it.
It's surprisingly new ;)
> 
> 
>>> +
>>> +     data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
>>> +     if (IS_ERR(data->regmap))
>>> +             return PTR_ERR(data->regmap);
>>> +
>>> +     r = regmap_write(data->regmap, TMP_CFG, TMP_EXT | TMP_PRC(1));
>>> +     if (r < 0)
>>> +             return r;
>>> +     r = regmap_write_bits(data->regmap, CFG_REG, TMP_SHIFT_EN, 0);
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /* Turn on temperature measurement in the background */
>>> +     r = regmap_write_bits(data->regmap, MEAS_CFG, MEAS_CTRL_BITS,
>>> +                     TEMP_EN | BACKGROUND);
>>
>> should be disabled again if _probe fails lateron
> 
> Ok.
> 
>>
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /*
>>> +      * Calibration coefficients required for reporting temperature.
>>> +      * They are availalbe 40ms after the device has started
>>
>> available
> 
> Thanks.
> 
>>
>>> +      */
>>> +     r = dps310_get_temp_coef(data);
>>> +     if (r == -EAGAIN)
>>> +             return -EPROBE_DEFER;
>>
>> wouldn't it make more sense to wait for the data to become ready, maybe
>> with a timeout?
> 
> Ok.
> 
>>
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     r = devm_iio_device_register(&client->dev, iio);
>>> +     if (r)
>>> +             return r;
>>> +
>>> +     i2c_set_clientdata(client, iio);
>>> +
>>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>>> +                     client->name);
>>
>> no extra logging, this adds little extra information
> 
> I disagree - lets discuss it in my reply to Jonathan.
> 
>>
>>> +     return 0;
>>> +}
>>
>> turn off measurement in _remove()?
> 
> Ok.
> 
> Thanks for the review.
> 
> Cheers,
> 
> Joel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web