[PATCH v4 0/7] Netronix embedded controller driver for Kobo and Tolino ebook readers

[PATCH v4 0/7] 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].

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

v4:
- Spell out ODM (original design manufacturer)
- Solve corner cases in the RTC driver
- Clean up use of log levels vs. error codes
- Add more comments explaining some peculiarities
- Add missing MODULE_ALIAS lines
- Various other cleanups


v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-1-j.neuschaefer@gmx.net/
- A few code cleanups
- A few devicetree related cleanups
- PWM and RTC functionality were moved from subnodes in the devicetree to
  the main node. This also means that the subdrivers no longer need DT
  compatible strings, but are instead loaded via the mfd_cell mechanism.
- The drivers are now published under GPLv2-or-later rather than GPLv2-only.


v2:
- https://lore.kernel.org/lkml/20200905133230.1014581-1-j.neuschaefer@gmx.net/
- 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 (7):
  dt-bindings: Add vendor prefix for Netronix, Inc.
  dt-bindings: mfd: Add binding for Netronix embedded controller
  mfd: Add base driver for Netronix embedded controller
  pwm: ntxec: Add driver for PWM function in Netronix EC
  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          |  76 ++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   9 +
 arch/arm/boot/dts/imx50-kobo-aura.dts         |  16 +-
 drivers/mfd/Kconfig                           |  11 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/ntxec.c                           | 216 ++++++++++++++++++
 drivers/pwm/Kconfig                           |   8 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ntxec.c                       | 166 ++++++++++++++
 drivers/rtc/Kconfig                           |   8 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-ntxec.c                       | 158 +++++++++++++
 include/linux/mfd/ntxec.h                     |  34 +++
 14 files changed, 706 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.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.29.2

[PATCH v4 1/7] 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>
Acked-by: Rob Herring <robh@kernel.org>
---
v4:
- No changes

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-2-j.neuschaefer@gmx.net/
- Add Acked-by tag

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 2735be1a84709..cbf28f992b71e 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -726,6 +726,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.29.2

[PATCH v4 2/7] dt-bindings: mfd: Add binding for Netronix 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---

v4:
- Add R-b tag

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-3-j.neuschaefer@gmx.net/
- Remove binding in text form patch description again
- Add additionalProperties: false
- Remove interrupt-controller property from example
- Merge pwm/rtc nodes into main node

v2:
- https://lore.kernel.org/lkml/20200905133230.1014581-3-j.neuschaefer@gmx.net/
- Add the plaintext DT binding for comparison
---
 .../bindings/mfd/netronix,ntxec.yaml          | 76 +++++++++++++++++++
 1 file changed, 76 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..59a630025f52f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -0,0 +1,76 @@
+# 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
+
+  "#pwm-cells":
+    const: 2
+    description: |
+      Number of cells in a PWM specifier.
+
+      The following PWM channels are supported:
+        - 0: The PWM channel controlled by registers 0xa1-0xa7
+
+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>;
+                    #pwm-cells = <2>;
+            };
+    };
+
+    backlight {
+            compatible = "pwm-backlight";
+            pwms = <&ec 0 50000>;
+            power-supply = <&backlight_regulator>;
+    };
+
+    backlight_regulator: regulator-dummy {
+            compatible = "regulator-fixed";
+            regulator-name = "backlight";
+    };
--
2.29.2

[PATCH v4 3/7] 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 original design manufacturer 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>
---

