[PATCH v2 00/10] Netronix embedded controller driver for Kobo and Tolino ebook readers

[PATCH v2 00/10] Netronix embedded controller driver for Kobo and Tolino ebook readers

[Jonathan Neuschäfer]

This patchset adds basic support for the embedded controller found on
older ebook reader boards designed by/with the ODM Netronix Inc.[1] and
sold by Kobo or Tolino, for example the Kobo Aura and the Tolino Shine.
These drivers are based on information contained in the vendor kernel
sources, but in order to all information in a single place, I documented
the register interface of the EC on GitHub[2].

As previously, I'm not sure I got the YAML DT bindings right. I have
included the plain text DT bindings for reference, in the patch
descriptions where they are relevant.

[1]: http://www.netronixinc.com/products.aspx?ID=1
[2]: https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller


Changes in v2:
- Moved txt DT bindings to patch descriptions and removed patch 1/10
  "DT bindings in plain text format"
- New patch 7/10 "rtc: Introduce RTC_TIMESTAMP_END_2255"
- Rebased on 5.9-rc3
- Various other changes which are documented in each patch

v1:
- https://lore.kernel.org/lkml/20200620223915.1311485-1-j.neuschaefer@gmx.net/


Jonathan Neuschäfer (10):
  dt-bindings: Add vendor prefix for Netronix, Inc.
  dt-bindings: mfd: Add binding for Netronix's embedded controller
  mfd: Add base driver for Netronix embedded controller
  dt-bindings: pwm: Add bindings for PWM function in Netronix EC
  pwm: ntxec: Add driver for PWM function in Netronix EC
  dt-bindings: rtc: Add bindings for Netronix embedded controller RTC
  rtc: Introduce RTC_TIMESTAMP_END_2255
  rtc: New driver for RTC in Netronix embedded controller
  MAINTAINERS: Add entry for Netronix embedded controller
  ARM: dts: imx50-kobo-aura: Add Netronix embedded controller

 .../bindings/mfd/netronix,ntxec.yaml          |  83 +++++++
 .../bindings/pwm/netronix,ntxec-pwm.yaml      |  33 +++
 .../bindings/rtc/netronix,ntxec-rtc.yaml      |  27 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |  11 +
 arch/arm/boot/dts/imx50-kobo-aura.dts         |  27 ++-
 drivers/mfd/Kconfig                           |   7 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/ntxec.c                           | 216 ++++++++++++++++++
 drivers/pwm/Kconfig                           |   8 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ntxec.c                       | 160 +++++++++++++
 drivers/rtc/Kconfig                           |   8 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-ntxec.c                       | 130 +++++++++++
 include/linux/mfd/ntxec.h                     |  24 ++
 include/linux/rtc.h                           |   1 +
 17 files changed, 739 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
 create mode 100644 Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
 create mode 100644 drivers/mfd/ntxec.c
 create mode 100644 drivers/pwm/pwm-ntxec.c
 create mode 100644 drivers/rtc/rtc-ntxec.c
 create mode 100644 include/linux/mfd/ntxec.h

--
2.28.0

[PATCH v2 01/10] dt-bindings: Add vendor prefix for Netronix, Inc.

[Jonathan Neuschäfer]

Netronix, Inc. (http://www.netronixinc.com/) makes ebook reader board
designs, which are for example used in Kobo and Tolino devices.

An alternative prefix for Netronix would be "ntx", which is already used
in code released by Netronix. It is shorter, but perhaps less clear.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
v2:
- No changes
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 63996ab035217..fa173802000a0 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -712,6 +712,8 @@ patternProperties:
     description: Broadcom Corporation (formerly NetLogic Microsystems)
   "^netron-dy,.*":
     description: Netron DY
+  "^netronix,.*":
+    description: Netronix, Inc.
   "^netxeon,.*":
     description: Shenzhen Netxeon Technology CO., LTD
   "^neweast,.*":
--
2.28.0

[PATCH v2 02/10] dt-bindings: mfd: Add binding for Netronix's embedded controller

[Jonathan Neuschäfer]

This EC is found in e-book readers of multiple brands (e.g. Kobo,
Tolino), and is typically implemented as a TI MSP430 microcontroller.

It controls different functions of the system, such as power on/off,
RTC, PWM for the backlight. The exact functionality provided can vary
between boards.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
v2:
- Add the plaintext DT binding for comparison


For reference, here is the binding in text form:

  Netronix Embedded Controller

  This EC is found in e-book readers of multiple brands (e.g. Kobo, Tolino), and
  is typically implemented as a TI MSP430 microcontroller.


  Required properties:
  - compatible: should be "netronix,ntxec"
  - reg: The I2C address of the EC

  Optional properties:
  - system-power-controller:
    See Documentation/devicetree/bindings/power/power-controller.txt
  - interrupts or interrupts-extended
  - interrupt-controller
  - #interrupt-cells: Should be 1

  Optional subnodes:

  Sub-nodes are identified by their compatible string.

   compatible string              | description
  --------------------------------|--------------------------------------
   netronix,ntxec-pwm             | PWM (used for backlight)
   netronix,ntxec-rtc             | real time clock


  Example:

  &i2c3 {
  	pinctrl-names = "default";
  	pinctrl-0 = <&pinctrl_i2c3>;
  	status = "okay";

  	ec: embedded-controller@43 {
  		pinctrl-names = "default";
  		pinctrl-0 = <&pinctrl_ntxec>;

  		compatible = "netronix,ntxec";
  		reg = <0x43>;
  		system-power-controller;
  		interrupt-parent = <&gpio4>;
  		interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
  		interrupt-controller;
  		#interrupt-cells = <1>;

  		pwm {
  			compatible = "netronix,ntxec-pwm";
  			#pwm-cells = <2>;
  		};

  		rtc {
  			compatible = "netronix,ntxec-rtc";
  			interrupts-extended = <&ec 15>;
  			interrupt-names = "alarm";
  		};
  	};
  };
---
 .../bindings/mfd/netronix,ntxec.yaml          | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml

diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
new file mode 100644
index 0000000000000..596df460f98eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/netronix,ntxec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Netronix Embedded Controller
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description: |
+  This EC is found in e-book readers of multiple brands (e.g. Kobo, Tolino), and
+  is typically implemented as a TI MSP430 microcontroller.
+
+
+properties:
+  compatible:
+    const: netronix,ntxec
+
+  reg:
+    items:
+      - description: The I2C address of the EC
+
+  system-power-controller:
+    type: boolean
+    description: See Documentation/devicetree/bindings/power/power-controller.txt
+
+  interrupts:
+    minItems: 1
+    description:
+      The EC can signal interrupts via a GPIO line
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            ec: embedded-controller@43 {
+                    pinctrl-names = "default";
+                    pinctrl-0 = <&pinctrl_ntxec>;
+
+                    compatible = "netronix,ntxec";
+                    reg = <0x43>;
+                    system-power-controller;
+                    interrupt-parent = <&gpio4>;
+                    interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
+                    interrupt-controller;
+                    #interrupt-cells = <1>;
+            };
+    };
--
2.28.0

[PATCH v2 03/10] mfd: Add base driver for Netronix embedded controller

[Jonathan Neuschäfer]

The Netronix embedded controller is a microcontroller found in some
e-book readers designed by the ODM Netronix, Inc. It contains RTC,
battery monitoring, system power management, and PWM functionality.

This driver implements register access and version detection.

Third-party hardware documentation is available at:

  https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller

The EC supports interrupts, but the driver doesn't make use of them so
far.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Add a description of the device to the patch text
- Unify spelling as 'Netronix embedded controller'.
  'Netronix' is the proper name of the manufacturer, but 'embedded controller'
  is just a label that I have assigned to the device.
- Switch to regmap, avoid regmap use in poweroff and reboot handlers.
  Inspired by cf84dc0bb40f4 ("mfd: rn5t618: Make restart handler atomic safe")
- Use a list of known-working firmware versions instead of checking for a
  known-incompatible version
- Prefix registers with NTXEC_REG_
- Define register values as constants
- Various style cleanups as suggested by Lee Jones
- Don't align = signs in struct initializers [Uwe Kleine-König]
- Don't use dev_dbg for an error message
- Explain sleep in poweroff handler
- Remove (struct ntxec).client
- Switch to .probe_new in i2c driver
- Add .remove callback
- Make CONFIG_MFD_NTXEC a tristate option
---
 drivers/mfd/Kconfig       |   7 ++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/ntxec.c       | 217 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ntxec.h |  24 +++++
 4 files changed, 249 insertions(+)
 create mode 100644 drivers/mfd/ntxec.c
 create mode 100644 include/linux/mfd/ntxec.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df0837ab415..bab999081f32d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -978,6 +978,13 @@ config MFD_VIPERBOARD
 	  You need to select the mfd cell drivers separately.
 	  The drivers do not support all features the board exposes.

+config MFD_NTXEC
+	tristate "Netronix embedded controller"
+	depends on I2C && OF
+	help
+	  Say yes here if you want to support the embedded controller found in
+	  certain e-book readers designed by the ODM Netronix.
+
 config MFD_RETU
 	tristate "Nokia Retu and Tahvo multi-function device"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f835283e..236a8acd917a0 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -217,6 +217,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
+obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
 obj-$(CONFIG_MFD_RK808)		+= rk808.o
 obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
new file mode 100644
index 0000000000000..49cc4fa35b9fe
--- /dev/null
+++ b/drivers/mfd/ntxec.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements register access, version detection, and system
+ * power-off/reset.
+ *
+ * Copyright 2020 Jonathan Neuschäfer
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#define NTXEC_REG_VERSION	0x00
+#define NTXEC_REG_POWEROFF	0x50
+#define NTXEC_REG_POWERKEEP	0x70
+#define NTXEC_REG_RESET		0x90
+
+#define NTXEC_POWEROFF_VALUE	0x0100
+#define NTXEC_POWERKEEP_VALUE	0x0800
+#define NTXEC_RESET_VALUE	0xff00
+
+static struct i2c_client *poweroff_restart_client;
+
+static void ntxec_poweroff(void)
+{
+	int res;
+
+	u8 buf[] = {
+		NTXEC_REG_POWEROFF,
+		(NTXEC_POWEROFF_VALUE >> 8) & 0xff,
+		NTXEC_POWEROFF_VALUE & 0xff,
+	};
+
+	struct i2c_msg msgs[] = {
+		{
+			.addr = poweroff_restart_client->addr,
+			.flags = 0,
+			.len = sizeof(buf),
+			.buf = buf
+		}
+	};
+
+	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+
+	if (res < 0)
+		dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res);
+
+	/*
+	 * The time from the register write until the host CPU is powered off
+	 * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
+	 * safely avoid returning from the poweroff handler.
+	 */
+	msleep(5000);
+}
+
+static int ntxec_restart(struct notifier_block *nb,
+			 unsigned long action, void *data)
+{
+	int res;
+
+	/*
+	 * NOTE: The lower half of the reset value is not sent, because sending
+	 * it causes an error
+	 */
+	u8 buf[] = {
+		NTXEC_REG_RESET,
+		(NTXEC_RESET_VALUE >> 8) & 0xff,
+	};
+
+	struct i2c_msg msgs[] = {
+		{
+			.addr = poweroff_restart_client->addr,
+			.flags = 0,
+			.len = sizeof(buf),
+			.buf = buf
+		}
+	};
+
+	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+
+	if (res < 0)
+		dev_alert(&poweroff_restart_client->dev, "Failed to restart (err = %d)\n", res);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block ntxec_restart_handler = {
+	.notifier_call = ntxec_restart,
+	.priority = 128
+};
+
+static const struct regmap_config regmap_config = {
+	.name = "ntxec",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.cache_type = REGCACHE_NONE,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+};
+
+static const struct ntxec_info ntxec_known_versions[] = {
+	{ 0xd726 }, /* found in Kobo Aura */
+};
+
+static const struct ntxec_info *ntxec_lookup_info(u16 version)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) {
+		const struct ntxec_info *info = &ntxec_known_versions[i];
+
+		if (info->version == version)
+			return info;
+	}
+
+	return NULL;
+}
+
+static int ntxec_probe(struct i2c_client *client)
+{
+	struct ntxec *ec;
+	unsigned int version;
+	int res;
+
+	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->dev = &client->dev;
+
+	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(ec->regmap)) {
+		dev_err(ec->dev, "Failed to set up regmap for device\n");
+		return res;
+	}
+
+	/* Determine the firmware version */
+	res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
+	if (res < 0) {
+		dev_err(ec->dev, "Failed to read firmware version number\n");
+		return res;
+	}
+	dev_info(ec->dev,
+		 "Netronix embedded controller version %04x detected.\n",
+		 version);
+
+	/* Bail out if we encounter an unknown firmware version */
+	ec->info = ntxec_lookup_info(version);
+	if (!ec->info)
+		return -EOPNOTSUPP;
+
+	if (of_device_is_system_power_controller(ec->dev->of_node)) {
+		/*
+		 * Set the 'powerkeep' bit. This is necessary on some boards
+		 * in order to keep the system running.
+		 */
+		res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
+				   NTXEC_POWERKEEP_VALUE);
+		if (res < 0)
+			return res;
+
+		/* Install poweroff handler */
+		WARN_ON(poweroff_restart_client);
+		poweroff_restart_client = client;
+		if (pm_power_off)
+			dev_err(ec->dev, "pm_power_off already assigned\n");
+		else
+			pm_power_off = ntxec_poweroff;
+
+		/* Install board reset handler */
+		res = register_restart_handler(&ntxec_restart_handler);
+		if (res)
+			dev_err(ec->dev,
+				"Failed to register restart handler: %d\n", res);
+	}
+
+	i2c_set_clientdata(client, ec);
+
+	return devm_of_platform_populate(ec->dev);
+}
+
+static int ntxec_remove(struct i2c_client *i2c)
+{
+	if (i2c == poweroff_restart_client) {
+		poweroff_restart_client = NULL;
+		pm_power_off = NULL;
+		unregister_restart_handler(&ntxec_restart_handler);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_ntxec_match_table[] = {
+	{ .compatible = "netronix,ntxec", },
+	{}
+};
+
+static struct i2c_driver ntxec_driver = {
+	.driver = {
+		.name = "ntxec",
+		.of_match_table = of_ntxec_match_table,
+	},
+	.probe_new = ntxec_probe,
+	.remove = ntxec_remove,
+};
+module_i2c_driver(ntxec_driver);
diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
new file mode 100644
index 0000000000000..65e389af79da6
--- /dev/null
+++ b/include/linux/mfd/ntxec.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Jonathan Neuschäfer
+ *
+ * Register access and version information for the Netronix embedded
+ * controller.
+ */
+
+#ifndef NTXEC_H
+#define NTXEC_H
+
+#include <linux/types.h>
+
+struct ntxec_info {
+	u16 version;
+};
+
+struct ntxec {
+	struct device *dev;
+	struct regmap *regmap;
+	const struct ntxec_info *info;
+};
+
+#endif
--
2.28.0

[PATCH v2 04/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC

[Jonathan Neuschäfer]

The Netronix embedded controller as found in Kobo Aura and Tolino Shine
supports one PWM channel, which is used to control the frontlight
brightness on these devices.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Add plaintext binding to patch description, for comparison
- Fix pwm-cells property (should be 2, not 1)
- Add dummy regulator to example, because the pwm-backlight binding requires a
  power supply


For reference, here is the binding in text form:


  PWM functionality in Netronix Embedded Controller

  Required properties:
  - compatible: should be "netronix,ntxec-pwm"
  - #pwm-cells: should be 2.

  Available PWM channels:
  - 0: The PWM channel controlled by registers 0xa1-0xa7

  Example:

  	embedded-controller@43 {
  		compatible = "netronix,ntxec";
  		...

  		ec_pwm: pwm {
  			compatible = "netronix,ntxec-pwm";
  			#pwm-cells = <1>;
  		};
  	};

  	...

  	backlight {
  		compatible = "pwm-backlight";
  		pwms = <&ec_pwm 0 50000>;
  	};
---
 .../bindings/mfd/netronix,ntxec.yaml          | 19 +++++++++++
 .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml

diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
index 596df460f98eb..73c873dda3e70 100644
--- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -31,6 +31,9 @@ properties:
     description:
       The EC can signal interrupts via a GPIO line

+  pwm:
+    $ref: ../pwm/netronix,ntxec-pwm.yaml
+
 required:
   - compatible
   - reg
@@ -53,5 +56,21 @@ examples:
                     interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
                     interrupt-controller;
                     #interrupt-cells = <1>;
+
+                    ec_pwm: pwm {
+                            compatible = "netronix,ntxec-pwm";
+                            #pwm-cells = <2>;
+                    };
             };
     };
+
+    backlight {
+            compatible = "pwm-backlight";
+            pwms = <&ec_pwm 0 50000>;
+            power-supply = <&backlight_regulator>;
+    };
+
+    backlight_regulator: regulator-dummy {
+            compatible = "regulator-fixed";
+            regulator-name = "backlight";
+    };
diff --git a/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
new file mode 100644
index 0000000000000..0c9d2801b8de1
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/netronix,ntxec-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PWM functionality in Netronix embedded controller
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description: |
+  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+
+  The Netronix EC contains PWM functionality, which is usually used to drive
+  the backlight LED.
+
+  The following PWM channels are supported:
+    - 0: The PWM channel controlled by registers 0xa1-0xa7
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: netronix,ntxec-pwm
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - "#pwm-cells"
--
2.28.0

[PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

The Netronix EC provides a PWM output which is used for the backlight
on some ebook readers. This patches adds a driver for the PWM output.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Various grammar and style improvements, as suggested by Uwe Kleine-König,
  Lee Jones, and Alexandre Belloni
- Switch to regmap
- Prefix registers with NTXEC_REG_
- Add help text to the Kconfig option
- Use the .apply callback instead of the old API
- Add a #define for the time base (125ns)
- Don't change device state in .probe; this avoids multiple problems
- Rework division and overflow check logic to perform divisions in 32 bits
- Avoid setting duty cycle to zero, to work around a hardware quirk
---
 drivers/pwm/Kconfig     |   8 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-ntxec.c | 160 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 drivers/pwm/pwm-ntxec.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d335..7fd17c6cda95e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -350,6 +350,14 @@ config PWM_MXS
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-mxs.

+config PWM_NTXEC
+	tristate "Netronix embedded controller PWM support"
+	depends on MFD_NTXEC && OF
+	help
+	  Say yes here if you want to support the PWM output of the embedded
+	  controller found in certain e-book readers designed by the ODM
+	  Netronix.
+
 config PWM_OMAP_DMTIMER
 	tristate "OMAP Dual-Mode Timer PWM support"
 	depends on OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a035577..1cc50dba22d1b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
+obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
new file mode 100644
index 0000000000000..325ec0e8f1996
--- /dev/null
+++ b/drivers/pwm/pwm-ntxec.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements PWM output.
+ *
+ * Copyright 2020 Jonathan Neuschäfer
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct ntxec_pwm {
+	struct device *dev;
+	struct ntxec *ec;
+	struct pwm_chip chip;
+};
+
+static struct ntxec_pwm *pwmchip_to_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ntxec_pwm, chip);
+}
+
+#define NTXEC_REG_AUTO_OFF_HI	0xa1
+#define NTXEC_REG_AUTO_OFF_LO	0xa2
+#define NTXEC_REG_ENABLE	0xa3
+#define NTXEC_REG_PERIOD_LOW	0xa4
+#define NTXEC_REG_PERIOD_HIGH	0xa5
+#define NTXEC_REG_DUTY_LOW	0xa6
+#define NTXEC_REG_DUTY_HIGH	0xa7
+
+/* Convert an 8-bit value into the correct format for writing into a register */
+#define u8_to_reg(x) (((x) & 0xff) << 8)
+
+/*
+ * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
+ * measured in this unit.
+ */
+#define TIME_BASE_NS 125
+
+/*
+ * The maximum input value (in nanoseconds) is determined by the time base and
+ * the range of the hardware registers that hold the converted value.
+ * It fits into 32 bits, so we can do our calculations in 32 bits as well.
+ */
+#define MAX_PERIOD_NS (TIME_BASE_NS * 0x10000 - 1)
+
+static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+			   const struct pwm_state *state)
+{
+	struct ntxec_pwm *pwm = pwmchip_to_pwm(pwm_dev->chip);
+	unsigned int duty = state->duty_cycle;
+	unsigned int period = state->period;
+	int res = 0;
+
+	if (period > MAX_PERIOD_NS) {
+		dev_warn(pwm->dev,
+			 "Period is not representable in 16 bits after division by %u: %u\n",
+			 TIME_BASE_NS, period);
+		return -ERANGE;
+	}
+
+	period /= TIME_BASE_NS;
+	duty /= TIME_BASE_NS;
+
+	res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, u8_to_reg(period >> 8));
+	res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, u8_to_reg(period));
+	res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, u8_to_reg(duty >> 8));
+	res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, u8_to_reg(duty));
+
+	if (res < 0)
+		return -EIO;
+
+	/*
+	 * Writing a duty cycle of zone puts the device into a state where
+	 * writing a higher duty cycle doesn't result in the brightness that it
+	 * usually results in. This can be fixed by cycling the ENABLE register.
+	 *
+	 * As a workaround, write ENABLE=0 when the duty cycle is zero.
+	 */
+	if (state->enabled && duty != 0) {
+		res |= regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, u8_to_reg(1));
+
+		/* Disable the auto-off timer */
+		res |= regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, u8_to_reg(0xff));
+		res |= regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, u8_to_reg(0xff));
+		return res ? -EIO : 0;
+	} else {
+		return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, u8_to_reg(0));
+	}
+}
+
+static struct pwm_ops ntxec_pwm_ops = {
+	.apply = ntxec_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static int ntxec_pwm_probe(struct platform_device *pdev)
+{
+	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
+	struct ntxec_pwm *pwm;
+	struct pwm_chip *chip;
+	int res;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->ec = ec;
+	pwm->dev = &pdev->dev;
+
+	chip = &pwm->chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &ntxec_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	res = pwmchip_add(chip);
+	if (res < 0)
+		return res;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return (res < 0) ? -EIO : 0;
+}
+
+static int ntxec_pwm_remove(struct platform_device *pdev)
+{
+	struct ntxec_pwm *pwm = platform_get_drvdata(pdev);
+	struct pwm_chip *chip = &pwm->chip;
+
+	return pwmchip_remove(chip);
+}
+
+static const struct of_device_id ntxec_pwm_of_match[] = {
+	{ .compatible = "netronix,ntxec-pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ntxec_pwm_of_match);
+
+static struct platform_driver ntxec_pwm_driver = {
+	.driver = {
+		.name = "ntxec-pwm",
+		.of_match_table = ntxec_pwm_of_match,
+	},
+	.probe = ntxec_pwm_probe,
+	.remove = ntxec_pwm_remove,
+};
+module_platform_driver(ntxec_pwm_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("PWM driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.28.0

[PATCH v2 06/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC

[Jonathan Neuschäfer]

The Netronix EC implements an RTC with the following functionality:

- Calendar-based time keeping with single-second resolution
- Automatic power-on with single-minute resolution
- Alarm at single-second resolution

This binding only supports timekeeping for now.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Add plaintext binding to the patch description, for reference


For reference, here is the binding in text form:


  Netronix embedded controller RTC

  Required properties:

  - compatible: should be "netronix,ntxec-rtc"


  Example:

  	embedded-controller@43 {
  		compatible = "netronix,ntxec";
  		...

  		rtc {
  			compatible = "netronix,ntxec-rtc";
  		};
  	}
---
 .../bindings/mfd/netronix,ntxec.yaml          |  7 +++++
 .../bindings/rtc/netronix,ntxec-rtc.yaml      | 27 +++++++++++++++++++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
index 73c873dda3e70..7e1a21a82d82b 100644
--- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -34,6 +34,9 @@ properties:
   pwm:
     $ref: ../pwm/netronix,ntxec-pwm.yaml

+  rtc:
+    $ref: ../rtc/netronix,ntxec-rtc.yaml
+
 required:
   - compatible
   - reg
@@ -61,6 +64,10 @@ examples:
                             compatible = "netronix,ntxec-pwm";
                             #pwm-cells = <2>;
                     };
+
+                    rtc {
+                            compatible = "netronix,ntxec-rtc";
+                    };
             };
     };

diff --git a/Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml b/Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
new file mode 100644
index 0000000000000..4b301ef7319c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/netronix,ntxec-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTC functionality in Netronix embedded controller
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description: |
+  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+
+  The Netronix EC contains an RTC, which can be used for time-keeping, alarm,
+  and automatic power-on. Note that not all of this functionality is currently
+  supported in this binding.
+
+allOf:
+  - $ref: "rtc.yaml#"
+
+properties:
+  compatible:
+    const: netronix,ntxec-rtc
+
+required:
+  - compatible
--
2.28.0

[PATCH v2 07/10] rtc: Introduce RTC_TIMESTAMP_END_2255

[Jonathan Neuschäfer]

Some RTCs store the year as an 8-bit number relative to the year 2000.
This results in a maximum timestamp of 2255-12-31 23:59:59.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- New patch
---
 include/linux/rtc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 22d1575e4991b..fcc086084a603 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -154,6 +154,7 @@ struct rtc_device {
 #define RTC_TIMESTAMP_END_2079		3471292799LL /* 2079-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_2099		4102444799LL /* 2099-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_2199		7258118399LL /* 2199-12-31 23:59:59 */
+#define RTC_TIMESTAMP_END_2255		9025257599LL /* 2255-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_9999		253402300799LL /* 9999-12-31 23:59:59 */

 extern struct rtc_device *devm_rtc_device_register(struct device *dev,
--
2.28.0

[PATCH v2 08/10] rtc: New driver for RTC in Netronix embedded controller

[Jonathan Neuschäfer]

With this driver, mainline Linux can keep its time and date in sync with
the vendor kernel.

Advanced functionality like alarm and automatic power-on is not yet
supported.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Rework top-of-file comment [Lee Jones]
- Sort the #include lines [Alexandre Belloni]
- don't align = signs in struct initializers [Uwe Kleine-König]
- Switch to regmap
- Fix register number used to read minutes and seconds
- Prefix registers with NTXEC_REG_
- Add help text to the Kconfig option
- Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max
---
 drivers/rtc/Kconfig     |   8 +++
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/rtc-ntxec.c | 130 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100644 drivers/rtc/rtc-ntxec.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 48c536acd777f..ae8f3dc36c9a3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-cros-ec.

+config RTC_DRV_NTXEC
+	tristate "Netronix embedded controller RTC driver"
+	depends on MFD_NTXEC
+	help
+	  Say yes here if you want to support the RTC functionality of the
+	  embedded controller found in certain e-book readers designed by the
+	  ODM Netronix.
+
 comment "on-CPU RTC drivers"

 config RTC_DRV_ASM9260
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 880e08a409c3d..733479db18896 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622)	+= rtc-mt7622.o
 obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.o
 obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc.o
 obj-$(CONFIG_RTC_DRV_MXC_V2)	+= rtc-mxc_v2.o
+obj-$(CONFIG_RTC_DRV_NTXEC)	+= rtc-ntxec.o
 obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
 obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
 obj-$(CONFIG_RTC_DRV_PALMAS)	+= rtc-palmas.o
diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c
new file mode 100644
index 0000000000000..82eb6352353cd
--- /dev/null
+++ b/drivers/rtc/rtc-ntxec.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements access to the RTC time and date.
+ *
+ * Copyright 2020 Jonathan Neuschäfer
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/types.h>
+
+struct ntxec_rtc {
+	struct device *dev;
+	struct ntxec *ec;
+};
+
+#define NTXEC_REG_WRITE_YEAR	0x10
+#define NTXEC_REG_WRITE_MONTH	0x11
+#define NTXEC_REG_WRITE_DAY	0x12
+#define NTXEC_REG_WRITE_HOUR	0x13
+#define NTXEC_REG_WRITE_MINUTE	0x14
+#define NTXEC_REG_WRITE_SECOND	0x15
+
+/* Convert an 8-bit value into the correct format for writing into a register */
+#define u8_to_reg(x) (((x) & 0xff) << 8)
+
+#define NTXEC_REG_READ_YM	0x20
+#define NTXEC_REG_READ_DH	0x21
+#define NTXEC_REG_READ_MS	0x23
+
+static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+	unsigned int value;
+	int res;
+
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
+	if (res < 0)
+		return res;
+
+	tm->tm_year = (value >> 8) + 100;
+	tm->tm_mon = (value & 0xff) - 1;
+
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
+	if (res < 0)
+		return res;
+
+	tm->tm_mday = value >> 8;
+	tm->tm_hour = value & 0xff;
+
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
+	if (res < 0)
+		return res;
+
+	tm->tm_min = value >> 8;
+	tm->tm_sec = value & 0xff;
+
+	return 0;
+}
+
+static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+	int res = 0;
+
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, u8_to_reg(tm->tm_year - 100));
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, u8_to_reg(tm->tm_mon + 1));
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, u8_to_reg(tm->tm_mday));
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, u8_to_reg(tm->tm_hour));
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, u8_to_reg(tm->tm_min));
+	res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, u8_to_reg(tm->tm_sec));
+
+	return (res < 0) ? -EIO : 0;
+}
+
+static const struct rtc_class_ops ntxec_rtc_ops = {
+	.read_time = ntxec_read_time,
+	.set_time = ntxec_set_time,
+};
+
+static int ntxec_rtc_probe(struct platform_device *pdev)
+{
+	struct rtc_device *dev;
+	struct ntxec_rtc *rtc;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	rtc->dev = &pdev->dev;
+	rtc->ec = dev_get_drvdata(pdev->dev.parent);
+	platform_set_drvdata(pdev, rtc);
+
+	dev = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	dev->ops = &ntxec_rtc_ops;
+	dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	dev->range_max = RTC_TIMESTAMP_END_2255;
+
+	return rtc_register_device(dev);
+}
+
+static const struct of_device_id ntxec_rtc_of_match[] = {
+	{ .compatible = "netronix,ntxec-rtc" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ntxec_rtc_of_match);
+
+static struct platform_driver ntxec_rtc_driver = {
+	.driver = {
+		.name = "ntxec-rtc",
+		.of_match_table = ntxec_rtc_of_match,
+	},
+	.probe = ntxec_rtc_probe,
+};
+module_platform_driver(ntxec_rtc_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("RTC driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.28.0

[PATCH v2 09/10] MAINTAINERS: Add entry for Netronix embedded controller

[Jonathan Neuschäfer]

Let's make sure I'll notice when there are patches for the NTXEC
drivers.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- No changes
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e4647c84c987e..ffe15a65bae2e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11995,6 +11995,17 @@ F:	include/net/netrom.h
 F:	include/uapi/linux/netrom.h
 F:	net/netrom/

+NETRONIX EMBEDDED CONTROLLER
+M:	Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+F:	Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
+F:	Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
+F:	drivers/mfd/ntxec.c
+F:	drivers/pwm/pwm-ntxec.c
+F:	drivers/rtc/rtc-ntxec.c
+F:	include/linux/mfd/ntxec.h
+
 NETRONOME ETHERNET DRIVERS
 M:	Simon Horman <simon.horman@netronome.com>
 R:	Jakub Kicinski <kuba@kernel.org>
--
2.28.0

[PATCH v2 10/10] ARM: dts: imx50-kobo-aura: Add Netronix embedded controller

[Jonathan Neuschäfer]

Enable the Netronix EC on the Kobo Aura ebook reader.

Several features are still missing:
 - Frontlight/backlight. The vendor kernel drives the frontlight LED
   using the PWM output of the EC and an additional boost pin that
   increases the brightness.
 - Battery monitoring
 - Interrupts for RTC alarm and low-battery events

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---

v2:
- Fix pwm-cells property (should be 2, not 1)
---
 arch/arm/boot/dts/imx50-kobo-aura.dts | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx50-kobo-aura.dts b/arch/arm/boot/dts/imx50-kobo-aura.dts
index a0eaf869b9135..003a7d894902c 100644
--- a/arch/arm/boot/dts/imx50-kobo-aura.dts
+++ b/arch/arm/boot/dts/imx50-kobo-aura.dts
@@ -6,6 +6,7 @@
 /dts-v1/;
 #include "imx50.dtsi"
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/interrupt-controller/irq.h>

 / {
 	model = "Kobo Aura (N514)";
@@ -135,10 +136,34 @@ &i2c3 {
 	pinctrl-0 = <&pinctrl_i2c3>;
 	status = "okay";

-	/* TODO: embedded controller at 0x43 */
+	embedded-controller@43 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ec>;
+		compatible = "netronix,ntxec";
+		reg = <0x43>;
+		system-power-controller;
+		interrupts-extended = <&gpio4 11 IRQ_TYPE_EDGE_FALLING>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		ec_pwm: pwm {
+			compatible = "netronix,ntxec-pwm";
+			#pwm-cells = <2>;
+		};
+
+		rtc {
+			compatible = "netronix,ntxec-rtc";
+		};
+	};
 };

 &iomuxc {
+	pinctrl_ec: ec {
+		fsl,pins = <
+			MX50_PAD_CSPI_SS0__GPIO4_11		0x0	/* INT */
+		>;
+	};
+
 	pinctrl_gpiokeys: gpiokeys {
 		fsl,pins = <
 			MX50_PAD_CSPI_MISO__GPIO4_10		0x0
--
2.28.0

Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Lee Jones]

On Sat, 05 Sep 2020, Andy Shevchenko wrote:

> On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> wrote:
> 
> > The Netronix EC provides a PWM output which is used for the backlight
> > on some ebook readers. This patches adds a driver for the PWM output.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> >
> > v2:
> > - Various grammar and style improvements, as suggested by Uwe Kleine-König,
> >   Lee Jones, and Alexandre Belloni
> > - Switch to regmap
> > - Prefix registers with NTXEC_REG_
> > - Add help text to the Kconfig option
> > - Use the .apply callback instead of the old API
> > - Add a #define for the time base (125ns)
> > - Don't change device state in .probe; this avoids multiple problems
> > - Rework division and overflow check logic to perform divisions in 32 bits
> > - Avoid setting duty cycle to zero, to work around a hardware quirk
> > ---
> >  drivers/pwm/Kconfig     |   8 ++
> >  drivers/pwm/Makefile    |   1 +
> >  drivers/pwm/pwm-ntxec.c | 160 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 169 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-ntxec.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 7dbcf6973d335..7fd17c6cda95e 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -350,6 +350,14 @@ config PWM_MXS
> >           To compile this driver as a module, choose M here: the module
> >           will be called pwm-mxs.
> >
> > +config PWM_NTXEC
> > +       tristate "Netronix embedded controller PWM support"
> 
> 
> 
> 
> > +       depends on MFD_NTXEC && OF
> 
> 
> I don’t see need to reduce test coverage and use of the driver by sticking
> with OF. Actually it’s not used.

If the device is only known to boot with OF, then it's pointless
building it when !OF.  If you want to increase test coverage enable
COMPILE_TEST instead.

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

Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Andy Shevchenko]

On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Sat, 05 Sep 2020, Andy Shevchenko wrote:
> > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > wrote:

...

> > > +config PWM_NTXEC
> > > +       tristate "Netronix embedded controller PWM support"
> >
> >
> >
> >
> > > +       depends on MFD_NTXEC && OF
> >
> >
> > I don’t see need to reduce test coverage and use of the driver by sticking
> > with OF. Actually it’s not used.
>
> If the device is only known to boot with OF, then it's pointless
> building it when !OF.

No, it's not. As I pointed out the (compilation) test coverage is better.

> If you want to increase test coverage enable
> COMPILE_TEST instead.

It is one way to achieve that, yes;

       depends on OF || COMPILE_TEST
       depends on MFD_NTXEC

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Lee Jones]

On Tue, 08 Sep 2020, Andy Shevchenko wrote:

> On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Sat, 05 Sep 2020, Andy Shevchenko wrote:
> > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > > wrote:
> 
> ...
> 
> > > > +config PWM_NTXEC
> > > > +       tristate "Netronix embedded controller PWM support"
> > >
> > >
> > >
> > >
> > > > +       depends on MFD_NTXEC && OF
> > >
> > >
> > > I don’t see need to reduce test coverage and use of the driver by sticking
> > > with OF. Actually it’s not used.
> >
> > If the device is only known to boot with OF, then it's pointless
> > building it when !OF.
> 
> No, it's not. As I pointed out the (compilation) test coverage is better.

No, it's a waste of disk space.

Why would you knowingly compile something you know you can't use?

That's the whole point of COMPILE_TEST.

Note that when you want real coverage and you use `allyesconfig`
and/or `allmodconfig` then CONFIG_OF is also enabled on platforms
which support it.

> > If you want to increase test coverage enable
> > COMPILE_TEST instead.
> 
> It is one way to achieve that, yes;
> 
>        depends on OF || COMPILE_TEST
>        depends on MFD_NTXEC

This is better.

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

Re: [PATCH v2 07/10] rtc: Introduce RTC_TIMESTAMP_END_2255

[Alexandre Belloni]

On 05/09/2020 15:32:27+0200, Jonathan Neuschäfer wrote:
> Some RTCs store the year as an 8-bit number relative to the year 2000.
> This results in a maximum timestamp of 2255-12-31 23:59:59.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v2:
> - New patch
> ---
>  include/linux/rtc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 22d1575e4991b..fcc086084a603 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -154,6 +154,7 @@ struct rtc_device {
>  #define RTC_TIMESTAMP_END_2079		3471292799LL /* 2079-12-31 23:59:59 */
>  #define RTC_TIMESTAMP_END_2099		4102444799LL /* 2099-12-31 23:59:59 */
>  #define RTC_TIMESTAMP_END_2199		7258118399LL /* 2199-12-31 23:59:59 */
> +#define RTC_TIMESTAMP_END_2255		9025257599LL /* 2255-12-31 23:59:59 */

Honestly, I wouldn't bother adding that one unless you have examples of
other RTCs endng at the same date, I'm fine having the value and comment
directly in the probe function.

>  #define RTC_TIMESTAMP_END_9999		253402300799LL /* 9999-12-31 23:59:59 */
> 
>  extern struct rtc_device *devm_rtc_device_register(struct device *dev,
> --
> 2.28.0
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 03/10] mfd: Add base driver for Netronix embedded controller

[Lee Jones]

On Sat, 05 Sep 2020, Jonathan Neuschäfer wrote:

> The Netronix embedded controller is a microcontroller found in some
> e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> battery monitoring, system power management, and PWM functionality.
> 
> This driver implements register access and version detection.
> 
> Third-party hardware documentation is available at:
> 
>   https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> 
> The EC supports interrupts, but the driver doesn't make use of them so
> far.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v2:
> - Add a description of the device to the patch text
> - Unify spelling as 'Netronix embedded controller'.
>   'Netronix' is the proper name of the manufacturer, but 'embedded controller'
>   is just a label that I have assigned to the device.
> - Switch to regmap, avoid regmap use in poweroff and reboot handlers.
>   Inspired by cf84dc0bb40f4 ("mfd: rn5t618: Make restart handler atomic safe")
> - Use a list of known-working firmware versions instead of checking for a
>   known-incompatible version
> - Prefix registers with NTXEC_REG_
> - Define register values as constants
> - Various style cleanups as suggested by Lee Jones
> - Don't align = signs in struct initializers [Uwe Kleine-König]
> - Don't use dev_dbg for an error message
> - Explain sleep in poweroff handler
> - Remove (struct ntxec).client
> - Switch to .probe_new in i2c driver
> - Add .remove callback
> - Make CONFIG_MFD_NTXEC a tristate option
> ---
>  drivers/mfd/Kconfig       |   7 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 217 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  24 +++++
>  4 files changed, 249 insertions(+)
>  create mode 100644 drivers/mfd/ntxec.c
>  create mode 100644 include/linux/mfd/ntxec.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab415..bab999081f32d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,13 @@ config MFD_VIPERBOARD
>  	  You need to select the mfd cell drivers separately.
>  	  The drivers do not support all features the board exposes.
> 
> +config MFD_NTXEC
> +	tristate "Netronix embedded controller"

Nit: "Embedded Controller (EC)"

> +	depends on I2C && OF

	depends on (I2C && OF) || COMPILE_TEST

> +	help
> +	  Say yes here if you want to support the embedded controller found in
> +	  certain e-book readers designed by the ODM Netronix.
> +
>  config MFD_RETU
>  	tristate "Nokia Retu and Tahvo multi-function device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f835283e..236a8acd917a0 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -217,6 +217,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
>  obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
> +obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
>  obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
>  obj-$(CONFIG_MFD_RK808)		+= rk808.o
>  obj-$(CONFIG_MFD_RN5T618)	+= rn5t618.o
> diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
> new file mode 100644
> index 0000000000000..49cc4fa35b9fe
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Why 2 only?

> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> + * battery monitoring, system power management, and PWM functionality.
> + *
> + * This driver implements register access, version detection, and system
> + * power-off/reset.
> + *
> + * Copyright 2020 Jonathan Neuschäfer

Email?

> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +#define NTXEC_REG_VERSION	0x00
> +#define NTXEC_REG_POWEROFF	0x50
> +#define NTXEC_REG_POWERKEEP	0x70
> +#define NTXEC_REG_RESET		0x90
> +
> +#define NTXEC_POWEROFF_VALUE	0x0100
> +#define NTXEC_POWERKEEP_VALUE	0x0800
> +#define NTXEC_RESET_VALUE	0xff00
> +
> +static struct i2c_client *poweroff_restart_client;
> +
> +static void ntxec_poweroff(void)
> +{
> +	int res;
> +

Remove this line please.

> +	u8 buf[] = {
> +		NTXEC_REG_POWEROFF,
> +		(NTXEC_POWEROFF_VALUE >> 8) & 0xff,
> +		NTXEC_POWEROFF_VALUE & 0xff,
> +	};
> +

And this one.

> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = poweroff_restart_client->addr,
> +			.flags = 0,
> +			.len = sizeof(buf),
> +			.buf = buf
> +		}
> +	};
> +
> +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> +

And this one.

> +	if (res < 0)
> +		dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res);

This looks way over 80 chars.

Did you run checkpatch.pl?

> +	/*
> +	 * The time from the register write until the host CPU is powered off
> +	 * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
> +	 * safely avoid returning from the poweroff handler.
> +	 */
> +	msleep(5000);
> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> +			 unsigned long action, void *data)
> +{
> +	int res;
> +
> +	/*
> +	 * NOTE: The lower half of the reset value is not sent, because sending
> +	 * it causes an error
> +	 */
> +	u8 buf[] = {
> +		NTXEC_REG_RESET,
> +		(NTXEC_RESET_VALUE >> 8) & 0xff,
> +	};
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = poweroff_restart_client->addr,
> +			.flags = 0,
> +			.len = sizeof(buf),
> +			.buf = buf
> +		}
> +	};
> +
> +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> +
> +	if (res < 0)
> +		dev_alert(&poweroff_restart_client->dev, "Failed to restart (err = %d)\n", res);
> +
> +	return NOTIFY_DONE;
> +}

All as above for this function.

> +static struct notifier_block ntxec_restart_handler = {
> +	.notifier_call = ntxec_restart,
> +	.priority = 128
> +};
> +
> +static const struct regmap_config regmap_config = {
> +	.name = "ntxec",
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.cache_type = REGCACHE_NONE,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +};
> +
> +static const struct ntxec_info ntxec_known_versions[] = {
> +	{ 0xd726 }, /* found in Kobo Aura */
> +};
> +
> +static const struct ntxec_info *ntxec_lookup_info(u16 version)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) {
> +		const struct ntxec_info *info = &ntxec_known_versions[i];
> +
> +		if (info->version == version)
> +			return info;
> +	}
> +
> +	return NULL;
> +}

Wait, what?  This is over-engineered.

Just have a look-up table (switch) of supported devices.

> +static int ntxec_probe(struct i2c_client *client)
> +{
> +	struct ntxec *ec;
> +	unsigned int version;
> +	int res;
> +
> +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = &client->dev;
> +
> +	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(ec->regmap)) {
> +		dev_err(ec->dev, "Failed to set up regmap for device\n");
> +		return res;
> +	}
> +
> +	/* Determine the firmware version */
> +	res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
> +	if (res < 0) {
> +		dev_err(ec->dev, "Failed to read firmware version number\n");
> +		return res;
> +	}
> +	dev_info(ec->dev,
> +		 "Netronix embedded controller version %04x detected.\n",
> +		 version);
> +
> +	/* Bail out if we encounter an unknown firmware version */
> +	ec->info = ntxec_lookup_info(version);
> +	if (!ec->info)
> +		return -EOPNOTSUPP;

