iio:humidity:si7020: cleanup read_raw and probe

PATCH In review Hartmut Knaack 2 participants 5 messages Oct 18, 2014 – Oct 25, 2014 IIO

Share & tools

Hartmut Knaack

author

2014-10-18 19:46 UTC #0 lore raw

PATCH 1/1 iio:humidity:si7020: cleanup read_raw and probe

In si7020_read_raw() the pointer to the i2c client was obtained as second level pointer, although a simple pointer would be sufficient. Cleaned this.
When reading temperature or humidity values, a right-shift of two bits needs to be applied, and only for the humidity channel a mask of the lower 12 bits needs to be applied. This reduces code repetition. During probe, i2c_set_clientdata() was used, although its counterpart was not, so drop it.

Signed-off-by Hartmut Knaack <[email protected]>

---

drivers/iio/humidity/si7020.c +5 -7

index e336af7..b541646 100644
@@ -45,21 +45,20 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	struct i2c_client **client = iio_priv(indio_dev);
+	struct i2c_client *client = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
+		ret = i2c_smbus_read_word_data(client,
 					       chan->type == IIO_TEMP ?
 					       SI7020CMD_TEMP_HOLD :
 					       SI7020CMD_RH_HOLD);
 		if (ret < 0)
 			return ret;
-		if (chan->type == IIO_TEMP)
-			*val = ret >> 2;
-		else
-			*val = (ret & 0x3FFF) >> 2;
+		*val = ret >> 2;
+		if (chan->type == IIO_HUMIDITYRELATIVE)
+			*val &= GENMASK(11, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)

