Skip to contentSkip to conversation

stk8312 fixes and cleanup

PATCH Applied Hartmut Knaack 3 participants 19 messages Jul 27, 2015 – Aug 2, 2015 IIO

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover stk8312 fixes and cleanup

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(-)
Signature · 1 line
2.4.6
#1 lore raw

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")
---
 drivers/iio/accel/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/Kconfig +2 -0
index 00e7bcbdbe24..c99d8e242218 100644
@@ -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.
Signature · 1 line
2.4.6
Reviewed-by on v1 1/7
#2 lore raw

RE: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency

Quoted 8 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency

Quoted 5 lines

Add the still missing dependencies for triggered buffer support.

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

+1

Quoted 19 lines
---
 drivers/iio/accel/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/Kconfig index renamed from drivers/iio/accel/Kconfig +0 -0

00e7bcbdbe24..c99d8e242218 100644

drivers/iio/accel/Kconfig +2 -1
@@ -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

maintainer Applied
#3 lore raw

Re: [PATCH 1/7] iio:accel:stk8312: add triggered buffer dependency

Quoted 10 lines · Tiberiu A
Quoted 10 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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.

Quoted 7 lines
Quoted 3 lines
Fixes 95c12bba51c37 ("iio: accel: Add buffer mode for Sensortek STK8312")

+1

One day I'll remember to actually check all the dependencies when reviewing! Thanks Hartmut.

Applied to the togreg branch of iio.git.

Jonathan

Quoted 26 lines
Quoted 19 lines
---
 drivers/iio/accel/Kconfig | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/Kconfig index renamed from drivers/iio/accel/Kconfig +0 -0

00e7bcbdbe24..c99d8e242218 100644

drivers/iio/accel/Kconfig +2 -1
@@ -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.
--
Quoted 1 line

2.4.6

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw

PATCH 2/7 iio:accel:stk8312: check for invalid value

In reply to Hartmut Knaack

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

---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/stk8312.c +2 -0
index c2bd1444d6da..6592be8e6377 100644
@@ -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;
 
Signature · 1 line
2.4.6
Reviewed-by on v1 2/7
#5 lore raw

RE: [PATCH 2/7] iio:accel:stk8312: check for invalid value

Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:linux-iio- [email protected]] On Behalf Of Hartmut Knaack
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: [PATCH 2/7] iio:accel:stk8312: check for invalid value

Quoted 4 lines

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

+1

Quoted 24 lines
---
 drivers/iio/accel/stk8312.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

c2bd1444d6da..6592be8e6377 100644

drivers/iio/accel/stk8312.c +2 -1
@@ -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 [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

#6 lore raw

PATCH 3/7 iio:accel:stk8312: improve error handling

In reply to Hartmut Knaack

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

---
 drivers/iio/accel/stk8312.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)
drivers/iio/accel/stk8312.c +25 -14
index 6592be8e6377..50a102ca44d3 100644
@@ -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);
Signature · 1 line
2.4.6
#7 lore raw

RE: [PATCH 3/7] iio:accel:stk8312: improve error handling

Quoted 16 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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

Quoted 1 line

Comments inline.

Quoted 60 lines
---
 drivers/iio/accel/stk8312.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

6592be8e6377..50a102ca44d3 100644

drivers/iio/accel/stk8312.c +3 -1
@@ -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);
drivers/iio/accel/stk8312.c +2 -1
 	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)

drivers/iio/accel/stk8312.c +0 -0
 		return ret;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_INTSU,

int_mask);

drivers/iio/accel/stk8312.c +4 -2
-	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)

drivers/iio/accel/stk8312.c +0 -0
 	masked_reg = (ret & (~STK8312_SR_MASK)) | rate;
 
 	ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,

masked_reg);

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate) · line 272

Quoted 6 lines
drivers/iio/accel/stk8312.c +2 -3
-	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.

Quoted 48 lines
drivers/iio/accel/stk8312.c +8 -3
+	}
+
+	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;
 
drivers/iio/accel/stk8312.c +4 -3
 	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));

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev, · line 362

Quoted 3 lines
drivers/iio/accel/stk8312.c +2 -0
 		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.

Quoted 14 lines
drivers/iio/accel/stk8312.c +2 -2
 		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

#8 lore raw

Re: [PATCH 3/7] iio:accel:stk8312: improve error handling

Breana, Tiberiu A schrieb am 28.07.2015 um 15:38:

Quoted 16 lines
Quoted 16 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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

Quoted 4 lines
Quoted 1 line

Comments inline.

Hi,

<...>

Quoted 4 lines
Quoted 4 lines

+267,12 @@ static int stk8312_set_sample_rate(struct stk8312_data *data, int rate)
masked_reg = (ret & (~STK8312_SR_MASK)) | rate;

Quoted 13 lines

drivers/iio/accel/stk8312.c static int stk8312_set_sample_rate(struct stk8312_data *data, int rate) · line 272

Quoted 8 lines

ret = i2c_smbus_write_byte_data(client, STK8312_REG_SR,
masked_reg);

drivers/iio/accel/stk8312.c +2 -3
-	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.

Quoted 46 lines
Quoted 46 lines
drivers/iio/accel/stk8312.c +8 -3
+	}
+
+	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;
 
drivers/iio/accel/stk8312.c +3 -3
 	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,
Quoted 9 lines

drivers/iio/accel/stk8312.c static int stk8312_read_raw(struct iio_dev *indio_dev, · line 362

Quoted 5 lines
(quoted diff) +1 -0
+				       data->mode &

(~STK8312_MODE_ACTIVE));

drivers/iio/accel/stk8312.c +2 -0
 		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