v4:
- include asm/unaligned.h after linux/*
- Use put_unaligned_be16 instead of open-coded big-endian packing
- Clarify that 0x90=0xff00 causes an error in downstream kernel too
- Add commas after non-sentinel positions
- ntxec.h: declare structs device and regmap
- Replace WARN_ON usage and add comments to explain errors
- Replace dev_alert with dev_warn when the result isn't handled
- Change subdevice registration error message to dev_err
- Declare ntxec_reg8 as returning __be16
- Restructure version detection code
- Spell out ODM

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-4-j.neuschaefer@gmx.net/
- Add (EC) to CONFIG_MFD_NTXEC prompt
- Relicense as GPLv2 or later
- Add email address to copyright line
- remove empty lines in ntxec_poweroff and ntxec_restart functions
- Split long lines
- Remove 'Install ... handler' comments
- Make naming of struct i2c_client parameter consistent
- Remove struct ntxec_info
- Rework 'depends on' lines in Kconfig, hard-depend on I2C, select REGMAP_I2C and
  MFD_CORE
- Register subdevices via mfd_cells
- Move 8-bit register conversion to ntxec.h

v2:
- https://lore.kernel.org/lkml/20200905133230.1014581-4-j.neuschaefer@gmx.net/
- 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       |  11 ++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/ntxec.c       | 216 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ntxec.h |  34 ++++++
 4 files changed, 262 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 8b99a13669bfc..d96751f884dc6 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -990,6 +990,17 @@ 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 (EC)"
+	depends on OF || COMPILE_TEST
+	depends on I2C
+	select REGMAP_I2C
+	select MFD_CORE
+	help
+	  Say yes here if you want to support the embedded controller found in
+	  certain e-book readers designed by the original design manufacturer
+	  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 1780019d24748..815c99b84019e 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -218,6 +218,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..c1510711d7363
--- /dev/null
+++ b/drivers/mfd/ntxec.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the original design manufacturer 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 <j.neuschaefer@gmx.net>
+ */
+
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <asm/unaligned.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[3] = { NTXEC_REG_POWEROFF };
+	struct i2c_msg msgs[] = {
+		{
+			.addr = poweroff_restart_client->addr,
+			.flags = 0,
+			.len = sizeof(buf),
+			.buf = buf,
+		},
+	};
+
+	put_unaligned_be16(NTXEC_POWEROFF_VALUE, buf + 1);
+
+	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (res < 0)
+		dev_warn(&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;
+	u8 buf[3] = { NTXEC_REG_RESET };
+	/*
+	 * NOTE: The lower half of the reset value is not sent, because sending
+	 * it causes an I2C error. (The reset handler in the downstream driver
+	 * does send the full two-byte value, but doesn't check the result).
+	 */
+	struct i2c_msg msgs[] = {
+		{
+			.addr = poweroff_restart_client->addr,
+			.flags = 0,
+			.len = sizeof(buf) - 1,
+			.buf = buf,
+		},
+	};
+
+	put_unaligned_be16(NTXEC_RESET_VALUE, buf + 1);
+
+	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (res < 0)
+		dev_warn(&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 mfd_cell ntxec_subdevices[] = {
+	{ .name = "ntxec-rtc" },
+	{ .name = "ntxec-pwm" },
+};
+
+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;
+	}
+
+	/* Bail out if we encounter an unknown firmware version */
+	switch (version) {
+	case 0xd726: /* found in Kobo Aura */
+		break;
+	default:
+		dev_err(ec->dev,
+			"Netronix embedded controller version %04x is not supported.\n",
+			version);
+		return -ENODEV;
+	}
+
+	dev_info(ec->dev,
+		 "Netronix embedded controller version %04x detected.\n", version);
+
+	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;
+
+		if (poweroff_restart_client)
+			/*
+			 * Another instance of the driver already took
+			 * poweroff/restart duties.
+			 */
+			dev_err(ec->dev, "poweroff_restart_client already assigned\n");
+		else
+			poweroff_restart_client = client;
+
+		if (pm_power_off)
+			/* Another driver already registered a poweroff handler. */
+			dev_err(ec->dev, "pm_power_off already assigned\n");
+		else
+			pm_power_off = ntxec_poweroff;
+
+		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);
+
+	res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
+				   ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
+	if (res)
+		dev_err(ec->dev, "Failed to add subdevices: %d\n", res);
+
+	return res;
+}
+
+static int ntxec_remove(struct i2c_client *client)
+{
+	if (client == 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..b1de91f4fdcad
--- /dev/null
+++ b/include/linux/mfd/ntxec.h
@@ -0,0 +1,34 @@
+/* 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 device;
+struct regmap;
+
+struct ntxec {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+/*
+ * 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 function converts an 8-bit value to 16-bit for use in the
+ * second kind of register.
+ */
+static inline __be16 ntxec_reg8(u8 value)
+{
+	return value << 8;
+}
+
+#endif
--
2.29.2

[PATCH v4 4/7] 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.

The .get_state callback is not implemented, because the PWM state can't
be read back from the hardware.

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

v4:
- Document hardware/driver limitations
- Only accept normal polarity
- Fix a typo ("zone" -> "zero")
- change MAX_PERIOD_NS to 0xffff * 125
- Clamp period to the maximum rather than returning an error
- Rename private struct pointer to priv
- Rearrage control flow in _probe to save a few lines and a temporary variable
- Add missing MODULE_ALIAS line
- Spell out ODM

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-5-j.neuschaefer@gmx.net/
- Relicense as GPLv2 or later
- Add email address to copyright line
- Remove OF compatible string and don't include linux/of_device.h
- Fix bogus ?: in return line
- Don't use a comma after sentinels
- Avoid ret |= ... pattern
- Move 8-bit register conversion to ntxec.h

v2:
- https://lore.kernel.org/lkml/20200905133230.1014581-6-j.neuschaefer@gmx.net/
- 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 | 166 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/pwm/pwm-ntxec.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 63be5362fd3a5..815f329ed5b46 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
+	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 original
+	  design manufacturer 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 cbdcd55d69eef..1deb29e6ae8e5 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..4f4f736d71aba
--- /dev/null
+++ b/drivers/pwm/pwm-ntxec.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the original design manufacturer Netronix, Inc.
+ * It contains RTC, battery monitoring, system power management, and PWM
+ * functionality.
+ *
+ * This driver implements PWM output.
+ *
+ * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+ *
+ * Limitations:
+ * - The get_state callback is not implemented, because the current state of
+ *   the PWM output can't be read back from the hardware.
+ * - The hardware can only generate normal polarity output.
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.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_priv(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
+
+/*
+ * 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 * 0xffff)
+
+static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+			   const struct pwm_state *state)
+{
+	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
+	unsigned int duty = state->duty_cycle;
+	unsigned int period = state->period;
+	int res = 0;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (period > MAX_PERIOD_NS) {
+		period = MAX_PERIOD_NS;
+
+		if (duty > period)
+			duty = period;
+	}
+
+	period /= TIME_BASE_NS;
+	duty /= TIME_BASE_NS;
+
+	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
+	if (res)
+		return res;
+
+	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
+	if (res)
+		return res;
+
+	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
+	if (res)
+		return res;
+
+	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
+	if (res)
+		return res;
+
+	/*
+	 * Writing a duty cycle of zero 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
+		if (res)
+			return res;
+
+		/* Disable the auto-off timer */
+		res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
+		if (res)
+			return res;
+
+		return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
+	} else {
+		return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(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 *priv;
+	struct pwm_chip *chip;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ec = ec;
+	priv->dev = &pdev->dev;
+
+	platform_set_drvdata(pdev, priv);
+
+	chip = &priv->chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &ntxec_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	return pwmchip_add(chip);
+}
+
+static int ntxec_pwm_remove(struct platform_device *pdev)
+{
+	struct ntxec_pwm *priv = platform_get_drvdata(pdev);
+	struct pwm_chip *chip = &priv->chip;
+
+	return pwmchip_remove(chip);
+}
+
+static struct platform_driver ntxec_pwm_driver = {
+	.driver = {
+		.name = "ntxec-pwm",
+	},
+	.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");
+MODULE_ALIAS("platform:ntxec-pwm");
--
2.29.2

[PATCH v4 5/7] 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>
---

v4:
- Remove "driver" from Kconfig entry for consistency with most other entries
- Add missing MODULE_ALIAS line
- Give NTXEC_REG_READ_ macros longer names
- Solve the read tearing issue using Alexandre Belloni's algorithm
- Solve the write tearing issue using Uwe Kleine-König's algorithm
- Spell out ODM

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-6-j.neuschaefer@gmx.net/
- Add email address to copyright line
- Remove OF compatible string and don't include linux/of_device.h
- Don't use a comma after sentinels
- Avoid ret |= ... pattern
- Move 8-bit register conversion to ntxec.h
- Relicense as GPLv2 or later

v2:
- https://lore.kernel.org/lkml/20200905133230.1014581-7-j.neuschaefer@gmx.net/
- 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 | 158 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/rtc/rtc-ntxec.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 65ad9d0b47ab1..fe009949728b3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1311,6 +1311,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"
+	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
+	  original design manufacturer Netronix.
+
 comment "on-CPU RTC drivers"

 config RTC_DRV_ASM9260
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index bfb57464118d0..5f2a7582b2780 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..dad5aba2852f1
--- /dev/null
+++ b/drivers/rtc/rtc-ntxec.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the original design manufacturer 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 <j.neuschaefer@gmx.net>
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.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
+
+#define NTXEC_REG_READ_YEAR_MONTH	0x20
+#define NTXEC_REG_READ_MDAY_HOUR	0x21
+#define NTXEC_REG_READ_MINUTE_SECOND	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;
+
+retry:
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MINUTE_SECOND, &value);
+	if (res < 0)
+		return res;
+
+	tm->tm_min = value >> 8;
+	tm->tm_sec = value & 0xff;
+
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MDAY_HOUR, &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_YEAR_MONTH, &value);
+	if (res < 0)
+		return res;
+
+	tm->tm_year = (value >> 8) + 100;
+	tm->tm_mon = (value & 0xff) - 1;
+
+	/*
+	 * Read the minutes/seconds field again. If it changed since the first
+	 * read, we can't assume that the values read so far are consistent,
+	 * and should start from the beginning.
+	 */
+	res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MINUTE_SECOND, &value);
+	if (res < 0)
+		return res;
+
+	if (tm->tm_min != value >> 8 || tm->tm_sec != (value & 0xff))
+		goto retry;
+
+	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;
+
+	/*
+	 * To avoid time overflows while we're writing the full date/time,
+	 * set the seconds field to zero before doing anything else. For the
+	 * next 59 seconds (plus however long it takes until the RTC's next
+	 * update of the second field), the seconds field will not overflow
+	 * into the other fields.
+	 */
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(0));
+	if (res)
+		return res;
+
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
+	if (res)
+		return res;
+
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
+	if (res)
+		return res;
+
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
+	if (res)
+		return res;
+
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
+	if (res)
+		return res;
+
+	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
+	if (res)
+		return res;
+
+	return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
+}
+
+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 = 9025257599LL; /* 2255-12-31 23:59:59 */
+
+	return rtc_register_device(dev);
+}
+
+static struct platform_driver ntxec_rtc_driver = {
+	.driver = {
+		.name = "ntxec-rtc",
+	},
+	.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");
+MODULE_ALIAS("platform:ntxec-rtc");
--
2.29.2

[PATCH v4 6/7] 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>
---
v4:
- No changes

v3:
- https://lore.kernel.org/lkml/20200924192455.2484005-7-j.neuschaefer@gmx.net/
- Remove pwm and rtc bindings

v2:
- https://lore.kernel.org/lkml/20200905144503.1067124-2-j.neuschaefer@gmx.net/
- No changes
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e451dcce054f0..ea4a6c7cfb8dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12100,6 +12100,15 @@ 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:	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.29.2

Re: [PATCH v4 5/7] rtc: New driver for RTC in Netronix embedded controller

[Alexandre Belloni]

Hi,

On 22/11/2020 23:27:37+0100, Jonathan Neuschäfer wrote:
> 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>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

However, two comments below:

> +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> +	int res = 0;
> +
> +	/*
> +	 * To avoid time overflows while we're writing the full date/time,
> +	 * set the seconds field to zero before doing anything else. For the
> +	 * next 59 seconds (plus however long it takes until the RTC's next
> +	 * update of the second field), the seconds field will not overflow
> +	 * into the other fields.
> +	 */
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(0));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> +	if (res)
> +		return res;
> +
> +	return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));

