iio: accel: cros_ec_accel_legacy: Mark expected switch fall-throughs

PATCH In review Gustavo A. R. Silva 3 participants 5 messages Aug 15, 2018 – Aug 19, 2018

Subsystems (2)

IIO ChromeOS EC subdrivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-08-15 16:38 UTC #0 lore raw

PATCH 1/1 iio: accel: cros_ec_accel_legacy: Mark expected switch fall-throughs

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.

Addresses-Coverity-ID: 1462408 ("Missing break in switch")

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/cros_ec_accel_legacy.c +2 -0

index 063e89e..d609654 100644
@@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
 		switch (i) {
 		case X:
 			ec_accel_channels[X].scan_index = Y;
+			/* fall through */
 		case Y:
 			ec_accel_channels[Y].scan_index = X;
+			/* fall through */
 		case Z:
 			ec_accel_channels[Z].scan_index = Z;
 		}

Signature · 1 line

2.7.4

Marcus Folkesson

2018-08-15 17:26 UTC #1 lore raw

Hi,

Quoted 14 lines · Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Addresses-Coverity-ID: 1462408 ("Missing break in switch")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +0 -0
index 063e89e..d609654 100644

drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394

Quoted 10 lines

drivers/iio/accel/cros_ec_accel_legacy.c +2 -0

@@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
 		switch (i) {
 		case X:
 			ec_accel_channels[X].scan_index = Y;
+			/* fall through */
 		case Y:
 			ec_accel_channels[Y].scan_index = X;
+			/* fall through */
 		case Z:
 			ec_accel_channels[Z].scan_index = Z;
 		}

Hum, I'm not sure we are supposed to fall through here, even if it does not hurt to do so.
I even think we can remove the switch and put that outside the for-loop, e.g:

	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	for (i = X ; i < MAX_AXIS; i++) {
		if (state->sensor_num == MOTIONSENSE_LOC_LID && i != Y)
			state->sign[i] = -1;
		else
			state->sign[i] = 1;
	}

Best regards,
Marcus Folkesson

Quoted 3 lines

Signature · 1 line
2.7.4

Gustavo A. R. Silva

author

2018-08-15 17:50 UTC #2 lore raw

Hi Marcus,

Quoted 22 lines · Marcus Folkesson

Hi,
Quoted 19 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Addresses-Coverity-ID: 1462408 ("Missing break in switch")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
index 063e89e..d609654 100644
@@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
 		switch (i) {
 		case X:
 			ec_accel_channels[X].scan_index = Y;
+			/* fall through */
 		case Y:

Quoted 8 lines

drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394
Quoted 5 lines
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
 			ec_accel_channels[Y].scan_index = X;
+			/* fall through */
 		case Z:
 			ec_accel_channels[Z].scan_index = Z;
 		}
Hum, I'm not sure we are supposed to fall through here, even if it does not hurt to do so.

Yeah. You're right. It doesn't hurt but is actually redundant. I think the original intention was to break instead of falling through.

Quoted 14 lines

I even think we can remove the switch and put that outside the for-loop, e.g:

	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	for (i = X ; i < MAX_AXIS; i++) {
		if (state->sensor_num == MOTIONSENSE_LOC_LID && i != Y)
			state->sign[i] = -1;
		else
			state->sign[i] = 1;
	}

I like this, but the code clearly depends on MAX_AXIS. So, if MAX_AXIS will be always 3, then the change you suggest is just fine. Otherwise, it seems that adding a break to each case is the right way to go.

What do you think?

Thanks for the feedback.
--
Gustavo

Marcus Folkesson

2018-08-18 15:34 UTC #3 lore raw

Hi Gutavo,

Sorry for the delay.

Quoted 52 lines · Gustavo A. R. Silva

Hi Marcus,
Quoted 22 lines · Marcus Folkesson
Hi,
Quoted 19 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Addresses-Coverity-ID: 1462408 ("Missing break in switch")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
index 063e89e..d609654 100644
@@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
 		switch (i) {
 		case X:
 			ec_accel_channels[X].scan_index = Y;
+			/* fall through */
 		case Y:
Quoted 8 lines
drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394
Quoted 5 lines
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
 			ec_accel_channels[Y].scan_index = X;
+			/* fall through */
 		case Z:
 			ec_accel_channels[Z].scan_index = Z;
 		}
