All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] stk8312 fixes and cleanup
@ 2015-07-27 22:49 Hartmut Knaack
  2015-07-27 22:49 ` [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency Hartmut Knaack
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Fixing some issues I spotted during review.

Hartmut Knaack (7):
  iio:accel:stk8312: add triggered buffer dependency
  iio:accel:stk8312: check for invalid value
  iio:accel:stk8312: improve error handling
  iio:accel:stk8312: rework macro definitions
  iio:accel:stk8312: use appropriate variable types
  iio:accel:stk8312: code style cleanup
  iio:accel:stk8312: drop local buffer

 drivers/iio/accel/Kconfig   |  2 ++
 drivers/iio/accel/stk8312.c | 85 +++++++++++++++++++++++++--------------------
 2 files changed, 49 insertions(+), 38 deletions(-)

-- 
2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:27   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 2/7] iio:accel:stk8312: check for invalid value Hartmut Knaack
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Add the still missing dependencies for triggered buffer support.

Fixes: 	95c12bba51c37 ("iio: accel: Add buffer mode for Sensortek STK8312")

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 00e7bcbdbe24..c99d8e242218 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -140,6 +140,8 @@ config MMA9553
 config STK8312
 	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
 	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to get support for the Sensortek STK8312 3-axis
 	  accelerometer.
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/7] iio:accel:stk8312: check for invalid value
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
  2015-07-27 22:49 ` [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:27   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 3/7] iio:accel:stk8312: improve error handling Hartmut Knaack
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
values read from OTP register 0x70 into AFECTRL register.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index c2bd1444d6da..6592be8e6377 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		goto exit_err;
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
+	if (ret == 0)
+		ret = -EINVAL;
 	if (ret < 0)
 		goto exit_err;
 
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/7] iio:accel:stk8312: improve error handling
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
  2015-07-27 22:49 ` [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency Hartmut Knaack
  2015-07-27 22:49 ` [PATCH 2/7] iio:accel:stk8312: check for invalid value Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:38   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 4/7] iio:accel:stk8312: rework macro definitions Hartmut Knaack
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Improve error handling in the following ways:
  - set return value on error condition to an appropriate error code
  - return error code immediately in case of an error (slightly changes
    code structure)
  - pass up real error code
  - add missing error handling
  - return 0 when error have been caught already

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 6592be8e6377..50a102ca44d3 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		count--;
 	} while (!(ret & 0x80) && count > 0);
 
-	if (count == 0)
+	if (count == 0) {
+		ret = -ETIMEDOUT;
 		goto exit_err;
+	}
 
 	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
 	if (ret == 0)
@@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		goto exit_err;
 	msleep(150);
 
-	return ret;
+	return 0;
 
 exit_err:
 	dev_err(&client->dev, "failed to initialize sensor\n");
@@ -205,8 +207,10 @@ static int stk8312_set_interrupts(struct stk8312_data *data, u8 int_mask)
 		return ret;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU, int_mask);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "failed to set interrupts\n");
+		return ret;
+	}
 
 	return stk8312_set_mode(data, mode);
 }
@@ -230,7 +234,7 @@ static int stk8312_data_rdy_trigger_set_state(struct iio_trigger *trig,
 
 	data->dready_trigger_on = state;
 
-	return ret;
+	return 0;
 }
 
 static const struct iio_trigger_ops stk8312_trigger_ops = {
@@ -263,10 +267,12 @@ static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
 	masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR, masked_reg);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "failed to set sampling rate\n");
-	else
-		data->sample_rate_idx = rate;
+		return ret;
+	}
+
+	data->sample_rate_idx = rate;
 
 	return stk8312_set_mode(data, mode);
 }
@@ -299,10 +305,12 @@ static int stk8312_set_range(struct stk8312_data *data, u8 range)
 	masked_reg |= range << STK8312_RNG_SHIFT;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH, masked_reg);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&client->dev, "failed to change sensor range\n");
-	else
-		data->range = range;
+		return ret;
+	}
+
+	data->range = range;
 
 	return stk8312_set_mode(data, mode);
 }
@@ -337,18 +345,21 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
 		ret = stk8312_set_mode(data, data->mode | STK8312_MODE_ACTIVE);
 		if (ret < 0) {
 			mutex_unlock(&data->lock);
-			return -EINVAL;
+			return ret;
 		}
 		ret = stk8312_read_accel(data, chan->address);
 		if (ret < 0) {
 			stk8312_set_mode(data,
 					 data->mode & (~STK8312_MODE_ACTIVE));
 			mutex_unlock(&data->lock);
-			return -EINVAL;
+			return ret;
 		}
 		*val = sign_extend32(ret, 7);
-		stk8312_set_mode(data, data->mode & (~STK8312_MODE_ACTIVE));
+		ret = stk8312_set_mode(data,
+				       data->mode & (~STK8312_MODE_ACTIVE));
 		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = stk8312_scale_table[data->range - 1][0];
@@ -608,7 +619,7 @@ static int stk8312_probe(struct i2c_client *client,
 		goto err_buffer_cleanup;
 	}
 