Couldn't you do a regmap_block_write or a regmap_multi_reg_write which
would be more efficient as they would be locking the regmap only once.

> +}
> +
> +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 = 9025257599LL; /* 2255-12-31 23:59:59 */
> +
> +	return rtc_register_device(dev);

Note that this won't compile after
https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?id=fdcfd854333be5b30377dc5daa9cd0fa1643a979

We can solve that with immutable branches though.

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

[PATCH v4 7/7] 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>
---
v4:
- Add 'grp' suffix to pinctrl node

v3:
- https://lore.kernel.org/lkml/20200925050818.2512375-1-j.neuschaefer@gmx.net/
- Remove interrupt-controller property from embedded-controller node
- subnodes of embedded-controller node in to the main node

v2:
- https://lore.kernel.org/lkml/20200905144503.1067124-3-j.neuschaefer@gmx.net/
- Fix pwm-cells property (should be 2, not 1)
---
 arch/arm/boot/dts/imx50-kobo-aura.dts | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx50-kobo-aura.dts b/arch/arm/boot/dts/imx50-kobo-aura.dts
index 97cfd970fe742..82ce8c43be867 100644
--- a/arch/arm/boot/dts/imx50-kobo-aura.dts
+++ b/arch/arm/boot/dts/imx50-kobo-aura.dts
@@ -143,10 +143,24 @@ &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>;
+		#pwm-cells = <2>;
+	};
 };

 &iomuxc {
+	pinctrl_ec: ecgrp {
+		fsl,pins = <
+			MX50_PAD_CSPI_SS0__GPIO4_11		0x0	/* INT */
+		>;
+	};
+
 	pinctrl_gpiokeys: gpiokeysgrp {
 		fsl,pins = <
 			MX50_PAD_CSPI_MISO__GPIO4_10		0x0
--
2.29.2

Re: [PATCH v4 5/7] rtc: New driver for RTC in Netronix embedded controller

[Jonathan Neuschäfer]

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

On Mon, Nov 23, 2020 at 12:10:54AM +0100, Alexandre Belloni wrote:
> Hi,
> 
> On 22/11/2020 23:27:37+0100, Jonathan Neuschäfer wrote:
> > 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>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> 
> However, two comments below:
> 
> > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > +	int res = 0;
> > +
> > +	/*
> > +	 * To avoid time overflows while we're writing the full date/time,
> > +	 * set the seconds field to zero before doing anything else. For the
> > +	 * next 59 seconds (plus however long it takes until the RTC's next
> > +	 * update of the second field), the seconds field will not overflow
> > +	 * into the other fields.
> > +	 */
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(0));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > +	if (res)
> > +		return res;
> > +
> > +	return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> 
> Couldn't you do a regmap_block_write or a regmap_multi_reg_write which
> would be more efficient as they would be locking the regmap only once.

I can't find regmap_block_write anywhere, but regmap_multi_reg_write
looks like a good approach to simplify the code here.


[...]
> Note that this won't compile after
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?id=fdcfd854333be5b30377dc5daa9cd0fa1643a979
> 
> We can solve that with immutable branches though.

Thanks for the heads-up. Please let me know if/when there is any action
that I need to take here.


Jonathan

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

Re: [PATCH v4 5/7] rtc: New driver for RTC in Netronix embedded controller

[Mark Brown]

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

On Mon, Nov 23, 2020 at 10:31:05PM +0100, Jonathan Neuschäfer wrote:
> On Mon, Nov 23, 2020 at 12:10:54AM +0100, Alexandre Belloni wrote:

> > Couldn't you do a regmap_block_write or a regmap_multi_reg_write which
> > would be more efficient as they would be locking the regmap only once.

> I can't find regmap_block_write anywhere, but regmap_multi_reg_write
> looks like a good approach to simplify the code here.

I suspect he's thinking of bulk rather than block there.

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

Re: [PATCH v4 5/7] rtc: New driver for RTC in Netronix embedded controller

[Alexandre Belloni]

On 23/11/2020 22:31:05+0100, Jonathan Neuschäfer wrote:
> > > +	res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > > +	if (res)
> > > +		return res;
> > > +
> > > +	return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> > 
> > Couldn't you do a regmap_block_write or a regmap_multi_reg_write which
> > would be more efficient as they would be locking the regmap only once.
> 
> I can't find regmap_block_write anywhere, but regmap_multi_reg_write
> looks like a good approach to simplify the code here.
> 

I was thinking regmap_bulk_write but regmap_multi_reg_write is probably
more fitting here.

> 
> [...]
> > Note that this won't compile after
> > https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/commit/?id=fdcfd854333be5b30377dc5daa9cd0fa1643a979
> > 
> > We can solve that with immutable branches though.
> 
> Thanks for the heads-up. Please let me know if/when there is any action
> that I need to take here.
> 

I wouldn't think so, I can carry a patch once Lee provides his usual
immutable branch.



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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Uwe Kleine-König]

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

Hello,

