linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
@ 2018-10-10 17:22 Lubomir Rintel
  2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
                   ` (17 more replies)
  0 siblings, 18 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

Hi.

This patchset adds support for the Embedded Controller on an OLPC XO
1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
the existing OLPC platform infrastructure, currently used by the x86
based models.

The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
uses extra handshake signal to signal readiness of SOC to accept data
from EC and to initiate a transaction if SOC wishes to submit data.

The SPI slave support for MMP2 was submitted separately:
https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t

THe "power: supply: olpc_battery: correct the temperature" patch was
already sent out separately, but I'm including it because the last
commit of the set depends on it.

Tested to work on an OLPC XO 1.75 and also tested not to break x86
support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
unlikely this breaks it when XO 1 works.

Thanks in advance for reviews and feedback of any kind.

Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH 01/15] power: supply: olpc_battery: correct the temperature units
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:00   ` Andy Shevchenko
  2018-11-02 22:16   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
                   ` (16 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel, stable

According to [1] and [2], the temperature values are in tenths of degree
Celsius. Exposing the Celsius value makes the battery appear on fire:

  $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
  ...
      temperature:         236.9 degrees C

Tested on OLPC XO-1 and OLPC XO-1.75 laptops.

[1] include/linux/power_supply.h
[2] Documentation/power/power_supply_class.txt

Cc: stable@vger.kernel.org
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 6da79ae14860..5a97e42a3547 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
+		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
 		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
+		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
 		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
  2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:04   ` Andy Shevchenko
  2018-11-02 22:16   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
                   ` (15 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

It doesn't make sense to always have this built-in, e.g. on ARM
multiplatform kernels. A better way to address the problem the original
commit aimed to solve is to fix Kconfig.

This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/olpc-ec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
index 374a8028fec7..f99b183d5296 100644
--- a/drivers/platform/olpc/olpc-ec.c
+++ b/drivers/platform/olpc/olpc-ec.c
@@ -1,8 +1,6 @@
 /*
  * Generic driver for the OLPC Embedded Controller.
  *
- * Author: Andres Salomon <dilinger@queued.net>
- *
  * Copyright (C) 2011-2012 One Laptop per Child Foundation.
  *
  * Licensed under the GPL v2 or later.
@@ -14,7 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
-#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/list.h>
 #include <linux/olpc-ec.h>
 #include <asm/olpc.h>
@@ -328,4 +326,8 @@ static int __init olpc_ec_init_module(void)
 {
 	return platform_driver_register(&olpc_ec_plat_driver);
 }
+
 arch_initcall(olpc_ec_init_module);
+
+MODULE_AUTHOR("Andres Salomon <dilinger@queued.net>");
+MODULE_LICENSE("GPL");
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
  2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
  2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-17 19:38   ` Rob Herring
  2018-11-04 12:26   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
                   ` (14 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

The OLPC XO-1.75 Embedded Controller is a SPI master that uses extra
signals for handshaking. It needs to know when is the slave (Linux)
side's TX FIFO ready for transfer (the ready-gpio signal on the SPI
controller node) and when does it wish to respond with a command (the
cmd-gpio property).

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 .../bindings/misc/olpc,xo1.75-ec.txt          | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt

diff --git a/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
new file mode 100644
index 000000000000..14385fee65d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
@@ -0,0 +1,24 @@
+OLPC XO-1.75 Embedded Controller
+
+Required properties:
+- compatible: Should be "olpc,xo1.75-ec".
+- cmd-gpio: gpio specifier of the CMD pin
+
+The embedded controller requires the SPI controller driver to signal readiness
+to receive a transfer (that is, when TX FIFO contains the response data) by
+strobing the ACK pin with the ready signal. See the "ready-gpio" property of the
+SSP binding as documented in:
+<Documentation/devicetree/bindings/spi/spi-pxa2xx.txt>.
+
+Example:
+	&ssp3 {
+		spi-slave;
+		status = "okay";
+		ready-gpio = <&gpio 125 GPIO_ACTIVE_HIGH>;
+
+		slave {
+			compatible = "olpc,xo1.75-ec";
+			spi-cpha;
+			cmd-gpio = <&gpio 155 GPIO_ACTIVE_HIGH>;
+		};
+	};
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 04/15] Platform: OLPC: Remove an unused include
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (2 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:05   ` Andy Shevchenko
  2018-11-04 12:26   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
                   ` (13 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

Also, the header is x86 specific, while there are non-x86 OLPC machines.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/olpc-ec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
index f99b183d5296..35a21c66cd0d 100644
--- a/drivers/platform/olpc/olpc-ec.c
+++ b/drivers/platform/olpc/olpc-ec.c
@@ -15,7 +15,6 @@
 #include <linux/module.h>
 #include <linux/list.h>
 #include <linux/olpc-ec.h>
-#include <asm/olpc.h>
 
 struct ec_cmd_desc {
 	u8 cmd;
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (3 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:09   ` Andy Shevchenko
  2018-11-04 12:27   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver Lubomir Rintel
                   ` (12 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

There are ARM OLPC machines that use mostly the same drivers, including
EC infrastructure, DCON and Battery.

While at that, fix Kconfig to allow building this as a module.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/x86/Kconfig                  | 11 -----------
 drivers/input/mouse/Kconfig       |  2 +-
 drivers/platform/Kconfig          |  2 ++
 drivers/platform/olpc/Kconfig     | 11 +++++++++++
 drivers/staging/olpc_dcon/Kconfig |  2 +-
 5 files changed, 15 insertions(+), 13 deletions(-)
 create mode 100644 drivers/platform/olpc/Kconfig

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 1a0be022f91d..be6af341a718 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2729,17 +2729,6 @@ config SCx200HR_TIMER
 	  processor goes idle (as is done by the scheduler).  The
 	  other workaround is idle=poll boot option.
 
-config OLPC
-	bool "One Laptop Per Child support"
-	depends on !X86_PAE
-	select GPIOLIB
-	select OF
-	select OF_PROMTREE
-	select IRQ_DOMAIN
-	---help---
-	  Add support for detecting the unique features of the OLPC
-	  XO hardware.
-
 config OLPC_XO1_PM
 	bool "OLPC XO-1 Power Management"
 	depends on OLPC && MFD_CS5535 && PM_SLEEP
diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
index 566a1e3aa504..58034892a4df 100644
--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -165,7 +165,7 @@ config MOUSE_PS2_TOUCHKIT
 
 config MOUSE_PS2_OLPC
 	bool "OLPC PS/2 mouse protocol extension"
-	depends on MOUSE_PS2 && OLPC
+	depends on MOUSE_PS2 && X86 && OLPC
 	help
 	  Say Y here if you have an OLPC XO-1 laptop (with built-in
 	  PS/2 touchpad/tablet device).  The manufacturer calls the
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index d4c2e424a700..4313d73d3618 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -10,3 +10,5 @@ source "drivers/platform/goldfish/Kconfig"
 source "drivers/platform/chrome/Kconfig"
 
 source "drivers/platform/mellanox/Kconfig"
+
+source "drivers/platform/olpc/Kconfig"
diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
new file mode 100644
index 000000000000..7b736c9e67ac
--- /dev/null
+++ b/drivers/platform/olpc/Kconfig
@@ -0,0 +1,11 @@
+config OLPC
+	tristate "One Laptop Per Child support"
+	depends on X86 || ARM || COMPILE_TEST
+	depends on !X86_PAE
+	select GPIOLIB
+	select OF
+	select OF_PROMTREE if X86
+	select IRQ_DOMAIN
+	help
+	  Add support for detecting the unique features of the OLPC
+	  XO hardware.
diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig
index c91a56f77bcb..07f9f1de8667 100644
--- a/drivers/staging/olpc_dcon/Kconfig
+++ b/drivers/staging/olpc_dcon/Kconfig
@@ -1,6 +1,6 @@
 config FB_OLPC_DCON
 	tristate "One Laptop Per Child Display CONtroller support"
-	depends on OLPC && FB
+	depends on X86 && OLPC && FB
 	depends on I2C
 	depends on (GPIO_CS5535 || GPIO_CS5535=n)
 	select BACKLIGHT_CLASS_DEVICE
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (4 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 16:06   ` Andy Shevchenko
  2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
                   ` (11 subsequent siblings)
  17 siblings, 1 reply; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

It's based off the driver from the OLPC kernel sources. Somewhat
modernized and cleaned up, for better or worse.

Modified to plug into the olpc-ec driver infrastructure (so that battery
interface and debugfs could be reused) and the SPI slave framework.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/Kconfig         |  15 +
 drivers/platform/olpc/Makefile        |   1 +
 drivers/platform/olpc/olpc-xo175-ec.c | 755 ++++++++++++++++++++++++++
 3 files changed, 771 insertions(+)
 create mode 100644 drivers/platform/olpc/olpc-xo175-ec.c

diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
index 7b736c9e67ac..7c643d24ad0f 100644
--- a/drivers/platform/olpc/Kconfig
+++ b/drivers/platform/olpc/Kconfig
@@ -9,3 +9,18 @@ config OLPC
 	help
 	  Add support for detecting the unique features of the OLPC
 	  XO hardware.
+
+if OLPC
+
+config OLPC_XO175_EC
+	tristate "OLPC XO 1.75 Embedded Controller"
+	depends on ARCH_MMP || COMPILE_TEST
+	select SPI_SLAVE
+	help
+	  Include support for the OLPC XO Embedded Controller (EC). The EC
+	  provides various platform services, including support for the power,
+	  button, restart, shutdown and battery charging status.
+
+	  Unless you have an OLPC XO laptop, you will want to say N.
+
+endif
diff --git a/drivers/platform/olpc/Makefile b/drivers/platform/olpc/Makefile
index dc8b26bc7209..5b43f383289e 100644
--- a/drivers/platform/olpc/Makefile
+++ b/drivers/platform/olpc/Makefile
@@ -2,3 +2,4 @@
 # OLPC XO platform-specific drivers
 #
 obj-$(CONFIG_OLPC)		+= olpc-ec.o
+obj-$(CONFIG_OLPC_XO175_EC)	+= olpc-xo175-ec.o
diff --git a/drivers/platform/olpc/olpc-xo175-ec.c b/drivers/platform/olpc/olpc-xo175-ec.c
new file mode 100644
index 000000000000..6333a1366730
--- /dev/null
+++ b/drivers/platform/olpc/olpc-xo175-ec.c
@@ -0,0 +1,755 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for the OLPC XO-1.75 Embedded Controller.
+ *
+ * The EC protocol is documented at:
+ * http://wiki.laptop.org/go/XO_1.75_HOST_to_EC_Protocol
+ *
+ * Copyright (C) 2010 One Laptop per Child Foundation.
+ * Copyright (C) 2018 Lubomir Rintel <lkundrak@v3.sk>
+ */
+
+#include <asm/system_misc.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spinlock.h>
+#include <linux/completion.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/ctype.h>
+#include <linux/olpc-ec.h>
+#include <linux/spi/spi.h>
+#include <linux/reboot.h>
+#include <linux/input.h>
+#include <linux/kfifo.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+
+struct ec_cmd_t {
+	u8 cmd;
+	u8 bytes_returned;
+};
+
+enum ec_chan_t {
+	CHAN_NONE = 0,
+	CHAN_SWITCH,
+	CHAN_CMD_RESP,
+	CHAN_KEYBOARD,
+	CHAN_TOUCHPAD,
+	CHAN_EVENT,
+	CHAN_DEBUG,
+	CHAN_CMD_ERROR,
+};
+
+/*
+ * EC events
+ */
+#define EVENT_AC_CHANGE			1  /* AC plugged/unplugged */
+#define EVENT_BATTERY_STATUS		2  /* Battery low/full/error/gone */
+#define EVENT_BATTERY_CRITICAL		3  /* Battery critical voltage */
+#define EVENT_BATTERY_SOC_CHANGE	4  /* 1% SOC Change */
+#define EVENT_BATTERY_ERROR		5  /* Abnormal error, query for cause */
+#define EVENT_POWER_PRESSED		6  /* Power button was pressed */
+#define EVENT_POWER_PRESS_WAKE		7  /* Woken up with a power button */
+#define EVENT_TIMED_HOST_WAKE		8  /* Host wake timer */
+#define EVENT_OLS_HIGH_LIMIT		9  /* OLS crossed dark threshold */
+#define EVENT_OLS_LOW_LIMIT		10 /* OLS crossed light threshold */
+
+/*
+ * EC commands
+ * (from http://dev.laptop.org/git/users/rsmith/ec-1.75/tree/ec_cmd.h)
+ */
+#define CMD_GET_API_VERSION		0x08 /* out: u8 */
+#define CMD_READ_VOLTAGE		0x10 /* out: u16, *9.76/32, mV */
+#define CMD_READ_CURRENT		0x11 /* out: s16, *15.625/120, mA */
+#define CMD_READ_ACR			0x12 /* out: s16, *6250/15, uAh */
+#define CMD_READ_BATT_TEMPERATURE	0x13 /* out: u16, *100/256, deg C */
+#define CMD_READ_AMBIENT_TEMPERATURE	0x14 /* unimplemented, no hardware */
+#define CMD_READ_BATTERY_STATUS		0x15 /* out: u8, bitmask */
+#define CMD_READ_SOC			0x16 /* out: u8, percentage */
+#define CMD_READ_GAUGE_ID		0x17 /* out: u8 * 8 */
+#define CMD_READ_GAUGE_DATA		0x18 /* in: u8 addr, out: u8 data */
+#define CMD_READ_BOARD_ID		0x19 /* out: u16 (platform id) */
+#define CMD_READ_BATT_ERR_CODE		0x1f /* out: u8, error bitmask */
+#define CMD_SET_DCON_POWER		0x26 /* in: u8 */
+#define CMD_RESET_EC			0x28 /* none */
+#define CMD_READ_BATTERY_TYPE		0x2c /* out: u8 */
+#define CMD_SET_AUTOWAK			0x33 /* out: u8 */
+#define CMD_SET_EC_WAKEUP_TIMER		0x36 /* in: u32, out: ? */
+#define CMD_READ_EXT_SCI_MASK		0x37 /* ? */
+#define CMD_WRITE_EXT_SCI_MASK		0x38 /* ? */
+#define CMD_CLEAR_EC_WAKEUP_TIMER	0x39 /* none */
+#define CMD_ENABLE_RUNIN_DISCHARGE	0x3B /* none */
+#define CMD_DISABLE_RUNIN_DISCHARGE	0x3C /* none */
+#define CMD_READ_MPPT_ACTIVE		0x3d /* out: u8 */
+#define CMD_READ_MPPT_LIMIT		0x3e /* out: u8 */
+#define CMD_SET_MPPT_LIMIT		0x3f /* in: u8 */
+#define CMD_DISABLE_MPPT		0x40 /* none */
+#define CMD_ENABLE_MPPT			0x41 /* none */
+#define CMD_READ_VIN			0x42 /* out: u16 */
+#define CMD_EXT_SCI_QUERY		0x43 /* ? */
+#define RSP_KEYBOARD_DATA		0x48 /* ? */
+#define RSP_TOUCHPAD_DATA		0x49 /* ? */
+#define CMD_GET_FW_VERSION		0x4a /* out: u8 * 16 */
+#define CMD_POWER_CYCLE			0x4b /* none */
+#define CMD_POWER_OFF			0x4c /* none */
+#define CMD_RESET_EC_SOFT		0x4d /* none */
+#define CMD_READ_GAUGE_U16		0x4e /* ? */
+#define CMD_ENABLE_MOUSE		0x4f /* ? */
+#define CMD_ECHO			0x52 /* in: u8 * 5, out: u8 * 5 */
+#define CMD_GET_FW_DATE			0x53 /* out: u8 * 16 */
+#define CMD_GET_FW_USER			0x54 /* out: u8 * 16 */
+#define CMD_TURN_OFF_POWER		0x55 /* none (same as 0x4c) */
+#define CMD_READ_OLS			0x56 /* out: u16 */
+#define CMD_OLS_SMT_LEDON		0x57 /* none */
+#define CMD_OLS_SMT_LEDOFF		0x58 /* none */
+#define CMD_START_OLS_ASSY		0x59 /* none */
+#define CMD_STOP_OLS_ASSY		0x5a /* none */
+#define CMD_OLS_SMTTEST_STOP		0x5b /* none */
+#define CMD_READ_VIN_SCALED		0x5c /* out: u16 */
+#define CMD_READ_BAT_MIN_W		0x5d /* out: u16 */
+#define CMD_READ_BAR_MAX_W		0x5e /* out: u16 */
+#define CMD_RESET_BAT_MINMAX_W		0x5f /* none */
+#define CMD_READ_LOCATION		0x60 /* in: u16 addr, out: u8 data */
+#define CMD_WRITE_LOCATION		0x61 /* in: u16 addr, u8 data */
+#define CMD_KEYBOARD_CMD		0x62 /* in: u8, out: ? */
+#define CMD_TOUCHPAD_CMD		0x63 /* in: u8, out: ? */
+#define CMD_GET_FW_HASH			0x64 /* out: u8 * 16 */
+#define CMD_SUSPEND_HINT		0x65 /* in: u8 */
+#define CMD_ENABLE_WAKE_TIMER		0x66 /* in: u8 */
+#define CMD_SET_WAKE_TIMER		0x67 /* in: 32 */
+#define CMD_ENABLE_WAKE_AUTORESET	0x68 /* in: u8 */
+#define CMD_OLS_SET_LIMITS		0x69 /* in: u16, u16 */
+#define CMD_OLS_GET_LIMITS		0x6a /* out: u16, u16 */
+#define CMD_OLS_SET_CEILING		0x6b /* in: u16 */
+#define CMD_OLS_GET_CEILING		0x6c /* out: u16 */
+
+/*
+ * Accepted EC commands, and how many bytes they return. There are plenty
+ * of EC commands that are no longer implemented, or are implemented only on
+ * certain older boards.
+ */
+static const struct ec_cmd_t olpc_xo175_ec_cmds[] = {
+	{ CMD_GET_API_VERSION, 1 },
+	{ CMD_READ_VOLTAGE, 2 },
+	{ CMD_READ_CURRENT, 2 },
+	{ CMD_READ_ACR, 2 },
+	{ CMD_READ_BATT_TEMPERATURE, 2 },
+	{ CMD_READ_BATTERY_STATUS, 1 },
+	{ CMD_READ_SOC, 1 },
+	{ CMD_READ_GAUGE_ID, 8 },
+	{ CMD_READ_GAUGE_DATA, 1 },
+	{ CMD_READ_BOARD_ID, 2 },
+	{ CMD_READ_BATT_ERR_CODE, 1 },
+	{ CMD_SET_DCON_POWER, 0 },
+	{ CMD_RESET_EC, 0 },
+	{ CMD_READ_BATTERY_TYPE, 1 },
+	{ CMD_ENABLE_RUNIN_DISCHARGE, 0 },
+	{ CMD_DISABLE_RUNIN_DISCHARGE, 0 },
+	{ CMD_READ_MPPT_ACTIVE, 1 },
+	{ CMD_READ_MPPT_LIMIT, 1 },
+	{ CMD_SET_MPPT_LIMIT, 0 },
+	{ CMD_DISABLE_MPPT, 0 },
+	{ CMD_ENABLE_MPPT, 0 },
+	{ CMD_READ_VIN, 2 },
+	{ CMD_GET_FW_VERSION, 16 },
+	{ CMD_POWER_CYCLE, 0 },
+	{ CMD_POWER_OFF, 0 },
+	{ CMD_RESET_EC_SOFT, 0 },
+	{ CMD_ECHO, 5 },
+	{ CMD_GET_FW_DATE, 16 },
+	{ CMD_GET_FW_USER, 16 },
+	{ CMD_TURN_OFF_POWER, 0 },
+	{ CMD_READ_OLS, 2 },
+	{ CMD_OLS_SMT_LEDON, 0 },
+	{ CMD_OLS_SMT_LEDOFF, 0 },
+	{ CMD_START_OLS_ASSY, 0 },
+	{ CMD_STOP_OLS_ASSY, 0 },
+	{ CMD_OLS_SMTTEST_STOP, 0 },
+	{ CMD_READ_VIN_SCALED, 2 },
+	{ CMD_READ_BAT_MIN_W, 2 },
+	{ CMD_READ_BAR_MAX_W, 2 },
+	{ CMD_RESET_BAT_MINMAX_W, 0 },
+	{ CMD_READ_LOCATION, 1 },
+	{ CMD_WRITE_LOCATION, 0 },
+	{ CMD_GET_FW_HASH, 16 },
+	{ CMD_SUSPEND_HINT, 0 },
+	{ CMD_ENABLE_WAKE_TIMER, 0 },
+	{ CMD_SET_WAKE_TIMER, 0 },
+	{ CMD_ENABLE_WAKE_AUTORESET, 0 },
+	{ CMD_OLS_SET_LIMITS, 0 },
+	{ CMD_OLS_GET_LIMITS, 4 },
+	{ CMD_OLS_SET_CEILING, 0 },
+	{ CMD_OLS_GET_CEILING, 2 },
+	{ CMD_READ_EXT_SCI_MASK, 2 },
+	{ CMD_WRITE_EXT_SCI_MASK, 0 },
+
+	{ 0 },
+};
+
+#define EC_CMD_LEN		8
+#define EC_MAX_RESP_LEN		16
+#define LOG_BUF_SIZE		127
+
+enum ec_state_t {
+	CMD_STATE_IDLE = 0,
+	CMD_STATE_WAITING_FOR_SWITCH,
+	CMD_STATE_CMD_IN_TX_FIFO,
+	CMD_STATE_CMD_SENT,
+	CMD_STATE_RESP_RECEIVED,
+	CMD_STATE_ERROR_RECEIVED,
+};
+
+struct olpc_xo175_ec {
+	struct spi_device *spi;
+
+	struct spi_transfer xfer;
+	struct spi_message msg;
+
+	u8 tx_buf[EC_CMD_LEN];
+	u8 rx_buf[EC_MAX_RESP_LEN];
+
+	bool suspended;
+
+	/* GPIOs for ACK and CMD signals. */
+	struct gpio_desc *gpio_cmd;
+
+	/* Command handling related state. */
+	spinlock_t cmd_state_lock;
+	int cmd_state;
+	bool cmd_running;
+	struct completion cmd_done;
+	u8 cmd[EC_CMD_LEN];
+	int expected_resp_len;
+	u8 resp[EC_MAX_RESP_LEN];
+	int resp_len;
+
+	/* Power button. */
+	struct input_dev *pwrbtn;
+
+	/* Debug handling. */
+	char logbuf[LOG_BUF_SIZE + 1];
+	int logbuf_len;
+};
+
+static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
+{
+	const struct ec_cmd_t *p;
+
+	for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
+		if (p->cmd == cmd)
+			return p->bytes_returned;
+	}
+
+	return -1;
+}
+
+static void olpc_xo175_ec_flush_logbuf(struct olpc_xo175_ec *priv)
+{
+	priv->logbuf[priv->logbuf_len] = 0;
+	priv->logbuf_len = 0;
+
+	dev_dbg(&priv->spi->dev, "got debug string [%s]\n", priv->logbuf);
+}
+
+static void olpc_xo175_ec_complete(void *arg);
+
+static void olpc_xo175_ec_send_command(struct olpc_xo175_ec *priv, u8 *cmd,
+								size_t cmdlen)
+{
+	int ret;
+
+	memcpy(priv->tx_buf, cmd, cmdlen);
+	priv->xfer.len = cmdlen;
+
+	spi_message_init_with_transfers(&priv->msg, &priv->xfer, 1);
+
+	priv->msg.complete = olpc_xo175_ec_complete;
+	priv->msg.context = priv;
+
+	ret = spi_async(priv->spi, &priv->msg);
+	if (ret)
+		dev_err(&priv->spi->dev, "spi_async() failed %d\n", ret);
+}
+
+static void olpc_xo175_ec_read_packet(struct olpc_xo175_ec *priv)
+{
+	u8 nonce[] = {0xA5, 0x5A};
+
+	olpc_xo175_ec_send_command(priv, nonce, sizeof(nonce));
+}
+
+static void olpc_xo175_ec_complete(void *arg)
+{
+	struct olpc_xo175_ec *priv = arg;
+	struct device *dev = &priv->spi->dev;
+	struct power_supply *psy;
+	unsigned long flags;
+	u8 channel;
+	u8 byte;
+	int ret;
+
+	ret = priv->msg.status;
+	if (ret) {
+		dev_err(dev, "SPI transfer failed: %d\n", ret);
+
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+		if (priv->cmd_running) {
+			priv->resp_len = 0;
+			priv->cmd_state = CMD_STATE_ERROR_RECEIVED;
+			complete(&priv->cmd_done);
+		}
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+		if (ret != -EINTR)
+			olpc_xo175_ec_read_packet(priv);
+
+		return;
+	}
+
+	channel = priv->rx_buf[0];
+	byte = priv->rx_buf[1];
+
+	switch (channel) {
+	case CHAN_NONE:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			/* We can safely ignore these */
+			dev_err(dev, "spurious FIFO read packet\n");
+			spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+			return;
+		}
+
+		priv->cmd_state = CMD_STATE_CMD_SENT;
+		if (!priv->expected_resp_len)
+			complete(&priv->cmd_done);
+		olpc_xo175_ec_read_packet(priv);
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		return;
+
+	case CHAN_SWITCH:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			/* Just go with the flow */
+			dev_err(dev, "spurious SWITCH packet\n");
+			memset(priv->cmd, 0, sizeof(priv->cmd));
+			priv->cmd[0] = CMD_ECHO;
+		}
+
+		priv->cmd_state = CMD_STATE_CMD_IN_TX_FIFO;
+
+		/* Throw command into TxFIFO */
+		gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+		olpc_xo175_ec_send_command(priv, priv->cmd, sizeof(priv->cmd));
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		return;
+
+	case CHAN_CMD_RESP:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			dev_err(dev, "spurious response packet\n");
+		} else if (priv->resp_len >= priv->expected_resp_len) {
+			dev_err(dev, "too many response packets\n");
+		} else {
+			priv->resp[priv->resp_len++] = byte;
+
+			if (priv->resp_len == priv->expected_resp_len) {
+				priv->cmd_state = CMD_STATE_RESP_RECEIVED;
+				complete(&priv->cmd_done);
+			}
+		}
+
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		break;
+
+	case CHAN_CMD_ERROR:
+		spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+		if (!priv->cmd_running) {
+			dev_err(dev, "spurious cmd error packet\n");
+		} else {
+			priv->resp[0] = byte;
+			priv->resp_len = 1;
+			priv->cmd_state = CMD_STATE_ERROR_RECEIVED;
+			complete(&priv->cmd_done);
+		}
+		spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+		break;
+
+	case CHAN_KEYBOARD:
+	case CHAN_TOUCHPAD:
+		dev_warn(dev, "kbd/tpad not supported\n");
+		break;
+
+	case CHAN_EVENT:
+		dev_dbg(dev, "got event %.2x\n", byte);
+		switch (byte) {
+		case EVENT_AC_CHANGE:
+			psy = power_supply_get_by_name("olpc-ac");
+			if (psy) {
+				power_supply_changed(psy);
+				power_supply_put(psy);
+			}
+			break;
+		case EVENT_BATTERY_STATUS:
+		case EVENT_BATTERY_CRITICAL:
+		case EVENT_BATTERY_SOC_CHANGE:
+		case EVENT_BATTERY_ERROR:
+			psy = power_supply_get_by_name("olpc-battery");
+			if (psy) {
+				power_supply_changed(psy);
+				power_supply_put(psy);
+			}
+			break;
+		case EVENT_POWER_PRESSED:
+			input_report_key(priv->pwrbtn, KEY_POWER, 1);
+			input_sync(priv->pwrbtn);
+			input_report_key(priv->pwrbtn, KEY_POWER, 0);
+			input_sync(priv->pwrbtn);
+			/* fall through */
+		case EVENT_POWER_PRESS_WAKE:
+		case EVENT_TIMED_HOST_WAKE:
+			pm_wakeup_event(priv->pwrbtn->dev.parent, 1000);
+			break;
+		default:
+			/* For now, we just ignore the unknown events. */
+			break;
+		}
+		break;
+
+	case CHAN_DEBUG:
+		if (byte == '\n') {
+			olpc_xo175_ec_flush_logbuf(priv);
+		} else if (isprint(byte)) {
+			priv->logbuf[priv->logbuf_len++] = byte;
+			if (priv->logbuf_len == LOG_BUF_SIZE)
+				olpc_xo175_ec_flush_logbuf(priv);
+		}
+		break;
+
+	default:
+		dev_warn(dev, "unknown channel: %d, %.2x\n", channel, byte);
+		break;
+	}
+
+	/* Most non-command packets get the TxFIFO refilled and an ACK. */
+	olpc_xo175_ec_read_packet(priv);
+}
+
+/*
+ * This function is protected with a mutex. We can safely assume that
+ * there will be only one instance of this function running at a time.
+ * One of the ways in which we enforce this is by waiting until we get
+ * all response bytes back from the EC, rather than just the number that
+ * the caller requests (otherwise, we might start a new command while an
+ * old command's response bytes are still incoming).
+ */
+static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *resp,
+					size_t resp_len, void *ec_cb_arg)
+{
+	struct olpc_xo175_ec *priv = ec_cb_arg;
+	struct device *dev = &priv->spi->dev;
+	unsigned long flags;
+	int nr_bytes;
+	int ret = 0;
+
+	dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
+
+	if (inlen > 5) {
+		dev_err(dev, "command len %d too big!\n", resp_len);
+		return -EOVERFLOW;
+	}
+
+	/* Suspending in the middle of an EC command hoses things badly! */
+	WARN_ON(priv->suspended);
+	if (priv->suspended)
+		return -EBUSY;
+
+	/* Ensure a valid command and return bytes */
+	nr_bytes = olpc_xo175_ec_is_valid_cmd(cmd);
+	if (nr_bytes < 0) {
+		dev_err_ratelimited(dev, "unknown command 0x%x\n", cmd);
+
+		/*
+		 * Assume the best in our callers, and allow unknown commands
+		 * through. I'm not the charitable type, but it was beaten
+		 * into me. Just maintain a minimum standard of sanity.
+		 */
+		if (resp_len > sizeof(priv->resp)) {
+			dev_err(dev, "response too big: %d!\n", resp_len);
+			return -EOVERFLOW;
+		}
+		nr_bytes = resp_len;
+	}
+	if (resp_len > nr_bytes)
+		resp_len = nr_bytes;
+
+	spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+	/* Initialize the state machine */
+	init_completion(&priv->cmd_done);
+	priv->cmd_running = true;
+	priv->cmd_state = CMD_STATE_WAITING_FOR_SWITCH;
+	memset(priv->cmd, 0, sizeof(priv->cmd));
+	priv->cmd[0] = cmd;
+	priv->cmd[1] = inlen;
+	priv->cmd[2] = 0;
+	memcpy(&priv->cmd[3], inbuf, inlen);
+	priv->expected_resp_len = nr_bytes;
+	priv->resp_len = 0;
+	memset(resp, 0, resp_len);
+
+	/* Tickle the cmd gpio to get things started */
+	gpiod_set_value_cansleep(priv->gpio_cmd, 1);
+
+	spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+	/* The irq handler should do the rest */
+	if (!wait_for_completion_timeout(&priv->cmd_done,
+			msecs_to_jiffies(4000))) {
+		dev_err(dev, "EC cmd error: timeout in STATE %d\n",
+				priv->cmd_state);
+		gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+		spi_slave_abort(priv->spi);
+		olpc_xo175_ec_read_packet(priv);
+		return -ETIMEDOUT;
+	}
+
+	spin_lock_irqsave(&priv->cmd_state_lock, flags);
+
+	/* Deal with the results. */
+	if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
+		/* EC-provided error is in the single response byte */
+		dev_err(dev, "command 0x%x returned error 0x%x\n",
+							cmd, priv->resp[0]);
+		ret = -EREMOTEIO;
+	} else if (priv->resp_len != nr_bytes) {
+		dev_err(dev, "command 0x%x returned %d bytes, expected %d bytes\n",
+						cmd, priv->resp_len, nr_bytes);
+		ret = -ETIMEDOUT;
+	} else {
+		/*
+		 * We may have 8 bytes in priv->resp, but we only care about
+		 * what we've been asked for. If the caller asked for only 2
+		 * bytes, give them that. We've guaranteed that
+		 * resp_len <= priv->resp_len and priv->resp_len == nr_bytes.
+		 */
+		memcpy(resp, priv->resp, resp_len);
+	}
+
+	/* This should already be low, but just in case. */
+	gpiod_set_value_cansleep(priv->gpio_cmd, 0);
+	priv->cmd_running = false;
+
+	spin_unlock_irqrestore(&priv->cmd_state_lock, flags);
+
+	return ret;
+}
+
+static int olpc_xo175_ec_set_event_mask(unsigned int mask)
+{
+	unsigned char args[2];
+
+	args[0] = mask & 0xff;
+	args[1] = (mask >> 8) & 0xff;
+	return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL, 0);
+}
+
+static void olpc_xo175_ec_restart(enum reboot_mode mode, const char *cmd)
+{
+	while (1) {
+		olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
+		mdelay(1000);
+	}
+}
+
+static void olpc_xo175_ec_power_off(void)
+{
+	while (1) {
+		olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
+		mdelay(1000);
+	}
+}
+
+#ifdef CONFIG_PM
+static int olpc_xo175_ec_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+	unsigned char hintargs[5];
+	static unsigned int suspend_count;
+
+	suspend_count++;
+	dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);
+
+	/*
+	 * First byte is 1 to indicate suspend, the rest is an integer
+	 * counter.
+	 */
+	hintargs[0] = 1;
+	memcpy(&hintargs[1], &suspend_count, 4);
+	olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
+
+	/*
+	 * After we've sent the suspend hint, don't allow further EC commands
+	 * to be run until we've resumed. Userspace tasks should be frozen,
+	 * but kernel threads and interrupts could still schedule EC commands.
+	 */
+	priv->suspended = true;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+
+	priv->suspended = false;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
+	unsigned char x = 0;
+
+	priv->suspended = false;
+
+	/*
+	 * The resume hint is only needed if no other commands are
+	 * being sent during resume. all it does is tell the EC
+	 * the SoC is definitely awake.
+	 */
+	olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
+
+	/* Enable all EC events while we're awake */
+	olpc_xo175_ec_set_event_mask(0xffff);
+
+	return 0;
+}
+#endif
+
+static struct platform_device *olpc_ec;
+
+static struct olpc_ec_driver olpc_xo175_ec_driver = {
+	.ec_cmd = olpc_xo175_ec_cmd,
+};
+
+static int olpc_xo175_ec_remove(struct spi_device *spi)
+{
+	if (pm_power_off == olpc_xo175_ec_power_off)
+		pm_power_off = NULL;
+	if (arm_pm_restart == olpc_xo175_ec_restart)
+		arm_pm_restart = NULL;
+
+	spi_slave_abort(spi);
+
+	platform_device_unregister(olpc_ec);
+	olpc_ec = NULL;
+
+	return 0;
+}
+
+static int olpc_xo175_ec_probe(struct spi_device *spi)
+{
+	struct olpc_xo175_ec *priv;
+	int ret;
+
+	if (olpc_ec) {
+		dev_err(&spi->dev, "OLPC EC already registered.\n");
+		return -EBUSY;
+	}
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->gpio_cmd = devm_gpiod_get(&spi->dev, "cmd", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->gpio_cmd)) {
+		dev_err(&spi->dev, "failed to get cmd gpio: %ld\n",
+					PTR_ERR(priv->gpio_cmd));
+		return PTR_ERR(priv->gpio_cmd);
+	}
+
+	priv->spi = spi;
+
+	spin_lock_init(&priv->cmd_state_lock);
+	priv->cmd_state = CMD_STATE_IDLE;
+	init_completion(&priv->cmd_done);
+
+	priv->logbuf_len = 0;
+
+	/* Set up power button input device */
+	priv->pwrbtn = devm_input_allocate_device(&spi->dev);
+	if (!priv->pwrbtn)
+		return -ENOMEM;
+	priv->pwrbtn->name = "Power Button";
+	priv->pwrbtn->dev.parent = &spi->dev;
+	input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
+	ret = input_register_device(priv->pwrbtn);
+	if (ret) {
+		dev_err(&spi->dev, "error registering input device: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, priv);
+
+	priv->xfer.rx_buf = priv->rx_buf;
+	priv->xfer.tx_buf = priv->tx_buf;
+
+	olpc_xo175_ec_read_packet(priv);
+
+	olpc_ec_driver_register(&olpc_xo175_ec_driver, priv);
+	olpc_ec = platform_device_register_resndata(&spi->dev, "olpc-ec", -1,
+							NULL, 0, NULL, 0);
+
+	/* Enable all EC events while we're awake */
+	olpc_xo175_ec_set_event_mask(0xffff);
+
+	if (pm_power_off == NULL)
+		pm_power_off = olpc_xo175_ec_power_off;
+	if (arm_pm_restart == NULL)
+		arm_pm_restart = olpc_xo175_ec_restart;
+
+	dev_info(&spi->dev, "OLPC XO-1.75 Embedded Controller driver\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops olpc_xo175_ec_pm_ops = {
+#ifdef CONFIG_PM
+	.suspend	= olpc_xo175_ec_suspend,
+	.resume_noirq	= olpc_xo175_ec_resume_noirq,
+	.resume		= olpc_xo175_ec_resume,
+#endif
+};
+
+static const struct of_device_id olpc_xo175_ec_of_match[] = {
+	{ .compatible = "olpc,xo1.75-ec" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, olpc_xo175_ec_of_match);
+
+static struct spi_driver olpc_xo175_ec_spi_driver = {
+	.driver = {
+		.name	= "olpc-xo175-ec",
+		.of_match_table = olpc_xo175_ec_of_match,
+		.pm = &olpc_xo175_ec_pm_ops,
+	},
+	.probe		= olpc_xo175_ec_probe,
+	.remove		= olpc_xo175_ec_remove,
+};
+module_spi_driver(olpc_xo175_ec_spi_driver);
+
+MODULE_DESCRIPTION("OLPC XO-1.75 Embedded Controller driver");
+MODULE_AUTHOR("Lennert Buytenhek <buytenh@wantstofly.org>"); /* Functionality */
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>"); /* Bugs */
+MODULE_LICENSE("GPL");
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (5 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:11   ` Andy Shevchenko
  2018-11-04 12:30   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
                   ` (10 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

Just return ENODEV, so that whoever attempted to use the EC call can
defer their work.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/olpc-ec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
index 35a21c66cd0d..342f5bb7f7a8 100644
--- a/drivers/platform/olpc/olpc-ec.c
+++ b/drivers/platform/olpc/olpc-ec.c
@@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf, size_t outlen)
 	struct olpc_ec_priv *ec = ec_priv;
 	struct ec_cmd_desc desc;
 
-	/* Ensure a driver and ec hook have been registered */
-	if (WARN_ON(!ec_driver || !ec_driver->ec_cmd))
+	/* Driver not yet registered. */
+	if (!ec_driver)
+		return -ENODEV;
+
+	if (WARN_ON(!ec_driver->ec_cmd))
 		return -ENODEV;
 
 	if (!ec)
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (6 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:36   ` Andy Shevchenko
  2018-11-04 12:31   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 09/15] Platform: OLPC: add a regulator for the DCON Lubomir Rintel
                   ` (9 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

It is actually plaform independent. Move it to the olpc-ec driver from
the X86 OLPC platform, so that it could be used by the ARM based laptops
too.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/x86/include/asm/olpc.h     |  17 -----
 arch/x86/platform/olpc/olpc.c   | 119 +++++---------------------------
 drivers/platform/olpc/olpc-ec.c | 103 ++++++++++++++++++++++++++-
 include/linux/olpc-ec.h         |  32 ++++++++-
 4 files changed, 149 insertions(+), 122 deletions(-)

diff --git a/arch/x86/include/asm/olpc.h b/arch/x86/include/asm/olpc.h
index c2bf1de5d901..cf13d1254550 100644
--- a/arch/x86/include/asm/olpc.h
+++ b/arch/x86/include/asm/olpc.h
@@ -9,12 +9,10 @@
 struct olpc_platform_t {
 	int flags;
 	uint32_t boardrev;
-	int ecver;
 };
 
 #define OLPC_F_PRESENT		0x01
 #define OLPC_F_DCON		0x02
-#define OLPC_F_EC_WIDE_SCI	0x04
 
 #ifdef CONFIG_OLPC
 
@@ -64,13 +62,6 @@ static inline int olpc_board_at_least(uint32_t rev)
 	return olpc_platform_info.boardrev >= rev;
 }
 
-extern void olpc_ec_wakeup_set(u16 value);
-extern void olpc_ec_wakeup_clear(u16 value);
-extern bool olpc_ec_wakeup_available(void);
-
-extern int olpc_ec_mask_write(u16 bits);
-extern int olpc_ec_sci_query(u16 *sci_value);
-
 #else
 
 static inline int machine_is_olpc(void)
@@ -83,14 +74,6 @@ static inline int olpc_has_dcon(void)
 	return 0;
 }
 
-static inline void olpc_ec_wakeup_set(u16 value) { }
-static inline void olpc_ec_wakeup_clear(u16 value) { }
-
-static inline bool olpc_ec_wakeup_available(void)
-{
-	return false;
-}
-
 #endif
 
 #ifdef CONFIG_OLPC_XO1_PM
diff --git a/arch/x86/platform/olpc/olpc.c b/arch/x86/platform/olpc/olpc.c
index f0e920fb98ad..c6c62b4f251f 100644
--- a/arch/x86/platform/olpc/olpc.c
+++ b/arch/x86/platform/olpc/olpc.c
@@ -30,9 +30,6 @@
 struct olpc_platform_t olpc_platform_info;
 EXPORT_SYMBOL_GPL(olpc_platform_info);
 
-/* EC event mask to be applied during suspend (defining wakeup sources). */
-static u16 ec_wakeup_mask;
-
 /* what the timeout *should* be (in ms) */
 #define EC_BASE_TIMEOUT 20
 
@@ -186,83 +183,6 @@ static int olpc_xo1_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf,
 	return ret;
 }
 
-void olpc_ec_wakeup_set(u16 value)
-{
-	ec_wakeup_mask |= value;
-}
-EXPORT_SYMBOL_GPL(olpc_ec_wakeup_set);
-
-void olpc_ec_wakeup_clear(u16 value)
-{
-	ec_wakeup_mask &= ~value;
-}
-EXPORT_SYMBOL_GPL(olpc_ec_wakeup_clear);
-
-/*
- * Returns true if the compile and runtime configurations allow for EC events
- * to wake the system.
- */
-bool olpc_ec_wakeup_available(void)
-{
-	if (!machine_is_olpc())
-		return false;
-
-	/*
-	 * XO-1 EC wakeups are available when olpc-xo1-sci driver is
-	 * compiled in
-	 */
-#ifdef CONFIG_OLPC_XO1_SCI
-	if (olpc_platform_info.boardrev < olpc_board_pre(0xd0)) /* XO-1 */
-		return true;
-#endif
-
-	/*
-	 * XO-1.5 EC wakeups are available when olpc-xo15-sci driver is
-	 * compiled in
-	 */
-#ifdef CONFIG_OLPC_XO15_SCI
-	if (olpc_platform_info.boardrev >= olpc_board_pre(0xd0)) /* XO-1.5 */
-		return true;
-#endif
-
-	return false;
-}
-EXPORT_SYMBOL_GPL(olpc_ec_wakeup_available);
-
-int olpc_ec_mask_write(u16 bits)
-{
-	if (olpc_platform_info.flags & OLPC_F_EC_WIDE_SCI) {
-		__be16 ec_word = cpu_to_be16(bits);
-		return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *) &ec_word, 2,
-				   NULL, 0);
-	} else {
-		unsigned char ec_byte = bits & 0xff;
-		return olpc_ec_cmd(EC_WRITE_SCI_MASK, &ec_byte, 1, NULL, 0);
-	}
-}
-EXPORT_SYMBOL_GPL(olpc_ec_mask_write);
-
-int olpc_ec_sci_query(u16 *sci_value)
-{
-	int ret;
-
-	if (olpc_platform_info.flags & OLPC_F_EC_WIDE_SCI) {
-		__be16 ec_word;
-		ret = olpc_ec_cmd(EC_EXT_SCI_QUERY,
-			NULL, 0, (void *) &ec_word, 2);
-		if (ret == 0)
-			*sci_value = be16_to_cpu(ec_word);
-	} else {
-		unsigned char ec_byte;
-		ret = olpc_ec_cmd(EC_SCI_QUERY, NULL, 0, &ec_byte, 1);
-		if (ret == 0)
-			*sci_value = ec_byte;
-	}
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
-
 static bool __init check_ofw_architecture(struct device_node *root)
 {
 	const char *olpc_arch;
@@ -296,6 +216,10 @@ static bool __init platform_detect(void)
 	if (success) {
 		olpc_platform_info.boardrev = get_board_revision(root);
 		olpc_platform_info.flags |= OLPC_F_PRESENT;
+
+		pr_info("OLPC board revision %s%X\n",
+			((olpc_platform_info.boardrev & 0xf) < 8) ? "pre" : "",
+			olpc_platform_info.boardrev >> 4);
 	}
 
 	of_node_put(root);
@@ -315,27 +239,8 @@ static int __init add_xo1_platform_devices(void)
 	return PTR_ERR_OR_ZERO(pdev);
 }
 
-static int olpc_xo1_ec_probe(struct platform_device *pdev)
-{
-	/* get the EC revision */
-	olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
-			(unsigned char *) &olpc_platform_info.ecver, 1);
-
-	/* EC version 0x5f adds support for wide SCI mask */
-	if (olpc_platform_info.ecver >= 0x5f)
-		olpc_platform_info.flags |= OLPC_F_EC_WIDE_SCI;
-
-	pr_info("OLPC board revision %s%X (EC=%x)\n",
-			((olpc_platform_info.boardrev & 0xf) < 8) ? "pre" : "",
-			olpc_platform_info.boardrev >> 4,
-			olpc_platform_info.ecver);
-
-	return 0;
-}
 static int olpc_xo1_ec_suspend(struct platform_device *pdev)
 {
-	olpc_ec_mask_write(ec_wakeup_mask);
-
 	/*
 	 * Squelch SCIs while suspended.  This is a fix for
 	 * <http://dev.laptop.org/ticket/1835>.
@@ -359,15 +264,27 @@ static int olpc_xo1_ec_resume(struct platform_device *pdev)
 }
 
 static struct olpc_ec_driver ec_xo1_driver = {
-	.probe = olpc_xo1_ec_probe,
 	.suspend = olpc_xo1_ec_suspend,
 	.resume = olpc_xo1_ec_resume,
 	.ec_cmd = olpc_xo1_ec_cmd,
+#ifdef CONFIG_OLPC_XO1_SCI
+	/*
+	 * XO-1 EC wakeups are available when olpc-xo1-sci driver is
+	 * compiled in
+	 */
+	.wakeup_available = true,
+#endif
 };
 
 static struct olpc_ec_driver ec_xo1_5_driver = {
-	.probe = olpc_xo1_ec_probe,
 	.ec_cmd = olpc_xo1_ec_cmd,
+#ifdef CONFIG_OLPC_XO1_5_SCI
+	/*
+	 * XO-1.5 EC wakeups are available when olpc-xo15-sci driver is
+	 * compiled in
+	 */
+	.wakeup_available = true,
+#endif
 };
 
 static int __init olpc_init(void)
diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
index 342f5bb7f7a8..b9d9a9897dd5 100644
--- a/drivers/platform/olpc/olpc-ec.c
+++ b/drivers/platform/olpc/olpc-ec.c
@@ -30,6 +30,7 @@ struct ec_cmd_desc {
 
 struct olpc_ec_priv {
 	struct olpc_ec_driver *drv;
+	int version;
 	struct work_struct worker;
 	struct mutex cmd_lock;
 
@@ -39,6 +40,12 @@ struct olpc_ec_priv {
 
 	struct dentry *dbgfs_dir;
 
+	/*
+	 * EC event mask to be applied during suspend (defining wakeup
+	 * sources).
+	 */
+	u16 ec_wakeup_mask;
+
 	/*
 	 * Running an EC command while suspending means we don't always finish
 	 * the command before the machine suspends.  This means that the EC
@@ -150,6 +157,91 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf, size_t outlen)
 }
 EXPORT_SYMBOL_GPL(olpc_ec_cmd);
 
+void olpc_ec_wakeup_set(u16 value)
+{
+	struct olpc_ec_priv *ec = ec_priv;
+
+	if (WARN_ON(!ec))
+		return;
+
+	ec->ec_wakeup_mask |= value;
+}
+EXPORT_SYMBOL_GPL(olpc_ec_wakeup_set);
+
+void olpc_ec_wakeup_clear(u16 value)
+{
+	struct olpc_ec_priv *ec = ec_priv;
+
+	if (WARN_ON(!ec))
+		return;
+
+	ec->ec_wakeup_mask &= ~value;
+}
+EXPORT_SYMBOL_GPL(olpc_ec_wakeup_clear);
+
+int olpc_ec_mask_write(u16 bits)
+{
+	struct olpc_ec_priv *ec = ec_priv;
+
+	if (WARN_ON(!ec))
+		return -ENODEV;
+
+	/* EC version 0x5f adds support for wide SCI mask */
+	if (ec->version >= 0x5f) {
+		__be16 ec_word = cpu_to_be16(bits);
+
+		return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *) &ec_word, 2,
+				   NULL, 0);
+	} else {
+		unsigned char ec_byte = bits & 0xff;
+
+		return olpc_ec_cmd(EC_WRITE_SCI_MASK, &ec_byte, 1, NULL, 0);
+	}
+}
+EXPORT_SYMBOL_GPL(olpc_ec_mask_write);
+
+/*
+ * Returns true if the compile and runtime configurations allow for EC events
+ * to wake the system.
+ */
+bool olpc_ec_wakeup_available(void)
+{
+	if (WARN_ON(!ec_driver))
+		return false;
+
+	return ec_driver->wakeup_available;
+}
+EXPORT_SYMBOL_GPL(olpc_ec_wakeup_available);
+
+int olpc_ec_sci_query(u16 *sci_value)
+{
+	struct olpc_ec_priv *ec = ec_priv;
+	int ret;
+
+	if (WARN_ON(!ec))
+		return -ENODEV;
+
+	/* EC version 0x5f adds support for wide SCI mask */
+	if (ec->version >= 0x5f) {
+		__be16 ec_word;
+
+		ret = olpc_ec_cmd(EC_EXT_SCI_QUERY,
+			NULL, 0, (void *) &ec_word, 2);
+		if (ret == 0)
+			*sci_value = be16_to_cpu(ec_word);
+	} else {
+		unsigned char ec_byte;
+
+		ret = olpc_ec_cmd(EC_SCI_QUERY,
+			NULL, 0, &ec_byte, 1);
+		if (ret == 0)
+			*sci_value = ec_byte;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
+
 #ifdef CONFIG_DEBUG_FS
 
 /*
@@ -277,14 +369,17 @@ static int olpc_ec_probe(struct platform_device *pdev)
 	ec_priv = ec;
 	platform_set_drvdata(pdev, ec);
 
-	err = ec_driver->probe ? ec_driver->probe(pdev) : 0;
+	/* get the EC revision */
+	err = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
+		(unsigned char *) &ec->version, 1);
 	if (err) {
 		ec_priv = NULL;
 		kfree(ec);
-	} else {
-		ec->dbgfs_dir = olpc_ec_setup_debugfs();
+		return err;
 	}
 
+	ec->dbgfs_dir = olpc_ec_setup_debugfs();
+
 	return err;
 }
 
@@ -294,6 +389,8 @@ static int olpc_ec_suspend(struct device *dev)
 	struct olpc_ec_priv *ec = platform_get_drvdata(pdev);
 	int err = 0;
 
+	olpc_ec_mask_write(ec->ec_wakeup_mask);
+
 	if (ec_driver->suspend)
 		err = ec_driver->suspend(pdev);
 	if (!err)
diff --git a/include/linux/olpc-ec.h b/include/linux/olpc-ec.h
index 79bdc6328c52..7fa3d27f7fee 100644
--- a/include/linux/olpc-ec.h
+++ b/include/linux/olpc-ec.h
@@ -16,14 +16,28 @@
 #define EC_SCI_QUERY			0x84
 #define EC_EXT_SCI_QUERY		0x85
 
+/* SCI source values */
+#define EC_SCI_SRC_EMPTY        0x00
+#define EC_SCI_SRC_GAME         0x01
+#define EC_SCI_SRC_BATTERY      0x02
+#define EC_SCI_SRC_BATSOC       0x04
+#define EC_SCI_SRC_BATERR       0x08
+#define EC_SCI_SRC_EBOOK        0x10    /* XO-1 only */
+#define EC_SCI_SRC_WLAN         0x20    /* XO-1 only */
+#define EC_SCI_SRC_ACPWR        0x40
+#define EC_SCI_SRC_BATCRIT      0x80
+#define EC_SCI_SRC_GPWAKE       0x100   /* XO-1.5 only */
+#define EC_SCI_SRC_ALL          0x1FF
+
 struct platform_device;
 
 struct olpc_ec_driver {
-	int (*probe)(struct platform_device *);
 	int (*suspend)(struct platform_device *);
 	int (*resume)(struct platform_device *);
 
 	int (*ec_cmd)(u8, u8 *, size_t, u8 *, size_t, void *);
+
+	bool wakeup_available;
 };
 
 #ifdef CONFIG_OLPC
@@ -33,11 +47,27 @@ extern void olpc_ec_driver_register(struct olpc_ec_driver *drv, void *arg);
 extern int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf,
 		size_t outlen);
 
+extern void olpc_ec_wakeup_set(u16 value);
+extern void olpc_ec_wakeup_clear(u16 value);
+
+extern int olpc_ec_mask_write(u16 bits);
+extern int olpc_ec_sci_query(u16 *sci_value);
+
+extern bool olpc_ec_wakeup_available(void);
+
 #else
 
 static inline int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf,
 		size_t outlen) { return -ENODEV; }
 
+static inline void olpc_ec_wakeup_set(u16 value) { }
+static inline void olpc_ec_wakeup_clear(u16 value) { }
+
+static inline bool olpc_ec_wakeup_available(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_OLPC */
 
 #endif /* _LINUX_OLPC_EC_H */
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 09/15] Platform: OLPC: add a regulator for the DCON
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (7 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:39   ` Andy Shevchenko
  2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
                   ` (8 subsequent siblings)
  17 siblings, 1 reply; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	linux-pm, Greg Kroah-Hartman, linux-kernel, Sebastian Reichel,
	Haojian Zhuang, linux-spi, Lubomir Rintel, Rob Herring,
	linux-arm-kernel, platform-driver-x86, Robert Jarzmik,
	Daniel Mack

All OLPC ECs are able to turn the power to the DCON on an off. Use the
regulator framework to expose the functionality.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/platform/olpc/Kconfig   |  1 +
 drivers/platform/olpc/olpc-ec.c | 65 +++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
index 7c643d24ad0f..c5a872fb286f 100644
--- a/drivers/platform/olpc/Kconfig
+++ b/drivers/platform/olpc/Kconfig
@@ -6,6 +6,7 @@ config OLPC
 	select OF
 	select OF_PROMTREE if X86
 	select IRQ_DOMAIN
+	select REGULATOR
 	help
 	  Add support for detecting the unique features of the OLPC
 	  XO hardware.
diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
index b9d9a9897dd5..8f82922631a9 100644
--- a/drivers/platform/olpc/olpc-ec.c
+++ b/drivers/platform/olpc/olpc-ec.c
@@ -14,6 +14,7 @@
 #include <linux/workqueue.h>
 #include <linux/module.h>
 #include <linux/list.h>
+#include <linux/regulator/driver.h>
 #include <linux/olpc-ec.h>
 
 struct ec_cmd_desc {
@@ -34,6 +35,10 @@ struct olpc_ec_priv {
 	struct work_struct worker;
 	struct mutex cmd_lock;
 
+	/* DCON regulator */
+	struct regulator_dev *dcon_rdev;
+	bool dcon_enabled;
+
 	/* Pending EC commands */
 	struct list_head cmd_q;
 	spinlock_t cmd_q_lock;
@@ -347,9 +352,60 @@ static struct dentry *olpc_ec_setup_debugfs(void)
 
 #endif /* CONFIG_DEBUG_FS */
 
+static int olpc_ec_set_dcon_power(struct olpc_ec_priv *ec, bool state)
+{
+	unsigned char ec_byte = state;
+	int ret;
+
+	if (ec->dcon_enabled == state)
+		return 0;
+
+	ret = olpc_ec_cmd(EC_DCON_POWER_MODE, &ec_byte, 1, NULL, 0);
+	if (ret == 0)
+		ec->dcon_enabled = state;
+
+	return ret;
+}
+
+static int dcon_regulator_enable(struct regulator_dev *rdev)
+{
+	struct olpc_ec_priv *ec = rdev_get_drvdata(rdev);
+
+	return olpc_ec_set_dcon_power(ec, 1);
+}
+
+static int dcon_regulator_disable(struct regulator_dev *rdev)
+{
+	struct olpc_ec_priv *ec = rdev_get_drvdata(rdev);
+
+	return olpc_ec_set_dcon_power(ec, 0);
+}
+
+static int dcon_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct olpc_ec_priv *ec = rdev_get_drvdata(rdev);
+
+	return ec->dcon_enabled;
+}
+
+static struct regulator_ops dcon_regulator_ops = {
+	.enable		= dcon_regulator_enable,
+	.disable	= dcon_regulator_disable,
+	.is_enabled	= dcon_regulator_is_enabled,
+};
+
+static const struct regulator_desc dcon_desc = {
+	.name	= "dcon",
+	.id	= 0,
+	.ops	= &dcon_regulator_ops,
+	.type	= REGULATOR_VOLTAGE,
+	.owner	= THIS_MODULE,
+};
+
 static int olpc_ec_probe(struct platform_device *pdev)
 {
 	struct olpc_ec_priv *ec;
+	struct regulator_config config = { };
 	int err;
 
 	if (!ec_driver)
@@ -378,6 +434,15 @@ static int olpc_ec_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	config.dev = pdev->dev.parent;
+	config.driver_data = ec;
+	ec->dcon_enabled = true;
+	ec->dcon_rdev = devm_regulator_register(&pdev->dev, &dcon_desc, &config);
+	if (IS_ERR(ec->dcon_rdev)) {
+		dev_err(&pdev->dev, "failed to register DCON regulator\n");
+		return PTR_ERR(ec->dcon_rdev);
+	}
+
 	ec->dbgfs_dir = olpc_ec_setup_debugfs();
 
 	return err;
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (8 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 09/15] Platform: OLPC: add a regulator for the DCON Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-17 19:38   ` Rob Herring
  2018-11-04 12:32   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
                   ` (7 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

The XO-1 and XO-1.5 batteries apparently differ in an ability to report
ambient temperature.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 Documentation/devicetree/bindings/power/supply/olpc_battery.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
index c8901b3992d9..8d87d6b35a98 100644
--- a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
@@ -2,4 +2,4 @@ OLPC battery
 ~~~~~~~~~~~~
 
 Required properties:
-  - compatible : "olpc,xo1-battery"
+  - compatible : "olpc,xo1-battery" or "olpc,xo1.5-battery"
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (9 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:43   ` Andy Shevchenko
  2018-11-04 12:34   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
                   ` (6 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

The XO-1 and XO-1.5 batteries apparently differ in an ability to report
ambient temperature. Add a different compatible string to the 1.5
battery.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/x86/platform/olpc/olpc_dt.c | 59 +++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index d6ee92986920..6e54e116b0c5 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -218,10 +218,28 @@ static u32 __init olpc_dt_get_board_revision(void)
 	return be32_to_cpu(rev);
 }
 
-void __init olpc_dt_fixup(void)
+int olpc_dt_compatible_match(phandle node, const char *compat)
 {
-	int r;
 	char buf[64];
+	int plen;
+	char *p;
+	int len;
+
+	plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
+	if (plen <= 0)
+		return 0;
+
+	len = strlen(compat);
+	for (p = buf; p < buf + plen; p += strlen(p) + 1) {
+		if (strcmp(p, compat) == 0)
+			return 1;
+	}
+
+	return 0;
+}
+
+void __init olpc_dt_fixup(void)
+{
 	phandle node;
 	u32 board_rev;
 
@@ -229,32 +247,33 @@ void __init olpc_dt_fixup(void)
 	if (!node)
 		return;
 
-	/*
-	 * If the battery node has a compatible property, we are running a new
-	 * enough firmware and don't have fixups to make.
-	 */
-	r = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
-	if (r > 0)
-		return;
-
-	pr_info("PROM DT: Old firmware detected, applying fixes\n");
-
-	/* Add olpc,xo1-battery compatible marker to battery node */
-	olpc_dt_interpret("\" /battery@0\" find-device"
-		" \" olpc,xo1-battery\" +compatible"
-		" device-end");
-
 	board_rev = olpc_dt_get_board_revision();
 	if (!board_rev)
 		return;
 
 	if (board_rev >= olpc_board_pre(0xd0)) {
+		if (olpc_dt_compatible_match(node, "olpc,xo1.5-battery"))
+			return;
+
+		/* Add olpc,xo1.5-battery compatible marker to battery node */
+		olpc_dt_interpret("\" /battery@0\" find-device"
+			" \" olpc,xo1.5-battery\" +compatible"
+			" device-end");
+
+		/* We're running a very old firmware if this is missing. */
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+			return;
+
 		/* XO-1.5: add dcon device */
 		olpc_dt_interpret("\" /pci/display@1\" find-device"
 			" new-device"
 			" \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
 			" finish-device device-end");
 	} else {
+		/* We're running a very old firmware if this is missing. */
+		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+			return;
+
 		/* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
 		olpc_dt_interpret("\" /pci/display@1,1\" find-device"
 			" new-device"
@@ -264,6 +283,11 @@ void __init olpc_dt_fixup(void)
 			" \" olpc,xo1-rtc\" +compatible"
 			" device-end");
 	}
+
+	/* Add olpc,xo1-battery compatible marker to battery node */
+	olpc_dt_interpret("\" /battery@0\" find-device"
+		" \" olpc,xo1-battery\" +compatible"
+		" device-end");
 }
 
 void __init olpc_dt_build_devicetree(void)
@@ -289,6 +313,7 @@ void __init olpc_dt_build_devicetree(void)
 /* A list of DT node/bus matches that we want to expose as platform devices */
 static struct of_device_id __initdata of_ids[] = {
 	{ .compatible = "olpc,xo1-battery" },
+	{ .compatible = "olpc,xo1.5-battery" },
 	{ .compatible = "olpc,xo1-dcon" },
 	{ .compatible = "olpc,xo1-rtc" },
 	{},
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (10 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:45   ` Andy Shevchenko
  2018-11-04 12:37   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
                   ` (5 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

Avoid using the x86 OLPC platform specific call to get the board
version. It won't work on FDT-based ARM MMP2 platform.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 5a97e42a3547..540d44bf536f 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -19,6 +19,7 @@
 #include <linux/jiffies.h>
 #include <linux/sched.h>
 #include <linux/olpc-ec.h>
+#include <linux/of.h>
 #include <asm/olpc.h>
 
 
@@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
 	if (IS_ERR(olpc_ac))
 		return PTR_ERR(olpc_ac);
-
-	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
+	if (of_property_match_string(pdev->dev.of_node, "compatible",
+					"olpc,xo1.5-battery") >= 0) {
+		/* XO-1.5 */
 		olpc_bat_desc.properties = olpc_xo15_bat_props;
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
-	} else { /* XO-1 */
+	} else {
+		/* XO-1 */
 		olpc_bat_desc.properties = olpc_xo1_bat_props;
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
@@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device *pdev)
 
 static const struct of_device_id olpc_battery_ids[] = {
 	{ .compatible = "olpc,xo1-battery" },
+	{ .compatible = "olpc,xo1.5-battery" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, olpc_battery_ids);
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (11 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:48   ` Andy Shevchenko
  2018-11-04 14:37   ` Pavel Machek
  2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
                   ` (4 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	linux-pm, Greg Kroah-Hartman, linux-kernel, Sebastian Reichel,
	Haojian Zhuang, linux-spi, Lubomir Rintel, Rob Herring,
	linux-arm-kernel, platform-driver-x86, Robert Jarzmik,
	Daniel Mack

The global variables for private data are not too nice. I'd like some
more, and that would clutter the global name space even further.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 73 +++++++++++++++--------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 540d44bf536f..2a2d7cc995f0 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -53,6 +53,12 @@
 
 #define BAT_ADDR_MFR_TYPE	0x5F
 
+struct olpc_battery_data {
+	struct power_supply *olpc_ac;
+	struct power_supply *olpc_bat;
+	char bat_serial[17];
+};
+
 /*********************************************************************
  *		Power
  *********************************************************************/
@@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
 	.get_property = olpc_ac_get_prop,
 };
 
-static struct power_supply *olpc_ac;
-
-static char bat_serial[17]; /* Ick */
-
-static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
+static int olpc_bat_get_status(struct olpc_battery_data *data,
+		union power_supply_propval *val, uint8_t ec_byte)
 {
 	if (olpc_platform_info.ecver > 0x44) {
 		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
@@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 				 enum power_supply_property psp,
 				 union power_supply_propval *val)
 {
+	struct olpc_battery_data *data = power_supply_get_drvdata(psy);
 	int ret = 0;
 	__be16 ec_word;
 	uint8_t ec_byte;
@@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		ret = olpc_bat_get_status(val, ec_byte);
+		ret = olpc_bat_get_status(data, val, ec_byte);
 		if (ret)
 			return ret;
 		break;
@@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		sprintf(bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
-		val->strval = bat_serial;
+		sprintf(data->bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
+		val->strval = data->bat_serial;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
 		ret = olpc_bat_get_voltage_max_design(val);
@@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
 	.use_for_apm = 1,
 };
 
-static struct power_supply *olpc_bat;
-
 static int olpc_battery_suspend(struct platform_device *pdev,
 				pm_message_t state)
 {
-	if (device_may_wakeup(&olpc_ac->dev))
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&data->olpc_ac->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
 	else
 		olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
 
-	if (device_may_wakeup(&olpc_bat->dev))
+	if (device_may_wakeup(&data->olpc_bat->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
 				   | EC_SCI_SRC_BATERR);
 	else
@@ -601,7 +605,8 @@ static int olpc_battery_suspend(struct platform_device *pdev,
 
 static int olpc_battery_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct power_supply_config psy_cfg = {};
+	struct olpc_battery_data *data;
 	uint8_t status;
 
 	/*
@@ -620,9 +625,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
 
 	/* Ignore the status. It doesn't actually matter */
 
-	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
-	if (IS_ERR(olpc_ac))
-		return PTR_ERR(olpc_ac);
+	psy_cfg.of_node = pdev->dev.of_node;
+	psy_cfg.drv_data = data;
+
+	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_ac))
+		return PTR_ERR(data->olpc_ac);
+
 	if (of_property_match_string(pdev->dev.of_node, "compatible",
 					"olpc,xo1.5-battery") >= 0) {
 		/* XO-1.5 */
@@ -634,42 +643,36 @@ static int olpc_battery_probe(struct platform_device *pdev)
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
-	if (IS_ERR(olpc_bat)) {
-		ret = PTR_ERR(olpc_bat);
-		goto battery_failed;
-	}
+	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_bat))
+		return PTR_ERR(data->olpc_bat);
 
-	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
+	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	if (ret)
-		goto eeprom_failed;
+		return ret;
 
-	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
+	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
 	if (ret)
 		goto error_failed;
 
 	if (olpc_ec_wakeup_available()) {
-		device_set_wakeup_capable(&olpc_ac->dev, true);
-		device_set_wakeup_capable(&olpc_bat->dev, true);
+		device_set_wakeup_capable(&data->olpc_ac->dev, true);
+		device_set_wakeup_capable(&data->olpc_bat->dev, true);
 	}
 
 	return 0;
 
 error_failed:
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
-eeprom_failed:
-	power_supply_unregister(olpc_bat);
-battery_failed:
-	power_supply_unregister(olpc_ac);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	return ret;
 }
 
 static int olpc_battery_remove(struct platform_device *pdev)
 {
-	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
-	power_supply_unregister(olpc_bat);
-	power_supply_unregister(olpc_ac);
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	return 0;
 }
 
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (12 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
@ 2018-10-10 17:22 ` Lubomir Rintel
  2018-10-19 13:50   ` Andy Shevchenko
  2018-11-04 14:39   ` Pavel Machek
  2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
                   ` (3 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:22 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

This wouldn't work on the DT-based ARM platform. Let's read the EC version
directly from the EC driver instead.

This makes the driver no longer x86 specific.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/Kconfig        |  2 +-
 drivers/power/supply/olpc_battery.c | 35 +++++++++++++++++++++--------
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index ff6dab0bf0dd..f0361e4dd457 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -151,7 +151,7 @@ config BATTERY_PMU
 
 config BATTERY_OLPC
 	tristate "One Laptop Per Child battery"
-	depends on X86_32 && OLPC
+	depends on OLPC
 	help
 	  Say Y to enable support for the battery on the OLPC laptop.
 
diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 2a2d7cc995f0..dde9863e5c4a 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -20,8 +20,6 @@
 #include <linux/sched.h>
 #include <linux/olpc-ec.h>
 #include <linux/of.h>
-#include <asm/olpc.h>
-
 
 #define EC_BAT_VOLTAGE	0x10	/* uint16_t,	*9.76/32,    mV   */
 #define EC_BAT_CURRENT	0x11	/* int16_t,	*15.625/120, mA   */
@@ -57,6 +55,7 @@ struct olpc_battery_data {
 	struct power_supply *olpc_ac;
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
+	int new_proto;
 };
 
 /*********************************************************************
@@ -100,7 +99,7 @@ static const struct power_supply_desc olpc_ac_desc = {
 static int olpc_bat_get_status(struct olpc_battery_data *data,
 		union power_supply_propval *val, uint8_t ec_byte)
 {
-	if (olpc_platform_info.ecver > 0x44) {
+	if (data->new_proto) {
 		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
 			val->intval = POWER_SUPPLY_STATUS_CHARGING;
 		else if (ec_byte & BAT_STAT_DISCHARGING)
@@ -608,14 +607,32 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	struct power_supply_config psy_cfg = {};
 	struct olpc_battery_data *data;
 	uint8_t status;
+	unsigned char ecver[1];
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, data);
+
+	/* See if the EC is already there and get the EC revision */
+	ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, ecver, ARRAY_SIZE(ecver));
+	if (ret) {
+		if (ret == -ENODEV)
+			return -EPROBE_DEFER;
+		return ret;
+	}
 
-	/*
-	 * We've seen a number of EC protocol changes; this driver requires
-	 * the latest EC protocol, supported by 0x44 and above.
-	 */
-	if (olpc_platform_info.ecver < 0x44) {
+	if (ecver[0] > 0x44) {
+		/* XO 1 or 1.5 with a new EC firmware. */
+		data->new_proto = 1;
+	} else if (ecver[0] < 0x44) {
+		/*
+		 * We've seen a number of EC protocol changes; this driver
+		 * requires the latest EC protocol, supported by 0x44 and above.
+		 */
 		printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
-			"battery driver.\n", olpc_platform_info.ecver);
+			"battery driver.\n", ecver[0]);
 		return -ENXIO;
 	}
 
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (13 preceding siblings ...)
  2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
@ 2018-10-10 17:23 ` Lubomir Rintel
  2018-10-19 13:55   ` Andy Shevchenko
  2018-11-04 14:39   ` Pavel Machek
  2018-10-10 19:26 ` [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Rob Herring
                   ` (2 subsequent siblings)
  17 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-10 17:23 UTC (permalink / raw)
  To: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko
  Cc: Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm,
	Lubomir Rintel

The battery and the protocol are essentially the same as OLPC XO 1.5,
but the responses from the EC are LSB first.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index dde9863e5c4a..2adf33b9f641 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -56,6 +56,7 @@ struct olpc_battery_data {
 	struct power_supply *olpc_bat;
 	char bat_serial[17];
 	int new_proto;
+	int little_endian;
 };
 
 /*********************************************************************
@@ -321,6 +322,14 @@ static int olpc_bat_get_voltage_max_design(union power_supply_propval *val)
 	return ret;
 }
 
+static s16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_byte)
+{
+	if (data->little_endian)
+		return le16_to_cpu(ec_byte);
+	else
+		return be16_to_cpu(ec_byte);
+}
+
 /*********************************************************************
  *		Battery properties
  *********************************************************************/
@@ -393,7 +402,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
+		val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
@@ -401,7 +410,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
+		val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
@@ -432,21 +441,21 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
+		val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
 		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
+		val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
 		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
+		val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
 		break;
 	case POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
@@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device *pdev)
 	if (ecver[0] > 0x44) {
 		/* XO 1 or 1.5 with a new EC firmware. */
 		data->new_proto = 1;
+	} else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
+		/* XO 1.75 */
+		data->new_proto = 1;
+		data->little_endian = 1;
 	} else if (ecver[0] < 0x44) {
 		/*
 		 * We've seen a number of EC protocol changes; this driver
-- 
2.19.0

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (14 preceding siblings ...)
  2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
@ 2018-10-10 19:26 ` Rob Herring
  2018-10-11 10:42   ` Lubomir Rintel
  2018-10-19 13:57 ` Andy Shevchenko
  2018-10-21 21:45 ` Sebastian Reichel
  17 siblings, 1 reply; 62+ messages in thread
From: Rob Herring @ 2018-10-10 19:26 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	platform-driver-x86, devel

On Wed, Oct 10, 2018 at 12:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> Hi.
>
> This patchset adds support for the Embedded Controller on an OLPC XO
> 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> the existing OLPC platform infrastructure, currently used by the x86
> based models.
>
> The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> uses extra handshake signal to signal readiness of SOC to accept data
> from EC and to initiate a transaction if SOC wishes to submit data.
>
> The SPI slave support for MMP2 was submitted separately:
> https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t
>
> THe "power: supply: olpc_battery: correct the temperature" patch was
> already sent out separately, but I'm including it because the last
> commit of the set depends on it.
>
> Tested to work on an OLPC XO 1.75 and also tested not to break x86
> support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> unlikely this breaks it when XO 1 works.

I asked this on the OLPC devel list recently, but I don't think my
message ever got past the moderator. Could you generate a DT dump from
/proc/device-tree of an XO 1 and send to me? I have some DT changes
planned and need to see if they'd be okay for x86 OLPC.

Rob

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
  2018-10-10 19:26 ` [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Rob Herring
@ 2018-10-11 10:42   ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-11 10:42 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, open list:THERMAL, Greg Kroah-Hartman,
	linux-kernel, Sebastian Reichel, Haojian Zhuang, linux-spi,
	Mark Brown, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Wed, 2018-10-10 at 14:26 -0500, Rob Herring wrote:
> On Wed, Oct 10, 2018 at 12:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
> > Hi.
> > 
> > This patchset adds support for the Embedded Controller on an OLPC XO
> > 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> > the existing OLPC platform infrastructure, currently used by the x86
> > based models.
> > 
> > The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> > uses extra handshake signal to signal readiness of SOC to accept data
> > from EC and to initiate a transaction if SOC wishes to submit data.
> > 
> > The SPI slave support for MMP2 was submitted separately:
> > https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t
> > 
> > THe "power: supply: olpc_battery: correct the temperature" patch was
> > already sent out separately, but I'm including it because the last
> > commit of the set depends on it.
> > 
> > Tested to work on an OLPC XO 1.75 and also tested not to break x86
> > support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> > unlikely this breaks it when XO 1 works.
> 
> I asked this on the OLPC devel list recently, but I don't think my
> message ever got past the moderator. Could you generate a DT dump from
> /proc/device-tree of an XO 1 and send to me? I have some DT changes
> planned and need to see if they'd be okay for x86 OLPC.

The /proc/device-tree tarball: http://v3.sk/~lkundrak/olpc/xo1.tar
(Mirror: https://people.freedesktop.org/~lkundrak/olpc/xo1.tar)

My distro's dtc crashes with this (could be related to your recent dtc
fixes), the git tip needs -f to work around some funny property names.
I figure a raw data as opposed to a dts/dtb dump would be a better
idea.

Also, there's no phandles, so it's of rather limited use. If you need
those, then I can share a dump directly from ofw instead or patch the
kernel to fabricate and expose the phandle properties.

This is with the latest firmware. I guess the exact version is
somewhere within the device tree.

> 
> Rob

Cheers
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings
  2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
@ 2018-10-17 19:38   ` Rob Herring
  2018-11-04 12:26   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Rob Herring @ 2018-10-17 19:38 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

On Wed, Oct 10, 2018 at 07:22:48PM +0200, Lubomir Rintel wrote:
> The OLPC XO-1.75 Embedded Controller is a SPI master that uses extra
> signals for handshaking. It needs to know when is the slave (Linux)
> side's TX FIFO ready for transfer (the ready-gpio signal on the SPI
> controller node) and when does it wish to respond with a command (the
> cmd-gpio property).
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  .../bindings/misc/olpc,xo1.75-ec.txt          | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> new file mode 100644
> index 000000000000..14385fee65d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> @@ -0,0 +1,24 @@
> +OLPC XO-1.75 Embedded Controller
> +
> +Required properties:
> +- compatible: Should be "olpc,xo1.75-ec".
> +- cmd-gpio: gpio specifier of the CMD pin

cmd-gpios

> +
> +The embedded controller requires the SPI controller driver to signal readiness
> +to receive a transfer (that is, when TX FIFO contains the response data) by
> +strobing the ACK pin with the ready signal. See the "ready-gpio" property of the

This will need updating based on the SPI binding.

> +SSP binding as documented in:
> +<Documentation/devicetree/bindings/spi/spi-pxa2xx.txt>.
> +
> +Example:
> +	&ssp3 {
> +		spi-slave;
> +		status = "okay";

Don't show status in examples.

> +		ready-gpio = <&gpio 125 GPIO_ACTIVE_HIGH>;
> +
> +		slave {
> +			compatible = "olpc,xo1.75-ec";
> +			spi-cpha;
> +			cmd-gpio = <&gpio 155 GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> -- 
> 2.19.0
> 

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery
  2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
@ 2018-10-17 19:38   ` Rob Herring
  2018-11-04 12:32   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Rob Herring @ 2018-10-17 19:38 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Sebastian Reichel, Haojian Zhuang, linux-spi, Lubomir Rintel,
	Mark Brown, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko, linux-arm-kernel

On Wed, 10 Oct 2018 19:22:55 +0200, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  Documentation/devicetree/bindings/power/supply/olpc_battery.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units
  2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
@ 2018-10-19 13:00   ` Andy Shevchenko
  2018-10-21 21:27     ` Sebastian Reichel
  2018-11-02 22:16   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:00 UTC (permalink / raw)
  To: Lubomir Rintel, David Woodhouse
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
>
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>       temperature:         236.9 degrees C
>
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.

It's interesting that the very author of that code is not included in
so-o long Cc list :)
Cc: David.

David, do you remember if and how you had tested temperature report of
the battery on OLPC?
I guess this kind of error would be appear immediately.

OTOH it might be that power framework had changed requirements (which
would be noticeable change).
If the latter is true, this patch misses Fixes tag. Actually in any
case it misses it.

>
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> +               val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>                 ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>                 if (ret)
>                         return ret;
>
> -               val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> +               val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>                 ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"
  2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
@ 2018-10-19 13:04   ` Andy Shevchenko
  2018-11-02 22:16   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:04 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> It doesn't make sense to always have this built-in, e.g. on ARM
> multiplatform kernels. A better way to address the problem the original
> commit aimed to solve is to fix Kconfig.
>
> This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.
>

This change doesn't make any sense when put in _this_ order in the series.

First, you need to show the CONFIG_OLPC as tristate, which doesn't (Am
I missing something?).

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/platform/olpc/olpc-ec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 374a8028fec7..f99b183d5296 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -1,8 +1,6 @@
>  /*
>   * Generic driver for the OLPC Embedded Controller.
>   *
> - * Author: Andres Salomon <dilinger@queued.net>
> - *
>   * Copyright (C) 2011-2012 One Laptop per Child Foundation.
>   *
>   * Licensed under the GPL v2 or later.
> @@ -14,7 +12,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/workqueue.h>
> -#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/list.h>
>  #include <linux/olpc-ec.h>
>  #include <asm/olpc.h>
> @@ -328,4 +326,8 @@ static int __init olpc_ec_init_module(void)
>  {
>         return platform_driver_register(&olpc_ec_plat_driver);
>  }
> +
>  arch_initcall(olpc_ec_init_module);
> +
> +MODULE_AUTHOR("Andres Salomon <dilinger@queued.net>");
> +MODULE_LICENSE("GPL");
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 04/15] Platform: OLPC: Remove an unused include
  2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
@ 2018-10-19 13:05   ` Andy Shevchenko
  2018-11-13 13:59     ` Lubomir Rintel
  2018-11-04 12:26   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:05 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> Also, the header is x86 specific, while there are non-x86 OLPC machines.

Same concern. as per patch 2.

Also, you might want to sort headers in alphabetical order.

>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/platform/olpc/olpc-ec.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index f99b183d5296..35a21c66cd0d 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -15,7 +15,6 @@
>  #include <linux/module.h>
>  #include <linux/list.h>
>  #include <linux/olpc-ec.h>
> -#include <asm/olpc.h>
>
>  struct ec_cmd_desc {
>         u8 cmd;
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree
  2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
@ 2018-10-19 13:09   ` Andy Shevchenko
  2018-11-04 12:27   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:09 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> There are ARM OLPC machines that use mostly the same drivers, including
> EC infrastructure, DCON and Battery.
>
> While at that, fix Kconfig to allow building this as a module.


> -       depends on MOUSE_PS2 && OLPC
> +       depends on MOUSE_PS2 && X86 && OLPC


> -       depends on OLPC && FB
> +       depends on X86 && OLPC && FB

Looking to this I would rather introduce an idiom

depends on OLPC && X86

on the opposite you may do similar for ARM

depends on OLPC && ARM // or ARM64 or whatever it's called

thus, above would look like

depends on MOUSE_PS2
depends on OLPC && X86

and

depends on FB
depends on OLPC && X86

respectively.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet
  2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
@ 2018-10-19 13:11   ` Andy Shevchenko
  2018-11-04 12:30   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:11 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> Just return ENODEV, so that whoever attempted to use the EC call can
> defer their work.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/platform/olpc/olpc-ec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 35a21c66cd0d..342f5bb7f7a8 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf, size_t outlen)
>         struct olpc_ec_priv *ec = ec_priv;
>         struct ec_cmd_desc desc;
>
> -       /* Ensure a driver and ec hook have been registered */
> -       if (WARN_ON(!ec_driver || !ec_driver->ec_cmd))
> +       /* Driver not yet registered. */
> +       if (!ec_driver)
> +               return -ENODEV;

Why -ENODEV is preferred over -EPROBE_DEFER?

> +
> +       if (WARN_ON(!ec_driver->ec_cmd))
>                 return -ENODEV;
>
>         if (!ec)
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86
  2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
@ 2018-10-19 13:36   ` Andy Shevchenko
  2018-11-14 16:20     ` Lubomir Rintel
  2018-11-04 12:31   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:36 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> It is actually plaform independent. Move it to the olpc-ec driver from
> the X86 OLPC platform, so that it could be used by the ARM based laptops
> too.

What is platform independent exactly?

>  #define OLPC_F_PRESENT         0x01
>  #define OLPC_F_DCON            0x02
> -#define OLPC_F_EC_WIDE_SCI     0x04

I think these lines grouped on purpose. Thus, I don't like this.
Either move either all or none.

> +int olpc_ec_mask_write(u16 bits)
> +{
>  #ifdef CONFIG_DEBUG_FS
>
>  /*
> @@ -277,14 +369,17 @@ static int olpc_ec_probe(struct platform_device *pdev)
>         ec_priv = ec;
>         platform_set_drvdata(pdev, ec);
>

> +       /* EC version 0x5f adds support for wide SCI mask */
> +       if (ec->version >= 0x5f) {
> +               __be16 ec_word = cpu_to_be16(bits);
> +

> +               return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *) &ec_word, 2,
> +                                  NULL, 0);

I would leave it on one line.

> +       } else {

> +               unsigned char ec_byte = bits & 0xff;

You may noticed that the parameter is of type u8, which clearly makes
& 0xff part redundant.

> +               return olpc_ec_cmd(EC_WRITE_SCI_MASK, &ec_byte, 1, NULL, 0);
> +       }
> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_mask_write);

I see that the above is inherited from older code, so, no need to
address those comments in here, but consider a follow up fix.


> +int olpc_ec_sci_query(u16 *sci_value)
> +{

> +}
> +EXPORT_SYMBOL_GPL(olpc_ec_sci_query);

Similar comments are applied here.

> +
> -       err = ec_driver->probe ? ec_driver->probe(pdev) : 0;
> +       /* get the EC revision */
> +       err = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
> +               (unsigned char *) &ec->version, 1);

Perhaps version should be defined as u8.

> +/* SCI source values */
> +#define EC_SCI_SRC_EMPTY        0x00
> +#define EC_SCI_SRC_GAME         0x01
> +#define EC_SCI_SRC_BATTERY      0x02
> +#define EC_SCI_SRC_BATSOC       0x04
> +#define EC_SCI_SRC_BATERR       0x08
> +#define EC_SCI_SRC_EBOOK        0x10    /* XO-1 only */
> +#define EC_SCI_SRC_WLAN         0x20    /* XO-1 only */
> +#define EC_SCI_SRC_ACPWR        0x40
> +#define EC_SCI_SRC_BATCRIT      0x80
> +#define EC_SCI_SRC_GPWAKE       0x100   /* XO-1.5 only */

BIT() ?

> +#define EC_SCI_SRC_ALL          0x1FF

GENMASK()?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 09/15] Platform: OLPC: add a regulator for the DCON
  2018-10-10 17:22 ` [PATCH 09/15] Platform: OLPC: add a regulator for the DCON Lubomir Rintel
@ 2018-10-19 13:39   ` Andy Shevchenko
  0 siblings, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:39 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> All OLPC ECs are able to turn the power to the DCON on an off. Use the
> regulator framework to expose the functionality.

> +static int olpc_ec_set_dcon_power(struct olpc_ec_priv *ec, bool state)
> +{
> +       unsigned char ec_byte = state;
> +       int ret;
> +
> +       if (ec->dcon_enabled == state)
> +               return 0;
> +

> +       ret = olpc_ec_cmd(EC_DCON_POWER_MODE, &ec_byte, 1, NULL, 0);
> +       if (ret == 0)
> +               ec->dcon_enabled = state;
> +
> +       return ret;

I would prefer to see usual pattern, i.e.

if (ret)
 return ret;

...
return 0;

This comment applies to entire series. (Yes, you might address an old
code in a separate patch to be on consistent side)

> +}

> +       return olpc_ec_set_dcon_power(ec, 1);

defined as boolean, supplied an int. Not good.

> +       return olpc_ec_set_dcon_power(ec, 0);

Ditto.

> +static int dcon_regulator_is_enabled(struct regulator_dev *rdev)
> +       return ec->dcon_enabled;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node
  2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
@ 2018-10-19 13:43   ` Andy Shevchenko
  2018-11-14 16:49     ` Lubomir Rintel
  2018-11-04 12:34   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:43 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.

> +int olpc_dt_compatible_match(phandle node, const char *compat)
>  {
>         char buf[64];
> +       int plen;
> +       char *p;
> +       int len;
> +
> +       plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> +       if (plen <= 0)
> +               return 0;
> +
> +       len = strlen(compat);
> +       for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> +               if (strcmp(p, compat) == 0)
> +                       return 1;
> +       }
> +
> +       return 0;
> +}

DT doesn't still have a helper for that?!
I hardly believe in that.

> +               olpc_dt_interpret("\" /battery@0\" find-device"
> +                       " \" olpc,xo1.5-battery\" +compatible"
> +                       " device-end");

Please, don't split string literals.

>                 olpc_dt_interpret("\" /pci/display@1\" find-device"
>                         " new-device"
>                         " \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
>                         " finish-device device-end");

Yeah, this and similar also needs to be fixed.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
  2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
@ 2018-10-19 13:45   ` Andy Shevchenko
  2018-11-15 18:33     ` Lubomir Rintel
  2018-11-04 12:37   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:45 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, quozl,
	Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sebastian Reichel, Rob Herring,
	linux-spi, Mark Brown, Haojian Zhuang, Daniel Mack, Darren Hart,
	Platform Driver, Robert Jarzmik, Andy Shevchenko,
	linux-arm Mailing List

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..540d44bf536f 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -19,6 +19,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/sched.h>
>  #include <linux/olpc-ec.h>
> +#include <linux/of.h>
>  #include <asm/olpc.h>

Keep it sorted, otherwise the change is good!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
>
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
>         olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
>         if (IS_ERR(olpc_ac))
>                 return PTR_ERR(olpc_ac);
> -
> -       if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> +       if (of_property_match_string(pdev->dev.of_node, "compatible",
> +                                       "olpc,xo1.5-battery") >= 0) {
> +               /* XO-1.5 */
>                 olpc_bat_desc.properties = olpc_xo15_bat_props;
>                 olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> -       } else { /* XO-1 */
> +       } else {
> +               /* XO-1 */
>                 olpc_bat_desc.properties = olpc_xo1_bat_props;
>                 olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>         }
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device *pdev)
>
>  static const struct of_device_id olpc_battery_ids[] = {
>         { .compatible = "olpc,xo1-battery" },
> +       { .compatible = "olpc,xo1.5-battery" },
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct
  2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
@ 2018-10-19 13:48   ` Andy Shevchenko
  2018-11-04 14:37   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:48 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, quozl,
	Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sebastian Reichel, Rob Herring,
	linux-spi, Mark Brown, Haojian Zhuang, Daniel Mack, Darren Hart,
	Platform Driver, Robert Jarzmik, Andy Shevchenko,
	linux-arm Mailing List

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
>

Good change!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 73 +++++++++++++++--------------
>  1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 540d44bf536f..2a2d7cc995f0 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -53,6 +53,12 @@
>
>  #define BAT_ADDR_MFR_TYPE      0x5F
>
> +struct olpc_battery_data {
> +       struct power_supply *olpc_ac;
> +       struct power_supply *olpc_bat;
> +       char bat_serial[17];
> +};
> +
>  /*********************************************************************
>   *             Power
>   *********************************************************************/
> @@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
>         .get_property = olpc_ac_get_prop,
>  };
>
> -static struct power_supply *olpc_ac;
> -
> -static char bat_serial[17]; /* Ick */
> -
> -static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
> +static int olpc_bat_get_status(struct olpc_battery_data *data,
> +               union power_supply_propval *val, uint8_t ec_byte)
>  {
>         if (olpc_platform_info.ecver > 0x44) {
>                 if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> @@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                                  enum power_supply_property psp,
>                                  union power_supply_propval *val)
>  {
> +       struct olpc_battery_data *data = power_supply_get_drvdata(psy);
>         int ret = 0;
>         __be16 ec_word;
>         uint8_t ec_byte;
> @@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> -               ret = olpc_bat_get_status(val, ec_byte);
> +               ret = olpc_bat_get_status(data, val, ec_byte);
>                 if (ret)
>                         return ret;
>                 break;
> @@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               sprintf(bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
> -               val->strval = bat_serial;
> +               sprintf(data->bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
> +               val->strval = data->bat_serial;
>                 break;
>         case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>                 ret = olpc_bat_get_voltage_max_design(val);
> @@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
>         .use_for_apm = 1,
>  };
>
> -static struct power_supply *olpc_bat;
> -
>  static int olpc_battery_suspend(struct platform_device *pdev,
>                                 pm_message_t state)
>  {
> -       if (device_may_wakeup(&olpc_ac->dev))
> +       struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +       if (device_may_wakeup(&data->olpc_ac->dev))
>                 olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
>         else
>                 olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
>
> -       if (device_may_wakeup(&olpc_bat->dev))
> +       if (device_may_wakeup(&data->olpc_bat->dev))
>                 olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
>                                    | EC_SCI_SRC_BATERR);
>         else
> @@ -601,7 +605,8 @@ static int olpc_battery_suspend(struct platform_device *pdev,
>
>  static int olpc_battery_probe(struct platform_device *pdev)
>  {
> -       int ret;
> +       struct power_supply_config psy_cfg = {};
> +       struct olpc_battery_data *data;
>         uint8_t status;
>
>         /*
> @@ -620,9 +625,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
>
>         /* Ignore the status. It doesn't actually matter */
>
> -       olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
> -       if (IS_ERR(olpc_ac))
> -               return PTR_ERR(olpc_ac);
> +       psy_cfg.of_node = pdev->dev.of_node;
> +       psy_cfg.drv_data = data;
> +
> +       data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
> +       if (IS_ERR(data->olpc_ac))
> +               return PTR_ERR(data->olpc_ac);
> +
>         if (of_property_match_string(pdev->dev.of_node, "compatible",
>                                         "olpc,xo1.5-battery") >= 0) {
>                 /* XO-1.5 */
> @@ -634,42 +643,36 @@ static int olpc_battery_probe(struct platform_device *pdev)
>                 olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>         }
>
> -       olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> -       if (IS_ERR(olpc_bat)) {
> -               ret = PTR_ERR(olpc_bat);
> -               goto battery_failed;
> -       }
> +       data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> +       if (IS_ERR(data->olpc_bat))
> +               return PTR_ERR(data->olpc_bat);
>
> -       ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> +       ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         if (ret)
> -               goto eeprom_failed;
> +               return ret;
>
> -       ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> +       ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
>         if (ret)
>                 goto error_failed;
>
>         if (olpc_ec_wakeup_available()) {
> -               device_set_wakeup_capable(&olpc_ac->dev, true);
> -               device_set_wakeup_capable(&olpc_bat->dev, true);
> +               device_set_wakeup_capable(&data->olpc_ac->dev, true);
> +               device_set_wakeup_capable(&data->olpc_bat->dev, true);
>         }
>
>         return 0;
>
>  error_failed:
> -       device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -eeprom_failed:
> -       power_supply_unregister(olpc_bat);
> -battery_failed:
> -       power_supply_unregister(olpc_ac);
> +       device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         return ret;
>  }
>
>  static int olpc_battery_remove(struct platform_device *pdev)
>  {
> -       device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> -       device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -       power_supply_unregister(olpc_bat);
> -       power_supply_unregister(olpc_ac);
> +       struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +       device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> +       device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         return 0;
>  }
>
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info
  2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
@ 2018-10-19 13:50   ` Andy Shevchenko
  2018-11-14 17:19     ` Lubomir Rintel
  2018-11-04 14:39   ` Pavel Machek
  1 sibling, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:50 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
>
> This makes the driver no longer x86 specific.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/Kconfig        |  2 +-
>  drivers/power/supply/olpc_battery.c | 35 +++++++++++++++++++++--------
>  2 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index ff6dab0bf0dd..f0361e4dd457 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -151,7 +151,7 @@ config BATTERY_PMU
>
>  config BATTERY_OLPC
>         tristate "One Laptop Per Child battery"
> -       depends on X86_32 && OLPC
> +       depends on OLPC
>         help
>           Say Y to enable support for the battery on the OLPC laptop.
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 2a2d7cc995f0..dde9863e5c4a 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -20,8 +20,6 @@
>  #include <linux/sched.h>
>  #include <linux/olpc-ec.h>

>  #include <linux/of.h>

Btw, Kconfig might miss
depends on OF
part.

> -#include <asm/olpc.h>
> -
>
>  #define EC_BAT_VOLTAGE 0x10    /* uint16_t,    *9.76/32,    mV   */
>  #define EC_BAT_CURRENT 0x11    /* int16_t,     *15.625/120, mA   */
> @@ -57,6 +55,7 @@ struct olpc_battery_data {
>         struct power_supply *olpc_ac;
>         struct power_supply *olpc_bat;
>         char bat_serial[17];
> +       int new_proto;
>  };
>
>  /*********************************************************************
> @@ -100,7 +99,7 @@ static const struct power_supply_desc olpc_ac_desc = {
>  static int olpc_bat_get_status(struct olpc_battery_data *data,
>                 union power_supply_propval *val, uint8_t ec_byte)
>  {
> -       if (olpc_platform_info.ecver > 0x44) {
> +       if (data->new_proto) {
>                 if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
>                         val->intval = POWER_SUPPLY_STATUS_CHARGING;
>                 else if (ec_byte & BAT_STAT_DISCHARGING)
> @@ -608,14 +607,32 @@ static int olpc_battery_probe(struct platform_device *pdev)
>         struct power_supply_config psy_cfg = {};
>         struct olpc_battery_data *data;
>         uint8_t status;

> +       unsigned char ecver[1];

isn't it simple
 uint8_t ecver;
?

> +       int ret;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +       platform_set_drvdata(pdev, data);
> +
> +       /* See if the EC is already there and get the EC revision */
> +       ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, ecver, ARRAY_SIZE(ecver));

> +       if (ret) {

> +               if (ret == -ENODEV)
> +                       return -EPROBE_DEFER;

Yeah, exactly a question I asked somewhere in the first part of the series.

> +               return ret;
> +       }
>
> -       /*
> -        * We've seen a number of EC protocol changes; this driver requires
> -        * the latest EC protocol, supported by 0x44 and above.
> -        */
> -       if (olpc_platform_info.ecver < 0x44) {
> +       if (ecver[0] > 0x44) {
> +               /* XO 1 or 1.5 with a new EC firmware. */
> +               data->new_proto = 1;
> +       } else if (ecver[0] < 0x44) {
> +               /*
> +                * We've seen a number of EC protocol changes; this driver
> +                * requires the latest EC protocol, supported by 0x44 and above.
> +                */
>                 printk(KERN_NOTICE "OLPC EC version 0x%02x too old for "
> -                       "battery driver.\n", olpc_platform_info.ecver);
> +                       "battery driver.\n", ecver[0]);
>                 return -ENXIO;
>         }
>
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support
  2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
@ 2018-10-19 13:55   ` Andy Shevchenko
  2018-11-04 14:39   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:55 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index dde9863e5c4a..2adf33b9f641 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -56,6 +56,7 @@ struct olpc_battery_data {
>         struct power_supply *olpc_bat;
>         char bat_serial[17];
>         int new_proto;
> +       int little_endian;
>  };
>
>  /*********************************************************************
> @@ -321,6 +322,14 @@ static int olpc_bat_get_voltage_max_design(union power_supply_propval *val)
>         return ret;
>  }
>
> +static s16 ecword_to_cpu(struct olpc_battery_data *data, u16 ec_byte)

ec_byte is misleading name. It's a 16-bit word. and since we called in
I/O accessor it as word, you may do the similar here.

And why result is s16 and not u16?

> +{
> +       if (data->little_endian)
> +               return le16_to_cpu(ec_byte);
> +       else
> +               return be16_to_cpu(ec_byte);

> +
>  /*********************************************************************
>   *             Battery properties
>   *********************************************************************/
> @@ -393,7 +402,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 9760L / 32;
> +               val->intval = ecword_to_cpu(data, ec_word) * 9760L / 32;
>                 break;
>         case POWER_SUPPLY_PROP_CURRENT_AVG:
>         case POWER_SUPPLY_PROP_CURRENT_NOW:
> @@ -401,7 +410,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 15625L / 120;
> +               val->intval = ecword_to_cpu(data, ec_word) * 15625L / 120;
>                 break;
>         case POWER_SUPPLY_PROP_CAPACITY:
>                 ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
> @@ -432,21 +441,21 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> +               val->intval = ecword_to_cpu(data, ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>                 ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>                 if (ret)
>                         return ret;
>
> -               val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> +               val->intval = (int)ecword_to_cpu(data, ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>                 ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 6250 / 15;
> +               val->intval = ecword_to_cpu(data, ec_word) * 6250 / 15;
>                 break;
>         case POWER_SUPPLY_PROP_SERIAL_NUMBER:
>                 ret = olpc_ec_cmd(EC_BAT_SERIAL, NULL, 0, (void *)&ser_buf, 8);
> @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device *pdev)
>         if (ecver[0] > 0x44) {
>                 /* XO 1 or 1.5 with a new EC firmware. */
>                 data->new_proto = 1;
> +       } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
> +               /* XO 1.75 */
> +               data->new_proto = 1;
> +               data->little_endian = 1;
>         } else if (ecver[0] < 0x44) {
>                 /*
>                  * We've seen a number of EC protocol changes; this driver
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (15 preceding siblings ...)
  2018-10-10 19:26 ` [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Rob Herring
@ 2018-10-19 13:57 ` Andy Shevchenko
  2018-10-23 17:03   ` Lubomir Rintel
  2018-10-21 21:45 ` Sebastian Reichel
  17 siblings, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 13:57 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> Hi.
>
> This patchset adds support for the Embedded Controller on an OLPC XO
> 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> the existing OLPC platform infrastructure, currently used by the x86
> based models.
>
> The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> uses extra handshake signal to signal readiness of SOC to accept data
> from EC and to initiate a transaction if SOC wishes to submit data.
>
> The SPI slave support for MMP2 was submitted separately:
> https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t
>

> THe "power: supply: olpc_battery: correct the temperature" patch was

The

> already sent out separately, but I'm including it because the last
> commit of the set depends on it.
>
> Tested to work on an OLPC XO 1.75 and also tested not to break x86
> support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> unlikely this breaks it when XO 1 works.
>
> Thanks in advance for reviews and feedback of any kind.

Thanks for the series.
I'm about to review the patch 6, otherwise read my comments for the
rest and consider addressing them.

>
> Lubo
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-10-10 17:22 ` [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver Lubomir Rintel
@ 2018-10-19 16:06   ` Andy Shevchenko
  2018-11-13 17:26     ` Lubomir Rintel
  0 siblings, 1 reply; 62+ messages in thread
From: Andy Shevchenko @ 2018-10-19 16:06 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> It's based off the driver from the OLPC kernel sources. Somewhat
> modernized and cleaned up, for better or worse.
>
> Modified to plug into the olpc-ec driver infrastructure (so that battery
> interface and debugfs could be reused) and the SPI slave framework.


> +#include <asm/system_misc.h>

asm/* goes after linux/*

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/spinlock.h>
> +#include <linux/completion.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/ctype.h>
> +#include <linux/olpc-ec.h>
> +#include <linux/spi/spi.h>
> +#include <linux/reboot.h>
> +#include <linux/input.h>
> +#include <linux/kfifo.h>
> +#include <linux/module.h>
> +#include <linux/power_supply.h>

Easy to maintain when it's sorted.

> +       { 0 },

Terminators are better without trailing comma.

> +#define EC_CMD_LEN             8
> +#define EC_MAX_RESP_LEN                16

> +#define LOG_BUF_SIZE           127

127 sounds slightly strange. Is it by specification of protocol? Would
it be better to define it 128 bytes / items?

> +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> +{
> +       const struct ec_cmd_t *p;
> +
> +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> +               if (p->cmd == cmd)
> +                       return p->bytes_returned;
> +       }
> +
> +       return -1;

-EINVAL ?

> +}

> +static void olpc_xo175_ec_complete(void *arg);

Hmm... Can we avoid forward declaration?

> +       channel = priv->rx_buf[0];
> +       byte = priv->rx_buf[1];

Maybe specific structures would fit better?

Like

struct olpc_ec_resp_hdr {
 u8 channel;
 u8 byte;
...
}

> +               dev_warn(dev, "kbd/tpad not supported\n");

Please, spell it fully as touchpad and keyboard.

> +                       pm_wakeup_event(priv->pwrbtn->dev.parent, 1000);

Magic number.

> +                       /* For now, we just ignore the unknown events. */

dev_dbg(dev, "Ignored unknown event %.2x\n", byte);

?

> if (isprint(byte)) {
> +                       priv->logbuf[priv->logbuf_len++] = byte;
> +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> +                               olpc_xo175_ec_flush_logbuf(priv);
> +               }

You may consider to take everything and run %pE when printing instead of %s.

> +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *resp,
> +                                       size_t resp_len, void *ec_cb_arg)
> +{
> +       struct olpc_xo175_ec *priv = ec_cb_arg;
> +       struct device *dev = &priv->spi->dev;
> +       unsigned long flags;
> +       int nr_bytes;
> +       int ret = 0;
> +
> +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> +
> +       if (inlen > 5) {

Magic number.

> +               dev_err(dev, "command len %d too big!\n", resp_len);
> +               return -EOVERFLOW;
> +       }

> +       WARN_ON(priv->suspended);
> +       if (priv->suspended)

if (WARN_ON(...)) ?

> +               return -EBUSY;

> +       if (resp_len > nr_bytes)
> +               resp_len = nr_bytes;

resp_len = min(resp_len, nr_bytes);

> +       priv->cmd[0] = cmd;
> +       priv->cmd[1] = inlen;
> +       priv->cmd[2] = 0;

Perhaps specific struct header for this?

> +       memset(resp, 0, resp_len);

Wouldn't be better to do this in where actual response has been filled?

> +       if (!wait_for_completion_timeout(&priv->cmd_done,
> +                       msecs_to_jiffies(4000))) {

Magic number.

> +       }

> +       /* Deal with the results. */

Somehow feels noisy / unneeded comment.

> +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> +               /* EC-provided error is in the single response byte */
> +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> +                                                       cmd, priv->resp[0]);

Indentation.

> +               ret = -EREMOTEIO;
> +       } else if (priv->resp_len != nr_bytes) {

> +               dev_err(dev, "command 0x%x returned %d bytes, expected %d bytes\n",
> +                                               cmd, priv->resp_len, nr_bytes);
> +               ret = -ETIMEDOUT;

In the message I see nothing about timeout.

> +       } else {

> +       }

> +}


> +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> +{
> +       unsigned char args[2];

u8

> +
> +       args[0] = mask & 0xff;
> +       args[1] = (mask >> 8) & 0xff;

...mask >> 0;
...mask >> 8;

> +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL, 0);
> +}

> +
> +static void olpc_xo175_ec_restart(enum reboot_mode mode, const char *cmd)
> +{
> +       while (1) {
> +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> +               mdelay(1000);
> +       }
> +}
> +

> +static void olpc_xo175_ec_power_off(void)
> +{
> +       while (1) {
> +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> +               mdelay(1000);
> +       }
> +}
> +

> +#ifdef CONFIG_PM
> +static int olpc_xo175_ec_suspend(struct device *dev)

__maybe_unused  instead of ugly #ifdef?

> +{

> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

dev_get_drvdata() or how is it called?

> +       unsigned char hintargs[5];

struct olpc_ec_hint_cmd {
u8 ...
u32 ...
};

?

> +       static unsigned int suspend_count;

u32 I suppose.

> +
> +       suspend_count++;
> +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__, suspend_count);

__func__ can be issued if user asked for via Dynamic Debug interface.

> +       /*
> +        * First byte is 1 to indicate suspend, the rest is an integer
> +        * counter.
> +        */
> +       hintargs[0] = 1;
> +       memcpy(&hintargs[1], &suspend_count, 4);
> +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);

What do you need this counter for?

> +       priv->suspended = true;

Hmm... Who is the user of it?

> +       return 0;
> +}
> +
> +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> +
> +       priv->suspended = false;
> +
> +       return 0;
> +}
> +
> +static int olpc_xo175_ec_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);