Hum, I'm not sure we are supposed to fall through here, even if it does not hurt to do so.
Yeah. You're right. It doesn't hurt but is actually redundant. I think the original intention was to break instead of falling through.
Quoted 14 lines
I even think we can remove the switch and put that outside the for-loop, e.g:
	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	for (i = X ; i < MAX_AXIS; i++) {
		if (state->sensor_num == MOTIONSENSE_LOC_LID && i != Y)
			state->sign[i] = -1;
		else
			state->sign[i] = 1;
	}

Quoted 5 lines

I like this, but the code clearly depends on MAX_AXIS. So, if MAX_AXIS will be always 3, then the change you suggest is just fine. Otherwise, it seems that adding a break to each case is the right way to go.
What do you think?

Well, I guess it is a matter of taste after all.
I don't think the number of axis will change, but just put the break in place is good enough.

Anyway, If we choose to not use the switch, I think we should remove the for-loop as well, eg:

	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	if (state->sensor_num == MOTIONSENSE_LOC_LID) {
		state->sign[X] = -1;
		state->sign[Y] = 1;
		state->sign[Z] = -1;
	} else {
		state->sign[X] = 1;
		state->sign[Y] = 1;
		state->sign[Z] = 1;
	}

But someone else may like to give their point of view on this change.

Quoted 4 lines

Thanks for the feedback.
--
Gustavo

Best regards
Marcus Folkesson

Jonathan Cameron

maintainer

2018-08-19 16:20 UTC #4 lore raw

Quoted 84 lines · On Sat, 18 Aug 2018 17:34:40 +0200 Marcus Folkesson…

Hi Gutavo,
Sorry for the delay.
Quoted 52 lines · Gustavo A. R. Silva
Hi Marcus,
Quoted 22 lines · Marcus Folkesson
Hi,
Quoted 19 lines · Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through.
Addresses-Coverity-ID: 1462408 ("Missing break in switch")
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
index 063e89e..d609654 100644
@@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
 		switch (i) {
 		case X:
 			ec_accel_channels[X].scan_index = Y;
+			/* fall through */
 		case Y:
Quoted 8 lines
drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394
Quoted 5 lines
drivers/iio/accel/cros_ec_accel_legacy.c +1 -0
 			ec_accel_channels[Y].scan_index = X;
+			/* fall through */
 		case Z:
 			ec_accel_channels[Z].scan_index = Z;
 		}  
Hum, I'm not sure we are supposed to fall through here, even if it does not hurt to do so.
Yeah. You're right. It doesn't hurt but is actually redundant. I think the original intention was to break instead of falling through.
Quoted 14 lines
I even think we can remove the switch and put that outside the for-loop, e.g:
	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	for (i = X ; i < MAX_AXIS; i++) {
		if (state->sensor_num == MOTIONSENSE_LOC_LID && i != Y)
			state->sign[i] = -1;
		else
			state->sign[i] = 1;
	}
Quoted 5 lines
I like this, but the code clearly depends on MAX_AXIS. So, if MAX_AXIS will be always 3, then the change you suggest is just fine. Otherwise, it seems that adding a break to each case is the right way to go.
What do you think?
Well, I guess it is a matter of taste after all.
I don't think the number of axis will change, but just put the break in place is good enough.
Anyway, If we choose to not use the switch, I think we should remove the for-loop as well, eg:
	ec_accel_channels[X].scan_index = Y;
	ec_accel_channels[Y].scan_index = X;
	ec_accel_channels[Z].scan_index = Z;

	if (state->sensor_num == MOTIONSENSE_LOC_LID) {
		state->sign[X] = -1;
		state->sign[Y] = 1;
		state->sign[Z] = -1;
	} else {
		state->sign[X] = 1;
		state->sign[Y] = 1;
		state->sign[Z] = 1;
	}

Quoted 1 line

But someone else may like to give their point of view on this change.

Looks like the right tidy up to me. The original code was 'novel' :)

Jonathan

Quoted 8 lines

Quoted 5 lines
Thanks for the feedback.
--
Gustavo
Best regards
Marcus Folkesson