On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer 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.
> 
> The .get_state callback is not implemented, because the PWM state can't
> be read back from the hardware.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
> 
> v4:
> - Document hardware/driver limitations
> - Only accept normal polarity
> - Fix a typo ("zone" -> "zero")
> - change MAX_PERIOD_NS to 0xffff * 125
> - Clamp period to the maximum rather than returning an error
> - Rename private struct pointer to priv
> - Rearrage control flow in _probe to save a few lines and a temporary variable
> - Add missing MODULE_ALIAS line
> - Spell out ODM
> 
> v3:
> - https://lore.kernel.org/lkml/20200924192455.2484005-5-j.neuschaefer@gmx.net/
> - Relicense as GPLv2 or later
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Fix bogus ?: in return line
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
> 
> v2:
> - https://lore.kernel.org/lkml/20200905133230.1014581-6-j.neuschaefer@gmx.net/
> - 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 | 166 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 drivers/pwm/pwm-ntxec.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 63be5362fd3a5..815f329ed5b46 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
> +	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 original
> +	  design manufacturer 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 cbdcd55d69eef..1deb29e6ae8e5 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..4f4f736d71aba
> --- /dev/null
> +++ b/drivers/pwm/pwm-ntxec.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the original design manufacturer Netronix, Inc.
> + * It contains RTC, battery monitoring, system power management, and PWM
> + * functionality.
> + *
> + * This driver implements PWM output.
> + *
> + * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> + *
> + * Limitations:
> + * - The get_state callback is not implemented, because the current state of
> + *   the PWM output can't be read back from the hardware.
> + * - The hardware can only generate normal polarity output.
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.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_priv(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
> +
> +/*
> + * 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 * 0xffff)
> +
> +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> +			   const struct pwm_state *state)
> +{
> +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> +	unsigned int duty = state->duty_cycle;
> +	unsigned int period = state->period;

state->duty_cycle and state->period are u64, so you're losing
information here. Consider state->duty_cycle = 0x100000001 and
state->period = 0x200000001.

> +	int res = 0;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (period > MAX_PERIOD_NS) {
> +		period = MAX_PERIOD_NS;
> +
> +		if (duty > period)
> +			duty = period;
> +	}
> +
> +	period /= TIME_BASE_NS;
> +	duty /= TIME_BASE_NS;
> +
> +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> +	if (res)
> +		return res;

I wonder if you can add some logic to the regmap in the mfd driver such
that ntxec_reg8 isn't necessary for all users.

> +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> +	if (res)
> +		return res;

I think I already asked, but I don't remember the reply: What happens to
the output between these writes? A comment here about this would be
suitable.

> +
> +	/*
> +	 * Writing a duty cycle of zero 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> +		if (res)
> +			return res;
> +
> +		/* Disable the auto-off timer */
> +		res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> +		if (res)
> +			return res;
> +
> +		return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> +	} else {
> +		return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> +	}
> +}
> +
> +static struct pwm_ops ntxec_pwm_ops = {

This can be const.

> +	.apply = ntxec_pwm_apply,

/*
 * The current state cannot be read out, so there is no .get_state
 * callback.
 */

Hmm, at least you could provice a .get_state() callback that reports the
setting that was actually implemented for in the last call to .apply()?

@Thierry: Do you have concerns here? Actually it would be more effective
to have a callback (like .apply()) that modfies its pwm_state
accordingly. (Some drivers did that in the past, but I changed that to
get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
The downside is that people have to understand that concept to properly
use it. I'm torn about the right approach.

> +	.owner = THIS_MODULE,
> +};
> +
> +static int ntxec_pwm_probe(struct platform_device *pdev)
> +{
> +	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct ntxec_pwm *priv;
> +	struct pwm_chip *chip;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->ec = ec;
> +	priv->dev = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	chip = &priv->chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &ntxec_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	return pwmchip_add(chip);
> +}
> +
> +static int ntxec_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ntxec_pwm *priv = platform_get_drvdata(pdev);
> +	struct pwm_chip *chip = &priv->chip;
> +
> +	return pwmchip_remove(chip);
> +}
> +
> +static struct platform_driver ntxec_pwm_driver = {
> +	.driver = {
> +		.name = "ntxec-pwm",
> +	},
> +	.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");
> +MODULE_ALIAS("platform:ntxec-pwm");

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
[...]
> > +/*
> > + * 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 * 0xffff)
> > +
> > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > +			   const struct pwm_state *state)
> > +{
> > +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > +	unsigned int duty = state->duty_cycle;
> > +	unsigned int period = state->period;
> 
> state->duty_cycle and state->period are u64, so you're losing
> information here. Consider state->duty_cycle = 0x100000001 and
> state->period = 0x200000001.

Oh, good point, I didn't notice the truncation.

The reason I picked unsigned int was to avoid a 64-bit division;
I suppose I can do something like this:

    period = (u32)period / TIME_BASE_NS;
    duty = (u32)duty / TIME_BASE_NS;

> > +	int res = 0;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	if (period > MAX_PERIOD_NS) {
> > +		period = MAX_PERIOD_NS;
> > +
> > +		if (duty > period)
> > +			duty = period;
> > +	}
> > +
> > +	period /= TIME_BASE_NS;
> > +	duty /= TIME_BASE_NS;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > +	if (res)
> > +		return res;
> 
> I wonder if you can add some logic to the regmap in the mfd driver such
> that ntxec_reg8 isn't necessary for all users.

I think that would involve:

1. adding custom register access functions to the regmap, which decide
   based on the register number whether a register needs 8-bit or 16-bit
   access. So far I have avoided information about registers into the
   main driver, when the registers are only used in the sub-drivers.

or

2. switching the regmap configuration to little endian, which would be
   advantageous for 8-bit registers, inconsequential for 16-bit
   registers that consist of independent high and low halves, and wrong
   for the 16-bit registers 0x41, which reads the battery voltage ADC
   value. It is also different from how the vendor kernel treats 16-bit
   registers.

Perhaps there is another option that I haven't considered yet.

> 
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > +	if (res)
> > +		return res;
> 
> I think I already asked, but I don't remember the reply: What happens to
> the output between these writes? A comment here about this would be
> suitable.

I will add something like the following:

/*
 * Changes to the period and duty cycle take effect as soon as the
 * corresponding low byte is written, so the hardware may be configured
 * to an inconsistent state after the period is written and before the
 * duty cycle is fully written. If, in such a case, the old duty cycle
 * is longer than the new period, the EC will output 100% for a moment.
 */

> 
> > +
> > +	/*
> > +	 * Writing a duty cycle of zero 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > +		if (res)
> > +			return res;
> > +
> > +		/* Disable the auto-off timer */
> > +		res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > +		if (res)
> > +			return res;
> > +
> > +		return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > +	} else {
> > +		return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > +	}
> > +}
> > +
> > +static struct pwm_ops ntxec_pwm_ops = {
> 
> This can be const.

Indeed, I'll change it.

> > +	.apply = ntxec_pwm_apply,
> 
> /*
>  * The current state cannot be read out, so there is no .get_state
>  * callback.
>  */
> 
> Hmm, at least you could provice a .get_state() callback that reports the
> setting that was actually implemented for in the last call to .apply()?

Yes... I see two options:

1. Caching the state in the driver's private struct. I'm not completely
   convinced of the value, given that the information is mostly
   available in the PWM core already (except for the adjustments that
   the driver makes).

2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
   This seems a bit dirty.

> @Thierry: Do you have concerns here? Actually it would be more effective
> to have a callback (like .apply()) that modfies its pwm_state
> accordingly. (Some drivers did that in the past, but I changed that to
> get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> The downside is that people have to understand that concept to properly
> use it. I'm torn about the right approach.

General guidance for such cases when the state can't be read back from
the hardware would be appreciated.


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Uwe Kleine-König]

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

Hello Jonathan,

On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
> [...]
> > > +/*
> > > + * 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 * 0xffff)
> > > +
> > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > +			   const struct pwm_state *state)
> > > +{
> > > +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > +	unsigned int duty = state->duty_cycle;
> > > +	unsigned int period = state->period;
> > 
> > state->duty_cycle and state->period are u64, so you're losing
> > information here. Consider state->duty_cycle = 0x100000001 and
> > state->period = 0x200000001.
> 
> Oh, good point, I didn't notice the truncation.
> 
> The reason I picked unsigned int was to avoid a 64-bit division;
> I suppose I can do something like this:
> 
>     period = (u32)period / TIME_BASE_NS;
>     duty = (u32)duty / TIME_BASE_NS;

You can do that after you checked period > MAX_PERIOD_NS below, yes.
Something like:

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -EINVAL;

	if (state->period > MAX_PERIOD_NS) {
		period = MAX_PERIOD_NS;
	else
		period = state->period;

	if (state->duty_cycle > period)
		duty_cycle = period;
	else
		duty_cycle = state->duty_cycle;

should work with even keeping the local variables as unsigned int.

> > > +	int res = 0;
> > > +
> > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	if (period > MAX_PERIOD_NS) {
> > > +		period = MAX_PERIOD_NS;
> > > +
> > > +		if (duty > period)
> > > +			duty = period;
> > > +	}
> > > +
> > > +	period /= TIME_BASE_NS;
> > > +	duty /= TIME_BASE_NS;
> > > +
> > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > +	if (res)
> > > +		return res;
> > 
> > I wonder if you can add some logic to the regmap in the mfd driver such
> > that ntxec_reg8 isn't necessary for all users.
> 
> I think that would involve:
> 
> 1. adding custom register access functions to the regmap, which decide
>    based on the register number whether a register needs 8-bit or 16-bit
>    access. So far I have avoided information about registers into the
>    main driver, when the registers are only used in the sub-drivers.
> 
> or
> 
> 2. switching the regmap configuration to little endian, which would be
>    advantageous for 8-bit registers, inconsequential for 16-bit
>    registers that consist of independent high and low halves, and wrong
>    for the 16-bit registers 0x41, which reads the battery voltage ADC
>    value. It is also different from how the vendor kernel treats 16-bit
>    registers.
> 
> Perhaps there is another option that I haven't considered yet.

I don't know enough about regmap to teach you something here. But maybe
Mark has an idea. (I promoted him from Cc: to To:, maybe he will
notice.)

> > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > +	if (res)
> > > +		return res;
> > > +
> > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > +	if (res)
> > > +		return res;
> > > +
> > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > +	if (res)
> > > +		return res;
> > 
> > I think I already asked, but I don't remember the reply: What happens to
> > the output between these writes? A comment here about this would be
> > suitable.
> 
> I will add something like the following:
> 
> /*
>  * Changes to the period and duty cycle take effect as soon as the
>  * corresponding low byte is written, so the hardware may be configured
>  * to an inconsistent state after the period is written and before the
>  * duty cycle is fully written. If, in such a case, the old duty cycle
>  * is longer than the new period, the EC will output 100% for a moment.
>  */

Is the value pair taken over by hardware atomically? That is, is it
really "will" in your last line, or only "might". (E.g. when changing
from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
after reducing period, the new duty_cycle is probably written before the
counter reaches 500. Do we get a 100% cycle here?)

Other than that the info is fine. Make sure to point this out in the
Limitations paragraph at the top of the driver please, too.

> > > +	.apply = ntxec_pwm_apply,
> > 
> > /*
> >  * The current state cannot be read out, so there is no .get_state
> >  * callback.
> >  */
> > 
> > Hmm, at least you could provice a .get_state() callback that reports the
> > setting that was actually implemented for in the last call to .apply()?
> 
> Yes... I see two options:
> 
> 1. Caching the state in the driver's private struct. I'm not completely
>    convinced of the value, given that the information is mostly
>    available in the PWM core already (except for the adjustments that
>    the driver makes).
> 
> 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
>    This seems a bit dirty.

2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
can be made doing that)?