-	return ret;
+	return 0;
 
 err_buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/7] iio:accel:stk8312: rework macro definitions
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
                   ` (2 preceding siblings ...)
  2015-07-27 22:49 ` [PATCH 3/7] iio:accel:stk8312: improve error handling Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:28   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 5/7] iio:accel:stk8312: use appropriate variable types Hartmut Knaack
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Make use of BIT to describe register bits, GENMASK for consecutive
bitmasks, rename and sort existing definitions, replace magic value with
an expressive definition, drop an unused definition.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 50a102ca44d3..8783f0bf7c53 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -37,16 +37,16 @@
 #define STK8312_REG_OTPDATA		0x3E
 #define STK8312_REG_OTPCTRL		0x3F
 
-#define STK8312_MODE_ACTIVE		0x01
+#define STK8312_MODE_ACTIVE		BIT(0)
 #define STK8312_MODE_STANDBY		0x00
-#define STK8312_DREADY_BIT		0x10
-#define STK8312_INT_MODE		0xC0
-#define STK8312_RNG_MASK		0xC0
-#define STK8312_SR_MASK			0x07
-#define STK8312_SR_400HZ_IDX		0
+#define STK8312_MODE_INT_AH_PP		0xC0	/* active-high, push-pull */
+#define STK8312_DREADY_BIT		BIT(4)
+#define STK8312_RNG_6G			1
 #define STK8312_RNG_SHIFT		6
-#define STK8312_READ_RETRIES		16
-#define STK8312_ALL_CHANNEL_MASK	7
+#define STK8312_RNG_MASK		GENMASK(7, 6)
+#define STK8312_SR_MASK			GENMASK(2, 0)
+#define STK8312_SR_400HZ_IDX		0
+#define STK8312_ALL_CHANNEL_MASK	GENMASK(2, 0)
 #define STK8312_ALL_CHANNEL_SIZE	3
 
 #define STK8312_DRIVER_NAME		"stk8312"
@@ -144,7 +144,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 		if (ret < 0)
 			goto exit_err;
 		count--;
