* [PATCH v2 01/10] mfd: cros_ec: Remove parent field
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 02/10] platform/chrome: cros_ec_lpc - Use existing function to check EC result Javier Martinez Canillas
` (9 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel, Gwendal Grignou,
Javier Martinez Canillas
From: Gwendal Grignou <gwendal@chromium.org>
parent and dev were pointing to the same device structure.
parent is unused, removed.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Puthikorn Voravootivat <puthik@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Changes since v1:
- Added Heiko Stuebner and Gwendal Grignou Tested-by tag
- Added Lee Jones Acked-by tag
- Use cros_ec instead of cros ec in the subject line to be consistent.
Suggested by Gwendal Grignou
---
drivers/mfd/cros_ec_i2c.c | 1 -
drivers/mfd/cros_ec_spi.c | 1 -
drivers/platform/chrome/cros_ec_lpc.c | 1 -
include/linux/mfd/cros_ec.h | 2 --
4 files changed, 5 deletions(-)
diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index c0c30f4f946f..82b4d6148698 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -145,7 +145,6 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
ec_dev->ec_name = client->name;
ec_dev->phys_name = client->adapter->name;
- ec_dev->parent = &client->dev;
err = cros_ec_register(ec_dev);
if (err) {
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index bf6e08e8013e..27bd52e5e8b7 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -363,7 +363,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
ec_dev->ec_name = ec_spi->spi->modalias;
ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
- ec_dev->parent = &ec_spi->spi->dev;
ec_dev->din_size = EC_MSG_BYTES + EC_MSG_PREAMBLE_COUNT;
ec_dev->dout_size = EC_MSG_BYTES;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 8f9ac4d7bbd0..860310513cf0 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -214,7 +214,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
ec_dev->dev = dev;
ec_dev->ec_name = pdev->name;
ec_dev->phys_name = dev_name(dev);
- ec_dev->parent = dev;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
ec_dev->cmd_readmem = cros_ec_lpc_readmem;
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 324a34683971..14cf522123dd 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -85,7 +85,6 @@ struct cros_ec_command {
* to using dword.
* @din_size: size of din buffer to allocate (zero to use static din)
* @dout_size: size of dout buffer to allocate (zero to use static dout)
- * @parent: pointer to parent device (e.g. i2c or spi device)
* @wake_enabled: true if this device can wake the system from sleep
* @cmd_xfer: send command to EC and get response
* Returns the number of bytes received if the communication succeeded, but
@@ -113,7 +112,6 @@ struct cros_ec_device {
uint8_t *dout;
int din_size;
int dout_size;
- struct device *parent;
bool wake_enabled;
int (*cmd_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 02/10] platform/chrome: cros_ec_lpc - Use existing function to check EC result
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 01/10] mfd: cros_ec: Remove parent field Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree Javier Martinez Canillas
` (8 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
Commit 6db07b633658 ("mfd: cros_ec: Check result code from EC messages")
added a common cros_ec_check_result() function that can be used to check
the ec_msg->result for errors and warns about them.
Use the existing function instead of duplicating same check in the driver.
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes since v1:
- Added Gwendal Grignou Reviewed and Tested-by tags
---
drivers/platform/chrome/cros_ec_lpc.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 860310513cf0..3a675817c95d 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -95,19 +95,9 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
/* Check result */
msg->result = inb(EC_LPC_ADDR_HOST_DATA);
-
- switch (msg->result) {
- case EC_RES_SUCCESS:
- break;
- case EC_RES_IN_PROGRESS:
- ret = -EAGAIN;
- dev_dbg(ec->dev, "command 0x%02x in progress\n",
- msg->command);
+ ret = cros_ec_check_result(ec, msg);
+ if (ret)
goto done;
- default:
- dev_dbg(ec->dev, "command 0x%02x returned %d\n",
- msg->command, msg->result);
- }
/* Read back args */
args.flags = inb(EC_LPC_ADDR_HOST_ARGS);
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 01/10] mfd: cros_ec: Remove parent field Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 02/10] platform/chrome: cros_ec_lpc - Use existing function to check EC result Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-13 11:32 ` Lee Jones
2015-05-09 10:10 ` [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data Javier Martinez Canillas
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
From: Todd Broch <tbroch@chromium.org>
If the EC device tree node has sub-nodes, try to instantiate them as
MFD sub-devices. We can configure the EC features provided by the board.
Signed-off-by: Todd Broch <tbroch@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes since v1:
- Added Heiko Stuebner and Gwendal Grignou Tested-by tag
- Added Gwendal Grignou Reviewed-by tag
- Use automatic device ID instead of 1 as suggested by Lee Jones
- Remove #ifdeffery and check for of_node to register sub-devices
Suggested by Lee Jones
---
drivers/mfd/cros_ec.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c4aecc6f8373..1574a9352a6d 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -17,6 +17,7 @@
* battery charging and regulator control, firmware update.
*/
+#include <linux/of_platform.h>
#include <linux/interrupt.h>
#include <linux/slab.h>
#include <linux/module.h>
@@ -109,18 +110,8 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
static const struct mfd_cell cros_devs[] = {
{
- .name = "cros-ec-keyb",
- .id = 1,
- .of_compatible = "google,cros-ec-keyb",
- },
- {
- .name = "cros-ec-i2c-tunnel",
- .id = 2,
- .of_compatible = "google,cros-ec-i2c-tunnel",
- },
- {
.name = "cros-ec-ctl",
- .id = 3,
+ .id = PLATFORM_DEVID_AUTO,
},
};
@@ -150,6 +141,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
return err;
}
+ if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+ err = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (err) {
+ mfd_remove_devices(dev);
+ dev_err(dev, "Failed to register sub-devices\n");
+ return err;
+ }
+ }
+
dev_info(dev, "Chrome EC device registered\n");
return 0;
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree
2015-05-09 10:10 ` [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree Javier Martinez Canillas
@ 2015-05-13 11:32 ` Lee Jones
2015-05-13 11:43 ` Javier Martinez Canillas
0 siblings, 1 reply; 29+ messages in thread
From: Lee Jones @ 2015-05-13 11:32 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> From: Todd Broch <tbroch@chromium.org>
>
> If the EC device tree node has sub-nodes, try to instantiate them as
> MFD sub-devices. We can configure the EC features provided by the board.
>
> Signed-off-by: Todd Broch <tbroch@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> Tested-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>
> Changes since v1:
> - Added Heiko Stuebner and Gwendal Grignou Tested-by tag
> - Added Gwendal Grignou Reviewed-by tag
> - Use automatic device ID instead of 1 as suggested by Lee Jones
> - Remove #ifdeffery and check for of_node to register sub-devices
> Suggested by Lee Jones
> ---
> drivers/mfd/cros_ec.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index c4aecc6f8373..1574a9352a6d 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -17,6 +17,7 @@
> * battery charging and regulator control, firmware update.
> */
>
> +#include <linux/of_platform.h>
> #include <linux/interrupt.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> @@ -109,18 +110,8 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
>
> static const struct mfd_cell cros_devs[] = {
> {
> - .name = "cros-ec-keyb",
> - .id = 1,
> - .of_compatible = "google,cros-ec-keyb",
> - },
> - {
> - .name = "cros-ec-i2c-tunnel",
> - .id = 2,
> - .of_compatible = "google,cros-ec-i2c-tunnel",
> - },
> - {
> .name = "cros-ec-ctl",
> - .id = 3,
> + .id = PLATFORM_DEVID_AUTO,
> },
> };
>
> @@ -150,6 +141,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> return err;
> }
>
> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
You don't need to check for OF. of_node will be NULL if OF isn't
enabled.
> + err = of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (err) {
> + mfd_remove_devices(dev);
> + dev_err(dev, "Failed to register sub-devices\n");
> + return err;
> + }
> + }
> +
> dev_info(dev, "Chrome EC device registered\n");
>
> return 0;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree
2015-05-13 11:32 ` Lee Jones
@ 2015-05-13 11:43 ` Javier Martinez Canillas
2015-05-13 12:10 ` Lee Jones
0 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-13 11:43 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
Hello Lee,
On 05/13/2015 01:32 PM, Lee Jones wrote:
> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
>
>> From: Todd Broch <tbroch@chromium.org>
>>
>> If the EC device tree node has sub-nodes, try to instantiate them as
>> MFD sub-devices. We can configure the EC features provided by the board.
>>
>> Signed-off-by: Todd Broch <tbroch@chromium.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
>> Tested-by: Gwendal Grignou <gwendal@chromium.org>
>> ---
>>
>> Changes since v1:
>> - Added Heiko Stuebner and Gwendal Grignou Tested-by tag
>> - Added Gwendal Grignou Reviewed-by tag
>> - Use automatic device ID instead of 1 as suggested by Lee Jones
>> - Remove #ifdeffery and check for of_node to register sub-devices
>> Suggested by Lee Jones
>> ---
>> drivers/mfd/cros_ec.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index c4aecc6f8373..1574a9352a6d 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -17,6 +17,7 @@
>> * battery charging and regulator control, firmware update.
>> */
>>
>> +#include <linux/of_platform.h>
>> #include <linux/interrupt.h>
>> #include <linux/slab.h>
>> #include <linux/module.h>
>> @@ -109,18 +110,8 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
>>
>> static const struct mfd_cell cros_devs[] = {
>> {
>> - .name = "cros-ec-keyb",
>> - .id = 1,
>> - .of_compatible = "google,cros-ec-keyb",
>> - },
>> - {
>> - .name = "cros-ec-i2c-tunnel",
>> - .id = 2,
>> - .of_compatible = "google,cros-ec-i2c-tunnel",
>> - },
>> - {
>> .name = "cros-ec-ctl",
>> - .id = 3,
>> + .id = PLATFORM_DEVID_AUTO,
>> },
>> };
>>
>> @@ -150,6 +141,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> return err;
>> }
>>
>> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>
> You don't need to check for OF. of_node will be NULL if OF isn't
> enabled.
>
Yes, you don't need it but IS_ENABLED(CONFIG_OF) causes the check to be
optimized away by the compiler if CONFIG_OF is not enabled AFAIK.
Without the IS_ENABLED(CONFIG_OF) check, this becomes a pointless pointer
check that will always evaluate to false on systems without CONFIG_OF
since as you said of_node will be present when CONFIG_OF is not enabled.
But the compiler has no way to know it will always be NULL to optimize it
away AFAICT.
>> + err = of_platform_populate(dev->of_node, NULL, NULL, dev);
>> + if (err) {
>> + mfd_remove_devices(dev);
>> + dev_err(dev, "Failed to register sub-devices\n");
>> + return err;
>> + }
>> + }
>> +
>> dev_info(dev, "Chrome EC device registered\n");
>>
>> return 0;
>
Best regards,
Javier
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree
2015-05-13 11:43 ` Javier Martinez Canillas
@ 2015-05-13 12:10 ` Lee Jones
0 siblings, 0 replies; 29+ messages in thread
From: Lee Jones @ 2015-05-13 12:10 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
On Wed, 13 May 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 05/13/2015 01:32 PM, Lee Jones wrote:
> > On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> >
> >> From: Todd Broch <tbroch@chromium.org>
> >>
> >> If the EC device tree node has sub-nodes, try to instantiate them as
> >> MFD sub-devices. We can configure the EC features provided by the board.
> >>
> >> Signed-off-by: Todd Broch <tbroch@chromium.org>
> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> >> Tested-by: Heiko Stuebner <heiko@sntech.de>
> >> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> >> Tested-by: Gwendal Grignou <gwendal@chromium.org>
> >> ---
> >>
> >> Changes since v1:
> >> - Added Heiko Stuebner and Gwendal Grignou Tested-by tag
> >> - Added Gwendal Grignou Reviewed-by tag
> >> - Use automatic device ID instead of 1 as suggested by Lee Jones
> >> - Remove #ifdeffery and check for of_node to register sub-devices
> >> Suggested by Lee Jones
> >> ---
> >> drivers/mfd/cros_ec.c | 22 +++++++++++-----------
> >> 1 file changed, 11 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index c4aecc6f8373..1574a9352a6d 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c
> >> @@ -17,6 +17,7 @@
> >> * battery charging and regulator control, firmware update.
> >> */
> >>
> >> +#include <linux/of_platform.h>
> >> #include <linux/interrupt.h>
> >> #include <linux/slab.h>
> >> #include <linux/module.h>
> >> @@ -109,18 +110,8 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >>
> >> static const struct mfd_cell cros_devs[] = {
> >> {
> >> - .name = "cros-ec-keyb",
> >> - .id = 1,
> >> - .of_compatible = "google,cros-ec-keyb",
> >> - },
> >> - {
> >> - .name = "cros-ec-i2c-tunnel",
> >> - .id = 2,
> >> - .of_compatible = "google,cros-ec-i2c-tunnel",
> >> - },
> >> - {
> >> .name = "cros-ec-ctl",
> >> - .id = 3,
> >> + .id = PLATFORM_DEVID_AUTO,
> >> },
> >> };
> >>
> >> @@ -150,6 +141,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> return err;
> >> }
> >>
> >> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> >
> > You don't need to check for OF. of_node will be NULL if OF isn't
> > enabled.
> >
>
> Yes, you don't need it but IS_ENABLED(CONFIG_OF) causes the check to be
> optimized away by the compiler if CONFIG_OF is not enabled AFAIK.
>
> Without the IS_ENABLED(CONFIG_OF) check, this becomes a pointless pointer
> check that will always evaluate to false on systems without CONFIG_OF
> since as you said of_node will be present when CONFIG_OF is not enabled.
> But the compiler has no way to know it will always be NULL to optimize it
> away AFAICT.
Yes, you're absolutely right. You passed the test. ;)
Acked-by: Lee Jones <lee.jones@linaro.org>
> >> + err = of_platform_populate(dev->of_node, NULL, NULL, dev);
> >> + if (err) {
> >> + mfd_remove_devices(dev);
> >> + dev_err(dev, "Failed to register sub-devices\n");
> >> + return err;
> >> + }
> >> + }
> >> +
> >> dev_info(dev, "Chrome EC device registered\n");
> >>
> >> return 0;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (2 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 03/10] mfd: cros_ec: Instantiate sub-devices from device tree Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-11 20:19 ` Gwendal Grignou
2015-05-13 11:10 ` Lee Jones
2015-05-09 10:10 ` [PATCH v2 05/10] mfd: cros_ec: rev cros_ec_commands.h Javier Martinez Canillas
` (6 subsequent siblings)
10 siblings, 2 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
data with the EC") modified the struct cros_ec_command fields to not
use pointers for the input and output buffers and use fixed length
arrays instead.
This change was made because the cros_ec ioctl API uses that struct
cros_ec_command to allow user-space to send commands to the EC and
to get data from the EC. So using pointers made the API not 64-bit
safe. Unfortunately this approach was not flexible enough for all
the use-cases since there may be a need to send larger commands
on newer versions of the EC command protocol.
So to avoid to choose a constant length that it may be too big for
most commands and thus wasting memory and CPU cycles on copy from
and to user-space or having a size that is too small for some big
commands, use a zero-length array that is both 64-bit safe and
flexible. The same buffer is used for both output and input data
so the maximum of these values should be used to allocate it.
Suggested-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Heiko Stuebner <heiko@sntech.de>
---
Changes since v1:
- Add Heiko Stuebner Tested-by tag
- Removed a new blank line at EOF warning. Reported by Heiko Stuebner
- Use kmalloc instead of kzalloc when the message is later initialized
Suggested by Gwendal Grignou
- Pre-allocate struct cros_ec_command instead of dynamically allocate it
whenever is possible. Suggested by Gwendal Grignou
- Pre-allocate buffers for the usual cases and only allocate dynamically
in the heap for bigger sizes. Suggested by Gwendal Grignou
- Don't access the cros_ec_command received from user-space before doing
a copy_from_user. Suggested by Gwendal Grignou
- Only copy from user-space outsize bytes and copy_to_user insize bytes
Suggested by Gwendal Grignou
- ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
on success. Suggested by Gwendal Grignou
- Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
---
drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
drivers/mfd/cros_ec.c | 18 ++--
drivers/mfd/cros_ec_i2c.c | 4 +-
drivers/mfd/cros_ec_spi.c | 2 +-
drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
drivers/platform/chrome/cros_ec_lpc.c | 8 +-
drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
include/linux/mfd/cros_ec.h | 6 +-
10 files changed, 273 insertions(+), 153 deletions(-)
diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index fa8dedd8c3a2..30cefcd02bbf 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -17,6 +17,12 @@
#include <linux/slab.h>
#define I2C_MAX_RETRIES 3
+/*
+ * I2C commands are generally very small so 32 bytes is in
+ * most cases enough to hold the EC command headers and
+ * payload. Pre-allocate a buffer for these common cases.
+ */
+#define PREALLOC_SIZE 32
/**
* struct ec_i2c_device - Driver data for I2C tunnel
@@ -182,8 +188,10 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
const u16 bus_num = bus->remote_bus;
int request_len;
int response_len;
+ int alloc_size;
int result;
- struct cros_ec_command msg = { };
+ struct cros_ec_command *msg;
+ u8 buf[sizeof(*msg) + PREALLOC_SIZE];
request_len = ec_i2c_count_message(i2c_msgs, num);
if (request_len < 0) {
@@ -198,25 +206,46 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
return response_len;
}
- result = ec_i2c_construct_message(msg.outdata, i2c_msgs, num, bus_num);
- if (result)
- return result;
+ alloc_size = max(request_len, response_len);
+
+ if (alloc_size > PREALLOC_SIZE) {
+ msg = kmalloc(sizeof(*msg) + alloc_size, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+ } else {
+ msg = (struct cros_ec_command *)buf;
+ alloc_size = 0;
+ }
+
+ result = ec_i2c_construct_message(msg->data, i2c_msgs, num, bus_num);
+ if (result) {
+ dev_err(dev, "Error constructing EC i2c message %d\n", result);
+ goto exit;
+ }
- msg.version = 0;
- msg.command = EC_CMD_I2C_PASSTHRU;
- msg.outsize = request_len;
- msg.insize = response_len;
+ msg->version = 0;
+ msg->command = EC_CMD_I2C_PASSTHRU;
+ msg->outsize = request_len;
+ msg->insize = response_len;
- result = cros_ec_cmd_xfer(bus->ec, &msg);
- if (result < 0)
- return result;
+ result = cros_ec_cmd_xfer(bus->ec, msg);
+ if (result < 0) {
+ dev_err(dev, "Error transferring EC i2c message %d\n", result);
+ goto exit;
+ }
- result = ec_i2c_parse_response(msg.indata, i2c_msgs, &num);
- if (result < 0)
- return result;
+ result = ec_i2c_parse_response(msg->data, i2c_msgs, &num);
+ if (result < 0) {
+ dev_err(dev, "Error parsing EC i2c message %d\n", result);
+ goto exit;
+ }
/* Indicate success by saying how many messages were sent */
- return num;
+ result = num;
+exit:
+ if (alloc_size)
+ kfree(msg);
+ return result;
}
static u32 ec_i2c_functionality(struct i2c_adapter *adap)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index b50c5b8b8a4d..090f8a75412a 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -149,16 +149,19 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
{
int ret;
- struct cros_ec_command msg = {
- .command = EC_CMD_MKBP_STATE,
- .insize = ckdev->cols,
- };
+ struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
- ret = cros_ec_cmd_xfer(ckdev->ec, &msg);
- if (ret < 0)
+ msg->command = EC_CMD_MKBP_STATE;
+ msg->insize = ckdev->cols;
+ msg->outsize = 0;
+
+ ret = cros_ec_cmd_xfer(ckdev->ec, msg);
+ if (ret < 0) {
+ dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
return ret;
+ }
- memcpy(kb_state, msg.indata, ckdev->cols);
+ memcpy(kb_state, msg->data, ckdev->cols);
return 0;
}
@@ -168,7 +171,7 @@ static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
struct cros_ec_keyb *ckdev = data;
struct cros_ec_device *ec = ckdev->ec;
int ret;
- uint8_t kb_state[ckdev->cols];
+ uint8_t kb_state[sizeof(struct cros_ec_command) + ckdev->cols];
if (device_may_wakeup(ec->dev))
pm_wakeup_event(ec->dev, 0);
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 1574a9352a6d..ee8aa8142932 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
out[2] = msg->outsize;
csum = out[0] + out[1] + out[2];
for (i = 0; i < msg->outsize; i++)
- csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
+ csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
return EC_MSG_TX_PROTO_BYTES + msg->outsize;
@@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
ret = ec_dev->cmd_xfer(ec_dev, msg);
if (msg->result == EC_RES_IN_PROGRESS) {
int i;
- struct cros_ec_command status_msg = { };
+ struct cros_ec_command *status_msg;
struct ec_response_get_comms_status *status;
+ u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
- status_msg.command = EC_CMD_GET_COMMS_STATUS;
- status_msg.insize = sizeof(*status);
+ status_msg = (struct cros_ec_command *)buf;
+ status_msg->command = EC_CMD_GET_COMMS_STATUS;
+ status_msg->insize = sizeof(*status);
/*
* Query the EC's status until it's no longer busy or
@@ -88,16 +90,16 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
for (i = 0; i < EC_COMMAND_RETRIES; i++) {
usleep_range(10000, 11000);
- ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
+ ret = ec_dev->cmd_xfer(ec_dev, status_msg);
if (ret < 0)
break;
- msg->result = status_msg.result;
- if (status_msg.result != EC_RES_SUCCESS)
+ msg->result = status_msg->result;
+ if (status_msg->result != EC_RES_SUCCESS)
break;
status = (struct ec_response_get_comms_status *)
- status_msg.indata;
+ status_msg->data;
if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
break;
}
diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index 82b4d6148698..fbf7819f5de5 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -76,7 +76,7 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
/* copy message payload and compute checksum */
sum = out_buf[0] + out_buf[1] + out_buf[2];
for (i = 0; i < msg->outsize; i++) {
- out_buf[3 + i] = msg->outdata[i];
+ out_buf[3 + i] = msg->data[i];
sum += out_buf[3 + i];
}
out_buf[3 + msg->outsize] = sum;
@@ -109,7 +109,7 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
/* copy response packet payload and compute checksum */
sum = in_buf[0] + in_buf[1];
for (i = 0; i < len; i++) {
- msg->indata[i] = in_buf[2 + i];
+ msg->data[i] = in_buf[2 + i];
sum += in_buf[2 + i];
}
dev_dbg(ec_dev->dev, "packet: %*ph, sum = %02x\n",
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 27bd52e5e8b7..573730fec947 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -299,7 +299,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
for (i = 0; i < len; i++) {
sum += ptr[i + 2];
if (ec_msg->insize)
- ec_msg->indata[i] = ptr[i + 2];
+ ec_msg->data[i] = ptr[i + 2];
}
sum &= 0xff;
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 6090d0b2826f..7f7e3db59234 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -20,12 +20,15 @@
#include <linux/fs.h>
#include <linux/module.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
#include <linux/uaccess.h>
#include "cros_ec_dev.h"
/* Device variables */
#define CROS_MAX_DEV 128
+/* Most EC commands size is ~64 bytes so allocate a buffer for the usual case */
+#define PREALLOC_SIZE 64
static struct class *cros_class;
static int ec_major;
@@ -36,28 +39,28 @@ static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
static const char * const current_image_name[] = {
"unknown", "read-only", "read-write", "invalid",
};
- struct cros_ec_command msg = {
- .version = 0,
- .command = EC_CMD_GET_VERSION,
- .outdata = { 0 },
- .outsize = 0,
- .indata = { 0 },
- .insize = sizeof(*resp),
- };
+ struct cros_ec_command *msg;
+ u8 buf[sizeof(*msg) + sizeof(*resp)];
int ret;
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg = (struct cros_ec_command *)buf;
+ msg->version = 0;
+ msg->command = EC_CMD_GET_VERSION;
+ msg->insize = sizeof(*resp);
+ msg->outsize = 0;
+
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
return ret;
- if (msg.result != EC_RES_SUCCESS) {
+ if (msg->result != EC_RES_SUCCESS) {
snprintf(str, maxlen,
"%s\nUnknown EC version: EC returned %d\n",
- CROS_EC_DEV_VERSION, msg.result);
- return 0;
+ CROS_EC_DEV_VERSION, msg->result);
+ return -EINVAL;
}
- resp = (struct ec_response_get_version *)msg.indata;
+ resp = (struct ec_response_get_version *)msg->data;
if (resp->current_image >= ARRAY_SIZE(current_image_name))
resp->current_image = 3; /* invalid */
@@ -110,20 +113,41 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
static long ec_device_ioctl_xcmd(struct cros_ec_device *ec, void __user *arg)
{
long ret;
- struct cros_ec_command s_cmd = { };
+ int alloc_size;
+ struct cros_ec_command u_cmd;
+ struct cros_ec_command *s_cmd;
+ u8 buf[sizeof(*s_cmd) + PREALLOC_SIZE];
- if (copy_from_user(&s_cmd, arg, sizeof(s_cmd)))
+ if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
return -EFAULT;
- ret = cros_ec_cmd_xfer(ec, &s_cmd);
+ alloc_size = max(u_cmd.outsize, u_cmd.insize);
+
+ if (alloc_size > PREALLOC_SIZE) {
+ s_cmd = kzalloc(sizeof(*s_cmd) + alloc_size, GFP_KERNEL);
+ if (!s_cmd)
+ return -ENOMEM;
+ } else {
+ s_cmd = (struct cros_ec_command *)buf;
+ alloc_size = 0;
+ }
+
+ if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) {
+ ret = -EFAULT;
+ goto exit;
+ }
+
+ ret = cros_ec_cmd_xfer(ec, s_cmd);
/* Only copy data to userland if data was received. */
if (ret < 0)
- return ret;
-
- if (copy_to_user(arg, &s_cmd, sizeof(s_cmd)))
- return -EFAULT;
+ goto exit;
- return 0;
+ if (copy_to_user(arg, s_cmd, sizeof(*s_cmd) + u_cmd.insize))
+ ret = -EFAULT;
+exit:
+ if (alloc_size)
+ kfree(s_cmd);
+ return ret;
}
static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index b4ff47a9069a..560e5d41b7ae 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -31,6 +31,7 @@
#include <linux/sched.h>
#include <linux/types.h>
#include <linux/uaccess.h>
+#include <linux/slab.h>
#include "cros_ec_dev.h"
@@ -91,54 +92,79 @@ out:
return ret;
}
-#define INIT_MSG(P, R) { \
- .command = EC_CMD_LIGHTBAR_CMD, \
- .outsize = sizeof(*P), \
- .insize = sizeof(*R), \
- }
+static struct cros_ec_command *alloc_lightbar_cmd_msg(void)
+{
+ struct cros_ec_command *msg;
+ int len;
+
+ len = max(sizeof(struct ec_params_lightbar),
+ sizeof(struct ec_response_lightbar));
+
+ msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
+ if (!msg)
+ return NULL;
+
+ msg->version = 0;
+ msg->command = EC_CMD_LIGHTBAR_CMD;
+ msg->outsize = sizeof(struct ec_params_lightbar);
+ msg->insize = sizeof(struct ec_response_lightbar);
+
+ return msg;
+}
static int get_lightbar_version(struct cros_ec_device *ec,
uint32_t *ver_ptr, uint32_t *flg_ptr)
{
struct ec_params_lightbar *param;
struct ec_response_lightbar *resp;
- struct cros_ec_command msg = INIT_MSG(param, resp);
+ struct cros_ec_command *msg;
int ret;
- param = (struct ec_params_lightbar *)msg.outdata;
- param->cmd = LIGHTBAR_CMD_VERSION;
- ret = cros_ec_cmd_xfer(ec, &msg);
- if (ret < 0)
+ msg = alloc_lightbar_cmd_msg();
+ if (!msg)
return 0;
- switch (msg.result) {
+ param = (struct ec_params_lightbar *)msg->data;
+ param->cmd = LIGHTBAR_CMD_VERSION;
+ ret = cros_ec_cmd_xfer(ec, msg);
+ if (ret < 0) {
+ ret = 0;
+ goto exit;
+ }
+
+ switch (msg->result) {
case EC_RES_INVALID_PARAM:
/* Pixel had no version command. */
if (ver_ptr)
*ver_ptr = 0;
if (flg_ptr)
*flg_ptr = 0;
- return 1;
+ ret = 1;
+ goto exit;
case EC_RES_SUCCESS:
- resp = (struct ec_response_lightbar *)msg.indata;
+ resp = (struct ec_response_lightbar *)msg->data;
/* Future devices w/lightbars should implement this command */
if (ver_ptr)
*ver_ptr = resp->version.num;
if (flg_ptr)
*flg_ptr = resp->version.flags;
- return 1;
+ ret = 1;
+ goto exit;
}
/* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
- return 0;
+ ret = 0;
+exit:
+ kfree(msg);
+ return ret;
}
static ssize_t version_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- uint32_t version, flags;
+ uint32_t version = 0, flags = 0;
struct cros_ec_device *ec = dev_get_drvdata(dev);
int ret;
@@ -158,8 +184,7 @@ static ssize_t brightness_store(struct device *dev,
const char *buf, size_t count)
{
struct ec_params_lightbar *param;
- struct ec_response_lightbar *resp;
- struct cros_ec_command msg = INIT_MSG(param, resp);
+ struct cros_ec_command *msg;
int ret;
unsigned int val;
struct cros_ec_device *ec = dev_get_drvdata(dev);
@@ -167,21 +192,30 @@ static ssize_t brightness_store(struct device *dev,
if (kstrtouint(buf, 0, &val))
return -EINVAL;
- param = (struct ec_params_lightbar *)msg.outdata;
+ msg = alloc_lightbar_cmd_msg();
+ if (!msg)
+ return -ENOMEM;
+
+ param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_BRIGHTNESS;
param->brightness.num = val;
ret = lb_throttle();
if (ret)
- return ret;
+ goto exit;
- ret = cros_ec_cmd_xfer(ec, &msg);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
- return ret;
+ goto exit;
- if (msg.result != EC_RES_SUCCESS)
- return -EINVAL;
+ if (msg->result != EC_RES_SUCCESS) {
+ ret = -EINVAL;
+ goto exit;
+ }
- return count;
+ ret = count;
+exit:
+ kfree(msg);
+ return ret;
}
@@ -196,12 +230,15 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct ec_params_lightbar *param;
- struct ec_response_lightbar *resp;
- struct cros_ec_command msg = INIT_MSG(param, resp);
+ struct cros_ec_command *msg;
struct cros_ec_device *ec = dev_get_drvdata(dev);
unsigned int val[4];
int ret, i = 0, j = 0, ok = 0;
+ msg = alloc_lightbar_cmd_msg();
+ if (!msg)
+ return -ENOMEM;
+
do {
/* Skip any whitespace */
while (*buf && isspace(*buf))
@@ -215,7 +252,7 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
return -EINVAL;
if (i == 4) {
- param = (struct ec_params_lightbar *)msg.outdata;
+ param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_RGB;
param->rgb.led = val[0];
param->rgb.red = val[1];
@@ -231,12 +268,14 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
return ret;
}
- ret = cros_ec_cmd_xfer(ec, &msg);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
- return ret;
+ goto exit;
- if (msg.result != EC_RES_SUCCESS)
- return -EINVAL;
+ if (msg->result != EC_RES_SUCCESS) {
+ ret = -EINVAL;
+ goto exit;
+ }
i = 0;
ok = 1;
@@ -248,6 +287,8 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
} while (*buf);
+exit:
+ kfree(msg);
return (ok && i == 0) ? count : -EINVAL;
}
@@ -261,42 +302,55 @@ static ssize_t sequence_show(struct device *dev,
{
struct ec_params_lightbar *param;
struct ec_response_lightbar *resp;
- struct cros_ec_command msg = INIT_MSG(param, resp);
+ struct cros_ec_command *msg;
int ret;
struct cros_ec_device *ec = dev_get_drvdata(dev);
- param = (struct ec_params_lightbar *)msg.outdata;
+ msg = alloc_lightbar_cmd_msg();
+ if (!msg)
+ return -ENOMEM;
+
+ param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_GET_SEQ;
ret = lb_throttle();
if (ret)
- return ret;
+ goto exit;
- ret = cros_ec_cmd_xfer(ec, &msg);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
- return ret;
+ goto exit;
- if (msg.result != EC_RES_SUCCESS)
- return scnprintf(buf, PAGE_SIZE,
- "ERROR: EC returned %d\n", msg.result);
+ if (msg->result != EC_RES_SUCCESS) {
+ ret = scnprintf(buf, PAGE_SIZE,
+ "ERROR: EC returned %d\n", msg->result);
+ goto exit;
+ }
- resp = (struct ec_response_lightbar *)msg.indata;
+ resp = (struct ec_response_lightbar *)msg->data;
if (resp->get_seq.num >= ARRAY_SIZE(seqname))
- return scnprintf(buf, PAGE_SIZE, "%d\n", resp->get_seq.num);
+ ret = scnprintf(buf, PAGE_SIZE, "%d\n", resp->get_seq.num);
else
- return scnprintf(buf, PAGE_SIZE, "%s\n",
- seqname[resp->get_seq.num]);
+ ret = scnprintf(buf, PAGE_SIZE, "%s\n",
+ seqname[resp->get_seq.num]);
+
+exit:
+ kfree(msg);
+ return ret;
}
static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct ec_params_lightbar *param;
- struct ec_response_lightbar *resp;
- struct cros_ec_command msg = INIT_MSG(param, resp);
+ struct cros_ec_command *msg;
unsigned int num;
int ret, len;
struct cros_ec_device *ec = dev_get_drvdata(dev);
+ msg = alloc_lightbar_cmd_msg();
+ if (!msg)
+ return -ENOMEM;
+
for (len = 0; len < count; len++)
if (!isalnum(buf[len]))
break;
@@ -311,18 +365,18 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
return ret;
}
- param = (struct ec_params_lightbar *)msg.outdata;
+ param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_SEQ;
param->seq.num = num;
ret = lb_throttle();
if (ret)
return ret;
- ret = cros_ec_cmd_xfer(ec, &msg);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
return ret;
- if (msg.result != EC_RES_SUCCESS)
+ if (msg->result != EC_RES_SUCCESS)
return -EINVAL;
return count;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 3a675817c95d..32b1df29fc58 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -73,8 +73,8 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
/* Copy data and update checksum */
for (i = 0; i < msg->outsize; i++) {
- outb(msg->outdata[i], EC_LPC_ADDR_HOST_PARAM + i);
- csum += msg->outdata[i];
+ outb(msg->data[i], EC_LPC_ADDR_HOST_PARAM + i);
+ csum += msg->data[i];
}
/* Finalize checksum and write args */
@@ -119,8 +119,8 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
/* Read response and update checksum */
for (i = 0; i < args.data_size; i++) {
- msg->indata[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
- csum += msg->indata[i];
+ msg->data[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
+ csum += msg->data[i];
}
/* Verify checksum */
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fb62ab6cc659..88d0a7c9bbff 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -29,6 +29,7 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/printk.h>
+#include <linux/slab.h>
#include <linux/stat.h>
#include <linux/types.h>
#include <linux/uaccess.h>
@@ -66,14 +67,17 @@ static ssize_t store_ec_reboot(struct device *dev,
{"hibernate", EC_REBOOT_HIBERNATE, 0},
{"at-shutdown", -1, EC_REBOOT_FLAG_ON_AP_SHUTDOWN},
};
- struct cros_ec_command msg = { 0 };
- struct ec_params_reboot_ec *param =
- (struct ec_params_reboot_ec *)msg.outdata;
+ struct cros_ec_command *msg;
+ struct ec_params_reboot_ec *param;
+ u8 msg_buf[sizeof(*msg) + sizeof(*param)] = { 0 };
int got_cmd = 0, offset = 0;
int i;
int ret;
struct cros_ec_device *ec = dev_get_drvdata(dev);
+ msg = (struct cros_ec_command *)msg_buf;
+ param = (struct ec_params_reboot_ec *)msg->data;
+
param->flags = 0;
while (1) {
/* Find word to start scanning */
@@ -103,13 +107,13 @@ static ssize_t store_ec_reboot(struct device *dev,
if (!got_cmd)
return -EINVAL;
- msg.command = EC_CMD_REBOOT_EC;
- msg.outsize = sizeof(param);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_REBOOT_EC;
+ msg->outsize = sizeof(*param);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
return ret;
- if (msg.result != EC_RES_SUCCESS) {
- dev_dbg(ec->dev, "EC result %d\n", msg.result);
+ if (msg->result != EC_RES_SUCCESS) {
+ dev_dbg(ec->dev, "EC result %d\n", msg->result);
return -EINVAL;
}
@@ -123,22 +127,25 @@ static ssize_t show_ec_version(struct device *dev,
struct ec_response_get_version *r_ver;
struct ec_response_get_chip_info *r_chip;
struct ec_response_board_version *r_board;
- struct cros_ec_command msg = { 0 };
+ struct cros_ec_command *msg;
+ u8 msg_buf[sizeof(*msg) + EC_HOST_PARAM_SIZE] = { 0 };
int ret;
int count = 0;
struct cros_ec_device *ec = dev_get_drvdata(dev);
+ msg = (struct cros_ec_command *)msg_buf;
+
/* Get versions. RW may change. */
- msg.command = EC_CMD_GET_VERSION;
- msg.insize = sizeof(*r_ver);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_GET_VERSION;
+ msg->insize = sizeof(*r_ver);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
return ret;
- if (msg.result != EC_RES_SUCCESS)
+ if (msg->result != EC_RES_SUCCESS)
return scnprintf(buf, PAGE_SIZE,
- "ERROR: EC returned %d\n", msg.result);
+ "ERROR: EC returned %d\n", msg->result);
- r_ver = (struct ec_response_get_version *)msg.indata;
+ r_ver = (struct ec_response_get_version *)msg->data;
/* Strings should be null-terminated, but let's be sure. */
r_ver->version_string_ro[sizeof(r_ver->version_string_ro) - 1] = '\0';
r_ver->version_string_rw[sizeof(r_ver->version_string_rw) - 1] = '\0';
@@ -152,33 +159,33 @@ static ssize_t show_ec_version(struct device *dev,
image_names[r_ver->current_image] : "?"));
/* Get build info. */
- msg.command = EC_CMD_GET_BUILD_INFO;
- msg.insize = sizeof(msg.indata);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_GET_BUILD_INFO;
+ msg->insize = EC_HOST_PARAM_SIZE;
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: XFER ERROR %d\n", ret);
- else if (msg.result != EC_RES_SUCCESS)
+ else if (msg->result != EC_RES_SUCCESS)
count += scnprintf(buf + count, PAGE_SIZE - count,
- "Build info: EC error %d\n", msg.result);
+ "Build info: EC error %d\n", msg->result);
else {
- msg.indata[sizeof(msg.indata) - 1] = '\0';
+ msg->data[sizeof(msg->data) - 1] = '\0';
count += scnprintf(buf + count, PAGE_SIZE - count,
- "Build info: %s\n", msg.indata);
+ "Build info: %s\n", msg->data);
}
/* Get chip info. */
- msg.command = EC_CMD_GET_CHIP_INFO;
- msg.insize = sizeof(*r_chip);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_GET_CHIP_INFO;
+ msg->insize = sizeof(*r_chip);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Chip info: XFER ERROR %d\n", ret);
- else if (msg.result != EC_RES_SUCCESS)
+ else if (msg->result != EC_RES_SUCCESS)
count += scnprintf(buf + count, PAGE_SIZE - count,
- "Chip info: EC error %d\n", msg.result);
+ "Chip info: EC error %d\n", msg->result);
else {
- r_chip = (struct ec_response_get_chip_info *)msg.indata;
+ r_chip = (struct ec_response_get_chip_info *)msg->data;
r_chip->vendor[sizeof(r_chip->vendor) - 1] = '\0';
r_chip->name[sizeof(r_chip->name) - 1] = '\0';
@@ -192,17 +199,17 @@ static ssize_t show_ec_version(struct device *dev,
}
/* Get board version */
- msg.command = EC_CMD_GET_BOARD_VERSION;
- msg.insize = sizeof(*r_board);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_GET_BOARD_VERSION;
+ msg->insize = sizeof(*r_board);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Board version: XFER ERROR %d\n", ret);
- else if (msg.result != EC_RES_SUCCESS)
+ else if (msg->result != EC_RES_SUCCESS)
count += scnprintf(buf + count, PAGE_SIZE - count,
- "Board version: EC error %d\n", msg.result);
+ "Board version: EC error %d\n", msg->result);
else {
- r_board = (struct ec_response_board_version *)msg.indata;
+ r_board = (struct ec_response_board_version *)msg->data;
count += scnprintf(buf + count, PAGE_SIZE - count,
"Board version: %d\n",
@@ -216,21 +223,24 @@ static ssize_t show_ec_flashinfo(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct ec_response_flash_info *resp;
- struct cros_ec_command msg = { 0 };
+ struct cros_ec_command *msg;
int ret;
struct cros_ec_device *ec = dev_get_drvdata(dev);
+ u8 msg_buf[sizeof(*msg) + sizeof(*resp)] = { 0 };
+
+ msg = (struct cros_ec_command *)msg_buf;
/* The flash info shouldn't ever change, but ask each time anyway. */
- msg.command = EC_CMD_FLASH_INFO;
- msg.insize = sizeof(*resp);
- ret = cros_ec_cmd_xfer(ec, &msg);
+ msg->command = EC_CMD_FLASH_INFO;
+ msg->insize = sizeof(*resp);
+ ret = cros_ec_cmd_xfer(ec, msg);
if (ret < 0)
return ret;
- if (msg.result != EC_RES_SUCCESS)
+ if (msg->result != EC_RES_SUCCESS)
return scnprintf(buf, PAGE_SIZE,
- "ERROR: EC returned %d\n", msg.result);
+ "ERROR: EC returned %d\n", msg->result);
- resp = (struct ec_response_flash_info *)msg.indata;
+ resp = (struct ec_response_flash_info *)msg->data;
return scnprintf(buf, PAGE_SIZE,
"FlashSize %d\nWriteSize %d\n"
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 14cf522123dd..7eee38abd02a 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -42,8 +42,7 @@ enum {
* @outsize: Outgoing length in bytes
* @insize: Max number of bytes to accept from EC
* @result: EC's response to the command (separate from communication failure)
- * @outdata: Outgoing data to EC
- * @indata: Where to put the incoming data from EC
+ * @data: Where to put the incoming data from EC and outgoing data to EC
*/
struct cros_ec_command {
uint32_t version;
@@ -51,8 +50,7 @@ struct cros_ec_command {
uint32_t outsize;
uint32_t insize;
uint32_t result;
- uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
- uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
+ uint8_t data[0];
};
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-09 10:10 ` [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data Javier Martinez Canillas
@ 2015-05-11 20:19 ` Gwendal Grignou
2015-05-11 21:33 ` Javier Martinez Canillas
2015-05-13 11:10 ` Lee Jones
1 sibling, 1 reply; 29+ messages in thread
From: Gwendal Grignou @ 2015-05-11 20:19 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Lee Jones, Samuel Ortiz, Olof Johansson, Doug Anderson,
Bill Richardson, Simon Glass, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, Linux Kernel
On Sat, May 9, 2015 at 3:10 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
> data with the EC") modified the struct cros_ec_command fields to not
> use pointers for the input and output buffers and use fixed length
> arrays instead.
>
> This change was made because the cros_ec ioctl API uses that struct
> cros_ec_command to allow user-space to send commands to the EC and
> to get data from the EC. So using pointers made the API not 64-bit
> safe. Unfortunately this approach was not flexible enough for all
> the use-cases since there may be a need to send larger commands
> on newer versions of the EC command protocol.
>
> So to avoid to choose a constant length that it may be too big for
> most commands and thus wasting memory and CPU cycles on copy from
> and to user-space or having a size that is too small for some big
> commands, use a zero-length array that is both 64-bit safe and
> flexible. The same buffer is used for both output and input data
> so the maximum of these values should be used to allocate it.
>
> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
>
> Changes since v1:
> - Add Heiko Stuebner Tested-by tag
> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
> - Use kmalloc instead of kzalloc when the message is later initialized
> Suggested by Gwendal Grignou
> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
> whenever is possible. Suggested by Gwendal Grignou
> - Pre-allocate buffers for the usual cases and only allocate dynamically
> in the heap for bigger sizes. Suggested by Gwendal Grignou
> - Don't access the cros_ec_command received from user-space before doing
> a copy_from_user. Suggested by Gwendal Grignou
> - Only copy from user-space outsize bytes and copy_to_user insize bytes
> Suggested by Gwendal Grignou
> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
> on success. Suggested by Gwendal Grignou
> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
> ---
> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
> drivers/mfd/cros_ec.c | 18 ++--
> drivers/mfd/cros_ec_i2c.c | 4 +-
> drivers/mfd/cros_ec_spi.c | 2 +-
> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
> include/linux/mfd/cros_ec.h | 6 +-
> 10 files changed, 273 insertions(+), 153 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index fa8dedd8c3a2..30cefcd02bbf 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -17,6 +17,12 @@
> #include <linux/slab.h>
>
> #define I2C_MAX_RETRIES 3
> +/*
> + * I2C commands are generally very small so 32 bytes is in
> + * most cases enough to hold the EC command headers and
> + * payload. Pre-allocate a buffer for these common cases.
> + */
> +#define PREALLOC_SIZE 32
>
> /**
> * struct ec_i2c_device - Driver data for I2C tunnel
> @@ -182,8 +188,10 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
> const u16 bus_num = bus->remote_bus;
> int request_len;
> int response_len;
> + int alloc_size;
> int result;
> - struct cros_ec_command msg = { };
> + struct cros_ec_command *msg;
> + u8 buf[sizeof(*msg) + PREALLOC_SIZE];
>
> request_len = ec_i2c_count_message(i2c_msgs, num);
> if (request_len < 0) {
> @@ -198,25 +206,46 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
> return response_len;
> }
>
> - result = ec_i2c_construct_message(msg.outdata, i2c_msgs, num, bus_num);
> - if (result)
> - return result;
> + alloc_size = max(request_len, response_len);
> +
> + if (alloc_size > PREALLOC_SIZE) {
> + msg = kmalloc(sizeof(*msg) + alloc_size, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> + } else {
> + msg = (struct cros_ec_command *)buf;
> + alloc_size = 0;
> + }
> +
> + result = ec_i2c_construct_message(msg->data, i2c_msgs, num, bus_num);
> + if (result) {
> + dev_err(dev, "Error constructing EC i2c message %d\n", result);
> + goto exit;
> + }
>
> - msg.version = 0;
> - msg.command = EC_CMD_I2C_PASSTHRU;
> - msg.outsize = request_len;
> - msg.insize = response_len;
> + msg->version = 0;
> + msg->command = EC_CMD_I2C_PASSTHRU;
> + msg->outsize = request_len;
> + msg->insize = response_len;
>
> - result = cros_ec_cmd_xfer(bus->ec, &msg);
> - if (result < 0)
> - return result;
> + result = cros_ec_cmd_xfer(bus->ec, msg);
> + if (result < 0) {
> + dev_err(dev, "Error transferring EC i2c message %d\n", result);
> + goto exit;
> + }
>
> - result = ec_i2c_parse_response(msg.indata, i2c_msgs, &num);
> - if (result < 0)
> - return result;
> + result = ec_i2c_parse_response(msg->data, i2c_msgs, &num);
> + if (result < 0) {
> + dev_err(dev, "Error parsing EC i2c message %d\n", result);
> + goto exit;
> + }
>
> /* Indicate success by saying how many messages were sent */
> - return num;
> + result = num;
> +exit:
> + if (alloc_size)
> + kfree(msg);
> + return result;
> }
>
> static u32 ec_i2c_functionality(struct i2c_adapter *adap)
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index b50c5b8b8a4d..090f8a75412a 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -149,16 +149,19 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
> {
> int ret;
> - struct cros_ec_command msg = {
> - .command = EC_CMD_MKBP_STATE,
> - .insize = ckdev->cols,
> - };
> + struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
Following amstan@ comments in 00/10 patch,
I think there is something wrong here:
kb_state should already have a cros_ec_command header, so that the
lower layer can
copy into the 'old' kb_state array directly.
There will be no need for the memcpy later.
Gwendal.
>
> - ret = cros_ec_cmd_xfer(ckdev->ec, &msg);
> - if (ret < 0)
> + msg->command = EC_CMD_MKBP_STATE;
> + msg->insize = ckdev->cols;
> + msg->outsize = 0;
> +
> + ret = cros_ec_cmd_xfer(ckdev->ec, msg);
> + if (ret < 0) {
> + dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
> return ret;
> + }
>
> - memcpy(kb_state, msg.indata, ckdev->cols);
> + memcpy(kb_state, msg->data, ckdev->cols);
>
> return 0;
> }
> @@ -168,7 +171,7 @@ static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
> struct cros_ec_keyb *ckdev = data;
> struct cros_ec_device *ec = ckdev->ec;
> int ret;
> - uint8_t kb_state[ckdev->cols];
> + uint8_t kb_state[sizeof(struct cros_ec_command) + ckdev->cols];
>
> if (device_may_wakeup(ec->dev))
> pm_wakeup_event(ec->dev, 0);
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 1574a9352a6d..ee8aa8142932 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> out[2] = msg->outsize;
> csum = out[0] + out[1] + out[2];
> for (i = 0; i < msg->outsize; i++)
> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>
> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> ret = ec_dev->cmd_xfer(ec_dev, msg);
> if (msg->result == EC_RES_IN_PROGRESS) {
> int i;
> - struct cros_ec_command status_msg = { };
> + struct cros_ec_command *status_msg;
> struct ec_response_get_comms_status *status;
> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
>
> - status_msg.command = EC_CMD_GET_COMMS_STATUS;
> - status_msg.insize = sizeof(*status);
> + status_msg = (struct cros_ec_command *)buf;
> + status_msg->command = EC_CMD_GET_COMMS_STATUS;
> + status_msg->insize = sizeof(*status);
>
> /*
> * Query the EC's status until it's no longer busy or
> @@ -88,16 +90,16 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> usleep_range(10000, 11000);
>
> - ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
> + ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> if (ret < 0)
> break;
>
> - msg->result = status_msg.result;
> - if (status_msg.result != EC_RES_SUCCESS)
> + msg->result = status_msg->result;
> + if (status_msg->result != EC_RES_SUCCESS)
> break;
>
> status = (struct ec_response_get_comms_status *)
> - status_msg.indata;
> + status_msg->data;
> if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> break;
> }
> diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
> index 82b4d6148698..fbf7819f5de5 100644
> --- a/drivers/mfd/cros_ec_i2c.c
> +++ b/drivers/mfd/cros_ec_i2c.c
> @@ -76,7 +76,7 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
> /* copy message payload and compute checksum */
> sum = out_buf[0] + out_buf[1] + out_buf[2];
> for (i = 0; i < msg->outsize; i++) {
> - out_buf[3 + i] = msg->outdata[i];
> + out_buf[3 + i] = msg->data[i];
> sum += out_buf[3 + i];
> }
> out_buf[3 + msg->outsize] = sum;
> @@ -109,7 +109,7 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
> /* copy response packet payload and compute checksum */
> sum = in_buf[0] + in_buf[1];
> for (i = 0; i < len; i++) {
> - msg->indata[i] = in_buf[2 + i];
> + msg->data[i] = in_buf[2 + i];
> sum += in_buf[2 + i];
> }
> dev_dbg(ec_dev->dev, "packet: %*ph, sum = %02x\n",
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index 27bd52e5e8b7..573730fec947 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -299,7 +299,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> for (i = 0; i < len; i++) {
> sum += ptr[i + 2];
> if (ec_msg->insize)
> - ec_msg->indata[i] = ptr[i + 2];
> + ec_msg->data[i] = ptr[i + 2];
> }
> sum &= 0xff;
>
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index 6090d0b2826f..7f7e3db59234 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -20,12 +20,15 @@
> #include <linux/fs.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/slab.h>
> #include <linux/uaccess.h>
>
> #include "cros_ec_dev.h"
>
> /* Device variables */
> #define CROS_MAX_DEV 128
> +/* Most EC commands size is ~64 bytes so allocate a buffer for the usual case */
> +#define PREALLOC_SIZE 64
> static struct class *cros_class;
> static int ec_major;
>
> @@ -36,28 +39,28 @@ static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
> static const char * const current_image_name[] = {
> "unknown", "read-only", "read-write", "invalid",
> };
> - struct cros_ec_command msg = {
> - .version = 0,
> - .command = EC_CMD_GET_VERSION,
> - .outdata = { 0 },
> - .outsize = 0,
> - .indata = { 0 },
> - .insize = sizeof(*resp),
> - };
> + struct cros_ec_command *msg;
> + u8 buf[sizeof(*msg) + sizeof(*resp)];
> int ret;
>
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg = (struct cros_ec_command *)buf;
> + msg->version = 0;
> + msg->command = EC_CMD_GET_VERSION;
> + msg->insize = sizeof(*resp);
> + msg->outsize = 0;
> +
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> return ret;
>
> - if (msg.result != EC_RES_SUCCESS) {
> + if (msg->result != EC_RES_SUCCESS) {
> snprintf(str, maxlen,
> "%s\nUnknown EC version: EC returned %d\n",
> - CROS_EC_DEV_VERSION, msg.result);
> - return 0;
> + CROS_EC_DEV_VERSION, msg->result);
> + return -EINVAL;
> }
>
> - resp = (struct ec_response_get_version *)msg.indata;
> + resp = (struct ec_response_get_version *)msg->data;
> if (resp->current_image >= ARRAY_SIZE(current_image_name))
> resp->current_image = 3; /* invalid */
>
> @@ -110,20 +113,41 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
> static long ec_device_ioctl_xcmd(struct cros_ec_device *ec, void __user *arg)
> {
> long ret;
> - struct cros_ec_command s_cmd = { };
> + int alloc_size;
> + struct cros_ec_command u_cmd;
> + struct cros_ec_command *s_cmd;
> + u8 buf[sizeof(*s_cmd) + PREALLOC_SIZE];
>
> - if (copy_from_user(&s_cmd, arg, sizeof(s_cmd)))
> + if (copy_from_user(&u_cmd, arg, sizeof(u_cmd)))
> return -EFAULT;
>
> - ret = cros_ec_cmd_xfer(ec, &s_cmd);
> + alloc_size = max(u_cmd.outsize, u_cmd.insize);
> +
> + if (alloc_size > PREALLOC_SIZE) {
> + s_cmd = kzalloc(sizeof(*s_cmd) + alloc_size, GFP_KERNEL);
> + if (!s_cmd)
> + return -ENOMEM;
> + } else {
> + s_cmd = (struct cros_ec_command *)buf;
> + alloc_size = 0;
> + }
> +
> + if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +
> + ret = cros_ec_cmd_xfer(ec, s_cmd);
> /* Only copy data to userland if data was received. */
> if (ret < 0)
> - return ret;
> -
> - if (copy_to_user(arg, &s_cmd, sizeof(s_cmd)))
> - return -EFAULT;
> + goto exit;
>
> - return 0;
> + if (copy_to_user(arg, s_cmd, sizeof(*s_cmd) + u_cmd.insize))
> + ret = -EFAULT;
> +exit:
> + if (alloc_size)
> + kfree(s_cmd);
> + return ret;
> }
>
> static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index b4ff47a9069a..560e5d41b7ae 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -31,6 +31,7 @@
> #include <linux/sched.h>
> #include <linux/types.h>
> #include <linux/uaccess.h>
> +#include <linux/slab.h>
>
> #include "cros_ec_dev.h"
>
> @@ -91,54 +92,79 @@ out:
> return ret;
> }
>
> -#define INIT_MSG(P, R) { \
> - .command = EC_CMD_LIGHTBAR_CMD, \
> - .outsize = sizeof(*P), \
> - .insize = sizeof(*R), \
> - }
> +static struct cros_ec_command *alloc_lightbar_cmd_msg(void)
> +{
> + struct cros_ec_command *msg;
> + int len;
> +
> + len = max(sizeof(struct ec_params_lightbar),
> + sizeof(struct ec_response_lightbar));
> +
> + msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> + if (!msg)
> + return NULL;
> +
> + msg->version = 0;
> + msg->command = EC_CMD_LIGHTBAR_CMD;
> + msg->outsize = sizeof(struct ec_params_lightbar);
> + msg->insize = sizeof(struct ec_response_lightbar);
> +
> + return msg;
> +}
>
> static int get_lightbar_version(struct cros_ec_device *ec,
> uint32_t *ver_ptr, uint32_t *flg_ptr)
> {
> struct ec_params_lightbar *param;
> struct ec_response_lightbar *resp;
> - struct cros_ec_command msg = INIT_MSG(param, resp);
> + struct cros_ec_command *msg;
> int ret;
>
> - param = (struct ec_params_lightbar *)msg.outdata;
> - param->cmd = LIGHTBAR_CMD_VERSION;
> - ret = cros_ec_cmd_xfer(ec, &msg);
> - if (ret < 0)
> + msg = alloc_lightbar_cmd_msg();
> + if (!msg)
> return 0;
>
> - switch (msg.result) {
> + param = (struct ec_params_lightbar *)msg->data;
> + param->cmd = LIGHTBAR_CMD_VERSION;
> + ret = cros_ec_cmd_xfer(ec, msg);
> + if (ret < 0) {
> + ret = 0;
> + goto exit;
> + }
> +
> + switch (msg->result) {
> case EC_RES_INVALID_PARAM:
> /* Pixel had no version command. */
> if (ver_ptr)
> *ver_ptr = 0;
> if (flg_ptr)
> *flg_ptr = 0;
> - return 1;
> + ret = 1;
> + goto exit;
>
> case EC_RES_SUCCESS:
> - resp = (struct ec_response_lightbar *)msg.indata;
> + resp = (struct ec_response_lightbar *)msg->data;
>
> /* Future devices w/lightbars should implement this command */
> if (ver_ptr)
> *ver_ptr = resp->version.num;
> if (flg_ptr)
> *flg_ptr = resp->version.flags;
> - return 1;
> + ret = 1;
> + goto exit;
> }
>
> /* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
> - return 0;
> + ret = 0;
> +exit:
> + kfree(msg);
> + return ret;
> }
>
> static ssize_t version_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - uint32_t version, flags;
> + uint32_t version = 0, flags = 0;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
> int ret;
>
> @@ -158,8 +184,7 @@ static ssize_t brightness_store(struct device *dev,
> const char *buf, size_t count)
> {
> struct ec_params_lightbar *param;
> - struct ec_response_lightbar *resp;
> - struct cros_ec_command msg = INIT_MSG(param, resp);
> + struct cros_ec_command *msg;
> int ret;
> unsigned int val;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
> @@ -167,21 +192,30 @@ static ssize_t brightness_store(struct device *dev,
> if (kstrtouint(buf, 0, &val))
> return -EINVAL;
>
> - param = (struct ec_params_lightbar *)msg.outdata;
> + msg = alloc_lightbar_cmd_msg();
> + if (!msg)
> + return -ENOMEM;
> +
> + param = (struct ec_params_lightbar *)msg->data;
> param->cmd = LIGHTBAR_CMD_BRIGHTNESS;
> param->brightness.num = val;
> ret = lb_throttle();
> if (ret)
> - return ret;
> + goto exit;
>
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> - return ret;
> + goto exit;
>
> - if (msg.result != EC_RES_SUCCESS)
> - return -EINVAL;
> + if (msg->result != EC_RES_SUCCESS) {
> + ret = -EINVAL;
> + goto exit;
> + }
>
> - return count;
> + ret = count;
> +exit:
> + kfree(msg);
> + return ret;
> }
>
>
> @@ -196,12 +230,15 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct ec_params_lightbar *param;
> - struct ec_response_lightbar *resp;
> - struct cros_ec_command msg = INIT_MSG(param, resp);
> + struct cros_ec_command *msg;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
> unsigned int val[4];
> int ret, i = 0, j = 0, ok = 0;
>
> + msg = alloc_lightbar_cmd_msg();
> + if (!msg)
> + return -ENOMEM;
> +
> do {
> /* Skip any whitespace */
> while (*buf && isspace(*buf))
> @@ -215,7 +252,7 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> return -EINVAL;
>
> if (i == 4) {
> - param = (struct ec_params_lightbar *)msg.outdata;
> + param = (struct ec_params_lightbar *)msg->data;
> param->cmd = LIGHTBAR_CMD_RGB;
> param->rgb.led = val[0];
> param->rgb.red = val[1];
> @@ -231,12 +268,14 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> - return ret;
> + goto exit;
>
> - if (msg.result != EC_RES_SUCCESS)
> - return -EINVAL;
> + if (msg->result != EC_RES_SUCCESS) {
> + ret = -EINVAL;
> + goto exit;
> + }
>
> i = 0;
> ok = 1;
> @@ -248,6 +287,8 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
>
> } while (*buf);
>
> +exit:
> + kfree(msg);
> return (ok && i == 0) ? count : -EINVAL;
> }
>
> @@ -261,42 +302,55 @@ static ssize_t sequence_show(struct device *dev,
> {
> struct ec_params_lightbar *param;
> struct ec_response_lightbar *resp;
> - struct cros_ec_command msg = INIT_MSG(param, resp);
> + struct cros_ec_command *msg;
> int ret;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
>
> - param = (struct ec_params_lightbar *)msg.outdata;
> + msg = alloc_lightbar_cmd_msg();
> + if (!msg)
> + return -ENOMEM;
> +
> + param = (struct ec_params_lightbar *)msg->data;
> param->cmd = LIGHTBAR_CMD_GET_SEQ;
> ret = lb_throttle();
> if (ret)
> - return ret;
> + goto exit;
>
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> - return ret;
> + goto exit;
>
> - if (msg.result != EC_RES_SUCCESS)
> - return scnprintf(buf, PAGE_SIZE,
> - "ERROR: EC returned %d\n", msg.result);
> + if (msg->result != EC_RES_SUCCESS) {
> + ret = scnprintf(buf, PAGE_SIZE,
> + "ERROR: EC returned %d\n", msg->result);
> + goto exit;
> + }
>
> - resp = (struct ec_response_lightbar *)msg.indata;
> + resp = (struct ec_response_lightbar *)msg->data;
> if (resp->get_seq.num >= ARRAY_SIZE(seqname))
> - return scnprintf(buf, PAGE_SIZE, "%d\n", resp->get_seq.num);
> + ret = scnprintf(buf, PAGE_SIZE, "%d\n", resp->get_seq.num);
> else
> - return scnprintf(buf, PAGE_SIZE, "%s\n",
> - seqname[resp->get_seq.num]);
> + ret = scnprintf(buf, PAGE_SIZE, "%s\n",
> + seqname[resp->get_seq.num]);
> +
> +exit:
> + kfree(msg);
> + return ret;
> }
>
> static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> struct ec_params_lightbar *param;
> - struct ec_response_lightbar *resp;
> - struct cros_ec_command msg = INIT_MSG(param, resp);
> + struct cros_ec_command *msg;
> unsigned int num;
> int ret, len;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
>
> + msg = alloc_lightbar_cmd_msg();
> + if (!msg)
> + return -ENOMEM;
> +
> for (len = 0; len < count; len++)
> if (!isalnum(buf[len]))
> break;
> @@ -311,18 +365,18 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> - param = (struct ec_params_lightbar *)msg.outdata;
> + param = (struct ec_params_lightbar *)msg->data;
> param->cmd = LIGHTBAR_CMD_SEQ;
> param->seq.num = num;
> ret = lb_throttle();
> if (ret)
> return ret;
>
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> return ret;
>
> - if (msg.result != EC_RES_SUCCESS)
> + if (msg->result != EC_RES_SUCCESS)
> return -EINVAL;
>
> return count;
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 3a675817c95d..32b1df29fc58 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -73,8 +73,8 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
>
> /* Copy data and update checksum */
> for (i = 0; i < msg->outsize; i++) {
> - outb(msg->outdata[i], EC_LPC_ADDR_HOST_PARAM + i);
> - csum += msg->outdata[i];
> + outb(msg->data[i], EC_LPC_ADDR_HOST_PARAM + i);
> + csum += msg->data[i];
> }
>
> /* Finalize checksum and write args */
> @@ -119,8 +119,8 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
>
> /* Read response and update checksum */
> for (i = 0; i < args.data_size; i++) {
> - msg->indata[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
> - csum += msg->indata[i];
> + msg->data[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
> + csum += msg->data[i];
> }
>
> /* Verify checksum */
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index fb62ab6cc659..88d0a7c9bbff 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -29,6 +29,7 @@
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
> +#include <linux/slab.h>
> #include <linux/stat.h>
> #include <linux/types.h>
> #include <linux/uaccess.h>
> @@ -66,14 +67,17 @@ static ssize_t store_ec_reboot(struct device *dev,
> {"hibernate", EC_REBOOT_HIBERNATE, 0},
> {"at-shutdown", -1, EC_REBOOT_FLAG_ON_AP_SHUTDOWN},
> };
> - struct cros_ec_command msg = { 0 };
> - struct ec_params_reboot_ec *param =
> - (struct ec_params_reboot_ec *)msg.outdata;
> + struct cros_ec_command *msg;
> + struct ec_params_reboot_ec *param;
> + u8 msg_buf[sizeof(*msg) + sizeof(*param)] = { 0 };
> int got_cmd = 0, offset = 0;
> int i;
> int ret;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
>
> + msg = (struct cros_ec_command *)msg_buf;
> + param = (struct ec_params_reboot_ec *)msg->data;
> +
> param->flags = 0;
> while (1) {
> /* Find word to start scanning */
> @@ -103,13 +107,13 @@ static ssize_t store_ec_reboot(struct device *dev,
> if (!got_cmd)
> return -EINVAL;
>
> - msg.command = EC_CMD_REBOOT_EC;
> - msg.outsize = sizeof(param);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_REBOOT_EC;
> + msg->outsize = sizeof(*param);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> return ret;
> - if (msg.result != EC_RES_SUCCESS) {
> - dev_dbg(ec->dev, "EC result %d\n", msg.result);
> + if (msg->result != EC_RES_SUCCESS) {
> + dev_dbg(ec->dev, "EC result %d\n", msg->result);
> return -EINVAL;
> }
>
> @@ -123,22 +127,25 @@ static ssize_t show_ec_version(struct device *dev,
> struct ec_response_get_version *r_ver;
> struct ec_response_get_chip_info *r_chip;
> struct ec_response_board_version *r_board;
> - struct cros_ec_command msg = { 0 };
> + struct cros_ec_command *msg;
> + u8 msg_buf[sizeof(*msg) + EC_HOST_PARAM_SIZE] = { 0 };
> int ret;
> int count = 0;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
>
> + msg = (struct cros_ec_command *)msg_buf;
> +
> /* Get versions. RW may change. */
> - msg.command = EC_CMD_GET_VERSION;
> - msg.insize = sizeof(*r_ver);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_GET_VERSION;
> + msg->insize = sizeof(*r_ver);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> return ret;
> - if (msg.result != EC_RES_SUCCESS)
> + if (msg->result != EC_RES_SUCCESS)
> return scnprintf(buf, PAGE_SIZE,
> - "ERROR: EC returned %d\n", msg.result);
> + "ERROR: EC returned %d\n", msg->result);
>
> - r_ver = (struct ec_response_get_version *)msg.indata;
> + r_ver = (struct ec_response_get_version *)msg->data;
> /* Strings should be null-terminated, but let's be sure. */
> r_ver->version_string_ro[sizeof(r_ver->version_string_ro) - 1] = '\0';
> r_ver->version_string_rw[sizeof(r_ver->version_string_rw) - 1] = '\0';
> @@ -152,33 +159,33 @@ static ssize_t show_ec_version(struct device *dev,
> image_names[r_ver->current_image] : "?"));
>
> /* Get build info. */
> - msg.command = EC_CMD_GET_BUILD_INFO;
> - msg.insize = sizeof(msg.indata);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_GET_BUILD_INFO;
> + msg->insize = EC_HOST_PARAM_SIZE;
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> "Build info: XFER ERROR %d\n", ret);
> - else if (msg.result != EC_RES_SUCCESS)
> + else if (msg->result != EC_RES_SUCCESS)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> - "Build info: EC error %d\n", msg.result);
> + "Build info: EC error %d\n", msg->result);
> else {
> - msg.indata[sizeof(msg.indata) - 1] = '\0';
> + msg->data[sizeof(msg->data) - 1] = '\0';
> count += scnprintf(buf + count, PAGE_SIZE - count,
> - "Build info: %s\n", msg.indata);
> + "Build info: %s\n", msg->data);
> }
>
> /* Get chip info. */
> - msg.command = EC_CMD_GET_CHIP_INFO;
> - msg.insize = sizeof(*r_chip);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_GET_CHIP_INFO;
> + msg->insize = sizeof(*r_chip);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> "Chip info: XFER ERROR %d\n", ret);
> - else if (msg.result != EC_RES_SUCCESS)
> + else if (msg->result != EC_RES_SUCCESS)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> - "Chip info: EC error %d\n", msg.result);
> + "Chip info: EC error %d\n", msg->result);
> else {
> - r_chip = (struct ec_response_get_chip_info *)msg.indata;
> + r_chip = (struct ec_response_get_chip_info *)msg->data;
>
> r_chip->vendor[sizeof(r_chip->vendor) - 1] = '\0';
> r_chip->name[sizeof(r_chip->name) - 1] = '\0';
> @@ -192,17 +199,17 @@ static ssize_t show_ec_version(struct device *dev,
> }
>
> /* Get board version */
> - msg.command = EC_CMD_GET_BOARD_VERSION;
> - msg.insize = sizeof(*r_board);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_GET_BOARD_VERSION;
> + msg->insize = sizeof(*r_board);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> "Board version: XFER ERROR %d\n", ret);
> - else if (msg.result != EC_RES_SUCCESS)
> + else if (msg->result != EC_RES_SUCCESS)
> count += scnprintf(buf + count, PAGE_SIZE - count,
> - "Board version: EC error %d\n", msg.result);
> + "Board version: EC error %d\n", msg->result);
> else {
> - r_board = (struct ec_response_board_version *)msg.indata;
> + r_board = (struct ec_response_board_version *)msg->data;
>
> count += scnprintf(buf + count, PAGE_SIZE - count,
> "Board version: %d\n",
> @@ -216,21 +223,24 @@ static ssize_t show_ec_flashinfo(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct ec_response_flash_info *resp;
> - struct cros_ec_command msg = { 0 };
> + struct cros_ec_command *msg;
> int ret;
> struct cros_ec_device *ec = dev_get_drvdata(dev);
> + u8 msg_buf[sizeof(*msg) + sizeof(*resp)] = { 0 };
> +
> + msg = (struct cros_ec_command *)msg_buf;
>
> /* The flash info shouldn't ever change, but ask each time anyway. */
> - msg.command = EC_CMD_FLASH_INFO;
> - msg.insize = sizeof(*resp);
> - ret = cros_ec_cmd_xfer(ec, &msg);
> + msg->command = EC_CMD_FLASH_INFO;
> + msg->insize = sizeof(*resp);
> + ret = cros_ec_cmd_xfer(ec, msg);
> if (ret < 0)
> return ret;
> - if (msg.result != EC_RES_SUCCESS)
> + if (msg->result != EC_RES_SUCCESS)
> return scnprintf(buf, PAGE_SIZE,
> - "ERROR: EC returned %d\n", msg.result);
> + "ERROR: EC returned %d\n", msg->result);
>
> - resp = (struct ec_response_flash_info *)msg.indata;
> + resp = (struct ec_response_flash_info *)msg->data;
>
> return scnprintf(buf, PAGE_SIZE,
> "FlashSize %d\nWriteSize %d\n"
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 14cf522123dd..7eee38abd02a 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -42,8 +42,7 @@ enum {
> * @outsize: Outgoing length in bytes
> * @insize: Max number of bytes to accept from EC
> * @result: EC's response to the command (separate from communication failure)
> - * @outdata: Outgoing data to EC
> - * @indata: Where to put the incoming data from EC
> + * @data: Where to put the incoming data from EC and outgoing data to EC
> */
> struct cros_ec_command {
> uint32_t version;
> @@ -51,8 +50,7 @@ struct cros_ec_command {
> uint32_t outsize;
> uint32_t insize;
> uint32_t result;
> - uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
> - uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
> + uint8_t data[0];
> };
>
> /**
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-11 20:19 ` Gwendal Grignou
@ 2015-05-11 21:33 ` Javier Martinez Canillas
2015-05-11 21:47 ` Heiko Stuebner
0 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-11 21:33 UTC (permalink / raw)
To: Gwendal Grignou
Cc: Lee Jones, Samuel Ortiz, Olof Johansson, Doug Anderson,
Bill Richardson, Simon Glass, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, Linux Kernel
Hello,
Thanks all for testing and sorry for this issue.
On 05/11/2015 10:19 PM, Gwendal Grignou wrote:
> On Sat, May 9, 2015 at 3:10 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
[snip]
>>
>> static u32 ec_i2c_functionality(struct i2c_adapter *adap)
>> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
>> index b50c5b8b8a4d..090f8a75412a 100644
>> --- a/drivers/input/keyboard/cros_ec_keyb.c
>> +++ b/drivers/input/keyboard/cros_ec_keyb.c
>> @@ -149,16 +149,19 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
>> static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
>> {
>> int ret;
>> - struct cros_ec_command msg = {
>> - .command = EC_CMD_MKBP_STATE,
>> - .insize = ckdev->cols,
>> - };
>> + struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
> Following amstan@ comments in 00/10 patch,
> I think there is something wrong here:
> kb_state should already have a cros_ec_command header, so that the
> lower layer can
> copy into the 'old' kb_state array directly.
> There will be no need for the memcpy later.
>
True, the memcpy should not be needed but I copied at the beginning of kb_state
to avoid having to compute the kb_state + sizeof(struct cros_ec_command) offset
in cros_ec_keyb_process().
> Gwendal.
>>
>> - ret = cros_ec_cmd_xfer(ckdev->ec, &msg);
>> - if (ret < 0)
>> + msg->command = EC_CMD_MKBP_STATE;
>> + msg->insize = ckdev->cols;
>> + msg->outsize = 0;
>> +
As Alexandru said, the EC is returning EC_RES_INVALID_VERSION and looking at
this code I see that there is a difference since v1 was using designated
initializers when declaring the struct cros_ec_command as a local variable in
the stack.
But in v2 the struct members are explicitly initialized since kb_state is not
initialized and I missed the version field so now it has an undefined value.
Heiko, Alexandru,
I don't have access to my Chromebooks right now but could you please test the
following patch [0] to see if that is the problem?
It seems I got it working on my test just out of luck since it happened that
version was initialized to 0 due the data it was on the stack.
Tomorrow when I've access to the Chromebooks I'll do more extensive testing
and see if I can reproduce the issue and if my assumption is correct.
Best regards,
Javier
[0]
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index ef3ba20f4560..4f233e248a4d 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -151,6 +151,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
int ret;
struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
+ msg->version = 0;
msg->command = EC_CMD_MKBP_STATE;
msg->insize = ckdev->cols;
msg->outsize = 0;
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-11 21:33 ` Javier Martinez Canillas
@ 2015-05-11 21:47 ` Heiko Stuebner
2015-05-11 21:53 ` Javier Martinez Canillas
0 siblings, 1 reply; 29+ messages in thread
From: Heiko Stuebner @ 2015-05-11 21:47 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Gwendal Grignou, Lee Jones, Samuel Ortiz, Olof Johansson,
Doug Anderson, Bill Richardson, Simon Glass, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
linux-samsung-soc, Linux Kernel
> Heiko, Alexandru,
>
> I don't have access to my Chromebooks right now but could you please test
> the following patch [0] to see if that is the problem?
>
> It seems I got it working on my test just out of luck since it happened that
> version was initialized to 0 due the data it was on the stack.
>
> Tomorrow when I've access to the Chromebooks I'll do more extensive testing
> and see if I can reproduce the issue and if my assumption is correct.
>
> Best regards,
> Javier
>
> [0]
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c
> b/drivers/input/keyboard/cros_ec_keyb.c index ef3ba20f4560..4f233e248a4d
> 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -151,6 +151,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb
> *ckdev, uint8_t *kb_state) int ret;
> struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
>
> + msg->version = 0;
> msg->command = EC_CMD_MKBP_STATE;
> msg->insize = ckdev->cols;
> msg->outsize = 0;
on my rk3288-pinky this seems to fix the issue ... keyboard works out of the
box again.
Heiko
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-11 21:47 ` Heiko Stuebner
@ 2015-05-11 21:53 ` Javier Martinez Canillas
0 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-11 21:53 UTC (permalink / raw)
To: Heiko Stuebner
Cc: Gwendal Grignou, Lee Jones, Samuel Ortiz, Olof Johansson,
Doug Anderson, Bill Richardson, Simon Glass, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
linux-samsung-soc, Linux Kernel
Hello Heiko and Alexandru,
On 05/11/2015 11:47 PM, Heiko Stuebner wrote:
>> Heiko, Alexandru,
>>
>> I don't have access to my Chromebooks right now but could you please test
>> the following patch [0] to see if that is the problem?
>>
>> It seems I got it working on my test just out of luck since it happened that
>> version was initialized to 0 due the data it was on the stack.
>>
>> Tomorrow when I've access to the Chromebooks I'll do more extensive testing
>> and see if I can reproduce the issue and if my assumption is correct.
>>
>> Best regards,
>> Javier
>>
>> [0]
>> diff --git a/drivers/input/keyboard/cros_ec_keyb.c
>> b/drivers/input/keyboard/cros_ec_keyb.c index ef3ba20f4560..4f233e248a4d
>> 100644
>> --- a/drivers/input/keyboard/cros_ec_keyb.c
>> +++ b/drivers/input/keyboard/cros_ec_keyb.c
>> @@ -151,6 +151,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb
>> *ckdev, uint8_t *kb_state) int ret;
>> struct cros_ec_command *msg = (struct cros_ec_command *)kb_state;
>>
>> + msg->version = 0;
>> msg->command = EC_CMD_MKBP_STATE;
>> msg->insize = ckdev->cols;
>> msg->outsize = 0;
>
> on my rk3288-pinky this seems to fix the issue ... keyboard works out of the
> box again.
>
Thank you both for testing the fix and again sorry for the silly mistake...
I'll wait a couple of days to see if I get more feedback and re-spin a v3
of the series with this fix patch squashed.
>
> Heiko
>
Best regards,
Javier
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-09 10:10 ` [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data Javier Martinez Canillas
2015-05-11 20:19 ` Gwendal Grignou
@ 2015-05-13 11:10 ` Lee Jones
2015-05-13 11:37 ` Javier Martinez Canillas
1 sibling, 1 reply; 29+ messages in thread
From: Lee Jones @ 2015-05-13 11:10 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
> data with the EC") modified the struct cros_ec_command fields to not
> use pointers for the input and output buffers and use fixed length
> arrays instead.
>
> This change was made because the cros_ec ioctl API uses that struct
> cros_ec_command to allow user-space to send commands to the EC and
> to get data from the EC. So using pointers made the API not 64-bit
> safe. Unfortunately this approach was not flexible enough for all
> the use-cases since there may be a need to send larger commands
> on newer versions of the EC command protocol.
>
> So to avoid to choose a constant length that it may be too big for
> most commands and thus wasting memory and CPU cycles on copy from
> and to user-space or having a size that is too small for some big
> commands, use a zero-length array that is both 64-bit safe and
> flexible. The same buffer is used for both output and input data
> so the maximum of these values should be used to allocate it.
>
> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> ---
>
> Changes since v1:
> - Add Heiko Stuebner Tested-by tag
> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
> - Use kmalloc instead of kzalloc when the message is later initialized
> Suggested by Gwendal Grignou
> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
> whenever is possible. Suggested by Gwendal Grignou
> - Pre-allocate buffers for the usual cases and only allocate dynamically
> in the heap for bigger sizes. Suggested by Gwendal Grignou
> - Don't access the cros_ec_command received from user-space before doing
> a copy_from_user. Suggested by Gwendal Grignou
> - Only copy from user-space outsize bytes and copy_to_user insize bytes
> Suggested by Gwendal Grignou
> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
> on success. Suggested by Gwendal Grignou
> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
> ---
> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
> drivers/mfd/cros_ec.c | 18 ++--
> drivers/mfd/cros_ec_i2c.c | 4 +-
> drivers/mfd/cros_ec_spi.c | 2 +-
> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
> include/linux/mfd/cros_ec.h | 6 +-
> 10 files changed, 273 insertions(+), 153 deletions(-)
[...]
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 1574a9352a6d..ee8aa8142932 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> out[2] = msg->outsize;
> csum = out[0] + out[1] + out[2];
> for (i = 0; i < msg->outsize; i++)
> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>
> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> ret = ec_dev->cmd_xfer(ec_dev, msg);
> if (msg->result == EC_RES_IN_PROGRESS) {
> int i;
> - struct cros_ec_command status_msg = { };
> + struct cros_ec_command *status_msg;
> struct ec_response_get_comms_status *status;
> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
This sort of thing is usually frowned upon. Can you allocate and free
buf's memory using the normal kernel helpers please?
[...]
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 14cf522123dd..7eee38abd02a 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -42,8 +42,7 @@ enum {
> * @outsize: Outgoing length in bytes
> * @insize: Max number of bytes to accept from EC
> * @result: EC's response to the command (separate from communication failure)
> - * @outdata: Outgoing data to EC
> - * @indata: Where to put the incoming data from EC
> + * @data: Where to put the incoming data from EC and outgoing data to EC
> */
> struct cros_ec_command {
> uint32_t version;
> @@ -51,8 +50,7 @@ struct cros_ec_command {
> uint32_t outsize;
> uint32_t insize;
> uint32_t result;
> - uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
> - uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
> + uint8_t data[0];
uint8_t *data?
> };
>
> /**
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-13 11:10 ` Lee Jones
@ 2015-05-13 11:37 ` Javier Martinez Canillas
2015-05-20 7:43 ` Javier Martinez Canillas
0 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-13 11:37 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
Hello Lee,
On 05/13/2015 01:10 PM, Lee Jones wrote:
> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
>
>> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
>> data with the EC") modified the struct cros_ec_command fields to not
>> use pointers for the input and output buffers and use fixed length
>> arrays instead.
>>
>> This change was made because the cros_ec ioctl API uses that struct
>> cros_ec_command to allow user-space to send commands to the EC and
>> to get data from the EC. So using pointers made the API not 64-bit
>> safe. Unfortunately this approach was not flexible enough for all
>> the use-cases since there may be a need to send larger commands
>> on newer versions of the EC command protocol.
>>
>> So to avoid to choose a constant length that it may be too big for
>> most commands and thus wasting memory and CPU cycles on copy from
>> and to user-space or having a size that is too small for some big
>> commands, use a zero-length array that is both 64-bit safe and
>> flexible. The same buffer is used for both output and input data
>> so the maximum of these values should be used to allocate it.
>>
>> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>
>> Changes since v1:
>> - Add Heiko Stuebner Tested-by tag
>> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
>> - Use kmalloc instead of kzalloc when the message is later initialized
>> Suggested by Gwendal Grignou
>> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
>> whenever is possible. Suggested by Gwendal Grignou
>> - Pre-allocate buffers for the usual cases and only allocate dynamically
>> in the heap for bigger sizes. Suggested by Gwendal Grignou
>> - Don't access the cros_ec_command received from user-space before doing
>> a copy_from_user. Suggested by Gwendal Grignou
>> - Only copy from user-space outsize bytes and copy_to_user insize bytes
>> Suggested by Gwendal Grignou
>> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
>> on success. Suggested by Gwendal Grignou
>> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
>> ---
>> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
>> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
>> drivers/mfd/cros_ec.c | 18 ++--
>> drivers/mfd/cros_ec_i2c.c | 4 +-
>> drivers/mfd/cros_ec_spi.c | 2 +-
>> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
>> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
>> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
>> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
>> include/linux/mfd/cros_ec.h | 6 +-
>> 10 files changed, 273 insertions(+), 153 deletions(-)
>
> [...]
>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index 1574a9352a6d..ee8aa8142932 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>> out[2] = msg->outsize;
>> csum = out[0] + out[1] + out[2];
>> for (i = 0; i < msg->outsize; i++)
>> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
>> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
>> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>>
>> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
>> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>> ret = ec_dev->cmd_xfer(ec_dev, msg);
>> if (msg->result == EC_RES_IN_PROGRESS) {
>> int i;
>> - struct cros_ec_command status_msg = { };
>> + struct cros_ec_command *status_msg;
>> struct ec_response_get_comms_status *status;
>> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
>
> This sort of thing is usually frowned upon. Can you allocate and free
> buf's memory using the normal kernel helpers please?
>
The first version of this patch used kmalloc (actually kzalloc) and kfree
to allocate and free the buffers but Gwendal suggested that we could
allocate in the stack instead as an optimization [0].
I have no strong opinion on this so I'm happy to change it again when
re-spinning the patches.
> [...]
>
>> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
>> index 14cf522123dd..7eee38abd02a 100644
>> --- a/include/linux/mfd/cros_ec.h
>> +++ b/include/linux/mfd/cros_ec.h
>> @@ -42,8 +42,7 @@ enum {
>> * @outsize: Outgoing length in bytes
>> * @insize: Max number of bytes to accept from EC
>> * @result: EC's response to the command (separate from communication failure)
>> - * @outdata: Outgoing data to EC
>> - * @indata: Where to put the incoming data from EC
>> + * @data: Where to put the incoming data from EC and outgoing data to EC
>> */
>> struct cros_ec_command {
>> uint32_t version;
>> @@ -51,8 +50,7 @@ struct cros_ec_command {
>> uint32_t outsize;
>> uint32_t insize;
>> uint32_t result;
>> - uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
>> - uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
>> + uint8_t data[0];
>
> uint8_t *data?
>
No, as explained in the commit message the, whole idea of using a zero
length array instead of a pointer is to make the data structure 64-bit
safe since using pointers will require to have a compat ioctl support
which Alan Cox and Olof said that shouldn't be needed for newer IOCTLs.
struct cros_ec_command {
uint32_t version;
uint32_t command;
uint32_t outsize;
uint32_t insize;
uint32_t result;
uint8_t* data;
};
IOW, the above structure has different sizes when building with -m32 and
-m64 due pointers being self-aligned to different byte boundaries in 32
and 64 bit machines. But when using a zero length array, the structure
has the same size in both 32 and 64 bit machines.
>> };
>>
>> /**
>
Best regards,
Javier
[0]: https://lkml.org/lkml/2015/4/24/8
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-13 11:37 ` Javier Martinez Canillas
@ 2015-05-20 7:43 ` Javier Martinez Canillas
2015-05-20 11:33 ` Lee Jones
0 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-20 7:43 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
Hello Lee,
On 05/13/2015 01:37 PM, Javier Martinez Canillas wrote:
>
> On 05/13/2015 01:10 PM, Lee Jones wrote:
>> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
>>
>>> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
>>> data with the EC") modified the struct cros_ec_command fields to not
>>> use pointers for the input and output buffers and use fixed length
>>> arrays instead.
>>>
>>> This change was made because the cros_ec ioctl API uses that struct
>>> cros_ec_command to allow user-space to send commands to the EC and
>>> to get data from the EC. So using pointers made the API not 64-bit
>>> safe. Unfortunately this approach was not flexible enough for all
>>> the use-cases since there may be a need to send larger commands
>>> on newer versions of the EC command protocol.
>>>
>>> So to avoid to choose a constant length that it may be too big for
>>> most commands and thus wasting memory and CPU cycles on copy from
>>> and to user-space or having a size that is too small for some big
>>> commands, use a zero-length array that is both 64-bit safe and
>>> flexible. The same buffer is used for both output and input data
>>> so the maximum of these values should be used to allocate it.
>>>
>>> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>>
>>> Changes since v1:
>>> - Add Heiko Stuebner Tested-by tag
>>> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
>>> - Use kmalloc instead of kzalloc when the message is later initialized
>>> Suggested by Gwendal Grignou
>>> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
>>> whenever is possible. Suggested by Gwendal Grignou
>>> - Pre-allocate buffers for the usual cases and only allocate dynamically
>>> in the heap for bigger sizes. Suggested by Gwendal Grignou
>>> - Don't access the cros_ec_command received from user-space before doing
>>> a copy_from_user. Suggested by Gwendal Grignou
>>> - Only copy from user-space outsize bytes and copy_to_user insize bytes
>>> Suggested by Gwendal Grignou
>>> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
>>> on success. Suggested by Gwendal Grignou
>>> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
>>> ---
>>> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
>>> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
>>> drivers/mfd/cros_ec.c | 18 ++--
>>> drivers/mfd/cros_ec_i2c.c | 4 +-
>>> drivers/mfd/cros_ec_spi.c | 2 +-
>>> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
>>> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
>>> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
>>> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
>>> include/linux/mfd/cros_ec.h | 6 +-
>>> 10 files changed, 273 insertions(+), 153 deletions(-)
>>
>> [...]
>>
>>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>>> index 1574a9352a6d..ee8aa8142932 100644
>>> --- a/drivers/mfd/cros_ec.c
>>> +++ b/drivers/mfd/cros_ec.c
>>> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>>> out[2] = msg->outsize;
>>> csum = out[0] + out[1] + out[2];
>>> for (i = 0; i < msg->outsize; i++)
>>> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
>>> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
>>> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>>>
>>> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
>>> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>>> ret = ec_dev->cmd_xfer(ec_dev, msg);
>>> if (msg->result == EC_RES_IN_PROGRESS) {
>>> int i;
>>> - struct cros_ec_command status_msg = { };
>>> + struct cros_ec_command *status_msg;
>>> struct ec_response_get_comms_status *status;
>>> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
>>
>> This sort of thing is usually frowned upon. Can you allocate and free
>> buf's memory using the normal kernel helpers please?
>>
>
> The first version of this patch used kmalloc (actually kzalloc) and kfree
> to allocate and free the buffers but Gwendal suggested that we could
> allocate in the stack instead as an optimization [0].
>
> I have no strong opinion on this so I'm happy to change it again when
> re-spinning the patches.
>
[snip]
>
> [0]: https://lkml.org/lkml/2015/4/24/8
>
You didn't answer if you agree with Gwendal that we can allocate things on
the stack or if you still prefer to use kmalloc/kfree. As I said I don't
have a strong argument on either approach but just want to agree to avoid
doing the same change on each revision.
Best regards,
Javier
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-20 7:43 ` Javier Martinez Canillas
@ 2015-05-20 11:33 ` Lee Jones
2015-05-20 11:34 ` Javier Martinez Canillas
0 siblings, 1 reply; 29+ messages in thread
From: Lee Jones @ 2015-05-20 11:33 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
On Wed, 20 May 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 05/13/2015 01:37 PM, Javier Martinez Canillas wrote:
> >
> > On 05/13/2015 01:10 PM, Lee Jones wrote:
> >> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> >>
> >>> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
> >>> data with the EC") modified the struct cros_ec_command fields to not
> >>> use pointers for the input and output buffers and use fixed length
> >>> arrays instead.
> >>>
> >>> This change was made because the cros_ec ioctl API uses that struct
> >>> cros_ec_command to allow user-space to send commands to the EC and
> >>> to get data from the EC. So using pointers made the API not 64-bit
> >>> safe. Unfortunately this approach was not flexible enough for all
> >>> the use-cases since there may be a need to send larger commands
> >>> on newer versions of the EC command protocol.
> >>>
> >>> So to avoid to choose a constant length that it may be too big for
> >>> most commands and thus wasting memory and CPU cycles on copy from
> >>> and to user-space or having a size that is too small for some big
> >>> commands, use a zero-length array that is both 64-bit safe and
> >>> flexible. The same buffer is used for both output and input data
> >>> so the maximum of these values should be used to allocate it.
> >>>
> >>> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
> >>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> >>> Tested-by: Heiko Stuebner <heiko@sntech.de>
> >>> ---
> >>>
> >>> Changes since v1:
> >>> - Add Heiko Stuebner Tested-by tag
> >>> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
> >>> - Use kmalloc instead of kzalloc when the message is later initialized
> >>> Suggested by Gwendal Grignou
> >>> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
> >>> whenever is possible. Suggested by Gwendal Grignou
> >>> - Pre-allocate buffers for the usual cases and only allocate dynamically
> >>> in the heap for bigger sizes. Suggested by Gwendal Grignou
> >>> - Don't access the cros_ec_command received from user-space before doing
> >>> a copy_from_user. Suggested by Gwendal Grignou
> >>> - Only copy from user-space outsize bytes and copy_to_user insize bytes
> >>> Suggested by Gwendal Grignou
> >>> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
> >>> on success. Suggested by Gwendal Grignou
> >>> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
> >>> ---
> >>> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
> >>> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
> >>> drivers/mfd/cros_ec.c | 18 ++--
> >>> drivers/mfd/cros_ec_i2c.c | 4 +-
> >>> drivers/mfd/cros_ec_spi.c | 2 +-
> >>> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
> >>> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
> >>> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
> >>> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
> >>> include/linux/mfd/cros_ec.h | 6 +-
> >>> 10 files changed, 273 insertions(+), 153 deletions(-)
> >>
> >> [...]
> >>
> >>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >>> index 1574a9352a6d..ee8aa8142932 100644
> >>> --- a/drivers/mfd/cros_ec.c
> >>> +++ b/drivers/mfd/cros_ec.c
> >>> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> >>> out[2] = msg->outsize;
> >>> csum = out[0] + out[1] + out[2];
> >>> for (i = 0; i < msg->outsize; i++)
> >>> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
> >>> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
> >>> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
> >>>
> >>> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
> >>> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> >>> ret = ec_dev->cmd_xfer(ec_dev, msg);
> >>> if (msg->result == EC_RES_IN_PROGRESS) {
> >>> int i;
> >>> - struct cros_ec_command status_msg = { };
> >>> + struct cros_ec_command *status_msg;
> >>> struct ec_response_get_comms_status *status;
> >>> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
> >>
> >> This sort of thing is usually frowned upon. Can you allocate and free
> >> buf's memory using the normal kernel helpers please?
> >>
> >
> > The first version of this patch used kmalloc (actually kzalloc) and kfree
> > to allocate and free the buffers but Gwendal suggested that we could
> > allocate in the stack instead as an optimization [0].
> >
> > I have no strong opinion on this so I'm happy to change it again when
> > re-spinning the patches.
> >
>
> [snip]
>
> >
> > [0]: https://lkml.org/lkml/2015/4/24/8
> >
>
> You didn't answer if you agree with Gwendal that we can allocate things on
> the stack or if you still prefer to use kmalloc/kfree. As I said I don't
> have a strong argument on either approach but just want to agree to avoid
> doing the same change on each revision.
I don't want you to use variable names to allocate arrays like this.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data
2015-05-20 11:33 ` Lee Jones
@ 2015-05-20 11:34 ` Javier Martinez Canillas
0 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-20 11:34 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
Hello Lee,
On 05/20/2015 01:33 PM, Lee Jones wrote:
> On Wed, 20 May 2015, Javier Martinez Canillas wrote:
>
>> Hello Lee,
>>
>> On 05/13/2015 01:37 PM, Javier Martinez Canillas wrote:
>> >
>> > On 05/13/2015 01:10 PM, Lee Jones wrote:
>> >> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
>> >>
>> >>> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
>> >>> data with the EC") modified the struct cros_ec_command fields to not
>> >>> use pointers for the input and output buffers and use fixed length
>> >>> arrays instead.
>> >>>
>> >>> This change was made because the cros_ec ioctl API uses that struct
>> >>> cros_ec_command to allow user-space to send commands to the EC and
>> >>> to get data from the EC. So using pointers made the API not 64-bit
>> >>> safe. Unfortunately this approach was not flexible enough for all
>> >>> the use-cases since there may be a need to send larger commands
>> >>> on newer versions of the EC command protocol.
>> >>>
>> >>> So to avoid to choose a constant length that it may be too big for
>> >>> most commands and thus wasting memory and CPU cycles on copy from
>> >>> and to user-space or having a size that is too small for some big
>> >>> commands, use a zero-length array that is both 64-bit safe and
>> >>> flexible. The same buffer is used for both output and input data
>> >>> so the maximum of these values should be used to allocate it.
>> >>>
>> >>> Suggested-by: Gwendal Grignou <gwendal@chromium.org>
>> >>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> >>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>> >>> ---
>> >>>
>> >>> Changes since v1:
>> >>> - Add Heiko Stuebner Tested-by tag
>> >>> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
>> >>> - Use kmalloc instead of kzalloc when the message is later initialized
>> >>> Suggested by Gwendal Grignou
>> >>> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
>> >>> whenever is possible. Suggested by Gwendal Grignou
>> >>> - Pre-allocate buffers for the usual cases and only allocate dynamically
>> >>> in the heap for bigger sizes. Suggested by Gwendal Grignou
>> >>> - Don't access the cros_ec_command received from user-space before doing
>> >>> a copy_from_user. Suggested by Gwendal Grignou
>> >>> - Only copy from user-space outsize bytes and copy_to_user insize bytes
>> >>> Suggested by Gwendal Grignou
>> >>> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
>> >>> on success. Suggested by Gwendal Grignou
>> >>> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
>> >>> ---
>> >>> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
>> >>> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
>> >>> drivers/mfd/cros_ec.c | 18 ++--
>> >>> drivers/mfd/cros_ec_i2c.c | 4 +-
>> >>> drivers/mfd/cros_ec_spi.c | 2 +-
>> >>> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
>> >>> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
>> >>> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
>> >>> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
>> >>> include/linux/mfd/cros_ec.h | 6 +-
>> >>> 10 files changed, 273 insertions(+), 153 deletions(-)
>> >>
>> >> [...]
>> >>
>> >>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> >>> index 1574a9352a6d..ee8aa8142932 100644
>> >>> --- a/drivers/mfd/cros_ec.c
>> >>> +++ b/drivers/mfd/cros_ec.c
>> >>> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>> >>> out[2] = msg->outsize;
>> >>> csum = out[0] + out[1] + out[2];
>> >>> for (i = 0; i < msg->outsize; i++)
>> >>> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
>> >>> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
>> >>> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>> >>>
>> >>> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
>> >>> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>> >>> ret = ec_dev->cmd_xfer(ec_dev, msg);
>> >>> if (msg->result == EC_RES_IN_PROGRESS) {
>> >>> int i;
>> >>> - struct cros_ec_command status_msg = { };
>> >>> + struct cros_ec_command *status_msg;
>> >>> struct ec_response_get_comms_status *status;
>> >>> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
>> >>
>> >> This sort of thing is usually frowned upon. Can you allocate and free
>> >> buf's memory using the normal kernel helpers please?
>> >>
>> >
>> > The first version of this patch used kmalloc (actually kzalloc) and kfree
>> > to allocate and free the buffers but Gwendal suggested that we could
>> > allocate in the stack instead as an optimization [0].
>> >
>> > I have no strong opinion on this so I'm happy to change it again when
>> > re-spinning the patches.
>> >
>>
>> [snip]
>>
>> >
>> > [0]: https://lkml.org/lkml/2015/4/24/8
>> >
>>
>> You didn't answer if you agree with Gwendal that we can allocate things on
>> the stack or if you still prefer to use kmalloc/kfree. As I said I don't
>> have a strong argument on either approach but just want to agree to avoid
>> doing the same change on each revision.
>
> I don't want you to use variable names to allocate arrays like this.
>
Perfect, thanks a lot for the clarification.
Best regards,
Javier
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 05/10] mfd: cros_ec: rev cros_ec_commands.h
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (3 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for command data Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton Javier Martinez Canillas
` (5 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
From: Stephen Barber <smbarber@chromium.org>
Update cros_ec_commands.h to the latest version in the EC
firmware sources and add power domain and passthru commands.
Also, update lightbar to use new command names.
Signed-off-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Changes since v1:
- Added Gwendal Grignou and Heiko Stuebner Tested-by tags
- Added Gwendal Grignou Reviewed-by tag
- Added Lee Jones Acked-by tag
---
drivers/platform/chrome/cros_ec_lightbar.c | 14 +-
include/linux/mfd/cros_ec_commands.h | 277 ++++++++++++++++++++++++++---
2 files changed, 262 insertions(+), 29 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 560e5d41b7ae..6e1986a2dce1 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -197,8 +197,8 @@ static ssize_t brightness_store(struct device *dev,
return -ENOMEM;
param = (struct ec_params_lightbar *)msg->data;
- param->cmd = LIGHTBAR_CMD_BRIGHTNESS;
- param->brightness.num = val;
+ param->cmd = LIGHTBAR_CMD_SET_BRIGHTNESS;
+ param->set_brightness.num = val;
ret = lb_throttle();
if (ret)
goto exit;
@@ -253,11 +253,11 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
if (i == 4) {
param = (struct ec_params_lightbar *)msg->data;
- param->cmd = LIGHTBAR_CMD_RGB;
- param->rgb.led = val[0];
- param->rgb.red = val[1];
- param->rgb.green = val[2];
- param->rgb.blue = val[3];
+ param->cmd = LIGHTBAR_CMD_SET_RGB;
+ param->set_rgb.led = val[0];
+ param->set_rgb.red = val[1];
+ param->set_rgb.green = val[2];
+ param->set_rgb.blue = val[3];
/*
* Throttle only the first of every four transactions,
* so that the user can update all four LEDs at once.
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index a49cd41feea7..13b630c10d4c 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -515,7 +515,7 @@ struct ec_host_response {
/*
* Notes on commands:
*
- * Each command is an 8-byte command value. Commands which take params or
+ * Each command is an 16-bit command value. Commands which take params or
* return response data specify structs for that data. If no struct is
* specified, the command does not input or output data, respectively.
* Parameter/response length is implicit in the structs. Some underlying
@@ -966,7 +966,7 @@ struct rgb_s {
/* List of tweakable parameters. NOTE: It's __packed so it can be sent in a
* host command, but the alignment is the same regardless. Keep it that way.
*/
-struct lightbar_params {
+struct lightbar_params_v0 {
/* Timing */
int32_t google_ramp_up;
int32_t google_ramp_down;
@@ -1000,32 +1000,81 @@ struct lightbar_params {
struct rgb_s color[8]; /* 0-3 are Google colors */
} __packed;
+struct lightbar_params_v1 {
+ /* Timing */
+ int32_t google_ramp_up;
+ int32_t google_ramp_down;
+ int32_t s3s0_ramp_up;
+ int32_t s0_tick_delay[2]; /* AC=0/1 */
+ int32_t s0a_tick_delay[2]; /* AC=0/1 */
+ int32_t s0s3_ramp_down;
+ int32_t s3_sleep_for;
+ int32_t s3_ramp_up;
+ int32_t s3_ramp_down;
+ int32_t tap_tick_delay;
+ int32_t tap_display_time;
+
+ /* Tap-for-battery params */
+ uint8_t tap_pct_red;
+ uint8_t tap_pct_green;
+ uint8_t tap_seg_min_on;
+ uint8_t tap_seg_max_on;
+ uint8_t tap_seg_osc;
+ uint8_t tap_idx[3];
+
+ /* Oscillation */
+ uint8_t osc_min[2]; /* AC=0/1 */
+ uint8_t osc_max[2]; /* AC=0/1 */
+ uint8_t w_ofs[2]; /* AC=0/1 */
+
+ /* Brightness limits based on the backlight and AC. */
+ uint8_t bright_bl_off_fixed[2]; /* AC=0/1 */
+ uint8_t bright_bl_on_min[2]; /* AC=0/1 */
+ uint8_t bright_bl_on_max[2]; /* AC=0/1 */
+
+ /* Battery level thresholds */
+ uint8_t battery_threshold[LB_BATTERY_LEVELS - 1];
+
+ /* Map [AC][battery_level] to color index */
+ uint8_t s0_idx[2][LB_BATTERY_LEVELS]; /* AP is running */
+ uint8_t s3_idx[2][LB_BATTERY_LEVELS]; /* AP is sleeping */
+
+ /* Color palette */
+ struct rgb_s color[8]; /* 0-3 are Google colors */
+} __packed;
+
struct ec_params_lightbar {
uint8_t cmd; /* Command (see enum lightbar_command) */
union {
struct {
/* no args */
- } dump, off, on, init, get_seq, get_params, version;
+ } dump, off, on, init, get_seq, get_params_v0, get_params_v1,
+ version, get_brightness, get_demo;
- struct num {
+ struct {
uint8_t num;
- } brightness, seq, demo;
+ } set_brightness, seq, demo;
- struct reg {
+ struct {
uint8_t ctrl, reg, value;
} reg;
- struct rgb {
+ struct {
uint8_t led, red, green, blue;
- } rgb;
+ } set_rgb;
+
+ struct {
+ uint8_t led;
+ } get_rgb;
- struct lightbar_params set_params;
+ struct lightbar_params_v0 set_params_v0;
+ struct lightbar_params_v1 set_params_v1;
};
} __packed;
struct ec_response_lightbar {
union {
- struct dump {
+ struct {
struct {
uint8_t reg;
uint8_t ic0;
@@ -1033,20 +1082,26 @@ struct ec_response_lightbar {
} vals[23];
} dump;
- struct get_seq {
+ struct {
uint8_t num;
- } get_seq;
+ } get_seq, get_brightness, get_demo;
- struct lightbar_params get_params;
+ struct lightbar_params_v0 get_params_v0;
+ struct lightbar_params_v1 get_params_v1;
- struct version {
+ struct {
uint32_t num;
uint32_t flags;
} version;
struct {
+ uint8_t red, green, blue;
+ } get_rgb;
+
+ struct {
/* no return params */
- } off, on, init, brightness, seq, reg, rgb, demo, set_params;
+ } off, on, init, set_brightness, seq, reg, set_rgb,
+ demo, set_params_v0, set_params_v1;
};
} __packed;
@@ -1056,15 +1111,20 @@ enum lightbar_command {
LIGHTBAR_CMD_OFF = 1,
LIGHTBAR_CMD_ON = 2,
LIGHTBAR_CMD_INIT = 3,
- LIGHTBAR_CMD_BRIGHTNESS = 4,
+ LIGHTBAR_CMD_SET_BRIGHTNESS = 4,
LIGHTBAR_CMD_SEQ = 5,
LIGHTBAR_CMD_REG = 6,
- LIGHTBAR_CMD_RGB = 7,
+ LIGHTBAR_CMD_SET_RGB = 7,
LIGHTBAR_CMD_GET_SEQ = 8,
LIGHTBAR_CMD_DEMO = 9,
- LIGHTBAR_CMD_GET_PARAMS = 10,
- LIGHTBAR_CMD_SET_PARAMS = 11,
+ LIGHTBAR_CMD_GET_PARAMS_V0 = 10,
+ LIGHTBAR_CMD_SET_PARAMS_V0 = 11,
LIGHTBAR_CMD_VERSION = 12,
+ LIGHTBAR_CMD_GET_BRIGHTNESS = 13,
+ LIGHTBAR_CMD_GET_RGB = 14,
+ LIGHTBAR_CMD_GET_DEMO = 15,
+ LIGHTBAR_CMD_GET_PARAMS_V1 = 16,
+ LIGHTBAR_CMD_SET_PARAMS_V1 = 17,
LIGHTBAR_NUM_CMDS
};
@@ -1421,8 +1481,40 @@ struct ec_response_rtc {
/*****************************************************************************/
/* Port80 log access */
+/* Maximum entries that can be read/written in a single command */
+#define EC_PORT80_SIZE_MAX 32
+
/* Get last port80 code from previous boot */
#define EC_CMD_PORT80_LAST_BOOT 0x48
+#define EC_CMD_PORT80_READ 0x48
+
+enum ec_port80_subcmd {
+ EC_PORT80_GET_INFO = 0,
+ EC_PORT80_READ_BUFFER,
+};
+
+struct ec_params_port80_read {
+ uint16_t subcmd;
+ union {
+ struct {
+ uint32_t offset;
+ uint32_t num_entries;
+ } read_buffer;
+ };
+} __packed;
+
+struct ec_response_port80_read {
+ union {
+ struct {
+ uint32_t writes;
+ uint32_t history_size;
+ uint32_t last_boot;
+ } get_info;
+ struct {
+ uint16_t codes[EC_PORT80_SIZE_MAX];
+ } data;
+ };
+} __packed;
struct ec_response_port80_last_boot {
uint16_t code;
@@ -1782,6 +1874,7 @@ struct ec_params_gpio_set {
/* Get GPIO value */
#define EC_CMD_GPIO_GET 0x93
+/* Version 0 of input params and response */
struct ec_params_gpio_get {
char name[32];
} __packed;
@@ -1789,6 +1882,38 @@ struct ec_response_gpio_get {
uint8_t val;
} __packed;
+/* Version 1 of input params and response */
+struct ec_params_gpio_get_v1 {
+ uint8_t subcmd;
+ union {
+ struct {
+ char name[32];
+ } get_value_by_name;
+ struct {
+ uint8_t index;
+ } get_info;
+ };
+} __packed;
+
+struct ec_response_gpio_get_v1 {
+ union {
+ struct {
+ uint8_t val;
+ } get_value_by_name, get_count;
+ struct {
+ uint8_t val;
+ char name[32];
+ uint32_t flags;
+ } get_info;
+ };
+} __packed;
+
+enum gpio_get_subcmd {
+ EC_GPIO_GET_BY_NAME = 0,
+ EC_GPIO_GET_COUNT = 1,
+ EC_GPIO_GET_INFO = 2,
+};
+
/*****************************************************************************/
/* I2C commands. Only available when flash write protect is unlocked. */
@@ -1857,13 +1982,21 @@ struct ec_params_charge_control {
/*****************************************************************************/
/*
- * Cut off battery power output if the battery supports.
+ * Cut off battery power immediately or after the host has shut down.
*
- * For unsupported battery, just don't implement this command and lets EC
- * return EC_RES_INVALID_COMMAND.
+ * return EC_RES_INVALID_COMMAND if unsupported by a board/battery.
+ * EC_RES_SUCCESS if the command was successful.
+ * EC_RES_ERROR if the cut off command failed.
*/
+
#define EC_CMD_BATTERY_CUT_OFF 0x99
+#define EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN (1 << 0)
+
+struct ec_params_battery_cutoff {
+ uint8_t flags;
+} __packed;
+
/*****************************************************************************/
/* USB port mux control. */
@@ -2142,6 +2275,32 @@ struct ec_params_sb_wr_block {
} __packed;
/*****************************************************************************/
+/* Battery vendor parameters
+ *
+ * Get or set vendor-specific parameters in the battery. Implementations may
+ * differ between boards or batteries. On a set operation, the response
+ * contains the actual value set, which may be rounded or clipped from the
+ * requested value.
+ */
+
+#define EC_CMD_BATTERY_VENDOR_PARAM 0xb4
+
+enum ec_battery_vendor_param_mode {
+ BATTERY_VENDOR_PARAM_MODE_GET = 0,
+ BATTERY_VENDOR_PARAM_MODE_SET,
+};
+
+struct ec_params_battery_vendor_param {
+ uint32_t param;
+ uint32_t value;
+ uint8_t mode;
+} __packed;
+
+struct ec_response_battery_vendor_param {
+ uint32_t value;
+} __packed;
+
+/*****************************************************************************/
/* System commands */
/*
@@ -2338,6 +2497,80 @@ struct ec_params_reboot_ec {
/*****************************************************************************/
/*
+ * PD commands
+ *
+ * These commands are for PD MCU communication.
+ */
+
+/* EC to PD MCU exchange status command */
+#define EC_CMD_PD_EXCHANGE_STATUS 0x100
+
+/* Status of EC being sent to PD */
+struct ec_params_pd_status {
+ int8_t batt_soc; /* battery state of charge */
+} __packed;
+
+/* Status of PD being sent back to EC */
+struct ec_response_pd_status {
+ int8_t status; /* PD MCU status */
+ uint32_t curr_lim_ma; /* input current limit */
+} __packed;
+
+/* Set USB type-C port role and muxes */
+#define EC_CMD_USB_PD_CONTROL 0x101
+
+enum usb_pd_control_role {
+ USB_PD_CTRL_ROLE_NO_CHANGE = 0,
+ USB_PD_CTRL_ROLE_TOGGLE_ON = 1, /* == AUTO */
+ USB_PD_CTRL_ROLE_TOGGLE_OFF = 2,
+ USB_PD_CTRL_ROLE_FORCE_SINK = 3,
+ USB_PD_CTRL_ROLE_FORCE_SOURCE = 4,
+};
+
+enum usb_pd_control_mux {
+ USB_PD_CTRL_MUX_NO_CHANGE = 0,
+ USB_PD_CTRL_MUX_NONE = 1,
+ USB_PD_CTRL_MUX_USB = 2,
+ USB_PD_CTRL_MUX_DP = 3,
+ USB_PD_CTRL_MUX_DOCK = 4,
+ USB_PD_CTRL_MUX_AUTO = 5,
+};
+
+struct ec_params_usb_pd_control {
+ uint8_t port;
+ uint8_t role;
+ uint8_t mux;
+} __packed;
+
+/*****************************************************************************/
+/*
+ * Passthru commands
+ *
+ * Some platforms have sub-processors chained to each other. For example.
+ *
+ * AP <--> EC <--> PD MCU
+ *
+ * The top 2 bits of the command number are used to indicate which device the
+ * command is intended for. Device 0 is always the device receiving the
+ * command; other device mapping is board-specific.
+ *
+ * When a device receives a command to be passed to a sub-processor, it passes
+ * it on with the device number set back to 0. This allows the sub-processor
+ * to remain blissfully unaware of whether the command originated on the next
+ * device up the chain, or was passed through from the AP.
+ *
+ * In the above example, if the AP wants to send command 0x0002 to the PD MCU,
+ * AP sends command 0x4002 to the EC
+ * EC sends command 0x0002 to the PD MCU
+ * EC forwards PD MCU response back to the AP
+ */
+
+/* Offset and max command number for sub-device n */
+#define EC_CMD_PASSTHRU_OFFSET(n) (0x4000 * (n))
+#define EC_CMD_PASSTHRU_MAX(n) (EC_CMD_PASSTHRU_OFFSET(n) + 0x3fff)
+
+/*****************************************************************************/
+/*
* Deprecated constants. These constants have been renamed for clarity. The
* meaning and size has not changed. Programs that use the old names should
* switch to the new names soon, as the old names may not be carried forward
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (4 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 05/10] mfd: cros_ec: rev cros_ec_commands.h Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-13 12:05 ` Lee Jones
2015-05-09 10:10 ` [PATCH v2 07/10] mfd: cros_ec: add bus-specific proto v3 code Javier Martinez Canillas
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
From: Stephen Barber <smbarber@chromium.org>
Add support in cros_ec.c to handle EC host command protocol v3.
For v3+, probe for maximum shared protocol version and max
request, response, and passthrough sizes. For now, this will
always fall back to v2, since there is no bus-specific code
for handling proto v3 packets.
Signed-off-by: Stephen Barber <smbarber@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes since v1:
- Squash change https://chromium-review.googlesource.com/#/c/262870/ in
the patch. Suggested by Gwendal Grignou
- Add Reviewed-by and Tested-by tags from Gwendal Grignou
---
drivers/mfd/cros_ec.c | 354 ++++++++++++++++++++++++++++++----
drivers/mfd/cros_ec_i2c.c | 4 +
drivers/mfd/cros_ec_spi.c | 7 +-
drivers/platform/chrome/cros_ec_lpc.c | 4 +
include/linux/mfd/cros_ec.h | 20 +-
5 files changed, 344 insertions(+), 45 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index ee8aa8142932..956a827de826 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -28,11 +28,276 @@
#define EC_COMMAND_RETRIES 50
+static int prepare_packet(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *msg)
+{
+ struct ec_host_request *request;
+ u8 *out;
+ int i;
+ u8 csum = 0;
+
+ BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
+ BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
+
+ out = ec_dev->dout;
+ request = (struct ec_host_request *)out;
+ request->struct_version = EC_HOST_REQUEST_VERSION;
+ request->checksum = 0;
+ request->command = msg->command;
+ request->command_version = msg->version;
+ request->reserved = 0;
+ request->data_len = msg->outsize;
+
+ for (i = 0; i < sizeof(*request); i++)
+ csum += out[i];
+
+ /* Copy data and update checksum */
+ memcpy(out + sizeof(*request), msg->data, msg->outsize);
+ for (i = 0; i < msg->outsize; i++)
+ csum += msg->data[i];
+
+ request->checksum = -csum;
+
+ return sizeof(*request) + msg->outsize;
+}
+
+static int send_command(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *msg)
+{
+ int ret;
+
+ if (ec_dev->proto_version > 2)
+ ret = ec_dev->pkt_xfer(ec_dev, msg);
+ else
+ ret = ec_dev->cmd_xfer(ec_dev, msg);
+
+ if (msg->result == EC_RES_IN_PROGRESS) {
+ int i;
+ struct cros_ec_command *status_msg;
+ struct ec_response_get_comms_status *status;
+ u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
+
+ status_msg = (struct cros_ec_command *)buf;
+ status_msg->command = EC_CMD_GET_COMMS_STATUS;
+ status_msg->insize = sizeof(*status);
+
+ /*
+ * Query the EC's status until it's no longer busy or
+ * we encounter an error.
+ */
+ for (i = 0; i < EC_COMMAND_RETRIES; i++) {
+ usleep_range(10000, 11000);
+
+ ret = ec_dev->cmd_xfer(ec_dev, status_msg);
+ if (ret < 0)
+ break;
+
+ msg->result = status_msg->result;
+ if (status_msg->result != EC_RES_SUCCESS)
+ break;
+
+ status = (struct ec_response_get_comms_status *)
+ status_msg->data;
+ if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int cros_ec_host_command_proto_probe(struct cros_ec_device *ec_dev,
+ int devidx,
+ struct cros_ec_command *msg)
+{
+ /*
+ * Try using v3+ to query for supported protocols. If this
+ * command fails, fall back to v2. Returns the highest protocol
+ * supported by the EC.
+ * Also sets the max request/response/passthru size.
+ */
+ int ret;
+
+ if (!ec_dev->pkt_xfer)
+ return -EPROTONOSUPPORT;
+
+ memset(msg, 0, sizeof(*msg));
+ msg->command = EC_CMD_PASSTHRU_OFFSET(devidx) | EC_CMD_GET_PROTOCOL_INFO;
+ msg->insize = sizeof(struct ec_response_get_protocol_info);
+
+ ret = send_command(ec_dev, msg);
+
+ if (ret < 0) {
+ dev_dbg(ec_dev->dev,
+ "failed to probe for EC[%d] protocol version: %d\n",
+ devidx, ret);
+ return ret;
+ }
+
+ if (devidx > 0 && msg->result == EC_RES_INVALID_COMMAND)
+ return -ENODEV;
+ else if (msg->result != EC_RES_SUCCESS)
+ return msg->result;
+
+ return 0;
+}
+
+static int cros_ec_host_command_proto_probe_v2(struct cros_ec_device *ec_dev)
+{
+ struct cros_ec_command *msg;
+ struct ec_params_hello *hello_params;
+ struct ec_response_hello *hello_response;
+ int ret;
+ int len = max(sizeof(*hello_params), sizeof(*hello_response));
+
+ msg = kzalloc(sizeof(*msg) + len, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->command = EC_CMD_HELLO;
+ hello_params = (struct ec_params_hello *)msg->data;
+ msg->outsize = sizeof(*hello_params);
+ hello_response = (struct ec_response_hello *)msg->data;
+ msg->insize = sizeof(*hello_response);
+
+ hello_params->in_data = 0xa0b0c0d0;
+
+ ret = send_command(ec_dev, msg);
+
+ if (ret < 0) {
+ dev_dbg(ec_dev->dev,
+ "EC failed to respond to v2 hello: %d\n",
+ ret);
+ goto exit;
+ } else if (msg->result != EC_RES_SUCCESS) {
+ dev_err(ec_dev->dev,
+ "EC responded to v2 hello with error: %d\n",
+ msg->result);
+ ret = msg->result;
+ goto exit;
+ } else if (hello_response->out_data != 0xa1b2c3d4) {
+ dev_err(ec_dev->dev,
+ "EC responded to v2 hello with bad result: %u\n",
+ hello_response->out_data);
+ ret = -EBADMSG;
+ goto exit;
+ }
+
+ ret = 0;
+
+exit:
+ kfree(msg);
+ return ret;
+}
+
+static int cros_ec_probe_all(struct cros_ec_device *ec_dev)
+{
+ struct device *dev = ec_dev->dev;
+ struct cros_ec_command *proto_msg;
+ struct ec_response_get_protocol_info *proto_info;
+ int ret;
+
+ proto_msg = kzalloc(sizeof(*proto_msg) + sizeof(*proto_info),
+ GFP_KERNEL);
+ if (!proto_msg)
+ return -ENOMEM;
+
+ /* First try sending with proto v3. */
+ ec_dev->proto_version = 3;
+ ret = cros_ec_host_command_proto_probe(ec_dev, 0, proto_msg);
+
+ if (ret == 0) {
+ proto_info = (struct ec_response_get_protocol_info *)
+ proto_msg->data;
+ ec_dev->max_request = proto_info->max_request_packet_size -
+ sizeof(struct ec_host_request);
+ ec_dev->max_response = proto_info->max_response_packet_size -
+ sizeof(struct ec_host_response);
+ ec_dev->proto_version =
+ min(EC_HOST_REQUEST_VERSION,
+ fls(proto_info->protocol_versions) - 1);
+ dev_dbg(ec_dev->dev,
+ "using proto v%u\n",
+ ec_dev->proto_version);
+
+ ec_dev->din_size = ec_dev->max_response +
+ sizeof(struct ec_host_response) +
+ EC_MAX_RESPONSE_OVERHEAD;
+ ec_dev->dout_size = ec_dev->max_request +
+ sizeof(struct ec_host_request) +
+ EC_MAX_REQUEST_OVERHEAD;
+
+ /*
+ * Check for PD
+ */
+ ret = cros_ec_host_command_proto_probe(ec_dev, 1, proto_msg);
+
+ if (ret) {
+ dev_dbg(ec_dev->dev, "no PD chip found: %d\n", ret);
+ ec_dev->max_passthru = 0;
+ } else {
+ dev_dbg(ec_dev->dev, "found PD chip\n");
+ ec_dev->max_passthru =
+ proto_info->max_request_packet_size -
+ sizeof(struct ec_host_request);
+ }
+ } else {
+ /* Try probing with a v2 hello message. */
+ ec_dev->proto_version = 2;
+ ret = cros_ec_host_command_proto_probe_v2(ec_dev);
+
+ if (ret == 0) {
+ /* V2 hello succeeded. */
+ dev_dbg(ec_dev->dev, "falling back to proto v2\n");
+
+ ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
+ ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
+ ec_dev->max_passthru = 0;
+ ec_dev->pkt_xfer = NULL;
+ ec_dev->din_size = EC_MSG_BYTES;
+ ec_dev->dout_size = EC_MSG_BYTES;
+ } else {
+ /*
+ * It's possible for a probe to occur too early when
+ * the EC isn't listening. If this happens, we'll
+ * probe later when the first command is run.
+ */
+ ec_dev->proto_version = EC_PROTO_VERSION_UNKNOWN;
+ dev_dbg(ec_dev->dev, "EC probe failed: %d\n", ret);
+ goto exit;
+ }
+ }
+
+ devm_kfree(dev, ec_dev->din);
+ devm_kfree(dev, ec_dev->dout);
+
+ ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
+ if (!ec_dev->din) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+ ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
+ if (!ec_dev->dout) {
+ devm_kfree(dev, ec_dev->din);
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+exit:
+ kfree(proto_msg);
+ return ret;
+}
+
int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
{
- uint8_t *out;
- int csum, i;
+ u8 *out;
+ u8 csum;
+ int i;
+
+ if (ec_dev->proto_version > 2)
+ return prepare_packet(ec_dev, msg);
BUG_ON(msg->outsize > EC_PROTO2_MAX_PARAM_SIZE);
out = ec_dev->dout;
@@ -42,7 +307,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
csum = out[0] + out[1] + out[2];
for (i = 0; i < msg->outsize; i++)
csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
- out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
+ out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = csum;
return EC_MSG_TX_PROTO_BYTES + msg->outsize;
}
@@ -72,38 +337,42 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
int ret;
mutex_lock(&ec_dev->lock);
- ret = ec_dev->cmd_xfer(ec_dev, msg);
- if (msg->result == EC_RES_IN_PROGRESS) {
- int i;
- struct cros_ec_command *status_msg;
- struct ec_response_get_comms_status *status;
- u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
-
- status_msg = (struct cros_ec_command *)buf;
- status_msg->command = EC_CMD_GET_COMMS_STATUS;
- status_msg->insize = sizeof(*status);
-
- /*
- * Query the EC's status until it's no longer busy or
- * we encounter an error.
- */
- for (i = 0; i < EC_COMMAND_RETRIES; i++) {
- usleep_range(10000, 11000);
-
- ret = ec_dev->cmd_xfer(ec_dev, status_msg);
- if (ret < 0)
- break;
+ if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
+ ret = cros_ec_probe_all(ec_dev);
+ if (ret) {
+ dev_err(ec_dev->dev,
+ "EC version unknown and probe failed; aborting command\n");
+ mutex_unlock(&ec_dev->lock);
+ return ret;
+ }
+ }
- msg->result = status_msg->result;
- if (status_msg->result != EC_RES_SUCCESS)
- break;
+ if (msg->insize > ec_dev->max_response) {
+ dev_dbg(ec_dev->dev, "clamping message receive buffer\n");
+ msg->insize = ec_dev->max_response;
+ }
- status = (struct ec_response_get_comms_status *)
- status_msg->data;
- if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
- break;
+ if (msg->command < EC_CMD_PASSTHRU_OFFSET(1)) {
+ if (msg->outsize > ec_dev->max_request) {
+ dev_err(ec_dev->dev,
+ "request of size %u is too big (max: %u)\n",
+ msg->outsize,
+ ec_dev->max_request);
+ mutex_unlock(&ec_dev->lock);
+ return -EMSGSIZE;
+ }
+ } else {
+ if (msg->outsize > ec_dev->max_passthru) {
+ dev_err(ec_dev->dev,
+ "passthru rq of size %u is too big (max: %u)\n",
+ msg->outsize,
+ ec_dev->max_passthru);
+ mutex_unlock(&ec_dev->lock);
+ return -EMSGSIZE;
}
}
+
+ ret = send_command(ec_dev, msg);
mutex_unlock(&ec_dev->lock);
return ret;
@@ -122,19 +391,22 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
struct device *dev = ec_dev->dev;
int err = 0;
- if (ec_dev->din_size) {
- ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
- if (!ec_dev->din)
- return -ENOMEM;
- }
- if (ec_dev->dout_size) {
- ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
- if (!ec_dev->dout)
- return -ENOMEM;
- }
+ ec_dev->max_request = sizeof(struct ec_params_hello);
+ ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
+ ec_dev->max_passthru = 0;
+
+ ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
+ if (!ec_dev->din)
+ return -ENOMEM;
+
+ ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
+ if (!ec_dev->dout)
+ return -ENOMEM;
mutex_init(&ec_dev->lock);
+ cros_ec_probe_all(ec_dev);
+
err = mfd_add_devices(dev, 0, cros_devs,
ARRAY_SIZE(cros_devs),
NULL, ec_dev->irq, NULL);
diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index fbf7819f5de5..b400bfa2772a 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -143,8 +143,12 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
ec_dev->priv = client;
ec_dev->irq = client->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
+ ec_dev->pkt_xfer = NULL;
ec_dev->ec_name = client->name;
ec_dev->phys_name = client->adapter->name;
+ ec_dev->din_size = sizeof(struct ec_host_response) +
+ sizeof(struct ec_response_get_protocol_info);
+ ec_dev->dout_size = sizeof(struct ec_host_request);
err = cros_ec_register(ec_dev);
if (err) {
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 573730fec947..04da2f288ef8 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -361,10 +361,13 @@ static int cros_ec_spi_probe(struct spi_device *spi)
ec_dev->priv = ec_spi;
ec_dev->irq = spi->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
+ ec_dev->pkt_xfer = NULL;
ec_dev->ec_name = ec_spi->spi->modalias;
ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
- ec_dev->din_size = EC_MSG_BYTES + EC_MSG_PREAMBLE_COUNT;
- ec_dev->dout_size = EC_MSG_BYTES;
+ ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
+ sizeof(struct ec_host_response) +
+ sizeof(struct ec_response_get_protocol_info);
+ ec_dev->dout_size = sizeof(struct ec_host_request);
err = cros_ec_register(ec_dev);
if (err) {
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 32b1df29fc58..d56dfb9dd463 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -205,7 +205,11 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
ec_dev->ec_name = pdev->name;
ec_dev->phys_name = dev_name(dev);
ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
+ ec_dev->pkt_xfer = NULL;
ec_dev->cmd_readmem = cros_ec_lpc_readmem;
+ ec_dev->din_size = sizeof(struct ec_host_response) +
+ sizeof(struct ec_response_get_protocol_info);
+ ec_dev->dout_size = sizeof(struct ec_host_request);
ret = cros_ec_register(ec_dev);
if (ret) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 7eee38abd02a..a2e87e81c956 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -22,6 +22,15 @@
#include <linux/mutex.h>
/*
+ * Max bus-specific overhead incurred by request/responses.
+ * I2C requires 1 additional byte for requests.
+ * I2C requires 2 additional bytes for responses.
+ * */
+#define EC_PROTO_VERSION_UNKNOWN 0
+#define EC_MAX_REQUEST_OVERHEAD 1
+#define EC_MAX_RESPONSE_OVERHEAD 2
+
+/*
* Command interface between EC and AP, for LPC, I2C and SPI interfaces.
*/
enum {
@@ -88,6 +97,7 @@ struct cros_ec_command {
* Returns the number of bytes received if the communication succeeded, but
* that doesn't mean the EC was happy with the command. The caller
* should check msg.result for the EC's result code.
+ * @pkt_xfer: send packet to EC and get response
* @lock: one transaction at a time
*/
struct cros_ec_device {
@@ -104,15 +114,21 @@ struct cros_ec_device {
unsigned int bytes, void *dest);
/* These are used to implement the platform-specific interface */
+ u16 max_request;
+ u16 max_response;
+ u16 max_passthru;
+ u16 proto_version;
void *priv;
int irq;
- uint8_t *din;
- uint8_t *dout;
+ u8 *din;
+ u8 *dout;
int din_size;
int dout_size;
bool wake_enabled;
int (*cmd_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
+ int (*pkt_xfer)(struct cros_ec_device *ec,
+ struct cros_ec_command *msg);
struct mutex lock;
};
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton
2015-05-09 10:10 ` [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton Javier Martinez Canillas
@ 2015-05-13 12:05 ` Lee Jones
2015-05-13 12:25 ` Javier Martinez Canillas
0 siblings, 1 reply; 29+ messages in thread
From: Lee Jones @ 2015-05-13 12:05 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> From: Stephen Barber <smbarber@chromium.org>
>
> Add support in cros_ec.c to handle EC host command protocol v3.
> For v3+, probe for maximum shared protocol version and max
> request, response, and passthrough sizes. For now, this will
> always fall back to v2, since there is no bus-specific code
> for handling proto v3 packets.
>
> Signed-off-by: Stephen Barber <smbarber@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> Tested-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>
> Changes since v1:
> - Squash change https://chromium-review.googlesource.com/#/c/262870/ in
> the patch. Suggested by Gwendal Grignou
> - Add Reviewed-by and Tested-by tags from Gwendal Grignou
> ---
> drivers/mfd/cros_ec.c | 354 ++++++++++++++++++++++++++++++----
> drivers/mfd/cros_ec_i2c.c | 4 +
> drivers/mfd/cros_ec_spi.c | 7 +-
> drivers/platform/chrome/cros_ec_lpc.c | 4 +
> include/linux/mfd/cros_ec.h | 20 +-
> 5 files changed, 344 insertions(+), 45 deletions(-)
This patch nearly triples the size of the driver, which I'm less than
happy about. This driver is starting to suffer from feature-creep
when comparing against the original implementation. I'd like to extract
'protocol drivers' out if possible. Would this be better suited in
drivers/platform?
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index ee8aa8142932..956a827de826 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -28,11 +28,276 @@
>
> #define EC_COMMAND_RETRIES 50
>
> +static int prepare_packet(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg)
> +{
> + struct ec_host_request *request;
> + u8 *out;
> + int i;
> + u8 csum = 0;
> +
> + BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
> + BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
> +
> + out = ec_dev->dout;
> + request = (struct ec_host_request *)out;
> + request->struct_version = EC_HOST_REQUEST_VERSION;
> + request->checksum = 0;
> + request->command = msg->command;
> + request->command_version = msg->version;
> + request->reserved = 0;
> + request->data_len = msg->outsize;
> +
> + for (i = 0; i < sizeof(*request); i++)
> + csum += out[i];
> +
> + /* Copy data and update checksum */
> + memcpy(out + sizeof(*request), msg->data, msg->outsize);
> + for (i = 0; i < msg->outsize; i++)
> + csum += msg->data[i];
> +
> + request->checksum = -csum;
> +
> + return sizeof(*request) + msg->outsize;
> +}
> +
> +static int send_command(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg)
> +{
> + int ret;
> +
> + if (ec_dev->proto_version > 2)
> + ret = ec_dev->pkt_xfer(ec_dev, msg);
> + else
> + ret = ec_dev->cmd_xfer(ec_dev, msg);
> +
> + if (msg->result == EC_RES_IN_PROGRESS) {
> + int i;
> + struct cros_ec_command *status_msg;
> + struct ec_response_get_comms_status *status;
> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
> +
> + status_msg = (struct cros_ec_command *)buf;
> + status_msg->command = EC_CMD_GET_COMMS_STATUS;
> + status_msg->insize = sizeof(*status);
> +
> + /*
> + * Query the EC's status until it's no longer busy or
> + * we encounter an error.
> + */
> + for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> + usleep_range(10000, 11000);
> +
> + ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> + if (ret < 0)
> + break;
> +
> + msg->result = status_msg->result;
> + if (status_msg->result != EC_RES_SUCCESS)
> + break;
> +
> + status = (struct ec_response_get_comms_status *)
> + status_msg->data;
> + if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int cros_ec_host_command_proto_probe(struct cros_ec_device *ec_dev,
> + int devidx,
> + struct cros_ec_command *msg)
> +{
> + /*
> + * Try using v3+ to query for supported protocols. If this
> + * command fails, fall back to v2. Returns the highest protocol
> + * supported by the EC.
> + * Also sets the max request/response/passthru size.
> + */
> + int ret;
> +
> + if (!ec_dev->pkt_xfer)
> + return -EPROTONOSUPPORT;
> +
> + memset(msg, 0, sizeof(*msg));
> + msg->command = EC_CMD_PASSTHRU_OFFSET(devidx) | EC_CMD_GET_PROTOCOL_INFO;
> + msg->insize = sizeof(struct ec_response_get_protocol_info);
> +
> + ret = send_command(ec_dev, msg);
> +
> + if (ret < 0) {
> + dev_dbg(ec_dev->dev,
> + "failed to probe for EC[%d] protocol version: %d\n",
> + devidx, ret);
> + return ret;
> + }
> +
> + if (devidx > 0 && msg->result == EC_RES_INVALID_COMMAND)
> + return -ENODEV;
> + else if (msg->result != EC_RES_SUCCESS)
> + return msg->result;
> +
> + return 0;
> +}
> +
> +static int cros_ec_host_command_proto_probe_v2(struct cros_ec_device *ec_dev)
> +{
> + struct cros_ec_command *msg;
> + struct ec_params_hello *hello_params;
> + struct ec_response_hello *hello_response;
> + int ret;
> + int len = max(sizeof(*hello_params), sizeof(*hello_response));
> +
> + msg = kzalloc(sizeof(*msg) + len, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + msg->command = EC_CMD_HELLO;
> + hello_params = (struct ec_params_hello *)msg->data;
> + msg->outsize = sizeof(*hello_params);
> + hello_response = (struct ec_response_hello *)msg->data;
> + msg->insize = sizeof(*hello_response);
> +
> + hello_params->in_data = 0xa0b0c0d0;
> +
> + ret = send_command(ec_dev, msg);
> +
> + if (ret < 0) {
> + dev_dbg(ec_dev->dev,
> + "EC failed to respond to v2 hello: %d\n",
> + ret);
> + goto exit;
> + } else if (msg->result != EC_RES_SUCCESS) {
> + dev_err(ec_dev->dev,
> + "EC responded to v2 hello with error: %d\n",
> + msg->result);
> + ret = msg->result;
> + goto exit;
> + } else if (hello_response->out_data != 0xa1b2c3d4) {
> + dev_err(ec_dev->dev,
> + "EC responded to v2 hello with bad result: %u\n",
> + hello_response->out_data);
> + ret = -EBADMSG;
> + goto exit;
> + }
> +
> + ret = 0;
> +
> +exit:
> + kfree(msg);
> + return ret;
> +}
> +
> +static int cros_ec_probe_all(struct cros_ec_device *ec_dev)
> +{
> + struct device *dev = ec_dev->dev;
> + struct cros_ec_command *proto_msg;
> + struct ec_response_get_protocol_info *proto_info;
> + int ret;
> +
> + proto_msg = kzalloc(sizeof(*proto_msg) + sizeof(*proto_info),
> + GFP_KERNEL);
> + if (!proto_msg)
> + return -ENOMEM;
> +
> + /* First try sending with proto v3. */
> + ec_dev->proto_version = 3;
> + ret = cros_ec_host_command_proto_probe(ec_dev, 0, proto_msg);
> +
> + if (ret == 0) {
> + proto_info = (struct ec_response_get_protocol_info *)
> + proto_msg->data;
> + ec_dev->max_request = proto_info->max_request_packet_size -
> + sizeof(struct ec_host_request);
> + ec_dev->max_response = proto_info->max_response_packet_size -
> + sizeof(struct ec_host_response);
> + ec_dev->proto_version =
> + min(EC_HOST_REQUEST_VERSION,
> + fls(proto_info->protocol_versions) - 1);
> + dev_dbg(ec_dev->dev,
> + "using proto v%u\n",
> + ec_dev->proto_version);
> +
> + ec_dev->din_size = ec_dev->max_response +
> + sizeof(struct ec_host_response) +
> + EC_MAX_RESPONSE_OVERHEAD;
> + ec_dev->dout_size = ec_dev->max_request +
> + sizeof(struct ec_host_request) +
> + EC_MAX_REQUEST_OVERHEAD;
> +
> + /*
> + * Check for PD
> + */
> + ret = cros_ec_host_command_proto_probe(ec_dev, 1, proto_msg);
> +
> + if (ret) {
> + dev_dbg(ec_dev->dev, "no PD chip found: %d\n", ret);
> + ec_dev->max_passthru = 0;
> + } else {
> + dev_dbg(ec_dev->dev, "found PD chip\n");
> + ec_dev->max_passthru =
> + proto_info->max_request_packet_size -
> + sizeof(struct ec_host_request);
> + }
> + } else {
> + /* Try probing with a v2 hello message. */
> + ec_dev->proto_version = 2;
> + ret = cros_ec_host_command_proto_probe_v2(ec_dev);
> +
> + if (ret == 0) {
> + /* V2 hello succeeded. */
> + dev_dbg(ec_dev->dev, "falling back to proto v2\n");
> +
> + ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
> + ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
> + ec_dev->max_passthru = 0;
> + ec_dev->pkt_xfer = NULL;
> + ec_dev->din_size = EC_MSG_BYTES;
> + ec_dev->dout_size = EC_MSG_BYTES;
> + } else {
> + /*
> + * It's possible for a probe to occur too early when
> + * the EC isn't listening. If this happens, we'll
> + * probe later when the first command is run.
> + */
> + ec_dev->proto_version = EC_PROTO_VERSION_UNKNOWN;
> + dev_dbg(ec_dev->dev, "EC probe failed: %d\n", ret);
> + goto exit;
> + }
> + }
> +
> + devm_kfree(dev, ec_dev->din);
> + devm_kfree(dev, ec_dev->dout);
> +
> + ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> + if (!ec_dev->din) {
> + ret = -ENOMEM;
> + goto exit;
> + }
> +
> + ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
> + if (!ec_dev->dout) {
> + devm_kfree(dev, ec_dev->din);
> + ret = -ENOMEM;
> + goto exit;
> + }
> +
> +exit:
> + kfree(proto_msg);
> + return ret;
> +}
> +
> int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> struct cros_ec_command *msg)
> {
> - uint8_t *out;
> - int csum, i;
> + u8 *out;
> + u8 csum;
> + int i;
> +
> + if (ec_dev->proto_version > 2)
> + return prepare_packet(ec_dev, msg);
>
> BUG_ON(msg->outsize > EC_PROTO2_MAX_PARAM_SIZE);
> out = ec_dev->dout;
> @@ -42,7 +307,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> csum = out[0] + out[1] + out[2];
> for (i = 0; i < msg->outsize; i++)
> csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
> - out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
> + out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = csum;
>
> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
> }
> @@ -72,38 +337,42 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> int ret;
>
> mutex_lock(&ec_dev->lock);
> - ret = ec_dev->cmd_xfer(ec_dev, msg);
> - if (msg->result == EC_RES_IN_PROGRESS) {
> - int i;
> - struct cros_ec_command *status_msg;
> - struct ec_response_get_comms_status *status;
> - u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
> -
> - status_msg = (struct cros_ec_command *)buf;
> - status_msg->command = EC_CMD_GET_COMMS_STATUS;
> - status_msg->insize = sizeof(*status);
> -
> - /*
> - * Query the EC's status until it's no longer busy or
> - * we encounter an error.
> - */
> - for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> - usleep_range(10000, 11000);
> -
> - ret = ec_dev->cmd_xfer(ec_dev, status_msg);
> - if (ret < 0)
> - break;
> + if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
> + ret = cros_ec_probe_all(ec_dev);
> + if (ret) {
> + dev_err(ec_dev->dev,
> + "EC version unknown and probe failed; aborting command\n");
> + mutex_unlock(&ec_dev->lock);
> + return ret;
> + }
> + }
>
> - msg->result = status_msg->result;
> - if (status_msg->result != EC_RES_SUCCESS)
> - break;
> + if (msg->insize > ec_dev->max_response) {
> + dev_dbg(ec_dev->dev, "clamping message receive buffer\n");
> + msg->insize = ec_dev->max_response;
> + }
>
> - status = (struct ec_response_get_comms_status *)
> - status_msg->data;
> - if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> - break;
> + if (msg->command < EC_CMD_PASSTHRU_OFFSET(1)) {
> + if (msg->outsize > ec_dev->max_request) {
> + dev_err(ec_dev->dev,
> + "request of size %u is too big (max: %u)\n",
> + msg->outsize,
> + ec_dev->max_request);
> + mutex_unlock(&ec_dev->lock);
> + return -EMSGSIZE;
> + }
> + } else {
> + if (msg->outsize > ec_dev->max_passthru) {
> + dev_err(ec_dev->dev,
> + "passthru rq of size %u is too big (max: %u)\n",
> + msg->outsize,
> + ec_dev->max_passthru);
> + mutex_unlock(&ec_dev->lock);
> + return -EMSGSIZE;
> }
> }
> +
> + ret = send_command(ec_dev, msg);
> mutex_unlock(&ec_dev->lock);
>
> return ret;
> @@ -122,19 +391,22 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> struct device *dev = ec_dev->dev;
> int err = 0;
>
> - if (ec_dev->din_size) {
> - ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> - if (!ec_dev->din)
> - return -ENOMEM;
> - }
> - if (ec_dev->dout_size) {
> - ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
> - if (!ec_dev->dout)
> - return -ENOMEM;
> - }
> + ec_dev->max_request = sizeof(struct ec_params_hello);
> + ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
> + ec_dev->max_passthru = 0;
> +
> + ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> + if (!ec_dev->din)
> + return -ENOMEM;
> +
> + ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
> + if (!ec_dev->dout)
> + return -ENOMEM;
>
> mutex_init(&ec_dev->lock);
>
> + cros_ec_probe_all(ec_dev);
> +
> err = mfd_add_devices(dev, 0, cros_devs,
> ARRAY_SIZE(cros_devs),
> NULL, ec_dev->irq, NULL);
> diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
> index fbf7819f5de5..b400bfa2772a 100644
> --- a/drivers/mfd/cros_ec_i2c.c
> +++ b/drivers/mfd/cros_ec_i2c.c
> @@ -143,8 +143,12 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
> ec_dev->priv = client;
> ec_dev->irq = client->irq;
> ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
> + ec_dev->pkt_xfer = NULL;
> ec_dev->ec_name = client->name;
> ec_dev->phys_name = client->adapter->name;
> + ec_dev->din_size = sizeof(struct ec_host_response) +
> + sizeof(struct ec_response_get_protocol_info);
> + ec_dev->dout_size = sizeof(struct ec_host_request);
>
> err = cros_ec_register(ec_dev);
> if (err) {
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index 573730fec947..04da2f288ef8 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -361,10 +361,13 @@ static int cros_ec_spi_probe(struct spi_device *spi)
> ec_dev->priv = ec_spi;
> ec_dev->irq = spi->irq;
> ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
> + ec_dev->pkt_xfer = NULL;
> ec_dev->ec_name = ec_spi->spi->modalias;
> ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
> - ec_dev->din_size = EC_MSG_BYTES + EC_MSG_PREAMBLE_COUNT;
> - ec_dev->dout_size = EC_MSG_BYTES;
> + ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
> + sizeof(struct ec_host_response) +
> + sizeof(struct ec_response_get_protocol_info);
> + ec_dev->dout_size = sizeof(struct ec_host_request);
>
> err = cros_ec_register(ec_dev);
> if (err) {
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 32b1df29fc58..d56dfb9dd463 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -205,7 +205,11 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> ec_dev->ec_name = pdev->name;
> ec_dev->phys_name = dev_name(dev);
> ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
> + ec_dev->pkt_xfer = NULL;
> ec_dev->cmd_readmem = cros_ec_lpc_readmem;
> + ec_dev->din_size = sizeof(struct ec_host_response) +
> + sizeof(struct ec_response_get_protocol_info);
> + ec_dev->dout_size = sizeof(struct ec_host_request);
>
> ret = cros_ec_register(ec_dev);
> if (ret) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 7eee38abd02a..a2e87e81c956 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -22,6 +22,15 @@
> #include <linux/mutex.h>
>
> /*
> + * Max bus-specific overhead incurred by request/responses.
> + * I2C requires 1 additional byte for requests.
> + * I2C requires 2 additional bytes for responses.
> + * */
> +#define EC_PROTO_VERSION_UNKNOWN 0
> +#define EC_MAX_REQUEST_OVERHEAD 1
> +#define EC_MAX_RESPONSE_OVERHEAD 2
> +
> +/*
> * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
> */
> enum {
> @@ -88,6 +97,7 @@ struct cros_ec_command {
> * Returns the number of bytes received if the communication succeeded, but
> * that doesn't mean the EC was happy with the command. The caller
> * should check msg.result for the EC's result code.
> + * @pkt_xfer: send packet to EC and get response
> * @lock: one transaction at a time
> */
> struct cros_ec_device {
> @@ -104,15 +114,21 @@ struct cros_ec_device {
> unsigned int bytes, void *dest);
>
> /* These are used to implement the platform-specific interface */
> + u16 max_request;
> + u16 max_response;
> + u16 max_passthru;
> + u16 proto_version;
> void *priv;
> int irq;
> - uint8_t *din;
> - uint8_t *dout;
> + u8 *din;
> + u8 *dout;
> int din_size;
> int dout_size;
> bool wake_enabled;
> int (*cmd_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> + int (*pkt_xfer)(struct cros_ec_device *ec,
> + struct cros_ec_command *msg);
> struct mutex lock;
> };
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton
2015-05-13 12:05 ` Lee Jones
@ 2015-05-13 12:25 ` Javier Martinez Canillas
0 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-13 12:25 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel
Hello Lee,
Thanks a lot for your feedback.
On 05/13/2015 02:05 PM, Lee Jones wrote:
> On Sat, 09 May 2015, Javier Martinez Canillas wrote:
>
>> From: Stephen Barber <smbarber@chromium.org>
>>
>> Add support in cros_ec.c to handle EC host command protocol v3.
>> For v3+, probe for maximum shared protocol version and max
>> request, response, and passthrough sizes. For now, this will
>> always fall back to v2, since there is no bus-specific code
>> for handling proto v3 packets.
>>
>> Signed-off-by: Stephen Barber <smbarber@chromium.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
>> Tested-by: Gwendal Grignou <gwendal@chromium.org>
>> ---
>>
>> Changes since v1:
>> - Squash change https://chromium-review.googlesource.com/#/c/262870/ in
>> the patch. Suggested by Gwendal Grignou
>> - Add Reviewed-by and Tested-by tags from Gwendal Grignou
>> ---
>> drivers/mfd/cros_ec.c | 354 ++++++++++++++++++++++++++++++----
>> drivers/mfd/cros_ec_i2c.c | 4 +
>> drivers/mfd/cros_ec_spi.c | 7 +-
>> drivers/platform/chrome/cros_ec_lpc.c | 4 +
>> include/linux/mfd/cros_ec.h | 20 +-
>> 5 files changed, 344 insertions(+), 45 deletions(-)
>
> This patch nearly triples the size of the driver, which I'm less than
> happy about. This driver is starting to suffer from feature-creep
> when comparing against the original implementation. I'd like to extract
> 'protocol drivers' out if possible. Would this be better suited in
> drivers/platform?
>
Agreed that this mfd driver is getting too complex. I'll see how I can
split the communication/transport logic into a separate driver under
platforms/chrome and only keep the mfd cells registration logic in
drivers/mfd/cros_ec.c
Best regards,
Javier
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 07/10] mfd: cros_ec: add bus-specific proto v3 code
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (5 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 06/10] mfd: cros_ec: add proto v3 skeleton Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 08/10] mfd: cros_ec: Support multiple EC in a system Javier Martinez Canillas
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
From: Stephen Barber <smbarber@chromium.org>
Add proto v3 support to the SPI, I2C, and LPC.
Signed-off-by: Stephen Barber <smbarber@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>
---
Changes since v1:
- Added Gwendal Grignou Reviewed-by and Tested-by tags
---
drivers/mfd/cros_ec_i2c.c | 166 ++++++++++++++-
drivers/mfd/cros_ec_spi.c | 382 +++++++++++++++++++++++++++++-----
drivers/platform/chrome/cros_ec_lpc.c | 73 ++++++-
include/linux/mfd/cros_ec.h | 6 +
4 files changed, 569 insertions(+), 58 deletions(-)
diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index b400bfa2772a..22e8a4ae1711 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -13,6 +13,7 @@
* GNU General Public License for more details.
*/
+#include <linux/delay.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/i2c.h>
@@ -22,6 +23,32 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
+/**
+ * Request format for protocol v3
+ * byte 0 0xda (EC_COMMAND_PROTOCOL_3)
+ * byte 1-8 struct ec_host_request
+ * byte 10- response data
+ */
+struct ec_host_request_i2c {
+ /* Always 0xda to backward compatible with v2 struct */
+ uint8_t command_protocol;
+ struct ec_host_request ec_request;
+} __packed;
+
+
+/*
+ * Response format for protocol v3
+ * byte 0 result code
+ * byte 1 packet_length
+ * byte 2-9 struct ec_host_response
+ * byte 10- response data
+ */
+struct ec_host_response_i2c {
+ uint8_t result;
+ uint8_t packet_length;
+ struct ec_host_response ec_response;
+} __packed;
+
static inline struct cros_ec_device *to_ec_dev(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -29,6 +56,134 @@ static inline struct cros_ec_device *to_ec_dev(struct device *dev)
return i2c_get_clientdata(client);
}
+static int cros_ec_pkt_xfer_i2c(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *msg)
+{
+ struct i2c_client *client = ec_dev->priv;
+ int ret = -ENOMEM;
+ int i;
+ int packet_len;
+ u8 *out_buf = NULL;
+ u8 *in_buf = NULL;
+ u8 sum;
+ struct i2c_msg i2c_msg[2];
+ struct ec_host_response *ec_response;
+ struct ec_host_request_i2c *ec_request_i2c;
+ struct ec_host_response_i2c *ec_response_i2c;
+ int request_header_size = sizeof(struct ec_host_request_i2c);
+ int response_header_size = sizeof(struct ec_host_response_i2c);
+
+ i2c_msg[0].addr = client->addr;
+ i2c_msg[0].flags = 0;
+ i2c_msg[1].addr = client->addr;
+ i2c_msg[1].flags = I2C_M_RD;
+
+ packet_len = msg->insize + response_header_size;
+ BUG_ON(packet_len > ec_dev->din_size);
+ in_buf = ec_dev->din;
+ i2c_msg[1].len = packet_len;
+ i2c_msg[1].buf = (char *) in_buf;
+
+ packet_len = msg->outsize + request_header_size;
+ BUG_ON(packet_len > ec_dev->dout_size);
+ out_buf = ec_dev->dout;
+ i2c_msg[0].len = packet_len;
+ i2c_msg[0].buf = (char *) out_buf;
+
+ /* create request data */
+ ec_request_i2c = (struct ec_host_request_i2c *) out_buf;
+ ec_request_i2c->command_protocol = EC_COMMAND_PROTOCOL_3;
+
+ ec_dev->dout++;
+ ret = cros_ec_prepare_tx(ec_dev, msg);
+ ec_dev->dout--;
+
+ /* send command to EC and read answer */
+ ret = i2c_transfer(client->adapter, i2c_msg, 2);
+ if (ret < 0) {
+ dev_dbg(ec_dev->dev, "i2c transfer failed: %d\n", ret);
+ goto done;
+ } else if (ret != 2) {
+ dev_err(ec_dev->dev, "failed to get response: %d\n", ret);
+ ret = -EIO;
+ goto done;
+ }
+
+ ec_response_i2c = (struct ec_host_response_i2c *) in_buf;
+ msg->result = ec_response_i2c->result;
+ ec_response = &ec_response_i2c->ec_response;
+
+ switch (msg->result) {
+ case EC_RES_SUCCESS:
+ break;
+ case EC_RES_IN_PROGRESS:
+ ret = -EAGAIN;
+ dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
+ msg->command);
+ goto done;
+
+ default:
+ dev_dbg(ec_dev->dev, "command 0x%02x returned %d\n",
+ msg->command, msg->result);
+ /*
+ * When we send v3 request to v2 ec, ec won't recognize the
+ * 0xda (EC_COMMAND_PROTOCOL_3) and will return with status
+ * EC_RES_INVALID_COMMAND with zero data length.
+ *
+ * In case of invalid command for v3 protocol the data length
+ * will be at least sizeof(struct ec_host_response)
+ */
+ if (ec_response_i2c->result == EC_RES_INVALID_COMMAND &&
+ ec_response_i2c->packet_length == 0) {
+ ret = -EPROTONOSUPPORT;
+ goto done;
+ }
+ }
+
+ if (ec_response_i2c->packet_length < sizeof(struct ec_host_response)) {
+ dev_err(ec_dev->dev,
+ "response of %u bytes too short; not a full header\n",
+ ec_response_i2c->packet_length);
+ ret = -EBADMSG;
+ goto done;
+ }
+
+ if (msg->insize < ec_response->data_len) {
+ dev_err(ec_dev->dev,
+ "response data size is too large: expected %u, got %u\n",
+ msg->insize,
+ ec_response->data_len);
+ ret = -EMSGSIZE;
+ goto done;
+ }
+
+ /* copy response packet payload and compute checksum */
+ sum = 0;
+ for (i = 0; i < sizeof(struct ec_host_response); i++)
+ sum += ((u8 *)ec_response)[i];
+
+ memcpy(msg->data,
+ in_buf + response_header_size,
+ ec_response->data_len);
+ for (i = 0; i < ec_response->data_len; i++)
+ sum += msg->data[i];
+
+ /* All bytes should sum to zero */
+ if (sum) {
+ dev_err(ec_dev->dev, "bad packet checksum\n");
+ ret = -EBADMSG;
+ goto done;
+ }
+
+ ret = ec_response->data_len;
+
+done:
+ if (msg->command == EC_CMD_REBOOT_EC)
+ msleep(EC_REBOOT_DELAY_MS);
+
+ return ret;
+}
+
static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
{
@@ -121,9 +276,12 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
}
ret = len;
- done:
+done:
kfree(in_buf);
kfree(out_buf);
+ if (msg->command == EC_CMD_REBOOT_EC)
+ msleep(EC_REBOOT_DELAY_MS);
+
return ret;
}
@@ -143,12 +301,12 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
ec_dev->priv = client;
ec_dev->irq = client->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
- ec_dev->pkt_xfer = NULL;
+ ec_dev->pkt_xfer = cros_ec_pkt_xfer_i2c;
ec_dev->ec_name = client->name;
ec_dev->phys_name = client->adapter->name;
- ec_dev->din_size = sizeof(struct ec_host_response) +
+ ec_dev->din_size = sizeof(struct ec_host_response_i2c) +
sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request);
+ ec_dev->dout_size = sizeof(struct ec_host_request_i2c);
err = cros_ec_register(ec_dev);
if (err) {
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 04da2f288ef8..4e6f2f6b1095 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -65,12 +65,6 @@
*/
#define EC_SPI_RECOVERY_TIME_NS (200 * 1000)
-/*
- * The EC is unresponsive for a time after a reboot command. Add a
- * simple delay to make sure that the bus stays locked.
- */
-#define EC_REBOOT_DELAY_MS 50
-
/**
* struct cros_ec_spi - information about a SPI-connected EC
*
@@ -87,7 +81,7 @@ struct cros_ec_spi {
};
static void debug_packet(struct device *dev, const char *name, u8 *ptr,
- int len)
+ int len)
{
#ifdef DEBUG
int i;
@@ -100,6 +94,172 @@ static void debug_packet(struct device *dev, const char *name, u8 *ptr,
#endif
}
+static int terminate_request(struct cros_ec_device *ec_dev)
+{
+ struct cros_ec_spi *ec_spi = ec_dev->priv;
+ struct spi_message msg;
+ struct spi_transfer trans;
+ int ret;
+
+ /*
+ * Turn off CS, possibly adding a delay to ensure the rising edge
+ * doesn't come too soon after the end of the data.
+ */
+ spi_message_init(&msg);
+ memset(&trans, 0, sizeof(trans));
+ trans.delay_usecs = ec_spi->end_of_msg_delay;
+ spi_message_add_tail(&trans, &msg);
+
+ ret = spi_sync(ec_spi->spi, &msg);
+
+ /* Reset end-of-response timer */
+ ec_spi->last_transfer_ns = ktime_get_ns();
+ if (ret < 0) {
+ dev_err(ec_dev->dev,
+ "cs-deassert spi transfer failed: %d\n",
+ ret);
+ }
+
+ return ret;
+}
+
+/**
+ * receive_n_bytes - receive n bytes from the EC.
+ *
+ * Assumes buf is a pointer into the ec_dev->din buffer
+ */
+static int receive_n_bytes(struct cros_ec_device *ec_dev, u8 *buf, int n)
+{
+ struct cros_ec_spi *ec_spi = ec_dev->priv;
+ struct spi_transfer trans;
+ struct spi_message msg;
+ int ret;
+
+ BUG_ON(buf - ec_dev->din + n > ec_dev->din_size);
+
+ memset(&trans, 0, sizeof(trans));
+ trans.cs_change = 1;
+ trans.rx_buf = buf;
+ trans.len = n;
+
+ spi_message_init(&msg);
+ spi_message_add_tail(&trans, &msg);
+ ret = spi_sync(ec_spi->spi, &msg);
+ if (ret < 0)
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
+ return ret;
+}
+
+/**
+ * cros_ec_spi_receive_packet - Receive a packet from the EC.
+ *
+ * This function has two phases: reading the preamble bytes (since if we read
+ * data from the EC before it is ready to send, we just get preamble) and
+ * reading the actual message.
+ *
+ * The received data is placed into ec_dev->din.
+ *
+ * @ec_dev: ChromeOS EC device
+ * @need_len: Number of message bytes we need to read
+ */
+static int cros_ec_spi_receive_packet(struct cros_ec_device *ec_dev,
+ int need_len)
+{
+ struct ec_host_response *response;
+ u8 *ptr, *end;
+ int ret;
+ unsigned long deadline;
+ int todo;
+
+ BUG_ON(EC_MSG_PREAMBLE_COUNT > ec_dev->din_size);
+
+ /* Receive data until we see the header byte */
+ deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
+ while (true) {
+ unsigned long start_jiffies = jiffies;
+
+ ret = receive_n_bytes(ec_dev,
+ ec_dev->din,
+ EC_MSG_PREAMBLE_COUNT);
+ if (ret < 0)
+ return ret;
+
+ ptr = ec_dev->din;
+ for (end = ptr + EC_MSG_PREAMBLE_COUNT; ptr != end; ptr++) {
+ if (*ptr == EC_SPI_FRAME_START) {
+ dev_dbg(ec_dev->dev, "msg found at %zd\n",
+ ptr - ec_dev->din);
+ break;
+ }
+ }
+ if (ptr != end)
+ break;
+
+ /*
+ * Use the time at the start of the loop as a timeout. This
+ * gives us one last shot at getting the transfer and is useful
+ * in case we got context switched out for a while.
+ */
+ if (time_after(start_jiffies, deadline)) {
+ dev_warn(ec_dev->dev, "EC failed to respond in time\n");
+ return -ETIMEDOUT;
+ }
+ }
+
+ /*
+ * ptr now points to the header byte. Copy any valid data to the
+ * start of our buffer
+ */
+ todo = end - ++ptr;
+ BUG_ON(todo < 0 || todo > ec_dev->din_size);
+ todo = min(todo, need_len);
+ memmove(ec_dev->din, ptr, todo);
+ ptr = ec_dev->din + todo;
+ dev_dbg(ec_dev->dev, "need %d, got %d bytes from preamble\n",
+ need_len, todo);
+ need_len -= todo;
+
+ /* If the entire response struct wasn't read, get the rest of it. */
+ if (todo < sizeof(*response)) {
+ ret = receive_n_bytes(ec_dev, ptr, sizeof(*response) - todo);
+ if (ret < 0)
+ return -EBADMSG;
+ ptr += (sizeof(*response) - todo);
+ todo = sizeof(*response);
+ }
+
+ response = (struct ec_host_response *)ec_dev->din;
+
+ /* Abort if data_len is too large. */
+ if (response->data_len > ec_dev->din_size)
+ return -EMSGSIZE;
+
+ /* Receive data until we have it all */
+ while (need_len > 0) {
+ /*
+ * We can't support transfers larger than the SPI FIFO size
+ * unless we have DMA. We don't have DMA on the ISP SPI ports
+ * for Exynos. We need a way of asking SPI driver for
+ * maximum-supported transfer size.
+ */
+ todo = min(need_len, 256);
+ dev_dbg(ec_dev->dev, "loop, todo=%d, need_len=%d, ptr=%zd\n",
+ todo, need_len, ptr - ec_dev->din);
+
+ ret = receive_n_bytes(ec_dev, ptr, todo);
+ if (ret < 0)
+ return ret;
+
+ ptr += todo;
+ need_len -= todo;
+ }
+
+ dev_dbg(ec_dev->dev, "loop done, ptr=%zd\n", ptr - ec_dev->din);
+
+ return 0;
+}
+
/**
* cros_ec_spi_receive_response - Receive a response from the EC.
*
@@ -115,34 +275,27 @@ static void debug_packet(struct device *dev, const char *name, u8 *ptr,
static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
int need_len)
{
- struct cros_ec_spi *ec_spi = ec_dev->priv;
- struct spi_transfer trans;
- struct spi_message msg;
u8 *ptr, *end;
int ret;
unsigned long deadline;
int todo;
+ BUG_ON(EC_MSG_PREAMBLE_COUNT > ec_dev->din_size);
+
/* Receive data until we see the header byte */
deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
while (true) {
unsigned long start_jiffies = jiffies;
- memset(&trans, 0, sizeof(trans));
- trans.cs_change = 1;
- trans.rx_buf = ptr = ec_dev->din;
- trans.len = EC_MSG_PREAMBLE_COUNT;
-
- spi_message_init(&msg);
- spi_message_add_tail(&trans, &msg);
- ret = spi_sync(ec_spi->spi, &msg);
- if (ret < 0) {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+ ret = receive_n_bytes(ec_dev,
+ ec_dev->din,
+ EC_MSG_PREAMBLE_COUNT);
+ if (ret < 0)
return ret;
- }
+ ptr = ec_dev->din;
for (end = ptr + EC_MSG_PREAMBLE_COUNT; ptr != end; ptr++) {
- if (*ptr == EC_MSG_HEADER) {
+ if (*ptr == EC_SPI_FRAME_START) {
dev_dbg(ec_dev->dev, "msg found at %zd\n",
ptr - ec_dev->din);
break;
@@ -187,21 +340,9 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
dev_dbg(ec_dev->dev, "loop, todo=%d, need_len=%d, ptr=%zd\n",
todo, need_len, ptr - ec_dev->din);
- memset(&trans, 0, sizeof(trans));
- trans.cs_change = 1;
- trans.rx_buf = ptr;
- trans.len = todo;
- spi_message_init(&msg);
- spi_message_add_tail(&trans, &msg);
-
- /* send command to EC and read answer */
- BUG_ON((u8 *)trans.rx_buf - ec_dev->din + todo >
- ec_dev->din_size);
- ret = spi_sync(ec_spi->spi, &msg);
- if (ret < 0) {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+ ret = receive_n_bytes(ec_dev, ptr, todo);
+ if (ret < 0)
return ret;
- }
debug_packet(ec_dev->dev, "interim", ptr, todo);
ptr += todo;
@@ -214,6 +355,128 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
}
/**
+ * cros_ec_pkt_xfer_spi - Transfer a packet over SPI and receive the reply
+ *
+ * @ec_dev: ChromeOS EC device
+ * @ec_msg: Message to transfer
+ */
+static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *ec_msg)
+{
+ struct ec_host_request *request;
+ struct ec_host_response *response;
+ struct cros_ec_spi *ec_spi = ec_dev->priv;
+ struct spi_transfer trans;
+ struct spi_message msg;
+ int i, len;
+ u8 *ptr;
+ u8 *rx_buf;
+ u8 sum;
+ int ret = 0, final_ret;
+
+ len = cros_ec_prepare_tx(ec_dev, ec_msg);
+ request = (struct ec_host_request *)ec_dev->dout;
+ dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
+
+ /* If it's too soon to do another transaction, wait */
+ if (ec_spi->last_transfer_ns) {
+ unsigned long delay; /* The delay completed so far */
+
+ delay = ktime_get_ns() - ec_spi->last_transfer_ns;
+ if (delay < EC_SPI_RECOVERY_TIME_NS)
+ ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
+ }
+
+ rx_buf = kzalloc(len, GFP_KERNEL);
+ if (!rx_buf) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+
+ /* Transmit phase - send our message */
+ memset(&trans, 0, sizeof(trans));
+ trans.tx_buf = ec_dev->dout;
+ trans.rx_buf = rx_buf;
+ trans.len = len;
+ trans.cs_change = 1;
+ spi_message_init(&msg);
+ spi_message_add_tail(&trans, &msg);
+ ret = spi_sync(ec_spi->spi, &msg);
+
+ /* Get the response */
+ if (!ret) {
+ /* Verify that EC can process command */
+ for (i = 0; i < len; i++) {
+ switch (rx_buf[i]) {
+ case EC_SPI_PAST_END:
+ case EC_SPI_RX_BAD_DATA:
+ case EC_SPI_NOT_READY:
+ ret = -EAGAIN;
+ ec_msg->result = EC_RES_IN_PROGRESS;
+ default:
+ break;
+ }
+ if (ret)
+ break;
+ }
+ if (!ret)
+ ret = cros_ec_spi_receive_packet(ec_dev,
+ ec_msg->insize + sizeof(*response));
+ } else {
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+ }
+
+ final_ret = terminate_request(ec_dev);
+ if (!ret)
+ ret = final_ret;
+ if (ret < 0)
+ goto exit;
+
+ ptr = ec_dev->din;
+
+ /* check response error code */
+ response = (struct ec_host_response *)ptr;
+ ec_msg->result = response->result;
+
+ ret = cros_ec_check_result(ec_dev, ec_msg);
+ if (ret)
+ goto exit;
+
+ len = response->data_len;
+ sum = 0;
+ if (len > ec_msg->insize) {
+ dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
+ len, ec_msg->insize);
+ ret = -EMSGSIZE;
+ goto exit;
+ }
+
+ for (i = 0; i < sizeof(*response); i++)
+ sum += ptr[i];
+
+ /* copy response packet payload and compute checksum */
+ memcpy(ec_msg->data, ptr + sizeof(*response), len);
+ for (i = 0; i < len; i++)
+ sum += ec_msg->data[i];
+
+ if (sum) {
+ dev_err(ec_dev->dev,
+ "bad packet checksum, calculated %x\n",
+ sum);
+ ret = -EBADMSG;
+ goto exit;
+ }
+
+ ret = len;
+exit:
+ kfree(rx_buf);
+ if (ec_msg->command == EC_CMD_REBOOT_EC)
+ msleep(EC_REBOOT_DELAY_MS);
+
+ return ret;
+}
+
+/**
* cros_ec_cmd_xfer_spi - Transfer a message over SPI and receive the reply
*
* @ec_dev: ChromeOS EC device
@@ -227,6 +490,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
struct spi_message msg;
int i, len;
u8 *ptr;
+ u8 *rx_buf;
int sum;
int ret = 0, final_ret;
@@ -242,10 +506,17 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
}
+ rx_buf = kzalloc(len, GFP_KERNEL);
+ if (!rx_buf) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+
/* Transmit phase - send our message */
debug_packet(ec_dev->dev, "out", ec_dev->dout, len);
memset(&trans, 0, sizeof(trans));
trans.tx_buf = ec_dev->dout;
+ trans.rx_buf = rx_buf;
trans.len = len;
trans.cs_change = 1;
spi_message_init(&msg);
@@ -254,29 +525,32 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
/* Get the response */
if (!ret) {
- ret = cros_ec_spi_receive_response(ec_dev,
- ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
+ /* Verify that EC can process command */
+ for (i = 0; i < len; i++) {
+ switch (rx_buf[i]) {
+ case EC_SPI_PAST_END:
+ case EC_SPI_RX_BAD_DATA:
+ case EC_SPI_NOT_READY:
+ ret = -EAGAIN;
+ ec_msg->result = EC_RES_IN_PROGRESS;
+ default:
+ break;
+ }
+ if (ret)
+ break;
+ }
+ if (!ret)
+ ret = cros_ec_spi_receive_response(ec_dev,
+ ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
} else {
dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
}
- /*
- * Turn off CS, possibly adding a delay to ensure the rising edge
- * doesn't come too soon after the end of the data.
- */
- spi_message_init(&msg);
- memset(&trans, 0, sizeof(trans));
- trans.delay_usecs = ec_spi->end_of_msg_delay;
- spi_message_add_tail(&trans, &msg);
-
- final_ret = spi_sync(ec_spi->spi, &msg);
- ec_spi->last_transfer_ns = ktime_get_ns();
+ final_ret = terminate_request(ec_dev);
if (!ret)
ret = final_ret;
- if (ret < 0) {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+ if (ret < 0)
goto exit;
- }
ptr = ec_dev->din;
@@ -315,6 +589,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
ret = len;
exit:
+ kfree(rx_buf);
if (ec_msg->command == EC_CMD_REBOOT_EC)
msleep(EC_REBOOT_DELAY_MS);
@@ -361,7 +636,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
ec_dev->priv = ec_spi;
ec_dev->irq = spi->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
- ec_dev->pkt_xfer = NULL;
+ ec_dev->pkt_xfer = cros_ec_pkt_xfer_spi;
ec_dev->ec_name = ec_spi->spi->modalias;
ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
@@ -369,6 +644,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
sizeof(struct ec_response_get_protocol_info);
ec_dev->dout_size = sizeof(struct ec_host_request);
+
err = cros_ec_register(ec_dev);
if (err) {
dev_err(dev, "cannot register EC\n");
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d56dfb9dd463..e98b0345288a 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -46,6 +46,77 @@ static int ec_response_timed_out(void)
return 1;
}
+static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
+ struct cros_ec_command *msg)
+{
+ struct ec_host_request *request;
+ struct ec_host_response response;
+ u8 sum = 0;
+ int i;
+ int ret = 0;
+ u8 *dout;
+
+ ret = cros_ec_prepare_tx(ec, msg);
+
+ /* Write buffer */
+ for (i = 0; i < ret; i++)
+ outb(ec->dout[i], EC_LPC_ADDR_HOST_PACKET + i);
+
+ request = (struct ec_host_request *)ec->dout;
+
+ /* Here we go */
+ outb(EC_COMMAND_PROTOCOL_3, EC_LPC_ADDR_HOST_CMD);
+
+ if (ec_response_timed_out()) {
+ dev_warn(ec->dev, "EC responsed timed out\n");
+ ret = -EIO;
+ goto done;
+ }
+
+ /* Check result */
+ msg->result = inb(EC_LPC_ADDR_HOST_DATA);
+ ret = cros_ec_check_result(ec, msg);
+ if (ret)
+ goto done;
+
+ /* Read back response */
+ dout = (u8 *)&response;
+ for (i = 0; i < sizeof(response); i++) {
+ dout[i] = inb(EC_LPC_ADDR_HOST_PACKET + i);
+ sum += dout[i];
+ }
+
+ msg->result = response.result;
+
+ if (response.data_len > msg->insize) {
+ dev_err(ec->dev,
+ "packet too long (%d bytes, expected %d)",
+ response.data_len, msg->insize);
+ ret = -EMSGSIZE;
+ goto done;
+ }
+
+ /* Read response and process checksum */
+ for (i = 0; i < response.data_len; i++) {
+ msg->data[i] =
+ inb(EC_LPC_ADDR_HOST_PACKET + sizeof(response) + i);
+ sum += msg->data[i];
+ }
+
+ if (sum) {
+ dev_err(ec->dev,
+ "bad packet checksum %02x\n",
+ response.checksum);
+ ret = -EBADMSG;
+ goto done;
+ }
+
+ /* Return actual amount of data received */
+ ret = response.data_len;
+done:
+ return ret;
+}
+
static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
struct cros_ec_command *msg)
{
@@ -205,7 +276,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
ec_dev->ec_name = pdev->name;
ec_dev->phys_name = dev_name(dev);
ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
- ec_dev->pkt_xfer = NULL;
+ ec_dev->pkt_xfer = cros_ec_pkt_xfer_lpc;
ec_dev->cmd_readmem = cros_ec_lpc_readmem;
ec_dev->din_size = sizeof(struct ec_host_response) +
sizeof(struct ec_response_get_protocol_info);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index a2e87e81c956..15dc8317a767 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -22,6 +22,12 @@
#include <linux/mutex.h>
/*
+ * The EC is unresponsive for a time after a reboot command. Add a
+ * simple delay to make sure that the bus stays locked.
+ */
+#define EC_REBOOT_DELAY_MS 50
+
+/*
* Max bus-specific overhead incurred by request/responses.
* I2C requires 1 additional byte for requests.
* I2C requires 2 additional bytes for responses.
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 08/10] mfd: cros_ec: Support multiple EC in a system
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (6 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 07/10] mfd: cros_ec: add bus-specific proto v3 code Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 09/10] platform/chrome: cros_ec_lpc - Add support for Google Pixel 2 Javier Martinez Canillas
` (2 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel, Gwendal Grignou,
Javier Martinez Canillas
From: Gwendal Grignou <gwendal@chromium.org>
Chromebooks can have more than one Embedded Controller so the
cros_ec device id has to be incremented for each EC registered.
Add code to handle multiple EC. First ec found is cros-ec0,
second cros-ec1 and so on.
Add a new structure to represent multiple EC as different char
devices (e.g: /dev/cros_ec, /dev/cros_pd). It connects to
cros_ec_device and allows sysfs inferface for cros_pd.
Also reduce number of allocated objects, make chromeos sysfs
class object a static and add refcounting to prevent object
deletion while command is in progress.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1:
- Squash patch that adds support to represent EC's as different
char devices (e.g: /dev/cros_ec, /dev/cros_pd):
https://chromium-review.googlesource.com/#/c/217297/
Suggested by Gwendal Grignou
- Use cros_ec instead of cros-ec in the subject line to be consistent.
Suggested by Gwendal Grignou
---
drivers/input/keyboard/cros_ec_keyb.c | 2 +-
drivers/mfd/cros_ec.c | 66 +++++++++++++--
drivers/mfd/cros_ec_i2c.c | 1 -
drivers/mfd/cros_ec_spi.c | 1 -
drivers/platform/chrome/cros_ec_dev.c | 128 ++++++++++++++++++++---------
drivers/platform/chrome/cros_ec_dev.h | 7 --
drivers/platform/chrome/cros_ec_lightbar.c | 75 +++++++++--------
drivers/platform/chrome/cros_ec_lpc.c | 1 -
drivers/platform/chrome/cros_ec_sysfs.c | 50 +++++------
include/linux/mfd/cros_ec.h | 44 ++++++++--
10 files changed, 248 insertions(+), 127 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 090f8a75412a..ef3ba20f4560 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -269,7 +269,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
ckdev->dev = dev;
dev_set_drvdata(&pdev->dev, ckdev);
- idev->name = ec->ec_name;
+ idev->name = CROS_EC_DEV_NAME;
idev->phys = ec->phys_name;
__set_bit(EV_REP, idev->evbit);
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 956a827de826..6acf350b3ce6 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -289,6 +289,8 @@ exit:
return ret;
}
+static int dev_id;
+
int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
{
@@ -379,12 +381,46 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
}
EXPORT_SYMBOL(cros_ec_cmd_xfer);
-static const struct mfd_cell cros_devs[] = {
- {
+static int cros_ec_dev_register(struct cros_ec_device *ec_dev,
+ int dev_id, int devidx)
+{
+ struct device *dev = ec_dev->dev;
+ struct cros_ec_platform ec_p = {
+ .cmd_offset = 0,
+ };
+
+ struct mfd_cell ec_cell = {
.name = "cros-ec-ctl",
.id = PLATFORM_DEVID_AUTO,
- },
-};
+ .platform_data = &ec_p,
+ .pdata_size = sizeof(ec_p),
+ };
+
+ switch (devidx) {
+ case 0:
+ if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+ ec_p.ec_name = of_get_property(dev->of_node, "devname",
+ NULL);
+ if (ec_p.ec_name == NULL) {
+ dev_dbg(dev,
+ "Device name not found, using default");
+ ec_p.ec_name = CROS_EC_DEV_NAME;
+ }
+ } else {
+ ec_p.ec_name = CROS_EC_DEV_NAME;
+ }
+ break;
+ case 1:
+ ec_p.ec_name = CROS_EC_DEV_PD_NAME;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ec_p.cmd_offset = EC_CMD_PASSTHRU_OFFSET(devidx);
+ return mfd_add_devices(dev, dev_id, &ec_cell, 1,
+ NULL, ec_dev->irq, NULL);
+}
int cros_ec_register(struct cros_ec_device *ec_dev)
{
@@ -407,14 +443,28 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
cros_ec_probe_all(ec_dev);
- err = mfd_add_devices(dev, 0, cros_devs,
- ARRAY_SIZE(cros_devs),
- NULL, ec_dev->irq, NULL);
+ err = cros_ec_dev_register(ec_dev, dev_id++, 0);
if (err) {
- dev_err(dev, "failed to add mfd devices\n");
+ dev_err(dev, "failed to add ec\n");
return err;
}
+ if (ec_dev->max_passthru) {
+ /*
+ * Register a PD device as well on top of this device.
+ * We make the following assumptions:
+ * - behind an EC, we have a pd
+ * - only one device added.
+ * - the EC is responsive at init time (it is not true for a
+ * sensor hub.
+ */
+ err = cros_ec_dev_register(ec_dev, dev_id++, 1);
+ if (err) {
+ dev_err(dev, "failed to add additional ec\n");
+ return err;
+ }
+ }
+
if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
err = of_platform_populate(dev->of_node, NULL, NULL, dev);
if (err) {
diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index 22e8a4ae1711..b9a0963ca5c3 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -302,7 +302,6 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
ec_dev->irq = client->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_i2c;
- ec_dev->ec_name = client->name;
ec_dev->phys_name = client->adapter->name;
ec_dev->din_size = sizeof(struct ec_host_response_i2c) +
sizeof(struct ec_response_get_protocol_info);
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 4e6f2f6b1095..faba03e2f1ef 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -637,7 +637,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
ec_dev->irq = spi->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_spi;
- ec_dev->ec_name = ec_spi->spi->modalias;
ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
sizeof(struct ec_host_response) +
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 7f7e3db59234..e52c636da404 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -29,11 +29,22 @@
#define CROS_MAX_DEV 128
/* Most EC commands size is ~64 bytes so allocate a buffer for the usual case */
#define PREALLOC_SIZE 64
-static struct class *cros_class;
static int ec_major;
+static const struct attribute_group *cros_ec_groups[] = {
+ &cros_ec_attr_group,
+ &cros_ec_lightbar_attr_group,
+ NULL,
+};
+
+static struct class cros_class = {
+ .owner = THIS_MODULE,
+ .name = "chromeos",
+ .dev_groups = cros_ec_groups,
+};
+
/* Basic communication */
-static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
+static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
{
struct ec_response_get_version *resp;
static const char * const current_image_name[] = {
@@ -49,7 +60,7 @@ static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
msg->insize = sizeof(*resp);
msg->outsize = 0;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
return ret;
@@ -74,8 +85,10 @@ static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
/* Device file ops */
static int ec_device_open(struct inode *inode, struct file *filp)
{
- filp->private_data = container_of(inode->i_cdev,
- struct cros_ec_device, cdev);
+ struct cros_ec_dev *ec = container_of(inode->i_cdev,
+ struct cros_ec_dev, cdev);
+ filp->private_data = ec;
+ nonseekable_open(inode, filp);
return 0;
}
@@ -87,7 +100,7 @@ static int ec_device_release(struct inode *inode, struct file *filp)
static ssize_t ec_device_read(struct file *filp, char __user *buffer,
size_t length, loff_t *offset)
{
- struct cros_ec_device *ec = filp->private_data;
+ struct cros_ec_dev *ec = filp->private_data;
char msg[sizeof(struct ec_response_get_version) +
sizeof(CROS_EC_DEV_VERSION)];
size_t count;
@@ -110,7 +123,7 @@ static ssize_t ec_device_read(struct file *filp, char __user *buffer,
}
/* Ioctls */
-static long ec_device_ioctl_xcmd(struct cros_ec_device *ec, void __user *arg)
+static long ec_device_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
{
long ret;
int alloc_size;
@@ -137,7 +150,8 @@ static long ec_device_ioctl_xcmd(struct cros_ec_device *ec, void __user *arg)
goto exit;
}
- ret = cros_ec_cmd_xfer(ec, s_cmd);
+ s_cmd->command += ec->cmd_offset;
+ ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
/* Only copy data to userland if data was received. */
if (ret < 0)
goto exit;
@@ -150,19 +164,21 @@ exit:
return ret;
}
-static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
+static long ec_device_ioctl_readmem(struct cros_ec_dev *ec, void __user *arg)
{
+ struct cros_ec_device *ec_dev = ec->ec_dev;
struct cros_ec_readmem s_mem = { };
long num;
/* Not every platform supports direct reads */
- if (!ec->cmd_readmem)
+ if (!ec_dev->cmd_readmem)
return -ENOTTY;
if (copy_from_user(&s_mem, arg, sizeof(s_mem)))
return -EFAULT;
- num = ec->cmd_readmem(ec, s_mem.offset, s_mem.bytes, s_mem.buffer);
+ num = ec_dev->cmd_readmem(ec_dev, s_mem.offset, s_mem.bytes,
+ s_mem.buffer);
if (num <= 0)
return num;
@@ -175,7 +191,7 @@ static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
static long ec_device_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
- struct cros_ec_device *ec = filp->private_data;
+ struct cros_ec_dev *ec = filp->private_data;
if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
return -ENOTTY;
@@ -198,45 +214,81 @@ static const struct file_operations fops = {
.unlocked_ioctl = ec_device_ioctl,
};
+static void __remove(struct device *dev)
+{
+ struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+ class_dev);
+ kfree(ec);
+}
+
static int ec_device_probe(struct platform_device *pdev)
{
- struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
- int retval = -ENOTTY;
- dev_t devno = MKDEV(ec_major, 0);
+ int retval = -ENOMEM;
+ struct device *dev = &pdev->dev;
+ struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
+ dev_t devno = MKDEV(ec_major, pdev->id);
+ struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
+
+ if (!ec)
+ return retval;
- /* Instantiate it (and remember the EC) */
+ dev_set_drvdata(dev, ec);
+ ec->ec_dev = dev_get_drvdata(dev->parent);
+ ec->dev = dev;
+ ec->cmd_offset = ec_platform->cmd_offset;
+ device_initialize(&ec->class_dev);
cdev_init(&ec->cdev, &fops);
+ /*
+ * Add the character device
+ * Link cdev to the class device to be sure device is not used
+ * before unbinding it.
+ */
+ ec->cdev.kobj.parent = &ec->class_dev.kobj;
retval = cdev_add(&ec->cdev, devno, 1);
if (retval) {
- dev_err(&pdev->dev, ": failed to add character device\n");
- return retval;
+ dev_err(dev, ": failed to add character device\n");
+ goto cdev_add_failed;
}
- ec->vdev = device_create(cros_class, NULL, devno, ec,
- CROS_EC_DEV_NAME);
- if (IS_ERR(ec->vdev)) {
- retval = PTR_ERR(ec->vdev);
- dev_err(&pdev->dev, ": failed to create device\n");
- cdev_del(&ec->cdev);
- return retval;
+ /*
+ * Add the class device
+ * Link to the character device for creating the /dev entry
+ * in devtmpfs.
+ */
+ ec->class_dev.devt = ec->cdev.dev;
+ ec->class_dev.class = &cros_class;
+ ec->class_dev.parent = dev;
+ ec->class_dev.release = __remove;
+
+ retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name);
+ if (retval) {
+ dev_err(dev, "dev_set_name failed => %d\n", retval);
+ goto set_named_failed;
}
- /* Initialize extra interfaces */
- ec_dev_sysfs_init(ec);
- ec_dev_lightbar_init(ec);
+ retval = device_add(&ec->class_dev);
+ if (retval) {
+ dev_err(dev, "device_register failed => %d\n", retval);
+ goto dev_reg_failed;
+ }
return 0;
+
+dev_reg_failed:
+set_named_failed:
+ dev_set_drvdata(dev, NULL);
+ cdev_del(&ec->cdev);
+cdev_add_failed:
+ kfree(ec);
+ return retval;
}
static int ec_device_remove(struct platform_device *pdev)
{
- struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
-
- ec_dev_lightbar_remove(ec);
- ec_dev_sysfs_remove(ec);
- device_destroy(cros_class, MKDEV(ec_major, 0));
+ struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
cdev_del(&ec->cdev);
+ device_unregister(&ec->class_dev);
return 0;
}
@@ -253,10 +305,10 @@ static int __init cros_ec_dev_init(void)
int ret;
dev_t dev = 0;
- cros_class = class_create(THIS_MODULE, "chromeos");
- if (IS_ERR(cros_class)) {
+ ret = class_register(&cros_class);
+ if (ret) {
pr_err(CROS_EC_DEV_NAME ": failed to register device class\n");
- return PTR_ERR(cros_class);
+ return ret;
}
/* Get a range of minor numbers (starting with 0) to work with */
@@ -278,7 +330,7 @@ static int __init cros_ec_dev_init(void)
failed_devreg:
unregister_chrdev_region(MKDEV(ec_major, 0), CROS_MAX_DEV);
failed_chrdevreg:
- class_destroy(cros_class);
+ class_unregister(&cros_class);
return ret;
}
@@ -286,7 +338,7 @@ static void __exit cros_ec_dev_exit(void)
{
platform_driver_unregister(&cros_ec_dev_driver);
unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
- class_destroy(cros_class);
+ class_unregister(&cros_class);
}
module_init(cros_ec_dev_init);
diff --git a/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
index 45d67f7e518c..bfd2c84c3571 100644
--- a/drivers/platform/chrome/cros_ec_dev.h
+++ b/drivers/platform/chrome/cros_ec_dev.h
@@ -24,7 +24,6 @@
#include <linux/types.h>
#include <linux/mfd/cros_ec.h>
-#define CROS_EC_DEV_NAME "cros_ec"
#define CROS_EC_DEV_VERSION "1.0.0"
/*
@@ -44,10 +43,4 @@ struct cros_ec_readmem {
#define CROS_EC_DEV_IOCXCMD _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
#define CROS_EC_DEV_IOCRDMEM _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
-void ec_dev_sysfs_init(struct cros_ec_device *);
-void ec_dev_sysfs_remove(struct cros_ec_device *);
-
-void ec_dev_lightbar_init(struct cros_ec_device *);
-void ec_dev_lightbar_remove(struct cros_ec_device *);
-
#endif /* _CROS_EC_DEV_H_ */
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 6e1986a2dce1..144e09df9b84 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -92,7 +92,7 @@ out:
return ret;
}
-static struct cros_ec_command *alloc_lightbar_cmd_msg(void)
+static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev *ec)
{
struct cros_ec_command *msg;
int len;
@@ -105,14 +105,14 @@ static struct cros_ec_command *alloc_lightbar_cmd_msg(void)
return NULL;
msg->version = 0;
- msg->command = EC_CMD_LIGHTBAR_CMD;
+ msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
msg->outsize = sizeof(struct ec_params_lightbar);
msg->insize = sizeof(struct ec_response_lightbar);
return msg;
}
-static int get_lightbar_version(struct cros_ec_device *ec,
+static int get_lightbar_version(struct cros_ec_dev *ec,
uint32_t *ver_ptr, uint32_t *flg_ptr)
{
struct ec_params_lightbar *param;
@@ -120,13 +120,13 @@ static int get_lightbar_version(struct cros_ec_device *ec,
struct cros_ec_command *msg;
int ret;
- msg = alloc_lightbar_cmd_msg();
+ msg = alloc_lightbar_cmd_msg(ec);
if (!msg)
return 0;
param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_VERSION;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0) {
ret = 0;
goto exit;
@@ -165,7 +165,8 @@ static ssize_t version_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
uint32_t version = 0, flags = 0;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
int ret;
ret = lb_throttle();
@@ -187,12 +188,13 @@ static ssize_t brightness_store(struct device *dev,
struct cros_ec_command *msg;
int ret;
unsigned int val;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
if (kstrtouint(buf, 0, &val))
return -EINVAL;
- msg = alloc_lightbar_cmd_msg();
+ msg = alloc_lightbar_cmd_msg(ec);
if (!msg)
return -ENOMEM;
@@ -203,7 +205,7 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
goto exit;
@@ -231,11 +233,12 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
{
struct ec_params_lightbar *param;
struct cros_ec_command *msg;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
unsigned int val[4];
int ret, i = 0, j = 0, ok = 0;
- msg = alloc_lightbar_cmd_msg();
+ msg = alloc_lightbar_cmd_msg(ec);
if (!msg)
return -ENOMEM;
@@ -268,7 +271,7 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
return ret;
}
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
goto exit;
@@ -304,9 +307,10 @@ static ssize_t sequence_show(struct device *dev,
struct ec_response_lightbar *resp;
struct cros_ec_command *msg;
int ret;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
- msg = alloc_lightbar_cmd_msg();
+ msg = alloc_lightbar_cmd_msg(ec);
if (!msg)
return -ENOMEM;
@@ -316,7 +320,7 @@ static ssize_t sequence_show(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
goto exit;
@@ -345,9 +349,10 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
struct cros_ec_command *msg;
unsigned int num;
int ret, len;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
- msg = alloc_lightbar_cmd_msg();
+ msg = alloc_lightbar_cmd_msg(ec);
if (!msg)
return -ENOMEM;
@@ -372,7 +377,7 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
if (ret)
return ret;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
return ret;
@@ -397,25 +402,27 @@ static struct attribute *__lb_cmds_attrs[] = {
&dev_attr_sequence.attr,
NULL,
};
-static struct attribute_group lb_cmds_attr_group = {
- .name = "lightbar",
- .attrs = __lb_cmds_attrs,
-};
-void ec_dev_lightbar_init(struct cros_ec_device *ec)
+static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
+ struct attribute *a, int n)
{
- int ret = 0;
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
+ struct platform_device *pdev = container_of(ec->dev,
+ struct platform_device, dev);
+ if (pdev->id != 0)
+ return 0;
/* Only instantiate this stuff if the EC has a lightbar */
- if (!get_lightbar_version(ec, NULL, NULL))
- return;
-
- ret = sysfs_create_group(&ec->vdev->kobj, &lb_cmds_attr_group);
- if (ret)
- pr_warn("sysfs_create_group() failed: %d\n", ret);
+ if (get_lightbar_version(ec, NULL, NULL))
+ return a->mode;
+ else
+ return 0;
}
-void ec_dev_lightbar_remove(struct cros_ec_device *ec)
-{
- sysfs_remove_group(&ec->vdev->kobj, &lb_cmds_attr_group);
-}
+struct attribute_group cros_ec_lightbar_attr_group = {
+ .name = "lightbar",
+ .attrs = __lb_cmds_attrs,
+ .is_visible = cros_ec_lightbar_attrs_are_visible,
+};
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index e98b0345288a..ca7cdc6eef0b 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -273,7 +273,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ec_dev);
ec_dev->dev = dev;
- ec_dev->ec_name = pdev->name;
ec_dev->phys_name = dev_name(dev);
ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_lpc;
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 88d0a7c9bbff..056855c32187 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -73,7 +73,8 @@ static ssize_t store_ec_reboot(struct device *dev,
int got_cmd = 0, offset = 0;
int i;
int ret;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
msg = (struct cros_ec_command *)msg_buf;
param = (struct ec_params_reboot_ec *)msg->data;
@@ -107,13 +108,13 @@ static ssize_t store_ec_reboot(struct device *dev,
if (!got_cmd)
return -EINVAL;
- msg->command = EC_CMD_REBOOT_EC;
+ msg->command = EC_CMD_REBOOT_EC + ec->cmd_offset;
msg->outsize = sizeof(*param);
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
return ret;
if (msg->result != EC_RES_SUCCESS) {
- dev_dbg(ec->dev, "EC result %d\n", msg->result);
+ dev_dbg(&ec->class_dev, "EC result %d\n", msg->result);
return -EINVAL;
}
@@ -131,14 +132,15 @@ static ssize_t show_ec_version(struct device *dev,
u8 msg_buf[sizeof(*msg) + EC_HOST_PARAM_SIZE] = { 0 };
int ret;
int count = 0;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
msg = (struct cros_ec_command *)msg_buf;
/* Get versions. RW may change. */
- msg->command = EC_CMD_GET_VERSION;
+ msg->command = EC_CMD_GET_VERSION + ec->cmd_offset;
msg->insize = sizeof(*r_ver);
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
return ret;
if (msg->result != EC_RES_SUCCESS)
@@ -159,9 +161,9 @@ static ssize_t show_ec_version(struct device *dev,
image_names[r_ver->current_image] : "?"));
/* Get build info. */
- msg->command = EC_CMD_GET_BUILD_INFO;
+ msg->command = EC_CMD_GET_BUILD_INFO + ec->cmd_offset;
msg->insize = EC_HOST_PARAM_SIZE;
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: XFER ERROR %d\n", ret);
@@ -175,9 +177,9 @@ static ssize_t show_ec_version(struct device *dev,
}
/* Get chip info. */
- msg->command = EC_CMD_GET_CHIP_INFO;
+ msg->command = EC_CMD_GET_CHIP_INFO + ec->cmd_offset;
msg->insize = sizeof(*r_chip);
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Chip info: XFER ERROR %d\n", ret);
@@ -199,9 +201,9 @@ static ssize_t show_ec_version(struct device *dev,
}
/* Get board version */
- msg->command = EC_CMD_GET_BOARD_VERSION;
+ msg->command = EC_CMD_GET_BOARD_VERSION + ec->cmd_offset;
msg->insize = sizeof(*r_board);
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
count += scnprintf(buf + count, PAGE_SIZE - count,
"Board version: XFER ERROR %d\n", ret);
@@ -225,15 +227,16 @@ static ssize_t show_ec_flashinfo(struct device *dev,
struct ec_response_flash_info *resp;
struct cros_ec_command *msg;
int ret;
- struct cros_ec_device *ec = dev_get_drvdata(dev);
+ struct cros_ec_dev *ec = container_of(dev,
+ struct cros_ec_dev, class_dev);
u8 msg_buf[sizeof(*msg) + sizeof(*resp)] = { 0 };
msg = (struct cros_ec_command *)msg_buf;
/* The flash info shouldn't ever change, but ask each time anyway. */
- msg->command = EC_CMD_FLASH_INFO;
+ msg->command = EC_CMD_FLASH_INFO + ec->cmd_offset;
msg->insize = sizeof(*resp);
- ret = cros_ec_cmd_xfer(ec, msg);
+ ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
if (ret < 0)
return ret;
if (msg->result != EC_RES_SUCCESS)
@@ -262,20 +265,7 @@ static struct attribute *__ec_attrs[] = {
NULL,
};
-static struct attribute_group ec_attr_group = {
+struct attribute_group cros_ec_attr_group = {
.attrs = __ec_attrs,
};
-void ec_dev_sysfs_init(struct cros_ec_device *ec)
-{
- int error;
-
- error = sysfs_create_group(&ec->vdev->kobj, &ec_attr_group);
- if (error)
- pr_warn("failed to create group: %d\n", error);
-}
-
-void ec_dev_sysfs_remove(struct cros_ec_device *ec)
-{
- sysfs_remove_group(&ec->vdev->kobj, &ec_attr_group);
-}
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 15dc8317a767..fd5b92b60d62 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -17,10 +17,14 @@
#define __LINUX_MFD_CROS_EC_H
#include <linux/cdev.h>
+#include <linux/device.h>
#include <linux/notifier.h>
#include <linux/mfd/cros_ec_commands.h>
#include <linux/mutex.h>
+#define CROS_EC_DEV_NAME "cros_ec"
+#define CROS_EC_DEV_PD_NAME "cros_pd"
+
/*
* The EC is unresponsive for a time after a reboot command. Add a
* simple delay to make sure that the bus stays locked.
@@ -71,11 +75,8 @@ struct cros_ec_command {
/**
* struct cros_ec_device - Information about a ChromeOS EC device
*
- * @ec_name: name of EC device (e.g. 'chromeos-ec')
* @phys_name: name of physical comms layer (e.g. 'i2c-4')
* @dev: Device pointer for physical comms device
- * @vdev: Device pointer for virtual comms device
- * @cdev: Character device structure for virtual comms device
* @was_wake_device: true if this device was set to wake the system from
* sleep at the last suspend
* @cmd_readmem: direct read of the EC memory-mapped region, if supported
@@ -87,6 +88,7 @@ struct cros_ec_command {
*
* @priv: Private data
* @irq: Interrupt to use
+ * @id: Device id
* @din: input buffer (for data from EC)
* @dout: output buffer (for data to EC)
* \note
@@ -109,11 +111,8 @@ struct cros_ec_command {
struct cros_ec_device {
/* These are used by other drivers that want to talk to the EC */
- const char *ec_name;
const char *phys_name;
struct device *dev;
- struct device *vdev;
- struct cdev cdev;
bool was_wake_device;
struct class *cros_class;
int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
@@ -138,6 +137,35 @@ struct cros_ec_device {
struct mutex lock;
};
+/* struct cros_ec_platform - ChromeOS EC platform information
+ *
+ * @ec_name: name of EC device (e.g. 'cros-ec', 'cros-pd', ...)
+ * used in /dev/ and sysfs.
+ * @cmd_offset: offset to apply for each command. Set when
+ * registering a devicde behind another one.
+ */
+struct cros_ec_platform {
+ const char *ec_name;
+ u16 cmd_offset;
+};
+
+/*
+ * struct cros_ec_dev - ChromeOS EC device entry point
+ *
+ * @class_dev: Device structure used in sysfs
+ * @cdev: Character device structure in /dev
+ * @ec_dev: cros_ec_device structure to talk to the physical device
+ * @dev: pointer to the platform device
+ * @cmd_offset: offset to apply for each command.
+ */
+struct cros_ec_dev {
+ struct device class_dev;
+ struct cdev cdev;
+ struct cros_ec_device *ec_dev;
+ struct device *dev;
+ u16 cmd_offset;
+};
+
/**
* cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
*
@@ -216,4 +244,8 @@ int cros_ec_remove(struct cros_ec_device *ec_dev);
*/
int cros_ec_register(struct cros_ec_device *ec_dev);
+/* sysfs stuff */
+extern struct attribute_group cros_ec_attr_group;
+extern struct attribute_group cros_ec_lightbar_attr_group;
+
#endif /* __LINUX_MFD_CROS_EC_H */
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 09/10] platform/chrome: cros_ec_lpc - Add support for Google Pixel 2
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (7 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 08/10] mfd: cros_ec: Support multiple EC in a system Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-09 10:10 ` [PATCH v2 10/10] mfd: cros_ec: spi: Add delay for asserting CS Javier Martinez Canillas
2015-05-11 17:53 ` [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Heiko Stuebner
10 siblings, 0 replies; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel,
Javier Martinez Canillas
Since the verion of ACPI in Google BIOS does not enumarate the devices
in the LPC bus, the cros_ec_lpc driver resorts to DMI data to check if
a system is supported by the driver and autoload if built as a module.
Add information about the Google Pixel 2 to the DMI device table.
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1: None, new patch
---
drivers/platform/chrome/cros_ec_lpc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index ca7cdc6eef0b..f9a245465fd0 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -320,6 +320,13 @@ static struct dmi_system_id cros_ec_lpc_dmi_table[] __initdata = {
},
},
{
+ /* x86-samus, the Chromebook Pixel 2. */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Samus"),
+ },
+ },
+ {
/* x86-peppy, the Acer C720 Chromebook. */
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 10/10] mfd: cros_ec: spi: Add delay for asserting CS
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (8 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 09/10] platform/chrome: cros_ec_lpc - Add support for Google Pixel 2 Javier Martinez Canillas
@ 2015-05-09 10:10 ` Javier Martinez Canillas
2015-05-13 11:16 ` Lee Jones
2015-05-11 17:53 ` [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Heiko Stuebner
10 siblings, 1 reply; 29+ messages in thread
From: Javier Martinez Canillas @ 2015-05-09 10:10 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel, Chris Zhong,
Javier Martinez Canillas
From: Alexandru M Stan <amstan@chromium.org>
Some ECs need a little time for waking up before they can accept
SPI data at a high speed. This is configurable via a DT property
"google,cros-ec-spi-pre-delay".
If this property isn't set, then no delay will be added. However,
if set it will cause a delay equal to the value passed to it to
be inserted at the beginning of a transaction.
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Chris Zhong <zyw@rock-chips.com>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1: None, new patch
---
Documentation/devicetree/bindings/mfd/cros-ec.txt | 4 ++++
drivers/mfd/cros_ec_spi.c | 21 +++++++++++++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt
index 8009c3d87f33..1777916e9e28 100644
--- a/Documentation/devicetree/bindings/mfd/cros-ec.txt
+++ b/Documentation/devicetree/bindings/mfd/cros-ec.txt
@@ -18,6 +18,10 @@ Required properties (SPI):
- reg: SPI chip select
Optional properties (SPI):
+- google,cros-ec-spi-pre-delay: Some implementations of the EC need a little
+ time to wake up from sleep before they can receive SPI transfers at a high
+ clock rate. This property specifies the delay, in usecs, between the
+ assertion of the CS to the start of the first clock pulse.
- google,cros-ec-spi-msg-delay: Some implementations of the EC require some
additional processing time in order to accept new transactions. If the delay
between transactions is not long enough the EC may not be able to respond
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index faba03e2f1ef..16f228dc243f 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -71,12 +71,15 @@
* @spi: SPI device we are connected to
* @last_transfer_ns: time that we last finished a transfer, or 0 if there
* if no record
+ * @start_of_msg_delay: used to set the delay_usecs on the spi_transfer that
+ * is sent when we want to turn on CS at the start of a transaction.
* @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
* is sent when we want to turn off CS at the end of a transaction.
*/
struct cros_ec_spi {
struct spi_device *spi;
s64 last_transfer_ns;
+ unsigned int start_of_msg_delay;
unsigned int end_of_msg_delay;
};
@@ -366,7 +369,7 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
struct ec_host_request *request;
struct ec_host_response *response;
struct cros_ec_spi *ec_spi = ec_dev->priv;
- struct spi_transfer trans;
+ struct spi_transfer trans, trans_delay;
struct spi_message msg;
int i, len;
u8 *ptr;
@@ -393,13 +396,23 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
goto exit;
}
+ /*
+ * Leave a gap between CS assertion and clocking of data to allow the
+ * EC time to wakeup.
+ */
+ spi_message_init(&msg);
+ if (ec_spi->start_of_msg_delay) {
+ memset(&trans_delay, 0, sizeof(trans_delay));
+ trans_delay.delay_usecs = ec_spi->start_of_msg_delay;
+ spi_message_add_tail(&trans_delay, &msg);
+ }
+
/* Transmit phase - send our message */
memset(&trans, 0, sizeof(trans));
trans.tx_buf = ec_dev->dout;
trans.rx_buf = rx_buf;
trans.len = len;
trans.cs_change = 1;
- spi_message_init(&msg);
spi_message_add_tail(&trans, &msg);
ret = spi_sync(ec_spi->spi, &msg);
@@ -602,6 +615,10 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
u32 val;
int ret;
+ ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
+ if (!ret)
+ ec_spi->start_of_msg_delay = val;
+
ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
if (!ret)
ec_spi->end_of_msg_delay = val;
--
2.1.4
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 10/10] mfd: cros_ec: spi: Add delay for asserting CS
2015-05-09 10:10 ` [PATCH v2 10/10] mfd: cros_ec: spi: Add delay for asserting CS Javier Martinez Canillas
@ 2015-05-13 11:16 ` Lee Jones
0 siblings, 0 replies; 29+ messages in thread
From: Lee Jones @ 2015-05-13 11:16 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Samuel Ortiz, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
Heiko Stuebner, linux-samsung-soc, linux-kernel, Chris Zhong
On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> From: Alexandru M Stan <amstan@chromium.org>
>
> Some ECs need a little time for waking up before they can accept
> SPI data at a high speed. This is configurable via a DT property
> "google,cros-ec-spi-pre-delay".
>
> If this property isn't set, then no delay will be added. However,
> if set it will cause a delay equal to the value passed to it to
> be inserted at the beginning of a transaction.
>
> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>
> Changes since v1: None, new patch
> ---
> Documentation/devicetree/bindings/mfd/cros-ec.txt | 4 ++++
> drivers/mfd/cros_ec_spi.c | 21 +++++++++++++++++++--
> 2 files changed, 23 insertions(+), 2 deletions(-)
Acked-by: Lee Jones <lee.jones@linaro.org>
> diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt
> index 8009c3d87f33..1777916e9e28 100644
> --- a/Documentation/devicetree/bindings/mfd/cros-ec.txt
> +++ b/Documentation/devicetree/bindings/mfd/cros-ec.txt
> @@ -18,6 +18,10 @@ Required properties (SPI):
> - reg: SPI chip select
>
> Optional properties (SPI):
> +- google,cros-ec-spi-pre-delay: Some implementations of the EC need a little
> + time to wake up from sleep before they can receive SPI transfers at a high
> + clock rate. This property specifies the delay, in usecs, between the
> + assertion of the CS to the start of the first clock pulse.
> - google,cros-ec-spi-msg-delay: Some implementations of the EC require some
> additional processing time in order to accept new transactions. If the delay
> between transactions is not long enough the EC may not be able to respond
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index faba03e2f1ef..16f228dc243f 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -71,12 +71,15 @@
> * @spi: SPI device we are connected to
> * @last_transfer_ns: time that we last finished a transfer, or 0 if there
> * if no record
> + * @start_of_msg_delay: used to set the delay_usecs on the spi_transfer that
> + * is sent when we want to turn on CS at the start of a transaction.
> * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
> * is sent when we want to turn off CS at the end of a transaction.
> */
> struct cros_ec_spi {
> struct spi_device *spi;
> s64 last_transfer_ns;
> + unsigned int start_of_msg_delay;
> unsigned int end_of_msg_delay;
> };
>
> @@ -366,7 +369,7 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
> struct ec_host_request *request;
> struct ec_host_response *response;
> struct cros_ec_spi *ec_spi = ec_dev->priv;
> - struct spi_transfer trans;
> + struct spi_transfer trans, trans_delay;
> struct spi_message msg;
> int i, len;
> u8 *ptr;
> @@ -393,13 +396,23 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device *ec_dev,
> goto exit;
> }
>
> + /*
> + * Leave a gap between CS assertion and clocking of data to allow the
> + * EC time to wakeup.
> + */
> + spi_message_init(&msg);
> + if (ec_spi->start_of_msg_delay) {
> + memset(&trans_delay, 0, sizeof(trans_delay));
> + trans_delay.delay_usecs = ec_spi->start_of_msg_delay;
> + spi_message_add_tail(&trans_delay, &msg);
> + }
> +
> /* Transmit phase - send our message */
> memset(&trans, 0, sizeof(trans));
> trans.tx_buf = ec_dev->dout;
> trans.rx_buf = rx_buf;
> trans.len = len;
> trans.cs_change = 1;
> - spi_message_init(&msg);
> spi_message_add_tail(&trans, &msg);
> ret = spi_sync(ec_spi->spi, &msg);
>
> @@ -602,6 +615,10 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> u32 val;
> int ret;
>
> + ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
> + if (!ret)
> + ec_spi->start_of_msg_delay = val;
> +
> ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> if (!ret)
> ec_spi->end_of_msg_delay = val;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support
2015-05-09 10:10 [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Javier Martinez Canillas
` (9 preceding siblings ...)
2015-05-09 10:10 ` [PATCH v2 10/10] mfd: cros_ec: spi: Add delay for asserting CS Javier Martinez Canillas
@ 2015-05-11 17:53 ` Heiko Stuebner
2015-05-11 20:08 ` Alexandru Stan
10 siblings, 1 reply; 29+ messages in thread
From: Heiko Stuebner @ 2015-05-11 17:53 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Lee Jones, Samuel Ortiz, Olof Johansson, Doug Anderson,
Bill Richardson, Simon Glass, Gwendal Grignou, Stephen Barber,
Filipe Brandenburger, Todd Broch, Alexandru M Stan,
linux-samsung-soc, linux-kernel
Hi Javier,
Am Samstag, 9. Mai 2015, 12:10:31 schrieb Javier Martinez Canillas:
> Newer Chromebooks have more than one Embedded Controller (EC) in the
> system. These additional ECs are connected through I2C with a host EC
> which is the one that is connected to the Application Processor (AP)
> through different transports (I2C, SPI or LPC).
>
> So on these platforms, sub-processors are chained to each other:
>
> AP <--> Host EC <--> Power Delivery (PD) EC
>
> The AP sends commands to the additional EC through the host EC using
> a set of passthru commands and the host redirects to the correct EC.
>
> This is a v2 of a series that adds support for multiple EC in a system
> and also for the protocol version 3 that is used on newer ECs.
>
> Most patches were taken from the downstream ChromiumOS v3.14 tree with
> fixes squashed, split to minimise the cross subsystem churn and changes
> for mainline inclusion but were not modified functionality wise.
>
> This version addresses a lot of issues pointed out by Gwendal Grignou
> and Heiko Stuebner on the v1 posted before [0].
>
> The patches were tested on a x86 Pixel Chromebook 2 (Samus) that uses the
> new protocol v3 and has 2 EC (cros_ec and cros_pd) using a modified ectool
> [1] that supports the new cros_ec IOCTL API.
>
> Also, testing was done on some Chromebooks that have a single EC and support
> protocol v2 such as the Exynos5250 Snow, Exynos5420 Peach Pit and
> Exynos5800 Peach Pi to be sure that no regressions were introduced for
> these machines.
>
> Gwendal Grignou and Heiko Stuebner tested the first version of the series
> but I did not carry their Tested-by tags for the patches that had not
> trivial changes. So testing on more platforms will be highly appreciated.
hmm, while it largely works (generally, as well as the touchpad) the keyboard
seems to have some problem on rk3288-pinky, it didn't have before with v1 +
the two patches on top that are now folded into this v2.
Doing an evtest /dev/input/event0 does not result in key events, now after
some keypresses I got a
[ 48.352023] cros-ec-spi spi0.0: EC failed to respond in time
[ 48.357695] cros-ec-spi spi0.0: Error transferring EC message -110
[ 48.363884] cros-ec-spi spi0.0: failed to get keyboard state: -110
[ 226.478588] cros-ec-spi spi0.0: Error transferring EC message -11
[ 226.484690] cros-ec-spi spi0.0: failed to get keyboard state: -11
a (regular) reboot seems to result in a working keyboard, while a
poweroff+poweron via dut-control brings me again back to a non-working
keyboard.
in my very first try it started similarly non-working but after some time
started magically working again, but now in subsequent tries I always get the
spi error + working keyboard after a reboot.
Strangely this really seems to be limited to the keyboard, touchpad works
flawlessly it seems.
Heiko
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support
2015-05-11 17:53 ` [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support Heiko Stuebner
@ 2015-05-11 20:08 ` Alexandru Stan
2015-05-11 20:51 ` Heiko Stuebner
0 siblings, 1 reply; 29+ messages in thread
From: Alexandru Stan @ 2015-05-11 20:08 UTC (permalink / raw)
To: Heiko Stuebner
Cc: Javier Martinez Canillas, Lee Jones, Samuel Ortiz, Olof Johansson,
Doug Anderson, Bill Richardson, Simon Glass, Gwendal Grignou,
Stephen Barber, Filipe Brandenburger, Todd Broch,
linux-samsung-soc, linux-kernel@vger.kernel.org
On Mon, May 11, 2015 at 10:53 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Doing an evtest /dev/input/event0 does not result in key events, now after
> some keypresses I got a
> [ 48.352023] cros-ec-spi spi0.0: EC failed to respond in time
> [ 48.357695] cros-ec-spi spi0.0: Error transferring EC message -110
> [ 48.363884] cros-ec-spi spi0.0: failed to get keyboard state: -110
> [ 226.478588] cros-ec-spi spi0.0: Error transferring EC message -11
> [ 226.484690] cros-ec-spi spi0.0: failed to get keyboard state: -11 well as the touchpad) the keyboard
At first I tought it was because your EC firmware version is pretty old.
A while ago we fixed a pretty nasty scheduler bug that would have
caused similar symptoms:
https://chromium-review.googlesource.com/#/c/248681/
I did my own testing with the trees you provided (v1 and v2) on
veyron-minnie and
I can definatelly confirm that v2 is broken.
Here's a snippet of what a key press generates on the EC log:
> [950.749097 KB poll]
[950.760357 HC 0x60]
[950.760555 HC err 6]
+[950.903657 HC 0x60]
[950.905243 HC err 6]
+[950.926239 KB wait]
Same key press on v1:
> [37.107390 KB poll]
[37.118457 HC 0x60]
[37.209218 HC 0x60]
[37.278431 KB wait]
In the EC code, error 6 is defined as EC_RES_INVALID_VERSION.
> in my very first try it started similarly non-working but after some time
> started magically working again, but now in subsequent tries I always get the
> spi error + working keyboard after a reboot.
At first the keyboard doesn't work for me either (20 or so seconds).
Then all keys show up and everything's fine (besides the errors on the EC log).
> Strangely this really seems to be limited to the keyboard, touchpad works
> flawlessly it seems.
Touchpad doesn't go through the EC, it's just a standard i2c device
directly connected to the AP.
Alexandru Stan (amstan)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 00/10] mfd: cros_ec: Add multi EC and proto v3 support
2015-05-11 20:08 ` Alexandru Stan
@ 2015-05-11 20:51 ` Heiko Stuebner
0 siblings, 0 replies; 29+ messages in thread
From: Heiko Stuebner @ 2015-05-11 20:51 UTC (permalink / raw)
To: Alexandru Stan
Cc: Javier Martinez Canillas, Lee Jones, Samuel Ortiz, Olof Johansson,
Doug Anderson, Bill Richardson, Simon Glass, Gwendal Grignou,
Stephen Barber, Filipe Brandenburger, Todd Broch,
linux-samsung-soc, linux-kernel@vger.kernel.org
Am Montag, 11. Mai 2015, 13:08:27 schrieb Alexandru Stan:
> On Mon, May 11, 2015 at 10:53 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Doing an evtest /dev/input/event0 does not result in key events, now after
> > some keypresses I got a
> > [ 48.352023] cros-ec-spi spi0.0: EC failed to respond in time
> > [ 48.357695] cros-ec-spi spi0.0: Error transferring EC message -110
> > [ 48.363884] cros-ec-spi spi0.0: failed to get keyboard state: -110
> > [ 226.478588] cros-ec-spi spi0.0: Error transferring EC message -11
> > [ 226.484690] cros-ec-spi spi0.0: failed to get keyboard state: -11 well
> > as the touchpad) the keyboard
> At first I tought it was because your EC firmware version is pretty old.
> A while ago we fixed a pretty nasty scheduler bug that would have
> caused similar symptoms:
> https://chromium-review.googlesource.com/#/c/248681/
>
> I did my own testing with the trees you provided (v1 and v2) on
> veyron-minnie and
> I can definatelly confirm that v2 is broken.
for completenes sake, branches in use where:
clean apply of v2 on top of 4.1-rc3 + clk-next
(already emits the error when using evtest):
https://github.com/mmind/linux-rockchip/commits/tmp/crosec_v3_2
merged into working branch that produces something usable for veyron devices:
https://github.com/mmind/linux-rockchip/commits/devel/somewhat-stable
same branch rolled back to patch-series v1
(working keyboard on both pinky and minnie):
https://github.com/mmind/linux-rockchip/commits/tmp/cros_ec_to_v1
I'm at least somewhat sure, that there shouldn't be any big merge errors in
there :-)
>
> Here's a snippet of what a key press generates on the EC log:
> > [950.749097 KB poll]
>
> [950.760357 HC 0x60]
> [950.760555 HC err 6]
> +[950.903657 HC 0x60]
> [950.905243 HC err 6]
> +[950.926239 KB wait]
>
> Same key press on v1:
> > [37.107390 KB poll]
>
> [37.118457 HC 0x60]
> [37.209218 HC 0x60]
> [37.278431 KB wait]
>
> In the EC code, error 6 is defined as EC_RES_INVALID_VERSION.
>
> > in my very first try it started similarly non-working but after some time
> > started magically working again, but now in subsequent tries I always get
> > the spi error + working keyboard after a reboot.
>
> At first the keyboard doesn't work for me either (20 or so seconds).
> Then all keys show up and everything's fine (besides the errors on the EC
> log).
> > Strangely this really seems to be limited to the keyboard, touchpad works
> > flawlessly it seems.
>
> Touchpad doesn't go through the EC, it's just a standard i2c device
> directly connected to the AP.
I should've looked at the dts before then to not overlook this ;-)
Heiko
^ permalink raw reply [flat|nested] 29+ messages in thread