#define EOPNOTSUPP      95      /* Operation not supported on transport endpoint */

I think you want:

#define ENODEV          19      /* No such device */

> +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> +		/*
> +		 * Set the 'powerkeep' bit. This is necessary on some boards
> +		 * in order to keep the system running.
> +		 */
> +		res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
> +				   NTXEC_POWERKEEP_VALUE);
> +		if (res < 0)
> +			return res;
> +
> +		/* Install poweroff handler */

Don't think this comment is required.

> +		WARN_ON(poweroff_restart_client);
> +		poweroff_restart_client = client;
> +		if (pm_power_off)
> +			dev_err(ec->dev, "pm_power_off already assigned\n");
> +		else
> +			pm_power_off = ntxec_poweroff;
> +
> +		/* Install board reset handler */

Nor this.

> +		res = register_restart_handler(&ntxec_restart_handler);
> +		if (res)
> +			dev_err(ec->dev,
> +				"Failed to register restart handler: %d\n", res);
> +	}
> +
> +	i2c_set_clientdata(client, ec);
> +
> +	return devm_of_platform_populate(ec->dev);
> +}
> +
> +static int ntxec_remove(struct i2c_client *i2c)

Why do you call it 'client' in .probe, but 'i2c' in .remove?

> +{
> +	if (i2c == poweroff_restart_client) {
> +		poweroff_restart_client = NULL;
> +		pm_power_off = NULL;
> +		unregister_restart_handler(&ntxec_restart_handler);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_ntxec_match_table[] = {
> +	{ .compatible = "netronix,ntxec", },
> +	{}
> +};
> +
> +static struct i2c_driver ntxec_driver = {
> +	.driver = {
> +		.name = "ntxec",
> +		.of_match_table = of_ntxec_match_table,
> +	},
> +	.probe_new = ntxec_probe,
> +	.remove = ntxec_remove,
> +};
> +module_i2c_driver(ntxec_driver);
> diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> new file mode 100644
> index 0000000000000..65e389af79da6
> --- /dev/null
> +++ b/include/linux/mfd/ntxec.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Jonathan Neuschäfer
> + *
> + * Register access and version information for the Netronix embedded
> + * controller.
> + */
> +
> +#ifndef NTXEC_H
> +#define NTXEC_H
> +
> +#include <linux/types.h>
> +
> +struct ntxec_info {
> +	u16 version;
> +};
> +
> +struct ntxec {
> +	struct device *dev;
> +	struct regmap *regmap;

> +	const struct ntxec_info *info;

What is this used for?

> +};
> +
> +#endif

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

Re: [PATCH v2 03/10] mfd: Add base driver for Netronix embedded controller

[Jonathan Neuschäfer]

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

On Tue, Sep 08, 2020 at 02:29:34PM +0100, Lee Jones wrote:
> On Sat, 05 Sep 2020, Jonathan Neuschäfer wrote:
[...]
> > +config MFD_NTXEC
> > +	tristate "Netronix embedded controller"
> 
> Nit: "Embedded Controller (EC)"

I intentionally left it lowercase, because the term 'embedded
controller' is not used in code coming from Netronix. It's just a label
that I found fitting to assign to the device. (In the vendor kernels
it's either called 'msp430', named after the TI MSP430 microcontroller
family, or 'uc', presumably for 'microcontroller')

Adding '(EC)' seems somewhat useful.

> 
> > +	depends on I2C && OF
> 
> 	depends on (I2C && OF) || COMPILE_TEST

Okay

> > +++ b/drivers/mfd/ntxec.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> Why 2 only?

No particular reason. If you prefer 2-or-later, I'll change it.

> > +/*
> > + * The Netronix embedded controller is a microcontroller found in some
> > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> > + * battery monitoring, system power management, and PWM functionality.
> > + *
> > + * This driver implements register access, version detection, and system
> > + * power-off/reset.
> > + *
> > + * Copyright 2020 Jonathan Neuschäfer
> 
> Email?

Alright, I'll add it

> > +static void ntxec_poweroff(void)
> > +{
> > +	int res;
> > +
> 
> Remove this line please.
> 
> > +	u8 buf[] = {
> > +		NTXEC_REG_POWEROFF,
> > +		(NTXEC_POWEROFF_VALUE >> 8) & 0xff,
> > +		NTXEC_POWEROFF_VALUE & 0xff,
> > +	};
> > +
> 
> And this one.
> 
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = poweroff_restart_client->addr,
> > +			.flags = 0,
> > +			.len = sizeof(buf),
> > +			.buf = buf
> > +		}
> > +	};
> > +
> > +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> > +
> 
> And this one.

Okay

> 
> > +	if (res < 0)
> > +		dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res);
> 
> This looks way over 80 chars.

Okay, I'll break it up.

> Did you run checkpatch.pl?

Yes, but it didn't complain about this line. - propabably because the
line length threshold in checkpatch has recently been increased to 100.

> > +	/*
> > +	 * The time from the register write until the host CPU is powered off
> > +	 * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
> > +	 * safely avoid returning from the poweroff handler.
> > +	 */
> > +	msleep(5000);
> > +}
> > +
> > +static int ntxec_restart(struct notifier_block *nb,
> > +			 unsigned long action, void *data)
[...]
> 
> All as above for this function.

Alright

> 
> > +static struct notifier_block ntxec_restart_handler = {
> > +	.notifier_call = ntxec_restart,
> > +	.priority = 128
> > +};
> > +
> > +static const struct regmap_config regmap_config = {
> > +	.name = "ntxec",
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.cache_type = REGCACHE_NONE,
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +};
> > +
> > +static const struct ntxec_info ntxec_known_versions[] = {
> > +	{ 0xd726 }, /* found in Kobo Aura */
> > +};
> > +
> > +static const struct ntxec_info *ntxec_lookup_info(u16 version)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) {
> > +		const struct ntxec_info *info = &ntxec_known_versions[i];
> > +
> > +		if (info->version == version)
> > +			return info;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> Wait, what?  This is over-engineered.

I thought it would be useful when we want to attach additional
information to specific versions.... okay, it is over-engineered.

> Just have a look-up table (switch) of supported devices.

Will do.

> 
> > +static int ntxec_probe(struct i2c_client *client)
> > +{
> > +	struct ntxec *ec;
> > +	unsigned int version;
> > +	int res;
> > +
> > +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> > +	if (!ec)
> > +		return -ENOMEM;
> > +
> > +	ec->dev = &client->dev;
> > +
> > +	ec->regmap = devm_regmap_init_i2c(client, &regmap_config);
> > +	if (IS_ERR(ec->regmap)) {
> > +		dev_err(ec->dev, "Failed to set up regmap for device\n");
> > +		return res;
> > +	}
> > +
> > +	/* Determine the firmware version */
> > +	res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
> > +	if (res < 0) {
> > +		dev_err(ec->dev, "Failed to read firmware version number\n");
> > +		return res;
> > +	}
> > +	dev_info(ec->dev,
> > +		 "Netronix embedded controller version %04x detected.\n",
> > +		 version);
> > +
> > +	/* Bail out if we encounter an unknown firmware version */
> > +	ec->info = ntxec_lookup_info(version);
> > +	if (!ec->info)
> > +		return -EOPNOTSUPP;
> 
> #define EOPNOTSUPP      95      /* Operation not supported on transport endpoint */
> 
> I think you want:
> 
> #define ENODEV          19      /* No such device */

Indeed, EOPNOTSUPP seems quite wrong here. I think I used ENOTSUPP at
some earlier point but moved away from it because it's one of the
internal error codes (≥512).

ENODEV makes sense when I read it as "Not a device that this driver can
deal with".

> 
> > +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> > +		/*
> > +		 * Set the 'powerkeep' bit. This is necessary on some boards
> > +		 * in order to keep the system running.
> > +		 */
> > +		res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
> > +				   NTXEC_POWERKEEP_VALUE);
> > +		if (res < 0)
> > +			return res;
> > +
> > +		/* Install poweroff handler */
> 
> Don't think this comment is required.
> 
> > +		WARN_ON(poweroff_restart_client);
> > +		poweroff_restart_client = client;
> > +		if (pm_power_off)
> > +			dev_err(ec->dev, "pm_power_off already assigned\n");
> > +		else
> > +			pm_power_off = ntxec_poweroff;
> > +
> > +		/* Install board reset handler */
> 
> Nor this.

Alright, I'll remove them.

> > +		res = register_restart_handler(&ntxec_restart_handler);
> > +		if (res)
> > +			dev_err(ec->dev,
> > +				"Failed to register restart handler: %d\n", res);
> > +	}
> > +
> > +	i2c_set_clientdata(client, ec);
> > +
> > +	return devm_of_platform_populate(ec->dev);
> > +}
> > +
> > +static int ntxec_remove(struct i2c_client *i2c)
> 
> Why do you call it 'client' in .probe, but 'i2c' in .remove?

