[PATCH v6 0/2] mfd: Add ENE KB3930 Embedded Controller driver

[PATCH v6 0/2] mfd: Add ENE KB3930 Embedded Controller driver

[Lubomir Rintel]

Hi,

please consider applying the patches chained to this message. It's the
sixth version of the driver for the ENE KB3930 Embedded Controller.

This addresses responses to the fifth version of the set.
Detailed change logs are in the individual patch descriptions.

One thing that I was asked to do in the review of the previous version
that I didn't do is that I didn't expand the hardware vendor name,
"ENE". It's not an abbreviation.

Thanks,
Lubo

[PATCH v6 1/2] dt-bindings: mfd: Add ENE KB3930 Embedded Controller binding

[Lubomir Rintel]

Add binding document for the ENE KB3930 Embedded Controller.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Rob Herring <robh@kernel.org>

---
Changes since v5:
- s/a I2C bus/an I2C bus/

Changes since v4:
- Collected Rob's Reviewed-by

Changes since v1:
- Addressed binding validation failure
---
 .../devicetree/bindings/mfd/ene-kb3930.yaml   | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/ene-kb3930.yaml

diff --git a/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
new file mode 100644
index 0000000000000..074243c408910
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ene-kb3930.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ene-kb3930.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ENE KB3930 Embedded Controller bindings
+
+description: |
+  This binding describes the ENE KB3930 Embedded Controller attached to an
+  I2C bus.
+
+maintainers:
+  - Lubomir Rintel <lkundrak@v3.sk>
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - dell,wyse-ariel-ec  # Dell Wyse Ariel board (3020)
+      - const: ene,kb3930
+  reg:
+    maxItems: 1
+
+  off-gpios:
+    description: GPIO used with the shutdown protocol on Ariel
+    maxItems: 2
+
+  system-power-controller: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      embedded-controller@58 {
+        compatible = "dell,wyse-ariel-ec", "ene,kb3930";
+        reg = <0x58>;
+        system-power-controller;
+
+        off-gpios = <&gpio 126 GPIO_ACTIVE_HIGH>,
+                    <&gpio 127 GPIO_ACTIVE_HIGH>;
+      };
+    };
+
+...
-- 
2.26.2

