staging: nvec: fixed few coding style warnings

PATCH In review Sakshi Bansal 3 participants 5 messages Oct 14, 2015 – Oct 15, 2015

Subsystems (3)

Staging Tegra architecture support Staging - NVIDIA compliant embedded controller interface (nvec)

lore

Share & tools

lore

Sakshi Bansal

author

2015-10-14 14:08 UTC #0 lore raw

PATCH 1/1 staging: nvec: fixed few coding style warnings

Fixed allignmnet issues and block comments usage

Signed-off-by Sakshi Bansal <[email protected]>

---
 drivers/staging/nvec/README       |  2 +-
 drivers/staging/nvec/nvec.c       | 41 ++++++++++++++++++++-------------------
 drivers/staging/nvec/nvec_paz00.c |  1 -
 drivers/staging/nvec/nvec_power.c |  6 ++++--
 4 files changed, 26 insertions(+), 24 deletions(-)

drivers/staging/nvec/README +1 -1

index 9a320b7..0e2d5c4 100644
@@ -1,4 +1,4 @@
-NVEC: An NVidia compliant Embedded Controller Protocol Implemenation
+NVEC: An NVidia compliant Embedded Controller Protocol Implementation
 
 This is an implementation of the NVEC protocol used to communicate with an
 embedded controller (EC) via I2C bus. The EC is an I2C master while the host

drivers/staging/nvec/nvec.c +21 ~~-20~~

138 lines not on this page · raw message

drivers/staging/nvec/nvec_paz00.c +0 -1

index 68146bf..f998193 100644
@@ -41,7 +41,6 @@ static void nvec_led_brightness_set(struct led_classdev *led_cdev,
 	nvec_write_async(led->nvec, buf, sizeof(buf));
 
 	led->cdev.brightness = value;
-
 }
 
 static int nvec_paz00_probe(struct platform_device *pdev)

drivers/staging/nvec/nvec_power.c +4 -2

index 04a7402..3702bac 100644
@@ -208,7 +208,8 @@ static int nvec_power_bat_notifier(struct notifier_block *nb,
 		memcpy(power->bat_type, &res->plc, res->length - 2);
 		power->bat_type[res->length - 2] = '\0';
 		/* this differs a little from the spec
-		   fill in more if you find some */
+		 * fill in more if you find some
+		 */
 		if (!strncmp(power->bat_type, "Li", 30))
 			power->bat_type_enum = POWER_SUPPLY_TECHNOLOGY_LION;
 		else

@@ -361,7 +362,8 @@ static void nvec_power_poll(struct work_struct *work)
 	msleep(100);
 
 /* select a battery request function via round robin
-   doing it all at once seems to overload the power supply */
+ * doing it all at once seems to overload the power supply
+ */
 	buf[0] = NVEC_BAT;
 	buf[1] = bat_iter[counter++];
 	nvec_write_async(power->nvec, buf, 2);

Signature · 1 line

2.1.0

Marc Dietrich

maintainer

2015-10-15 08:39 UTC #2 lore raw

Am Mittwoch, 14. Oktober 2015, 21:12:36 schrieb Dan Carpenter:

Quoted 17 lines · Sakshi Bansal

Quoted 1 line
Fixed allignmnet issues and block comments usage
Split it apart by type of fix.
drivers/staging/nvec/nvec.c static irqreturn_t nvec_interrupt(int irq, void *dev) · line 624
Quoted 9 lines
drivers/staging/nvec/nvec.c +1 -1
@@ -617,7 +618,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 
 		} else {
 
 			nvec->rx = nvec_msg_alloc(nvec, NVEC_MSG_RX);
 			/* Should not happen in a normal world */
 
-			if (unlikely(nvec->rx == NULL)) {
+			if (!unlikely(nvec->rx)) {
This isn't right. You intented to say:

Quoted 5 lines

	if (unlikely(!nvec->rx)) {
But even better to just remove the unlikely entirely.
	if (!nvec->rx) {

why? the "unlikely" is there to optimize a critical interrupt path.

Marc

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Dan Carpenter

2015-10-15 08:48 UTC #3 lore raw

Quoted 5 lines · Marc Dietrich

Quoted 5 lines
drivers/staging/nvec/nvec.c static irqreturn_t nvec_interrupt(int irq, void *dev) · line 621
Quoted 2 lines
drivers/staging/nvec/nvec.c +1 -1
-			if (unlikely(nvec->rx == NULL)) {
+			if (!unlikely(nvec->rx)) {
This isn't right. You intented to say:

Quoted 7 lines

Quoted 5 lines
	if (unlikely(!nvec->rx)) {
But even better to just remove the unlikely entirely.
	if (!nvec->rx) {
why? the "unlikely" is there to optimize a critical interrupt path.

The rule is that drivers should not use likely/unlikely() unless there is a difference in benchmark numbers. How critical can it be when it's always followed by a udelay(100)???

There are more important optimizations needed here.

regards,
dan carpenter

Marc Dietrich

maintainer

2015-10-15 09:15 UTC #4 lore raw

Am Donnerstag, 15. Oktober 2015, 11:48:12 schrieb Dan Carpenter:

Quoted 7 lines · Marc Dietrich

Quoted 6 lines
Quoted 6 lines
drivers/staging/nvec/nvec.c static irqreturn_t nvec_interrupt(int irq, void *dev) · line 621
Quoted 2 lines
drivers/staging/nvec/nvec.c +1 -1
-			if (unlikely(nvec->rx == NULL)) {
+			if (!unlikely(nvec->rx)) {
This isn't right. You intented to say:
	if (unlikely(!nvec->rx)) {

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
But even better to just remove the unlikely entirely.
	if (!nvec->rx) {
why? the "unlikely" is there to optimize a critical interrupt path.
The rule is that drivers should not use likely/unlikely() unless there is a difference in benchmark numbers.

well, we know that additional cpu cycles in this path break transfer for unknown reasons. However, the unlikely here may be overkill. On the other hand, I prefer not to change something here until these timing effects are better understood.

Quoted 2 lines

How critical can it be when it's
always followed by a udelay(100)???

yes, this delay shouldn't be there at all. This is one of the timing mysteries we still have to resolve.

Quoted 1 line

There are more important optimizations needed here.

sure. We are currently trying to move all this out into the tegra-i2c driver, so this code block will get a major review/rewrite in the near future anyway.

Marc

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel