[PATCH 0/5] ACPI/EC: Cleanup debugging messages and coding styles.

[PATCH 0/5] ACPI/EC: Cleanup debugging messages and coding styles.

[Lv Zheng]

This patchset contains trivial cleanup code to make it easier to search in
the EC debugging messages.
It also includes a patch to correct coding style issues that detected by
recent scripts/checkpatch.pl.

No functional changes. This patchset can only make remote debugging more
efficient.

Lv Zheng (5):
  ACPI/EC: Add CPU ID to debugging messages.
  ACPI/EC: Enhance the logs to apply to QR_EC transactions.
  ACPI/EC: Add detailed command/query debugging information.
  ACPI/EC: Refine event/query debugging messages.
  ACPI/EC: Cleanup coding style.

 drivers/acpi/ec.c |  109 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 39 deletions(-)

-- 
1.7.10

[PATCH 1/5] ACPI/EC: Add CPU ID to debugging messages.

[Lv Zheng]

This patch adds CPU ID to the context entries' debugging output. no
functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 083e513..3eac545 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -181,7 +181,8 @@ static bool advance_transaction(struct acpi_ec *ec)
 	u8 status;
 	bool wakeup = false;
 
-	pr_debug("===== %s =====\n", in_interrupt() ? "IRQ" : "TASK");
+	pr_debug("===== %s (%d) =====\n",
+		 in_interrupt() ? "IRQ" : "TASK", smp_processor_id());
 	status = acpi_ec_read_status(ec);
 	t = ec->curr;
 	if (!t)
-- 
1.7.10

[PATCH 2/5] ACPI/EC: Enhance the logs to apply to QR_EC transactions.

[Lv Zheng]

Currently some logs are applied to new transactions, but QR_EC transactions
are not included. This patch merges the code path to make the logs also
applying to the QR_EC transactions.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3eac545..49c4ced 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,12 +303,15 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	spin_lock_irqsave(&ec->lock, tmp);
 	/* following two actions should be kept atomic */
 	ec->curr = t;
+	pr_debug("transaction start (cmd=0x%02x, addr=0x%02x)\n",
+			t->command, t->wdata ? t->wdata[0] : 0);
 	start_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+	pr_debug("transaction end\n");
 	ec->curr = NULL;
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	return ret;
@@ -334,8 +337,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 			goto unlock;
 		}
 	}
-	pr_debug("transaction start (cmd=0x%02x, addr=0x%02x)\n",
-			t->command, t->wdata ? t->wdata[0] : 0);
 	/* disable GPE during transaction if storm is detected */
 	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
 		/* It has to be disabled, so that it doesn't trigger. */
@@ -356,7 +357,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 			t->irq_count);
 		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
 	}
-	pr_debug("transaction end\n");
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
 unlock:
-- 
1.7.10

[PATCH 3/5] ACPI/EC: Add detailed command/query debugging information.

[Lv Zheng]

Developers really don't need to translate EC commands in mind. This patch
adds detailed debugging information for the EC commands.
The address can be found in the follow-up sequential EC_DATA(W) accesses,
thus this patch also removes some of the redundant address information.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 49c4ced..8957d51 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -164,6 +164,27 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
+#ifdef DEBUG
+static const char *acpi_ec_cmd_string(u8 cmd)
+{
+	switch (cmd) {
+	case 0x80:
+		return "RD_EC";
+	case 0x81:
+		return "WR_EC";
+	case 0x82:
+		return "BE_EC";
+	case 0x83:
+		return "BD_EC";
+	case 0x84:
+		return "QR_EC";
+	}
+	return "UNKNOWN";
+}
+#else
+#define acpi_ec_cmd_string(cmd)		"UNDEF"
+#endif
+
 static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -199,7 +220,8 @@ static bool advance_transaction(struct acpi_ec *ec)
 				if (t->rlen == t->ri) {
 					t->flags |= ACPI_EC_COMMAND_COMPLETE;
 					if (t->command == ACPI_EC_COMMAND_QUERY)
-						pr_debug("hardware QR_EC completion\n");
+						pr_debug("***** Command(%s) hardware completion *****\n",
+							 acpi_ec_cmd_string(t->command));
 					wakeup = true;
 				}
 			} else
@@ -222,7 +244,8 @@ static bool advance_transaction(struct acpi_ec *ec)
 			t->flags |= ACPI_EC_COMMAND_POLL;
 			t->rdata[t->ri++] = 0x00;
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
-			pr_debug("software QR_EC completion\n");
+			pr_debug("***** Command(%s) software completion *****\n",
+				 acpi_ec_cmd_string(t->command));
 			wakeup = true;
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
@@ -303,15 +326,16 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	spin_lock_irqsave(&ec->lock, tmp);
 	/* following two actions should be kept atomic */
 	ec->curr = t;
-	pr_debug("transaction start (cmd=0x%02x, addr=0x%02x)\n",
-			t->command, t->wdata ? t->wdata[0] : 0);
+	pr_debug("***** Command(%s) started *****\n",
+		 acpi_ec_cmd_string(t->command));
 	start_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-	pr_debug("transaction end\n");
+	pr_debug("***** Command(%s) stopped *****\n",
+		 acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	return ret;
-- 
1.7.10

[PATCH 4/5] ACPI/EC: Refine event/query debugging messages.

[Lv Zheng]

This patch refines event/query debugging messages to use a unified format
as commands. Developers can clearly find different processes by checking
different log seperators. No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 8957d51..d993df6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -332,8 +332,10 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
 		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+		pr_debug("***** Event stopped *****\n");
+	}
 	pr_debug("***** Command(%s) stopped *****\n",
 		 acpi_ec_cmd_string(t->command));
 	ec->curr = NULL;
@@ -617,12 +619,12 @@ static void acpi_ec_run(void *cxt)
 	struct acpi_ec_query_handler *handler = cxt;
 	if (!handler)
 		return;
-	pr_debug("start query execution\n");
+	pr_debug("##### Query(0x%02x) started #####\n", handler->query_bit);
 	if (handler->func)
 		handler->func(handler->data);
 	else if (handler->handle)
 		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
-	pr_debug("stop query execution\n");
+	pr_debug("##### Query(0x%02x) stopped #####\n", handler->query_bit);
 	kfree(handler);
 }
 