> +       unsigned char x = 0;

u8

> +       priv->suspended = false;

Isn't it redundant since noirq callback above?

> +       /*
> +        * The resume hint is only needed if no other commands are
> +        * being sent during resume. all it does is tell the EC
> +        * the SoC is definitely awake.
> +        */
> +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> +

> +       /* Enable all EC events while we're awake */
> +       olpc_xo175_ec_set_event_mask(0xffff);

#define EC_ALL_EVENTS GENMASK(15, 0)

> +}
> +#endif

> +static struct platform_device *olpc_ec;

I would rather see this at the top of file.

> +static int olpc_xo175_ec_probe(struct spi_device *spi)
> +{

> +       if (olpc_ec) {
> +               dev_err(&spi->dev, "OLPC EC already registered.\n");
> +               return -EBUSY;
> +       }

It's racy against parallel probe called. I don't think it would be a
real issue, just let you know.


> +       /* Set up power button input device */
> +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> +       if (!priv->pwrbtn)
> +               return -ENOMEM;
> +       priv->pwrbtn->name = "Power Button";
> +       priv->pwrbtn->dev.parent = &spi->dev;
> +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> +       ret = input_register_device(priv->pwrbtn);
> +       if (ret) {
> +               dev_err(&spi->dev, "error registering input device: %d\n", ret);
> +               return ret;
> +       }

I would split out power button driver, but it's up to you.


> +       /* Enable all EC events while we're awake */
> +       olpc_xo175_ec_set_event_mask(0xffff);

See above about this magic.

> +}