@@ -133,7 +132,6 @@ static int si7020_probe(struct i2c_client *client,
 
 	data = iio_priv(indio_dev);
 	*data = client;
-	i2c_set_clientdata(client, indio_dev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = dev_name(&client->dev);

Jonathan Cameron

maintainer

2014-10-25 18:21 UTC #1 lore raw

Quoted 5 lines · Hartmut Knaack

In si7020_read_raw() the pointer to the i2c client was obtained as second level pointer, although a simple pointer would be sufficient. Cleaned this.
When reading temperature or humidity values, a right-shift of two bits needs to be applied, and only for the humidity channel a mask of the lower 12 bits needs to be applied. This reduces code repetition. During probe, i2c_set_clientdata() was used, although its counterpart was not, so drop it.
Signed-off-by Hartmut Knaack <[email protected]>

I think the incorrect pointer is a bug (rather than simply unnecessary). The other stuff is a cleanup (sensible but not stable material).

Please split the patch into two.

Thanks,

Jonathan

Quoted 45 lines

---

drivers/iio/humidity/si7020.c +5 -8

index e336af7..b541646 100644
@@ -45,21 +45,20 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	struct i2c_client **client = iio_priv(indio_dev);
+	struct i2c_client *client = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
+		ret = i2c_smbus_read_word_data(client,
 					       chan->type == IIO_TEMP ?
 					       SI7020CMD_TEMP_HOLD :
 					       SI7020CMD_RH_HOLD);
 		if (ret < 0)
 			return ret;
-		if (chan->type == IIO_TEMP)
-			*val = ret >> 2;
-		else
-			*val = (ret & 0x3FFF) >> 2;
+		*val = ret >> 2;
+		if (chan->type == IIO_HUMIDITYRELATIVE)
+			*val &= GENMASK(11, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)

@@ -133,7 +132,6 @@ static int si7020_probe(struct i2c_client *client,
 
 	data = iio_priv(indio_dev);
 	*data = client;
-	i2c_set_clientdata(client, indio_dev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = dev_name(&client->dev);
--

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

Hartmut Knaack

author

2014-10-25 18:42 UTC #2 lore raw

Jonathan Cameron schrieb am 25.10.2014 12:02:

Quoted 7 lines · Hartmut Knaack

Quoted 5 lines
In si7020_read_raw() the pointer to the i2c client was obtained as second level pointer, although a simple pointer would be sufficient. Cleaned this.
When reading temperature or humidity values, a right-shift of two bits needs to be applied, and only for the humidity channel a mask of the lower 12 bits needs to be applied. This reduces code repetition. During probe, i2c_set_clientdata() was used, although its counterpart was not, so drop it.
Signed-off-by Hartmut Knaack <[email protected]>
I think the incorrect pointer is a bug (rather than simply unnecessary).

Could you give me some infos, why you assess this issue as a bug?

Quoted 3 lines

The other stuff is a cleanup (sensible but not stable material).
Please split the patch into two.

No problem at all.

Quoted 50 lines

Thanks,

Jonathan

Quoted 45 lines

---

drivers/iio/humidity/si7020.c +5 -8

index e336af7..b541646 100644
@@ -45,21 +45,20 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	struct i2c_client **client = iio_priv(indio_dev);
+	struct i2c_client *client = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
+		ret = i2c_smbus_read_word_data(client,
 					       chan->type == IIO_TEMP ?
 					       SI7020CMD_TEMP_HOLD :
 					       SI7020CMD_RH_HOLD);
 		if (ret < 0)
 			return ret;
-		if (chan->type == IIO_TEMP)
-			*val = ret >> 2;
-		else
-			*val = (ret & 0x3FFF) >> 2;
+		*val = ret >> 2;
+		if (chan->type == IIO_HUMIDITYRELATIVE)
+			*val &= GENMASK(11, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)

@@ -133,7 +132,6 @@ static int si7020_probe(struct i2c_client *client,
 
 	data = iio_priv(indio_dev);
 	*data = client;
-	i2c_set_clientdata(client, indio_dev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = dev_name(&client->dev);
--

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

Jonathan Cameron

maintainer

2014-10-25 19:34 UTC #3 lore raw

Quoted 9 lines · Hartmut Knaack

Jonathan Cameron schrieb am 25.10.2014 12:02:
Quoted 7 lines · Hartmut Knaack
Quoted 5 lines
In si7020_read_raw() the pointer to the i2c client was obtained as second level pointer, although a simple pointer would be sufficient. Cleaned this.
When reading temperature or humidity values, a right-shift of two bits needs to be applied, and only for the humidity channel a mask of the lower 12 bits needs to be applied. This reduces code repetition. During probe, i2c_set_clientdata() was used, although its counterpart was not, so drop it.
Signed-off-by Hartmut Knaack <[email protected]>
I think the incorrect pointer is a bug (rather than simply unnecessary).
Could you give me some infos, why you assess this issue as a bug?

Fair point. It's a bug with no effect whatsoever.
I didn't really follow that through other than going
"Gah it has the wrong type, we'll dereference it once too many and it will eat babies!"

Clearly it just gets passed on wards thus two bugs cancel each other out.

Quoted 4 lines

Quoted 3 lines
The other stuff is a cleanup (sensible but not stable material).
Please split the patch into two.
No problem at all.

I'll probably just take them both through togreg anyway so would have been fine in the first place. Sorry about that.

Quoted 51 lines

Quoted 50 lines

Thanks,

Jonathan

Quoted 45 lines

---

drivers/iio/humidity/si7020.c +5 -8

index e336af7..b541646 100644
@@ -45,21 +45,20 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	struct i2c_client **client = iio_priv(indio_dev);
+	struct i2c_client *client = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
+		ret = i2c_smbus_read_word_data(client,
 					       chan->type == IIO_TEMP ?
 					       SI7020CMD_TEMP_HOLD :
 					       SI7020CMD_RH_HOLD);
 		if (ret < 0)
 			return ret;
-		if (chan->type == IIO_TEMP)
-			*val = ret >> 2;
-		else
-			*val = (ret & 0x3FFF) >> 2;
+		*val = ret >> 2;
+		if (chan->type == IIO_HUMIDITYRELATIVE)
+			*val &= GENMASK(11, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)

@@ -133,7 +132,6 @@ static int si7020_probe(struct i2c_client *client,
 
 	data = iio_priv(indio_dev);
 	*data = client;
-	i2c_set_clientdata(client, indio_dev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = dev_name(&client->dev);
--

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

Jonathan Cameron

maintainer

2014-10-25 19:37 UTC #4 lore raw

Quoted 8 lines · Jonathan Cameron

Quoted 7 lines · Hartmut Knaack
Jonathan Cameron schrieb am 25.10.2014 12:02:
Quoted 6 lines · Hartmut Knaack
Quoted 5 lines
In si7020_read_raw() the pointer to the i2c client was obtained as second level pointer, although a simple pointer would be sufficient. Cleaned this.
When reading temperature or humidity values, a right-shift of two bits needs to be applied, and only for the humidity channel a mask of the lower 12 bits needs to be applied. This reduces code repetition. During probe, i2c_set_clientdata() was used, although its counterpart was not, so drop it.
Signed-off-by Hartmut Knaack <[email protected]>

Quoted 8 lines

Quoted 2 lines
Quoted 1 line
I think the incorrect pointer is a bug (rather than simply unnecessary).
Could you give me some infos, why you assess this issue as a bug?
Fair point. It's a bug with no effect whatsoever.
I didn't really follow that through other than going
"Gah it has the wrong type, we'll dereference it once too many and it will eat babies!"
Clearly it just gets passed on wards thus two bugs cancel each other out.

I take it back. Just taken a look at your updated patch. The dereference will mean that instead of passing a pointer into the read, we will be passing whatever is the start of an actual i2c_client structure.

Quoted 65 lines

Quoted 5 lines
Quoted 4 lines
The other stuff is a cleanup (sensible but not stable material).
Please split the patch into two.
No problem at all.
I'll probably just take them both through togreg anyway so would have been fine in the first place. Sorry about that.
J
Quoted 42 lines
Quoted 42 lines
Thanks,
Jonathan
Quoted 38 lines
---
drivers/iio/humidity/si7020.c +5 -7
index e336af7..b541646 100644
@@ -45,21 +45,20 @@ static int si7020_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan, int *val,
 			   int *val2, long mask)
 {
-	struct i2c_client **client = iio_priv(indio_dev);
+	struct i2c_client *client = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = i2c_smbus_read_word_data(*client,
+		ret = i2c_smbus_read_word_data(client,
 					       chan->type == IIO_TEMP ?
 					       SI7020CMD_TEMP_HOLD :
 					       SI7020CMD_RH_HOLD);
 		if (ret < 0)
 			return ret;
-		if (chan->type == IIO_TEMP)
-			*val = ret >> 2;
-		else
-			*val = (ret & 0x3FFF) >> 2;
+		*val = ret >> 2;
+		if (chan->type == IIO_HUMIDITYRELATIVE)
+			*val &= GENMASK(11, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP)
@@ -133,7 +132,6 @@ static int si7020_probe(struct i2c_client *client,
 
 	data = iio_priv(indio_dev);
 	*data = client;
-	i2c_set_clientdata(client, indio_dev);
 
Quoted 9 lines
Quoted 8 lines
Quoted 7 lines
drivers/iio/humidity/si7020.c +0 -1
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = dev_name(&client->dev);
--
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
--
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