@@ -645,8 +647,8 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 			if (!copy)
 				return -ENOMEM;
 			memcpy(copy, handler, sizeof(*copy));
-			pr_debug("push query execution (0x%2x) on queue\n",
-				value);
+			pr_debug("##### Query(0x%02x) scheduled #####\n",
+				 handler->query_bit);
 			return acpi_os_execute((copy->func) ?
 				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
 				acpi_ec_run, copy);
@@ -669,7 +671,7 @@ static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
 	if (state & ACPI_EC_FLAG_SCI) {
 		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-			pr_debug("push gpe query to the queue\n");
+			pr_debug("***** Event started *****\n");
 			return acpi_os_execute(OSL_NOTIFY_HANDLER,
 				acpi_ec_gpe_query, ec);
 		}
-- 
1.7.10

[PATCH 5/5] ACPI/EC: Cleanup coding style.

[Lv Zheng]

This patch cleans up the following coding style issues that are detected by
scripts/checkpatch.pl:
 ERROR: code indent should use tabs where possible
 ERROR: "foo * bar" should be "foo *bar"
 WARNING: Missing a blank line after declarations
 WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
 WARNING: void function return statements are not generally useful
 WARNING: else is not generally useful after a break or return
 WARNING: break is not useful after a goto or return
 WARNING: braces {} are not necessary for single statement blocks
 WARNING: line over 80 characters
 WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   58 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d993df6..5f4e6c6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -128,12 +128,13 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
 static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
 
 /* --------------------------------------------------------------------------
-                             Transaction Management
-   -------------------------------------------------------------------------- */
+ *                           Transaction Management
+ * -------------------------------------------------------------------------- */
 
 static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->command_addr);
+
 	pr_debug("EC_SC(R) = 0x%2.2x "
 		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
 		 x,
@@ -148,6 +149,7 @@ static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
 static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
+
 	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
 	return x;
 }
@@ -189,6 +191,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	int ret = 0;
+
 	spin_lock_irqsave(&ec->lock, flags);
 	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
 		ret = 1;
@@ -288,6 +291,7 @@ static int ec_poll(struct acpi_ec *ec)
 {
 	unsigned long flags;
 	int repeat = 5; /* number of command restarts */
+
 	while (repeat--) {
 		unsigned long delay = jiffies +
 			msecs_to_jiffies(ec_delay);
@@ -320,6 +324,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 {
 	unsigned long tmp;
 	int ret = 0;
+
 	if (EC_FLAGS_MSI)
 		udelay(ACPI_EC_MSI_UDELAY);
 	/* start transaction */
@@ -347,6 +352,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 {
 	int status;
 	u32 glk;
+
 	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
 		return -EINVAL;
 	if (t->rdata)
@@ -374,7 +380,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 	/* check if we received SCI during transaction */
 	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
 	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
-		msleep(1);
+		msleep(20);
 		/* It is safe to enable the GPE outside of the transaction. */
 		acpi_enable_gpe(NULL, ec->gpe, TRUE);
 	} else if (t->irq_count > ec_storm_threshold) {
@@ -410,7 +416,7 @@ static int acpi_ec_burst_disable(struct acpi_ec *ec)
 				acpi_ec_transaction(ec, &t) : 0;
 }
 
-static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
+static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
 {
 	int result;
 	u8 d;
@@ -446,10 +452,9 @@ int ec_read(u8 addr, u8 *val)
 	if (!err) {
 		*val = temp_data;
 		return 0;
-	} else
-		return err;
+	}
+	return err;
 }
-
 EXPORT_SYMBOL(ec_read);
 
 int ec_write(u8 addr, u8 val)
@@ -463,22 +468,21 @@ int ec_write(u8 addr, u8 val)
 
 	return err;
 }
-
 EXPORT_SYMBOL(ec_write);
 
 int ec_transaction(u8 command,
-		   const u8 * wdata, unsigned wdata_len,
-		   u8 * rdata, unsigned rdata_len)
+		   const u8 *wdata, unsigned wdata_len,
+		   u8 *rdata, unsigned rdata_len)
 {
 	struct transaction t = {.command = command,
 				.wdata = wdata, .rdata = rdata,
 				.wlen = wdata_len, .rlen = rdata_len};
+
 	if (!first_ec)
 		return -ENODEV;
 
 	return acpi_ec_transaction(first_ec, &t);
 }
-
 EXPORT_SYMBOL(ec_transaction);
 
 /* Get the handle to the EC device */
@@ -488,7 +492,6 @@ acpi_handle ec_get_handle(void)
 		return NULL;
 	return first_ec->handle;
 }
-
 EXPORT_SYMBOL(ec_get_handle);
 
 /*
@@ -552,13 +555,14 @@ void acpi_ec_unblock_transactions_early(void)
 		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
 }
 
-static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
+static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
 {
 	int result;
 	u8 d;
 	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
 				.wdata = NULL, .rdata = &d,
 				.wlen = 0, .rlen = 1};
+
 	if (!ec || !data)
 		return -EINVAL;
 	/*
@@ -584,6 +588,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 {
 	struct acpi_ec_query_handler *handler =
 	    kzalloc(sizeof(struct acpi_ec_query_handler), GFP_KERNEL);
+
 	if (!handler)
 		return -ENOMEM;
 
@@ -596,12 +601,12 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 	mutex_unlock(&ec->mutex);
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
+
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
@@ -611,12 +616,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 	}
 	mutex_unlock(&ec->mutex);
 }
-
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
 static void acpi_ec_run(void *cxt)
 {
 	struct acpi_ec_query_handler *handler = cxt;
+
 	if (!handler)
 		return;
 	pr_debug("##### Query(0x%02x) started #####\n", handler->query_bit);
@@ -660,6 +665,7 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 static void acpi_ec_gpe_query(void *ec_cxt)
 {
 	struct acpi_ec *ec = ec_cxt;
+
 	if (!ec)
 		return;
 	mutex_lock(&ec->mutex);
@@ -694,8 +700,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 }
 
 /* --------------------------------------------------------------------------
-                             Address Space Management
-   -------------------------------------------------------------------------- */
+ *                           Address Space Management
+ * -------------------------------------------------------------------------- */
 
 static acpi_status
 acpi_ec_space_handler(u32 function, acpi_physical_address address,
@@ -726,27 +732,26 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
 	switch (result) {
 	case -EINVAL:
 		return AE_BAD_PARAMETER;
-		break;
 	case -ENODEV:
 		return AE_NOT_FOUND;
-		break;
 	case -ETIME:
 		return AE_TIME;
-		break;
 	default:
 		return AE_OK;
 	}
 }
 
 /* --------------------------------------------------------------------------
-                               Driver Interface
-   -------------------------------------------------------------------------- */
+ *                             Driver Interface
+ * -------------------------------------------------------------------------- */
+
 static acpi_status
 ec_parse_io_ports(struct acpi_resource *resource, void *context);
 
 static struct acpi_ec *make_acpi_ec(void)
 {
 	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
+
 	if (!ec)
 		return NULL;
 	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
@@ -769,9 +774,8 @@ acpi_ec_register_query_methods(acpi_handle handle, u32 level,
 
 	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
 
-	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1) {
+	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1)
 		acpi_ec_add_query_handler(ec, value, handle, NULL, NULL);
-	}
 	return AE_OK;
 }
 