> +#ifdef CONFIG_PM
> +       .suspend        = olpc_xo175_ec_suspend,
> +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> +       .resume         = olpc_xo175_ec_resume,
> +#endif

SET_SYSTEM_SLEEP_PM_OPS() ?
SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?

> +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> +       { .compatible = "olpc,xo1.75-ec" },

> +       { },

No comma for terminators.

> +};

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units
  2018-10-19 13:00   ` Andy Shevchenko
@ 2018-10-21 21:27     ` Sebastian Reichel
  0 siblings, 0 replies; 62+ messages in thread
From: Sebastian Reichel @ 2018-10-21 21:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Lubomir Rintel, David Woodhouse, Mark Brown, Geert Uytterhoeven,
	Darren Hart, Andy Shevchenko, Greg Kroah-Hartman, quozl,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree,
	Linux Kernel Mailing List, linux-arm Mailing List,
	Platform Driver

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

Hi,

On Fri, Oct 19, 2018 at 04:00:32PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
> >
> > According to [1] and [2], the temperature values are in tenths of degree
> > Celsius. Exposing the Celsius value makes the battery appear on fire:
> >
> >   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
> >   ...
> >       temperature:         236.9 degrees C
> >
> > Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
> 
> It's interesting that the very author of that code is not included in
> so-o long Cc list :)
> Cc: David.
> 
> David, do you remember if and how you had tested temperature report of
> the battery on OLPC? I guess this kind of error would be appear immediately.