Quoted 20 lines
Quoted 13 lines
drivers/iio/accel/stk8312.c +2 -2
 		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);
--
Quoted 1 line

2.4.6

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#9 lore raw

PATCH 4/7 iio:accel:stk8312: rework macro definitions

In reply to Hartmut Knaack

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.

---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/iio/accel/stk8312.c +13 -12
index 50a102ca44d3..8783f0bf7c53 100644
@@ -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),
 	},
Signature · 1 line
2.4.6
Reviewed-by on v1 4/7
#10 lore raw

RE: [PATCH 4/7] iio:accel:stk8312: rework macro definitions

Quoted 8 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: [PATCH 4/7] iio:accel:stk8312: rework macro definitions

Quoted 5 lines

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.

+1

Quoted 71 lines
---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

50a102ca44d3..8783f0bf7c53 100644

drivers/iio/accel/stk8312.c +2 -6
@@ -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 */

drivers/iio/accel/stk8312.c +10 -5
+#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)
drivers/iio/accel/stk8312.c +1 -2
 		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

maintainer
#11 lore raw

Re: [PATCH 4/7] iio:accel:stk8312: rework macro definitions

Quoted 12 lines · Tiberiu A
Quoted 12 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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.

Quoted 5 lines
Quoted 1 line

+1

Start getting fuzz over the earlier patch that you are rerolling. Hence will stop here until you have rerolled.

Jonathan

Quoted 78 lines
Quoted 71 lines
---
 drivers/iio/accel/stk8312.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

50a102ca44d3..8783f0bf7c53 100644

drivers/iio/accel/stk8312.c +2 -6
@@ -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 */

drivers/iio/accel/stk8312.c +10 -5
+#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)
drivers/iio/accel/stk8312.c +1 -2
 		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),
 	},
--
Quoted 1 line

2.4.6

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw

PATCH 5/7 iio:accel:stk8312: use appropriate variable types

In reply to Hartmut Knaack

Adapt some variable types to reduce unnecessary casting.

---
 drivers/iio/accel/stk8312.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/iio/accel/stk8312.c +4 -4
index 8783f0bf7c53..e3624e2b3ec4 100644
@@ -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;
Signature · 1 line
2.4.6
Reviewed-by on v1 5/7
#13 lore raw

RE: [PATCH 5/7] iio:accel:stk8312: use appropriate variable types

Quoted 11 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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.

+1

Quoted 38 lines
---
 drivers/iio/accel/stk8312.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

8783f0bf7c53..e3624e2b3ec4 100644

drivers/iio/accel/stk8312.c +2 -2
@@ -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 {
drivers/iio/accel/stk8312.c +2 -2
 	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

#14 lore raw

PATCH 6/7 iio:accel:stk8312: code style cleanup

In reply to Hartmut Knaack

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

---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/iio/accel/stk8312.c +2 -3
index e3624e2b3ec4..b059a6cabfbb 100644
@@ -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);
Signature · 1 line
2.4.6
#15 lore raw

RE: [PATCH 6/7] iio:accel:stk8312: code style cleanup

Quoted 11 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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.

Comment inline.

Quoted 9 lines
---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

e3624e2b3ec4..b059a6cabfbb 100644

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 162

Quoted 8 lines
drivers/iio/accel/stk8312.c +2 -2
@@ -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.

Quoted 15 lines
drivers/iio/accel/stk8312.c +1 -1
 	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

#16 lore raw

Re: [PATCH 6/7] iio:accel:stk8312: code style cleanup

Breana, Tiberiu A schrieb am 28.07.2015 um 15:45:

Quoted 26 lines
Quoted 11 lines

-----Original Message-----
From: Hartmut Knaack [mailto:[email protected]]
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
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.

Comment inline.

Quoted 12 lines
---
 drivers/iio/accel/stk8312.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

e3624e2b3ec4..b059a6cabfbb 100644

drivers/iio/accel/stk8312.c +0 -0
@@ -157,8 +157,7 @@ static int stk8312_otp_init(struct stk8312_data *data)
 	if (ret < 0)
 		goto exit_err;
 
Quoted 9 lines

drivers/iio/accel/stk8312.c static int stk8312_otp_init(struct stk8312_data *data) · line 162

Quoted 5 lines
drivers/iio/accel/stk8312.c +2 -2
-	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

Quoted 21 lines
Quoted 12 lines
drivers/iio/accel/stk8312.c +1 -1
 	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) {
Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#17 lore raw

PATCH 7/7 iio:accel:stk8312: drop local buffer

In reply to Hartmut Knaack

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

---
 drivers/iio/accel/stk8312.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/iio/accel/stk8312.c +1 -5
index b059a6cabfbb..a6b2bda7a63c 100644
@@ -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) {
Signature · 1 line
2.4.6
Reviewed-by on v1 7/7
#18 lore raw

RE: [PATCH 7/7] iio:accel:stk8312: drop local buffer

Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:linux-iio- [email protected]] On Behalf Of Hartmut Knaack
Sent: Tuesday, July 28, 2015 1:49 AM
To: [email protected]
Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
Subject: [PATCH 7/7] iio:accel:stk8312: drop local buffer

Quoted 4 lines

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

+1

Quoted 44 lines
---
 drivers/iio/accel/stk8312.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/iio/accel/stk8312.c index renamed from drivers/iio/accel/stk8312.c +0 -0

b059a6cabfbb..a6b2bda7a63c 100644

drivers/iio/accel/stk8312.c +0 -1
@@ -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);
drivers/iio/accel/stk8312.c +1 -4
 	/*
@@ -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];
drivers/iio/accel/stk8312.c +0 -1
 	} 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 [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html