@@ -780,7 +784,6 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 {
 	acpi_status status;
 	unsigned long long tmp = 0;
-
 	struct acpi_ec *ec = context;
 
 	/* clear addr values, ec_parse_io_ports depend on it */
@@ -808,6 +811,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 static int ec_install_handlers(struct acpi_ec *ec)
 {
 	acpi_status status;
+
 	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
 		return 0;
 	status = acpi_install_gpe_handler(NULL, ec->gpe,
@@ -1105,7 +1109,8 @@ int __init acpi_ec_ecdt_probe(void)
 		boot_ec->data_addr = ecdt_ptr->data.address;
 		boot_ec->gpe = ecdt_ptr->gpe;
 		boot_ec->handle = ACPI_ROOT_OBJECT;
-		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
+		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
+				&boot_ec->handle);
 		/* Don't trust ECDT, which comes from ASUSTek */
 		if (!EC_FLAGS_VALIDATE_ECDT)
 			goto install;
@@ -1189,6 +1194,5 @@ static void __exit acpi_ec_exit(void)
 {
 
 	acpi_bus_unregister_driver(&acpi_ec_driver);
-	return;
 }
 #endif	/* 0 */
-- 
1.7.10

Re: [PATCH 5/5] ACPI/EC: Cleanup coding style.

[Rafael J. Wysocki]

On Tuesday, October 14, 2014 02:24:01 PM Lv Zheng wrote:
> This patch cleans up the following coding style issues that are detected by
> scripts/checkpatch.pl:
>  ERROR: code indent should use tabs where possible
>  ERROR: "foo * bar" should be "foo *bar"
>  WARNING: Missing a blank line after declarations
>  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
>  WARNING: void function return statements are not generally useful
>  WARNING: else is not generally useful after a break or return
>  WARNING: break is not useful after a goto or return
>  WARNING: braces {} are not necessary for single statement blocks
>  WARNING: line over 80 characters
>  WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> No functional changes.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c |   58 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index d993df6..5f4e6c6 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -128,12 +128,13 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
>  static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
>  
>  /* --------------------------------------------------------------------------
> -                             Transaction Management
> -   -------------------------------------------------------------------------- */
> + *                           Transaction Management
> + * -------------------------------------------------------------------------- */
>  
>  static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
>  {
>  	u8 x = inb(ec->command_addr);
> +
>  	pr_debug("EC_SC(R) = 0x%2.2x "
>  		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
>  		 x,
> @@ -148,6 +149,7 @@ static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
>  static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
>  {
>  	u8 x = inb(ec->data_addr);
> +
>  	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
>  	return x;
>  }
> @@ -189,6 +191,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
>  {
>  	unsigned long flags;
>  	int ret = 0;
> +
>  	spin_lock_irqsave(&ec->lock, flags);
>  	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
>  		ret = 1;
> @@ -288,6 +291,7 @@ static int ec_poll(struct acpi_ec *ec)
>  {
>  	unsigned long flags;
>  	int repeat = 5; /* number of command restarts */
> +
>  	while (repeat--) {
>  		unsigned long delay = jiffies +
>  			msecs_to_jiffies(ec_delay);
> @@ -320,6 +324,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>  {
>  	unsigned long tmp;
>  	int ret = 0;
> +
>  	if (EC_FLAGS_MSI)
>  		udelay(ACPI_EC_MSI_UDELAY);
>  	/* start transaction */
> @@ -347,6 +352,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
>  {
>  	int status;
>  	u32 glk;
> +
>  	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
>  		return -EINVAL;
>  	if (t->rdata)
> @@ -374,7 +380,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
>  	/* check if we received SCI during transaction */
>  	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
>  	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> -		msleep(1);
> +		msleep(20);

This is a functional change, so I've dropped it from the patch (it didn't apply for
me anyway).  Please send it as a separate patch with an appropriate changelog if
you want to make it.

>  		/* It is safe to enable the GPE outside of the transaction. */
>  		acpi_enable_gpe(NULL, ec->gpe, TRUE);
>  	} else if (t->irq_count > ec_storm_threshold) {
> @@ -410,7 +416,7 @@ static int acpi_ec_burst_disable(struct acpi_ec *ec)
>  				acpi_ec_transaction(ec, &t) : 0;
>  }
>  
> -static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
> +static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
>  {
>  	int result;
>  	u8 d;
> @@ -446,10 +452,9 @@ int ec_read(u8 addr, u8 *val)
>  	if (!err) {
>  		*val = temp_data;
>  		return 0;
> -	} else
> -		return err;
> +	}
> +	return err;
>  }
> -
>  EXPORT_SYMBOL(ec_read);
>  
>  int ec_write(u8 addr, u8 val)
> @@ -463,22 +468,21 @@ int ec_write(u8 addr, u8 val)
>  
>  	return err;
>  }
> -
>  EXPORT_SYMBOL(ec_write);
>  
>  int ec_transaction(u8 command,
> -		   const u8 * wdata, unsigned wdata_len,
> -		   u8 * rdata, unsigned rdata_len)
> +		   const u8 *wdata, unsigned wdata_len,
> +		   u8 *rdata, unsigned rdata_len)
>  {
>  	struct transaction t = {.command = command,
>  				.wdata = wdata, .rdata = rdata,
>  				.wlen = wdata_len, .rlen = rdata_len};
> +
>  	if (!first_ec)
>  		return -ENODEV;
>  
>  	return acpi_ec_transaction(first_ec, &t);
>  }
> -
>  EXPORT_SYMBOL(ec_transaction);
>  
>  /* Get the handle to the EC device */
> @@ -488,7 +492,6 @@ acpi_handle ec_get_handle(void)
>  		return NULL;
>  	return first_ec->handle;
>  }
> -
>  EXPORT_SYMBOL(ec_get_handle);
>  
>  /*
> @@ -552,13 +555,14 @@ void acpi_ec_unblock_transactions_early(void)
>  		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
>  }
>  
> -static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
> +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
>  {
>  	int result;
>  	u8 d;
>  	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
>  				.wdata = NULL, .rdata = &d,
>  				.wlen = 0, .rlen = 1};
> +
>  	if (!ec || !data)
>  		return -EINVAL;
>  	/*
> @@ -584,6 +588,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
>  {
>  	struct acpi_ec_query_handler *handler =
>  	    kzalloc(sizeof(struct acpi_ec_query_handler), GFP_KERNEL);
> +
>  	if (!handler)
>  		return -ENOMEM;
>  
> @@ -596,12 +601,12 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
>  	mutex_unlock(&ec->mutex);
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
>  
>  void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
>  {
>  	struct acpi_ec_query_handler *handler, *tmp;
> +
>  	mutex_lock(&ec->mutex);
>  	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
>  		if (query_bit == handler->query_bit) {
> @@ -611,12 +616,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
>  	}
>  	mutex_unlock(&ec->mutex);
>  }
> -
>  EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
>  
>  static void acpi_ec_run(void *cxt)
>  {
>  	struct acpi_ec_query_handler *handler = cxt;
> +
>  	if (!handler)
>  		return;
>  	pr_debug("##### Query(0x%02x) started #####\n", handler->query_bit);
> @@ -660,6 +665,7 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
>  static void acpi_ec_gpe_query(void *ec_cxt)
>  {
>  	struct acpi_ec *ec = ec_cxt;
> +
>  	if (!ec)
>  		return;
>  	mutex_lock(&ec->mutex);
> @@ -694,8 +700,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
>  }
>  
>  /* --------------------------------------------------------------------------
> -                             Address Space Management
> -   -------------------------------------------------------------------------- */
> + *                           Address Space Management
> + * -------------------------------------------------------------------------- */
>  
>  static acpi_status
>  acpi_ec_space_handler(u32 function, acpi_physical_address address,
> @@ -726,27 +732,26 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
>  	switch (result) {
>  	case -EINVAL:
>  		return AE_BAD_PARAMETER;
> -		break;
>  	case -ENODEV:
>  		return AE_NOT_FOUND;
> -		break;
>  	case -ETIME:
>  		return AE_TIME;
> -		break;
>  	default:
>  		return AE_OK;
>  	}
>  }
>  
>  /* --------------------------------------------------------------------------
> -                               Driver Interface
> -   -------------------------------------------------------------------------- */
> + *                             Driver Interface
> + * -------------------------------------------------------------------------- */
> +
>  static acpi_status
>  ec_parse_io_ports(struct acpi_resource *resource, void *context);
>  
>  static struct acpi_ec *make_acpi_ec(void)
>  {
>  	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
> +
>  	if (!ec)
>  		return NULL;
>  	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
> @@ -769,9 +774,8 @@ acpi_ec_register_query_methods(acpi_handle handle, u32 level,
>  
>  	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
>  
> -	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1) {
> +	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1)
>  		acpi_ec_add_query_handler(ec, value, handle, NULL, NULL);
> -	}
>  	return AE_OK;
>  }
>  
> @@ -780,7 +784,6 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  {
>  	acpi_status status;
>  	unsigned long long tmp = 0;
> -
>  	struct acpi_ec *ec = context;
>  
>  	/* clear addr values, ec_parse_io_ports depend on it */
> @@ -808,6 +811,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  static int ec_install_handlers(struct acpi_ec *ec)
>  {
>  	acpi_status status;
> +
>  	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
>  		return 0;
>  	status = acpi_install_gpe_handler(NULL, ec->gpe,
> @@ -1105,7 +1109,8 @@ int __init acpi_ec_ecdt_probe(void)
>  		boot_ec->data_addr = ecdt_ptr->data.address;
>  		boot_ec->gpe = ecdt_ptr->gpe;
>  		boot_ec->handle = ACPI_ROOT_OBJECT;
> -		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
> +		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
> +				&boot_ec->handle);
>  		/* Don't trust ECDT, which comes from ASUSTek */
>  		if (!EC_FLAGS_VALIDATE_ECDT)
>  			goto install;
> @@ -1189,6 +1194,5 @@ static void __exit acpi_ec_exit(void)
>  {
>  
>  	acpi_bus_unregister_driver(&acpi_ec_driver);
> -	return;
>  }
>  #endif	/* 0 */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 5/5] ACPI/EC: Cleanup coding style.

[Zheng, Lv]

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Tuesday, October 21, 2014 9:29 PM
> 
> On Tuesday, October 14, 2014 02:24:01 PM Lv Zheng wrote:
> > This patch cleans up the following coding style issues that are detected by
> > scripts/checkpatch.pl:
> >  ERROR: code indent should use tabs where possible
> >  ERROR: "foo * bar" should be "foo *bar"
> >  WARNING: Missing a blank line after declarations
> >  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> >  WARNING: void function return statements are not generally useful
> >  WARNING: else is not generally useful after a break or return
> >  WARNING: break is not useful after a goto or return
> >  WARNING: braces {} are not necessary for single statement blocks
> >  WARNING: line over 80 characters
> >  WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> > No functional changes.
> >
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/ec.c |   58 ++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 31 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index d993df6..5f4e6c6 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -128,12 +128,13 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
> >  static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
> >
> >  /* --------------------------------------------------------------------------
> > -                             Transaction Management
> > -   -------------------------------------------------------------------------- */
> > + *                           Transaction Management
> > + * -------------------------------------------------------------------------- */
> >
> >  static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> >  {
> >  	u8 x = inb(ec->command_addr);
> > +
> >  	pr_debug("EC_SC(R) = 0x%2.2x "
> >  		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
> >  		 x,
> > @@ -148,6 +149,7 @@ static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> >  static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
> >  {
> >  	u8 x = inb(ec->data_addr);
> > +
> >  	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
> >  	return x;
> >  }
> > @@ -189,6 +191,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
> >  {
> >  	unsigned long flags;
> >  	int ret = 0;
> > +
> >  	spin_lock_irqsave(&ec->lock, flags);
> >  	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
> >  		ret = 1;
> > @@ -288,6 +291,7 @@ static int ec_poll(struct acpi_ec *ec)
> >  {
> >  	unsigned long flags;
> >  	int repeat = 5; /* number of command restarts */
> > +
> >  	while (repeat--) {
> >  		unsigned long delay = jiffies +
> >  			msecs_to_jiffies(ec_delay);
> > @@ -320,6 +324,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
> >  {
> >  	unsigned long tmp;
> >  	int ret = 0;
> > +
> >  	if (EC_FLAGS_MSI)
> >  		udelay(ACPI_EC_MSI_UDELAY);
> >  	/* start transaction */
> > @@ -347,6 +352,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> >  {
> >  	int status;
> >  	u32 glk;
> > +
> >  	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
> >  		return -EINVAL;
> >  	if (t->rdata)
> > @@ -374,7 +380,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> >  	/* check if we received SCI during transaction */
> >  	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
> >  	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> > -		msleep(1);
> > +		msleep(20);
> 
> This is a functional change, so I've dropped it from the patch (it didn't apply for
> me anyway).  Please send it as a separate patch with an appropriate changelog if
> you want to make it.

OK. I'll split this as a separate patch.

This was just detected by scripts/checkpatch.pl as:
WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
So I was thinking this could be merged into this checkpatch.pl cleanup commit.

Thanks and best regards
-Lv


> 
> >  		/* It is safe to enable the GPE outside of the transaction. */
> >  		acpi_enable_gpe(NULL, ec->gpe, TRUE);
> >  	} else if (t->irq_count > ec_storm_threshold) {
> > @@ -410,7 +416,7 @@ static int acpi_ec_burst_disable(struct acpi_ec *ec)
> >  				acpi_ec_transaction(ec, &t) : 0;
> >  }
> >
> > -static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
> > +static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
> >  {
> >  	int result;
> >  	u8 d;
> > @@ -446,10 +452,9 @@ int ec_read(u8 addr, u8 *val)
> >  	if (!err) {
> >  		*val = temp_data;
> >  		return 0;
> > -	} else
> > -		return err;
> > +	}
> > +	return err;
> >  }
> > -
> >  EXPORT_SYMBOL(ec_read);
> >
> >  int ec_write(u8 addr, u8 val)
> > @@ -463,22 +468,21 @@ int ec_write(u8 addr, u8 val)
> >
> >  	return err;
> >  }
> > -
> >  EXPORT_SYMBOL(ec_write);
> >
> >  int ec_transaction(u8 command,
> > -		   const u8 * wdata, unsigned wdata_len,
> > -		   u8 * rdata, unsigned rdata_len)
> > +		   const u8 *wdata, unsigned wdata_len,
> > +		   u8 *rdata, unsigned rdata_len)
> >  {
> >  	struct transaction t = {.command = command,
> >  				.wdata = wdata, .rdata = rdata,
> >  				.wlen = wdata_len, .rlen = rdata_len};
> > +
> >  	if (!first_ec)
> >  		return -ENODEV;
> >
> >  	return acpi_ec_transaction(first_ec, &t);
> >  }
> > -
> >  EXPORT_SYMBOL(ec_transaction);
> >
> >  /* Get the handle to the EC device */
> > @@ -488,7 +492,6 @@ acpi_handle ec_get_handle(void)
> >  		return NULL;
> >  	return first_ec->handle;
> >  }
> > -
> >  EXPORT_SYMBOL(ec_get_handle);
> >
> >  /*
> > @@ -552,13 +555,14 @@ void acpi_ec_unblock_transactions_early(void)
> >  		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
> >  }
> >
> > -static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
> > +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
> >  {
> >  	int result;
> >  	u8 d;
> >  	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
> >  				.wdata = NULL, .rdata = &d,
> >  				.wlen = 0, .rlen = 1};
> > +
> >  	if (!ec || !data)
> >  		return -EINVAL;
> >  	/*
> > @@ -584,6 +588,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
> >  {
> >  	struct acpi_ec_query_handler *handler =
> >  	    kzalloc(sizeof(struct acpi_ec_query_handler), GFP_KERNEL);
> > +
> >  	if (!handler)
> >  		return -ENOMEM;
> >
> > @@ -596,12 +601,12 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
> >  	mutex_unlock(&ec->mutex);
> >  	return 0;
> >  }
> > -
> >  EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
> >
> >  void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
> >  {
> >  	struct acpi_ec_query_handler *handler, *tmp;
> > +
> >  	mutex_lock(&ec->mutex);
> >  	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
> >  		if (query_bit == handler->query_bit) {
> > @@ -611,12 +616,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
> >  	}
> >  	mutex_unlock(&ec->mutex);
> >  }
> > -
> >  EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
> >
> >  static void acpi_ec_run(void *cxt)
> >  {
> >  	struct acpi_ec_query_handler *handler = cxt;
> > +
> >  	if (!handler)
> >  		return;
> >  	pr_debug("##### Query(0x%02x) started #####\n", handler->query_bit);
> > @@ -660,6 +665,7 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
> >  static void acpi_ec_gpe_query(void *ec_cxt)
> >  {
> >  	struct acpi_ec *ec = ec_cxt;
> > +
> >  	if (!ec)
> >  		return;
> >  	mutex_lock(&ec->mutex);
> > @@ -694,8 +700,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
> >  }
> >
> >  /* --------------------------------------------------------------------------
> > -                             Address Space Management
> > -   -------------------------------------------------------------------------- */
> > + *                           Address Space Management
> > + * -------------------------------------------------------------------------- */
> >
> >  static acpi_status
> >  acpi_ec_space_handler(u32 function, acpi_physical_address address,
> > @@ -726,27 +732,26 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
> >  	switch (result) {
> >  	case -EINVAL:
> >  		return AE_BAD_PARAMETER;
> > -		break;
> >  	case -ENODEV:
> >  		return AE_NOT_FOUND;
> > -		break;
> >  	case -ETIME:
> >  		return AE_TIME;
> > -		break;
> >  	default:
> >  		return AE_OK;
> >  	}
> >  }
> >
> >  /* --------------------------------------------------------------------------
> > -                               Driver Interface
> > -   -------------------------------------------------------------------------- */
> > + *                             Driver Interface
> > + * -------------------------------------------------------------------------- */
> > +
> >  static acpi_status
> >  ec_parse_io_ports(struct acpi_resource *resource, void *context);
> >
> >  static struct acpi_ec *make_acpi_ec(void)
> >  {
> >  	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
> > +
> >  	if (!ec)
> >  		return NULL;
> >  	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
> > @@ -769,9 +774,8 @@ acpi_ec_register_query_methods(acpi_handle handle, u32 level,
> >
> >  	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
> >
> > -	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1) {
> > +	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1)
> >  		acpi_ec_add_query_handler(ec, value, handle, NULL, NULL);
> > -	}
> >  	return AE_OK;
> >  }
> >
> > @@ -780,7 +784,6 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> >  {
> >  	acpi_status status;
> >  	unsigned long long tmp = 0;
> > -
> >  	struct acpi_ec *ec = context;
> >
> >  	/* clear addr values, ec_parse_io_ports depend on it */
> > @@ -808,6 +811,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> >  static int ec_install_handlers(struct acpi_ec *ec)
> >  {
> >  	acpi_status status;
> > +
> >  	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
> >  		return 0;
> >  	status = acpi_install_gpe_handler(NULL, ec->gpe,
> > @@ -1105,7 +1109,8 @@ int __init acpi_ec_ecdt_probe(void)
> >  		boot_ec->data_addr = ecdt_ptr->data.address;
> >  		boot_ec->gpe = ecdt_ptr->gpe;
> >  		boot_ec->handle = ACPI_ROOT_OBJECT;
> > -		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
> > +		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
> > +				&boot_ec->handle);
> >  		/* Don't trust ECDT, which comes from ASUSTek */
> >  		if (!EC_FLAGS_VALIDATE_ECDT)
> >  			goto install;
> > @@ -1189,6 +1194,5 @@ static void __exit acpi_ec_exit(void)
> >  {
> >
> >  	acpi_bus_unregister_driver(&acpi_ec_driver);
> > -	return;
> >  }
> >  #endif	/* 0 */
> >
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/5] ACPI/EC: Cleanup coding style.

[Zheng, Lv]

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Wednesday, October 22, 2014 1:06 PM
> 
> Hi, Rafael
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > Sent: Tuesday, October 21, 2014 9:29 PM
> >
> > On Tuesday, October 14, 2014 02:24:01 PM Lv Zheng wrote:
> > > This patch cleans up the following coding style issues that are detected by
> > > scripts/checkpatch.pl:
> > >  ERROR: code indent should use tabs where possible
> > >  ERROR: "foo * bar" should be "foo *bar"
> > >  WARNING: Missing a blank line after declarations
> > >  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > >  WARNING: void function return statements are not generally useful
> > >  WARNING: else is not generally useful after a break or return
> > >  WARNING: break is not useful after a goto or return
> > >  WARNING: braces {} are not necessary for single statement blocks
> > >  WARNING: line over 80 characters
> > >  WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> > > No functional changes.
> > >
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > ---
> > >  drivers/acpi/ec.c |   58 ++++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 31 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > > index d993df6..5f4e6c6 100644
> > > --- a/drivers/acpi/ec.c
> > > +++ b/drivers/acpi/ec.c
> > > @@ -128,12 +128,13 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
> > >  static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
> > >
> > >  /* --------------------------------------------------------------------------
> > > -                             Transaction Management
> > > -   -------------------------------------------------------------------------- */
> > > + *                           Transaction Management
> > > + * -------------------------------------------------------------------------- */
> > >
> > >  static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> > >  {
> > >  	u8 x = inb(ec->command_addr);
> > > +
> > >  	pr_debug("EC_SC(R) = 0x%2.2x "
> > >  		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
> > >  		 x,
> > > @@ -148,6 +149,7 @@ static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> > >  static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
> > >  {
> > >  	u8 x = inb(ec->data_addr);
> > > +
> > >  	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
> > >  	return x;
> > >  }
> > > @@ -189,6 +191,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
> > >  {
> > >  	unsigned long flags;
> > >  	int ret = 0;
> > > +
> > >  	spin_lock_irqsave(&ec->lock, flags);
> > >  	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
> > >  		ret = 1;
> > > @@ -288,6 +291,7 @@ static int ec_poll(struct acpi_ec *ec)
> > >  {
> > >  	unsigned long flags;
> > >  	int repeat = 5; /* number of command restarts */
> > > +
> > >  	while (repeat--) {
> > >  		unsigned long delay = jiffies +
> > >  			msecs_to_jiffies(ec_delay);
> > > @@ -320,6 +324,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
> > >  {
> > >  	unsigned long tmp;
> > >  	int ret = 0;
> > > +
> > >  	if (EC_FLAGS_MSI)
> > >  		udelay(ACPI_EC_MSI_UDELAY);
> > >  	/* start transaction */
> > > @@ -347,6 +352,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> > >  {
> > >  	int status;
> > >  	u32 glk;
> > > +
> > >  	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
> > >  		return -EINVAL;
> > >  	if (t->rdata)
> > > @@ -374,7 +380,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> > >  	/* check if we received SCI during transaction */
> > >  	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
> > >  	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> > > -		msleep(1);
> > > +		msleep(20);
> >
> > This is a functional change, so I've dropped it from the patch (it didn't apply for
> > me anyway).  Please send it as a separate patch with an appropriate changelog if
> > you want to make it.
> 
> OK. I'll split this as a separate patch.

I saw the rest of the diff blocks are merged.
So since no strict requirement on doing this besides of the checkpatch.pl warning.
I'll stop doing this.

Thanks and best regards
-Lv

> 
> This was just detected by scripts/checkpatch.pl as:
> WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> So I was thinking this could be merged into this checkpatch.pl cleanup commit.
> 
> Thanks and best regards
> -Lv
> 
> 
> >
> > >  		/* It is safe to enable the GPE outside of the transaction. */
> > >  		acpi_enable_gpe(NULL, ec->gpe, TRUE);
> > >  	} else if (t->irq_count > ec_storm_threshold) {
> > > @@ -410,7 +416,7 @@ static int acpi_ec_burst_disable(struct acpi_ec *ec)
> > >  				acpi_ec_transaction(ec, &t) : 0;
> > >  }
> > >
> > > -static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
> > > +static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 *data)
> > >  {
> > >  	int result;
> > >  	u8 d;
> > > @@ -446,10 +452,9 @@ int ec_read(u8 addr, u8 *val)
> > >  	if (!err) {
> > >  		*val = temp_data;
> > >  		return 0;
> > > -	} else
> > > -		return err;
> > > +	}
> > > +	return err;
> > >  }
> > > -
> > >  EXPORT_SYMBOL(ec_read);
> > >
> > >  int ec_write(u8 addr, u8 val)
> > > @@ -463,22 +468,21 @@ int ec_write(u8 addr, u8 val)
> > >
> > >  	return err;
> > >  }
> > > -
> > >  EXPORT_SYMBOL(ec_write);
> > >
> > >  int ec_transaction(u8 command,
> > > -		   const u8 * wdata, unsigned wdata_len,
> > > -		   u8 * rdata, unsigned rdata_len)
> > > +		   const u8 *wdata, unsigned wdata_len,
> > > +		   u8 *rdata, unsigned rdata_len)
> > >  {
> > >  	struct transaction t = {.command = command,
> > >  				.wdata = wdata, .rdata = rdata,
> > >  				.wlen = wdata_len, .rlen = rdata_len};
> > > +
> > >  	if (!first_ec)
> > >  		return -ENODEV;
> > >
> > >  	return acpi_ec_transaction(first_ec, &t);
> > >  }
> > > -
> > >  EXPORT_SYMBOL(ec_transaction);
> > >
> > >  /* Get the handle to the EC device */
> > > @@ -488,7 +492,6 @@ acpi_handle ec_get_handle(void)
> > >  		return NULL;
> > >  	return first_ec->handle;
> > >  }
> > > -
> > >  EXPORT_SYMBOL(ec_get_handle);
> > >
> > >  /*
> > > @@ -552,13 +555,14 @@ void acpi_ec_unblock_transactions_early(void)
> > >  		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
> > >  }
> > >
> > > -static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
> > > +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
> > >  {
> > >  	int result;
> > >  	u8 d;
> > >  	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
> > >  				.wdata = NULL, .rdata = &d,
> > >  				.wlen = 0, .rlen = 1};
> > > +
> > >  	if (!ec || !data)
> > >  		return -EINVAL;
> > >  	/*
> > > @@ -584,6 +588,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
> > >  {
> > >  	struct acpi_ec_query_handler *handler =
> > >  	    kzalloc(sizeof(struct acpi_ec_query_handler), GFP_KERNEL);
> > > +
> > >  	if (!handler)
> > >  		return -ENOMEM;
> > >
> > > @@ -596,12 +601,12 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
> > >  	mutex_unlock(&ec->mutex);
> > >  	return 0;
> > >  }
> > > -
> > >  EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
> > >
> > >  void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
> > >  {
> > >  	struct acpi_ec_query_handler *handler, *tmp;
> > > +
> > >  	mutex_lock(&ec->mutex);
> > >  	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
> > >  		if (query_bit == handler->query_bit) {
> > > @@ -611,12 +616,12 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
> > >  	}
> > >  	mutex_unlock(&ec->mutex);
> > >  }
> > > -
> > >  EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
> > >
> > >  static void acpi_ec_run(void *cxt)
> > >  {
> > >  	struct acpi_ec_query_handler *handler = cxt;
> > > +
> > >  	if (!handler)
> > >  		return;
> > >  	pr_debug("##### Query(0x%02x) started #####\n", handler->query_bit);
> > > @@ -660,6 +665,7 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
> > >  static void acpi_ec_gpe_query(void *ec_cxt)
> > >  {
> > >  	struct acpi_ec *ec = ec_cxt;
> > > +
> > >  	if (!ec)
> > >  		return;
> > >  	mutex_lock(&ec->mutex);
> > > @@ -694,8 +700,8 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
> > >  }
> > >
> > >  /* --------------------------------------------------------------------------
> > > -                             Address Space Management
> > > -   -------------------------------------------------------------------------- */
> > > + *                           Address Space Management
> > > + * -------------------------------------------------------------------------- */
> > >
> > >  static acpi_status
> > >  acpi_ec_space_handler(u32 function, acpi_physical_address address,
> > > @@ -726,27 +732,26 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address,
> > >  	switch (result) {
> > >  	case -EINVAL:
> > >  		return AE_BAD_PARAMETER;
> > > -		break;
> > >  	case -ENODEV:
> > >  		return AE_NOT_FOUND;
> > > -		break;
> > >  	case -ETIME:
> > >  		return AE_TIME;
> > > -		break;
> > >  	default:
> > >  		return AE_OK;
> > >  	}
> > >  }
> > >
> > >  /* --------------------------------------------------------------------------
> > > -                               Driver Interface
> > > -   -------------------------------------------------------------------------- */
> > > + *                             Driver Interface
> > > + * -------------------------------------------------------------------------- */
> > > +
> > >  static acpi_status
> > >  ec_parse_io_ports(struct acpi_resource *resource, void *context);
> > >
> > >  static struct acpi_ec *make_acpi_ec(void)
> > >  {
> > >  	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
> > > +
> > >  	if (!ec)
> > >  		return NULL;
> > >  	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
> > > @@ -769,9 +774,8 @@ acpi_ec_register_query_methods(acpi_handle handle, u32 level,
> > >
> > >  	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
> > >
> > > -	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1) {
> > > +	if (ACPI_SUCCESS(status) && sscanf(node_name, "_Q%x", &value) == 1)
> > >  		acpi_ec_add_query_handler(ec, value, handle, NULL, NULL);
> > > -	}
> > >  	return AE_OK;
> > >  }
> > >
> > > @@ -780,7 +784,6 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> > >  {
> > >  	acpi_status status;
> > >  	unsigned long long tmp = 0;
> > > -
> > >  	struct acpi_ec *ec = context;
> > >
> > >  	/* clear addr values, ec_parse_io_ports depend on it */
> > > @@ -808,6 +811,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> > >  static int ec_install_handlers(struct acpi_ec *ec)
> > >  {
> > >  	acpi_status status;
> > > +
> > >  	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
> > >  		return 0;
> > >  	status = acpi_install_gpe_handler(NULL, ec->gpe,
> > > @@ -1105,7 +1109,8 @@ int __init acpi_ec_ecdt_probe(void)
> > >  		boot_ec->data_addr = ecdt_ptr->data.address;
> > >  		boot_ec->gpe = ecdt_ptr->gpe;
> > >  		boot_ec->handle = ACPI_ROOT_OBJECT;
> > > -		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
> > > +		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
> > > +				&boot_ec->handle);
> > >  		/* Don't trust ECDT, which comes from ASUSTek */
> > >  		if (!EC_FLAGS_VALIDATE_ECDT)
> > >  			goto install;
> > > @@ -1189,6 +1194,5 @@ static void __exit acpi_ec_exit(void)
> > >  {
> > >
> > >  	acpi_bus_unregister_driver(&acpi_ec_driver);
> > > -	return;
> > >  }
> > >  #endif	/* 0 */
> > >
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{�i�b�{ay�\x1dʇڙ�,j\r��f���h���z�\x1e�w���
> 
> ���j:+v���w�j�m����\r����zZ+�����ݢj"��!�i

Re: [PATCH 5/5] ACPI/EC: Cleanup coding style.

[Rafael J. Wysocki]

On Wednesday, October 22, 2014 05:59:59 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> > Sent: Wednesday, October 22, 2014 1:06 PM
> > 
> > Hi, Rafael
> > 
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > Sent: Tuesday, October 21, 2014 9:29 PM
> > >
> > > On Tuesday, October 14, 2014 02:24:01 PM Lv Zheng wrote:
> > > > This patch cleans up the following coding style issues that are detected by
> > > > scripts/checkpatch.pl:
> > > >  ERROR: code indent should use tabs where possible
> > > >  ERROR: "foo * bar" should be "foo *bar"
> > > >  WARNING: Missing a blank line after declarations
> > > >  WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > > >  WARNING: void function return statements are not generally useful
> > > >  WARNING: else is not generally useful after a break or return
> > > >  WARNING: break is not useful after a goto or return
> > > >  WARNING: braces {} are not necessary for single statement blocks
> > > >  WARNING: line over 80 characters
> > > >  WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> > > > No functional changes.
> > > >
> > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > > ---
> > > >  drivers/acpi/ec.c |   58 ++++++++++++++++++++++++++++-------------------------
> > > >  1 file changed, 31 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > > > index d993df6..5f4e6c6 100644
> > > > --- a/drivers/acpi/ec.c
> > > > +++ b/drivers/acpi/ec.c
> > > > @@ -128,12 +128,13 @@ static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
> > > >  static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */
> > > >
> > > >  /* --------------------------------------------------------------------------
> > > > -                             Transaction Management
> > > > -   -------------------------------------------------------------------------- */
> > > > + *                           Transaction Management
> > > > + * -------------------------------------------------------------------------- */
> > > >
> > > >  static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> > > >  {
> > > >  	u8 x = inb(ec->command_addr);
> > > > +
> > > >  	pr_debug("EC_SC(R) = 0x%2.2x "
> > > >  		 "SCI_EVT=%d BURST=%d CMD=%d IBF=%d OBF=%d\n",
> > > >  		 x,
> > > > @@ -148,6 +149,7 @@ static inline u8 acpi_ec_read_status(struct acpi_ec *ec)
> > > >  static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
> > > >  {
> > > >  	u8 x = inb(ec->data_addr);
> > > > +
> > > >  	pr_debug("EC_DATA(R) = 0x%2.2x\n", x);
> > > >  	return x;
> > > >  }
> > > > @@ -189,6 +191,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
> > > >  {
> > > >  	unsigned long flags;
> > > >  	int ret = 0;
> > > > +
> > > >  	spin_lock_irqsave(&ec->lock, flags);
> > > >  	if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
> > > >  		ret = 1;
> > > > @@ -288,6 +291,7 @@ static int ec_poll(struct acpi_ec *ec)
> > > >  {
> > > >  	unsigned long flags;
> > > >  	int repeat = 5; /* number of command restarts */
> > > > +
> > > >  	while (repeat--) {
> > > >  		unsigned long delay = jiffies +
> > > >  			msecs_to_jiffies(ec_delay);
> > > > @@ -320,6 +324,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
> > > >  {
> > > >  	unsigned long tmp;
> > > >  	int ret = 0;
> > > > +
> > > >  	if (EC_FLAGS_MSI)
> > > >  		udelay(ACPI_EC_MSI_UDELAY);
> > > >  	/* start transaction */
> > > > @@ -347,6 +352,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> > > >  {
> > > >  	int status;
> > > >  	u32 glk;
> > > > +
> > > >  	if (!ec || (!t) || (t->wlen && !t->wdata) || (t->rlen && !t->rdata))
> > > >  		return -EINVAL;
> > > >  	if (t->rdata)
> > > > @@ -374,7 +380,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
> > > >  	/* check if we received SCI during transaction */
> > > >  	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
> > > >  	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> > > > -		msleep(1);
> > > > +		msleep(20);
> > >
> > > This is a functional change, so I've dropped it from the patch (it didn't apply for
> > > me anyway).  Please send it as a separate patch with an appropriate changelog if
> > > you want to make it.
> > 
> > OK. I'll split this as a separate patch.
> 
> I saw the rest of the diff blocks are merged.
> So since no strict requirement on doing this besides of the checkpatch.pl warning.
> I'll stop doing this.

OK, thanks.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/5] ACPI/EC: Cleanup debugging messages and coding styles.

[Rafael J. Wysocki]

On Tuesday, October 14, 2014 02:23:33 PM Lv Zheng wrote:
> This patchset contains trivial cleanup code to make it easier to search in
> the EC debugging messages.
> It also includes a patch to correct coding style issues that detected by
> recent scripts/checkpatch.pl.
> 
> No functional changes. This patchset can only make remote debugging more
> efficient.
> 
> Lv Zheng (5):
>   ACPI/EC: Add CPU ID to debugging messages.
>   ACPI/EC: Enhance the logs to apply to QR_EC transactions.
>   ACPI/EC: Add detailed command/query debugging information.
>   ACPI/EC: Refine event/query debugging messages.
>   ACPI/EC: Cleanup coding style.
> 
>  drivers/acpi/ec.c |  109 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 70 insertions(+), 39 deletions(-)

Applied, except for one thing (please see the comment on patch [5/5]).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.