> > @Thierry: Do you have concerns here? Actually it would be more effective
> > to have a callback (like .apply()) that modfies its pwm_state
> > accordingly. (Some drivers did that in the past, but I changed that to
> > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > The downside is that people have to understand that concept to properly
> > use it. I'm torn about the right approach.
> 
> General guidance for such cases when the state can't be read back from
> the hardware would be appreciated.

Yes, improving the documentation would be great here. Thierry, can you
please comment on
https://lore.kernel.org/r/20191209213233.29574-2-u.kleine-koenig@pengutronix.de
which I'm waiting on before describing our understanding in more detail.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Thierry Reding]

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

On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-König wrote:
> Hello Jonathan,
> 
> On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
> > [...]
> > > > +/*
> > > > + * 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 * 0xffff)
> > > > +
> > > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > > +			   const struct pwm_state *state)
> > > > +{
> > > > +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > > +	unsigned int duty = state->duty_cycle;
> > > > +	unsigned int period = state->period;
> > > 
> > > state->duty_cycle and state->period are u64, so you're losing
> > > information here. Consider state->duty_cycle = 0x100000001 and
> > > state->period = 0x200000001.
> > 
> > Oh, good point, I didn't notice the truncation.
> > 
> > The reason I picked unsigned int was to avoid a 64-bit division;
> > I suppose I can do something like this:
> > 
> >     period = (u32)period / TIME_BASE_NS;
> >     duty = (u32)duty / TIME_BASE_NS;
> 
> You can do that after you checked period > MAX_PERIOD_NS below, yes.
> Something like:
> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 		return -EINVAL;
> 
> 	if (state->period > MAX_PERIOD_NS) {
> 		period = MAX_PERIOD_NS;
> 	else
> 		period = state->period;
> 
> 	if (state->duty_cycle > period)
> 		duty_cycle = period;
> 	else
> 		duty_cycle = state->duty_cycle;
> 
> should work with even keeping the local variables as unsigned int.
> 
> > > > +	int res = 0;
> > > > +
> > > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (period > MAX_PERIOD_NS) {
> > > > +		period = MAX_PERIOD_NS;
> > > > +
> > > > +		if (duty > period)
> > > > +			duty = period;
> > > > +	}
> > > > +
> > > > +	period /= TIME_BASE_NS;
> > > > +	duty /= TIME_BASE_NS;
> > > > +
> > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > > +	if (res)
> > > > +		return res;
> > > 
> > > I wonder if you can add some logic to the regmap in the mfd driver such
> > > that ntxec_reg8 isn't necessary for all users.
> > 
> > I think that would involve:
> > 
> > 1. adding custom register access functions to the regmap, which decide
> >    based on the register number whether a register needs 8-bit or 16-bit
> >    access. So far I have avoided information about registers into the
> >    main driver, when the registers are only used in the sub-drivers.
> > 
> > or
> > 
> > 2. switching the regmap configuration to little endian, which would be
> >    advantageous for 8-bit registers, inconsequential for 16-bit
> >    registers that consist of independent high and low halves, and wrong
> >    for the 16-bit registers 0x41, which reads the battery voltage ADC
> >    value. It is also different from how the vendor kernel treats 16-bit
> >    registers.
> > 
> > Perhaps there is another option that I haven't considered yet.
> 
> I don't know enough about regmap to teach you something here. But maybe
> Mark has an idea. (I promoted him from Cc: to To:, maybe he will
> notice.)
> 
> > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > > +	if (res)
> > > > +		return res;
> > > > +
> > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > > +	if (res)
> > > > +		return res;
> > > > +
> > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > > +	if (res)
> > > > +		return res;
> > > 
> > > I think I already asked, but I don't remember the reply: What happens to
> > > the output between these writes? A comment here about this would be
> > > suitable.
> > 
> > I will add something like the following:
> > 
> > /*
> >  * Changes to the period and duty cycle take effect as soon as the
> >  * corresponding low byte is written, so the hardware may be configured
> >  * to an inconsistent state after the period is written and before the
> >  * duty cycle is fully written. If, in such a case, the old duty cycle
> >  * is longer than the new period, the EC will output 100% for a moment.
> >  */
> 
> Is the value pair taken over by hardware atomically? That is, is it
> really "will" in your last line, or only "might". (E.g. when changing
> from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> after reducing period, the new duty_cycle is probably written before the
> counter reaches 500. Do we get a 100% cycle here?)
> 
> Other than that the info is fine. Make sure to point this out in the
> Limitations paragraph at the top of the driver please, too.

Perhaps also use something like regmap_bulk_write() to make sure the
time between these writes is a short as possible.

> 
> > > > +	.apply = ntxec_pwm_apply,
> > > 
> > > /*
> > >  * The current state cannot be read out, so there is no .get_state
> > >  * callback.
> > >  */
> > > 
> > > Hmm, at least you could provice a .get_state() callback that reports the
> > > setting that was actually implemented for in the last call to .apply()?
> > 
> > Yes... I see two options:
> > 
> > 1. Caching the state in the driver's private struct. I'm not completely
> >    convinced of the value, given that the information is mostly
> >    available in the PWM core already (except for the adjustments that
> >    the driver makes).
> > 
> > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> >    This seems a bit dirty.
> 
> 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> can be made doing that)?
> 
> > > @Thierry: Do you have concerns here? Actually it would be more effective
> > > to have a callback (like .apply()) that modfies its pwm_state
> > > accordingly. (Some drivers did that in the past, but I changed that to
> > > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > > The downside is that people have to understand that concept to properly
> > > use it. I'm torn about the right approach.
> > 
> > General guidance for such cases when the state can't be read back from
> > the hardware would be appreciated.
> 
> Yes, improving the documentation would be great here. Thierry, can you
> please comment on
> https://lore.kernel.org/r/20191209213233.29574-2-u.kleine-koenig@pengutronix.de
> which I'm waiting on before describing our understanding in more detail.

Hm... that link gives me a "Not Found" message. Anyway, I think perhaps
the best compromise would be for the core to provide an implementation
of ->get_state() that drivers can use if they can't read out hardware
state. This generic implementation would then just copy over the
internal state and we have to trust that that's really what was applied.

One drawback of that is that we don't factor in things like rounding
errors and other limitations. So a better alternative may be to require
drivers to store a cached version of the state and return that in their
->get_state() implementation.

Or perhaps a hybrid of the above would work where the core provides the
helper that copies cached state and a cached state structure for storage
and then the drivers that can't properly read back hardware state just
need to update the cached state during ->apply().

I slightly prefer variant 2 because it's not clear to me how often we'll
need this and we can always easily convert to variant 3 if this becomes
a more common thing to do.

Thierry

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Uwe Kleine-König]

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

On Fri, Nov 27, 2020 at 12:08:51PM +0100, Thierry Reding wrote:
> On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-König wrote:
> > Hello Jonathan,
> > 
> > On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> > > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> > > > On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
> > > [...]
> > > > > +/*
> > > > > + * 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 * 0xffff)
> > > > > +
> > > > > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > > > > +			   const struct pwm_state *state)
> > > > > +{
> > > > > +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > > > > +	unsigned int duty = state->duty_cycle;
> > > > > +	unsigned int period = state->period;
> > > > 
> > > > state->duty_cycle and state->period are u64, so you're losing
> > > > information here. Consider state->duty_cycle = 0x100000001 and
> > > > state->period = 0x200000001.
> > > 
> > > Oh, good point, I didn't notice the truncation.
> > > 
> > > The reason I picked unsigned int was to avoid a 64-bit division;
> > > I suppose I can do something like this:
> > > 
> > >     period = (u32)period / TIME_BASE_NS;
> > >     duty = (u32)duty / TIME_BASE_NS;
> > 
> > You can do that after you checked period > MAX_PERIOD_NS below, yes.
> > Something like:
> > 
> > 	if (state->polarity != PWM_POLARITY_NORMAL)
> > 		return -EINVAL;
> > 
> > 	if (state->period > MAX_PERIOD_NS) {
> > 		period = MAX_PERIOD_NS;
> > 	else
> > 		period = state->period;
> > 
> > 	if (state->duty_cycle > period)
> > 		duty_cycle = period;
> > 	else
> > 		duty_cycle = state->duty_cycle;
> > 
> > should work with even keeping the local variables as unsigned int.
> > 
> > > > > +	int res = 0;
> > > > > +
> > > > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (period > MAX_PERIOD_NS) {
> > > > > +		period = MAX_PERIOD_NS;
> > > > > +
> > > > > +		if (duty > period)
> > > > > +			duty = period;
> > > > > +	}
> > > > > +
> > > > > +	period /= TIME_BASE_NS;
> > > > > +	duty /= TIME_BASE_NS;
> > > > > +
> > > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > > > > +	if (res)
> > > > > +		return res;
> > > > 
> > > > I wonder if you can add some logic to the regmap in the mfd driver such
> > > > that ntxec_reg8 isn't necessary for all users.
> > > 
> > > I think that would involve:
> > > 
> > > 1. adding custom register access functions to the regmap, which decide
> > >    based on the register number whether a register needs 8-bit or 16-bit
> > >    access. So far I have avoided information about registers into the
> > >    main driver, when the registers are only used in the sub-drivers.
> > > 
> > > or
> > > 
> > > 2. switching the regmap configuration to little endian, which would be
> > >    advantageous for 8-bit registers, inconsequential for 16-bit
> > >    registers that consist of independent high and low halves, and wrong
> > >    for the 16-bit registers 0x41, which reads the battery voltage ADC
> > >    value. It is also different from how the vendor kernel treats 16-bit
> > >    registers.
> > > 
> > > Perhaps there is another option that I haven't considered yet.
> > 
> > I don't know enough about regmap to teach you something here. But maybe
> > Mark has an idea. (I promoted him from Cc: to To:, maybe he will
> > notice.)
> > 
> > > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > > > > +	if (res)
> > > > > +		return res;
> > > > > +
> > > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > > > > +	if (res)
> > > > > +		return res;
> > > > > +
> > > > > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > > > > +	if (res)
> > > > > +		return res;
> > > > 
> > > > I think I already asked, but I don't remember the reply: What happens to
> > > > the output between these writes? A comment here about this would be
> > > > suitable.
> > > 
> > > I will add something like the following:
> > > 
> > > /*
> > >  * Changes to the period and duty cycle take effect as soon as the
> > >  * corresponding low byte is written, so the hardware may be configured
> > >  * to an inconsistent state after the period is written and before the
> > >  * duty cycle is fully written. If, in such a case, the old duty cycle
> > >  * is longer than the new period, the EC will output 100% for a moment.
> > >  */
> > 
> > Is the value pair taken over by hardware atomically? That is, is it
> > really "will" in your last line, or only "might". (E.g. when changing
> > from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> > after reducing period, the new duty_cycle is probably written before the
> > counter reaches 500. Do we get a 100% cycle here?)
> > 
> > Other than that the info is fine. Make sure to point this out in the
> > Limitations paragraph at the top of the driver please, too.
> 
> Perhaps also use something like regmap_bulk_write() to make sure the
> time between these writes is a short as possible.
> 
> > 
> > > > > +	.apply = ntxec_pwm_apply,
> > > > 
> > > > /*
> > > >  * The current state cannot be read out, so there is no .get_state
> > > >  * callback.
> > > >  */
> > > > 
> > > > Hmm, at least you could provice a .get_state() callback that reports the
> > > > setting that was actually implemented for in the last call to .apply()?
> > > 
> > > Yes... I see two options:
> > > 
> > > 1. Caching the state in the driver's private struct. I'm not completely
> > >    convinced of the value, given that the information is mostly
> > >    available in the PWM core already (except for the adjustments that
> > >    the driver makes).
> > > 
> > > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> > >    This seems a bit dirty.
> > 
> > 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> > can be made doing that)?
> > 
> > > > @Thierry: Do you have concerns here? Actually it would be more effective
> > > > to have a callback (like .apply()) that modfies its pwm_state
> > > > accordingly. (Some drivers did that in the past, but I changed that to
> > > > get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> > > > The downside is that people have to understand that concept to properly
> > > > use it. I'm torn about the right approach.
> > > 
> > > General guidance for such cases when the state can't be read back from
> > > the hardware would be appreciated.
> > 
> > Yes, improving the documentation would be great here. Thierry, can you
> > please comment on
> > https://lore.kernel.org/r/20191209213233.29574-2-u.kleine-koenig@pengutronix.de
> > which I'm waiting on before describing our understanding in more detail.
> 
> Hm... that link gives me a "Not Found" message. Anyway, I think perhaps

I thought I tested that before sending the link, but it gives Not Found
for me, too.
https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/
is the patchwork link.

> the best compromise would be for the core to provide an implementation
> of ->get_state() that drivers can use if they can't read out hardware
> state. This generic implementation would then just copy over the
> internal state and we have to trust that that's really what was applied.
> 
> One drawback of that is that we don't factor in things like rounding
> errors and other limitations. So a better alternative may be to require
> drivers to store a cached version of the state and return that in their
> ->get_state() implementation.

Yes. That's what Jonathan called 1.

> Or perhaps a hybrid of the above would work where the core provides the
> helper that copies cached state and a cached state structure for storage
> and then the drivers that can't properly read back hardware state just
> need to update the cached state during ->apply().

I thought about doing

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index a13ff383fa1d..c60a638ebfad 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -261,7 +261,7 @@ struct pwm_ops {
 	int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
 		       struct pwm_capture *result, unsigned long timeout);
 	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
-		     const struct pwm_state *state);
+		     struct pwm_state *state);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
 	struct module *owner;