-	} while (!(ret & 0x80) && count > 0);
+	} while (!(ret & BIT(7)) && count > 0);
 
 	if (count == 0) {
 		ret = -ETIMEDOUT;
@@ -560,11 +560,12 @@ static int stk8312_probe(struct i2c_client *client,
 		return ret;
 	}
 	data->sample_rate_idx = STK8312_SR_400HZ_IDX;
-	ret = stk8312_set_range(data, 1);
+	ret = stk8312_set_range(data, STK8312_RNG_6G);
 	if (ret < 0)
 		return ret;
 
-	ret = stk8312_set_mode(data, STK8312_INT_MODE | STK8312_MODE_ACTIVE);
+	ret = stk8312_set_mode(data,
+			       STK8312_MODE_INT_AH_PP | STK8312_MODE_ACTIVE);
 	if (ret < 0)
 		return ret;
 
@@ -685,7 +686,7 @@ MODULE_DEVICE_TABLE(acpi, stk8312_acpi_id);
 
 static struct i2c_driver stk8312_driver = {
 	.driver = {
-		.name = "stk8312",
+		.name = STK8312_DRIVER_NAME,
 		.pm = STK8312_PM_OPS,
 		.acpi_match_table = ACPI_PTR(stk8312_acpi_id),
 	},
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/7] iio:accel:stk8312: use appropriate variable types
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
                   ` (3 preceding siblings ...)
  2015-07-27 22:49 ` [PATCH 4/7] iio:accel:stk8312: rework macro definitions Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:28   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 6/7] iio:accel:stk8312: code style cleanup Hartmut Knaack
  2015-07-27 22:49 ` [PATCH 7/7] iio:accel:stk8312: drop local buffer Hartmut Knaack
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Adapt some variable types to reduce unnecessary casting.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 8783f0bf7c53..e3624e2b3ec4 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -69,8 +69,8 @@ static const int stk8312_scale_table[][2] = {
 };
 
 static const struct {
-	u16 val;
-	u32 val2;
+	int val;
+	int val2;
 } stk8312_samp_freq_table[] = {
 	{400, 0}, {200, 0}, {100, 0}, {50, 0}, {25, 0},
 	{12, 500000}, {6, 250000}, {3, 125000}
@@ -103,7 +103,7 @@ static const struct iio_chan_spec stk8312_channels[] = {
 struct stk8312_data {
 	struct i2c_client *client;
 	struct mutex lock;
-	int range;
+	u8 range;
 	u8 sample_rate_idx;
 	u8 mode;
 	struct iio_trigger *dready_trig;
@@ -242,7 +242,7 @@ static const struct iio_trigger_ops stk8312_trigger_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
+static int stk8312_set_sample_rate(struct stk8312_data *data, u8 rate)
 {
 	int ret;
 	u8 masked_reg;
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/7] iio:accel:stk8312: code style cleanup
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
                   ` (4 preceding siblings ...)
  2015-07-27 22:49 ` [PATCH 5/7] iio:accel:stk8312: use appropriate variable types Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:45   ` Breana, Tiberiu A
  2015-07-27 22:49 ` [PATCH 7/7] iio:accel:stk8312: drop local buffer Hartmut Knaack
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Adjust some indentation issues to make checkpatch.pl happy in strict mode.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index e3624e2b3ec4..b059a6cabfbb 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 	if (ret < 0)
 		goto exit_err;
 
-	ret = i2c_smbus_write_byte_data(data->client,
-			STK8312_REG_AFECTRL, ret);
+	ret = i2c_smbus_write_byte_data(data->client, STK8312_REG_AFECTRL, ret);
 	if (ret < 0)
 		goto exit_err;
 	msleep(150);
@@ -453,7 +452,7 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 		data->buffer[2] = buffer[2];
 	} else {
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
-			   indio_dev->masklength) {
+				 indio_dev->masklength) {
 			ret = stk8312_read_accel(data, bit);
 			if (ret < 0) {
 				mutex_unlock(&data->lock);
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 7/7] iio:accel:stk8312: drop local buffer
  2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
                   ` (5 preceding siblings ...)
  2015-07-27 22:49 ` [PATCH 6/7] iio:accel:stk8312: code style cleanup Hartmut Knaack
@ 2015-07-27 22:49 ` Hartmut Knaack
  2015-07-28 13:29   ` Breana, Tiberiu A
  6 siblings, 1 reply; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-27 22:49 UTC (permalink / raw)
  To: linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald, Tiberiu Breana

Drop the local buffer in stk8312_trigger_handler() and use data-buffer
instead for bulk reads.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/accel/stk8312.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index b059a6cabfbb..a6b2bda7a63c 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -430,7 +430,6 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct stk8312_data *data = iio_priv(indio_dev);
 	int bit, ret, i = 0;
-	u8 buffer[STK8312_ALL_CHANNEL_SIZE];
 
 	mutex_lock(&data->lock);
 	/*
@@ -441,15 +440,12 @@ static irqreturn_t stk8312_trigger_handler(int irq, void *p)
 		ret = i2c_smbus_read_i2c_block_data(data->client,
 						    STK8312_REG_XOUT,
 						    STK8312_ALL_CHANNEL_SIZE,
-						    buffer);
+						    data->buffer);
 		if (ret < STK8312_ALL_CHANNEL_SIZE) {
 			dev_err(&data->client->dev, "register read failed\n");
 			mutex_unlock(&data->lock);
 			goto err;
 		}
-		data->buffer[0] = buffer[0];
-		data->buffer[1] = buffer[1];
-		data->buffer[2] = buffer[2];
 	} else {
 		for_each_set_bit(bit, indio_dev->active_scan_mask,
 				 indio_dev->masklength) {
-- 
2.4.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* RE: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency
  2015-07-27 22:49 ` [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency Hartmut Knaack
@ 2015-07-28 13:27   ` Breana, Tiberiu A
  2015-08-02 18:13     ` Jonathan Cameron
  0 siblings, 1 reply; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:27 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency
> 
> Add the still missing dependencies for triggered buffer support.
> 
> Fixes: 	95c12bba51c37 ("iio: accel: Add buffer mode for Sensortek STK8312")
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

> ---
>  drivers/iio/accel/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index
> 00e7bcbdbe24..c99d8e242218 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -140,6 +140,8 @@ config MMA9553
>  config STK8312
>  	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
>  	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to get support for the Sensortek STK8312 3-axis
>  	  accelerometer.
> --
> 2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH 2/7] iio:accel:stk8312: check for invalid value
  2015-07-27 22:49 ` [PATCH 2/7] iio:accel:stk8312: check for invalid value Hartmut Knaack
@ 2015-07-28 13:27   ` Breana, Tiberiu A
  0 siblings, 0 replies; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:27 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
> owner@vger.kernel.org] On Behalf Of Hartmut Knaack
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 2/7] iio:accel:stk8312: check for invalid value
> 
> Revision 1.2 of the datasheet recommends on page 22 to only write non-zero
> values read from OTP register 0x70 into AFECTRL register.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

> ---
>  drivers/iio/accel/stk8312.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> c2bd1444d6da..6592be8e6377 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -150,6 +150,8 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  		goto exit_err;
> 
>  	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
> +	if (ret == 0)
> +		ret = -EINVAL;
>  	if (ret < 0)
>  		goto exit_err;
> 
> --
> 2.4.6
> 
> --
> 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] 19+ messages in thread

* RE: [PATCH 4/7] iio:accel:stk8312: rework macro definitions
  2015-07-27 22:49 ` [PATCH 4/7] iio:accel:stk8312: rework macro definitions Hartmut Knaack
@ 2015-07-28 13:28   ` Breana, Tiberiu A
  2015-08-02 18:17     ` Jonathan Cameron
  0 siblings, 1 reply; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:28 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 4/7] iio:accel:stk8312: rework macro definitions
> 
> Make use of BIT to describe register bits, GENMASK for consecutive
> bitmasks, rename and sort existing definitions, replace magic value with an
> expressive definition, drop an unused definition.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

> ---
>  drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> 50a102ca44d3..8783f0bf7c53 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -37,16 +37,16 @@
>  #define STK8312_REG_OTPDATA		0x3E
>  #define STK8312_REG_OTPCTRL		0x3F
> 
> -#define STK8312_MODE_ACTIVE		0x01
> +#define STK8312_MODE_ACTIVE		BIT(0)
>  #define STK8312_MODE_STANDBY		0x00
> -#define STK8312_DREADY_BIT		0x10
> -#define STK8312_INT_MODE		0xC0
> -#define STK8312_RNG_MASK		0xC0
> -#define STK8312_SR_MASK			0x07
> -#define STK8312_SR_400HZ_IDX		0
> +#define STK8312_MODE_INT_AH_PP		0xC0	/* active-high, push-
> pull */
> +#define STK8312_DREADY_BIT		BIT(4)
> +#define STK8312_RNG_6G			1
>  #define STK8312_RNG_SHIFT		6
> -#define STK8312_READ_RETRIES		16
> -#define STK8312_ALL_CHANNEL_MASK	7
> +#define STK8312_RNG_MASK		GENMASK(7, 6)
> +#define STK8312_SR_MASK			GENMASK(2, 0)
> +#define STK8312_SR_400HZ_IDX		0
> +#define STK8312_ALL_CHANNEL_MASK	GENMASK(2, 0)
>  #define STK8312_ALL_CHANNEL_SIZE	3
> 
>  #define STK8312_DRIVER_NAME		"stk8312"
> @@ -144,7 +144,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  		if (ret < 0)
>  			goto exit_err;
>  		count--;
> -	} while (!(ret & 0x80) && count > 0);
> +	} while (!(ret & BIT(7)) && count > 0);
> 
>  	if (count == 0) {
>  		ret = -ETIMEDOUT;
> @@ -560,11 +560,12 @@ static int stk8312_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  	data->sample_rate_idx = STK8312_SR_400HZ_IDX;
> -	ret = stk8312_set_range(data, 1);
> +	ret = stk8312_set_range(data, STK8312_RNG_6G);
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = stk8312_set_mode(data, STK8312_INT_MODE |
> STK8312_MODE_ACTIVE);
> +	ret = stk8312_set_mode(data,
> +			       STK8312_MODE_INT_AH_PP |
> STK8312_MODE_ACTIVE);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -685,7 +686,7 @@ MODULE_DEVICE_TABLE(acpi, stk8312_acpi_id);
> 
>  static struct i2c_driver stk8312_driver = {
>  	.driver = {
> -		.name = "stk8312",
> +		.name = STK8312_DRIVER_NAME,
>  		.pm = STK8312_PM_OPS,
>  		.acpi_match_table = ACPI_PTR(stk8312_acpi_id),
>  	},
> --
> 2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH 5/7] iio:accel:stk8312: use appropriate variable types
  2015-07-27 22:49 ` [PATCH 5/7] iio:accel:stk8312: use appropriate variable types Hartmut Knaack
@ 2015-07-28 13:28   ` Breana, Tiberiu A
  0 siblings, 0 replies; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:28 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 5/7] iio:accel:stk8312: use appropriate variable types
> 
> Adapt some variable types to reduce unnecessary casting.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

> ---
>  drivers/iio/accel/stk8312.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> 8783f0bf7c53..e3624e2b3ec4 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -69,8 +69,8 @@ static const int stk8312_scale_table[][2] = {  };
> 
>  static const struct {
> -	u16 val;
> -	u32 val2;
> +	int val;
> +	int val2;
>  } stk8312_samp_freq_table[] = {
>  	{400, 0}, {200, 0}, {100, 0}, {50, 0}, {25, 0},
>  	{12, 500000}, {6, 250000}, {3, 125000} @@ -103,7 +103,7 @@ static
> const struct iio_chan_spec stk8312_channels[] = {  struct stk8312_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> -	int range;
> +	u8 range;
>  	u8 sample_rate_idx;
>  	u8 mode;
>  	struct iio_trigger *dready_trig;
> @@ -242,7 +242,7 @@ static const struct iio_trigger_ops stk8312_trigger_ops
> = {
>  	.owner = THIS_MODULE,
>  };
> 
> -static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
> +static int stk8312_set_sample_rate(struct stk8312_data *data, u8 rate)
>  {
>  	int ret;
>  	u8 masked_reg;
> --
> 2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH 7/7] iio:accel:stk8312: drop local buffer
  2015-07-27 22:49 ` [PATCH 7/7] iio:accel:stk8312: drop local buffer Hartmut Knaack
@ 2015-07-28 13:29   ` Breana, Tiberiu A
  0 siblings, 0 replies; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:29 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-
> owner@vger.kernel.org] On Behalf Of Hartmut Knaack
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 7/7] iio:accel:stk8312: drop local buffer
> 
> Drop the local buffer in stk8312_trigger_handler() and use data-buffer
> instead for bulk reads.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

> ---
>  drivers/iio/accel/stk8312.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> b059a6cabfbb..a6b2bda7a63c 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -430,7 +430,6 @@ static irqreturn_t stk8312_trigger_handler(int irq, void
> *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct stk8312_data *data = iio_priv(indio_dev);
>  	int bit, ret, i = 0;
> -	u8 buffer[STK8312_ALL_CHANNEL_SIZE];
> 
>  	mutex_lock(&data->lock);
>  	/*
> @@ -441,15 +440,12 @@ static irqreturn_t stk8312_trigger_handler(int irq,
> void *p)
>  		ret = i2c_smbus_read_i2c_block_data(data->client,
>  						    STK8312_REG_XOUT,
> 
> STK8312_ALL_CHANNEL_SIZE,
> -						    buffer);
> +						    data->buffer);
>  		if (ret < STK8312_ALL_CHANNEL_SIZE) {
>  			dev_err(&data->client->dev, "register read
> failed\n");
>  			mutex_unlock(&data->lock);
>  			goto err;
>  		}
> -		data->buffer[0] = buffer[0];
> -		data->buffer[1] = buffer[1];
> -		data->buffer[2] = buffer[2];
>  	} else {
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
>  				 indio_dev->masklength) {
> --
> 2.4.6
> 
> --
> 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] 19+ messages in thread

* RE: [PATCH 3/7] iio:accel:stk8312: improve error handling
  2015-07-27 22:49 ` [PATCH 3/7] iio:accel:stk8312: improve error handling Hartmut Knaack
@ 2015-07-28 13:38   ` Breana, Tiberiu A
  2015-07-28 23:08     ` Hartmut Knaack
  0 siblings, 1 reply; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:38 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 3/7] iio:accel:stk8312: improve error handling
> 
> Improve error handling in the following ways:
>   - set return value on error condition to an appropriate error code
>   - return error code immediately in case of an error (slightly changes
>     code structure)
>   - pass up real error code
>   - add missing error handling
>   - return 0 when error have been caught already
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

Comments inline.

> ---
>  drivers/iio/accel/stk8312.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> 6592be8e6377..50a102ca44d3 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -146,8 +146,10 @@ static int stk8312_otp_init(struct stk8312_data
> *data)
>  		count--;
>  	} while (!(ret & 0x80) && count > 0);
> 
> -	if (count == 0)
> +	if (count == 0) {
> +		ret = -ETIMEDOUT;
>  		goto exit_err;
> +	}
> 
>  	ret = i2c_smbus_read_byte_data(client, STK8312_REG_OTPDATA);
>  	if (ret == 0)
> @@ -161,7 +163,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  		goto exit_err;
>  	msleep(150);
> 
> -	return ret;
> +	return 0;
> 
>  exit_err:
>  	dev_err(&client->dev, "failed to initialize sensor\n"); @@ -205,8
> +207,10 @@ static int stk8312_set_interrupts(struct stk8312_data *data, u8
> int_mask)
>  		return ret;
> 
>  	ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,
> int_mask);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&client->dev, "failed to set interrupts\n");
> +		return ret;
> +	}
> 
>  	return stk8312_set_mode(data, mode);
>  }
> @@ -230,7 +234,7 @@ static int stk8312_data_rdy_trigger_set_state(struct
> iio_trigger *trig,
> 
>  	data->dready_trigger_on = state;
> 
> -	return ret;
> +	return 0;
>  }
> 
>  static const struct iio_trigger_ops stk8312_trigger_ops = { @@ -263,10
> +267,12 @@ static int stk8312_set_sample_rate(struct stk8312_data *data,
> int rate)
>  	masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
> 
>  	ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
> masked_reg);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&client->dev, "failed to set sampling rate\n");
> -	else
> -		data->sample_rate_idx = rate;
> +		return ret;

We can't just return here. Bear in mind that earlier in the function we set the
sensor's operation mode to standby. Return to active mode before returning.
This should be added @258 as well.

> +	}
> +
> +	data->sample_rate_idx = rate;
> 
>  	return stk8312_set_mode(data, mode);
>  }
> @@ -299,10 +305,12 @@ static int stk8312_set_range(struct stk8312_data
> *data, u8 range)
>  	masked_reg |= range << STK8312_RNG_SHIFT;
> 
>  	ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
> masked_reg);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&client->dev, "failed to change sensor range\n");
> -	else
> -		data->range = range;
> +		return ret;
> +	}
> +
> +	data->range = range;
> 
>  	return stk8312_set_mode(data, mode);
>  }
> @@ -337,18 +345,21 @@ static int stk8312_read_raw(struct iio_dev
> *indio_dev,
>  		ret = stk8312_set_mode(data, data->mode |
> STK8312_MODE_ACTIVE);
>  		if (ret < 0) {
>  			mutex_unlock(&data->lock);
> -			return -EINVAL;
> +			return ret;
>  		}
>  		ret = stk8312_read_accel(data, chan->address);
>  		if (ret < 0) {
>  			stk8312_set_mode(data,
>  					 data->mode &
> (~STK8312_MODE_ACTIVE));
>  			mutex_unlock(&data->lock);
> -			return -EINVAL;
> +			return ret;
>  		}
>  		*val = sign_extend32(ret, 7);
> -		stk8312_set_mode(data, data->mode &
> (~STK8312_MODE_ACTIVE));
> +		ret = stk8312_set_mode(data,
> +				       data->mode &
> (~STK8312_MODE_ACTIVE));
>  		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;

I don't agree with this. We've succesfully read the data, we shouldn't return an
error if the sensor couldn't be put back to standby. I suggest leaving it as it is.

>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = stk8312_scale_table[data->range - 1][0]; @@ -608,7
> +619,7 @@ static int stk8312_probe(struct i2c_client *client,
>  		goto err_buffer_cleanup;
>  	}
> 
> -	return ret;
> +	return 0;
> 
>  err_buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
> --
> 2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH 6/7] iio:accel:stk8312: code style cleanup
  2015-07-27 22:49 ` [PATCH 6/7] iio:accel:stk8312: code style cleanup Hartmut Knaack
@ 2015-07-28 13:45   ` Breana, Tiberiu A
  2015-07-28 23:13     ` Hartmut Knaack
  0 siblings, 1 reply; 19+ messages in thread
From: Breana, Tiberiu A @ 2015-07-28 13:45 UTC (permalink / raw)
  To: Hartmut Knaack, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Tuesday, July 28, 2015 1:49 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 6/7] iio:accel:stk8312: code style cleanup
> 
> Adjust some indentation issues to make checkpatch.pl happy in strict mode.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

Comment inline.

> ---
>  drivers/iio/accel/stk8312.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
> e3624e2b3ec4..b059a6cabfbb 100644
> --- a/drivers/iio/accel/stk8312.c
> +++ b/drivers/iio/accel/stk8312.c
> @@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>  	if (ret < 0)
>  		goto exit_err;
> 
> -	ret = i2c_smbus_write_byte_data(data->client,
> -			STK8312_REG_AFECTRL, ret);
> +	ret = i2c_smbus_write_byte_data(data->client,
> STK8312_REG_AFECTRL,
> +ret);

On a single line, this write op would take up 81 chars.
I think it's best to just add 2 tabs to the 2nd line.

>  	if (ret < 0)
>  		goto exit_err;
>  	msleep(150);
> @@ -453,7 +452,7 @@ static irqreturn_t stk8312_trigger_handler(int irq, void
> *p)
>  		data->buffer[2] = buffer[2];
>  	} else {
>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
> -			   indio_dev->masklength) {
> +				 indio_dev->masklength) {
>  			ret = stk8312_read_accel(data, bit);
>  			if (ret < 0) {
>  				mutex_unlock(&data->lock);
> --
> 2.4.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/7] iio:accel:stk8312: improve error handling
  2015-07-28 13:38   ` Breana, Tiberiu A
