* [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready
@ 2018-04-30 4:56 Richard Tresidder
2018-04-30 6:23 ` [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode Richard Tresidder
2018-04-30 9:50 ` [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
0 siblings, 2 replies; 7+ messages in thread
From: Richard Tresidder @ 2018-04-30 4:56 UTC (permalink / raw)
To: linux-iio
Hi
First patch attempt.
Currently the driver runs into problems when trying to acquire at sample
rates higher than 50sps.
This is due to the usage of msleep when data is not ready.
This patch attempts to speed things up by utilising the usleep_range
call to allow shorter sleep times.
I've tested this using iio buffers and hr triggers up to 100sps on a
mma8451 device.
It should technically be ok up to the 800sps rate though.
I seem to have snuck a couple of whitespace alignment fixes into this also
Please let me know if I should remove separate them
Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
---
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7a2da7f..cede523 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -57,7 +57,7 @@
#define MMA8452_FF_MT_THS 0x17
#define MMA8452_FF_MT_THS_MASK 0x7f
#define MMA8452_FF_MT_COUNT 0x18
-#define MMA8452_FF_MT_CHAN_SHIFT 3
+#define MMA8452_FF_MT_CHAN_SHIFT 3
#define MMA8452_TRANSIENT_CFG 0x1d
#define MMA8452_TRANSIENT_CFG_CHAN(chan) BIT(chan + 1)
#define MMA8452_TRANSIENT_CFG_HPF_BYP BIT(0)
@@ -69,7 +69,7 @@
#define MMA8452_TRANSIENT_THS 0x1f
#define MMA8452_TRANSIENT_THS_MASK GENMASK(6, 0)
#define MMA8452_TRANSIENT_COUNT 0x20
-#define MMA8452_TRANSIENT_CHAN_SHIFT 1
+#define MMA8452_TRANSIENT_CHAN_SHIFT 1
#define MMA8452_CTRL_REG1 0x2a
#define MMA8452_CTRL_ACTIVE BIT(0)
#define MMA8452_CTRL_DR_MASK GENMASK(5, 3)
@@ -106,6 +106,7 @@ struct mma8452_data {
u8 ctrl_reg1;
u8 data_cfg;
const struct mma_chip_info *chip_info;
+ int sleep_val;
};
/**
@@ -193,7 +194,10 @@ static int mma8452_drdy(struct mma8452_data *data)
if ((ret & MMA8452_STATUS_DRDY) == MMA8452_STATUS_DRDY)
return 0;
- msleep(20);
+ if (data->sleep_val <= 20)
+ usleep_range(data->sleep_val * 250, data->sleep_val * 500);
+ else
+ msleep(20);
}
dev_err(&data->client->dev, "data not ready\n");
@@ -544,10 +548,22 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int mma8452_calculate_sleep(struct mma8452_data *data)
+{
+ int ret, i = mma8452_get_odr_index(data);
+
+ if (mma8452_samp_freq[i][0] > 0)
+ ret = 1000 / mma8452_samp_freq[i][0];
+ else
+ ret = 1000;
+
+ return ret == 0 ? 1 : ret;
+}
+
static int mma8452_standby(struct mma8452_data *data)
{
return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
- data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
+ data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
}
static int mma8452_active(struct mma8452_data *data)
@@ -700,6 +716,8 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
+ data->sleep_val = mma8452_calculate_sleep(data);
+
ret = mma8452_change_config(data, MMA8452_CTRL_REG1,
data->ctrl_reg1);
break;
@@ -738,7 +756,7 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
}
ret = mma8452_change_config(data, MMA8452_DATA_CFG,
- data->data_cfg);
+ data->data_cfg);
break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -761,8 +779,9 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
}
static int mma8452_get_event_regs(struct mma8452_data *data,
- const struct iio_chan_spec *chan, enum iio_event_direction dir,
- const struct mma8452_event_regs **ev_reg)
+ const struct iio_chan_spec *chan,
+ enum iio_event_direction dir,
+ const struct mma8452_event_regs **ev_reg)
{
if (!chan)
return -EINVAL;
@@ -772,9 +791,9 @@ static int mma8452_get_event_regs(struct
mma8452_data *data,
switch (dir) {
case IIO_EV_DIR_RISING:
if ((data->chip_info->all_events
- & MMA8452_INT_TRANS) &&
- (data->chip_info->enabled_events
- & MMA8452_INT_TRANS))
+ & MMA8452_INT_TRANS) &&
+ (data->chip_info->enabled_events
+ & MMA8452_INT_TRANS))
*ev_reg = &trans_ev_regs;
else
*ev_reg = &ff_mt_ev_regs;
@@ -791,11 +810,11 @@ static int mma8452_get_event_regs(struct
mma8452_data *data,
}
static int mma8452_read_event_value(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir,
- enum iio_event_info info,
- int *val, int *val2)
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
@@ -854,11 +873,11 @@ static int mma8452_read_event_value(struct iio_dev
*indio_dev,
}
static int mma8452_write_event_value(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir,
- enum iio_event_info info,
- int val, int val2)
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
@@ -1528,6 +1547,7 @@ static int mma8452_probe(struct i2c_client *client,
case FXLS8471_DEVICE_ID:
if (ret == data->chip_info->chip_id)
break;
+ /* FALLTHRU */
default:
return -ENODEV;
}
@@ -1593,6 +1613,9 @@ static int mma8452_probe(struct i2c_client *client,
data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
+
+ data->sleep_val = mma8452_calculate_sleep(data);
+
ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
data->ctrl_reg1);
if (ret < 0)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode
2018-04-30 4:56 [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
@ 2018-04-30 6:23 ` Richard Tresidder
2018-04-30 9:53 ` Richard Tresidder
2018-04-30 9:50 ` [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
1 sibling, 1 reply; 7+ messages in thread
From: Richard Tresidder @ 2018-04-30 6:23 UTC (permalink / raw)
To: linux-iio, jic23
Hi
This patch adds the ability to run the Mag3110 in continuous mode to
speed up the sampling rate.
Depending on the sampling rate requested the device can be put in or out
of continuous mode automatically.
Shifting out of continuous mode requires a potential 1 / ODR wait which
is also implemented
This part is largely based on the mma8542 driver implementation.
Also modified the sleep method when data is not ready to allow for
sampling > 50sps to work.
This is similar to my other recent patch regarding the mma8452 driver.
Have tested upto 80sps using hr timer and iio buffer
Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
---
diff --git a/drivers/iio/magnetometer/mag3110.c
b/drivers/iio/magnetometer/mag3110.c
index b34ace7..7cdd185 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -26,6 +26,7 @@
#define MAG3110_OUT_Y 0x03
#define MAG3110_OUT_Z 0x05
#define MAG3110_WHO_AM_I 0x07
+#define MAG3110_SYSMOD 0x08
#define MAG3110_OFF_X 0x09 /* MSB first */
#define MAG3110_OFF_Y 0x0b
#define MAG3110_OFF_Z 0x0d
@@ -39,6 +40,8 @@
#define MAG3110_CTRL_DR_SHIFT 5
#define MAG3110_CTRL_DR_DEFAULT 0
+#define MAG3110_SYSMOD_MODE_MASK (BIT(1) | BIT(0))
+
#define MAG3110_CTRL_TM BIT(1) /* trigger single measurement */
#define MAG3110_CTRL_AC BIT(0) /* continuous measurements */
@@ -52,26 +55,35 @@ struct mag3110_data {
struct i2c_client *client;
struct mutex lock;
u8 ctrl_reg1;
+ int sleep_val;
};
static int mag3110_request(struct mag3110_data *data)
{
int ret, tries = 150;
- /* trigger measurement */
- ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
- data->ctrl_reg1 | MAG3110_CTRL_TM);
- if (ret < 0)
- return ret;
+ if ((data->ctrl_reg1 & MAG3110_CTRL_AC) == 0) {
+ /* trigger measurement */
+ ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1 | MAG3110_CTRL_TM);
+ if (ret < 0)
+ return ret;
+ }
while (tries-- > 0) {
ret = i2c_smbus_read_byte_data(data->client, MAG3110_STATUS);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&data->client->dev, "i2c error\n");
return ret;
+ }
/* wait for data ready */
if ((ret & MAG3110_STATUS_DRDY) == MAG3110_STATUS_DRDY)
break;
- msleep(20);
+
+ if (data->sleep_val <= 20)
+ usleep_range(data->sleep_val * 250, data->sleep_val * 500);
+ else
+ msleep(20);
}
if (tries < 0) {
@@ -100,7 +112,7 @@ static int mag3110_read(struct mag3110_data *data,
__be16 buf[3])
}
static ssize_t mag3110_show_int_plus_micros(char *buf,
- const int (*vals)[2], int n)
+ const int (*vals)[2], int n)
{
size_t len = 0;
@@ -115,7 +127,7 @@ static ssize_t mag3110_show_int_plus_micros(char *buf,
}
static int mag3110_get_int_plus_micros_index(const int (*vals)[2], int n,
- int val, int val2)
+ int val, int val2)
{
while (n-- > 0)
if (val == vals[n][0] && val2 == vals[n][1])
@@ -130,7 +142,8 @@ static int mag3110_get_int_plus_micros_index(const
int (*vals)[2], int n,
};
static ssize_t mag3110_show_samp_freq_avail(struct device *dev,
- struct device_attribute *attr, char *buf)
+ struct device_attribute *attr,
+ char *buf)
{
return mag3110_show_int_plus_micros(buf, mag3110_samp_freq, 8);
}
@@ -138,12 +151,118 @@ static ssize_t
mag3110_show_samp_freq_avail(struct device *dev,
static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mag3110_show_samp_freq_avail);
static int mag3110_get_samp_freq_index(struct mag3110_data *data,
- int val, int val2)
+ int val, int val2)
{
return mag3110_get_int_plus_micros_index(mag3110_samp_freq, 8, val,
val2);
}
+static int mag3110_calculate_sleep(struct mag3110_data *data)
+{
+ int ret, i = data->ctrl_reg1 >> MAG3110_CTRL_DR_SHIFT;
+
+ if(mag3110_samp_freq[i][0] > 0)
+ ret = 1000 / mag3110_samp_freq[i][0];
+ else
+ ret = 1000;
+
+ return ret == 0 ? 1 : ret;
+}
+
+static int mag3110_standby(struct mag3110_data *data)
+{
+ return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1 & ~MAG3110_CTRL_AC);
+}
+
+static int mag3110_wait_standby(struct mag3110_data *data)
+{
+ int ret, tries = 30;
+
+ /* Takes up to 1/ODR to come out of active mode into stby
+ Longest expected period is 12.5seconds. We'll sleep for 500ms
between checks*/
+ while (tries-- > 0) {
+ ret = i2c_smbus_read_byte_data(data->client, MAG3110_SYSMOD);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "i2c error\n");
+ return ret;
+ }
+ /* wait for standby */
+ if ((ret & MAG3110_SYSMOD_MODE_MASK) == 0)
+ break;
+
+ msleep_interruptible(500);
+ }
+
+ if (tries < 0) {
+ dev_err(&data->client->dev, "device not entering standby mode\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int mag3110_active(struct mag3110_data *data)
+{
+ return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1);
+}
+
+/* returns >0 if active, 0 if in standby and <0 on error */
+static int mag3110_is_active(struct mag3110_data *data)
+{
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client, MAG3110_CTRL_REG1);
+ if (reg < 0)
+ return reg;
+
+ return reg & MAG3110_CTRL_AC;
+}
+
+static int mag3110_change_config(struct mag3110_data *data, u8 reg, u8 val)
+{
+ int ret;
+ int is_active;
+
+ mutex_lock(&data->lock);
+
+ is_active = mag3110_is_active(data);
+ if (is_active < 0) {
+ ret = is_active;
+ goto fail;
+ }
+
+ /* config can only be changed when in standby */
+ if (is_active > 0) {
+ ret = mag3110_standby(data);
+ if (ret < 0)
+ goto fail;
+ }
+
+ /* After coming out of active we must wait for the part to
transition to STBY
+ This can take up to 1 /ODR to occur */
+ ret = mag3110_wait_standby(data);
+ if (ret < 0)
+ goto fail;
+
+ ret = i2c_smbus_write_byte_data(data->client, reg, val);
+ if (ret < 0)
+ goto fail;
+
+ if (is_active > 0) {
+ ret = mag3110_active(data);
+ if (ret < 0)
+ goto fail;
+ }
+
+ ret = 0;
+fail:
+ mutex_unlock(&data->lock);
+
+ return ret;
+}
+
static int mag3110_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -235,11 +354,13 @@ static int mag3110_write_raw(struct iio_dev
*indio_dev,
ret = -EINVAL;
break;
}
-
- data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK;
+ data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK & ~MAG3110_CTRL_AC;
data->ctrl_reg1 |= rate << MAG3110_CTRL_DR_SHIFT;
- ret = i2c_smbus_write_byte_data(data->client,
- MAG3110_CTRL_REG1, data->ctrl_reg1);
+ data->sleep_val = mag3110_calculate_sleep(data);
+ if (data->sleep_val < 40)
+ data->ctrl_reg1 |= MAG3110_CTRL_AC;
+
+ ret = mag3110_change_config(data, MAG3110_CTRL_REG1,
data->ctrl_reg1);
break;
case IIO_CHAN_INFO_CALIBBIAS:
if (val < -10000 || val > 10000) {
@@ -337,12 +458,6 @@ static irqreturn_t mag3110_trigger_handler(int irq,
void *p)
static const unsigned long mag3110_scan_masks[] = {0x7, 0xf, 0};
-static int mag3110_standby(struct mag3110_data *data)
-{
- return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
- data->ctrl_reg1 & ~MAG3110_CTRL_AC);
-}
-
static int mag3110_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -374,8 +489,11 @@ static int mag3110_probe(struct i2c_client *client,
indio_dev->available_scan_masks = mag3110_scan_masks;
data->ctrl_reg1 = MAG3110_CTRL_DR_DEFAULT << MAG3110_CTRL_DR_SHIFT;
- ret = i2c_smbus_write_byte_data(client, MAG3110_CTRL_REG1,
- data->ctrl_reg1);
+ data->sleep_val = mag3110_calculate_sleep(data);
+ if (data->sleep_val < 40)
+ data->ctrl_reg1 |= MAG3110_CTRL_AC;
+
+ ret = mag3110_change_config(data, MAG3110_CTRL_REG1, data->ctrl_reg1);
if (ret < 0)
return ret;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready
2018-04-30 4:56 [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
2018-04-30 6:23 ` [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode Richard Tresidder
@ 2018-04-30 9:50 ` Richard Tresidder
2018-05-06 16:29 ` Jonathan Cameron
1 sibling, 1 reply; 7+ messages in thread
From: Richard Tresidder @ 2018-04-30 9:50 UTC (permalink / raw)
To: linux-iio, jic23
Hi
Sorry realised I hadn't cc'd a maintainer so trying again
First patch attempt.
Currently the driver runs into problems when trying to acquire at sample
rates higher than 50sps.
This is due to the usage of msleep when data is not ready.
This patch attempts to speed things up by utilising the usleep_range
call to allow shorter sleep times.
I've tested this using iio buffers and hr triggers up to 100sps on a
mma8451 device.
It should technically be ok up to the 800sps rate though.
I seem to have snuck a couple of whitespace alignment fixes into this also
Please let me know if I should remove separate them
Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
---
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7a2da7f..cede523 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -57,7 +57,7 @@
#define MMA8452_FF_MT_THS 0x17
#define MMA8452_FF_MT_THS_MASK 0x7f
#define MMA8452_FF_MT_COUNT 0x18
-#define MMA8452_FF_MT_CHAN_SHIFT 3
+#define MMA8452_FF_MT_CHAN_SHIFT 3
#define MMA8452_TRANSIENT_CFG 0x1d
#define MMA8452_TRANSIENT_CFG_CHAN(chan) BIT(chan + 1)
#define MMA8452_TRANSIENT_CFG_HPF_BYP BIT(0)
@@ -69,7 +69,7 @@
#define MMA8452_TRANSIENT_THS 0x1f
#define MMA8452_TRANSIENT_THS_MASK GENMASK(6, 0)
#define MMA8452_TRANSIENT_COUNT 0x20
-#define MMA8452_TRANSIENT_CHAN_SHIFT 1
+#define MMA8452_TRANSIENT_CHAN_SHIFT 1
#define MMA8452_CTRL_REG1 0x2a
#define MMA8452_CTRL_ACTIVE BIT(0)
#define MMA8452_CTRL_DR_MASK GENMASK(5, 3)
@@ -106,6 +106,7 @@ struct mma8452_data {
u8 ctrl_reg1;
u8 data_cfg;
const struct mma_chip_info *chip_info;
+ int sleep_val;
};
/**
@@ -193,7 +194,10 @@ static int mma8452_drdy(struct mma8452_data *data)
if ((ret & MMA8452_STATUS_DRDY) == MMA8452_STATUS_DRDY)
return 0;
- msleep(20);
+ if (data->sleep_val <= 20)
+ usleep_range(data->sleep_val * 250, data->sleep_val * 500);
+ else
+ msleep(20);
}
dev_err(&data->client->dev, "data not ready\n");
@@ -544,10 +548,22 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int mma8452_calculate_sleep(struct mma8452_data *data)
+{
+ int ret, i = mma8452_get_odr_index(data);
+
+ if (mma8452_samp_freq[i][0] > 0)
+ ret = 1000 / mma8452_samp_freq[i][0];
+ else
+ ret = 1000;
+
+ return ret == 0 ? 1 : ret;
+}
+
static int mma8452_standby(struct mma8452_data *data)
{
return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
- data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
+ data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
}
static int mma8452_active(struct mma8452_data *data)
@@ -700,6 +716,8 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
+ data->sleep_val = mma8452_calculate_sleep(data);
+
ret = mma8452_change_config(data, MMA8452_CTRL_REG1,
data->ctrl_reg1);
break;
@@ -738,7 +756,7 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
}
ret = mma8452_change_config(data, MMA8452_DATA_CFG,
- data->data_cfg);
+ data->data_cfg);
break;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -761,8 +779,9 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
}
static int mma8452_get_event_regs(struct mma8452_data *data,
- const struct iio_chan_spec *chan, enum iio_event_direction dir,
- const struct mma8452_event_regs **ev_reg)
+ const struct iio_chan_spec *chan,
+ enum iio_event_direction dir,
+ const struct mma8452_event_regs **ev_reg)
{
if (!chan)
return -EINVAL;
@@ -772,9 +791,9 @@ static int mma8452_get_event_regs(struct
mma8452_data *data,
switch (dir) {
case IIO_EV_DIR_RISING:
if ((data->chip_info->all_events
- & MMA8452_INT_TRANS) &&
- (data->chip_info->enabled_events
- & MMA8452_INT_TRANS))
+ & MMA8452_INT_TRANS) &&
+ (data->chip_info->enabled_events
+ & MMA8452_INT_TRANS))
*ev_reg = &trans_ev_regs;
else
*ev_reg = &ff_mt_ev_regs;
@@ -791,11 +810,11 @@ static int mma8452_get_event_regs(struct
mma8452_data *data,
}
static int mma8452_read_event_value(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir,
- enum iio_event_info info,
- int *val, int *val2)
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, us, power_mode;
@@ -854,11 +873,11 @@ static int mma8452_read_event_value(struct iio_dev
*indio_dev,
}
static int mma8452_write_event_value(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir,
- enum iio_event_info info,
- int val, int val2)
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
{
struct mma8452_data *data = iio_priv(indio_dev);
int ret, reg, steps;
@@ -1528,6 +1547,7 @@ static int mma8452_probe(struct i2c_client *client,
case FXLS8471_DEVICE_ID:
if (ret == data->chip_info->chip_id)
break;
+ /* FALLTHRU */
default:
return -ENODEV;
}
@@ -1593,6 +1613,9 @@ static int mma8452_probe(struct i2c_client *client,
data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
+
+ data->sleep_val = mma8452_calculate_sleep(data);
+
ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
data->ctrl_reg1);
if (ret < 0)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode
2018-04-30 6:23 ` [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode Richard Tresidder
@ 2018-04-30 9:53 ` Richard Tresidder
2018-05-06 16:45 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Richard Tresidder @ 2018-04-30 9:53 UTC (permalink / raw)
To: linux-iio, jic23
Hi
Again sorry had maintainer wrong...
This patch adds the ability to run the Mag3110 in continuous mode to
speed up the sampling rate.
Depending on the sampling rate requested the device can be put in or out
of continuous mode automatically.
Shifting out of continuous mode requires a potential 1 / ODR wait which
is also implemented
This part is largely based on the mma8542 driver implementation.
Also modified the sleep method when data is not ready to allow for
sampling > 50sps to work.
This is similar to my other recent patch regarding the mma8452 driver.
Have tested upto 80sps using hr timer and iio buffer
Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
---
diff --git a/drivers/iio/magnetometer/mag3110.c
b/drivers/iio/magnetometer/mag3110.c
index b34ace7..7cdd185 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -26,6 +26,7 @@
#define MAG3110_OUT_Y 0x03
#define MAG3110_OUT_Z 0x05
#define MAG3110_WHO_AM_I 0x07
+#define MAG3110_SYSMOD 0x08
#define MAG3110_OFF_X 0x09 /* MSB first */
#define MAG3110_OFF_Y 0x0b
#define MAG3110_OFF_Z 0x0d
@@ -39,6 +40,8 @@
#define MAG3110_CTRL_DR_SHIFT 5
#define MAG3110_CTRL_DR_DEFAULT 0
+#define MAG3110_SYSMOD_MODE_MASK (BIT(1) | BIT(0))
+
#define MAG3110_CTRL_TM BIT(1) /* trigger single measurement */
#define MAG3110_CTRL_AC BIT(0) /* continuous measurements */
@@ -52,26 +55,35 @@ struct mag3110_data {
struct i2c_client *client;
struct mutex lock;
u8 ctrl_reg1;
+ int sleep_val;
};
static int mag3110_request(struct mag3110_data *data)
{
int ret, tries = 150;
- /* trigger measurement */
- ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
- data->ctrl_reg1 | MAG3110_CTRL_TM);
- if (ret < 0)
- return ret;
+ if ((data->ctrl_reg1 & MAG3110_CTRL_AC) == 0) {
+ /* trigger measurement */
+ ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1 | MAG3110_CTRL_TM);
+ if (ret < 0)
+ return ret;
+ }
while (tries-- > 0) {
ret = i2c_smbus_read_byte_data(data->client, MAG3110_STATUS);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&data->client->dev, "i2c error\n");
return ret;
+ }
/* wait for data ready */
if ((ret & MAG3110_STATUS_DRDY) == MAG3110_STATUS_DRDY)
break;
- msleep(20);
+
+ if (data->sleep_val <= 20)
+ usleep_range(data->sleep_val * 250, data->sleep_val * 500);
+ else
+ msleep(20);
}
if (tries < 0) {
@@ -100,7 +112,7 @@ static int mag3110_read(struct mag3110_data *data,
__be16 buf[3])
}
static ssize_t mag3110_show_int_plus_micros(char *buf,
- const int (*vals)[2], int n)
+ const int (*vals)[2], int n)
{
size_t len = 0;
@@ -115,7 +127,7 @@ static ssize_t mag3110_show_int_plus_micros(char *buf,
}
static int mag3110_get_int_plus_micros_index(const int (*vals)[2], int n,
- int val, int val2)
+ int val, int val2)
{
while (n-- > 0)
if (val == vals[n][0] && val2 == vals[n][1])
@@ -130,7 +142,8 @@ static int mag3110_get_int_plus_micros_index(const
int (*vals)[2], int n,
};
static ssize_t mag3110_show_samp_freq_avail(struct device *dev,
- struct device_attribute *attr, char *buf)
+ struct device_attribute *attr,
+ char *buf)
{
return mag3110_show_int_plus_micros(buf, mag3110_samp_freq, 8);
}
@@ -138,12 +151,118 @@ static ssize_t
mag3110_show_samp_freq_avail(struct device *dev,
static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mag3110_show_samp_freq_avail);
static int mag3110_get_samp_freq_index(struct mag3110_data *data,
- int val, int val2)
+ int val, int val2)
{
return mag3110_get_int_plus_micros_index(mag3110_samp_freq, 8, val,
val2);
}
+static int mag3110_calculate_sleep(struct mag3110_data *data)
+{
+ int ret, i = data->ctrl_reg1 >> MAG3110_CTRL_DR_SHIFT;
+
+ if(mag3110_samp_freq[i][0] > 0)
+ ret = 1000 / mag3110_samp_freq[i][0];
+ else
+ ret = 1000;
+
+ return ret == 0 ? 1 : ret;
+}
+
+static int mag3110_standby(struct mag3110_data *data)
+{
+ return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1 & ~MAG3110_CTRL_AC);
+}
+
+static int mag3110_wait_standby(struct mag3110_data *data)
+{
+ int ret, tries = 30;
+
+ /* Takes up to 1/ODR to come out of active mode into stby
+ Longest expected period is 12.5seconds. We'll sleep for 500ms
between checks*/
+ while (tries-- > 0) {
+ ret = i2c_smbus_read_byte_data(data->client, MAG3110_SYSMOD);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "i2c error\n");
+ return ret;
+ }
+ /* wait for standby */
+ if ((ret & MAG3110_SYSMOD_MODE_MASK) == 0)
+ break;
+
+ msleep_interruptible(500);
+ }
+
+ if (tries < 0) {
+ dev_err(&data->client->dev, "device not entering standby mode\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int mag3110_active(struct mag3110_data *data)
+{
+ return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
+ data->ctrl_reg1);
+}
+
+/* returns >0 if active, 0 if in standby and <0 on error */
+static int mag3110_is_active(struct mag3110_data *data)
+{
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client, MAG3110_CTRL_REG1);
+ if (reg < 0)
+ return reg;
+
+ return reg & MAG3110_CTRL_AC;
+}
+
+static int mag3110_change_config(struct mag3110_data *data, u8 reg, u8 val)
+{
+ int ret;
+ int is_active;
+
+ mutex_lock(&data->lock);
+
+ is_active = mag3110_is_active(data);
+ if (is_active < 0) {
+ ret = is_active;
+ goto fail;
+ }
+
+ /* config can only be changed when in standby */
+ if (is_active > 0) {
+ ret = mag3110_standby(data);
+ if (ret < 0)
+ goto fail;
+ }
+
+ /* After coming out of active we must wait for the part to
transition to STBY
+ This can take up to 1 /ODR to occur */
+ ret = mag3110_wait_standby(data);
+ if (ret < 0)
+ goto fail;
+
+ ret = i2c_smbus_write_byte_data(data->client, reg, val);
+ if (ret < 0)
+ goto fail;
+
+ if (is_active > 0) {
+ ret = mag3110_active(data);
+ if (ret < 0)
+ goto fail;
+ }
+
+ ret = 0;
+fail:
+ mutex_unlock(&data->lock);
+
+ return ret;
+}
+
static int mag3110_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -235,11 +354,13 @@ static int mag3110_write_raw(struct iio_dev
*indio_dev,
ret = -EINVAL;
break;
}
-
- data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK;
+ data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK & ~MAG3110_CTRL_AC;
data->ctrl_reg1 |= rate << MAG3110_CTRL_DR_SHIFT;
- ret = i2c_smbus_write_byte_data(data->client,
- MAG3110_CTRL_REG1, data->ctrl_reg1);
+ data->sleep_val = mag3110_calculate_sleep(data);
+ if (data->sleep_val < 40)
+ data->ctrl_reg1 |= MAG3110_CTRL_AC;
+
+ ret = mag3110_change_config(data, MAG3110_CTRL_REG1,
data->ctrl_reg1);
break;
case IIO_CHAN_INFO_CALIBBIAS:
if (val < -10000 || val > 10000) {
@@ -337,12 +458,6 @@ static irqreturn_t mag3110_trigger_handler(int irq,
void *p)
static const unsigned long mag3110_scan_masks[] = {0x7, 0xf, 0};
-static int mag3110_standby(struct mag3110_data *data)
-{
- return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
- data->ctrl_reg1 & ~MAG3110_CTRL_AC);
-}
-
static int mag3110_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -374,8 +489,11 @@ static int mag3110_probe(struct i2c_client *client,
indio_dev->available_scan_masks = mag3110_scan_masks;
data->ctrl_reg1 = MAG3110_CTRL_DR_DEFAULT << MAG3110_CTRL_DR_SHIFT;
- ret = i2c_smbus_write_byte_data(client, MAG3110_CTRL_REG1,
- data->ctrl_reg1);
+ data->sleep_val = mag3110_calculate_sleep(data);
+ if (data->sleep_val < 40)
+ data->ctrl_reg1 |= MAG3110_CTRL_AC;
+
+ ret = mag3110_change_config(data, MAG3110_CTRL_REG1, data->ctrl_reg1);
if (ret < 0)
return ret;
--
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready
2018-04-30 9:50 ` [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
@ 2018-05-06 16:29 ` Jonathan Cameron
0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2018-05-06 16:29 UTC (permalink / raw)
To: Richard Tresidder; +Cc: linux-iio
On Mon, 30 Apr 2018 17:50:06 +0800
Richard Tresidder <rtresidd@electromag.com.au> wrote:
> Hi
> Sorry realised I hadn't cc'd a maintainer so trying again
> First patch attempt.
> Currently the driver runs into problems when trying to acquire at sample
> rates higher than 50sps.
> This is due to the usage of msleep when data is not ready.
> This patch attempts to speed things up by utilising the usleep_range
> call to allow shorter sleep times.
>=20
> I've tested this using iio buffers and hr triggers up to 100sps on a
> mma8451 device.
> It should technically be ok up to the 800sps rate though.
>=20
> I seem to have snuck a couple of whitespace alignment fixes into this also
> Please let me know if I should remove separate them
Separate patches please. It is possible we will want to backport
the reduction in sleep time to older kernels, where as we won't bother
with the white space stuff.
Also I think your email client has replaced all tabs with spaces
thus making the patch broken. Please fix that up before sending
a v2.
The fundamental change seems sensible to me. Please cc
the original driver author on future versions (it's also
worth checking for anyone else who had made large changes
recently and adding them to the cc.)
Jonathan
>=20
> Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
> ---
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7a2da7f..cede523 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -57,7 +57,7 @@
> =C2=A0#define MMA8452_FF_MT_THS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 0x17
> =C2=A0#define=C2=A0 MMA8452_FF_MT_THS_MASK=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x7f
> =C2=A0#define MMA8452_FF_MT_COUNT=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x18
> -#define MMA8452_FF_MT_CHAN_SHIFT=C2=A0=C2=A0=C2=A0 3
> +#define MMA8452_FF_MT_CHAN_SHIFT=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 3
> =C2=A0#define MMA8452_TRANSIENT_CFG=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x1d
> =C2=A0#define=C2=A0 MMA8452_TRANSIENT_CFG_CHAN(chan)=C2=A0=C2=A0=C2=A0 BI=
T(chan + 1)
> =C2=A0#define=C2=A0 MMA8452_TRANSIENT_CFG_HPF_BYP=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 BIT(0)
> @@ -69,7 +69,7 @@
> =C2=A0#define MMA8452_TRANSIENT_THS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x1f
> =C2=A0#define=C2=A0 MMA8452_TRANSIENT_THS_MASK=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 GENMASK(6, 0)
> =C2=A0#define MMA8452_TRANSIENT_COUNT=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0x20
> -#define MMA8452_TRANSIENT_CHAN_SHIFT 1
> +#define MMA8452_TRANSIENT_CHAN_SHIFT=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 1
> =C2=A0#define MMA8452_CTRL_REG1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 0x2a
> =C2=A0#define=C2=A0 MMA8452_CTRL_ACTIVE=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 BIT(0)
> =C2=A0#define=C2=A0 MMA8452_CTRL_DR_MASK=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GENMASK(5, 3)
> @@ -106,6 +106,7 @@ struct mma8452_data {
> =C2=A0=C2=A0=C2=A0=C2=A0 u8 ctrl_reg1;
> =C2=A0=C2=A0=C2=A0=C2=A0 u8 data_cfg;
> =C2=A0=C2=A0=C2=A0=C2=A0 const struct mma_chip_info *chip_info;
> +=C2=A0=C2=A0=C2=A0 int sleep_val;
> =C2=A0};
> =C2=A0
> =C2=A0 /**
> @@ -193,7 +194,10 @@ static int mma8452_drdy(struct mma8452_data *data)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((ret & MMA8452_STATU=
S_DRDY) =3D=3D MMA8452_STATUS_DRDY)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
return 0;
> =C2=A0
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 msleep(20);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (data->sleep_val <=3D 20)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uslee=
p_range(data->sleep_val * 250, data->sleep_val * 500);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mslee=
p(20);
> =C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 dev_err(&data->client->dev, "data not ready\n");
> @@ -544,10 +548,22 @@ static int mma8452_read_raw(struct iio_dev *indio_d=
ev,
> =C2=A0=C2=A0=C2=A0=C2=A0 return -EINVAL;
> =C2=A0}
> =C2=A0
> +static int mma8452_calculate_sleep(struct mma8452_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 int ret, i =3D mma8452_get_odr_index(data);
> +=C2=A0=C2=A0
> +=C2=A0=C2=A0=C2=A0 if (mma8452_samp_freq[i][0] > 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D 1000 / mma8452_samp_f=
req[i][0];
> +=C2=A0=C2=A0=C2=A0 else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D 1000;
> +
> +=C2=A0=C2=A0=C2=A0 return ret =3D=3D 0 ? 1 : ret;
> +}
> +
> =C2=A0static int mma8452_standby(struct mma8452_data *data)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 return i2c_smbus_write_byte_data(data->client, M=
MA8452_CTRL_REG1,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 & ~MMA8452_CTRL_=
ACTIVE);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 & ~MMA8452=
_CTRL_ACTIVE);
> =C2=A0}
> =C2=A0
> =C2=A0static int mma8452_active(struct mma8452_data *data)
> @@ -700,6 +716,8 @@ static int mma8452_write_raw(struct iio_dev *indio_de=
v,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 &=3D ~MM=
A8452_CTRL_DR_MASK;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 |=3D i <=
< MMA8452_CTRL_DR_SHIFT;
> =C2=A0
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->sleep_val =3D mma8452_c=
alculate_sleep(data);
> +
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D mma8452_change_c=
onfig(data, MMA8452_CTRL_REG1,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 da=
ta->ctrl_reg1);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
> @@ -738,7 +756,7 @@ static int mma8452_write_raw(struct iio_dev *indio_de=
v,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D mma8452_change_c=
onfig(data, MMA8452_DATA_CFG,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 da=
ta->data_cfg);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->da=
ta_cfg);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -761,8 +779,9 @@ static int mma8452_write_raw(struct iio_dev *indio_de=
v,
> =C2=A0}
> =C2=A0
> =C2=A0static int mma8452_get_event_regs(struct mma8452_data *data,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec *c=
han, enum iio_event_direction dir,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct mma8452_event_re=
gs **ev_reg)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec *chan,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_direction dir,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct mma8452_event_regs **ev_reg)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 if (!chan)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EINVAL;
> @@ -772,9 +791,9 @@ static int mma8452_get_event_regs(struct
> mma8452_data *data,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 switch (dir) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 case IIO_EV_DIR_RISING:
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
if ((data->chip_info->all_events
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 & MMA8452_INT_TRANS) &&
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 (data->chip_info->enabled_events
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 & MMA8452_INT_TRANS))
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 & MMA8452_INT_TRANS) &&
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 (data->chip_info->enabled_events
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 & MMA8452_INT_TRANS))
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 *ev_reg =3D &trans_ev_regs;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
else
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 *ev_reg =3D &ff_mt_ev_regs;
> @@ -791,11 +810,11 @@ static int mma8452_get_event_regs(struct
> mma8452_data *data,
> =C2=A0}
> =C2=A0
> =C2=A0static int mma8452_read_event_value(struct iio_dev *indio_dev,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec *chan,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_type type,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_direction dir,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_info info,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int *val, int *val2)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec *chan,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_type type,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_direction dir,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_info info,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int *val, int *val2)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 struct mma8452_data *data =3D iio_priv(indio_dev=
);
> =C2=A0=C2=A0=C2=A0=C2=A0 int ret, us, power_mode;
> @@ -854,11 +873,11 @@ static int mma8452_read_event_value(struct iio_dev
> *indio_dev,
> =C2=A0}
> =C2=A0
> =C2=A0static int mma8452_write_event_value(struct iio_dev *indio_dev,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec *chan,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 enum iio_event_type type,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 enum iio_event_direction dir,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 enum iio_event_info info,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 int val, int val2)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct iio_chan_spec=
*chan,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_type type,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_direction d=
ir,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum iio_event_info info,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int val, int val2)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 struct mma8452_data *data =3D iio_priv(indio_dev=
);
> =C2=A0=C2=A0=C2=A0=C2=A0 int ret, reg, steps;
> @@ -1528,6 +1547,7 @@ static int mma8452_probe(struct i2c_client *client,
> =C2=A0=C2=A0=C2=A0=C2=A0 case FXLS8471_DEVICE_ID:
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret =3D=3D data->chi=
p_info->chip_id)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
break;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* FALLTHRU */
> =C2=A0=C2=A0=C2=A0=C2=A0 default:
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -ENODEV;
> =C2=A0=C2=A0=C2=A0=C2=A0 }
> @@ -1593,6 +1613,9 @@ static int mma8452_probe(struct i2c_client *client,
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 =3D MMA8452_CTRL_ACTIVE |
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0
> +=C2=A0=C2=A0=C2=A0 data->sleep_val =3D mma8452_calculate_sleep(data);
> +
> =C2=A0=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_data(client, MMA845=
2_CTRL_REG1,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1);
> =C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
>=20
>=20
> --
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode
2018-04-30 9:53 ` Richard Tresidder
@ 2018-05-06 16:45 ` Jonathan Cameron
2018-05-07 2:37 ` Richard Tresidder
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2018-05-06 16:45 UTC (permalink / raw)
To: Richard Tresidder; +Cc: linux-iio
On Mon, 30 Apr 2018 17:53:54 +0800
Richard Tresidder <rtresidd@electromag.com.au> wrote:
> Hi
> Again sorry had maintainer wrong...
If that happens, don't resend, just reply ccing the maintainer.
Sometimes the maintainer may then ask you to resend. Right now all
I got was a cryptic mess of patches when they turned up in my email
client.
Note, when a patch is applied all the text above the --- goes into
the commit log for ever more. Hence any side comments like this should
be below the --- cut line. That is also where version change logs etc
belong.
Also this has nothing to do with your other patch (at least they should
not be in the same series.) If you didn't want to put them in a single
series then it should have numbered patches and a cover letter.
As it is, we just have the two different sets overlapping because
you sent the second one as a reply.
> =C2=A0=C2=A0 This patch adds the ability to run the Mag3110 in continuous=
mode to
> speed up the sampling rate.
> Depending on the sampling rate requested the device can be put in or out
> of continuous mode automatically.
> Shifting out of continuous mode requires a potential 1 / ODR wait which
> is also implemented
> This part is largely based on the mma8542 driver implementation.
>=20
> Also modified the sleep method when data is not ready to allow for
> sampling > 50sps to work.
> This is similar to my other recent patch regarding the mma8452 driver.
>=20
> Have tested upto 80sps using hr timer and iio buffer
>=20
> Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
Again, I think the fundamental change is good, but as you suggested
in the other patch, you should be sticking to just that change
not a mixture of 'tidy' ups and the real change.
Thanks,
Jonathan
> ---
> diff --git a/drivers/iio/magnetometer/mag3110.c
> b/drivers/iio/magnetometer/mag3110.c
> index b34ace7..7cdd185 100644
> --- a/drivers/iio/magnetometer/mag3110.c
> +++ b/drivers/iio/magnetometer/mag3110.c
> @@ -26,6 +26,7 @@
> =C2=A0#define MAG3110_OUT_Y 0x03
> =C2=A0#define MAG3110_OUT_Z 0x05
> =C2=A0#define MAG3110_WHO_AM_I 0x07
> +#define MAG3110_SYSMOD 0x08
> =C2=A0#define MAG3110_OFF_X 0x09 /* MSB first */
> =C2=A0#define MAG3110_OFF_Y 0x0b
> =C2=A0#define MAG3110_OFF_Z 0x0d
> @@ -39,6 +40,8 @@
> =C2=A0#define MAG3110_CTRL_DR_SHIFT 5
> =C2=A0#define MAG3110_CTRL_DR_DEFAULT 0
> =C2=A0
> +#define MAG3110_SYSMOD_MODE_MASK (BIT(1) | BIT(0))
GENMASK not combination of two individual bits. That would
have been fine if they had independent uses but they don't so
this needs fixing.
> +
> =C2=A0#define MAG3110_CTRL_TM BIT(1) /* trigger single measurement */
> =C2=A0#define MAG3110_CTRL_AC BIT(0) /* continuous measurements */
> =C2=A0
> @@ -52,26 +55,35 @@ struct mag3110_data {
> =C2=A0=C2=A0=C2=A0=C2=A0 struct i2c_client *client;
> =C2=A0=C2=A0=C2=A0=C2=A0 struct mutex lock;
> =C2=A0=C2=A0=C2=A0=C2=A0 u8 ctrl_reg1;
> +=C2=A0=C2=A0=C2=A0 int sleep_val;
> =C2=A0};
> =C2=A0
> =C2=A0static int mag3110_request(struct mag3110_data *data)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 int ret, tries =3D 150;
> =C2=A0
> -=C2=A0=C2=A0=C2=A0 /* trigger measurement */
> -=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_data(data->client, MAG31=
10_CTRL_REG1,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 | MAG3110_CTR=
L_TM);
> -=C2=A0=C2=A0=C2=A0 if (ret < 0)
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ret;
> +=C2=A0=C2=A0=C2=A0 if ((data->ctrl_reg1 & MAG3110_CTRL_AC) =3D=3D 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* trigger measurement */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_=
data(data->client, MAG3110_CTRL_REG1,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data-=
>ctrl_reg1 | MAG3110_CTRL_TM);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retur=
n ret;
> +=C2=A0=C2=A0=C2=A0 }
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 while (tries-- > 0) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_read_b=
yte_data(data->client, MAG3110_STATUS);
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dev_e=
rr(&data->client->dev, "i2c error\n");
Don't introduce new error messages into existing code paths. Definitely
don't do it in a patch making more substantial changes elsewhere.
It just distracts from the important stuff.
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
return ret;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* wait for data ready */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((ret & MAG3110_STATU=
S_DRDY) =3D=3D MAG3110_STATUS_DRDY)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
break;
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 msleep(20);
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (data->sleep_val <=3D 20)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uslee=
p_range(data->sleep_val * 250, data->sleep_val * 500);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mslee=
p(20);
> =C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 if (tries < 0) {
> @@ -100,7 +112,7 @@ static int mag3110_read(struct mag3110_data *data,
> __be16 buf[3])
> =C2=A0}
> =C2=A0
> =C2=A0static ssize_t mag3110_show_int_plus_micros(char *buf,
> -=C2=A0=C2=A0=C2=A0 const int (*vals)[2], int n)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const in=
t (*vals)[2], int n)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 size_t len =3D 0;
> =C2=A0
> @@ -115,7 +127,7 @@ static ssize_t mag3110_show_int_plus_micros(char *buf,
> =C2=A0}
> =C2=A0
> =C2=A0static int mag3110_get_int_plus_micros_index(const int (*vals)[2], =
int n,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int val, int val2)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 in=
t val, int val2)
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 while (n-- > 0)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (val =3D=3D vals[n][0=
] && val2 =3D=3D vals[n][1])
> @@ -130,7 +142,8 @@ static int mag3110_get_int_plus_micros_index(const
> int (*vals)[2], int n,
> =C2=A0};
> =C2=A0
> =C2=A0static ssize_t mag3110_show_samp_freq_avail(struct device *dev,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 struct device_attribute *attr, char *buf)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct d=
evice_attribute *attr,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 char *bu=
f)
Not in this patch same for any more of these above this point.
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 return mag3110_show_int_plus_micros(buf, mag3110=
_samp_freq, 8);
> =C2=A0}
> @@ -138,12 +151,118 @@ static ssize_t
> mag3110_show_samp_freq_avail(struct device *dev,
> =C2=A0static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mag3110_show_samp_freq_avail);
> =C2=A0
> =C2=A0static int mag3110_get_samp_freq_index(struct mag3110_data *data,
> -=C2=A0=C2=A0=C2=A0 int val, int val2)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int val, int v=
al2)
This sort of tidy up doesn't not belong in any patch making a functional
change.
> =C2=A0{
> =C2=A0=C2=A0=C2=A0=C2=A0 return mag3110_get_int_plus_micros_index(mag3110=
_samp_freq, 8, val,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 val2);
> =C2=A0}
> =C2=A0
> +static int mag3110_calculate_sleep(struct mag3110_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 int ret, i =3D data->ctrl_reg1 >> MAG3110_CTRL_DR_SHI=
FT;
> +
> +=C2=A0=C2=A0=C2=A0 if(mag3110_samp_freq[i][0] > 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D 1000 / mag3110_samp_f=
req[i][0];
> +=C2=A0=C2=A0=C2=A0 else
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D 1000;
> +
> +=C2=A0=C2=A0=C2=A0 return ret =3D=3D 0 ? 1 : ret;
> +}
> +
> +static int mag3110_standby(struct mag3110_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 return i2c_smbus_write_byte_data(data->client, MAG311=
0_CTRL_REG1,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 & ~MAG3110=
_CTRL_AC);
> +}
> +
> +static int mag3110_wait_standby(struct mag3110_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 int ret, tries =3D 30;
> +
> +=C2=A0=C2=A0=C2=A0 /* Takes up to 1/ODR to come out of active mode into =
stby
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Longest expected period=
is 12.5seconds. We'll sleep for 500ms
> between checks*/
Please run checkpatch.pl over your patches. I expect it will complain
about this comment. Coding style is very rigorous in the kernel and
this isn't how we do multiline comments.
> +=C2=A0=C2=A0=C2=A0 while (tries-- > 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_read_byte_d=
ata(data->client, MAG3110_SYSMOD);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dev_e=
rr(&data->client->dev, "i2c error\n");
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retur=
n ret;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* wait for standby */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((ret & MAG3110_SYSMOD_MOD=
E_MASK) =3D=3D 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 msleep_interruptible(500);
> +=C2=A0=C2=A0=C2=A0 }
> +
> +=C2=A0=C2=A0=C2=A0 if (tries < 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dev_err(&data->client->dev, "=
device not entering standby mode\n");
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EIO;
> +=C2=A0=C2=A0=C2=A0 }
> +
> +=C2=A0=C2=A0=C2=A0 return 0;
> +}
> +
> +static int mag3110_active(struct mag3110_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 return i2c_smbus_write_byte_data(data->client, MAG311=
0_CTRL_REG1,
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1);
This little wrapper doesn't add anything - if anything it makes
it a little harder to review as the reviewer has to come find it to
see what the call is doing.
Just put the i2c write inline where you are currently calling this
function.
> +}
> +
> +/* returns >0 if active, 0 if in standby and <0 on error */
> +static int mag3110_is_active(struct mag3110_data *data)
> +{
> +=C2=A0=C2=A0=C2=A0 int reg;
> +
> +=C2=A0=C2=A0=C2=A0 reg =3D i2c_smbus_read_byte_data(data->client, MAG311=
0_CTRL_REG1);
> +=C2=A0=C2=A0=C2=A0 if (reg < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return reg;
> +
> +=C2=A0=C2=A0=C2=A0 return reg & MAG3110_CTRL_AC;
> +}
> +
> +static int mag3110_change_config(struct mag3110_data *data, u8 reg, u8 v=
al)
> +{
> +=C2=A0=C2=A0=C2=A0 int ret;
> +=C2=A0=C2=A0=C2=A0 int is_active;
> +
> +=C2=A0=C2=A0=C2=A0 mutex_lock(&data->lock);
> +
> +=C2=A0=C2=A0=C2=A0 is_active =3D mag3110_is_active(data);
> +=C2=A0=C2=A0=C2=A0 if (is_active < 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D is_active;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto fail;
> +=C2=A0=C2=A0=C2=A0 }
> +
> +=C2=A0=C2=A0=C2=A0 /* config can only be changed when in standby */
> +=C2=A0=C2=A0=C2=A0 if (is_active > 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D mag3110_standby(data);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto =
fail;
> +=C2=A0=C2=A0=C2=A0 }
> +
> +=C2=A0=C2=A0=C2=A0 /* After coming out of active we must wait for the pa=
rt to
> transition to STBY
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 This can take up to 1 /ODR to occur=
*/
Wrong comment style.
> +=C2=A0=C2=A0=C2=A0 ret =3D mag3110_wait_standby(data);
> +=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto fail;
> +
> +=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_data(data->client, reg, =
val);
> +=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto fail;
> +
> +=C2=A0=C2=A0=C2=A0 if (is_active > 0) {
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D mag3110_active(data);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto =
fail;
> +=C2=A0=C2=A0=C2=A0 }
> +
> +=C2=A0=C2=A0=C2=A0 ret =3D 0;
> +fail:
> +=C2=A0=C2=A0=C2=A0 mutex_unlock(&data->lock);
> +
> +=C2=A0=C2=A0=C2=A0 return ret;
> +}
> +
> =C2=A0static int mag3110_read_raw(struct iio_dev *indio_dev,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 struct iio_chan_spec const *chan,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 int *val, int *val2, long mask)
> @@ -235,11 +354,13 @@ static int mag3110_write_raw(struct iio_dev
> *indio_dev,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
ret =3D -EINVAL;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
break;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> -
Clean out any unrelated white space changes.
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 &=3D ~MAG3110=
_CTRL_DR_MASK;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 &=3D ~MAG3110=
_CTRL_DR_MASK & ~MAG3110_CTRL_AC;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 |=3D rat=
e << MAG3110_CTRL_DR_SHIFT;
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_=
data(data->client,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MAG31=
10_CTRL_REG1, data->ctrl_reg1);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->sleep_val =3D mag3110_c=
alculate_sleep(data);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (data->sleep_val < 40)
Why 40?
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data-=
>ctrl_reg1 |=3D MAG3110_CTRL_AC;
> +
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D mag3110_change_config=
(data, MAG3110_CTRL_REG1,
> data->ctrl_reg1);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 break;
> =C2=A0=C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_CALIBBIAS:
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (val < -10000 || val =
> 10000) {
> @@ -337,12 +458,6 @@ static irqreturn_t mag3110_trigger_handler(int irq,
> void *p)
> =C2=A0
> =C2=A0static const unsigned long mag3110_scan_masks[] =3D {0x7, 0xf, 0};
> =C2=A0
> -static int mag3110_standby(struct mag3110_data *data)
> -{
> -=C2=A0=C2=A0=C2=A0 return i2c_smbus_write_byte_data(data->client, MAG311=
0_CTRL_REG1,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 & ~MAG3110_CT=
RL_AC);
> -}
> -
> =C2=A0static int mag3110_probe(struct i2c_client *client,
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 const struct i2c_device_id *id)
> =C2=A0{
> @@ -374,8 +489,11 @@ static int mag3110_probe(struct i2c_client *client,
> =C2=A0=C2=A0=C2=A0=C2=A0 indio_dev->available_scan_masks =3D mag3110_scan=
_masks;
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 =3D MAG3110_CTRL_DR_DEFAULT << M=
AG3110_CTRL_DR_SHIFT;
> -=C2=A0=C2=A0=C2=A0 ret =3D i2c_smbus_write_byte_data(client, MAG3110_CTR=
L_REG1,
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1);
> +=C2=A0=C2=A0=C2=A0 data->sleep_val =3D mag3110_calculate_sleep(data);
> +=C2=A0=C2=A0=C2=A0 if (data->sleep_val < 40)
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data->ctrl_reg1 |=3D MAG3110_=
CTRL_AC;
> +
> +=C2=A0=C2=A0=C2=A0 ret =3D mag3110_change_config(data, MAG3110_CTRL_REG1=
, data->ctrl_reg1);
> =C2=A0=C2=A0=C2=A0=C2=A0 if (ret < 0)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ret;
> =C2=A0
>=20
>=20
> --
> 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=C2=A0 http://vger.kernel.org/majordomo-info.html
>=20
>=20
>=20
> --
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode
2018-05-06 16:45 ` Jonathan Cameron
@ 2018-05-07 2:37 ` Richard Tresidder
0 siblings, 0 replies; 7+ messages in thread
From: Richard Tresidder @ 2018-05-07 2:37 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
On 7/05/2018 12:45 AM, Jonathan Cameron wrote:
> On Mon, 30 Apr 2018 17:53:54 +0800
> Richard Tresidder <rtresidd@electromag.com.au> wrote:
>
>> Hi
>> Again sorry had maintainer wrong...
> If that happens, don't resend, just reply ccing the maintainer.
> Sometimes the maintainer may then ask you to resend. Right now all
> I got was a cryptic mess of patches when they turned up in my email
> client.
Noted, wasn't sure of the procedure.
And sorry about the tab issue, seems Thunderbird even though I told it 'Text only', reformats stuff unless you change the
the paragraph style from 'Body Text' to 'Preformat' which I wasn't aware of. :(
>
> Note, when a patch is applied all the text above the --- goes into
> the commit log for ever more. Hence any side comments like this should
> be below the --- cut line. That is also where version change logs etc
> belong.
Thanks, I'll limit comments above the first --- cut line and stick general rambling comments in the change log area.
> Also this has nothing to do with your other patch (at least they should
> not be in the same series.) If you didn't want to put them in a single
> series then it should have numbered patches and a cover letter.
> As it is, we just have the two different sets overlapping because
> you sent the second one as a reply.
Yep sorry was just trying to inform that some of the code additions were sourced from the other driver (prior work), probably not relevant.
>
>> This patch adds the ability to run the Mag3110 in continuous mode to
>> speed up the sampling rate.
>> Depending on the sampling rate requested the device can be put in or out
>> of continuous mode automatically.
>> Shifting out of continuous mode requires a potential 1 / ODR wait which
>> is also implemented
>> This part is largely based on the mma8542 driver implementation.
>>
>> Also modified the sleep method when data is not ready to allow for
>> sampling > 50sps to work.
>> This is similar to my other recent patch regarding the mma8452 driver.
>>
>> Have tested upto 80sps using hr timer and iio buffer
>>
>> Signed-off-by: Richard Tresidder <rtresidd@electromag.com.au>
> Again, I think the fundamental change is good, but as you suggested
> in the other patch, you should be sticking to just that change
> not a mixture of 'tidy' ups and the real change.
Ok wasn't sure re the tidy up stuff, as about half the patch sets I read through did a mix.
I'll remove all the white space changes and resubmit as v2 patch
Will look at logs and cc recent reviewers and submitters.
Will then do a separate cleanup patch.
Thanks
Richard
>
> Thanks,
>
> Jonathan
>> ---
>> diff --git a/drivers/iio/magnetometer/mag3110.c
>> b/drivers/iio/magnetometer/mag3110.c
>> index b34ace7..7cdd185 100644
>> --- a/drivers/iio/magnetometer/mag3110.c
>> +++ b/drivers/iio/magnetometer/mag3110.c
>> @@ -26,6 +26,7 @@
>> #define MAG3110_OUT_Y 0x03
>> #define MAG3110_OUT_Z 0x05
>> #define MAG3110_WHO_AM_I 0x07
>> +#define MAG3110_SYSMOD 0x08
>> #define MAG3110_OFF_X 0x09 /* MSB first */
>> #define MAG3110_OFF_Y 0x0b
>> #define MAG3110_OFF_Z 0x0d
>> @@ -39,6 +40,8 @@
>> #define MAG3110_CTRL_DR_SHIFT 5
>> #define MAG3110_CTRL_DR_DEFAULT 0
>>
>> +#define MAG3110_SYSMOD_MODE_MASK (BIT(1) | BIT(0))
> GENMASK not combination of two individual bits. That would
> have been fine if they had independent uses but they don't so
> this needs fixing.
>
>> +
>> #define MAG3110_CTRL_TM BIT(1) /* trigger single measurement */
>> #define MAG3110_CTRL_AC BIT(0) /* continuous measurements */
>>
>> @@ -52,26 +55,35 @@ struct mag3110_data {
>> struct i2c_client *client;
>> struct mutex lock;
>> u8 ctrl_reg1;
>> + int sleep_val;
>> };
>>
>> static int mag3110_request(struct mag3110_data *data)
>> {
>> int ret, tries = 150;
>>
>> - /* trigger measurement */
>> - ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>> - data->ctrl_reg1 | MAG3110_CTRL_TM);
>> - if (ret < 0)
>> - return ret;
>> + if ((data->ctrl_reg1 & MAG3110_CTRL_AC) == 0) {
>> + /* trigger measurement */
>> + ret = i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>> + data->ctrl_reg1 | MAG3110_CTRL_TM);
>> + if (ret < 0)
>> + return ret;
>> + }
>>
>> while (tries-- > 0) {
>> ret = i2c_smbus_read_byte_data(data->client, MAG3110_STATUS);
>> - if (ret < 0)
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "i2c error\n");
> Don't introduce new error messages into existing code paths. Definitely
> don't do it in a patch making more substantial changes elsewhere.
> It just distracts from the important stuff.
>
>
>> return ret;
>> + }
>> /* wait for data ready */
>> if ((ret & MAG3110_STATUS_DRDY) == MAG3110_STATUS_DRDY)
>> break;
>> - msleep(20);
>> +
>> + if (data->sleep_val <= 20)
>> + usleep_range(data->sleep_val * 250, data->sleep_val * 500);
>> + else
>> + msleep(20);
>> }
>>
>> if (tries < 0) {
>> @@ -100,7 +112,7 @@ static int mag3110_read(struct mag3110_data *data,
>> __be16 buf[3])
>> }
>>
>> static ssize_t mag3110_show_int_plus_micros(char *buf,
>> - const int (*vals)[2], int n)
>> + const int (*vals)[2], int n)
>> {
>> size_t len = 0;
>>
>> @@ -115,7 +127,7 @@ static ssize_t mag3110_show_int_plus_micros(char *buf,
>> }
>>
>> static int mag3110_get_int_plus_micros_index(const int (*vals)[2], int n,
>> - int val, int val2)
>> + int val, int val2)
>> {
>> while (n-- > 0)
>> if (val == vals[n][0] && val2 == vals[n][1])
>> @@ -130,7 +142,8 @@ static int mag3110_get_int_plus_micros_index(const
>> int (*vals)[2], int n,
>> };
>>
>> static ssize_t mag3110_show_samp_freq_avail(struct device *dev,
>> - struct device_attribute *attr, char *buf)
>> + struct device_attribute *attr,
>> + char *buf)
> Not in this patch same for any more of these above this point.
>
>> {
>> return mag3110_show_int_plus_micros(buf, mag3110_samp_freq, 8);
>> }
>> @@ -138,12 +151,118 @@ static ssize_t
>> mag3110_show_samp_freq_avail(struct device *dev,
>> static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mag3110_show_samp_freq_avail);
>>
>> static int mag3110_get_samp_freq_index(struct mag3110_data *data,
>> - int val, int val2)
>> + int val, int val2)
> This sort of tidy up doesn't not belong in any patch making a functional
> change.
>
>> {
>> return mag3110_get_int_plus_micros_index(mag3110_samp_freq, 8, val,
>> val2);
>> }
>>
>> +static int mag3110_calculate_sleep(struct mag3110_data *data)
>> +{
>> + int ret, i = data->ctrl_reg1 >> MAG3110_CTRL_DR_SHIFT;
>> +
>> + if(mag3110_samp_freq[i][0] > 0)
>> + ret = 1000 / mag3110_samp_freq[i][0];
>> + else
>> + ret = 1000;
>> +
>> + return ret == 0 ? 1 : ret;
>> +}
>> +
>> +static int mag3110_standby(struct mag3110_data *data)
>> +{
>> + return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>> + data->ctrl_reg1 & ~MAG3110_CTRL_AC);
>> +}
>> +
>> +static int mag3110_wait_standby(struct mag3110_data *data)
>> +{
>> + int ret, tries = 30;
>> +
>> + /* Takes up to 1/ODR to come out of active mode into stby
>> + Longest expected period is 12.5seconds. We'll sleep for 500ms
>> between checks*/
> Please run checkpatch.pl over your patches. I expect it will complain
> about this comment. Coding style is very rigorous in the kernel and
> this isn't how we do multiline comments.
>
>> + while (tries-- > 0) {
>> + ret = i2c_smbus_read_byte_data(data->client, MAG3110_SYSMOD);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "i2c error\n");
>> + return ret;
>> + }
>> + /* wait for standby */
>> + if ((ret & MAG3110_SYSMOD_MODE_MASK) == 0)
>> + break;
>> +
>> + msleep_interruptible(500);
>> + }
>> +
>> + if (tries < 0) {
>> + dev_err(&data->client->dev, "device not entering standby mode\n");
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mag3110_active(struct mag3110_data *data)
>> +{
>> + return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>> + data->ctrl_reg1);
> This little wrapper doesn't add anything - if anything it makes
> it a little harder to review as the reviewer has to come find it to
> see what the call is doing.
>
> Just put the i2c write inline where you are currently calling this
> function.
>
>> +}
>> +
>> +/* returns >0 if active, 0 if in standby and <0 on error */
>> +static int mag3110_is_active(struct mag3110_data *data)
>> +{
>> + int reg;
>> +
>> + reg = i2c_smbus_read_byte_data(data->client, MAG3110_CTRL_REG1);
>> + if (reg < 0)
>> + return reg;
>> +
>> + return reg & MAG3110_CTRL_AC;
>> +}
>> +
>> +static int mag3110_change_config(struct mag3110_data *data, u8 reg, u8 val)
>> +{
>> + int ret;
>> + int is_active;
>> +
>> + mutex_lock(&data->lock);
>> +
>> + is_active = mag3110_is_active(data);
>> + if (is_active < 0) {
>> + ret = is_active;
>> + goto fail;
>> + }
>> +
>> + /* config can only be changed when in standby */
>> + if (is_active > 0) {
>> + ret = mag3110_standby(data);
>> + if (ret < 0)
>> + goto fail;
>> + }
>> +
>> + /* After coming out of active we must wait for the part to
>> transition to STBY
>> + This can take up to 1 /ODR to occur */
> Wrong comment style.
>
>> + ret = mag3110_wait_standby(data);
>> + if (ret < 0)
>> + goto fail;
>> +
>> + ret = i2c_smbus_write_byte_data(data->client, reg, val);
>> + if (ret < 0)
>> + goto fail;
>> +
>> + if (is_active > 0) {
>> + ret = mag3110_active(data);
>> + if (ret < 0)
>> + goto fail;
>> + }
>> +
>> + ret = 0;
>> +fail:
>> + mutex_unlock(&data->lock);
>> +
>> + return ret;
>> +}
>> +
>> static int mag3110_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val, int *val2, long mask)
>> @@ -235,11 +354,13 @@ static int mag3110_write_raw(struct iio_dev
>> *indio_dev,
>> ret = -EINVAL;
>> break;
>> }
>> -
> Clean out any unrelated white space changes.
>
>> - data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK;
>> + data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK & ~MAG3110_CTRL_AC;
>> data->ctrl_reg1 |= rate << MAG3110_CTRL_DR_SHIFT;
>> - ret = i2c_smbus_write_byte_data(data->client,
>> - MAG3110_CTRL_REG1, data->ctrl_reg1);
>> + data->sleep_val = mag3110_calculate_sleep(data);
>> + if (data->sleep_val < 40)
> Why 40?
>
>> + data->ctrl_reg1 |= MAG3110_CTRL_AC;
>> +
>> + ret = mag3110_change_config(data, MAG3110_CTRL_REG1,
>> data->ctrl_reg1);
>> break;
>> case IIO_CHAN_INFO_CALIBBIAS:
>> if (val < -10000 || val > 10000) {
>> @@ -337,12 +458,6 @@ static irqreturn_t mag3110_trigger_handler(int irq,
>> void *p)
>>
>> static const unsigned long mag3110_scan_masks[] = {0x7, 0xf, 0};
>>
>> -static int mag3110_standby(struct mag3110_data *data)
>> -{
>> - return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>> - data->ctrl_reg1 & ~MAG3110_CTRL_AC);
>> -}
>> -
>> static int mag3110_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -374,8 +489,11 @@ static int mag3110_probe(struct i2c_client *client,
>> indio_dev->available_scan_masks = mag3110_scan_masks;
>>
>> data->ctrl_reg1 = MAG3110_CTRL_DR_DEFAULT << MAG3110_CTRL_DR_SHIFT;
>> - ret = i2c_smbus_write_byte_data(client, MAG3110_CTRL_REG1,
>> - data->ctrl_reg1);
>> + data->sleep_val = mag3110_calculate_sleep(data);
>> + if (data->sleep_val < 40)
>> + data->ctrl_reg1 |= MAG3110_CTRL_AC;
>> +
>> + ret = mag3110_change_config(data, MAG3110_CTRL_REG1, data->ctrl_reg1);
>> if (ret < 0)
>> return ret;
>>
>>
>>
>> --
>> 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
>>
>>
>>
>> --
>> 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
> --
> 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
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-07 2:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-30 4:56 [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
2018-04-30 6:23 ` [PATCH] iio: magnetometer: mag3110: Add ability to run in continuous mode Richard Tresidder
2018-04-30 9:53 ` Richard Tresidder
2018-05-06 16:45 ` Jonathan Cameron
2018-05-07 2:37 ` Richard Tresidder
2018-04-30 9:50 ` [PATCH] iio: accell: mma8452: Reduce sleep time when data not ready Richard Tresidder
2018-05-06 16:29 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.