No particular reason. I'll make them consistent.

> > +struct ntxec_info {
> > +	u16 version;
> > +};
> > +
> > +struct ntxec {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> 
> > +	const struct ntxec_info *info;
> 
> What is this used for?

At this point, nothing. I'll remove it.


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Sat, Sep 05, 2020 at 09:08:49PM +0300, Andy Shevchenko wrote:
> On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> wrote:
> 
> > The Netronix EC provides a PWM output which is used for the backlight
> > on some ebook readers. This patches adds a driver for the PWM output.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +#include <linux/of_device.h>
> 
> 
> mod_devicetable.h

Okay

> > +/* Convert an 8-bit value into the correct format for writing into a
> > register */
> > +#define u8_to_reg(x) (((x) & 0xff) << 8)
> 
> 
> You spread this macro among the drivers w/o explanation what’s going on. I
> think there will be better approach.

Okay, I'll move it to ntxec.h and expand on the explanation. I think
what's missing is the following part:

  Some registers, such as the battery status register (0x41), are in
  big-endian, but others only have eight significant bits, which are in
  the first byte transmitted over I2C (the MSB of the big-endian value).
  This convenience macro/function converts an 8-bit value to 16-bit for
  use in the second kind of register.

> > +
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH,
> > u8_to_reg(period >> 8));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW,
> > u8_to_reg(period));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH,
> > u8_to_reg(duty >> 8));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW,
> > u8_to_reg(duty));
> 
> 
> These funny res |= is unusual pattern for returned error codes. Moreover
> you are shadowing the real ones. Same go the rest drovers. Please fix by
> checking each separately.

