Message-ID: <6616f6f5-652a-3666-a60e-a700f706e11b@kernel.org>
Date:   Sun, 14 May 2017 15:56:13 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Joel Stanley <joel@....id.au>
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:18, Joel Stanley wrote:
> On Sat, May 6, 2017 at 4:15 AM, Jonathan Cameron <jic23@...nel.org> wrote:
>> On 04/05/17 08:19, Joel Stanley wrote:
>>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>>> i2c and SPI.
>>>
>>> This driver supports polled measurement of temperature over i2c only.
>>>
>>> Signed-off-by: Joel Stanley <joel@....id.au>
>> Few bits inline, but looks pretty good for a v1.
> 
> Thanks for the reivew.
> 
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>> Slight preference for alphabetical order though grouping the iio
>> ones at the end is fine.
> 
> Ok.
> 
>>> +
>> General preference in IIO is to prefix
>> all defines with something driver appropriate.
>>
>> DPS310_PRS_BASE etc
> 
> Sure.
> 
>>
>> Avoids potential mess down the line with defines
>> in headers. Lots of these are very generic so
>> that's not implausible.
>>> +#define PRS_BASE     0x00
>>> +#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)
>> You define these but then have it long hand in the code?
> 
> I will drop these I think. I've reworked the code a few times and
> these were left over.
> 
>>> +static int dps310_get_temp_k(struct dps310_data *data)
>>> +{
>>> +     int index = ilog2(dps310_get_temp_precision(data));
>>> +
>>> +     return scale_factor[index];
>> I'd just put the whole thing in one line, but up to you.
> 
> I previously had a check here that the range was not out of range but
> I dropped the check as it can't happen.
> 
> I'll put it on one line.
> 
>>> +static bool dps310_is_volatile_reg(struct device *dev, unsigned int reg)
>>> +{
>>> +     switch (reg) {
>>> +     case PRS_BASE ... (PRS_BASE + 2):
>> Bit ugly defining this like this.  IS PRS_BASE + 2 something that has
>> a logical name of its own?
> 
> Agreed. I used to have PRS_B0, PRS_B1, PRS_B2 as we read out the three
> bytes at once they were not required. I'll clean it up.
> 
> 
>>> +
>>> +static int dps310_read_raw(struct iio_dev *iio,
>>> +                        struct iio_chan_spec const *chan,
>>> +                        int *val, int *val2, long mask)
>>> +{
>>> +     struct dps310_data *data = iio_priv(iio);
>>> +     int ret;
>>> +
>>> +     /* c0 * 0.5 + c1 * T_raw / kT °C */
>> Whilst interesting why is this comment right here?
> 
> Will move it to the top of the file.
> 
>>> +
>>> +static int dps310_probe(struct i2c_client *client,
>>> +                     const struct i2c_device_id *id)
>>> +{
>>> +     struct dps310_data *data;
>>> +     struct iio_dev *iio;
>>> +     int r;
>>> +
>>> +     iio = devm_iio_device_alloc(&client->dev,  sizeof(*data));
>>> +     if (!iio)
>>> +             return -ENOMEM;
>>> +
>>> +     data = iio_priv(iio);
>>> +     data->client = client;
>>> +
>>> +     iio->dev.parent = &client->dev;
>>> +     iio->name = id->name;
>>> +     iio->channels = dps310_channels;
>>> +     iio->num_channels = ARRAY_SIZE(dps310_channels);
>>> +     iio->info = &dps310_info;
>>> +     iio->modes = INDIO_DIRECT_MODE;
>>> +
>>> +     data->regmap = devm_regmap_init_i2c(client, &dps310_regmap_config);
>>> +     if (IS_ERR(data->regmap))
>>> +             return PTR_ERR(data->regmap);
>>> +
>> Not that obvious what the next two calls are doing, perhaps comments?
>> (the third one is good)
> 
> Ack.
> 
>>> +     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);
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     /*
>>> +      * Calibration coefficients required for reporting temperature.
>>> +      * They are availalbe 40ms after the device has started
>> available
>>> +      */
>>> +     r = dps310_get_temp_coef(data);
>>> +     if (r == -EAGAIN)
>>> +             return -EPROBE_DEFER;
>> Deferred why?  If you need 40ms then sleep for 40ms to be sure it
>> has woken up. Or even better, loop with a sleep.
> 
> I didn't want to delay booting the system for the driver to probe.
Hmm. Guessing parallel probing not an option on your system.
Add a comment to that effect and leave this in.
> 
>>
>> Deferred will only result in it being probed if something changes
>> such as another driver being loaded (which might provide
>> some resource we are waiting for).
> 
> Ok, I misunderstood how it worked. Thanks for clearing that up.
> 
> I will put the loop/sleep in.
> 
>>> +     if (r < 0)
>>> +             return r;
>>> +
>>> +     r = devm_iio_device_register(&client->dev, iio);
>>> +     if (r)
>>> +             return r;
>> With the two lines below removed, just
>> return devm_iio...
>>> +
>>> +     i2c_set_clientdata(client, iio);
>> Used?
> 
> I must admit I was cargo culting. Can you explain under what
> circumstances I would want this?
If you had a remove as you'd use i2c_get_clientdata to get back
a reference to the iio structures.
> 
>>> +
>>> +     dev_info(&client->dev, "%s: sensor '%s'\n", dev_name(&iio->dev),
>>> +                     client->name);
>> Provides little useful information and just spams the log so
>> I would prefer this was dropped.
> 
> I would really like the core to do something like this, for both hwmon
> and iio devices. I spend a lot of time developing and using systems
> that use these drivers, and it's very useful to know the name of the
> driver, the bus it's on, and the fact that it was both compiled in and
> probed.
> 
> Would you take a patch for the core?
No. To my mind it's trivial to look in sysfs.  There is a lot of
feeling in the community that people are way to tempted to put
messages in the log which can be easily established without it.

A few lines of script would get you the same information.

This isn't a time critical element either so it's timing in the
log doesn't tell you anything either.
> 
> 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