lkml.org 
[lkml]   [2017]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] iio: Add driver for Infineon DPS310
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

\
 
 \ /
  Last update: 2017-05-12 08:29    [W:1.052 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site