Okay

> > +       platform_set_drvdata(pdev, pwm);
> > +
> > +       return (res < 0) ? -EIO : 0;
> 
> 
> What?!

That's an editing error, sorry.

> > +static const struct of_device_id ntxec_pwm_of_match[] = {
> > +       { .compatible = "netronix,ntxec-pwm" },
> >
> >
> 
> > +       { },
> 
> 
> No comma.

Okay


Thanks for the review,
Jonathan Neuschäfer

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

Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Tue, Sep 08, 2020 at 10:35:38AM +0100, Lee Jones wrote:
> On Tue, 08 Sep 2020, Andy Shevchenko wrote:
> 
> > On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Sat, 05 Sep 2020, Andy Shevchenko wrote:
> > > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
[...]
> > > > > +       depends on MFD_NTXEC && OF
> > > >
> > > >
> > > > I don’t see need to reduce test coverage and use of the driver by sticking
> > > > with OF. Actually it’s not used.
> > >
> > > If the device is only known to boot with OF, then it's pointless
> > > building it when !OF.
> > 
> > No, it's not. As I pointed out the (compilation) test coverage is better.
> 
> No, it's a waste of disk space.
> 
> Why would you knowingly compile something you know you can't use?
> 
> That's the whole point of COMPILE_TEST.
> 
> Note that when you want real coverage and you use `allyesconfig`
> and/or `allmodconfig` then CONFIG_OF is also enabled on platforms
> which support it.
> 
> > > If you want to increase test coverage enable
> > > COMPILE_TEST instead.
> > 
> > It is one way to achieve that, yes;
> > 
> >        depends on OF || COMPILE_TEST
> >        depends on MFD_NTXEC
> 
> This is better.

Ok, I'll go with this variant.


Thanks

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

Re: [PATCH v2 07/10] rtc: Introduce RTC_TIMESTAMP_END_2255

[Jonathan Neuschäfer]

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

On Tue, Sep 08, 2020 at 11:39:46AM +0200, Alexandre Belloni wrote:
> On 05/09/2020 15:32:27+0200, Jonathan Neuschäfer wrote:
[...]
> >  #define RTC_TIMESTAMP_END_2079		3471292799LL /* 2079-12-31 23:59:59 */
> >  #define RTC_TIMESTAMP_END_2099		4102444799LL /* 2099-12-31 23:59:59 */
> >  #define RTC_TIMESTAMP_END_2199		7258118399LL /* 2199-12-31 23:59:59 */
> > +#define RTC_TIMESTAMP_END_2255		9025257599LL /* 2255-12-31 23:59:59 */
> 
> Honestly, I wouldn't bother adding that one unless you have examples of
> other RTCs endng at the same date, I'm fine having the value and comment
> directly in the probe function.

Indeed, it's the only RTC I know with this end date. I'll fold it into
the driver.


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v2 08/10] rtc: New driver for RTC in Netronix embedded controller