[PATCH v6 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller

[Lubomir Rintel]

This driver provides access to the EC RAM of said embedded controller
attached to the I2C bus as well as optionally supporting its slightly weird
power-off/restart protocol.

A particular implementation of the EC firmware can be identified by a
model byte. If this driver identifies the Dell Ariel platform, it
registers the appropriate cells.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v5:
- Clarify the command in kb3930_off()
- Remove an extraneous comment above the call to devm_mfd_add_devices()
- s/pm_power_off == NULL/!pm_power_off/

Changes since v3:
- Clarify that the power-off function is provided by this driver while
  LEDS_ARIEL is necessary to drive the leds in Kconfig help text
- s/kb3930_power_off/kb3930_pm_power_off/, s/global_kb3930/kb3930_power_off/
- Don't fail with -EEXIST with more than one instance
- s/ret < 0/ret/ for devm_mfd_add_devices() return value
- Drop a \n between struct i2c_driver and module_i2c_driver()

Changes since v2:
- Sort the includes
- s/EC_MODEL_ID/EC_MODEL/
- Add a couple of clarifying comments
- Use #defines for values used in poweroff routine
- Remove priority from a restart notifier block
- s/priv/ddata/
- s/ec_ram/ram_regmap/ for the regmap name
- Fix the error handling when getting off gpios was not successful
- Remove a useless dev_info at the end of probe()
- Use i2c probe_new() callback, drop i2c_device_id
- Modify the logic in checking the model ID
---
 drivers/mfd/Kconfig      |  11 ++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/ene-kb3930.c | 212 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/mfd/ene-kb3930.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index a37d7d1713820..7569f1d1703e3 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -398,6 +398,17 @@ config MFD_DLN2
 	  etc. must be enabled in order to use the functionality of
 	  the device.
 
+config MFD_ENE_KB3930
+	tristate "ENE KB3930 Embedded Controller support"
+	depends on I2C
+	depends on MACH_MMP3_DT || COMPILE_TEST
+	select MFD_CORE
+	help
+	  This adds support for the power-off functionality and access to
+	  the registers that control LEDS and USB port power on ENE KB3930
+	  Embedded Controller. To use the LED functionality LEDS_ARIEL must
+	  be enabled.
+
 config MFD_EXYNOS_LPASS
 	tristate "Samsung Exynos SoC Low Power Audio Subsystem"
 	depends on ARCH_EXYNOS || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9367a92f795a6..11085a6968398 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
 obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
+obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
 obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
 
diff --git a/drivers/mfd/ene-kb3930.c b/drivers/mfd/ene-kb3930.c
new file mode 100644
index 0000000000000..1c32ff586816b
--- /dev/null
+++ b/drivers/mfd/ene-kb3930.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
+/*
+ * ENE KB3930 Embedded Controller Driver
+ *
+ * Copyright (C) 2020 Lubomir Rintel
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+
+/* I2C registers that are multiplexing access to the EC RAM. */
+enum {
+	EC_DATA_IN	= 0x00,
+	EC_RAM_OUT	= 0x80,
+	EC_RAM_IN	= 0x81,
+};
+
+/* EC RAM registers. */
+enum {
+	EC_MODEL	= 0x30,
+	EC_VERSION_MAJ	= 0x31,
+	EC_VERSION_MIN	= 0x32,
+};
+
+struct kb3930 {
+	struct i2c_client *client;
+	struct regmap *ram_regmap;
+	struct gpio_descs *off_gpios;
+};
+
+struct kb3930 *kb3930_power_off;
+
+#define EC_GPIO_WAVE		0
+#define EC_GPIO_OFF_MODE	1
+
+#define EC_OFF_MODE_REBOOT	0
+#define EC_OFF_MODE_POWER	1
+
+static void kb3930_off(struct kb3930 *ddata, int off_mode)
+{
+	gpiod_direction_output(ddata->off_gpios->desc[EC_GPIO_OFF_MODE],
+			       off_mode);
+
+	/*
+	 * This creates a 10 Hz wave on EC_GPIO_WAVE that signals a
+	 * shutdown request to the EC. Once the EC detects it, it will
+	 * proceed to turn the power off or reset the board depending on
+	 * the value of EC_GPIO_OFF_MODE.
+	 */
+	while (1) {
+		mdelay(50);
+		gpiod_direction_output(ddata->off_gpios->desc[EC_GPIO_WAVE], 0);
+		mdelay(50);
+		gpiod_direction_output(ddata->off_gpios->desc[EC_GPIO_WAVE], 1);
+	}
+}
+
+static int kb3930_restart(struct notifier_block *this,
+			  unsigned long mode, void *cmd)
+{
+	kb3930_off(kb3930_power_off, EC_OFF_MODE_REBOOT);
+	return NOTIFY_DONE;
+}
+
+static void kb3930_pm_power_off(void)
+{
+	kb3930_off(kb3930_power_off, EC_OFF_MODE_POWER);
+}
+
+static struct notifier_block kb3930_restart_nb = {
+	.notifier_call = kb3930_restart,
+};
+
+static const struct mfd_cell ariel_ec_cells[] = {
+	{ .name = "dell-wyse-ariel-led", },
+	{ .name = "dell-wyse-ariel-power", },
+};
+
+static int kb3930_ec_ram_reg_write(void *context, unsigned int reg,
+				   unsigned int val)
+{
+	struct kb3930 *ddata = context;
+
+	return i2c_smbus_write_word_data(ddata->client, EC_RAM_OUT,
+					 (val << 8) | reg);
+}
+
+static int kb3930_ec_ram_reg_read(void *context, unsigned int reg,
+				  unsigned int *val)
+{
+	struct kb3930 *ddata = context;
+	int ret;
+
+	ret = i2c_smbus_write_word_data(ddata->client, EC_RAM_IN, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_word_data(ddata->client, EC_DATA_IN);
+	if (ret < 0)
+		return ret;
+
+	*val = ret >> 8;
+	return 0;
+}
+
+static const struct regmap_config kb3930_ram_regmap_config = {
+	.name = "ec_ram",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_register = 0xff,
+	.reg_write = kb3930_ec_ram_reg_write,
+	.reg_read = kb3930_ec_ram_reg_read,
+	.fast_io = false,
+};
+
+static int kb3930_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node;
+	struct kb3930 *ddata;
+	unsigned int model;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	kb3930_power_off = ddata;
+	ddata->client = client;
+	i2c_set_clientdata(client, ddata);
+
+	ddata->ram_regmap = devm_regmap_init(dev, NULL, ddata,
+					     &kb3930_ram_regmap_config);
+	if (IS_ERR(ddata->ram_regmap))
+		return PTR_ERR(ddata->ram_regmap);
+
+	ret = regmap_read(ddata->ram_regmap, EC_MODEL, &model);
+	if (ret < 0)
+		return ret;
+
+	/* Currently we only support the cells present on Dell Ariel model. */
+	if (model != 'J') {
+		dev_err(dev, "unknown board model: %02x\n", model);
+		return -ENODEV;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				   ariel_ec_cells,
+				   ARRAY_SIZE(ariel_ec_cells),
+				   NULL, 0, NULL);
+	if (ret)
+		return ret;
+
+	if (of_property_read_bool(np, "system-power-controller")) {
+		ddata->off_gpios =
+			devm_gpiod_get_array_optional(dev, "off", GPIOD_IN);
+		if (IS_ERR(ddata->off_gpios))
+			return PTR_ERR(ddata->off_gpios);
+		if (ddata->off_gpios->ndescs < 2) {
+			dev_err(dev, "invalid off-gpios property\n");
+			return -EINVAL;
+		}
+	}
+
+	if (ddata->off_gpios) {
+		register_restart_handler(&kb3930_restart_nb);
+		if (!pm_power_off)
+			pm_power_off = kb3930_pm_power_off;
+	}
+
+	return 0;
+}
+
+static int kb3930_remove(struct i2c_client *client)
+{
+	struct kb3930 *ddata = i2c_get_clientdata(client);
+
+	if (ddata->off_gpios) {
+		if (pm_power_off == kb3930_pm_power_off)
+			pm_power_off = NULL;
+		unregister_restart_handler(&kb3930_restart_nb);
+	}
+	kb3930_power_off = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id kb3930_dt_ids[] = {
+	{ .compatible = "ene,kb3930" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, kb3930_dt_ids);
+
+static struct i2c_driver kb3930_driver = {
+	.probe_new = kb3930_probe,
+	.remove = kb3930_remove,
+	.driver = {
+		.name = "ene-kb3930",
+		.of_match_table = of_match_ptr(kb3930_dt_ids),
+	},
+};
+module_i2c_driver(kb3930_driver);
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("ENE KB3930 Embedded Controller Driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.26.2

Re: [PATCH v6 1/2] dt-bindings: mfd: Add ENE KB3930 Embedded Controller binding

[Dmitry Osipenko]

10.07.2020 02:12, Lubomir Rintel пишет:
> Add binding document for the ENE KB3930 Embedded Controller.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes since v5:
> - s/a I2C bus/an I2C bus/
> 
> Changes since v4:
> - Collected Rob's Reviewed-by
> 
> Changes since v1:
> - Addressed binding validation failure
> ---

Hello! I'm happy that I noted this patch on the ML because I'm preparing
patches that will add support for KB930, which is compatible with
KB3930. Will be nice to see this patch applied ASAP! I'm going to extend
this binding with the KB930.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

Re: [PATCH v6 1/2] dt-bindings: mfd: Add ENE KB3930 Embedded Controller binding

[Lee Jones]

On Fri, 10 Jul 2020, Lubomir Rintel wrote:

> Add binding document for the ENE KB3930 Embedded Controller.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> Changes since v5:
> - s/a I2C bus/an I2C bus/
> 
> Changes since v4:
> - Collected Rob's Reviewed-by
> 
> Changes since v1:
> - Addressed binding validation failure
> ---
>  .../devicetree/bindings/mfd/ene-kb3930.yaml   | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/ene-kb3930.yaml

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller

[Lee Jones]

On Fri, 10 Jul 2020, Lubomir Rintel wrote:

> This driver provides access to the EC RAM of said embedded controller
> attached to the I2C bus as well as optionally supporting its slightly weird
> power-off/restart protocol.
> 
> A particular implementation of the EC firmware can be identified by a
> model byte. If this driver identifies the Dell Ariel platform, it
> registers the appropriate cells.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v5:
> - Clarify the command in kb3930_off()
> - Remove an extraneous comment above the call to devm_mfd_add_devices()
> - s/pm_power_off == NULL/!pm_power_off/
> 
> Changes since v3:
> - Clarify that the power-off function is provided by this driver while
>   LEDS_ARIEL is necessary to drive the leds in Kconfig help text
> - s/kb3930_power_off/kb3930_pm_power_off/, s/global_kb3930/kb3930_power_off/
> - Don't fail with -EEXIST with more than one instance
> - s/ret < 0/ret/ for devm_mfd_add_devices() return value
> - Drop a \n between struct i2c_driver and module_i2c_driver()
> 
> Changes since v2:
> - Sort the includes
> - s/EC_MODEL_ID/EC_MODEL/
> - Add a couple of clarifying comments
> - Use #defines for values used in poweroff routine
> - Remove priority from a restart notifier block
> - s/priv/ddata/
> - s/ec_ram/ram_regmap/ for the regmap name
> - Fix the error handling when getting off gpios was not successful
> - Remove a useless dev_info at the end of probe()
> - Use i2c probe_new() callback, drop i2c_device_id
> - Modify the logic in checking the model ID
> ---
>  drivers/mfd/Kconfig      |  11 ++
>  drivers/mfd/Makefile     |   1 +
>  drivers/mfd/ene-kb3930.c | 212 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/mfd/ene-kb3930.c

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog