Message-ID: <ebf193f2-9606-2bf1-fd75-146ea7b57677@kernel.org>
Date:   Sun, 14 May 2017 15:51:57 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Joel Stanley <joel@....id.au>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-iio@...r.kernel.org, Andrew Jeffery <andrew@...id.au>
Subject: Re: [PATCH] iio: Add driver for Infineon DPS310

On 12/05/17 07:28, Joel Stanley wrote:
> On Fri, May 5, 2017 at 9:13 PM, Peter Meerwald-Stadler
> <pmeerw@...erw.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@....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@....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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists