linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc.
@ 2020-06-20 22:42 Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 03/10] dt-bindings: mfd: Add binding for Netronix's embedded controller Jonathan Neuschäfer
                   ` (8 more replies)
  0 siblings, 9 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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>
---
 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 9aeab66be85fc..516c6b6668fba 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -704,6 +704,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.27.0


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

* [RFC PATCH 03/10] dt-bindings: mfd: Add binding for Netronix's embedded controller
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 04/10] mfd: Add base driver for Netronix " Jonathan Neuschäfer
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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>
---
 .../bindings/mfd/netronix,ntxec.yaml          | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml

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


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

* [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 03/10] dt-bindings: mfd: Add binding for Netronix's embedded controller Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-22 10:55   ` Lee Jones
  2020-06-27  8:17   ` Andreas Kemnade
  2020-06-20 22:42 ` [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC Jonathan Neuschäfer
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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.

Known problems:
- The reboot handler is installed in such a way that it directly calls
  into the i2c subsystem to send the reboot command to the EC. This
  means that the reboot handler may sleep, which is not allowed.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/mfd/Kconfig       |   7 ++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ntxec.h |  30 ++++++
 4 files changed, 226 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 a37d7d1713820..78410b928648e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -978,6 +978,13 @@ config MFD_VIPERBOARD
 	  You need to select the mfd cell drivers separately.
 	  The drivers do not support all features the board exposes.

+config MFD_NTXEC
+	bool "Netronix Embedded Controller"
+	depends on I2C && OF
+	help
+	  Say yes here if you want to support the embedded controller of
+	  certain e-book readers designed by the ODM Netronix.
+
 config MFD_RETU
 	tristate "Nokia Retu and Tahvo multi-function device"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9367a92f795a6..19d9391ed6f32 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..82adea34ea746
--- /dev/null
+++ b/drivers/mfd/ntxec.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright 2020 Jonathan Neuschäfer
+//
+// MFD driver for the usually MSP430-based embedded controller used in certain
+// Netronix ebook reader board designs
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mfd/ntxec.h>
+#include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <linux/reboot.h>
+#include <linux/types.h>
+
+
+#define NTXEC_VERSION		0x00
+#define NTXEC_POWEROFF		0x50
+#define NTXEC_POWERKEEP		0x70
+#define NTXEC_RESET		0x90
+
+
+/* Register access */
+
+int ntxec_read16(struct ntxec *ec, u8 addr)
+{
+	u8 request[1] = { addr };
+	u8 response[2];
+	int res;
+
+	struct i2c_msg msgs[] = {
+		{
+			.addr = ec->client->addr,
+			.flags = ec->client->flags,
+			.len = sizeof(request),
+			.buf = request
+		}, {
+			.addr = ec->client->addr,
+			.flags = ec->client->flags | I2C_M_RD,
+			.len = sizeof(response),
+			.buf = response
+		}
+	};
+
+	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (res < 0)
+		return res;
+	if (res != ARRAY_SIZE(msgs))
+		return -EIO;
+
+	return get_unaligned_be16(response);
+}
+EXPORT_SYMBOL(ntxec_read16);
+
+int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
+{
+	u8 request[3] = { addr, };
+	int res;
+
+	put_unaligned_be16(value, request + 1);
+
+	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
+					ec->client->flags);
+	if (res < 0)
+		return res;
+
+	return 0;
+}
+EXPORT_SYMBOL(ntxec_write16);
+
+int ntxec_read8(struct ntxec *ec, u8 addr)
+{
+	int res = ntxec_read16(ec, addr);
+
+	if (res < 0)
+		return res;
+
+	return (res >> 8) & 0xff;
+}
+EXPORT_SYMBOL(ntxec_read8);
+
+int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
+{
+	return ntxec_write16(ec, addr, value << 8);
+}
+EXPORT_SYMBOL(ntxec_write8);
+
+
+/* Reboot/poweroff handling */
+
+static struct ntxec *poweroff_restart_instance;
+
+static void ntxec_poweroff(void)
+{
+	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);
+	msleep(5000);
+}
+
+static int ntxec_restart(struct notifier_block *nb,
+		unsigned long action, void *data)
+{
+	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */
+	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
+	/* TODO: delay? */
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block ntxec_restart_handler = {
+	.notifier_call = ntxec_restart,
+	.priority = 128
+};
+
+
+/* Driver setup */
+
+static int ntxec_probe(struct i2c_client *client,
+			    const struct i2c_device_id *ids)
+{
+	struct ntxec *ec;
+	int res;
+
+	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->dev = &client->dev;
+	ec->client = client;
+
+	/* Determine the firmware version */
+	res = ntxec_read16(ec, NTXEC_VERSION);
+	if (res < 0) {
+		dev_dbg(ec->dev, "Failed to read firmware version number\n");
+		return res;
+	}
+	ec->version = res;
+
+	dev_info(ec->dev,
+		 "Netronix embedded controller version %04x detected.\n",
+		 ec->version);
+
+	/* For now, we don't support the new register layout. */
+	if (ntxec_has_new_layout(ec))
+		return -EOPNOTSUPP;
+
+	if (of_device_is_system_power_controller(ec->dev->of_node)) {
+		/*
+		 * Set the 'powerkeep' bit. This is necessary on some boards
+		 * in order to keep the system running.
+		 */
+		res = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
+		if (res < 0)
+			return res;
+
+		/* Install poweroff handler */
+		WARN_ON(poweroff_restart_instance);
+		poweroff_restart_instance = ec;
+		if (pm_power_off != NULL)
+			/* TODO: Refactor among all poweroff drivers */
+			dev_err(ec->dev, "pm_power_off already assigned\n");
+		else
+			pm_power_off = ntxec_poweroff;
+
+		/* Install board reset handler */
+		res = register_restart_handler(&ntxec_restart_handler);
+		if (res < 0)
+			dev_err(ec->dev,
+				"Failed to register restart handler: %d\n", res);
+	}
+
+	i2c_set_clientdata(client, ec);
+
+	return devm_of_platform_populate(ec->dev);
+}
+
+static 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		= ntxec_probe,
+};
+builtin_i2c_driver(ntxec_driver);
diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
new file mode 100644
index 0000000000000..9f9d6f2141751
--- /dev/null
+++ b/include/linux/mfd/ntxec.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Jonathan Neuschäfer
+ *
+ * MFD access functions for the Netronix embedded controller.
+ */
+
+#ifndef NTXEC_H
+#define NTXEC_H
+
+#include <linux/types.h>
+
+struct ntxec {
+	struct device *dev;
+	struct i2c_client *client;
+	u16 version;
+	/* TODO: Add a mutex to protect actions consisting of multiple accesses? */
+};
+
+static inline bool ntxec_has_new_layout(struct ntxec *ec)
+{
+	return ec->version == 0xe916;
+}
+
+int ntxec_read16(struct ntxec *ec, u8 addr);
+int ntxec_write16(struct ntxec *ec, u8 addr, u16 value);
+int ntxec_read8(struct ntxec *ec, u8 addr);
+int ntxec_write8(struct ntxec *ec, u8 addr, u8 value);
+
+#endif
--
2.27.0


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

* [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 03/10] dt-bindings: mfd: Add binding for Netronix's embedded controller Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 04/10] mfd: Add base driver for Netronix " Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-21 18:41   ` Andreas Kemnade
  2020-06-20 22:42 ` [RFC PATCH 06/10] pwm: ntxec: Add driver " Jonathan Neuschäfer
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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

Known problems:
- `make dt_binding_check` shows the following warnings:
  Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
  Warning (pwms_property): /example-0/backlight:pwms: cell 2 is not a
  phandle reference
  Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
  Warning (pwms_property): /example-0/backlight:pwms: Could not get
  phandle node for (cell 2)

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../bindings/mfd/netronix,ntxec.yaml          | 13 ++++++++
 .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml

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

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


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

* [RFC PATCH 06/10] pwm: ntxec: Add driver for PWM function in Netronix EC
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (2 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-22  8:18   ` Uwe Kleine-König
  2020-06-20 22:42 ` [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC Jonathan Neuschäfer
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/pwm/Kconfig     |   4 ++
 drivers/pwm/Makefile    |   1 +
 drivers/pwm/pwm-ntxec.c | 148 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 drivers/pwm/pwm-ntxec.c

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

+config PWM_NTXEC
+	tristate "Netronix embedded controller PWM support"
+	depends on MFD_NTXEC && OF
+
 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 a59c710e98c76..15507a6d9ca12 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_PUV3)		+= pwm-puv3.o
diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
new file mode 100644
index 0000000000000..eca305d8e915b
--- /dev/null
+++ b/drivers/pwm/pwm-ntxec.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+//
+// PWM driver for Netronix embedded controller.
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/of_device.h>
+#include <linux/types.h>
+
+struct ntxec_pwm {
+	struct device *dev;
+	struct ntxec *ec;
+	struct pwm_chip chip;
+};
+
+static struct ntxec_pwm *pwmchip_to_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ntxec_pwm, chip);
+}
+
+#define NTXEC_UNK_A		0xa1
+#define NTXEC_UNK_B		0xa2
+#define NTXEC_ENABLE		0xa3
+#define NTXEC_PERIOD_LOW	0xa4
+#define NTXEC_PERIOD_HIGH	0xa5
+#define NTXEC_DUTY_LOW		0xa6
+#define NTXEC_DUTY_HIGH		0xa7
+
+/*
+ * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
+ * measured in this unit.
+ */
+static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+				 int duty_ns, int period_ns)
+{
+	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
+	uint64_t duty = duty_ns;
+	uint64_t period = period_ns;
+	int res = 0;
+
+	do_div(period, 125);
+	if (period > 0xffff) {
+		dev_warn(pwm->dev,
+			 "Period is not representable in 16 bits: %llu\n", period);
+		return -ERANGE;
+	}
+
+	do_div(duty, 125);
+	if (duty > 0xffff) {
+		dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n",
+			duty);
+		return -ERANGE;
+	}
+
+	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
+	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
+	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
+	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);
+
+	return (res < 0) ? -EIO : 0;
+}
+
+static int ntxec_pwm_enable(struct pwm_chip *chip,
+				 struct pwm_device *pwm_dev)
+{
+	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
+
+	return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1);
+}
+
+static void ntxec_pwm_disable(struct pwm_chip *chip,
+				   struct pwm_device *pwm_dev)
+{
+	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
+
+	ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
+}
+
+static struct pwm_ops ntxec_pwm_ops = {
+	.config		= ntxec_pwm_config,
+	.enable		= ntxec_pwm_enable,
+	.disable	= ntxec_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static int ntxec_pwm_probe(struct platform_device *pdev)
+{
+	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
+	struct ntxec_pwm *pwm;
+	struct pwm_chip *chip;
+	int res;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->ec = ec;
+	pwm->dev = &pdev->dev;
+
+	chip = &pwm->chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &ntxec_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	res = pwmchip_add(chip);
+	if (res < 0)
+		return res;
+
+	platform_set_drvdata(pdev, pwm);
+
+	res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
+	res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff);
+	res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff);
+
+	return (res < 0) ? -EIO : 0;
+}
+
+static int ntxec_pwm_remove(struct platform_device *pdev)
+{
+	struct ntxec_pwm *pwm = platform_get_drvdata(pdev);
+	struct pwm_chip *chip = &pwm->chip;
+
+	return pwmchip_remove(chip);
+}
+
+static const struct of_device_id ntxec_pwm_of_match[] = {
+	{ .compatible = "netronix,ntxec-pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ntxec_pwm_of_match);
+
+static struct platform_driver ntxec_pwm_driver = {
+	.driver = {
+		.name = "ntxec-pwm",
+		.of_match_table = ntxec_pwm_of_match,
+	},
+	.probe = ntxec_pwm_probe,
+	.remove = ntxec_pwm_remove,
+};
+module_platform_driver(ntxec_pwm_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("PWM driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.27.0


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

* [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (3 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 06/10] pwm: ntxec: Add driver " Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-21  0:02   ` Alexandre Belloni
  2020-06-20 22:42 ` [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

The Netronix EC implements an RTC with the following functionality:

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

This binding only supports timekeeping for now.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../bindings/mfd/netronix,ntxec.yaml          |  7 +++++
 .../bindings/rtc/netronix,ntxec-rtc.yaml      | 27 +++++++++++++++++++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
index 6562c41c5a9a9..f6a32f46f47bb 100644
--- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -34,6 +34,9 @@ properties:
   pwm:
     $ref: ../pwm/netronix,ntxec-pwm.yaml

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

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


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

* [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (4 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-21  0:11   ` Alexandre Belloni
  2020-06-20 22:42 ` [RFC PATCH 09/10] MAINTAINERS: Add entry for " Jonathan Neuschäfer
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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>
---
 drivers/rtc/Kconfig     |   4 ++
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/rtc-ntxec.c | 115 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)
 create mode 100644 drivers/rtc/rtc-ntxec.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b54d87d45c89b..2310d08933f9c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1300,6 +1300,10 @@ config RTC_DRV_CROS_EC
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-cros-ec.

+config RTC_DRV_NTXEC
+	tristate "Netronix embedded controller RTC driver"
+	depends on MFD_NTXEC
+
 comment "on-CPU RTC drivers"

 config RTC_DRV_ASM9260
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 0721752c6ed4c..8653d04aefa99 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..44d5a5eedb597
--- /dev/null
+++ b/drivers/rtc/rtc-ntxec.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright 2020 Jonathan Neuschäfer
+
+#include <linux/rtc.h>
+#include <linux/mfd/ntxec.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+struct ntxec_rtc {
+	struct device *dev;
+	struct ntxec *ec;
+};
+
+#define NTXEC_WRITE_YEAR	0x10
+#define NTXEC_WRITE_MONTH	0x11
+#define NTXEC_WRITE_DAY		0x12
+#define NTXEC_WRITE_HOUR	0x13
+#define NTXEC_WRITE_MINUTE	0x14
+#define NTXEC_WRITE_SECOND	0x15
+
+#define NTXEC_READ_YM		0x20
+#define NTXEC_READ_DH		0x21
+#define NTXEC_READ_MS		0x22
+
+
+static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+	int res;
+
+	res = ntxec_read16(rtc->ec, NTXEC_READ_YM);
+	if (res < 0)
+		return res;
+
+	tm->tm_year = (res >> 8) + 100;
+	tm->tm_mon = (res & 0xff) - 1;
+
+	res = ntxec_read16(rtc->ec, NTXEC_READ_DH);
+	if (res < 0)
+		return res;
+
+	tm->tm_mday = res >> 8;
+	tm->tm_hour = res & 0xff;
+
+	res = ntxec_read16(rtc->ec, NTXEC_READ_MS);
+	if (res < 0)
+		return res;
+
+	tm->tm_min = res >> 8;
+	tm->tm_sec = res & 0xff;
+
+	return 0;
+}
+
+static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+	int res = 0;
+
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_YEAR, tm->tm_year - 100);
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_MONTH, tm->tm_mon + 1);
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_DAY, tm->tm_mday);
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_HOUR, tm->tm_hour);
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_MINUTE, tm->tm_min);
+	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_SECOND, tm->tm_sec);
+
+	return (res < 0)? -EIO : 0;
+}
+
+static const struct rtc_class_ops ntxec_rtc_ops = {
+	.read_time		= ntxec_read_time,
+	.set_time		= ntxec_set_time,
+};
+
+static int ntxec_rtc_probe(struct platform_device *pdev)
+{
+	struct rtc_device *rtcdev;
+	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);
+
+	rtcdev = devm_rtc_device_register(&pdev->dev, "ntxec-rtc",
+					  &ntxec_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtcdev))
+		return PTR_ERR(rtc);
+
+	return 0;
+}
+
+static const struct of_device_id ntxec_rtc_of_match[] = {
+	{ .compatible = "netronix,ntxec-rtc" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ntxec_rtc_of_match);
+
+static struct platform_driver ntxec_rtc_driver = {
+	.driver = {
+		.name = "ntxec-rtc",
+		.of_match_table = ntxec_rtc_of_match,
+	},
+	.probe = ntxec_rtc_probe,
+};
+module_platform_driver(ntxec_rtc_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("RTC driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.27.0


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

* [RFC PATCH 09/10] MAINTAINERS: Add entry for Netronix embedded controller
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (5 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-20 22:42 ` [RFC PATCH 10/10] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer
  2020-06-20 22:46 ` [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
  8 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade, Rob Herring

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>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 68f21d46614c4..d4333f7490f5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11850,6 +11850,17 @@ F:	include/net/netrom.h
 F:	include/uapi/linux/netrom.h
 F:	net/netrom/

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


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

* [RFC PATCH 10/10] ARM: dts: imx50-kobo-aura: Add Netronix embedded controller
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (6 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 09/10] MAINTAINERS: Add entry for " Jonathan Neuschäfer
@ 2020-06-20 22:42 ` Jonathan Neuschäfer
  2020-06-20 22:46 ` [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
  8 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Lee Jones, Rob Herring, Jonathan Neuschäfer, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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>
---
 arch/arm/boot/dts/imx50-kobo-aura.dts | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

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

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

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

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


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

* Re: [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc.
  2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
                   ` (7 preceding siblings ...)
  2020-06-20 22:42 ` [RFC PATCH 10/10] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer
@ 2020-06-20 22:46 ` Jonathan Neuschäfer
  8 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-20 22:46 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Lee Jones, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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

On Sun, Jun 21, 2020 at 12:42:13AM +0200, Jonathan Neuschäfer wrote:
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++

Oops, it seems the mail thread got split here when I resent patches 2-10
after an error. Oh well.

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

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

* Re: [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC
  2020-06-20 22:42 ` [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC Jonathan Neuschäfer
@ 2020-06-21  0:02   ` Alexandre Belloni
  2020-06-26 21:55     ` Andreas Kemnade
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandre Belloni @ 2020-06-21  0:02 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Lee Jones, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer, Andreas Kemnade

Hi,

On 21/06/2020 00:42:18+0200, Jonathan Neuschäfer wrote:
> The Netronix EC implements an RTC with the following functionality:
> 
> - Calendar-based time keeping with single-second resolution
> - Automatic power-on with single-minute resolution
> - Alarm at single-second resolution
> 
> This binding only supports timekeeping for now.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../bindings/mfd/netronix,ntxec.yaml          |  7 +++++
>  .../bindings/rtc/netronix,ntxec-rtc.yaml      | 27 +++++++++++++++++++
>  2 files changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> index 6562c41c5a9a9..f6a32f46f47bb 100644
> --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> @@ -34,6 +34,9 @@ properties:
>    pwm:
>      $ref: ../pwm/netronix,ntxec-pwm.yaml
> 
> +  rtc:
> +    $ref: ../rtc/netronix,ntxec-rtc.yaml
> +

Shouldn't the node simply be documented here?

Also, do you really need a compatible string to be able to proe the
driver? What are the chances that you'll get a similar EC without an
RTC?

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

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

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

* Re: [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller
  2020-06-20 22:42 ` [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer
@ 2020-06-21  0:11   ` Alexandre Belloni
  2020-07-04 19:23     ` Jonathan Neuschäfer
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandre Belloni @ 2020-06-21  0:11 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Lee Jones, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer, Andreas Kemnade

On 21/06/2020 00:42:19+0200, 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.
> 

Please report the results of rtctest (from the kernel tree) and
rtc-range
(https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c)

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/rtc/Kconfig     |   4 ++
>  drivers/rtc/Makefile    |   1 +
>  drivers/rtc/rtc-ntxec.c | 115 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+)
>  create mode 100644 drivers/rtc/rtc-ntxec.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index b54d87d45c89b..2310d08933f9c 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1300,6 +1300,10 @@ config RTC_DRV_CROS_EC
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-cros-ec.
> 
> +config RTC_DRV_NTXEC
> +	tristate "Netronix embedded controller RTC driver"
> +	depends on MFD_NTXEC
> +

This should get an help section.


>  comment "on-CPU RTC drivers"
> 
>  config RTC_DRV_ASM9260
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 0721752c6ed4c..8653d04aefa99 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..44d5a5eedb597
> --- /dev/null
> +++ b/drivers/rtc/rtc-ntxec.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2020 Jonathan Neuschäfer
> +
> +#include <linux/rtc.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>

Please sort the includes.

> +
> +struct ntxec_rtc {
> +	struct device *dev;
> +	struct ntxec *ec;
> +};
> +
> +#define NTXEC_WRITE_YEAR	0x10
> +#define NTXEC_WRITE_MONTH	0x11
> +#define NTXEC_WRITE_DAY		0x12
> +#define NTXEC_WRITE_HOUR	0x13
> +#define NTXEC_WRITE_MINUTE	0x14
> +#define NTXEC_WRITE_SECOND	0x15
> +
> +#define NTXEC_READ_YM		0x20
> +#define NTXEC_READ_DH		0x21
> +#define NTXEC_READ_MS		0x22
> +
> +
> +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> +	int res;
> +
> +	res = ntxec_read16(rtc->ec, NTXEC_READ_YM);
> +	if (res < 0)
> +		return res;
> +
> +	tm->tm_year = (res >> 8) + 100;
> +	tm->tm_mon = (res & 0xff) - 1;
> +
> +	res = ntxec_read16(rtc->ec, NTXEC_READ_DH);
> +	if (res < 0)
> +		return res;
> +
> +	tm->tm_mday = res >> 8;
> +	tm->tm_hour = res & 0xff;
> +
> +	res = ntxec_read16(rtc->ec, NTXEC_READ_MS);
> +	if (res < 0)
> +		return res;
> +
> +	tm->tm_min = res >> 8;
> +	tm->tm_sec = res & 0xff;
> +
> +	return 0;
> +}
> +
> +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> +	int res = 0;
> +
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_YEAR, tm->tm_year - 100);
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_MONTH, tm->tm_mon + 1);
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_DAY, tm->tm_mday);
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_HOUR, tm->tm_hour);
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_MINUTE, tm->tm_min);
> +	res |= ntxec_write8(rtc->ec, NTXEC_WRITE_SECOND, tm->tm_sec);
> +
> +	return (res < 0)? -EIO : 0;
> +}
> +
> +static const struct rtc_class_ops ntxec_rtc_ops = {
> +	.read_time		= ntxec_read_time,
> +	.set_time		= ntxec_set_time,
> +};
> +
> +static int ntxec_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rtc_device *rtcdev;
> +	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);
> +
> +	rtcdev = devm_rtc_device_register(&pdev->dev, "ntxec-rtc",
> +					  &ntxec_rtc_ops, THIS_MODULE);

Please use devm_rtc_allocate_device and rtc_register_device. Also, set
the supported range (->range_min and ->range_max).


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

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

* Re: [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC
  2020-06-20 22:42 ` [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC Jonathan Neuschäfer
@ 2020-06-21 18:41   ` Andreas Kemnade
  2020-08-23 22:42     ` Jonathan Neuschäfer
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Kemnade @ 2020-06-21 18:41 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Lee Jones, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer

On Sun, 21 Jun 2020 00:42:16 +0200
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:

> The Netronix embedded controller as found in Kobo Aura and Tolino Shine
> supports one PWM channel, which is used to control the frontlight
> brightness on these devices.
> 
> Known problems:
> - `make dt_binding_check` shows the following warnings:
>   Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
>   Warning (pwms_property): /example-0/backlight:pwms: cell 2 is not a
>   phandle reference
>   Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
>   Warning (pwms_property): /example-0/backlight:pwms: Could not get
>   phandle node for (cell 2)
> 
In the tolino sources in ./drivers/misc/ntx-misc.c I find this line

        if(4==gptHWCFG->m_val.bFL_PWM) {

No idea what it does but I would expect to have a kind of translation to
a dt property?

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../bindings/mfd/netronix,ntxec.yaml          | 13 ++++++++
>  .../bindings/pwm/netronix,ntxec-pwm.yaml      | 33 +++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> index 596df460f98eb..6562c41c5a9a9 100644
> --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> @@ -31,6 +31,9 @@ properties:
>      description:
>        The EC can signal interrupts via a GPIO line
> 
> +  pwm:
> +    $ref: ../pwm/netronix,ntxec-pwm.yaml
> +
>  required:
>    - compatible
>    - reg
> @@ -53,5 +56,15 @@ examples:
>                      interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
>                      interrupt-controller;
>                      #interrupt-cells = <1>;
> +
> +                    ec_pwm: pwm {
> +                            compatible = "netronix,ntxec-pwm";
> +                            #pwm-cells = <1>;
shouldn't that be 2?
> +                    };
>              };
>      };
> +
> +    backlight {
> +            compatible = "pwm-backlight";
> +            pwms = <&ec_pwm 0 50000>;
since you have 2 values after the &ec_pwm 

> +    };
> diff --git a/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> new file mode 100644
> index 0000000000000..1dc1b1aba081c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/netronix,ntxec-pwm.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/netronix,ntxec-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PWM functionality in Netronix embedded controller
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +description: |
> +  See also Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> +
> +  The Netronix EC contains PWM functionality, which is usually used to drive
> +  the backlight LED.
> +
> +  The following PWM channels are supported:
> +    - 0: The PWM channel controlled by registers 0xa1-0xa7
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: netronix,ntxec-pwm
> +
> +  "#pwm-cells":
> +    const: 1

shouln't that be 2?
> +
> +required:
> +  - compatible
> +  - "#pwm-cells"
> --
> 2.27.0
> 
> 

Regards,
Andreas

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

* Re: [RFC PATCH 06/10] pwm: ntxec: Add driver for PWM function in Netronix EC
  2020-06-20 22:42 ` [RFC PATCH 06/10] pwm: ntxec: Add driver " Jonathan Neuschäfer
@ 2020-06-22  8:18   ` Uwe Kleine-König
  2020-07-03 16:15     ` Jonathan Neuschäfer
  0 siblings, 1 reply; 23+ messages in thread
From: Uwe Kleine-König @ 2020-06-22  8:18 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Alexandre Belloni, Heiko Stuebner, linux-pwm,
	Linus Walleij, Thierry Reding, Fabio Estevam, linux-rtc,
	Mauro Carvalho Chehab, Sam Ravnborg, Andreas Kemnade,
	NXP Linux Team, devicetree, Stephan Gerhold, allen, Sascha Hauer,
	Lubomir Rintel, Rob Herring, Lee Jones, linux-arm-kernel,
	Alessandro Zummo, Mark Brown, Pengutronix Kernel Team,
	Heiko Stuebner, Josua Mayer, Shawn Guo, David S. Miller

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

Hello,

On Sun, Jun 21, 2020 at 12:42:17AM +0200, Jonathan Neuschäfer wrote:
> The Netronix EC provides a PWM output, which is used for the backlight

s/,//

> on ebook readers. This patches adds a driver for the PWM output.

on *some* ebook readers


> +#define NTXEC_UNK_A		0xa1
> +#define NTXEC_UNK_B		0xa2
> +#define NTXEC_ENABLE		0xa3
> +#define NTXEC_PERIOD_LOW	0xa4
> +#define NTXEC_PERIOD_HIGH	0xa5
> +#define NTXEC_DUTY_LOW		0xa6
> +#define NTXEC_DUTY_HIGH		0xa7
> +
> +/*
> + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> + * measured in this unit.
> + */
> +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> +				 int duty_ns, int period_ns)
> +{
> +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> +	uint64_t duty = duty_ns;
> +	uint64_t period = period_ns;

As you cannot use values bigger than 8191999 anyhow, I wonder why you
use a 64 bit type here.

> +	int res = 0;
> +
> +	do_div(period, 125);

Please use a define instead of plain 125.

> +	if (period > 0xffff) {
> +		dev_warn(pwm->dev,
> +			 "Period is not representable in 16 bits: %llu\n", period);
> +		return -ERANGE;
> +	}
> +
> +	do_div(duty, 125);
> +	if (duty > 0xffff) {
> +		dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n",
> +			duty);
> +		return -ERANGE;
> +	}

This check isn't necessary as the pwm core ensures that duty <= period.

> +	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
> +	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
> +	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
> +	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);

Does this complete the currently running period? Can it happen that a
new period starts between the first and the last write and so a mixed
period can be seen at the output?

> +
> +	return (res < 0) ? -EIO : 0;
> +}
> +
> +static int ntxec_pwm_enable(struct pwm_chip *chip,
> +				 struct pwm_device *pwm_dev)
> +{
> +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> +
> +	return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1);
> +}
> +
> +static void ntxec_pwm_disable(struct pwm_chip *chip,
> +				   struct pwm_device *pwm_dev)
> +{
> +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> +
> +	ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
> +}
> +
> +static struct pwm_ops ntxec_pwm_ops = {
> +	.config		= ntxec_pwm_config,
> +	.enable		= ntxec_pwm_enable,
> +	.disable	= ntxec_pwm_disable,
> +	.owner		= THIS_MODULE,

Please don't align the =, just a single space before them is fine.
More important: Please implement .apply() (and .get_state()) instead of
the old API. Also please enable PWM_DEBUG which might save us a review
iteration.

> +};
> +
> +static int ntxec_pwm_probe(struct platform_device *pdev)
> +{
> +	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct ntxec_pwm *pwm;
> +	struct pwm_chip *chip;
> +	int res;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->ec = ec;
> +	pwm->dev = &pdev->dev;
> +
> +	chip = &pwm->chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &ntxec_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	res = pwmchip_add(chip);
> +	if (res < 0)
> +		return res;
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
> +	res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff);
> +	res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff);
> +
> +	return (res < 0) ? -EIO : 0;

This is broken for several reasons:

 - You're not supposed to modify the output in .probe
 - if ntxec_write8 results in an error you keep the pwm registered.
 - From the moment on pwmchip_add returns the callbacks can be called.
   The calls to ntxec_write8 probably interfere here.

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 --]

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

* Re: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
  2020-06-20 22:42 ` [RFC PATCH 04/10] mfd: Add base driver for Netronix " Jonathan Neuschäfer
@ 2020-06-22 10:55   ` Lee Jones
  2020-06-28  8:11     ` Jonathan Neuschäfer
  2020-06-27  8:17   ` Andreas Kemnade
  1 sibling, 1 reply; 23+ messages in thread
From: Lee Jones @ 2020-06-22 10:55 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Alessandro Zummo, Alexandre Belloni, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer, Andreas Kemnade

On Sun, 21 Jun 2020, Jonathan Neuschäfer wrote:

Description of the device here please.

> Third-party hardware documentation is available at

'\n'

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

Indent this with a couple of spaces.

> The EC supports interrupts, but the driver doesn't make use of them so
> far.
> 
> Known problems:
> - The reboot handler is installed in such a way that it directly calls
>   into the i2c subsystem to send the reboot command to the EC. This
>   means that the reboot handler may sleep, which is not allowed.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/mfd/Kconfig       |   7 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  30 ++++++
>  4 files changed, 226 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 a37d7d1713820..78410b928648e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,13 @@ config MFD_VIPERBOARD
>  	  You need to select the mfd cell drivers separately.
>  	  The drivers do not support all features the board exposes.
> 
> +config MFD_NTXEC
> +	bool "Netronix Embedded Controller"
> +	depends on I2C && OF
> +	help
> +	  Say yes here if you want to support the embedded controller of
> +	  certain e-book readers designed by the ODM Netronix.

s/of certain/found in certain/.

>  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 9367a92f795a6..19d9391ed6f32 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..82adea34ea746
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2020 Jonathan Neuschäfer
> +//
> +// MFD driver for the usually MSP430-based embedded controller used in certain
> +// Netronix ebook reader board designs

Only the SPDX line should contain C++ style comments.

Description at the top, copyright after.

An "MFD driver" isn't really a thing.  Please describe the device.

> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +
> +

No need for double line spacing.

> +#define NTXEC_VERSION		0x00
> +#define NTXEC_POWEROFF		0x50
> +#define NTXEC_POWERKEEP		0x70
> +#define NTXEC_RESET		0x90

Are these registers?  Might be worth pre/post-fixing with _REG.

> +/* Register access */
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr)
> +{
> +	u8 request[1] = { addr };
> +	u8 response[2];
> +	int res;
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags,
> +			.len = sizeof(request),
> +			.buf = request
> +		}, {
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags | I2C_M_RD,
> +			.len = sizeof(response),
> +			.buf = response
> +		}
> +	};
> +
> +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		return res;
> +	if (res != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	return get_unaligned_be16(response);
> +}
> +EXPORT_SYMBOL(ntxec_read16);
> +
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> +{
> +	u8 request[3] = { addr, };
> +	int res;
> +
> +	put_unaligned_be16(value, request + 1);
> +
> +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> +					ec->client->flags);
> +	if (res < 0)
> +		return res;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ntxec_write16);
> +
> +int ntxec_read8(struct ntxec *ec, u8 addr)
> +{
> +	int res = ntxec_read16(ec, addr);
> +
> +	if (res < 0)
> +		return res;
> +
> +	return (res >> 8) & 0xff;
> +}
> +EXPORT_SYMBOL(ntxec_read8);
> +
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> +{
> +	return ntxec_write16(ec, addr, value << 8);
> +}
> +EXPORT_SYMBOL(ntxec_write8);

Why are you hand-rolling your own accessors?

> +
> +
> +/* Reboot/poweroff handling */

These comments seem to be stating the obvious.

Please remove them.

> +static struct ntxec *poweroff_restart_instance;
> +
> +static void ntxec_poweroff(void)
> +{
> +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);

All magic numbers should be defined?

> +	msleep(5000);

Why 5000?

> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> +		unsigned long action, void *data)
> +{
> +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */

Then this is not allowed.

You need to fix this *before* submitting upstream.

> +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> +	/* TODO: delay? */

TODO *before* submitting upstream.  This is not a development branch.

> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ntxec_restart_handler = {
> +	.notifier_call = ntxec_restart,
> +	.priority = 128
> +};
> +
> +
> +/* Driver setup */
> +
> +static int ntxec_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *ids)
> +{
> +	struct ntxec *ec;
> +	int res;

What does 'res' mean?

> +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = &client->dev;
> +	ec->client = client;

You don't need both (if any).

> +	/* Determine the firmware version */
> +	res = ntxec_read16(ec, NTXEC_VERSION);
> +	if (res < 0) {
> +		dev_dbg(ec->dev, "Failed to read firmware version number\n");

Why dbg?

> +		return res;
> +	}
> +	ec->version = res;
> +
> +	dev_info(ec->dev,
> +		 "Netronix embedded controller version %04x detected.\n",
> +		 ec->version);
> +
> +	/* For now, we don't support the new register layout. */
> +	if (ntxec_has_new_layout(ec))
> +		return -EOPNOTSUPP;

What is the new layout?

Why don't you support it?

> +	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 = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> +		if (res < 0)
> +			return res;
> +
> +		/* Install poweroff handler */
> +		WARN_ON(poweroff_restart_instance);

Why WARN?

> +		poweroff_restart_instance = ec;
> +		if (pm_power_off != NULL)

if (pm_power_off)

> +			/* TODO: Refactor among all poweroff drivers */

Nope.

> +			dev_err(ec->dev, "pm_power_off already assigned\n");

Error, but execution carries on anyway?

> +		else
> +			pm_power_off = ntxec_poweroff;
> +
> +		/* Install board reset handler */
> +		res = register_restart_handler(&ntxec_restart_handler);
> +		if (res < 0)

Is >0 valid?

> +			dev_err(ec->dev,
> +				"Failed to register restart handler: %d\n", res);
> +	}
> +
> +	i2c_set_clientdata(client, ec);

Where do you use this?

> +	return devm_of_platform_populate(ec->dev);
> +}
> +
> +static const struct of_device_id of_ntxec_match_table[] = {
> +	{ .compatible = "netronix,ntxec", },
> +	{}
> +};

Use .probe_new() and drop this.

> +static struct i2c_driver ntxec_driver = {
> +	.driver	= {
> +		.name	= "ntxec",
> +		.of_match_table = of_ntxec_match_table,
> +	},
> +	.probe		= ntxec_probe,

Nothing to .remove()?

> +};
> +builtin_i2c_driver(ntxec_driver);

Only built-in?

> diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> new file mode 100644
> index 0000000000000..9f9d6f2141751
> --- /dev/null
> +++ b/include/linux/mfd/ntxec.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Jonathan Neuschäfer
> + *
> + * MFD access functions for the Netronix embedded controller.

No such thing as an MFD.

"Embedded Controller"

> + */
> +
> +#ifndef NTXEC_H
> +#define NTXEC_H
> +
> +#include <linux/types.h>
> +
> +struct ntxec {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	u16 version;
> +	/* TODO: Add a mutex to protect actions consisting of multiple accesses? */

No TODOs.  Please fix before upstreaming.

> +};
> +
> +static inline bool ntxec_has_new_layout(struct ntxec *ec)
> +{
> +	return ec->version == 0xe916;

Why don't you just check for the devices you *do* support?

> +}
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr);
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value);
> +int ntxec_read8(struct ntxec *ec, u8 addr);
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value);
> +
> +#endif

#endif /* NTXEC_H */

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

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

* Re: [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC
  2020-06-21  0:02   ` Alexandre Belloni
@ 2020-06-26 21:55     ` Andreas Kemnade
  2020-07-03 17:04       ` Jonathan Neuschäfer
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Kemnade @ 2020-06-26 21:55 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Jonathan Neuschäfer, linux-kernel, Lee Jones, Rob Herring,
	Thierry Reding, Uwe Kleine-König, Alessandro Zummo,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer

On Sun, 21 Jun 2020 02:02:20 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Hi,
> 
> On 21/06/2020 00:42:18+0200, Jonathan Neuschäfer wrote:
> > The Netronix EC implements an RTC with the following functionality:
> > 
> > - Calendar-based time keeping with single-second resolution
> > - Automatic power-on with single-minute resolution
> > - Alarm at single-second resolution
> > 
> > This binding only supports timekeeping for now.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> >  .../bindings/mfd/netronix,ntxec.yaml          |  7 +++++
> >  .../bindings/rtc/netronix,ntxec-rtc.yaml      | 27 +++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/netronix,ntxec-rtc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > index 6562c41c5a9a9..f6a32f46f47bb 100644
> > --- a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
> > @@ -34,6 +34,9 @@ properties:
> >    pwm:
> >      $ref: ../pwm/netronix,ntxec-pwm.yaml
> > 
> > +  rtc:
> > +    $ref: ../rtc/netronix,ntxec-rtc.yaml
> > +  
> 
> Shouldn't the node simply be documented here?
> 
> Also, do you really need a compatible string to be able to proe the
> driver? What are the chances that you'll get a similar EC without an
> RTC?
> 
Tolino Shine 2 HD has the mentioned EC but the vendor kernel does not use
its RTC (not checked whether it is present or functional).
As a key for grepping in the vendor sources: 
gptNtxHwCfg->m_val.bPCB = 0x50

Tolino Shine 3  and Kobo Clara HD do not have that EC.

Regrads,
Andreas

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

* Re: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
  2020-06-20 22:42 ` [RFC PATCH 04/10] mfd: Add base driver for Netronix " Jonathan Neuschäfer
  2020-06-22 10:55   ` Lee Jones
@ 2020-06-27  8:17   ` Andreas Kemnade
  2020-06-28  8:29     ` Jonathan Neuschäfer
  1 sibling, 1 reply; 23+ messages in thread
From: Andreas Kemnade @ 2020-06-27  8:17 UTC (permalink / raw)
  To: Jonathan Neuschäfer
  Cc: linux-kernel, Lee Jones, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Alessandro Zummo, Alexandre Belloni,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer

On Sun, 21 Jun 2020 00:42:15 +0200
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:

> 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.
> 
> Known problems:
> - The reboot handler is installed in such a way that it directly calls
>   into the i2c subsystem to send the reboot command to the EC. This
>   means that the reboot handler may sleep, which is not allowed.
> 
see
https://patchwork.ozlabs.org/project/linux-i2c/patch/20190415213432.8972-3-contact@stefanchrist.eu/

for a fix of such problems. 

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  drivers/mfd/Kconfig       |   7 ++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/ntxec.c       | 188 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ntxec.h |  30 ++++++
>  4 files changed, 226 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 a37d7d1713820..78410b928648e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -978,6 +978,13 @@ config MFD_VIPERBOARD
>  	  You need to select the mfd cell drivers separately.
>  	  The drivers do not support all features the board exposes.
> 
> +config MFD_NTXEC
> +	bool "Netronix Embedded Controller"
> +	depends on I2C && OF
> +	help
> +	  Say yes here if you want to support the embedded controller of
> +	  certain e-book readers designed by the ODM Netronix.
> +
>  config MFD_RETU
>  	tristate "Nokia Retu and Tahvo multi-function device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 9367a92f795a6..19d9391ed6f32 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..82adea34ea746
> --- /dev/null
> +++ b/drivers/mfd/ntxec.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2020 Jonathan Neuschäfer
> +//
> +// MFD driver for the usually MSP430-based embedded controller used in certain
> +// Netronix ebook reader board designs
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/types.h>
> +
> +
> +#define NTXEC_VERSION		0x00
> +#define NTXEC_POWEROFF		0x50
> +#define NTXEC_POWERKEEP		0x70
> +#define NTXEC_RESET		0x90
> +
> +
> +/* Register access */
> +
> +int ntxec_read16(struct ntxec *ec, u8 addr)
> +{
> +	u8 request[1] = { addr };
> +	u8 response[2];
> +	int res;
> +
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags,
> +			.len = sizeof(request),
> +			.buf = request
> +		}, {
> +			.addr = ec->client->addr,
> +			.flags = ec->client->flags | I2C_M_RD,
> +			.len = sizeof(response),
> +			.buf = response
> +		}
> +	};
> +
> +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (res < 0)
> +		return res;
> +	if (res != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	return get_unaligned_be16(response);
> +}
> +EXPORT_SYMBOL(ntxec_read16);
> +
> +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> +{
> +	u8 request[3] = { addr, };
> +	int res;
> +
> +	put_unaligned_be16(value, request + 1);
> +
> +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> +					ec->client->flags);
> +	if (res < 0)
> +		return res;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ntxec_write16);
> +
> +int ntxec_read8(struct ntxec *ec, u8 addr)
> +{
> +	int res = ntxec_read16(ec, addr);
> +
> +	if (res < 0)
> +		return res;
> +
> +	return (res >> 8) & 0xff;
> +}
> +EXPORT_SYMBOL(ntxec_read8);
> +
> +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> +{
> +	return ntxec_write16(ec, addr, value << 8);
> +}
> +EXPORT_SYMBOL(ntxec_write8);
> +

do we really need both 16bit and 8bit accessors? If not,
then simply use regmap_i2c_init and set val_bits accordingly.
Maybe just doing the << 8 in the constants?

> +
> +/* Reboot/poweroff handling */
> +
> +static struct ntxec *poweroff_restart_instance;
> +
> +static void ntxec_poweroff(void)
> +{
> +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);
> +	msleep(5000);
> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> +		unsigned long action, void *data)
> +{
> +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */
> +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> +	/* TODO: delay? */
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ntxec_restart_handler = {
> +	.notifier_call = ntxec_restart,
> +	.priority = 128
> +};
> +
> +
> +/* Driver setup */
> +
> +static int ntxec_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *ids)
> +{
> +	struct ntxec *ec;
> +	int res;
> +
> +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = &client->dev;
> +	ec->client = client;
> +
> +	/* Determine the firmware version */
> +	res = ntxec_read16(ec, NTXEC_VERSION);
> +	if (res < 0) {
> +		dev_dbg(ec->dev, "Failed to read firmware version number\n");
> +		return res;
> +	}
> +	ec->version = res;
> +
> +	dev_info(ec->dev,
> +		 "Netronix embedded controller version %04x detected.\n",
> +		 ec->version);
> +
> +	/* For now, we don't support the new register layout. */
> +	if (ntxec_has_new_layout(ec))
> +		return -EOPNOTSUPP;
> +
> +	if (of_device_is_system_power_controller(ec->dev->of_node)) {
> +		/*
> +		 * Set the 'powerkeep' bit. This is necessary on some boards
> +		 * in order to keep the system running.
> +		 */
> +		res = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> +		if (res < 0)
> +			return res;
> +
> +		/* Install poweroff handler */
> +		WARN_ON(poweroff_restart_instance);
> +		poweroff_restart_instance = ec;
> +		if (pm_power_off != NULL)
> +			/* TODO: Refactor among all poweroff drivers */
> +			dev_err(ec->dev, "pm_power_off already assigned\n");
> +		else
> +			pm_power_off = ntxec_poweroff;
> +
common pattern, across drivers, so I think doing something else would
be a separate cleanup issue.

Regards,
Andreas

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

* Re: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
  2020-06-22 10:55   ` Lee Jones
@ 2020-06-28  8:11     ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-28  8:11 UTC (permalink / raw)
  To: Lee Jones
  Cc: Jonathan Neuschäfer, linux-kernel, Rob Herring,
	Thierry Reding, Uwe Kleine-König, Alessandro Zummo,
	Alexandre Belloni, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer, Andreas Kemnade

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

On Mon, Jun 22, 2020 at 11:55:44AM +0100, Lee Jones wrote:
> On Sun, 21 Jun 2020, Jonathan Neuschäfer wrote:
> 
> Description of the device here please.
> 
> > Third-party hardware documentation is available at
> 
> '\n'
> 
> > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> 
> Indent this with a couple of spaces.
> 
[...]
> > +	help
> > +	  Say yes here if you want to support the embedded controller of
> > +	  certain e-book readers designed by the ODM Netronix.
> 
> s/of certain/found in certain/.
[...]
> > +++ b/drivers/mfd/ntxec.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// Copyright 2020 Jonathan Neuschäfer
> > +//
> > +// MFD driver for the usually MSP430-based embedded controller used in certain
> > +// Netronix ebook reader board designs
> 
> Only the SPDX line should contain C++ style comments.
> 
> Description at the top, copyright after.
> 
> An "MFD driver" isn't really a thing.  Please describe the device.
[...]
> > +#include <linux/types.h>
> > +
> > +
> 
> No need for double line spacing.

Alright, I'll fix these.

> > +#define NTXEC_VERSION		0x00
> > +#define NTXEC_POWEROFF		0x50
> > +#define NTXEC_POWERKEEP		0x70
> > +#define NTXEC_RESET		0x90
> 
> Are these registers?  Might be worth pre/post-fixing with _REG.

Yes, good idea.

> 
> > +/* Register access */
> > +
> > +int ntxec_read16(struct ntxec *ec, u8 addr)
> > +{
> > +	u8 request[1] = { addr };
> > +	u8 response[2];
> > +	int res;
> > +
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr = ec->client->addr,
> > +			.flags = ec->client->flags,
> > +			.len = sizeof(request),
> > +			.buf = request
> > +		}, {
> > +			.addr = ec->client->addr,
> > +			.flags = ec->client->flags | I2C_M_RD,
> > +			.len = sizeof(response),
> > +			.buf = response
> > +		}
> > +	};
> > +
> > +	res = i2c_transfer(ec->client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (res < 0)
> > +		return res;
> > +	if (res != ARRAY_SIZE(msgs))
> > +		return -EIO;
> > +
> > +	return get_unaligned_be16(response);
> > +}
> > +EXPORT_SYMBOL(ntxec_read16);
> > +
> > +int ntxec_write16(struct ntxec *ec, u8 addr, u16 value)
> > +{
> > +	u8 request[3] = { addr, };
> > +	int res;
> > +
> > +	put_unaligned_be16(value, request + 1);
> > +
> > +	res = i2c_transfer_buffer_flags(ec->client, request, sizeof(request),
> > +					ec->client->flags);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ntxec_write16);
> > +
> > +int ntxec_read8(struct ntxec *ec, u8 addr)
> > +{
> > +	int res = ntxec_read16(ec, addr);
> > +
> > +	if (res < 0)
> > +		return res;
> > +
> > +	return (res >> 8) & 0xff;
> > +}
> > +EXPORT_SYMBOL(ntxec_read8);
> > +
> > +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> > +{
> > +	return ntxec_write16(ec, addr, value << 8);
> > +}
> > +EXPORT_SYMBOL(ntxec_write8);
> 
> Why are you hand-rolling your own accessors?

There are registers which only require one byte of data and others which
require two. This didn't quite seem to fit into the regmap API.

It is possible to always use 16-bit accessors directly but that would
push shifts into call sites that only use 8 bits.
(If the 16 bits are interpreted as big endian)

I'm not sure what's ideal here.


> > +/* Reboot/poweroff handling */
> 
> These comments seem to be stating the obvious.
> 
> Please remove them.

Ok.


> > +	ntxec_write8(poweroff_restart_instance, NTXEC_POWEROFF, 0x01);
> 
> All magic numbers should be defined?

Alright, I'll move them to the register definitions.

> > +	msleep(5000);
> 
> Why 5000?

The idea was to avoid returning from the poweroff handler by allowing
enough time until the power is actually off. However:

 - I just tested it and the board I have turns off about 80ms after the
   I2C command.

 - I'm not sure if returning from the poweroff handler is actually bad.
   It does seem to be wrong, because I get a lockdep report when I
   remove the msleep and the kernel reaches do_exit in the reboot
   syscall handler:

	Requesting system poweroff
	[   14.465312] cfg80211: failed to load regulatory.db
	[   14.471171] imx-sdma 63fb0000.sdma: external firmware not found, using ROM firmware
	[   14.541097] reboot: Power down
	[   14.547296]
	[   14.548840] ====================================
	[   14.553477] WARNING: init/156 still has locks held!
	[   14.558521] 5.8.0-rc1-00011-g65740c2d29bee-dirty #329 Not tainted
	[   14.564647] ------------------------------------
	[   14.569399] 1 lock held by init/156:
	[   14.573004]  #0: c17278a8 (system_transition_mutex){+.+.}-{3:3}, at: __do_sys_reboot+0x13c/0x1fc
	[   14.581978]
	[   14.581978] stack backtrace:
	[   14.586378] CPU: 0 PID: 156 Comm: init Not tainted 5.8.0-rc1-00011-g65740c2d29bee-dirty #329
	[   14.594834] Hardware name: Freescale i.MX50 (Device Tree Support)
	[   14.600979] [<c01121c8>] (unwind_backtrace) from [<c010cf3c>] (show_stack+0x10/0x14)
	[   14.608763] [<c010cf3c>] (show_stack) from [<c05e54ec>] (dump_stack+0xe4/0x118)
	[   14.616115] [<c05e54ec>] (dump_stack) from [<c0130d54>] (do_exit+0x6ec/0xc04)
	[   14.623291] [<c0130d54>] (do_exit) from [<c01582f0>] (__do_sys_reboot+0x148/0x1fc)
	[   14.630892] [<c01582f0>] (__do_sys_reboot) from [<c0100080>] (ret_fast_syscall+0x0/0x28)
	[   14.639000] Exception stack(0xc8c6dfa8 to 0xc8c6dff0)
	[   14.644071] dfa0:                   00000000 00000000 fee1dead 28121969 4321fedc 00000000
	[   14.652266] dfc0: 00000000 00000000 00000001 00000058 00000001 00000000 00000000 00000000
	[   14.660458] dfe0: 000a2290 bef21db4 0006db1f b6f02a0c


I'll adjust the number a bit and add a comment to explain the msleep
call.


> > +static int ntxec_restart(struct notifier_block *nb,
> > +		unsigned long action, void *data)
> > +{
> > +	/* FIXME: The I2C driver sleeps, but restart handlers may not sleep */
> 
> Then this is not allowed.
> 
> You need to fix this *before* submitting upstream.
> 
> > +	ntxec_write8(poweroff_restart_instance, NTXEC_RESET, 0xff);
> > +	/* TODO: delay? */
> 
> TODO *before* submitting upstream.  This is not a development branch.

Sorry. I'll research how other drivers deal with the issue of sleeping
I/O in the reset path.


> > +/* Driver setup */
> > +
> > +static int ntxec_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *ids)
> > +{
> > +	struct ntxec *ec;
> > +	int res;
> 
> What does 'res' mean?

Result

> > +	ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> > +	if (!ec)
> > +		return -ENOMEM;
> > +
> > +	ec->dev = &client->dev;
> > +	ec->client = client;
> 
> You don't need both (if any).

Ok, I'll drop ec->client.

> 
> > +	/* Determine the firmware version */
> > +	res = ntxec_read16(ec, NTXEC_VERSION);
> > +	if (res < 0) {
> > +		dev_dbg(ec->dev, "Failed to read firmware version number\n");
> 
> Why dbg?

I'm not sure anymore, but I agree that it seems more significant than
debug level.


> > +	/* For now, we don't support the new register layout. */
> > +	if (ntxec_has_new_layout(ec))
> > +		return -EOPNOTSUPP;
> 
> What is the new layout?

It's a significantly different command set, see 'if (NEWMSP) ...' in:

  https://github.com/neuschaefer/linux/blob/vendor/kobo-aura/drivers/mxc/pmic/core/pmic_core_i2c.c


> Why don't you support it?

I don't have any hardware that uses it. The downstream driver supports
both layouts so I wanted to ensure that this driver doesn't use the
wrong one.

> > +	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 = ntxec_write8(ec, NTXEC_POWERKEEP, 0x08);
> > +		if (res < 0)
> > +			return res;
> > +
> > +		/* Install poweroff handler */
> > +		WARN_ON(poweroff_restart_instance);
> 
> Why WARN?

Two instances of this driver where both try to be the poweroff/restart
instance seemed like a situation that is always unintended and a bug in
the devicetree (or somewhere else).

> 
> > +		poweroff_restart_instance = ec;
> > +		if (pm_power_off != NULL)
> 
> if (pm_power_off)

Ok

> 
> > +			/* TODO: Refactor among all poweroff drivers */
> 
> Nope.

Alright.

> 
> > +			dev_err(ec->dev, "pm_power_off already assigned\n");
> 
> Error, but execution carries on anyway?

Hmm, I guess if the poweroff handler can't be installed there also isn't
much point in installing a restart handler...

> > +		/* Install board reset handler */
> > +		res = register_restart_handler(&ntxec_restart_handler);
> > +		if (res < 0)
> 
> Is >0 valid?

"Currently always returns zero"

However, other callers tend to do "if (res)".

> > +			dev_err(ec->dev,
> > +				"Failed to register restart handler: %d\n", res);
> > +	}
> > +
> > +	i2c_set_clientdata(client, ec);
> 
> Where do you use this?

The sub-devices drivers (RTC, PWM) call `dev_get_drvdata(pdev->dev.parent)`
to get a pointer to the ntxec struct, which is then passed to the
register accessors.

> > +static const struct of_device_id of_ntxec_match_table[] = {
> > +	{ .compatible = "netronix,ntxec", },
> > +	{}
> > +};
> 
> Use .probe_new() and drop this.

Ok, I'll switch to .probe_new()

Should I really drop the OF match table, though?

> > +static struct i2c_driver ntxec_driver = {
> > +	.driver	= {
> > +		.name	= "ntxec",
> > +		.of_match_table = of_ntxec_match_table,
> > +	},
> > +	.probe		= ntxec_probe,
> 
> Nothing to .remove()?

Good point, I'll add a remove method.

> > +};
> > +builtin_i2c_driver(ntxec_driver);
> 
> Only built-in?

Currently, this is consistent with the Kconfig entry, but I'll look into
making the driver buildable as a module.

> > diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
> > new file mode 100644
> > index 0000000000000..9f9d6f2141751
> > --- /dev/null
> > +++ b/include/linux/mfd/ntxec.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright 2020 Jonathan Neuschäfer
> > + *
> > + * MFD access functions for the Netronix embedded controller.
> 
> No such thing as an MFD.
> 
> "Embedded Controller"

Ok, I'll rephrase the comment.

> > +struct ntxec {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	u16 version;
> > +	/* TODO: Add a mutex to protect actions consisting of multiple accesses? */
> 
> No TODOs.  Please fix before upstreaming.

Sorry, I'll clean them up.

> > +static inline bool ntxec_has_new_layout(struct ntxec *ec)
> > +{
> > +	return ec->version == 0xe916;
> 
> Why don't you just check for the devices you *do* support?

Good point. This would be the most conservative approach. If someone
then shows up with a different version, it can be added as tested and
known working.


Thanks for the review,
Jonathan Neuschäfer

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

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

* Re: [RFC PATCH 04/10] mfd: Add base driver for Netronix embedded controller
  2020-06-27  8:17   ` Andreas Kemnade
@ 2020-06-28  8:29     ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-06-28  8:29 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Jonathan Neuschäfer, linux-kernel, Lee Jones, Rob Herring,
	Thierry Reding, Uwe Kleine-König, Alessandro Zummo,
	Alexandre Belloni, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer

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

On Sat, Jun 27, 2020 at 10:17:38AM +0200, Andreas Kemnade wrote:
> On Sun, 21 Jun 2020 00:42:15 +0200
> Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 
> > 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.
> > 
> > Known problems:
> > - The reboot handler is installed in such a way that it directly calls
> >   into the i2c subsystem to send the reboot command to the EC. This
> >   means that the reboot handler may sleep, which is not allowed.
> > 
> see
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20190415213432.8972-3-contact@stefanchrist.eu/
> 
> for a fix of such problems. 

So far, regmap isn't involved here, but I'll remember it when I switch
to regmap.

Between when I first wrote this driver and now, the I2C has added
support for transfers in atomic contexts very late in the system's life
(exactly what happens when you reset a system via PMIC/EC), so this
problem seems to be gone from my driver, for now.
(See commit 63b96983a5ddf ("i2c: core: introduce callbacks for atomic transfers"))


[...]
> > +int ntxec_write8(struct ntxec *ec, u8 addr, u8 value)
> > +{
> > +	return ntxec_write16(ec, addr, value << 8);
> > +}
> > +EXPORT_SYMBOL(ntxec_write8);
> > +
> 
> do we really need both 16bit and 8bit accessors?

No, the hardware/firmware doesn't care.

> If not, then simply use regmap_i2c_init and set val_bits accordingly.
> Maybe just doing the << 8 in the constants?

Thanks, I'll try this approach.

The values are not always constants, for example in the PWM driver:

	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);


Jonathan

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

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

* Re: [RFC PATCH 06/10] pwm: ntxec: Add driver for PWM function in Netronix EC
  2020-06-22  8:18   ` Uwe Kleine-König
@ 2020-07-03 16:15     ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-07-03 16:15 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jonathan Neuschäfer, linux-kernel, Alexandre Belloni,
	Heiko Stuebner, linux-pwm, Linus Walleij, Thierry Reding,
	Fabio Estevam, linux-rtc, Mauro Carvalho Chehab, Sam Ravnborg,
	Andreas Kemnade, NXP Linux Team, devicetree, Stephan Gerhold,
	allen, Sascha Hauer, Lubomir Rintel, Rob Herring, Lee Jones,
	linux-arm-kernel, Alessandro Zummo, Mark Brown,
	Pengutronix Kernel Team, Heiko Stuebner, Josua Mayer, Shawn Guo,
	David S. Miller

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

On Mon, Jun 22, 2020 at 10:18:02AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Jun 21, 2020 at 12:42:17AM +0200, Jonathan Neuschäfer wrote:
> > The Netronix EC provides a PWM output, which is used for the backlight
> 
> s/,//
> 
> > on ebook readers. This patches adds a driver for the PWM output.
> 
> on *some* ebook readers

Ok, I'll fix these.

> 
> > +#define NTXEC_UNK_A		0xa1
> > +#define NTXEC_UNK_B		0xa2
> > +#define NTXEC_ENABLE		0xa3
> > +#define NTXEC_PERIOD_LOW	0xa4
> > +#define NTXEC_PERIOD_HIGH	0xa5
> > +#define NTXEC_DUTY_LOW		0xa6
> > +#define NTXEC_DUTY_HIGH		0xa7
> > +
> > +/*
> > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > + * measured in this unit.
> > + */
> > +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > +				 int duty_ns, int period_ns)
> > +{
> > +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> > +	uint64_t duty = duty_ns;
> > +	uint64_t period = period_ns;
> 
> As you cannot use values bigger than 8191999 anyhow, I wonder why you
> use a 64 bit type here.

No particular reason; I possibly got confused by the division API. I'll
use uint32_t instead.

> > +	int res = 0;
> > +
> > +	do_div(period, 125);
> 
> Please use a define instead of plain 125.

Will do.

> > +	if (period > 0xffff) {
> > +		dev_warn(pwm->dev,
> > +			 "Period is not representable in 16 bits: %llu\n", period);
> > +		return -ERANGE;
> > +	}
> > +
> > +	do_div(duty, 125);
> > +	if (duty > 0xffff) {
> > +		dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n",
> > +			duty);
> > +		return -ERANGE;
> > +	}
> 
> This check isn't necessary as the pwm core ensures that duty <= period.

Ok, I'll remove it.
> 
> > +	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8);
> > +	res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period);
> > +	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8);
> > +	res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty);
> 
> Does this complete the currently running period? Can it happen that a
> new period starts between the first and the last write and so a mixed
> period can be seen at the output?

Good question. I haven't measured it, and also don't have the code
running on the EC.

> 
> > +
> > +	return (res < 0) ? -EIO : 0;
> > +}
> > +
> > +static int ntxec_pwm_enable(struct pwm_chip *chip,
> > +				 struct pwm_device *pwm_dev)
> > +{
> > +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> > +
> > +	return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1);
> > +}
> > +
> > +static void ntxec_pwm_disable(struct pwm_chip *chip,
> > +				   struct pwm_device *pwm_dev)
> > +{
> > +	struct ntxec_pwm *pwm = pwmchip_to_pwm(chip);
> > +
> > +	ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
> > +}
> > +
> > +static struct pwm_ops ntxec_pwm_ops = {
> > +	.config		= ntxec_pwm_config,
> > +	.enable		= ntxec_pwm_enable,
> > +	.disable	= ntxec_pwm_disable,
> > +	.owner		= THIS_MODULE,
> 
> Please don't align the =, just a single space before them is fine.

Ok

> More important: Please implement .apply() (and .get_state()) instead of
> the old API. Also please enable PWM_DEBUG which might save us a review
> iteration.

Will do!

> 
> > +};
> > +
> > +static int ntxec_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
> > +	struct ntxec_pwm *pwm;
> > +	struct pwm_chip *chip;
> > +	int res;
> > +
> > +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> > +	if (!pwm)
> > +		return -ENOMEM;
> > +
> > +	pwm->ec = ec;
> > +	pwm->dev = &pdev->dev;
> > +
> > +	chip = &pwm->chip;
> > +	chip->dev = &pdev->dev;
> > +	chip->ops = &ntxec_pwm_ops;
> > +	chip->base = -1;
> > +	chip->npwm = 1;
> > +
> > +	res = pwmchip_add(chip);
> > +	if (res < 0)
> > +		return res;
> > +
> > +	platform_set_drvdata(pdev, pwm);
> > +
> > +	res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0);
> > +	res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff);
> > +	res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff);
> > +
> > +	return (res < 0) ? -EIO : 0;
> 
> This is broken for several reasons:
> 
>  - You're not supposed to modify the output in .probe
>  - if ntxec_write8 results in an error you keep the pwm registered.
>  - From the moment on pwmchip_add returns the callbacks can be called.
>    The calls to ntxec_write8 probably interfere here.

Ok, I'll rework the probe function to avoid these issues.


Thanks for the review,
Jonathan Neuschäfer

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

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