[Jonathan Neuschäfer]

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

On Sat, Sep 05, 2020 at 08:56:35PM +0300, Andy Shevchenko wrote:
> On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> wrote:
[...]
> > +#include <linux/of_device.h>
> 
> 
> No user for this. Perhaps you meant mod_devicetable.h?

Yes

> > +/* Convert an 8-bit value into the correct format for writing into a
> > register */
> > +#define u8_to_reg(x) (((x) & 0xff) << 8)
> 
> 
> This needs more explanation. Does register be16?

Yes, the registers are treated as be16 in the base driver. I wrote a
slightly longer explanation in response to your other review.

> > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +       struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > +       int res = 0;
> > +
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR,
> > u8_to_reg(tm->tm_year - 100));
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH,
> > u8_to_reg(tm->tm_mon + 1));
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY,
> > u8_to_reg(tm->tm_mday));
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR,
> > u8_to_reg(tm->tm_hour));
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE,
> > u8_to_reg(tm->tm_min));
> > +       res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND,
> > u8_to_reg(tm->tm_sec));
> > +
> > +       return (res < 0) ? -EIO : 0;
> 
> 
> Hmm... (I stumbled over res |= parts)

I'll convert it to the (more verbose but also more correct) pattern of
returning each error early.

> > +static const struct of_device_id ntxec_rtc_of_match[] = {
> > +       { .compatible = "netronix,ntxec-rtc" },
> > +       { },
> 
> 
> No need for comma in terminator line.

Okay


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v2 01/10] dt-bindings: Add vendor prefix for Netronix, Inc.

[Rob Herring]

On Sat, 05 Sep 2020 15:32:21 +0200, Jonathan Neuschäfer wrote:
> Netronix, Inc. (http://www.netronixinc.com/) makes ebook reader board
> designs, which are for example used in Kobo and Tolino devices.
> 
> An alternative prefix for Netronix would be "ntx", which is already used
> in code released by Netronix. It is shorter, but perhaps less clear.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> v2:
> - No changes
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

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

Re: [PATCH v2 02/10] dt-bindings: mfd: Add binding for Netronix's embedded controller

[Rob Herring]

On Sat, Sep 05, 2020 at 03:32:22PM +0200, Jonathan Neuschäfer wrote:
> This EC is found in e-book readers of multiple brands (e.g. Kobo,
> Tolino), and is typically implemented as a TI MSP430 microcontroller.
> 
> It controls different functions of the system, such as power on/off,
> RTC, PWM for the backlight. The exact functionality provided can vary
> between boards.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> v2:
> - Add the plaintext DT binding for comparison
> 
> 
> For reference, here is the binding in text form:
> 
>   Netronix Embedded Controller
> 
>   This EC is found in e-book readers of multiple brands (e.g. Kobo, Tolino), and
>   is typically implemented as a TI MSP430 microcontroller.
> 
> 
>   Required properties:
>   - compatible: should be "netronix,ntxec"
>   - reg: The I2C address of the EC
> 
>   Optional properties:
>   - system-power-controller:
>     See Documentation/devicetree/bindings/power/power-controller.txt
>   - interrupts or interrupts-extended
>   - interrupt-controller
>   - #interrupt-cells: Should be 1
> 
>   Optional subnodes:
> 
>   Sub-nodes are identified by their compatible string.
> 
>    compatible string              | description
>   --------------------------------|--------------------------------------
>    netronix,ntxec-pwm             | PWM (used for backlight)
>    netronix,ntxec-rtc             | real time clock
> 
> 
>   Example:
> 
>   &i2c3 {
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&pinctrl_i2c3>;
>   	status = "okay";
> 
>   	ec: embedded-controller@43 {
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&pinctrl_ntxec>;
> 
>   		compatible = "netronix,ntxec";
>   		reg = <0x43>;
>   		system-power-controller;
>   		interrupt-parent = <&gpio4>;
>   		interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
>   		interrupt-controller;
>   		#interrupt-cells = <1>;
> 
>   		pwm {
>   			compatible = "netronix,ntxec-pwm";
>   			#pwm-cells = <2>;
>   		};
> 
>   		rtc {
>   			compatible = "netronix,ntxec-rtc";
>   			interrupts-extended = <&ec 15>;
>   			interrupt-names = "alarm";
>   		};
>   	};
>   };
> ---
>  .../bindings/mfd/netronix,ntxec.yaml          | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> new file mode 100644
> index 0000000000000..596df460f98eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/netronix,ntxec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Netronix Embedded Controller
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +description: |
> +  This EC is found in e-book readers of multiple brands (e.g. Kobo, Tolino), and
> +  is typically implemented as a TI MSP430 microcontroller.
> +
> +
> +properties:
> +  compatible:
> +    const: netronix,ntxec
> +
> +  reg:
> +    items:
> +      - description: The I2C address of the EC
> +
> +  system-power-controller:
> +    type: boolean
> +    description: See Documentation/devicetree/bindings/power/power-controller.txt
> +
> +  interrupts:
> +    minItems: 1
> +    description:
> +      The EC can signal interrupts via a GPIO line
> +
> +required:
> +  - compatible
> +  - reg

additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            ec: embedded-controller@43 {
> +                    pinctrl-names = "default";
> +                    pinctrl-0 = <&pinctrl_ntxec>;
> +
> +                    compatible = "netronix,ntxec";
> +                    reg = <0x43>;
> +                    system-power-controller;
> +                    interrupt-parent = <&gpio4>;
> +                    interrupts = <11 IRQ_TYPE_EDGE_FALLING>;

> +                    interrupt-controller;
> +                    #interrupt-cells = <1>;

These need to be documented too.

> +            };
> +    };
> --
> 2.28.0
> 

Re: [PATCH v2 04/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC

[Rob Herring]

On Sat, Sep 05, 2020 at 03:32:24PM +0200, Jonathan Neuschäfer wrote:
> The Netronix embedded controller as found in Kobo Aura and Tolino Shine
> supports one PWM channel, which is used to control the frontlight
> brightness on these devices.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v2:
> - Add plaintext binding to patch description, for comparison
> - Fix pwm-cells property (should be 2, not 1)
> - Add dummy regulator to example, because the pwm-backlight binding requires a
>   power supply
> 
> 
> For reference, here is the binding in text form:
> 
> 
>   PWM functionality in Netronix Embedded Controller
> 
>   Required properties:
>   - compatible: should be "netronix,ntxec-pwm"
>   - #pwm-cells: should be 2.
> 
>   Available PWM channels:
>   - 0: The PWM channel controlled by registers 0xa1-0xa7
> 
>   Example:
> 
>   	embedded-controller@43 {
>   		compatible = "netronix,ntxec";
>   		...
> 
>   		ec_pwm: pwm {
>   			compatible = "netronix,ntxec-pwm";
>   			#pwm-cells = <1>;
>   		};
>   	};
> 
>   	...
> 
>   	backlight {
>   		compatible = "pwm-backlight";
>   		pwms = <&ec_pwm 0 50000>;
>   	};
> ---
>  .../bindings/mfd/netronix,ntxec.yaml          | 19 +++++++++++
>  .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
>  2 files changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> index 596df460f98eb..73c873dda3e70 100644
> --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> @@ -31,6 +31,9 @@ properties:
>      description:
>        The EC can signal interrupts via a GPIO line
> 
> +  pwm:
> +    $ref: ../pwm/netronix,ntxec-pwm.yaml
> +
>  required:
>    - compatible
>    - reg
> @@ -53,5 +56,21 @@ examples:
>                      interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
>                      interrupt-controller;
>                      #interrupt-cells = <1>;
> +
> +                    ec_pwm: pwm {
> +                            compatible = "netronix,ntxec-pwm";
> +                            #pwm-cells = <2>;
> +                    };
>              };
>      };
> +
> +    backlight {
> +            compatible = "pwm-backlight";
> +            pwms = <&ec_pwm 0 50000>;
> +            power-supply = <&backlight_regulator>;
> +    };
> +
> +    backlight_regulator: regulator-dummy {
> +            compatible = "regulator-fixed";
> +            regulator-name = "backlight";
> +    };
> diff --git a/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> new file mode 100644
> index 0000000000000..0c9d2801b8de1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/netronix,ntxec-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PWM functionality in Netronix embedded controller
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +description: |
> +  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> +
> +  The Netronix EC contains PWM functionality, which is usually used to drive
> +  the backlight LED.
> +
> +  The following PWM channels are supported:
> +    - 0: The PWM channel controlled by registers 0xa1-0xa7
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: netronix,ntxec-pwm
> +
> +  "#pwm-cells":
> +    const: 2

Just move this to the parent and make the parent a pwm provider. There's 
no need for child nodes for this or the rtc.

Rob