It depends on the way of testing. It's not so obvious when you just
do a `cat /sys/class/power_supply/.../temperature`, since you just
get the raw integer.

> OTOH it might be that power framework had changed requirements (which
> would be noticeable change).

As far as I know, power-supply has always used 1/10 °C. People tend
to get units wrong all the time though (i.e. mV instead of uV). I'm
not surprised, that this sneaked into the kernel.

> If the latter is true, this patch misses Fixes tag. Actually in any
> case it misses it.

Yes, this should probably have Fixes: fb972873a767 ("[BATTERY] One Laptop
Per Child power/battery driver").

-- Sebastian

> > [1] include/linux/power_supply.h
> > [2] Documentation/power/power_supply_class.txt
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  drivers/power/supply/olpc_battery.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> > index 6da79ae14860..5a97e42a3547 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
> >                 if (ret)
> >                         return ret;
> >
> > -               val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> > +               val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> >                 break;
> >         case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> >                 ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
> >                 if (ret)
> >                         return ret;
> >
> > -               val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> > +               val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> >                 break;
> >         case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> >                 ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> > --
> > 2.19.0
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
  2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
                   ` (16 preceding siblings ...)
  2018-10-19 13:57 ` Andy Shevchenko
@ 2018-10-21 21:45 ` Sebastian Reichel
  17 siblings, 0 replies; 62+ messages in thread
From: Sebastian Reichel @ 2018-10-21 21:45 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Rob Herring, linux-spi, Mark Brown, Haojian Zhuang, Daniel Mack,
	Darren Hart, platform-driver-x86, Robert Jarzmik,
	Andy Shevchenko, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1264 bytes --]

Hi,

On Wed, Oct 10, 2018 at 07:22:45PM +0200, Lubomir Rintel wrote:
> Hi.
> 
> This patchset adds support for the Embedded Controller on an OLPC XO
> 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> the existing OLPC platform infrastructure, currently used by the x86
> based models.
> 
> The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> uses extra handshake signal to signal readiness of SOC to accept data
> from EC and to initiate a transaction if SOC wishes to submit data.
> 
> The SPI slave support for MMP2 was submitted separately:
> https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t
> 
> THe "power: supply: olpc_battery: correct the temperature" patch was
> already sent out separately, but I'm including it because the last
> commit of the set depends on it.
> 
> Tested to work on an OLPC XO 1.75 and also tested not to break x86
> support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> unlikely this breaks it when XO 1 works.
> 
> Thanks in advance for reviews and feedback of any kind.

I reviewed the power-supply related patches (1, 10, 12-15) and think
they are fine apart from the things found by Andy Shevchenko.

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller
  2018-10-19 13:57 ` Andy Shevchenko
@ 2018-10-23 17:03   ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-10-23 17:03 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Rutland, devicetree, devel, Eric Miao, quozl,
	Geert Uytterhoeven, Linux PM, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sebastian Reichel, Rob Herring,
	linux-spi, Mark Brown, Haojian Zhuang, Daniel Mack, Darren Hart,
	Platform Driver, Robert Jarzmik, Andy Shevchenko,
	linux-arm Mailing List

On Fri, 2018-10-19 at 16:57 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
> > Hi.
> > 
> > This patchset adds support for the Embedded Controller on an OLPC XO
> > 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> > the existing OLPC platform infrastructure, currently used by the x86
> > based models.
> > 
> > The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> > uses extra handshake signal to signal readiness of SOC to accept data
> > from EC and to initiate a transaction if SOC wishes to submit data.
> > 
> > The SPI slave support for MMP2 was submitted separately:
> > https://lore.kernel.org/lkml/20181010170936.316862-1-lkundrak@v3.sk/T/#t
> > 
> > THe "power: supply: olpc_battery: correct the temperature" patch was
> 
> The
> 
> > already sent out separately, but I'm including it because the last
> > commit of the set depends on it.
> > 
> > Tested to work on an OLPC XO 1.75 and also tested not to break x86
> > support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> > unlikely this breaks it when XO 1 works.
> > 
> > Thanks in advance for reviews and feedback of any kind.
> 
> Thanks for the series.
> I'm about to review the patch 6, otherwise read my comments for the
> rest and consider addressing them.

Thank you very much for your feedback, it is very appreciated.

The XO 1.75 patch set has grown somewhat larger than I'm comfortable
with, so I need some time to digest and address the review. I hope I'll
be able to post a v2 some time after the 4.20 merge window closes and
respond to questions raised in individual patches before that.

> 
> > Lubo
> > 
> 
> 

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units
  2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
  2018-10-19 13:00   ` Andy Shevchenko
@ 2018-11-02 22:16   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-02 22:16 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm, stable

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Wed 2018-10-10 19:22:46, Lubomir Rintel wrote:
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
> 
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>       temperature:         236.9 degrees C
> 
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
> 
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> +		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>  		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> +		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"
  2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
  2018-10-19 13:04   ` Andy Shevchenko
@ 2018-11-02 22:16   ` Pavel Machek
  2018-11-15 18:57     ` Lubomir Rintel
  1 sibling, 1 reply; 62+ messages in thread
From: Pavel Machek @ 2018-11-02 22:16 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Wed 2018-10-10 19:22:47, Lubomir Rintel wrote:
> It doesn't make sense to always have this built-in, e.g. on ARM
> multiplatform kernels. A better way to address the problem the original
> commit aimed to solve is to fix Kconfig.
> 
> This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.

This looks ok, but I don't see the Kconfig fix in the series. Is it
needed?
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings
  2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
  2018-10-17 19:38   ` Rob Herring
@ 2018-11-04 12:26   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:26 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Sebastian Reichel, Rob Herring, linux-spi, Mark Brown,
	Haojian Zhuang, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1868 bytes --]

On Wed 2018-10-10 19:22:48, Lubomir Rintel wrote:
> The OLPC XO-1.75 Embedded Controller is a SPI master that uses extra
> signals for handshaking. It needs to know when is the slave (Linux)
> side's TX FIFO ready for transfer (the ready-gpio signal on the SPI
> controller node) and when does it wish to respond with a command (the
> cmd-gpio property).
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>


> ---
>  .../bindings/misc/olpc,xo1.75-ec.txt          | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> new file mode 100644
> index 000000000000..14385fee65d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/olpc,xo1.75-ec.txt
> @@ -0,0 +1,24 @@
> +OLPC XO-1.75 Embedded Controller
> +
> +Required properties:
> +- compatible: Should be "olpc,xo1.75-ec".
> +- cmd-gpio: gpio specifier of the CMD pin
> +
> +The embedded controller requires the SPI controller driver to signal readiness
> +to receive a transfer (that is, when TX FIFO contains the response data) by
> +strobing the ACK pin with the ready signal. See the "ready-gpio" property of the
> +SSP binding as documented in:
> +<Documentation/devicetree/bindings/spi/spi-pxa2xx.txt>.
> +
> +Example:
> +	&ssp3 {
> +		spi-slave;
> +		status = "okay";
> +		ready-gpio = <&gpio 125 GPIO_ACTIVE_HIGH>;
> +
> +		slave {
> +			compatible = "olpc,xo1.75-ec";
> +			spi-cpha;
> +			cmd-gpio = <&gpio 155 GPIO_ACTIVE_HIGH>;
> +		};
> +	};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 04/15] Platform: OLPC: Remove an unused include
  2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
  2018-10-19 13:05   ` Andy Shevchenko
@ 2018-11-04 12:26   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:26 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Wed 2018-10-10 19:22:49, Lubomir Rintel wrote:
> Also, the header is x86 specific, while there are non-x86 OLPC machines.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/platform/olpc/olpc-ec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index f99b183d5296..35a21c66cd0d 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -15,7 +15,6 @@
>  #include <linux/module.h>
>  #include <linux/list.h>
>  #include <linux/olpc-ec.h>
> -#include <asm/olpc.h>
>  
>  struct ec_cmd_desc {
>  	u8 cmd;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree
  2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
  2018-10-19 13:09   ` Andy Shevchenko
@ 2018-11-04 12:27   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:27 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 3466 bytes --]

On Wed 2018-10-10 19:22:50, Lubomir Rintel wrote:
> There are ARM OLPC machines that use mostly the same drivers, including
> EC infrastructure, DCON and Battery.
> 
> While at that, fix Kconfig to allow building this as a module.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  arch/x86/Kconfig                  | 11 -----------
>  drivers/input/mouse/Kconfig       |  2 +-
>  drivers/platform/Kconfig          |  2 ++
>  drivers/platform/olpc/Kconfig     | 11 +++++++++++
>  drivers/staging/olpc_dcon/Kconfig |  2 +-
>  5 files changed, 15 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/platform/olpc/Kconfig
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1a0be022f91d..be6af341a718 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2729,17 +2729,6 @@ config SCx200HR_TIMER
>  	  processor goes idle (as is done by the scheduler).  The
>  	  other workaround is idle=poll boot option.
>  
> -config OLPC
> -	bool "One Laptop Per Child support"
> -	depends on !X86_PAE
> -	select GPIOLIB
> -	select OF
> -	select OF_PROMTREE
> -	select IRQ_DOMAIN
> -	---help---
> -	  Add support for detecting the unique features of the OLPC
> -	  XO hardware.
> -
>  config OLPC_XO1_PM
>  	bool "OLPC XO-1 Power Management"
>  	depends on OLPC && MFD_CS5535 && PM_SLEEP
> diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
> index 566a1e3aa504..58034892a4df 100644
> --- a/drivers/input/mouse/Kconfig
> +++ b/drivers/input/mouse/Kconfig
> @@ -165,7 +165,7 @@ config MOUSE_PS2_TOUCHKIT
>  
>  config MOUSE_PS2_OLPC
>  	bool "OLPC PS/2 mouse protocol extension"
> -	depends on MOUSE_PS2 && OLPC
> +	depends on MOUSE_PS2 && X86 && OLPC
>  	help
>  	  Say Y here if you have an OLPC XO-1 laptop (with built-in
>  	  PS/2 touchpad/tablet device).  The manufacturer calls the
> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
> index d4c2e424a700..4313d73d3618 100644
> --- a/drivers/platform/Kconfig
> +++ b/drivers/platform/Kconfig
> @@ -10,3 +10,5 @@ source "drivers/platform/goldfish/Kconfig"
>  source "drivers/platform/chrome/Kconfig"
>  
>  source "drivers/platform/mellanox/Kconfig"
> +
> +source "drivers/platform/olpc/Kconfig"
> diff --git a/drivers/platform/olpc/Kconfig b/drivers/platform/olpc/Kconfig
> new file mode 100644
> index 000000000000..7b736c9e67ac
> --- /dev/null
> +++ b/drivers/platform/olpc/Kconfig
> @@ -0,0 +1,11 @@
> +config OLPC
> +	tristate "One Laptop Per Child support"
> +	depends on X86 || ARM || COMPILE_TEST
> +	depends on !X86_PAE
> +	select GPIOLIB
> +	select OF
> +	select OF_PROMTREE if X86
> +	select IRQ_DOMAIN
> +	help
> +	  Add support for detecting the unique features of the OLPC
> +	  XO hardware.
> diff --git a/drivers/staging/olpc_dcon/Kconfig b/drivers/staging/olpc_dcon/Kconfig
> index c91a56f77bcb..07f9f1de8667 100644
> --- a/drivers/staging/olpc_dcon/Kconfig
> +++ b/drivers/staging/olpc_dcon/Kconfig
> @@ -1,6 +1,6 @@
>  config FB_OLPC_DCON
>  	tristate "One Laptop Per Child Display CONtroller support"
> -	depends on OLPC && FB
> +	depends on X86 && OLPC && FB
>  	depends on I2C
>  	depends on (GPIO_CS5535 || GPIO_CS5535=n)
>  	select BACKLIGHT_CLASS_DEVICE

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet
  2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
  2018-10-19 13:11   ` Andy Shevchenko
@ 2018-11-04 12:30   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:30 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Sebastian Reichel, Rob Herring, linux-spi, Mark Brown,
	Haojian Zhuang, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1152 bytes --]

On Wed 2018-10-10 19:22:52, Lubomir Rintel wrote:
> Just return ENODEV, so that whoever attempted to use the EC call can
> defer their work.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/platform/olpc/olpc-ec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c
> index 35a21c66cd0d..342f5bb7f7a8 100644
> --- a/drivers/platform/olpc/olpc-ec.c
> +++ b/drivers/platform/olpc/olpc-ec.c
> @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 *outbuf, size_t outlen)
>  	struct olpc_ec_priv *ec = ec_priv;
>  	struct ec_cmd_desc desc;
>  
> -	/* Ensure a driver and ec hook have been registered */
> -	if (WARN_ON(!ec_driver || !ec_driver->ec_cmd))
> +	/* Driver not yet registered. */
> +	if (!ec_driver)
> +		return -ENODEV;
> +
> +	if (WARN_ON(!ec_driver->ec_cmd))
>  		return -ENODEV;
>  
>  	if (!ec)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86
  2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
  2018-10-19 13:36   ` Andy Shevchenko
@ 2018-11-04 12:31   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:31 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Wed 2018-10-10 19:22:53, Lubomir Rintel wrote:
> It is actually plaform independent. Move it to the olpc-ec driver from
> the X86 OLPC platform, so that it could be used by the ARM based laptops
> too.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery
  2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
  2018-10-17 19:38   ` Rob Herring
@ 2018-11-04 12:32   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:32 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Wed 2018-10-10 19:22:55, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Rob Herring <robh@kernel.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

Rob, can you apply?

Thanks,
								Pavel
> ---
>  Documentation/devicetree/bindings/power/supply/olpc_battery.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> index c8901b3992d9..8d87d6b35a98 100644
> --- a/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/olpc_battery.txt
> @@ -2,4 +2,4 @@ OLPC battery
>  ~~~~~~~~~~~~
>  
>  Required properties:
> -  - compatible : "olpc,xo1-battery"
> +  - compatible : "olpc,xo1-battery" or "olpc,xo1.5-battery"

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node
  2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
  2018-10-19 13:43   ` Andy Shevchenko
@ 2018-11-04 12:34   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:34 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Sebastian Reichel, Rob Herring, linux-spi, Mark Brown,
	Haojian Zhuang, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 3908 bytes --]

On Wed 2018-10-10 19:22:56, Lubomir Rintel wrote:
> The XO-1 and XO-1.5 batteries apparently differ in an ability to report
> ambient temperature. Add a different compatible string to the 1.5
> battery.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  arch/x86/platform/olpc/olpc_dt.c | 59 +++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
> index d6ee92986920..6e54e116b0c5 100644
> --- a/arch/x86/platform/olpc/olpc_dt.c
> +++ b/arch/x86/platform/olpc/olpc_dt.c
> @@ -218,10 +218,28 @@ static u32 __init olpc_dt_get_board_revision(void)
>  	return be32_to_cpu(rev);
>  }
>  
> -void __init olpc_dt_fixup(void)
> +int olpc_dt_compatible_match(phandle node, const char *compat)
>  {
> -	int r;
>  	char buf[64];
> +	int plen;
> +	char *p;
> +	int len;
> +
> +	plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> +	if (plen <= 0)
> +		return 0;
> +
> +	len = strlen(compat);
> +	for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> +		if (strcmp(p, compat) == 0)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +void __init olpc_dt_fixup(void)
> +{
>  	phandle node;
>  	u32 board_rev;
>  
> @@ -229,32 +247,33 @@ void __init olpc_dt_fixup(void)
>  	if (!node)
>  		return;
>  
> -	/*
> -	 * If the battery node has a compatible property, we are running a new
> -	 * enough firmware and don't have fixups to make.
> -	 */
> -	r = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
> -	if (r > 0)
> -		return;
> -
> -	pr_info("PROM DT: Old firmware detected, applying fixes\n");
> -
> -	/* Add olpc,xo1-battery compatible marker to battery node */
> -	olpc_dt_interpret("\" /battery@0\" find-device"
> -		" \" olpc,xo1-battery\" +compatible"
> -		" device-end");
> -
>  	board_rev = olpc_dt_get_board_revision();
>  	if (!board_rev)
>  		return;
>  
>  	if (board_rev >= olpc_board_pre(0xd0)) {
> +		if (olpc_dt_compatible_match(node, "olpc,xo1.5-battery"))
> +			return;
> +
> +		/* Add olpc,xo1.5-battery compatible marker to battery node */
> +		olpc_dt_interpret("\" /battery@0\" find-device"
> +			" \" olpc,xo1.5-battery\" +compatible"
> +			" device-end");
> +
> +		/* We're running a very old firmware if this is missing. */
> +		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
> +			return;
> +
>  		/* XO-1.5: add dcon device */
>  		olpc_dt_interpret("\" /pci/display@1\" find-device"
>  			" new-device"
>  			" \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
>  			" finish-device device-end");
>  	} else {
> +		/* We're running a very old firmware if this is missing. */
> +		if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
> +			return;
> +
>  		/* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
>  		olpc_dt_interpret("\" /pci/display@1,1\" find-device"
>  			" new-device"
> @@ -264,6 +283,11 @@ void __init olpc_dt_fixup(void)
>  			" \" olpc,xo1-rtc\" +compatible"
>  			" device-end");
>  	}
> +
> +	/* Add olpc,xo1-battery compatible marker to battery node */
> +	olpc_dt_interpret("\" /battery@0\" find-device"
> +		" \" olpc,xo1-battery\" +compatible"
> +		" device-end");
>  }
>  
>  void __init olpc_dt_build_devicetree(void)
> @@ -289,6 +313,7 @@ void __init olpc_dt_build_devicetree(void)
>  /* A list of DT node/bus matches that we want to expose as platform devices */
>  static struct of_device_id __initdata of_ids[] = {
>  	{ .compatible = "olpc,xo1-battery" },
> +	{ .compatible = "olpc,xo1.5-battery" },
>  	{ .compatible = "olpc,xo1-dcon" },
>  	{ .compatible = "olpc,xo1-rtc" },
>  	{},

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
  2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
  2018-10-19 13:45   ` Andy Shevchenko
@ 2018-11-04 12:37   ` Pavel Machek
  2018-11-15 18:36     ` Lubomir Rintel
  1 sibling, 1 reply; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 12:37 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On Wed 2018-10-10 19:22:57, Lubomir Rintel wrote:
> Avoid using the x86 OLPC platform specific call to get the board
> version. It won't work on FDT-based ARM MMP2 platform.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

AFAICT, this should go earlier in the series; first, add support in
the code, then switch to new name in DTS.
								Pavel

> ---
>  drivers/power/supply/olpc_battery.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 5a97e42a3547..540d44bf536f 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -19,6 +19,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/sched.h>
>  #include <linux/olpc-ec.h>
> +#include <linux/of.h>
>  #include <asm/olpc.h>
>  
>  
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
>  	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
>  	if (IS_ERR(olpc_ac))
>  		return PTR_ERR(olpc_ac);
> -
> -	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> +	if (of_property_match_string(pdev->dev.of_node, "compatible",
> +					"olpc,xo1.5-battery") >= 0) {
> +		/* XO-1.5 */
>  		olpc_bat_desc.properties = olpc_xo15_bat_props;
>  		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo15_bat_props);
> -	} else { /* XO-1 */
> +	} else {
> +		/* XO-1 */
>  		olpc_bat_desc.properties = olpc_xo1_bat_props;
>  		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>  	}
> @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id olpc_battery_ids[] = {
>  	{ .compatible = "olpc,xo1-battery" },
> +	{ .compatible = "olpc,xo1.5-battery" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, olpc_battery_ids);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct
  2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
  2018-10-19 13:48   ` Andy Shevchenko
@ 2018-11-04 14:37   ` Pavel Machek
  2018-11-14 17:10     ` Lubomir Rintel
  1 sibling, 1 reply; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 14:37 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

Hi!

> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Ok...

> -	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> -	if (IS_ERR(olpc_bat)) {
> -		ret = PTR_ERR(olpc_bat);
> -		goto battery_failed;
> -	}
> +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> +	if (IS_ERR(data->olpc_bat))
> +		return PTR_ERR(data->olpc_bat);
>  
> -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	if (ret)
> -		goto eeprom_failed;
> +		return ret;
>  
> -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
>  	if (ret)
>  		goto error_failed;
>  
>  	if (olpc_ec_wakeup_available()) {
> -		device_set_wakeup_capable(&olpc_ac->dev, true);
> -		device_set_wakeup_capable(&olpc_bat->dev, true);
> +		device_set_wakeup_capable(&data->olpc_ac->dev, true);
> +		device_set_wakeup_capable(&data->olpc_bat->dev, true);
>  	}
>  
>  	return 0;
>  
>  error_failed:
> -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -eeprom_failed:
> -	power_supply_unregister(olpc_bat);
> -battery_failed:
> -	power_supply_unregister(olpc_ac);
> +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	return ret;
>  }

...but you are changing error handling here, which is not mentioned in
the changelog, and I'm nut sure you got it right.

Are you sure?

>  static int olpc_battery_remove(struct platform_device *pdev)
>  {
> -	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -	power_supply_unregister(olpc_bat);
> -	power_supply_unregister(olpc_ac);
> +	struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	return 0;
>  }

Here too.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info
  2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
  2018-10-19 13:50   ` Andy Shevchenko
@ 2018-11-04 14:39   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 14:39 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Wed 2018-10-10 19:22:59, Lubomir Rintel wrote:
> This wouldn't work on the DT-based ARM platform. Let's read the EC version
> directly from the EC driver instead.
> 
> This makes the driver no longer x86 specific.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support
  2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
  2018-10-19 13:55   ` Andy Shevchenko
@ 2018-11-04 14:39   ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: Pavel Machek @ 2018-11-04 14:39 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Mark Rutland, devicetree, devel, Eric Miao, James Cameron,
	Geert Uytterhoeven, linux-pm, Greg Kroah-Hartman, linux-kernel,
	Sebastian Reichel, Rob Herring, linux-spi, Mark Brown,
	Haojian Zhuang, Daniel Mack, Darren Hart, platform-driver-x86,
	Robert Jarzmik, Andy Shevchenko, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 410 bytes --]

On Wed 2018-10-10 19:23:00, Lubomir Rintel wrote:
> The battery and the protocol are essentially the same as OLPC XO 1.5,
> but the responses from the EC are LSB first.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 04/15] Platform: OLPC: Remove an unused include
  2018-10-19 13:05   ` Andy Shevchenko
@ 2018-11-13 13:59     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-13 13:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

Hi,

thank you for the response.

On Fri, 2018-10-19 at 16:05 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > Also, the header is x86 specific, while there are non-x86 OLPC
> > machines.
> 
> Same concern. as per patch 2.

Which concern? If it's that it doesn't make sense in that particular
place in the patch set, then I don't think so; this header is
unnecessary and the patch has no other dependencies. But the changes
that enable the OLPC EC platform code depend on this.

> Also, you might want to sort headers in alphabetical order.

But should I? Doing so in the same commit would obscure the actual
change, and a separate commit would needlessly clobber git annotate.

Thank you
Lubo

> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  drivers/platform/olpc/olpc-ec.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/platform/olpc/olpc-ec.c
> > b/drivers/platform/olpc/olpc-ec.c
> > index f99b183d5296..35a21c66cd0d 100644
> > --- a/drivers/platform/olpc/olpc-ec.c
> > +++ b/drivers/platform/olpc/olpc-ec.c
> > @@ -15,7 +15,6 @@
> >  #include <linux/module.h>
> >  #include <linux/list.h>
> >  #include <linux/olpc-ec.h>
> > -#include <asm/olpc.h>
> > 
> >  struct ec_cmd_desc {
> >         u8 cmd;
> > --
> > 2.19.0
> > 
> 
> 

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-10-19 16:06   ` Andy Shevchenko
@ 2018-11-13 17:26     ` Lubomir Rintel
  2018-11-13 22:06       ` James Cameron
  2018-11-19 10:40       ` Pavel Machek
  0 siblings, 2 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-13 17:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

Hi,

first of all -- thanks for such a careful review. It is very helpful.

Wherever I don't respond to you, I'm just following what you wrote. It
would perhaps be tiresome to respond to "Yes, will fix in next version"
to every single point.

I'll be following up with a new version in a few days; I'm mostly done
with this one but I've not finished addressing the followup ones.

On Fri, 2018-10-19 at 19:06 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > It's based off the driver from the OLPC kernel sources. Somewhat
> > modernized and cleaned up, for better or worse.
> > 
> > Modified to plug into the olpc-ec driver infrastructure (so that
> > battery
> > interface and debugfs could be reused) and the SPI slave framework.
> > +#include <asm/system_misc.h>
> 
> asm/* goes after linux/*
> 
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/completion.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/ctype.h>
> > +#include <linux/olpc-ec.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/reboot.h>
> > +#include <linux/input.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/module.h>
> > +#include <linux/power_supply.h>
> 
> Easy to maintain when it's sorted.
> 
> > +       { 0 },
> 
> Terminators are better without trailing comma.
> 
> > +#define EC_CMD_LEN             8
> > +#define EC_MAX_RESP_LEN                16
> > +#define LOG_BUF_SIZE           127
> 
> 127 sounds slightly strange. Is it by specification of protocol?
> Would
> it be better to define it 128 bytes / items?
> 
> > +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> > +{
> > +       const struct ec_cmd_t *p;
> > +
> > +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> > +               if (p->cmd == cmd)
> > +                       return p->bytes_returned;
> > +       }
> > +
> > +       return -1;
> 
> -EINVAL ?
> 
> > +}
> > +static void olpc_xo175_ec_complete(void *arg);
> 
> Hmm... Can we avoid forward declaration?

I don't think we can.

> > +       channel = priv->rx_buf[0];
> > +       byte = priv->rx_buf[1];
> 
> Maybe specific structures would fit better?
> 
> Like
> 
> struct olpc_ec_resp_hdr {
>  u8 channel;
>  u8 byte;
> ...
> }
> 
> > +               dev_warn(dev, "kbd/tpad not supported\n");
> 
> Please, spell it fully as touchpad and keyboard.
> 
> > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > 1000);
> 
> Magic number.
> 
> > +                       /* For now, we just ignore the unknown
> > events. */
> 
> dev_dbg(dev, "Ignored unknown event %.2x\n", byte);
> 
> ?
> 
> > if (isprint(byte)) {
> > +                       priv->logbuf[priv->logbuf_len++] = byte;
> > +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> > +                               olpc_xo175_ec_flush_logbuf(priv);
> > +               }
> 
> You may consider to take everything and run %pE when printing instead
> of %s.
> 
> > +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8
> > *resp,
> > +                                       size_t resp_len, void
> > *ec_cb_arg)
> > +{
> > +       struct olpc_xo175_ec *priv = ec_cb_arg;
> > +       struct device *dev = &priv->spi->dev;
> > +       unsigned long flags;
> > +       int nr_bytes;
> > +       int ret = 0;
> > +
> > +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> > +
> > +       if (inlen > 5) {
> 
> Magic number.
> 
> > +               dev_err(dev, "command len %d too big!\n",
> > resp_len);
> > +               return -EOVERFLOW;
> > +       }
> > +       WARN_ON(priv->suspended);
> > +       if (priv->suspended)
> 
> if (WARN_ON(...)) ?
> 
> > +               return -EBUSY;
> > +       if (resp_len > nr_bytes)
> > +               resp_len = nr_bytes;
> 
> resp_len = min(resp_len, nr_bytes);
> 
> > +       priv->cmd[0] = cmd;
> > +       priv->cmd[1] = inlen;
> > +       priv->cmd[2] = 0;
> 
> Perhaps specific struct header for this?
> 
> > +       memset(resp, 0, resp_len);
> 
> Wouldn't be better to do this in where actual response has been
> filled?
> 
> > +       if (!wait_for_completion_timeout(&priv->cmd_done,
> > +                       msecs_to_jiffies(4000))) {
> 
> Magic number.
> 
> > +       }
> > +       /* Deal with the results. */
> 
> Somehow feels noisy / unneeded comment.
> 
> > +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> > +               /* EC-provided error is in the single response byte
> > */
> > +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> > +                                                       cmd, priv-
> > >resp[0]);
> 
> Indentation.
> 
> > +               ret = -EREMOTEIO;
> > +       } else if (priv->resp_len != nr_bytes) {
> > +               dev_err(dev, "command 0x%x returned %d bytes,
> > expected %d bytes\n",
> > +                                               cmd, priv-
> > >resp_len, nr_bytes);
> > +               ret = -ETIMEDOUT;
> 
> In the message I see nothing about timeout.
> 
> > +       } else {
> > +       }
> > +}
> > +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> > +{
> > +       unsigned char args[2];
> 
> u8
> 
> > +
> > +       args[0] = mask & 0xff;
> > +       args[1] = (mask >> 8) & 0xff;
> 
> ...mask >> 0;
> ...mask >> 8;
> 
> > +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL,
> > 0);
> > +}
> > +
> > +static void olpc_xo175_ec_restart(enum reboot_mode mode, const
> > char *cmd)
> > +{
> > +       while (1) {
> > +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> > +               mdelay(1000);
> > +       }
> > +}
> > +
> > +static void olpc_xo175_ec_power_off(void)
> > +{
> > +       while (1) {
> > +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> > +               mdelay(1000);
> > +       }
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int olpc_xo175_ec_suspend(struct device *dev)
> 
> __maybe_unused  instead of ugly #ifdef?
> 
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> 
> dev_get_drvdata() or how is it called?
> 
> > +       unsigned char hintargs[5];
> 
> struct olpc_ec_hint_cmd {
> u8 ...
> u32 ...
> };
> 
> ?
> 
> > +       static unsigned int suspend_count;
> 
> u32 I suppose.
> 
> > +
> > +       suspend_count++;
> > +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__,
> > suspend_count);
> 
> __func__ can be issued if user asked for via Dynamic Debug interface.
> 
> > +       /*
> > +        * First byte is 1 to indicate suspend, the rest is an
> > integer
> > +        * counter.
> > +        */
> > +       hintargs[0] = 1;
> > +       memcpy(&hintargs[1], &suspend_count, 4);
> > +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
> 
> What do you need this counter for?

It doesn't seem to be actually used in the EC; the firmware just
includes it in its debug log. I'm not sure if all firmware versions
behave this way and I'd prefer to keep it.

I'm adding a comment.

> 
> > +       priv->suspended = true;
> 
> Hmm... Who is the user of it?
> 
> > +       return 0;
> > +}
> > +
> > +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > +
> > +       priv->suspended = false;
> > +
> > +       return 0;
> > +}
> > +
> > +static int olpc_xo175_ec_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > +       unsigned char x = 0;
> 
> u8
> 
> > +       priv->suspended = false;
> 
> Isn't it redundant since noirq callback above?
> 
> > +       /*
> > +        * The resume hint is only needed if no other commands are
> > +        * being sent during resume. all it does is tell the EC
> > +        * the SoC is definitely awake.
> > +        */
> > +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> > +
> > +       /* Enable all EC events while we're awake */
> > +       olpc_xo175_ec_set_event_mask(0xffff);
> 
> #define EC_ALL_EVENTS GENMASK(15, 0)
> 
> > +}
> > +#endif
> > +static struct platform_device *olpc_ec;
> 
> I would rather see this at the top of file.
> 
> > +static int olpc_xo175_ec_probe(struct spi_device *spi)
> > +{
> > +       if (olpc_ec) {
> > +               dev_err(&spi->dev, "OLPC EC already
> > registered.\n");
> > +               return -EBUSY;
> > +       }
> 
> It's racy against parallel probe called. I don't think it would be a
> real issue, just let you know.
> 
> 
> > +       /* Set up power button input device */
> > +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> > +       if (!priv->pwrbtn)
> > +               return -ENOMEM;
> > +       priv->pwrbtn->name = "Power Button";
> > +       priv->pwrbtn->dev.parent = &spi->dev;
> > +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> > +       ret = input_register_device(priv->pwrbtn);
> > +       if (ret) {
> > +               dev_err(&spi->dev, "error registering input device:
> > %d\n", ret);
> > +               return ret;
> > +       }
> 
> I would split out power button driver, but it's up to you.
> 
> 
> > +       /* Enable all EC events while we're awake */
> > +       olpc_xo175_ec_set_event_mask(0xffff);
> 
> See above about this magic.
> 
> > +}
> > +#ifdef CONFIG_PM
> > +       .suspend        = olpc_xo175_ec_suspend,
> > +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> > +       .resume         = olpc_xo175_ec_resume,
> > +#endif
> 
> SET_SYSTEM_SLEEP_PM_OPS() ?
> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?
> 
> > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > +       { .compatible = "olpc,xo1.75-ec" },
> > +       { },
> 
> No comma for terminators.
> 
> > +};

Thanks,
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-11-13 17:26     ` Lubomir Rintel
@ 2018-11-13 22:06       ` James Cameron
  2018-11-19 10:40       ` Pavel Machek
  1 sibling, 0 replies; 62+ messages in thread
From: James Cameron @ 2018-11-13 22:06 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Andy Shevchenko, Mark Brown, Geert Uytterhoeven, Darren Hart,
	Andy Shevchenko, Greg Kroah-Hartman, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree,
	Linux Kernel Mailing List, linux-arm Mailing List,
	Platform Driver

On Tue, Nov 13, 2018 at 06:26:09PM +0100, Lubomir Rintel wrote:
> Hi,
> 
> first of all -- thanks for such a careful review. It is very helpful.
> 
> Wherever I don't respond to you, I'm just following what you wrote. It
> would perhaps be tiresome to respond to "Yes, will fix in next version"
> to every single point.
> 
> I'll be following up with a new version in a few days; I'm mostly done
> with this one but I've not finished addressing the followup ones.
> 
> On Fri, 2018-10-19 at 19:06 +0300, Andy Shevchenko wrote:
> > On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> > wrote:
> > > It's based off the driver from the OLPC kernel sources. Somewhat
> > > modernized and cleaned up, for better or worse.
> > > 
> > > Modified to plug into the olpc-ec driver infrastructure (so that
> > > battery
> > > interface and debugfs could be reused) and the SPI slave framework.
> > > +#include <asm/system_misc.h>
> > 
> > asm/* goes after linux/*
> > 
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/olpc-ec.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kfifo.h>
> > > +#include <linux/module.h>
> > > +#include <linux/power_supply.h>
> > 
> > Easy to maintain when it's sorted.
> > 
> > > +       { 0 },
> > 
> > Terminators are better without trailing comma.
> > 
> > > +#define EC_CMD_LEN             8
> > > +#define EC_MAX_RESP_LEN                16
> > > +#define LOG_BUF_SIZE           127
> > 
> > 127 sounds slightly strange. Is it by specification of protocol?
> > Would
> > it be better to define it 128 bytes / items?
> > 
> > > +static int olpc_xo175_ec_is_valid_cmd(u8 cmd)
> > > +{
> > > +       const struct ec_cmd_t *p;
> > > +
> > > +       for (p = olpc_xo175_ec_cmds; p->cmd; p++) {
> > > +               if (p->cmd == cmd)
> > > +                       return p->bytes_returned;
> > > +       }
> > > +
> > > +       return -1;
> > 
> > -EINVAL ?
> > 
> > > +}
> > > +static void olpc_xo175_ec_complete(void *arg);
> > 
> > Hmm... Can we avoid forward declaration?
> 
> I don't think we can.
> 
> > > +       channel = priv->rx_buf[0];
> > > +       byte = priv->rx_buf[1];
> > 
> > Maybe specific structures would fit better?
> > 
> > Like
> > 
> > struct olpc_ec_resp_hdr {
> >  u8 channel;
> >  u8 byte;
> > ...
> > }
> > 
> > > +               dev_warn(dev, "kbd/tpad not supported\n");
> > 
> > Please, spell it fully as touchpad and keyboard.
> > 
> > > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> > 
> > Magic number.
> > 
> > > +                       /* For now, we just ignore the unknown
> > > events. */
> > 
> > dev_dbg(dev, "Ignored unknown event %.2x\n", byte);
> > 
> > ?
> > 
> > > if (isprint(byte)) {
> > > +                       priv->logbuf[priv->logbuf_len++] = byte;
> > > +                       if (priv->logbuf_len == LOG_BUF_SIZE)
> > > +                               olpc_xo175_ec_flush_logbuf(priv);
> > > +               }
> > 
> > You may consider to take everything and run %pE when printing instead
> > of %s.
> > 
> > > +static int olpc_xo175_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8
> > > *resp,
> > > +                                       size_t resp_len, void
> > > *ec_cb_arg)
> > > +{
> > > +       struct olpc_xo175_ec *priv = ec_cb_arg;
> > > +       struct device *dev = &priv->spi->dev;
> > > +       unsigned long flags;
> > > +       int nr_bytes;
> > > +       int ret = 0;
> > > +
> > > +       dev_dbg(dev, "CMD %x, %d bytes expected\n", cmd, resp_len);
> > > +
> > > +       if (inlen > 5) {
> > 
> > Magic number.
> > 
> > > +               dev_err(dev, "command len %d too big!\n",
> > > resp_len);
> > > +               return -EOVERFLOW;
> > > +       }
> > > +       WARN_ON(priv->suspended);
> > > +       if (priv->suspended)
> > 
> > if (WARN_ON(...)) ?
> > 
> > > +               return -EBUSY;
> > > +       if (resp_len > nr_bytes)
> > > +               resp_len = nr_bytes;
> > 
> > resp_len = min(resp_len, nr_bytes);
> > 
> > > +       priv->cmd[0] = cmd;
> > > +       priv->cmd[1] = inlen;
> > > +       priv->cmd[2] = 0;
> > 
> > Perhaps specific struct header for this?
> > 
> > > +       memset(resp, 0, resp_len);
> > 
> > Wouldn't be better to do this in where actual response has been
> > filled?
> > 
> > > +       if (!wait_for_completion_timeout(&priv->cmd_done,
> > > +                       msecs_to_jiffies(4000))) {
> > 
> > Magic number.
> > 
> > > +       }
> > > +       /* Deal with the results. */
> > 
> > Somehow feels noisy / unneeded comment.
> > 
> > > +       if (priv->cmd_state == CMD_STATE_ERROR_RECEIVED) {
> > > +               /* EC-provided error is in the single response byte
> > > */
> > > +               dev_err(dev, "command 0x%x returned error 0x%x\n",
> > > +                                                       cmd, priv-
> > > >resp[0]);
> > 
> > Indentation.
> > 
> > > +               ret = -EREMOTEIO;
> > > +       } else if (priv->resp_len != nr_bytes) {
> > > +               dev_err(dev, "command 0x%x returned %d bytes,
> > > expected %d bytes\n",
> > > +                                               cmd, priv-
> > > >resp_len, nr_bytes);
> > > +               ret = -ETIMEDOUT;
> > 
> > In the message I see nothing about timeout.
> > 
> > > +       } else {
> > > +       }
> > > +}
> > > +static int olpc_xo175_ec_set_event_mask(unsigned int mask)
> > > +{
> > > +       unsigned char args[2];
> > 
> > u8
> > 
> > > +
> > > +       args[0] = mask & 0xff;
> > > +       args[1] = (mask >> 8) & 0xff;
> > 
> > ...mask >> 0;
> > ...mask >> 8;
> > 
> > > +       return olpc_ec_cmd(CMD_WRITE_EXT_SCI_MASK, args, 2, NULL,
> > > 0);
> > > +}
> > > +
> > > +static void olpc_xo175_ec_restart(enum reboot_mode mode, const
> > > char *cmd)
> > > +{
> > > +       while (1) {
> > > +               olpc_ec_cmd(CMD_POWER_CYCLE, NULL, 0, NULL, 0);
> > > +               mdelay(1000);
> > > +       }
> > > +}
> > > +
> > > +static void olpc_xo175_ec_power_off(void)
> > > +{
> > > +       while (1) {
> > > +               olpc_ec_cmd(CMD_POWER_OFF, NULL, 0, NULL, 0);
> > > +               mdelay(1000);
> > > +       }
> > > +}
> > > +
> > > +#ifdef CONFIG_PM
> > > +static int olpc_xo175_ec_suspend(struct device *dev)
> > 
> > __maybe_unused  instead of ugly #ifdef?
> > 
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata() or how is it called?
> > 
> > > +       unsigned char hintargs[5];
> > 
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> > 
> > ?
> > 
> > > +       static unsigned int suspend_count;
> > 
> > u32 I suppose.
> > 
> > > +
> > > +       suspend_count++;
> > > +       dev_dbg(dev, "%s: suspend sync %08x\n", __func__,
> > > suspend_count);
> > 
> > __func__ can be issued if user asked for via Dynamic Debug interface.
> > 
> > > +       /*
> > > +        * First byte is 1 to indicate suspend, the rest is an
> > > integer
> > > +        * counter.
> > > +        */
> > > +       hintargs[0] = 1;
> > > +       memcpy(&hintargs[1], &suspend_count, 4);
> > > +       olpc_ec_cmd(CMD_SUSPEND_HINT, hintargs, 5, NULL, 0);
> > 
> > What do you need this counter for?
> 
> It doesn't seem to be actually used in the EC; the firmware just
> includes it in its debug log. I'm not sure if all firmware versions
> behave this way and I'd prefer to keep it.

Some firmware versions rely on it, as the SOC_SLEEP line was
unreliable where the board revision is B3 or earlier.

(internal reference: Paul Fox Wed, 10 Oct 2012 09:39:44 -0400)

Although population of B3 and earlier was low, prototypes were given
out to many rather than destroyed.

> 
> I'm adding a comment.
> 
> > 
> > > +       priv->suspended = true;
> > 
> > Hmm... Who is the user of it?
> > 
> > > +       return 0;
> > > +}
> > > +
> > > +static int olpc_xo175_ec_resume_noirq(struct device *dev)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > > +
> > > +       priv->suspended = false;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int olpc_xo175_ec_resume(struct device *dev)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > > +       unsigned char x = 0;
> > 
> > u8
> > 
> > > +       priv->suspended = false;
> > 
> > Isn't it redundant since noirq callback above?
> > 
> > > +       /*
> > > +        * The resume hint is only needed if no other commands are
> > > +        * being sent during resume. all it does is tell the EC
> > > +        * the SoC is definitely awake.
> > > +        */
> > > +       olpc_ec_cmd(CMD_SUSPEND_HINT, &x, 1, NULL, 0);
> > > +
> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > #define EC_ALL_EVENTS GENMASK(15, 0)
> > 
> > > +}
> > > +#endif
> > > +static struct platform_device *olpc_ec;
> > 
> > I would rather see this at the top of file.
> > 
> > > +static int olpc_xo175_ec_probe(struct spi_device *spi)
> > > +{
> > > +       if (olpc_ec) {
> > > +               dev_err(&spi->dev, "OLPC EC already
> > > registered.\n");
> > > +               return -EBUSY;
> > > +       }
> > 
> > It's racy against parallel probe called. I don't think it would be a
> > real issue, just let you know.
> > 
> > 
> > > +       /* Set up power button input device */
> > > +       priv->pwrbtn = devm_input_allocate_device(&spi->dev);
> > > +       if (!priv->pwrbtn)
> > > +               return -ENOMEM;
> > > +       priv->pwrbtn->name = "Power Button";
> > > +       priv->pwrbtn->dev.parent = &spi->dev;
> > > +       input_set_capability(priv->pwrbtn, EV_KEY, KEY_POWER);
> > > +       ret = input_register_device(priv->pwrbtn);
> > > +       if (ret) {
> > > +               dev_err(&spi->dev, "error registering input device:
> > > %d\n", ret);
> > > +               return ret;
> > > +       }
> > 
> > I would split out power button driver, but it's up to you.
> > 
> > 
> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > See above about this magic.
> > 
> > > +}
> > > +#ifdef CONFIG_PM
> > > +       .suspend        = olpc_xo175_ec_suspend,
> > > +       .resume_noirq   = olpc_xo175_ec_resume_noirq,
> > > +       .resume         = olpc_xo175_ec_resume,
> > > +#endif
> > 
> > SET_SYSTEM_SLEEP_PM_OPS() ?
> > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() ?
> > 
> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > +       { .compatible = "olpc,xo1.75-ec" },
> > > +       { },
> > 
> > No comma for terminators.
> > 
> > > +};
> 
> Thanks,
> Lubo
> 

-- 
James Cameron
http://quozl.netrek.org/

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86
  2018-10-19 13:36   ` Andy Shevchenko
@ 2018-11-14 16:20     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-14 16:20 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

Hello,

On Fri, 2018-10-19 at 16:36 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > It is actually plaform independent. Move it to the olpc-ec driver
> > from
> > the X86 OLPC platform, so that it could be used by the ARM based
> > laptops
> > too.
> 
> What is platform independent exactly?

The commands with their argument and responses are mostly the same, but
the delivery mechanism is different (SPI on ARM vs. LPC on x86).

Notably, the driver for the OLPC battery (which is the same on all XO
generations) builds on the API provided by the olpc-ec driver.

I'll try to extend the commit message to make this clear.

> >  #define OLPC_F_PRESENT         0x01
> >  #define OLPC_F_DCON            0x02
> > -#define OLPC_F_EC_WIDE_SCI     0x04
> 
> I think these lines grouped on purpose. Thus, I don't like this.
> Either move either all or none.

I'm not moving this -- I'm removing it and using the EC version
instead.

This flag doesn't make sense for non-x86 ECs -- they don't use SCIs to
deliver EC-to-CPU communication, but initiate SPI transactions instead.

> 
> > +int olpc_ec_mask_write(u16 bits)
> > +{
> >  #ifdef CONFIG_DEBUG_FS
> > 
> >  /*
> > @@ -277,14 +369,17 @@ static int olpc_ec_probe(struct
> > platform_device *pdev)
> >         ec_priv = ec;
> >         platform_set_drvdata(pdev, ec);
> > 
> > +       /* EC version 0x5f adds support for wide SCI mask */
> > +       if (ec->version >= 0x5f) {
> > +               __be16 ec_word = cpu_to_be16(bits);
> > +
> > +               return olpc_ec_cmd(EC_WRITE_EXT_SCI_MASK, (void *)
> > &ec_word, 2,
> > +                                  NULL, 0);
> 
> I would leave it on one line.

Okay. I do agree this looks better, but was not sure how seriously to
take checkpatch.pl's warnings.

> 
> > +       } else {
> > +               unsigned char ec_byte = bits & 0xff;
> 
> You may noticed that the parameter is of type u8, which clearly makes
> & 0xff part redundant.

At least for me, the explicit mask makes this easier for me to read.

But I don't mind really. If you'd really like to see this changes I can
follow up with such change (or am happy to ack such change from you).

> 
> > +               return olpc_ec_cmd(EC_WRITE_SCI_MASK, &ec_byte, 1,
> > NULL, 0);
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(olpc_ec_mask_write);
> 
> I see that the above is inherited from older code, so, no need to
> address those comments in here, but consider a follow up fix.
> 
> 
> > +int olpc_ec_sci_query(u16 *sci_value)
> > +{
> > +}
> > +EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
> 
> Similar comments are applied here.
> 
> > +
> > -       err = ec_driver->probe ? ec_driver->probe(pdev) : 0;
> > +       /* get the EC revision */
> > +       err = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0,
> > +               (unsigned char *) &ec->version, 1);
> 
> Perhaps version should be defined as u8.

Yes.

> > +/* SCI source values */
> > +#define EC_SCI_SRC_EMPTY        0x00
> > +#define EC_SCI_SRC_GAME         0x01
> > +#define EC_SCI_SRC_BATTERY      0x02
> > +#define EC_SCI_SRC_BATSOC       0x04
> > +#define EC_SCI_SRC_BATERR       0x08
> > +#define EC_SCI_SRC_EBOOK        0x10    /* XO-1 only */
> > +#define EC_SCI_SRC_WLAN         0x20    /* XO-1 only */
> > +#define EC_SCI_SRC_ACPWR        0x40
> > +#define EC_SCI_SRC_BATCRIT      0x80
> > +#define EC_SCI_SRC_GPWAKE       0x100   /* XO-1.5 only */
> 
> BIT() ?
> 
> > +#define EC_SCI_SRC_ALL          0x1FF
> 
> GENMASK()?

Yes. I meant to move this, but it turns out I've left the original ones
in place. Will fix them in a follow-up commit also.

Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node
  2018-10-19 13:43   ` Andy Shevchenko
@ 2018-11-14 16:49     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-14 16:49 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

Hello,

On Fri, 2018-10-19 at 16:43 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > The XO-1 and XO-1.5 batteries apparently differ in an ability to
> > report
> > ambient temperature. Add a different compatible string to the 1.5
> > battery.
> > +int olpc_dt_compatible_match(phandle node, const char *compat)
> >  {
> >         char buf[64];
> > +       int plen;
> > +       char *p;
> > +       int len;
> > +
> > +       plen = olpc_dt_getproperty(node, "compatible", buf,
> > sizeof(buf));
> > +       if (plen <= 0)
> > +               return 0;
> > +
> > +       len = strlen(compat);
> > +       for (p = buf; p < buf + plen; p += strlen(p) + 1) {
> > +               if (strcmp(p, compat) == 0)
> > +                       return 1;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> DT doesn't still have a helper for that?!
> I hardly believe in that.

There indeed is of_device_compatible_match() (and
of_find_node_by_phandle() for that matter).

However, these fixups are executed before the DT is built with
of_pdt_build_devicetree(), so I don't think any of_*() routines can be
used at that point.

> > +               olpc_dt_interpret("\" /battery@0\" find-device"
> > +                       " \" olpc,xo1.5-battery\" +compatible"
> > +                       " device-end");
> 
> Please, don't split string literals.
> 
> >                 olpc_dt_interpret("\" /pci/display@1\" find-device"
> >                         " new-device"
> >                         " \" dcon\" device-name \" olpc,xo1-dcon\"
> > +compatible"
> >                         " finish-device device-end");
> 
> Yeah, this and similar also needs to be fixed.

Okay.

Thank you
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct
  2018-11-04 14:37   ` Pavel Machek
@ 2018-11-14 17:10     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-14 17:10 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

On Sun, 2018-11-04 at 15:37 +0100, Pavel Machek wrote:
> Hi!
> 
> > The global variables for private data are not too nice. I'd like some
> > more, and that would clutter the global name space even further.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Ok...
> 
> > -	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> > -	if (IS_ERR(olpc_bat)) {
> > -		ret = PTR_ERR(olpc_bat);
> > -		goto battery_failed;
> > -	}
> > +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> > +	if (IS_ERR(data->olpc_bat))
> > +		return PTR_ERR(data->olpc_bat);
> >  
> > -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	if (ret)
> > -		goto eeprom_failed;
> > +		return ret;
> >  
> > -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> > +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
> >  	if (ret)
> >  		goto error_failed;
> >  
> >  	if (olpc_ec_wakeup_available()) {
> > -		device_set_wakeup_capable(&olpc_ac->dev, true);
> > -		device_set_wakeup_capable(&olpc_bat->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_ac->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_bat->dev, true);
> >  	}
> >  
> >  	return 0;
> >  
> >  error_failed:
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -eeprom_failed:
> > -	power_supply_unregister(olpc_bat);
> > -battery_failed:
> > -	power_supply_unregister(olpc_ac);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return ret;
> >  }
> 
> ...but you are changing error handling here, which is not mentioned in
> the changelog, and I'm nut sure you got it right.
> 
> Are you sure?

I can't see what's wrong. I'll split the priv structure and devm/error
handling changes into two separate patches as you're right they indeed
are somewhat unrelated.

If v2 (tomorrow or so) will still seem wrong to you I'd be thankful if
you could elaborate a bit more.
> 
> >  static int olpc_battery_remove(struct platform_device *pdev)
> >  {
> > -	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -	power_supply_unregister(olpc_bat);
> > -	power_supply_unregister(olpc_ac);
> > +	struct olpc_battery_data *data = platform_get_drvdata(pdev);
> > +
> > +	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return 0;
> >  }
> 
> Here too.
> 									Pavel

Cheers
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info
  2018-10-19 13:50   ` Andy Shevchenko
@ 2018-11-14 17:19     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-14 17:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Fri, 2018-10-19 at 16:50 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > This wouldn't work on the DT-based ARM platform. Let's read the EC
> > version
> > directly from the EC driver instead.
> > 
> > This makes the driver no longer x86 specific.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  drivers/power/supply/Kconfig        |  2 +-
> >  drivers/power/supply/olpc_battery.c | 35 +++++++++++++++++++++--
> > ------
> >  2 files changed, 27 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/power/supply/Kconfig
> > b/drivers/power/supply/Kconfig
> > index ff6dab0bf0dd..f0361e4dd457 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -151,7 +151,7 @@ config BATTERY_PMU
> > 
> >  config BATTERY_OLPC
> >         tristate "One Laptop Per Child battery"
> > -       depends on X86_32 && OLPC
> > +       depends on OLPC
> >         help
> >           Say Y to enable support for the battery on the OLPC
> > laptop.
> > 
> > diff --git a/drivers/power/supply/olpc_battery.c
> > b/drivers/power/supply/olpc_battery.c
> > index 2a2d7cc995f0..dde9863e5c4a 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -20,8 +20,6 @@
> >  #include <linux/sched.h>
> >  #include <linux/olpc-ec.h>
> >  #include <linux/of.h>
> 
> Btw, Kconfig might miss
> depends on OF
> part.

But will it? I thought this header can be included regardless,
providing stubs that always fail instead of actual OF functionality.

It just wouldn't be too useful apart for compile-testing things.

Apart from that, CONFIG_OLPC drags in CONFIG_OF, so we're always
getting CONFIG_OF transitively.

> 
> > -#include <asm/olpc.h>
> > -
> > 
> >  #define EC_BAT_VOLTAGE 0x10    /*
> > uint16_t,    *9.76/32,    mV   */
> >  #define EC_BAT_CURRENT 0x11    /* int16_t,     *15.625/120,
> > mA   */
> > @@ -57,6 +55,7 @@ struct olpc_battery_data {
> >         struct power_supply *olpc_ac;
> >         struct power_supply *olpc_bat;
> >         char bat_serial[17];
> > +       int new_proto;
> >  };
> > 
> >  /*****************************************************************
> > ****
> > @@ -100,7 +99,7 @@ static const struct power_supply_desc
> > olpc_ac_desc = {
> >  static int olpc_bat_get_status(struct olpc_battery_data *data,
> >                 union power_supply_propval *val, uint8_t ec_byte)
> >  {
> > -       if (olpc_platform_info.ecver > 0x44) {
> > +       if (data->new_proto) {
> >                 if (ec_byte & (BAT_STAT_CHARGING |
> > BAT_STAT_TRICKLE))
> >                         val->intval = POWER_SUPPLY_STATUS_CHARGING;
> >                 else if (ec_byte & BAT_STAT_DISCHARGING)
> > @@ -608,14 +607,32 @@ static int olpc_battery_probe(struct
> > platform_device *pdev)
> >         struct power_supply_config psy_cfg = {};
> >         struct olpc_battery_data *data;
> >         uint8_t status;
> > +       unsigned char ecver[1];
> 
> isn't it simple
>  uint8_t ecver;
> ?

Yes, it's probably going to be nicer that way.

> 
> > +       int ret;
> > +
> > +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +       platform_set_drvdata(pdev, data);
> > +
> > +       /* See if the EC is already there and get the EC revision
> > */
> > +       ret = olpc_ec_cmd(EC_FIRMWARE_REV, NULL, 0, ecver,
> > ARRAY_SIZE(ecver));
> > +       if (ret) {
> > +               if (ret == -ENODEV)
> > +                       return -EPROBE_DEFER;
> 
> Yeah, exactly a question I asked somewhere in the first part of the
> series.
> 
> > +               return ret;
> > +       }
> > 
> > -       /*
> > -        * We've seen a number of EC protocol changes; this driver
> > requires
> > -        * the latest EC protocol, supported by 0x44 and above.
> > -        */
> > -       if (olpc_platform_info.ecver < 0x44) {
> > +       if (ecver[0] > 0x44) {
> > +               /* XO 1 or 1.5 with a new EC firmware. */
> > +               data->new_proto = 1;
> > +       } else if (ecver[0] < 0x44) {
> > +               /*
> > +                * We've seen a number of EC protocol changes; this
> > driver
> > +                * requires the latest EC protocol, supported by
> > 0x44 and above.
> > +                */
> >                 printk(KERN_NOTICE "OLPC EC version 0x%02x too old
> > for "
> > -                       "battery driver.\n",
> > olpc_platform_info.ecver);
> > +                       "battery driver.\n", ecver[0]);
> >                 return -ENXIO;
> >         }
> > 
> > --
> > 2.19.0
> > 

Thanks,
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
  2018-10-19 13:45   ` Andy Shevchenko
@ 2018-11-15 18:33     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-15 18:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, quozl, Sebastian Reichel, Rob Herring,
	Mark Rutland, Eric Miao, Haojian Zhuang, Daniel Mack,
	Robert Jarzmik, linux-spi, devicetree, Linux Kernel Mailing List,
	linux-arm Mailing List, Platform Driver, devel

On Fri, 2018-10-19 at 16:45 +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > Avoid using the x86 OLPC platform specific call to get the board
> > version. It won't work on FDT-based ARM MMP2 platform.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  drivers/power/supply/olpc_battery.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/power/supply/olpc_battery.c
> > b/drivers/power/supply/olpc_battery.c
> > index 5a97e42a3547..540d44bf536f 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/sched.h>
> >  #include <linux/olpc-ec.h>
> > +#include <linux/of.h>
> >  #include <asm/olpc.h>
> 
> Keep it sorted, otherwise the change is good!

Yes, but... the headers are not sorted at the moment. I'll sort the new
include before <linux/platform_device.h> so that I don't mess it up
even more, but I don't feel like just sorting everything so that I
don't obscure the actual change.


> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > 
> > @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct
> > platform_device *pdev)
> >         olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc,
> > NULL);
> >         if (IS_ERR(olpc_ac))
> >                 return PTR_ERR(olpc_ac);
> > -
> > -       if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5
> > */
> > +       if (of_property_match_string(pdev->dev.of_node,
> > "compatible",
> > +                                       "olpc,xo1.5-battery") >= 0)
> > {
> > +               /* XO-1.5 */
> >                 olpc_bat_desc.properties = olpc_xo15_bat_props;
> >                 olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo15_bat_props);
> > -       } else { /* XO-1 */
> > +       } else {
> > +               /* XO-1 */
> >                 olpc_bat_desc.properties = olpc_xo1_bat_props;
> >                 olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo1_bat_props);
> >         }
> > @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct
> > platform_device *pdev)
> > 
> >  static const struct of_device_id olpc_battery_ids[] = {
> >         { .compatible = "olpc,xo1-battery" },
> > +       { .compatible = "olpc,xo1.5-battery" },
> >         {}
> >  };
> >  MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> > --
> > 2.19.0
> > 

Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version
  2018-11-04 12:37   ` Pavel Machek
@ 2018-11-15 18:36     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-15 18:36 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

On Sun, 2018-11-04 at 13:37 +0100, Pavel Machek wrote:
> On Wed 2018-10-10 19:22:57, Lubomir Rintel wrote:
> > Avoid using the x86 OLPC platform specific call to get the board
> > version. It won't work on FDT-based ARM MMP2 platform.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> AFAICT, this should go earlier in the series; first, add support in
> the code, then switch to new name in DTS.

I think it's all right this way: I'm not removing the old compatible
string -- functionality provided by one version of the battery is a
superset of the another.

Also, the older version of the driver with only the XO-1 compatible
string guesses the actual battery version by querying the board version
directly, so up to this patch things keep working the way they used to
regardless of the new compatible string.

Lubo

> 								Pavel
> 
> > ---
> >  drivers/power/supply/olpc_battery.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/power/supply/olpc_battery.c
> > b/drivers/power/supply/olpc_battery.c
> > index 5a97e42a3547..540d44bf536f 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/sched.h>
> >  #include <linux/olpc-ec.h>
> > +#include <linux/of.h>
> >  #include <asm/olpc.h>
> >  
> >  
> > @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct
> > platform_device *pdev)
> >  	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc,
> > NULL);
> >  	if (IS_ERR(olpc_ac))
> >  		return PTR_ERR(olpc_ac);
> > -
> > -	if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> > +	if (of_property_match_string(pdev->dev.of_node, "compatible",
> > +					"olpc,xo1.5-battery") >= 0) {
> > +		/* XO-1.5 */
> >  		olpc_bat_desc.properties = olpc_xo15_bat_props;
> >  		olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo15_bat_props);
> > -	} else { /* XO-1 */
> > +	} else {
> > +		/* XO-1 */
> >  		olpc_bat_desc.properties = olpc_xo1_bat_props;
> >  		olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo1_bat_props);
> >  	}
> > @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct
> > platform_device *pdev)
> >  
> >  static const struct of_device_id olpc_battery_ids[] = {
> >  	{ .compatible = "olpc,xo1-battery" },
> > +	{ .compatible = "olpc,xo1.5-battery" },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, olpc_battery_ids);

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular"
  2018-11-02 22:16   ` Pavel Machek
@ 2018-11-15 18:57     ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-15 18:57 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Mark Brown, Geert Uytterhoeven, Darren Hart, Andy Shevchenko,
	Greg Kroah-Hartman, James Cameron, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree, linux-kernel,
	linux-arm-kernel, platform-driver-x86, devel, linux-pm

On Fri, 2018-11-02 at 23:16 +0100, Pavel Machek wrote:
> On Wed 2018-10-10 19:22:47, Lubomir Rintel wrote:
> > It doesn't make sense to always have this built-in, e.g. on ARM
> > multiplatform kernels. A better way to address the problem the
> > original
> > commit aimed to solve is to fix Kconfig.
> > 
> > This reverts commit f48d1496b8537d75776478c6942dd87f34d7f270.
> 
> This looks ok, but I don't see the Kconfig fix in the series. Is it
> needed?

It's flipped to a tristate in "Platform: OLPC: Move OLPC config symbol
out of x86 tree" and mentioned in the commit message.

Perhaps I could made that clear by separating the change into a
separate patch. I'm keeping it as it is in next version of the patch
set, but I'm going to improve this one's commit message and sort the
patches closer together.

> 							Pavel

Lubo								

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-11-13 17:26     ` Lubomir Rintel
  2018-11-13 22:06       ` James Cameron
@ 2018-11-19 10:40       ` Pavel Machek
  2018-11-19 13:25         ` Lubomir Rintel
  1 sibling, 1 reply; 62+ messages in thread
From: Pavel Machek @ 2018-11-19 10:40 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Andy Shevchenko, Mark Brown, Geert Uytterhoeven, Darren Hart,
	Andy Shevchenko, Greg Kroah-Hartman, quozl, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree,
	Linux Kernel Mailing List, linux-arm Mailing List,
	Platform Driver

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

Hi!

> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/olpc-ec.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kfifo.h>
> > > +#include <linux/module.h>
> > > +#include <linux/power_supply.h>
> > 
> > Easy to maintain when it's sorted.

No / it depends.

> > > +       channel = priv->rx_buf[0];
> > > +       byte = priv->rx_buf[1];
> > 
> > Maybe specific structures would fit better?
> > 
> > Like
> > 
> > struct olpc_ec_resp_hdr {
> >  u8 channel;
> >  u8 byte;
> > ...
> > }

Structures have padding and other nastyness...

> > > +                       pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> > 
> > Magic number.

Nothing wrong with magic numbers.

> > > +       args[0] = mask & 0xff;
> > > +       args[1] = (mask >> 8) & 0xff;
> > 
> > ...mask >> 0;
> > ...mask >> 8;

No, please.

> > __maybe_unused  instead of ugly #ifdef?
> > 
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(dev);
> > > +       struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata() or how is it called?
> > 
> > > +       unsigned char hintargs[5];
> > 
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> > 
> > ?

No, unless you want to break the code. Or add __attribute__ packed and
deal with endianness.

Just no.

> > > +       static unsigned int suspend_count;
> > 
> > u32 I suppose.

You know, there's semantic difference between unsigned int and
u32. And this sounds like candidate for unsigned int.

> > > +       /* Enable all EC events while we're awake */
> > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > 
> > #define EC_ALL_EVENTS GENMASK(15, 0)

Actually that's less readable. Just don't.

> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > +       { .compatible = "olpc,xo1.75-ec" },
> > > +       { },
> > 
> > No comma for terminators.

Comma is fine.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver
  2018-11-19 10:40       ` Pavel Machek
@ 2018-11-19 13:25         ` Lubomir Rintel
  0 siblings, 0 replies; 62+ messages in thread
From: Lubomir Rintel @ 2018-11-19 13:25 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andy Shevchenko, Mark Brown, Geert Uytterhoeven, Darren Hart,
	Andy Shevchenko, Greg Kroah-Hartman, quozl, Sebastian Reichel,
	Rob Herring, Mark Rutland, Eric Miao, Haojian Zhuang,
	Daniel Mack, Robert Jarzmik, linux-spi, devicetree,
	Linux Kernel Mailing List, linux-arm Mailing List,
	Platform Driver

Hi Pavel,

I've addressed some of Andy's concerns you're replying to below in a
follow-up version of the patch. To many points I'm generally
indifferent and prefer to lean towards making things easy for whoever
may be likely to deal with the code in past. It's not clear to me who
that might be, or how important either of you consider your remarks to
be. I'll be thankful if anyone makes this clearer to me.

(I hoped that you're in the Cc list; thought git send-mail will see
your address in the Acked-by tags and add you. I failed to notice that
it's not the case. I apologize. Here's the v2 posting: [1].)

[1] https://lore.kernel.org/lkml/20181116162403.49854-1-lkundrak@v3.sk/


On Mon, 2018-11-19 at 11:40 +0100, Pavel Machek wrote:
> Hi!
> 
> > > > +#include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/ctype.h>
> > > > +#include <linux/olpc-ec.h>
> > > > +#include <linux/spi/spi.h>
> > > > +#include <linux/reboot.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/kfifo.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/power_supply.h>
> > > 
> > > Easy to maintain when it's sorted.
> 
> No / it depends.

I've sorted it in the new files; following a rule of keeping things
sorted at the very least has an advantage of being unambiguous.

I'm not sorting things where they currently are not in order, since it
would obscure real changes.

> > > > +       channel = priv->rx_buf[0];
> > > > +       byte = priv->rx_buf[1];
> > > 
> > > Maybe specific structures would fit better?
> > > 
> > > Like
> > > 
> > > struct olpc_ec_resp_hdr {
> > >  u8 channel;
> > >  u8 byte;
> > > ...
> > > }
> 
> Structures have padding and other nastyness...

I've turned them into structs. It perhaps looks a bit better -- the
structure members serving as a documentation for the protocol.

> > > > +                       pm_wakeup_event(priv->pwrbtn-
> > > > >dev.parent,
> > > > 1000);
> > > 
> > > Magic number.
> 
> Nothing wrong with magic numbers.

Turned it into a define, but it's not like it's any clearer why that
particular value works...

I actually have little idea. I've copied this from the vendor's
original driver.

> > > > +       args[0] = mask & 0xff;
> > > > +       args[1] = (mask >> 8) & 0xff;
> > > 
> > > ...mask >> 0;
> > > ...mask >> 8;
> 
> No, please.

I've done this too, but I don't see much point either. Perhaps those
within the OCD spectrum may appreciate :)

> > > __maybe_unused  instead of ugly #ifdef?
> > > 
> > > > +{
> > > > +       struct platform_device *pdev = to_platform_device(dev);
> > > > +       struct olpc_xo175_ec *priv =
> > > > platform_get_drvdata(pdev);
> > > 
> > > dev_get_drvdata() or how is it called?
> > > 
> > > > +       unsigned char hintargs[5];
> > > 
> > > struct olpc_ec_hint_cmd {
> > > u8 ...
> > > u32 ...
> > > };
> > > 
> > > ?
> 
> No, unless you want to break the code. Or add __attribute__ packed
> and
> deal with endianness.
> 
> Just no.

Turned it into a packed struct too for the same reasons as above.

The endianness of the hints argument doesn't actually matter, since the
EC firmware actually uses this in a debugging printf. It's not even
cleat what endianness did EC expect initially.

> > > > +       static unsigned int suspend_count;
> > > 
> > > u32 I suppose.
> 
> You know, there's semantic difference between unsigned int and
> u32. And this sounds like candidate for unsigned int.
> 
> > > > +       /* Enable all EC events while we're awake */
> > > > +       olpc_xo175_ec_set_event_mask(0xffff);
> > > 
> > > #define EC_ALL_EVENTS GENMASK(15, 0)
> 
> Actually that's less readable. Just don't.

Done this too, and, like the points above, I am indifferent too. Both
seem equal to me.

> > > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > > +       { .compatible = "olpc,xo1.75-ec" },
> > > > +       { },
> > > 
> > > No comma for terminators.
> 
> Comma is fine.

Removed it. Perhaps the lack of a comma could be understood as an
indication that no further initializers shall follow.

> 	Pavel

Just in case I didn't stress that enough: I'm much more interested in
things working well than style issues that too often are just matter of
tast. I happily accept advice on those though.

Take care,
Lubo

^ permalink raw reply	[flat|nested] 62+ messages in thread

end of thread, other threads:[~2018-11-19 13:25 UTC | newest]

Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 17:22 [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Lubomir Rintel
2018-10-10 17:22 ` [PATCH 01/15] power: supply: olpc_battery: correct the temperature units Lubomir Rintel
2018-10-19 13:00   ` Andy Shevchenko
2018-10-21 21:27     ` Sebastian Reichel
2018-11-02 22:16   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 02/15] Revert "platform/olpc: Make ec explicitly non-modular" Lubomir Rintel
2018-10-19 13:04   ` Andy Shevchenko
2018-11-02 22:16   ` Pavel Machek
2018-11-15 18:57     ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 03/15] dt-bindings: olpc,xo1.75-ec: Add OLPC XO-1.75 EC bindings Lubomir Rintel
2018-10-17 19:38   ` Rob Herring
2018-11-04 12:26   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 04/15] Platform: OLPC: Remove an unused include Lubomir Rintel
2018-10-19 13:05   ` Andy Shevchenko
2018-11-13 13:59     ` Lubomir Rintel
2018-11-04 12:26   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 05/15] Platform: OLPC: Move OLPC config symbol out of x86 tree Lubomir Rintel
2018-10-19 13:09   ` Andy Shevchenko
2018-11-04 12:27   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver Lubomir Rintel
2018-10-19 16:06   ` Andy Shevchenko
2018-11-13 17:26     ` Lubomir Rintel
2018-11-13 22:06       ` James Cameron
2018-11-19 10:40       ` Pavel Machek
2018-11-19 13:25         ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 07/15] Platform: OLPC: Avoid a warning if the EC didn't register yet Lubomir Rintel
2018-10-19 13:11   ` Andy Shevchenko
2018-11-04 12:30   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 08/15] Platform: OLPC: Move EC-specific functionality out from x86 Lubomir Rintel
2018-10-19 13:36   ` Andy Shevchenko
2018-11-14 16:20     ` Lubomir Rintel
2018-11-04 12:31   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 09/15] Platform: OLPC: add a regulator for the DCON Lubomir Rintel
2018-10-19 13:39   ` Andy Shevchenko
2018-10-10 17:22 ` [PATCH 10/15] dt-bindings: olpc_battery: Add XO-1.5 battery Lubomir Rintel
2018-10-17 19:38   ` Rob Herring
2018-11-04 12:32   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 11/15] x86, olpc: Use a correct version when making up a battery node Lubomir Rintel
2018-10-19 13:43   ` Andy Shevchenko
2018-11-14 16:49     ` Lubomir Rintel
2018-11-04 12:34   ` Pavel Machek
2018-10-10 17:22 ` [PATCH 12/15] power: supply: olpc_battery: Use DT to get battery version Lubomir Rintel
2018-10-19 13:45   ` Andy Shevchenko
2018-11-15 18:33     ` Lubomir Rintel
2018-11-04 12:37   ` Pavel Machek
2018-11-15 18:36     ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 13/15] power: supply: olpc_battery: Move priv data to a struct Lubomir Rintel
2018-10-19 13:48   ` Andy Shevchenko
2018-11-04 14:37   ` Pavel Machek
2018-11-14 17:10     ` Lubomir Rintel
2018-10-10 17:22 ` [PATCH 14/15] power: supply: olpc_battery: Avoid using platform_info Lubomir Rintel
2018-10-19 13:50   ` Andy Shevchenko
2018-11-14 17:19     ` Lubomir Rintel
2018-11-04 14:39   ` Pavel Machek
2018-10-10 17:23 ` [PATCH 15/15] power: supply: olpc_battery: Add OLPC XO 1.75 support Lubomir Rintel
2018-10-19 13:55   ` Andy Shevchenko
2018-11-04 14:39   ` Pavel Machek
2018-10-10 19:26 ` [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller Rob Herring
2018-10-11 10:42   ` Lubomir Rintel
2018-10-19 13:57 ` Andy Shevchenko
2018-10-23 17:03   ` Lubomir Rintel
2018-10-21 21:45 ` Sebastian Reichel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).