On Sat, May 6, 2017 at 4:15 AM, Jonathan Cameron <jic23@xxxxxxxxxx> 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@xxxxxxxxx>
> 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.
>
> 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?
>> +
>> + 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?
Cheers,
Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