Re: [PATCH v2 04/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC

[Andreas Kemnade]

Hi,

On Mon, 14 Sep 2020 18:54:43 -0600
Rob Herring <robh@kernel.org> wrote:

> On Sat, Sep 05, 2020 at 03:32:24PM +0200, Jonathan Neuschäfer wrote:
> > The Netronix embedded controller as found in Kobo Aura and Tolino Shine
> > supports one PWM channel, which is used to control the frontlight
> > brightness on these devices.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> > 
> > v2:
> > - Add plaintext binding to patch description, for comparison
> > - Fix pwm-cells property (should be 2, not 1)
> > - Add dummy regulator to example, because the pwm-backlight binding requires a
> >   power supply
> > 
> > 
> > For reference, here is the binding in text form:
> > 
> > 
> >   PWM functionality in Netronix Embedded Controller
> > 
> >   Required properties:
> >   - compatible: should be "netronix,ntxec-pwm"
> >   - #pwm-cells: should be 2.
> > 
> >   Available PWM channels:
> >   - 0: The PWM channel controlled by registers 0xa1-0xa7
> > 
> >   Example:
> > 
> >   	embedded-controller@43 {
> >   		compatible = "netronix,ntxec";
> >   		...
> > 
> >   		ec_pwm: pwm {
> >   			compatible = "netronix,ntxec-pwm";
> >   			#pwm-cells = <1>;
> >   		};
> >   	};
> > 
> >   	...
> > 
> >   	backlight {
> >   		compatible = "pwm-backlight";
> >   		pwms = <&ec_pwm 0 50000>;
> >   	};
> > ---
> >  .../bindings/mfd/netronix,ntxec.yaml          | 19 +++++++++++
> >  .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > index 596df460f98eb..73c873dda3e70 100644
> > --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > @@ -31,6 +31,9 @@ properties:
> >      description:
> >        The EC can signal interrupts via a GPIO line
> > 
> > +  pwm:
> > +    $ref: ../pwm/netronix,ntxec-pwm.yaml
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -53,5 +56,21 @@ examples:
> >                      interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
> >                      interrupt-controller;
> >                      #interrupt-cells = <1>;
> > +
> > +                    ec_pwm: pwm {
> > +                            compatible = "netronix,ntxec-pwm";
> > +                            #pwm-cells = <2>;
> > +                    };
> >              };
> >      };
> > +
> > +    backlight {
> > +            compatible = "pwm-backlight";
> > +            pwms = <&ec_pwm 0 50000>;
> > +            power-supply = <&backlight_regulator>;
> > +    };
> > +
> > +    backlight_regulator: regulator-dummy {
> > +            compatible = "regulator-fixed";
> > +            regulator-name = "backlight";
> > +    };
> > diff --git a/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > new file mode 100644
> > index 0000000000000..0c9d2801b8de1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > @@ -0,0 +1,33 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/netronix,ntxec-pwm.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: PWM functionality in Netronix embedded controller
> > +
> > +maintainers:
> > +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > +
> > +description: |
> > +  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > +
> > +  The Netronix EC contains PWM functionality, which is usually used to drive
> > +  the backlight LED.
> > +
> > +  The following PWM channels are supported:
> > +    - 0: The PWM channel controlled by registers 0xa1-0xa7
> > +
> > +allOf:
> > +  - $ref: pwm.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: netronix,ntxec-pwm
> > +
> > +  "#pwm-cells":
> > +    const: 2  
> 
> Just move this to the parent and make the parent a pwm provider. There's 
> no need for child nodes for this or the rtc.
> 
hmm, there are apparently devices without rtc. If there is a child node
for the rtc, the corresponding devicetrees could disable rtc by not
having that node.
But maybe using the controller version is also feasible for that task.

Regards,
Andreas

Re: [PATCH v2 04/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC

[Rob Herring]

On Tue, Sep 15, 2020 at 12:24 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi,
>
> On Mon, 14 Sep 2020 18:54:43 -0600
> Rob Herring <robh@kernel.org> wrote:
>
> > On Sat, Sep 05, 2020 at 03:32:24PM +0200, Jonathan Neuschäfer wrote:
> > > The Netronix embedded controller as found in Kobo Aura and Tolino Shine
> > > supports one PWM channel, which is used to control the frontlight
> > > brightness on these devices.
> > >
> > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > > ---
> > >
> > > v2:
> > > - Add plaintext binding to patch description, for comparison
> > > - Fix pwm-cells property (should be 2, not 1)
> > > - Add dummy regulator to example, because the pwm-backlight binding requires a
> > >   power supply
> > >
> > >
> > > For reference, here is the binding in text form:
> > >
> > >
> > >   PWM functionality in Netronix Embedded Controller
> > >
> > >   Required properties:
> > >   - compatible: should be "netronix,ntxec-pwm"
> > >   - #pwm-cells: should be 2.
> > >
> > >   Available PWM channels:
> > >   - 0: The PWM channel controlled by registers 0xa1-0xa7
> > >
> > >   Example:
> > >
> > >     embedded-controller@43 {
> > >             compatible = "netronix,ntxec";
> > >             ...
> > >
> > >             ec_pwm: pwm {
> > >                     compatible = "netronix,ntxec-pwm";
> > >                     #pwm-cells = <1>;
> > >             };
> > >     };
> > >
> > >     ...
> > >
> > >     backlight {
> > >             compatible = "pwm-backlight";
> > >             pwms = <&ec_pwm 0 50000>;
> > >     };
> > > ---
> > >  .../bindings/mfd/netronix,ntxec.yaml          | 19 +++++++++++
> > >  .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
> > >  2 files changed, 52 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > > index 596df460f98eb..73c873dda3e70 100644
> > > --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > > +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > > @@ -31,6 +31,9 @@ properties:
> > >      description:
> > >        The EC can signal interrupts via a GPIO line
> > >
> > > +  pwm:
> > > +    $ref: ../pwm/netronix,ntxec-pwm.yaml
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > > @@ -53,5 +56,21 @@ examples:
> > >                      interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
> > >                      interrupt-controller;
> > >                      #interrupt-cells = <1>;
> > > +
> > > +                    ec_pwm: pwm {
> > > +                            compatible = "netronix,ntxec-pwm";
> > > +                            #pwm-cells = <2>;
> > > +                    };
> > >              };
> > >      };
> > > +
> > > +    backlight {
> > > +            compatible = "pwm-backlight";
> > > +            pwms = <&ec_pwm 0 50000>;
> > > +            power-supply = <&backlight_regulator>;
> > > +    };
> > > +
> > > +    backlight_regulator: regulator-dummy {
> > > +            compatible = "regulator-fixed";
> > > +            regulator-name = "backlight";
> > > +    };
> > > diff --git a/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > > new file mode 100644
> > > index 0000000000000..0c9d2801b8de1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> > > @@ -0,0 +1,33 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pwm/netronix,ntxec-pwm.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: PWM functionality in Netronix embedded controller
> > > +
> > > +maintainers:
> > > +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > > +
> > > +description: |
> > > +  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > > +
> > > +  The Netronix EC contains PWM functionality, which is usually used to drive
> > > +  the backlight LED.
> > > +
> > > +  The following PWM channels are supported:
> > > +    - 0: The PWM channel controlled by registers 0xa1-0xa7
> > > +
> > > +allOf:
> > > +  - $ref: pwm.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: netronix,ntxec-pwm
> > > +
> > > +  "#pwm-cells":
> > > +    const: 2
> >
> > Just move this to the parent and make the parent a pwm provider. There's
> > no need for child nodes for this or the rtc.
> >
> hmm, there are apparently devices without rtc. If there is a child node
> for the rtc, the corresponding devicetrees could disable rtc by not
> having that node.
> But maybe using the controller version is also feasible for that task.

If not probeable, then the compatible string should distinguish that.

Rob

Re: [PATCH v2 02/10] dt-bindings: mfd: Add binding for Netronix's embedded controller

[Jonathan Neuschäfer]

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

On Mon, Sep 14, 2020 at 06:50:34PM -0600, Rob Herring wrote:
> On Sat, Sep 05, 2020 at 03:32:22PM +0200, Jonathan Neuschäfer wrote:
> > This EC is found in e-book readers of multiple brands (e.g. Kobo,
> > Tolino), and is typically implemented as a TI MSP430 microcontroller.
[...]

> > +required:
> > +  - compatible
> > +  - reg
> 
> additionalProperties: false

Ok, I'll add that.

> > +                    interrupt-controller;
> > +                    #interrupt-cells = <1>;
> 
> These need to be documented too.

Interrupt support is something I haven't really worked out yet for this
(set of) binding(s). My idea here was to have the embedded-controller
node act as an interrupt controller, and the subnodes use specific
interrupts that are relevant for their functionality.

I think I'll rather omit the interrupt-controller and #interrupt-cells
properties now, and add them later if they become necessary. If the pwm
and rtc nodes are merged into the main node, I don't think they will
become necessary.


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v2 04/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Tue, Sep 15, 2020 at 08:31:55AM -0600, Rob Herring wrote:
> On Tue, Sep 15, 2020 at 12:24 AM Andreas Kemnade <andreas@kemnade.info> wrote:
> > On Mon, 14 Sep 2020 18:54:43 -0600 Rob Herring <robh@kernel.org> wrote:
[...]
> > > Just move this to the parent and make the parent a pwm provider. There's
> > > no need for child nodes for this or the rtc.
> > >
> > hmm, there are apparently devices without rtc. If there is a child node
> > for the rtc, the corresponding devicetrees could disable rtc by not
> > having that node.
> > But maybe using the controller version is also feasible for that task.
> 
> If not probeable, then the compatible string should distinguish that.

Okay.

It's even simpler in some cases: The firmware version reported by the EC
should tell us if it's one that is known to have no RTC.

That said, I don't have a good overview of the different variants of
this device.


Thanks,
Jonathan Neuschäfer

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