@ 2015-07-28 23:08     ` Hartmut Knaack
  0 siblings, 0 replies; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-28 23:08 UTC (permalink / raw)
  To: Breana, Tiberiu A, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

Breana, Tiberiu A schrieb am 28.07.2015 um 15:38:
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: Tuesday, July 28, 2015 1:49 AM
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
>> A
>> Subject: [PATCH 3/7] iio:accel:stk8312: improve error handling
>>
>> Improve error handling in the following ways:
>>   - set return value on error condition to an appropriate error code
>>   - return error code immediately in case of an error (slightly changes
>>     code structure)
>>   - pass up real error code
>>   - add missing error handling
>>   - return 0 when error have been caught already
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> Comments inline.
> 

Hi,

<...>
>> +267,12 @@ static int stk8312_set_sample_rate(struct stk8312_data *data,
>> int rate)
>>  	masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
>>
>>  	ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
>> masked_reg);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>  		dev_err(&client->dev, "failed to set sampling rate\n");
>> -	else
>> -		data->sample_rate_idx = rate;
>> +		return ret;
> 
> We can't just return here. Bear in mind that earlier in the function we set the
> sensor's operation mode to standby. Return to active mode before returning.
> This should be added @258 as well.
> 

OK, I will respin with a common error path to put the device back in active mode.

>> +	}
>> +
>> +	data->sample_rate_idx = rate;
>>
>>  	return stk8312_set_mode(data, mode);
>>  }
>> @@ -299,10 +305,12 @@ static int stk8312_set_range(struct stk8312_data
>> *data, u8 range)
>>  	masked_reg |= range << STK8312_RNG_SHIFT;
>>
>>  	ret = i2c_smbus_write_byte_data(client, STK8312_REG_STH,
>> masked_reg);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>  		dev_err(&client->dev, "failed to change sensor range\n");
>> -	else
>> -		data->range = range;
>> +		return ret;
>> +	}
>> +
>> +	data->range = range;
>>
>>  	return stk8312_set_mode(data, mode);
>>  }
>> @@ -337,18 +345,21 @@ static int stk8312_read_raw(struct iio_dev
>> *indio_dev,
>>  		ret = stk8312_set_mode(data, data->mode |
>> STK8312_MODE_ACTIVE);
>>  		if (ret < 0) {
>>  			mutex_unlock(&data->lock);
>> -			return -EINVAL;
>> +			return ret;
>>  		}
>>  		ret = stk8312_read_accel(data, chan->address);
>>  		if (ret < 0) {
>>  			stk8312_set_mode(data,
>>  					 data->mode &
>> (~STK8312_MODE_ACTIVE));
>>  			mutex_unlock(&data->lock);
>> -			return -EINVAL;
>> +			return ret;
>>  		}
>>  		*val = sign_extend32(ret, 7);
>> -		stk8312_set_mode(data, data->mode &
>> (~STK8312_MODE_ACTIVE));
>> +		ret = stk8312_set_mode(data,
>> +				       data->mode &
>> (~STK8312_MODE_ACTIVE));
>>  		mutex_unlock(&data->lock);
>> +		if (ret < 0)
>> +			return ret;
> 
> I don't agree with this. We've succesfully read the data, we shouldn't return an
> error if the sensor couldn't be put back to standby. I suggest leaving it as it is.
> 

Well, encountering an error should be an exceptional case, which should not happen
in normal operation (and thus do no harm). But something goes wrong, then better
report it, even a data sample has been read successfully.
Thanks,

Hartmut

>>  		return IIO_VAL_INT;
>>  	case IIO_CHAN_INFO_SCALE:
>>  		*val = stk8312_scale_table[data->range - 1][0]; @@ -608,7
>> +619,7 @@ static int stk8312_probe(struct i2c_client *client,
>>  		goto err_buffer_cleanup;
>>  	}
>>
>> -	return ret;
>> +	return 0;
>>
>>  err_buffer_cleanup:
>>  	iio_triggered_buffer_cleanup(indio_dev);
>> --
>> 2.4.6
> 
> --
> 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] 19+ messages in thread

* Re: [PATCH 6/7] iio:accel:stk8312: code style cleanup
  2015-07-28 13:45   ` Breana, Tiberiu A
@ 2015-07-28 23:13     ` Hartmut Knaack
  0 siblings, 0 replies; 19+ messages in thread
From: Hartmut Knaack @ 2015-07-28 23:13 UTC (permalink / raw)
  To: Breana, Tiberiu A, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald

Breana, Tiberiu A schrieb am 28.07.2015 um 15:45:
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: Tuesday, July 28, 2015 1:49 AM
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
>> A
>> Subject: [PATCH 6/7] iio:accel:stk8312: code style cleanup
>>
>> Adjust some indentation issues to make checkpatch.pl happy in strict mode.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> Comment inline.
> 
>> ---
>>  drivers/iio/accel/stk8312.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
>> e3624e2b3ec4..b059a6cabfbb 100644
>> --- a/drivers/iio/accel/stk8312.c
>> +++ b/drivers/iio/accel/stk8312.c
>> @@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>  	if (ret < 0)
>>  		goto exit_err;
>>
>> -	ret = i2c_smbus_write_byte_data(data->client,
>> -			STK8312_REG_AFECTRL, ret);
>> +	ret = i2c_smbus_write_byte_data(data->client,
>> STK8312_REG_AFECTRL,
>> +ret);
> 
> On a single line, this write op would take up 81 chars.
> I think it's best to just add 2 tabs to the 2nd line.
> 

Don't worry, it is 80 chars sharp. I have double checked, even checkpatch.pl
in strict mode doesn't complain on this one. Just on 2 other points, which I
don't consider essential.
Thanks,

Hartmut

>>  	if (ret < 0)
>>  		goto exit_err;
>>  	msleep(150);
>> @@ -453,7 +452,7 @@ static irqreturn_t stk8312_trigger_handler(int irq, void
>> *p)
>>  		data->buffer[2] = buffer[2];
>>  	} else {
>>  		for_each_set_bit(bit, indio_dev->active_scan_mask,
>> -			   indio_dev->masklength) {
>> +				 indio_dev->masklength) {
>>  			ret = stk8312_read_accel(data, bit);
>>  			if (ret < 0) {
>>  				mutex_unlock(&data->lock);
>> --
>> 2.4.6
> 
> --
> 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] 19+ messages in thread

* Re: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency
  2015-07-28 13:27   ` Breana, Tiberiu A
@ 2015-08-02 18:13     ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2015-08-02 18:13 UTC (permalink / raw)
  To: Breana, Tiberiu A, Hartmut Knaack, linux-iio
  Cc: Lars-Peter Clausen, Peter Meerwald

On 28/07/15 14:27, Breana, Tiberiu A wrote:
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: Tuesday, July 28, 2015 1:49 AM
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
>> A
>> Subject: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency
>>
>> Add the still missing dependencies for triggered buffer support.
>>
>> Fixes: 	95c12bba51c37 ("iio: accel: Add buffer mode for Sensortek STK8312")
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> +1
> 
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
One day I'll remember to actually check all the dependencies when reviewing!
Thanks Hartmut.

Applied to the togreg branch of iio.git.

Jonathan
> 
>> ---
>>  drivers/iio/accel/Kconfig | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index
>> 00e7bcbdbe24..c99d8e242218 100644
>> --- a/drivers/iio/accel/Kconfig
>> +++ b/drivers/iio/accel/Kconfig
>> @@ -140,6 +140,8 @@ config MMA9553
>>  config STK8312
>>  	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
>>  	depends on I2C
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>>  	help
>>  	  Say yes here to get support for the Sensortek STK8312 3-axis
>>  	  accelerometer.
>> --
>> 2.4.6
> 
> --
> 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] 19+ messages in thread