and adapting the callbacks to provide the actually implemented setting
in *state. But I don't like this because providing this is (for probably
most drivers) extra effort. So the drivers should only cache the written
register values and calculate the actual timing from these values if and
when required. Providing help from the framework for this is more
complicated because different drivers have different needs here. (Two
u64 for duty_cycle and period is probably enough for everybody?)

> I slightly prefer variant 2 because it's not clear to me how often we'll
> need this and we can always easily convert to variant 3 if this becomes
> a more common thing to do.

I cannot follow because I don't know what variant is "variant 2" and
"variant 3" for you.

I suggest for now we go for "don't provide a .get_state() callback"
because in the only case where it is currently called there is no cached
data to rely on anyhow.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Fri, Nov 27, 2020 at 08:11:05AM +0100, Uwe Kleine-König wrote:
> Hello Jonathan,
> 
> On Fri, Nov 27, 2020 at 12:19:31AM +0100, Jonathan Neuschäfer wrote:
> > On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
[...]
> > > state->duty_cycle and state->period are u64, so you're losing
> > > information here. Consider state->duty_cycle = 0x100000001 and
> > > state->period = 0x200000001.
> > 
> > Oh, good point, I didn't notice the truncation.
> > 
> > The reason I picked unsigned int was to avoid a 64-bit division;
> > I suppose I can do something like this:
> > 
> >     period = (u32)period / TIME_BASE_NS;
> >     duty = (u32)duty / TIME_BASE_NS;
> 
> You can do that after you checked period > MAX_PERIOD_NS below, yes.
> Something like:
> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 		return -EINVAL;
> 
> 	if (state->period > MAX_PERIOD_NS) {
> 		period = MAX_PERIOD_NS;
> 	else
> 		period = state->period;
> 
> 	if (state->duty_cycle > period)
> 		duty_cycle = period;
> 	else
> 		duty_cycle = state->duty_cycle;
> 
> should work with even keeping the local variables as unsigned int.

With the min_t() macro, this becomes nice and short:

	 period = min_t(u64, state->period, MAX_PERIOD_NS);
	 duty   = min_t(u64, state->duty_cycle, period);

	 period /= TIME_BASE_NS;
	 duty   /= TIME_BASE_NS;


> > > I think I already asked, but I don't remember the reply: What happens to
> > > the output between these writes? A comment here about this would be
> > > suitable.
> > 
> > I will add something like the following:
> > 
> > /*
> >  * Changes to the period and duty cycle take effect as soon as the
> >  * corresponding low byte is written, so the hardware may be configured
> >  * to an inconsistent state after the period is written and before the
> >  * duty cycle is fully written. If, in such a case, the old duty cycle
> >  * is longer than the new period, the EC will output 100% for a moment.
> >  */
> 
> Is the value pair taken over by hardware atomically? That is, is it
> really "will" in your last line, or only "might". (E.g. when changing
> from duty_cycle, period = 1000, 2000 to 500, 800 and a new cycle begins
> after reducing period, the new duty_cycle is probably written before the
> counter reaches 500. Do we get a 100% cycle here?)

I am not sure when exactly a new period or duty cycle value is applied,
and I don't have the test equipment to measure it. I'll change the text
to "may output 100%".

> Other than that the info is fine. Make sure to point this out in the
> Limitations paragraph at the top of the driver please, too.

Okay.


> > > /*
> > >  * The current state cannot be read out, so there is no .get_state
> > >  * callback.
> > >  */
> > > 
> > > Hmm, at least you could provice a .get_state() callback that reports the
> > > setting that was actually implemented for in the last call to .apply()?
> > 
> > Yes... I see two options:
> > 
> > 1. Caching the state in the driver's private struct. I'm not completely
> >    convinced of the value, given that the information is mostly
> >    available in the PWM core already (except for the adjustments that
> >    the driver makes).
> > 
> > 2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
> >    This seems a bit dirty.
> 
> 2. isn't a good option. Maybe regmap caches this stuff anyhow for 1. (or
> can be made doing that)?

With regmap caching, I'd be concerned that a read operation may slip
through and reach the device, producing a bogus result. Not sure if
write-only/write-through caching can be configured in regmap.


Thanks,
Jonathan

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

Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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

On Fri, Nov 27, 2020 at 03:23:25PM +0100, Uwe Kleine-König wrote:
[...]
> I suggest for now we go for "don't provide a .get_state() callback"
> because in the only case where it is currently called there is no cached
> data to rely on anyhow.

Alright!


Thanks,
Jonathan

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

Re: [PATCH v4 3/7] mfd: Add base driver for Netronix embedded controller

[Lee Jones]

On Sun, 22 Nov 2020, Jonathan Neuschäfer wrote:

> The Netronix embedded controller is a microcontroller found in some
> e-book readers designed by the original design manufacturer 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>
> ---
> 
> v4:
> - include asm/unaligned.h after linux/*
> - Use put_unaligned_be16 instead of open-coded big-endian packing
> - Clarify that 0x90=0xff00 causes an error in downstream kernel too
> - Add commas after non-sentinel positions
> - ntxec.h: declare structs device and regmap
> - Replace WARN_ON usage and add comments to explain errors
> - Replace dev_alert with dev_warn when the result isn't handled
> - Change subdevice registration error message to dev_err
> - Declare ntxec_reg8 as returning __be16
> - Restructure version detection code
> - Spell out ODM
> 
> v3:
> - https://lore.kernel.org/lkml/20200924192455.2484005-4-j.neuschaefer@gmx.net/
> - Add (EC) to CONFIG_MFD_NTXEC prompt
> - Relicense as GPLv2 or later
> - Add email address to copyright line
> - remove empty lines in ntxec_poweroff and ntxec_restart functions
> - Split long lines
> - Remove 'Install ... handler' comments
> - Make naming of struct i2c_client parameter consistent
> - Remove struct ntxec_info
> - Rework 'depends on' lines in Kconfig, hard-depend on I2C, select REGMAP_I2C and
>   MFD_CORE
> - Register subdevices via mfd_cells
> - Move 8-bit register conversion to ntxec.h
> 
> v2:
> - https://lore.kernel.org/lkml/20200905133230.1014581-4-j.neuschaefer@gmx.net/
> - 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       |  11 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 216 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  34 ++++++
>  4 files changed, 262 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 8b99a13669bfc..d96751f884dc6 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -990,6 +990,17 @@ 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 (EC)"
> +	depends on OF || COMPILE_TEST
> +	depends on I2C
> +	select REGMAP_I2C
> +	select MFD_CORE
> +	help
> +	  Say yes here if you want to support the embedded controller found in
> +	  certain e-book readers designed by the original design manufacturer
> +	  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 1780019d24748..815c99b84019e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -218,6 +218,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..c1510711d7363
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the original design manufacturer 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 <j.neuschaefer@gmx.net>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <asm/unaligned.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[3] = { NTXEC_REG_POWEROFF };
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = poweroff_restart_client->addr,
> +			.flags = 0,
> +			.len = sizeof(buf),
> +			.buf = buf,
> +		},
> +	};
> +
> +	put_unaligned_be16(NTXEC_POWEROFF_VALUE, buf + 1);
> +
> +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		dev_warn(&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;
> +	u8 buf[3] = { NTXEC_REG_RESET };
> +	/*
> +	 * NOTE: The lower half of the reset value is not sent, because sending
> +	 * it causes an I2C error. (The reset handler in the downstream driver
> +	 * does send the full two-byte value, but doesn't check the result).
> +	 */
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = poweroff_restart_client->addr,
> +			.flags = 0,
> +			.len = sizeof(buf) - 1,
> +			.buf = buf,
> +		},
> +	};
> +
> +	put_unaligned_be16(NTXEC_RESET_VALUE, buf + 1);
> +
> +	res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		dev_warn(&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 mfd_cell ntxec_subdevices[] = {
> +	{ .name = "ntxec-rtc" },
> +	{ .name = "ntxec-pwm" },
> +};
> +
> +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;
> +	}
> +
> +	/* Bail out if we encounter an unknown firmware version */
> +	switch (version) {
> +	case 0xd726: /* found in Kobo Aura */

No magic numbers.

Please submit a subsequent patch to define this.

> +		break;
> +	default:
> +		dev_err(ec->dev,
> +			"Netronix embedded controller version %04x is not supported.\n",
> +			version);
> +		return -ENODEV;
> +	}