* Re: [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC
  2020-06-26 21:55     ` Andreas Kemnade
@ 2020-07-03 17:04       ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-07-03 17:04 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Alexandre Belloni, Jonathan Neuschäfer, linux-kernel,
	Lee Jones, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Alessandro Zummo, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer

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

On Fri, Jun 26, 2020 at 11:55:52PM +0200, Andreas Kemnade wrote:
> On Sun, 21 Jun 2020 02:02:20 +0200
> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
[...]
> > Also, do you really need a compatible string to be able to proe the
> > driver? What are the chances that you'll get a similar EC without an
> > RTC?
> > 
> Tolino Shine 2 HD has the mentioned EC but the vendor kernel does not use
> its RTC (not checked whether it is present or functional).
> As a key for grepping in the vendor sources: 
> gptNtxHwCfg->m_val.bPCB = 0x50

Thanks for checking.

(In the MMC dump from my Shine 2 HD, it's decimal 50 rather than hex)

> Tolino Shine 3  and Kobo Clara HD do not have that EC.

Fortunately, this set of drivers isn't needed at all in that case.


Thanks,
Jonathan

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

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

* Re: [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller
  2020-06-21  0:11   ` Alexandre Belloni
@ 2020-07-04 19:23     ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-07-04 19:23 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Jonathan Neuschäfer, linux-kernel, Lee Jones, Rob Herring,
	Thierry Reding, Uwe Kleine-König, Alessandro Zummo,
	Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Sam Ravnborg, Linus Walleij, Heiko Stuebner,
	Stephan Gerhold, Lubomir Rintel, Mark Brown, allen,
	Mauro Carvalho Chehab, David S. Miller, devicetree, linux-pwm,
	linux-rtc, linux-arm-kernel, Heiko Stuebner, Josua Mayer,
	Andreas Kemnade

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

Hi,

On Sun, Jun 21, 2020 at 02:11:06AM +0200, Alexandre Belloni wrote:
> On 21/06/2020 00:42:19+0200, 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.
> > 
> 
> Please report the results of rtctest (from the kernel tree) [...]

  # ./rtctest
  [==========] Running 7 tests from 2 test cases.
  [ RUN      ] rtc.date_read
  ../../tools/testing/selftests/rtc/rtctest.c:49:date_read:Current RTC date/time is 11/04/2006 23:11:23.
  [       OK ] rtc.date_read
  [ RUN      ] rtc.uie_read
  [  180.651355] random: crng init done
  uie_read: Test terminated by timeout
  [     FAIL ] rtc.uie_read
  [ RUN      ] rtc.uie_select
  ../../tools/testing/selftests/rtc/rtctest.c:98:uie_select:Expected 0 (0) != rc (0)
  uie_select: Test terminated by assertion
  [     FAIL ] rtc.uie_select
  [ RUN      ] rtc.alarm_alm_set
  ../../tools/testing/selftests/rtc/rtctest.c:129:alarm_alm_set:skip alarms are not supported.
  [       OK ] rtc.alarm_alm_set
  [ RUN      ] rtc.alarm_wkalm_set
  ../../tools/testing/selftests/rtc/rtctest.c:185:alarm_wkalm_set:skip alarms are not supported.
  [       OK ] rtc.alarm_wkalm_set
  [ RUN      ] rtc.alarm_alm_set_minute
  ../../tools/testing/selftests/rtc/rtctest.c:231:alarm_alm_set_minute:skip alarms are not supported.
  [       OK ] rtc.alarm_alm_set_minute
  [ RUN      ] rtc.alarm_wkalm_set_minute
  ../../tools/testing/selftests/rtc/rtctest.c:287:alarm_wkalm_set_minute:skip alarms are not supported.
  [       OK ] rtc.alarm_wkalm_set_minute
  [==========] 5 / 7 tests passed.
  [  FAILED  ]

> [...] and rtc-range
> (https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c)

  # ./rtc-range
  
  Testing 1970-01-01 00:00:00.
  KO  Read back 2226-01-01 00:01:00.
  
  Testing 2000-02-28 23:59:59.
  KO  Read back 2000-02-28 23:28:23.
  
  Testing 2020-02-28 23:59:59.
  KO  Read back 2020-02-28 23:28:23.
  
  Testing 2038-01-19 03:14:07.
  KO  Read back 2038-01-19 03:19:03.
  
  Testing 2069-12-31 23:59:59.
  KO  Read back 2069-12-31 23:31:23.
  
  Testing 2079-12-31 23:59:59.
  KO  Read back 2079-12-31 23:31:23.
  
  Testing 2099-12-31 23:59:59.
  KO  Read back 2099-12-31 23:31:23.
  
  Testing 2255-12-31 23:59:59.
  KO  Read back 2255-12-31 23:31:23.
  
  Testing 2100-02-28 23:59:59.
  KO  Read back 2100-02-28 23:28:23.
  
  Testing 2106-02-07 06:28:15.
  KO  Read back 2106-02-07 06:07:06.
  
  Testing 2262-04-11 23:47:16.
  KO  Read back 2006-04-11 23:11:23.


Something is very wrong here.

I'll try to fix the failures in rtctest and the problems in rtc-range
before version 2 of the patchset.

(The 2255 date was my addition, because I suspect this to be the upper
limit of the RTC's range.)


[...]
> > +config RTC_DRV_NTXEC
> > +	tristate "Netronix embedded controller RTC driver"
> > +	depends on MFD_NTXEC
> > +
> 
> This should get an help section.

Ok, I'll add one.


[...]
> > +#include <linux/rtc.h>
> > +#include <linux/mfd/ntxec.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> 
> Please sort the includes.

Will do.


[...]
> > +	rtcdev = devm_rtc_device_register(&pdev->dev, "ntxec-rtc",
> > +					  &ntxec_rtc_ops, THIS_MODULE);
> 
> Please use devm_rtc_allocate_device and rtc_register_device. Also, set
> the supported range (->range_min and ->range_max).

Ok, will do.


Thanks for the review and the testing tips.
Jonathan Neuschäfer

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

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

* Re: [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC
  2020-06-21 18:41   ` Andreas Kemnade
@ 2020-08-23 22:42     ` Jonathan Neuschäfer
  0 siblings, 0 replies; 23+ messages in thread
From: Jonathan Neuschäfer @ 2020-08-23 22:42 UTC (permalink / raw)
  To: Andreas Kemnade
  Cc: Jonathan Neuschäfer, linux-kernel, Lee Jones, Rob Herring,
	Thierry Reding, Uwe Kleine-König, Alessandro Zummo,
	Alexandre Belloni, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Sam Ravnborg, Linus Walleij, Heiko Stuebner, Stephan Gerhold,
	Lubomir Rintel, Mark Brown, allen, Mauro Carvalho Chehab,
	David S. Miller, devicetree, linux-pwm, linux-rtc,
	linux-arm-kernel, Heiko Stuebner, Josua Mayer

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

On Sun, Jun 21, 2020 at 08:41:23PM +0200, Andreas Kemnade wrote:
> On Sun, 21 Jun 2020 00:42:16 +0200
> Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 
> > The Netronix embedded controller as found in Kobo Aura and Tolino Shine
> > supports one PWM channel, which is used to control the frontlight
> > brightness on these devices.
> > 
> > Known problems:
> > - `make dt_binding_check` shows the following warnings:
> >   Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
> >   Warning (pwms_property): /example-0/backlight:pwms: cell 2 is not a
> >   phandle reference
> >   Documentation/devicetree/bindings/mfd/netronix,ntxec.example.dts:49.17-42:
> >   Warning (pwms_property): /example-0/backlight:pwms: Could not get
> >   phandle node for (cell 2)
> > 
> In the tolino sources in ./drivers/misc/ntx-misc.c I find this line
> 
>         if(4==gptHWCFG->m_val.bFL_PWM) {
> 
> No idea what it does but I would expect to have a kind of translation to
> a dt property?

As far as I understand it, FL_PWM=4 means that there is a second PWM
channel, in order to provide different backlight colors.

I think it should be possible to simply extend the binding to list
another available PWM channel, once we add support for such hardware.

> > +                    ec_pwm: pwm {
> > +                            compatible = "netronix,ntxec-pwm";
> > +                            #pwm-cells = <1>;
> shouldn't that be 2?
> > +                    };
> >              };
> >      };
> > +
> > +    backlight {
> > +            compatible = "pwm-backlight";
> > +            pwms = <&ec_pwm 0 50000>;
> since you have 2 values after the &ec_pwm 
[...]
> > +properties:
> > +  compatible:
> > +    const: netronix,ntxec-pwm
> > +
> > +  "#pwm-cells":
> > +    const: 1
> 
> shouln't that be 2?

Right, I'll fix that.


Thanks,
Jonathan

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

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

end of thread, other threads:[~2020-08-23 22:43 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-20 22:42 [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 03/10] dt-bindings: mfd: Add binding for Netronix's embedded controller Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 04/10] mfd: Add base driver for Netronix " Jonathan Neuschäfer
2020-06-22 10:55   ` Lee Jones
2020-06-28  8:11     ` Jonathan Neuschäfer
2020-06-27  8:17   ` Andreas Kemnade
2020-06-28  8:29     ` Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 05/10] dt-bindings: pwm: Add bindings for PWM function in Netronix EC Jonathan Neuschäfer
2020-06-21 18:41   ` Andreas Kemnade
2020-08-23 22:42     ` Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 06/10] pwm: ntxec: Add driver " Jonathan Neuschäfer
2020-06-22  8:18   ` Uwe Kleine-König
2020-07-03 16:15     ` Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 07/10] dt-bindings: rtc: Add bindings for Netronix embedded controller RTC Jonathan Neuschäfer
2020-06-21  0:02   ` Alexandre Belloni
2020-06-26 21:55     ` Andreas Kemnade
2020-07-03 17:04       ` Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 08/10] rtc: New driver for RTC in Netronix embedded controller Jonathan Neuschäfer
2020-06-21  0:11   ` Alexandre Belloni
2020-07-04 19:23     ` Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 09/10] MAINTAINERS: Add entry for " Jonathan Neuschäfer
2020-06-20 22:42 ` [RFC PATCH 10/10] ARM: dts: imx50-kobo-aura: Add " Jonathan Neuschäfer
2020-06-20 22:46 ` [RFC PATCH 02/10] dt-bindings: Add vendor prefix for Netronix, Inc Jonathan Neuschäfer

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