* Re: [PATCH 4/7] iio:accel:stk8312: rework macro definitions
  2015-07-28 13:28   ` Breana, Tiberiu A
@ 2015-08-02 18:17     ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2015-08-02 18:17 UTC (permalink / raw)
  To: Breana, Tiberiu A, Hartmut Knaack, linux-iio
  Cc: Lars-Peter Clausen, Peter Meerwald

On 28/07/15 14:28, Breana, Tiberiu A wrote:
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: Tuesday, July 28, 2015 1:49 AM
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
>> A
>> Subject: [PATCH 4/7] iio:accel:stk8312: rework macro definitions
>>
>> Make use of BIT to describe register bits, GENMASK for consecutive
>> bitmasks, rename and sort existing definitions, replace magic value with an
>> expressive definition, drop an unused definition.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> 
> +1
> 
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Start getting fuzz over the earlier patch that you are rerolling.
Hence will stop here until you have rerolled.

Jonathan
> 
>> ---
>>  drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c index
>> 50a102ca44d3..8783f0bf7c53 100644
>> --- a/drivers/iio/accel/stk8312.c
>> +++ b/drivers/iio/accel/stk8312.c
>> @@ -37,16 +37,16 @@
>>  #define STK8312_REG_OTPDATA		0x3E
>>  #define STK8312_REG_OTPCTRL		0x3F
>>
>> -#define STK8312_MODE_ACTIVE		0x01
>> +#define STK8312_MODE_ACTIVE		BIT(0)
>>  #define STK8312_MODE_STANDBY		0x00
>> -#define STK8312_DREADY_BIT		0x10
>> -#define STK8312_INT_MODE		0xC0
>> -#define STK8312_RNG_MASK		0xC0
>> -#define STK8312_SR_MASK			0x07
>> -#define STK8312_SR_400HZ_IDX		0
>> +#define STK8312_MODE_INT_AH_PP		0xC0	/* active-high, push-
>> pull */
>> +#define STK8312_DREADY_BIT		BIT(4)
>> +#define STK8312_RNG_6G			1
>>  #define STK8312_RNG_SHIFT		6
>> -#define STK8312_READ_RETRIES		16
>> -#define STK8312_ALL_CHANNEL_MASK	7
>> +#define STK8312_RNG_MASK		GENMASK(7, 6)
>> +#define STK8312_SR_MASK			GENMASK(2, 0)
>> +#define STK8312_SR_400HZ_IDX		0
>> +#define STK8312_ALL_CHANNEL_MASK	GENMASK(2, 0)
>>  #define STK8312_ALL_CHANNEL_SIZE	3
>>
>>  #define STK8312_DRIVER_NAME		"stk8312"
>> @@ -144,7 +144,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
>>  		if (ret < 0)
>>  			goto exit_err;
>>  		count--;
>> -	} while (!(ret & 0x80) && count > 0);
>> +	} while (!(ret & BIT(7)) && count > 0);
>>
>>  	if (count == 0) {
>>  		ret = -ETIMEDOUT;
>> @@ -560,11 +560,12 @@ static int stk8312_probe(struct i2c_client *client,
>>  		return ret;
>>  	}
>>  	data->sample_rate_idx = STK8312_SR_400HZ_IDX;
>> -	ret = stk8312_set_range(data, 1);
>> +	ret = stk8312_set_range(data, STK8312_RNG_6G);
>>  	if (ret < 0)
>>  		return ret;
>>
>> -	ret = stk8312_set_mode(data, STK8312_INT_MODE |
>> STK8312_MODE_ACTIVE);
>> +	ret = stk8312_set_mode(data,
>> +			       STK8312_MODE_INT_AH_PP |
>> STK8312_MODE_ACTIVE);
>>  	if (ret < 0)
>>  		return ret;
>>
>> @@ -685,7 +686,7 @@ MODULE_DEVICE_TABLE(acpi, stk8312_acpi_id);
>>
>>  static struct i2c_driver stk8312_driver = {
>>  	.driver = {
>> -		.name = "stk8312",
>> +		.name = STK8312_DRIVER_NAME,
>>  		.pm = STK8312_PM_OPS,
>>  		.acpi_match_table = ACPI_PTR(stk8312_acpi_id),
>>  	},
>> --
>> 2.4.6
> 
> --
> 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] 19+ messages in thread

end of thread, other threads:[~2015-08-02 18:17 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-27 22:49 [PATCH 0/7] stk8312 fixes and cleanup Hartmut Knaack
2015-07-27 22:49 ` [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency Hartmut Knaack
2015-07-28 13:27   ` Breana, Tiberiu A
2015-08-02 18:13     ` Jonathan Cameron
2015-07-27 22:49 ` [PATCH 2/7] iio:accel:stk8312: check for invalid value Hartmut Knaack
2015-07-28 13:27   ` Breana, Tiberiu A
2015-07-27 22:49 ` [PATCH 3/7] iio:accel:stk8312: improve error handling Hartmut Knaack
2015-07-28 13:38   ` Breana, Tiberiu A
2015-07-28 23:08     ` Hartmut Knaack
2015-07-27 22:49 ` [PATCH 4/7] iio:accel:stk8312: rework macro definitions Hartmut Knaack
2015-07-28 13:28   ` Breana, Tiberiu A
2015-08-02 18:17     ` Jonathan Cameron
2015-07-27 22:49 ` [PATCH 5/7] iio:accel:stk8312: use appropriate variable types Hartmut Knaack
2015-07-28 13:28   ` Breana, Tiberiu A
2015-07-27 22:49 ` [PATCH 6/7] iio:accel:stk8312: code style cleanup Hartmut Knaack
2015-07-28 13:45   ` Breana, Tiberiu A
2015-07-28 23:13     ` Hartmut Knaack
2015-07-27 22:49 ` [PATCH 7/7] iio:accel:stk8312: drop local buffer Hartmut Knaack
2015-07-28 13:29   ` Breana, Tiberiu A

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.