Applied, thanks.

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

Re: [PATCH v4 3/7] mfd: Add base driver for Netronix embedded controller

[Lee Jones]

On Wed, 02 Dec 2020, Lee Jones wrote:

> On Sun, 22 Nov 2020, Jonathan Neuschäfer wrote:
> 
> > The Netronix embedded controller is a microcontroller found in some
> > e-book readers designed by the original design manufacturer 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>
> > ---
> > 
> > v4:
> > - include asm/unaligned.h after linux/*
> > - Use put_unaligned_be16 instead of open-coded big-endian packing
> > - Clarify that 0x90=0xff00 causes an error in downstream kernel too
> > - Add commas after non-sentinel positions
> > - ntxec.h: declare structs device and regmap
> > - Replace WARN_ON usage and add comments to explain errors
> > - Replace dev_alert with dev_warn when the result isn't handled
> > - Change subdevice registration error message to dev_err
> > - Declare ntxec_reg8 as returning __be16
> > - Restructure version detection code
> > - Spell out ODM
> > 
> > v3:
> > - https://lore.kernel.org/lkml/20200924192455.2484005-4-j.neuschaefer@gmx.net/
> > - Add (EC) to CONFIG_MFD_NTXEC prompt
> > - Relicense as GPLv2 or later
> > - Add email address to copyright line
> > - remove empty lines in ntxec_poweroff and ntxec_restart functions
> > - Split long lines
> > - Remove 'Install ... handler' comments
> > - Make naming of struct i2c_client parameter consistent
> > - Remove struct ntxec_info
> > - Rework 'depends on' lines in Kconfig, hard-depend on I2C, select REGMAP_I2C and
> >   MFD_CORE
> > - Register subdevices via mfd_cells
> > - Move 8-bit register conversion to ntxec.h
> > 
> > v2:
> > - https://lore.kernel.org/lkml/20200905133230.1014581-4-j.neuschaefer@gmx.net/
> > - 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       |  11 ++
> >  drivers/mfd/Makefile      |   1 +
> >  drivers/mfd/ntxec.c       | 216 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/ntxec.h |  34 ++++++
> >  4 files changed, 262 insertions(+)
> >  create mode 100644 drivers/mfd/ntxec.c
> >  create mode 100644 include/linux/mfd/ntxec.h

[...]

> > +	/* Bail out if we encounter an unknown firmware version */
> > +	switch (version) {
> > +	case 0xd726: /* found in Kobo Aura */
> 
> No magic numbers.
> 
> Please submit a subsequent patch to define this.
> 
> > +		break;
> > +	default:
> > +		dev_err(ec->dev,
> > +			"Netronix embedded controller version %04x is not supported.\n",
> > +			version);
> > +		return -ENODEV;
> > +	}
> 
> Applied, thanks.

Sorry, that should have been:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

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

Re: [PATCH v4 3/7] mfd: Add base driver for Netronix embedded controller

[Jonathan Neuschäfer]

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

On Wed, Dec 02, 2020 at 01:05:20PM +0000, Lee Jones wrote:
> On Sun, 22 Nov 2020, Jonathan Neuschäfer wrote:
[...]
> > +	/* Bail out if we encounter an unknown firmware version */
> > +	switch (version) {
> > +	case 0xd726: /* found in Kobo Aura */
> 
> No magic numbers.
> 
> Please submit a subsequent patch to define this.

Will do.

But I don't think I'll be able to give it a more meaningful name than
NTXEC_VERSION_D726. I don't have a good overview of which versions
appear in which devices. "0xd726 found in Kobo Aura" only means that;
I don't know if it's the only version used in the Kobo Aura, and I don't
know if the Kobo Aura is the only device where it is used.


Thanks,
Jonathan Neuschäfer

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

Re: [PATCH v4 3/7] mfd: Add base driver for Netronix embedded controller

[Lee Jones]

On Wed, 02 Dec 2020, Jonathan Neuschäfer wrote:

> On Wed, Dec 02, 2020 at 01:05:20PM +0000, Lee Jones wrote:
> > On Sun, 22 Nov 2020, Jonathan Neuschäfer wrote:
> [...]
> > > +	/* Bail out if we encounter an unknown firmware version */
> > > +	switch (version) {
> > > +	case 0xd726: /* found in Kobo Aura */
> > 
> > No magic numbers.
> > 
> > Please submit a subsequent patch to define this.
> 
> Will do.
> 
> But I don't think I'll be able to give it a more meaningful name than
> NTXEC_VERSION_D726. I don't have a good overview of which versions
> appear in which devices. "0xd726 found in Kobo Aura" only means that;
> I don't know if it's the only version used in the Kobo Aura, and I don't
> know if the Kobo Aura is the only device where it is used.

Defines are not set in stone.

They can evolve over time as more is known.

NTXEC_KOBO_AURA would be fine for now.

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

Re: [PATCH v4 3/7] mfd: Add base driver for Netronix embedded controller

[Jonathan Neuschäfer]

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

On Wed, Dec 02, 2020 at 03:09:43PM +0000, Lee Jones wrote:
> On Wed, 02 Dec 2020, Jonathan Neuschäfer wrote:
> 
> > On Wed, Dec 02, 2020 at 01:05:20PM +0000, Lee Jones wrote:
> > > On Sun, 22 Nov 2020, Jonathan Neuschäfer wrote:
> > [...]
> > > > +	/* Bail out if we encounter an unknown firmware version */
> > > > +	switch (version) {
> > > > +	case 0xd726: /* found in Kobo Aura */
> > > 
> > > No magic numbers.
> > > 
> > > Please submit a subsequent patch to define this.
> > 
> > Will do.
> > 
> > But I don't think I'll be able to give it a more meaningful name than
> > NTXEC_VERSION_D726. I don't have a good overview of which versions
> > appear in which devices. "0xd726 found in Kobo Aura" only means that;
> > I don't know if it's the only version used in the Kobo Aura, and I don't
> > know if the Kobo Aura is the only device where it is used.
> 
> Defines are not set in stone.
> 
> They can evolve over time as more is known.
> 
> NTXEC_KOBO_AURA would be fine for now.

Alright.


Thanks,
Jonathan Neuschäfer

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