Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
Doug, On Fri, 2014-06-13 at 08:22 -0700, Doug Anderson wrote: On Fri, Jun 13, 2014 at 1:08 AM, Paul Bolle pebo...@tiscali.nl wrote: On Wed, 2014-06-11 at 08:11 -0700, Doug Anderson wrote: This is a config option on the ChromeOS EC https://chromium.googlesource.com/chromiumos/platform/ec. Doing a grep there: board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE driver/battery/samus.c:#endif /* CONFIG_CHARGER_PROFILE_OVERRIDE */ include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE include/ec_commands.h: /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */ test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE I see. So this is not a Kconfig macro but a general macro with a CONFIG_ prefix. There are quite a bit of those in the tree already, but still, would another prefix also do? Given that it's an entirely separate project and this is a valid CONFIG option in that project, it seems a lot to ask them not to use the CONFIG_ prefix. Also: the part you are objecting to is only a comment, right? So all the hits you quoted above are actually from code that's never going to be included in the kernel tree, right? If so, then yes, we're only discussing a single comment. We could certainly add extra wording in the comment to make it obvious that this is a CONFIG option for the EC and not the kernel. Would that be enough? ...or are you trying to use some scripts to automatically process files to look for CONFIG options? Yes, I'm using a script to check for Kconfig macros, among other things. It doesn't care about comments (because every now and then mistakes are made in comments too, and some of those can get surprisingly confusing). Anyhow, the CONFIG_ prefix used in the kernel tree is quite generic, but we're stuck with it. Would it be bothersome to drop it in that comment? Mentioning a preprocessor macro from a separate project is a bit confusing to begin with. How is one supposed to know that this is a reference to something out of tree? So, in summary, while we're apparently only discussing a single comment, I would appreciate it if it could be reworded, preferably by dropping that the CONFIG_ prefix. But other people might care very little, as they don't share this particular pet peeve. Thanks, Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
On 06/17/2014 02:53 AM, Paul Bolle wrote: Doug, On Fri, 2014-06-13 at 08:22 -0700, Doug Anderson wrote: On Fri, Jun 13, 2014 at 1:08 AM, Paul Bolle pebo...@tiscali.nl wrote: On Wed, 2014-06-11 at 08:11 -0700, Doug Anderson wrote: This is a config option on the ChromeOS EC https://chromium.googlesource.com/chromiumos/platform/ec. Doing a grep there: board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE driver/battery/samus.c:#endif /* CONFIG_CHARGER_PROFILE_OVERRIDE */ include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE include/ec_commands.h: /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */ test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE I see. So this is not a Kconfig macro but a general macro with a CONFIG_ prefix. There are quite a bit of those in the tree already, but still, would another prefix also do? Given that it's an entirely separate project and this is a valid CONFIG option in that project, it seems a lot to ask them not to use the CONFIG_ prefix. Also: the part you are objecting to is only a comment, right? So all the hits you quoted above are actually from code that's never going to be included in the kernel tree, right? If so, then yes, we're only discussing a single comment. We could certainly add extra wording in the comment to make it obvious that this is a CONFIG option for the EC and not the kernel. Would that be enough? ...or are you trying to use some scripts to automatically process files to look for CONFIG options? Yes, I'm using a script to check for Kconfig macros, among other things. It doesn't care about comments (because every now and then mistakes are made in comments too, and some of those can get surprisingly confusing). Anyhow, the CONFIG_ prefix used in the kernel tree is quite generic, but we're stuck with it. Would it be bothersome to drop it in that comment? Mentioning a preprocessor macro from a separate project is a bit confusing to begin with. How is one supposed to know that this is a reference to something out of tree? So, in summary, while we're apparently only discussing a single comment, I would appreciate it if it could be reworded, preferably by dropping that the CONFIG_ prefix. But other people might care very little, as they don't share this particular pet peeve. Can't your tool maintain a whitelist or ignore list? There are many cases where the kernel can pull in headers/data from other projects (Firmware interfaces to an arbitrarily large set of HW, Device trees, IO/network protocools, perhaps more). It feels quite unreasonable for the kernel to decide that it exclusively owns the CONFIG_* namespace even in comments, and that every other project it interacts with must not use that namespace. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
On Tue, 2014-06-17 at 10:20 -0600, Stephen Warren wrote: On 06/17/2014 02:53 AM, Paul Bolle wrote: So, in summary, while we're apparently only discussing a single comment, I would appreciate it if it could be reworded, preferably by dropping that the CONFIG_ prefix. But other people might care very little, as they don't share this particular pet peeve. Can't your tool maintain a whitelist or ignore list? Sure it can. But I do think I should try to fix the (in my view, at least) problems I find before adding stuff to a whitelist or (whatever). There are many cases where the kernel can pull in headers/data from other projects (Firmware interfaces to an arbitrarily large set of HW, Device trees, IO/network protocools, perhaps more). It feels quite unreasonable for the kernel to decide that it exclusively owns the CONFIG_* namespace even in comments, and that every other project it interacts with must not use that namespace. As I said, this is more my peeve. Then again, referring to a macro from some other project is likely to confuse people. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
Paul,
On Wed, Jun 11, 2014 at 3:37 AM, Paul Bolle pebo...@tiscali.nl wrote:
On Tue, 2014-05-20 at 09:46 +0100, Lee Jones wrote:
On Wed, 30 Apr 2014, Doug Anderson wrote:
From: Bill Richardson wfric...@chromium.org
This just updates include/linux/mfd/cros_ec_commands.h to match the
latest EC version (which is the One True Source for such things). See
https://chromium.googlesource.com/chromiumos/platform/ec
I believe most of your questions are answered by checking out the git
tree referenced above. ...but see below for details. This header is
a common interface between the kernel and the EC.
[dianders: took today's ToT version from the Chromium OS EC; deleted
references to cros_ec_dev and cros_ec_lpc since those aren't upstream
yet]
Signed-off-by: Bill Richardson wfric...@chromium.org
Signed-off-by: Doug Anderson diand...@chromium.org
Acked-by: Lee Jones lee.jo...@linaro.org
Reviewed-by: Simon Glass s...@chromium.org
Tested-by: Andrew Bresticker abres...@chromium.org
Tested-by: Stephen Warren swar...@nvidia.com
---
Changes in v3: None
Changes in v2: None
drivers/mfd/cros_ec.c|2 +-
include/linux/mfd/cros_ec.h |4 +-
include/linux/mfd/cros_ec_commands.h | 1128
+++---
3 files changed, 1059 insertions(+), 75 deletions(-)
Applied, thanks.
This patch is included in linux-next since next-20140521. I'm not sure
why I waited three weeks before sending this question...
[...]
diff --git a/include/linux/mfd/cros_ec_commands.h
b/include/linux/mfd/cros_ec_commands.h
index 86fd069..7853a64 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
[...]
@@ -1206,14 +1865,231 @@ struct ec_params_force_idle {
[...]
+/*
+ * Known param numbers are defined here. Ranges are reserved for
board-specific
+ * params, which are handled by the particular implementations.
+ */
+enum charge_state_params {
+ CS_PARAM_CHG_VOLTAGE, /* charger voltage limit */
+ CS_PARAM_CHG_CURRENT, /* charger current limit */
+ CS_PARAM_CHG_INPUT_CURRENT, /* charger input current limit */
+ CS_PARAM_CHG_STATUS, /* charger-specific status */
+ CS_PARAM_CHG_OPTION, /* charger-specific options */
+ /* How many so far? */
+ CS_NUM_BASE_PARAMS,
+
+ /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */
CONFIG_CHARGER_PROFILE_OVERRIDE doesn't match anything in linux-next. Is
a Kconfig symbol CHARGER_PROFILE_OVERRIDE perhaps queued somewhere?
This is a config option on the ChromeOS EC
https://chromium.googlesource.com/chromiumos/platform/ec. Doing a
grep there:
board/samus/board.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
common/charge_state_v2.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
driver/battery/samus.c:#ifdef CONFIG_CHARGER_PROFILE_OVERRIDE
driver/battery/samus.c:#endif /* CONFIG_CHARGER_PROFILE_OVERRIDE */
include/config.h:#undef CONFIG_CHARGER_PROFILE_OVERRIDE
include/ec_commands.h: /* Range for CONFIG_CHARGER_PROFILE_OVERRIDE params */
test/test_config.h:#define CONFIG_CHARGER_PROFILE_OVERRIDE
+ CS_PARAM_CUSTOM_PROFILE_MIN = 0x1,
+ CS_PARAM_CUSTOM_PROFILE_MAX = 0x1,
+
I guess so, because these two constants aren't used anywhere.
A grep from the EC sources:
common/charge_state_v2.c: if (in-get_param.param =
CS_PARAM_CUSTOM_PROFILE_MIN
common/charge_state_v2.c: if (in-set_param.param =
CS_PARAM_CUSTOM_PROFILE_MIN
driver/battery/samus.c:#define PARAM_FASTCHARGE
(CS_PARAM_CUSTOM_PROFILE_MIN + 0)
include/ec_commands.h: CS_PARAM_CUSTOM_PROFILE_MIN = 0x1,
test/sbs_charging_v2.c: if (param == CS_PARAM_CUSTOM_PROFILE_MIN) {
test/sbs_charging_v2.c: if (param == CS_PARAM_CUSTOM_PROFILE_MIN) {
test/sbs_charging_v2.c: params.get_param.param = CS_PARAM_CUSTOM_PROFILE_MIN;
test/sbs_charging_v2.c: params.set_param.param = CS_PARAM_CUSTOM_PROFILE_MIN;
util/ectool.c: printf( 0x%x - 0x%x\n, CS_PARAM_CUSTOM_PROFILE_MIN,
-Doug
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources
On Wed, 30 Apr 2014, Doug Anderson wrote:
From: Bill Richardson wfric...@chromium.org
This just updates include/linux/mfd/cros_ec_commands.h to match the
latest EC version (which is the One True Source for such things). See
https://chromium.googlesource.com/chromiumos/platform/ec
[dianders: took today's ToT version from the Chromium OS EC; deleted
references to cros_ec_dev and cros_ec_lpc since those aren't upstream
yet]
Signed-off-by: Bill Richardson wfric...@chromium.org
Signed-off-by: Doug Anderson diand...@chromium.org
Acked-by: Lee Jones lee.jo...@linaro.org
Reviewed-by: Simon Glass s...@chromium.org
Tested-by: Andrew Bresticker abres...@chromium.org
Tested-by: Stephen Warren swar...@nvidia.com
---
Changes in v3: None
Changes in v2: None
drivers/mfd/cros_ec.c|2 +-
include/linux/mfd/cros_ec.h |4 +-
include/linux/mfd/cros_ec_commands.h | 1128
+++---
3 files changed, 1059 insertions(+), 75 deletions(-)
Applied, thanks.
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 783fe2e..c58ab96 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -30,7 +30,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
uint8_t *out;
int csum, i;
- BUG_ON(msg-out_len EC_HOST_PARAM_SIZE);
+ BUG_ON(msg-out_len EC_PROTO2_MAX_PARAM_SIZE);
out = ec_dev-dout;
out[0] = EC_CMD_VERSION0 + msg-version;
out[1] = msg-cmd;
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 032af7f..887ef4f 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -29,8 +29,8 @@ enum {
EC_MSG_RX_PROTO_BYTES = 3,
/* Max length of messages */
- EC_MSG_BYTES= EC_HOST_PARAM_SIZE + EC_MSG_TX_PROTO_BYTES,
-
+ EC_MSG_BYTES= EC_PROTO2_MAX_PARAM_SIZE +
+ EC_MSG_TX_PROTO_BYTES,
};
/**
diff --git a/include/linux/mfd/cros_ec_commands.h
b/include/linux/mfd/cros_ec_commands.h
index 86fd069..7853a64 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -24,25 +24,12 @@
#define __CROS_EC_COMMANDS_H
/*
- * Protocol overview
+ * Current version of this protocol
*
- * request: CMD [ P0 P1 P2 ... Pn S ]
- * response: ERR [ P0 P1 P2 ... Pn S ]
- *
- * where the bytes are defined as follow :
- * - CMD is the command code. (defined by EC_CMD_ constants)
- * - ERR is the error code. (defined by EC_RES_ constants)
- * - Px is the optional payload.
- *it is not sent if the error code is not success.
- *(defined by ec_params_ and ec_response_ structures)
- * - S is the checksum which is the sum of all payload bytes.
- *
- * On LPC, CMD and ERR are sent/received at EC_LPC_ADDR_KERNEL|USER_CMD
- * and the payloads are sent/received at EC_LPC_ADDR_KERNEL|USER_PARAM.
- * On I2C, all bytes are sent serially in the same message.
+ * TODO(crosbug.com/p/11223): This is effectively useless; protocol is
+ * determined in other ways. Remove this once the kernel code no longer
+ * depends on it.
*/
-
-/* Current version of this protocol */
#define EC_PROTO_VERSION 0x0002
/* Command version mask */
@@ -57,13 +44,19 @@
#define EC_LPC_ADDR_HOST_CMD 0x204
/* I/O addresses for host command args and params */
-#define EC_LPC_ADDR_HOST_ARGS 0x800
-#define EC_LPC_ADDR_HOST_PARAM 0x804
-#define EC_HOST_PARAM_SIZE 0x0fc /* Size of param area in bytes */
-
-/* I/O addresses for host command params, old interface */
-#define EC_LPC_ADDR_OLD_PARAM 0x880
-#define EC_OLD_PARAM_SIZE 0x080 /* Size of param area in bytes */
+/* Protocol version 2 */
+#define EC_LPC_ADDR_HOST_ARGS0x800 /* And 0x801, 0x802, 0x803 */
+#define EC_LPC_ADDR_HOST_PARAM 0x804 /* For version 2 params; size is
+ * EC_PROTO2_MAX_PARAM_SIZE */
+/* Protocol version 3 */
+#define EC_LPC_ADDR_HOST_PACKET 0x800 /* Offset of version 3 packet */
+#define EC_LPC_HOST_PACKET_SIZE 0x100 /* Max size of version 3 packet */
+
+/* The actual block is 0x800-0x8ff, but some BIOSes think it's 0x880-0x8ff
+ * and they tell the kernel that so we have to think of it as two parts. */
+#define EC_HOST_CMD_REGION00x800
+#define EC_HOST_CMD_REGION10x880
+#define EC_HOST_CMD_REGION_SIZE 0x80
/* EC command register bit functions */
#define EC_LPC_CMDR_DATA (1 0) /* Data ready for host to read */
@@ -79,18 +72,22 @@
#define EC_MEMMAP_TEXT_MAX 8 /* Size of a string in the memory map */
/* The offset address of each type of data in mapped memory. */
-#define EC_MEMMAP_TEMP_SENSOR 0x00 /* Temp sensors */
-#define EC_MEMMAP_FAN 0x10 /* Fan speeds */
-#define EC_MEMMAP_TEMP_SENSOR_B0x18 /* Temp sensors (second set) */
-#define EC_MEMMAP_ID
