This patchset adds basic support for the embedded controller found on
older ebook reader boards designed by/with the ODM Netronix Inc.[1] and
sold by Kobo or Tolino, for example the Kobo Aura and the Tolino Shine.
These drivers are based on information contained in the vendor kernel
sources, but in order to all information in a single place, I documented
the register interface of the EC on GitHub[2].
[1]: http://www.netronixinc.com/products.aspx?ID=1
[2]: https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
v3:
- A few code cleanups
- A few devicetree related cleanups
- PWM and RTC functionality were moved from subnodes in the devicetree to
the main node. This also means that the subdrivers no longer need DT
compatible strings, but are instead loaded via the mfd_cell mechanism.
- The drivers are now published under GPLv2-or-later rather than GPLv2-only.
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Moved txt DT bindings to patch descriptions and removed patch 1/10
"DT bindings in plain text format"
- New patch 7/10 "rtc: Introduce RTC_TIMESTAMP_END_2255"
- Rebased on 5.9-rc3
- Various other changes which are documented in each patch
v1:
- https://lore.kernel.org/lkml/[email protected]/
Jonathan Neuschäfer (7):
dt-bindings: Add vendor prefix for Netronix, Inc.
dt-bindings: mfd: Add binding for Netronix embedded controller
mfd: Add base driver for Netronix embedded controller
pwm: ntxec: Add driver for PWM function in Netronix EC
rtc: New driver for RTC in Netronix embedded controller
MAINTAINERS: Add entry for Netronix embedded controller
ARM: dts: imx50-kobo-aura: Add Netronix embedded controller
.../bindings/mfd/netronix,ntxec.yaml | 76 +++++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 9 +
arch/arm/boot/dts/imx50-kobo-aura.dts | 17 +-
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/ntxec.c | 206 ++++++++++++++++++
drivers/pwm/Kconfig | 8 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-ntxec.c | 161 ++++++++++++++
drivers/rtc/Kconfig | 8 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-ntxec.c | 132 +++++++++++
include/linux/mfd/ntxec.h | 31 +++
14 files changed, 662 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
create mode 100644 drivers/mfd/ntxec.c
create mode 100644 drivers/pwm/pwm-ntxec.c
create mode 100644 drivers/rtc/rtc-ntxec.c
create mode 100644 include/linux/mfd/ntxec.h
--
2.28.0
The Netronix embedded controller is a microcontroller found in some
e-book readers designed by the ODM Netronix, Inc. It contains RTC,
battery monitoring, system power management, and PWM functionality.
This driver implements register access and version detection.
Third-party hardware documentation is available at:
https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
The EC supports interrupts, but the driver doesn't make use of them so
far.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v3:
- Add (EC) to CONFIG_MFD_NTXEC prompt
- Relicense as GPLv2 or later
- Add email address to copyright line
- remove empty lines in ntxec_poweroff and ntxec_restart functions
- Split long lines
- Remove 'Install ... handler' comments
- Make naming of struct i2c_client parameter consistent
- Remove struct ntxec_info
- Rework 'depends on' lines in Kconfig, hard-depend on I2C, select REGMAP_I2C and
MFD_CORE
- Register subdevices via mfd_cells
- Move 8-bit register conversion to ntxec.h
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Add a description of the device to the patch text
- Unify spelling as 'Netronix embedded controller'.
'Netronix' is the proper name of the manufacturer, but 'embedded controller'
is just a label that I have assigned to the device.
- Switch to regmap, avoid regmap use in poweroff and reboot handlers.
Inspired by cf84dc0bb40f4 ("mfd: rn5t618: Make restart handler atomic safe")
- Use a list of known-working firmware versions instead of checking for a
known-incompatible version
- Prefix registers with NTXEC_REG_
- Define register values as constants
- Various style cleanups as suggested by Lee Jones
- Don't align = signs in struct initializers [Uwe Kleine-König]
- Don't use dev_dbg for an error message
- Explain sleep in poweroff handler
- Remove (struct ntxec).client
- Switch to .probe_new in i2c driver
- Add .remove callback
- Make CONFIG_MFD_NTXEC a tristate option
---
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/ntxec.c | 206 ++++++++++++++++++++++++++++++++++++++
include/linux/mfd/ntxec.h | 31 ++++++
4 files changed, 248 insertions(+)
create mode 100644 drivers/mfd/ntxec.c
create mode 100644 include/linux/mfd/ntxec.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df0837ab415..b313103151508 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -978,6 +978,16 @@ config MFD_VIPERBOARD
You need to select the mfd cell drivers separately.
The drivers do not support all features the board exposes.
+config MFD_NTXEC
+ tristate "Netronix embedded controller (EC)"
+ depends on OF || COMPILE_TEST
+ depends on I2C
+ select REGMAP_I2C
+ select MFD_CORE
+ help
+ Say yes here if you want to support the embedded controller found in
+ certain e-book readers designed by the ODM Netronix.
+
config MFD_RETU
tristate "Nokia Retu and Tahvo multi-function device"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f835283e..236a8acd917a0 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -217,6 +217,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
obj-$(CONFIG_MFD_INTEL_PMC_BXT) += intel_pmc_bxt.o
obj-$(CONFIG_MFD_PALMAS) += palmas.o
obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
+obj-$(CONFIG_MFD_NTXEC) += ntxec.o
obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
obj-$(CONFIG_MFD_RK808) += rk808.o
obj-$(CONFIG_MFD_RN5T618) += rn5t618.o
diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c
new file mode 100644
index 0000000000000..93611b85a32e0
--- /dev/null
+++ b/drivers/mfd/ntxec.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements register access, version detection, and system
+ * power-off/reset.
+ *
+ * Copyright 2020 Jonathan Neuschäfer <[email protected]>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#define NTXEC_REG_VERSION 0x00
+#define NTXEC_REG_POWEROFF 0x50
+#define NTXEC_REG_POWERKEEP 0x70
+#define NTXEC_REG_RESET 0x90
+
+#define NTXEC_POWEROFF_VALUE 0x0100
+#define NTXEC_POWERKEEP_VALUE 0x0800
+#define NTXEC_RESET_VALUE 0xff00
+
+static struct i2c_client *poweroff_restart_client;
+
+static void ntxec_poweroff(void)
+{
+ int res;
+ u8 buf[] = {
+ NTXEC_REG_POWEROFF,
+ (NTXEC_POWEROFF_VALUE >> 8) & 0xff,
+ NTXEC_POWEROFF_VALUE & 0xff,
+ };
+ struct i2c_msg msgs[] = {
+ {
+ .addr = poweroff_restart_client->addr,
+ .flags = 0,
+ .len = sizeof(buf),
+ .buf = buf
+ }
+ };
+
+ res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (res < 0)
+ dev_alert(&poweroff_restart_client->dev,
+ "Failed to power off (err = %d)\n", res);
+
+ /*
+ * The time from the register write until the host CPU is powered off
+ * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
+ * safely avoid returning from the poweroff handler.
+ */
+ msleep(5000);
+}
+
+static int ntxec_restart(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ int res;
+ /*
+ * NOTE: The lower half of the reset value is not sent, because sending
+ * it causes an error
+ */
+ u8 buf[] = {
+ NTXEC_REG_RESET,
+ (NTXEC_RESET_VALUE >> 8) & 0xff,
+ };
+ struct i2c_msg msgs[] = {
+ {
+ .addr = poweroff_restart_client->addr,
+ .flags = 0,
+ .len = sizeof(buf),
+ .buf = buf
+ }
+ };
+
+ res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (res < 0)
+ dev_alert(&poweroff_restart_client->dev,
+ "Failed to restart (err = %d)\n", res);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block ntxec_restart_handler = {
+ .notifier_call = ntxec_restart,
+ .priority = 128
+};
+
+static const struct regmap_config regmap_config = {
+ .name = "ntxec",
+ .reg_bits = 8,
+ .val_bits = 16,
+ .cache_type = REGCACHE_NONE,
+ .val_format_endian = REGMAP_ENDIAN_BIG,
+};
+
+static const struct mfd_cell ntxec_subdevices[] = {
+ { .name = "ntxec-rtc" },
+ { .name = "ntxec-pwm" },
+};
+
+static int ntxec_probe(struct i2c_client *client)
+{
+ struct ntxec *ec;
+ unsigned int version;
+ int res;
+
+ ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
+ if (!ec)
+ return -ENOMEM;
+
+ ec->dev = &client->dev;
+
+ ec->regmap = devm_regmap_init_i2c(client, ®map_config);
+ if (IS_ERR(ec->regmap)) {
+ dev_err(ec->dev, "Failed to set up regmap for device\n");
+ return res;
+ }
+
+ /* Determine the firmware version */
+ res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
+ if (res < 0) {
+ dev_err(ec->dev, "Failed to read firmware version number\n");
+ return res;
+ }
+ dev_info(ec->dev,
+ "Netronix embedded controller version %04x detected.\n",
+ version);
+
+ /* Bail out if we encounter an unknown firmware version */
+ switch (version) {
+ case 0xd726: /* found in Kobo Aura */
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ if (of_device_is_system_power_controller(ec->dev->of_node)) {
+ /*
+ * Set the 'powerkeep' bit. This is necessary on some boards
+ * in order to keep the system running.
+ */
+ res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
+ NTXEC_POWERKEEP_VALUE);
+ if (res < 0)
+ return res;
+
+ WARN_ON(poweroff_restart_client);
+ poweroff_restart_client = client;
+ if (pm_power_off)
+ dev_err(ec->dev, "pm_power_off already assigned\n");
+ else
+ pm_power_off = ntxec_poweroff;
+
+ res = register_restart_handler(&ntxec_restart_handler);
+ if (res)
+ dev_err(ec->dev,
+ "Failed to register restart handler: %d\n", res);
+ }
+
+ i2c_set_clientdata(client, ec);
+
+ res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
+ ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
+ if (res)
+ dev_warn(ec->dev, "Failed to add subdevices: %d\n", res);
+
+ return res;
+}
+
+static int ntxec_remove(struct i2c_client *client)
+{
+ if (client == poweroff_restart_client) {
+ poweroff_restart_client = NULL;
+ pm_power_off = NULL;
+ unregister_restart_handler(&ntxec_restart_handler);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id of_ntxec_match_table[] = {
+ { .compatible = "netronix,ntxec", },
+ {}
+};
+
+static struct i2c_driver ntxec_driver = {
+ .driver = {
+ .name = "ntxec",
+ .of_match_table = of_ntxec_match_table,
+ },
+ .probe_new = ntxec_probe,
+ .remove = ntxec_remove,
+};
+module_i2c_driver(ntxec_driver);
diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h
new file mode 100644
index 0000000000000..a39c85978f61b
--- /dev/null
+++ b/include/linux/mfd/ntxec.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Jonathan Neuschäfer
+ *
+ * Register access and version information for the Netronix embedded
+ * controller.
+ */
+
+#ifndef NTXEC_H
+#define NTXEC_H
+
+#include <linux/types.h>
+
+struct ntxec {
+ struct device *dev;
+ struct regmap *regmap;
+};
+
+/*
+ * Some registers, such as the battery status register (0x41), are in
+ * big-endian, but others only have eight significant bits, which are in the
+ * first byte transmitted over I2C (the MSB of the big-endian value).
+ * This convenience function converts an 8-bit value to 16-bit for use in the
+ * second kind of register.
+ */
+static inline u16 ntxec_reg8(u8 value)
+{
+ return value << 8;
+}
+
+#endif
--
2.28.0
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 <[email protected]>
---
v3:
- Remove binding in text form patch description again
- Add additionalProperties: false
- Remove interrupt-controller property from example
- Merge pwm/rtc nodes into main node
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Add the plaintext DT binding for comparison
---
.../bindings/mfd/netronix,ntxec.yaml | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
diff --git a/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
new file mode 100644
index 0000000000000..59a630025f52f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/netronix,ntxec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Netronix Embedded Controller
+
+maintainers:
+ - Jonathan Neuschäfer <[email protected]>
+
+description: |
+ This EC is found in e-book readers of multiple brands (e.g. Kobo, Tolino), and
+ is typically implemented as a TI MSP430 microcontroller.
+
+properties:
+ compatible:
+ const: netronix,ntxec
+
+ reg:
+ items:
+ - description: The I2C address of the EC
+
+ system-power-controller:
+ type: boolean
+ description: See Documentation/devicetree/bindings/power/power-controller.txt
+
+ interrupts:
+ minItems: 1
+ description:
+ The EC can signal interrupts via a GPIO line
+
+ "#pwm-cells":
+ const: 2
+ description: |
+ Number of cells in a PWM specifier.
+
+ The following PWM channels are supported:
+ - 0: The PWM channel controlled by registers 0xa1-0xa7
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ec: embedded-controller@43 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ntxec>;
+
+ compatible = "netronix,ntxec";
+ reg = <0x43>;
+ system-power-controller;
+ interrupt-parent = <&gpio4>;
+ interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
+ #pwm-cells = <2>;
+ };
+ };
+
+ backlight {
+ compatible = "pwm-backlight";
+ pwms = <&ec 0 50000>;
+ power-supply = <&backlight_regulator>;
+ };
+
+ backlight_regulator: regulator-dummy {
+ compatible = "regulator-fixed";
+ regulator-name = "backlight";
+ };
--
2.28.0
The Netronix EC provides a PWM output which is used for the backlight
on some ebook readers. This patches adds a driver for the PWM output.
The .get_state callback is not implemented, because the PWM state can't
be read back from the hardware.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v3:
- Relicense as GPLv2 or later
- Add email address to copyright line
- Remove OF compatible string and don't include linux/of_device.h
- Fix bogus ?: in return line
- Don't use a comma after sentinels
- Avoid ret |= ... pattern
- Move 8-bit register conversion to ntxec.h
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Various grammar and style improvements, as suggested by Uwe Kleine-König,
Lee Jones, and Alexandre Belloni
- Switch to regmap
- Prefix registers with NTXEC_REG_
- Add help text to the Kconfig option
- Use the .apply callback instead of the old API
- Add a #define for the time base (125ns)
- Don't change device state in .probe; this avoids multiple problems
- Rework division and overflow check logic to perform divisions in 32 bits
- Avoid setting duty cycle to zero, to work around a hardware quirk
---
drivers/pwm/Kconfig | 8 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-ntxec.c | 161 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 170 insertions(+)
create mode 100644 drivers/pwm/pwm-ntxec.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d335..530dfda38d65e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -350,6 +350,14 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_NTXEC
+ tristate "Netronix embedded controller PWM support"
+ depends on MFD_NTXEC
+ help
+ Say yes here if you want to support the PWM output of the embedded
+ controller found in certain e-book readers designed by the ODM
+ Netronix.
+
config PWM_OMAP_DMTIMER
tristate "OMAP Dual-Mode Timer PWM support"
depends on OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a035577..1cc50dba22d1b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON) += pwm-meson.o
obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o
obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
new file mode 100644
index 0000000000000..50da2dc14bb03
--- /dev/null
+++ b/drivers/pwm/pwm-ntxec.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements PWM output.
+ *
+ * Copyright 2020 Jonathan Neuschäfer <[email protected]>
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct ntxec_pwm {
+ struct device *dev;
+ struct ntxec *ec;
+ struct pwm_chip chip;
+};
+
+static struct ntxec_pwm *pwmchip_to_pwm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct ntxec_pwm, chip);
+}
+
+#define NTXEC_REG_AUTO_OFF_HI 0xa1
+#define NTXEC_REG_AUTO_OFF_LO 0xa2
+#define NTXEC_REG_ENABLE 0xa3
+#define NTXEC_REG_PERIOD_LOW 0xa4
+#define NTXEC_REG_PERIOD_HIGH 0xa5
+#define NTXEC_REG_DUTY_LOW 0xa6
+#define NTXEC_REG_DUTY_HIGH 0xa7
+
+/*
+ * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
+ * measured in this unit.
+ */
+#define TIME_BASE_NS 125
+
+/*
+ * The maximum input value (in nanoseconds) is determined by the time base and
+ * the range of the hardware registers that hold the converted value.
+ * It fits into 32 bits, so we can do our calculations in 32 bits as well.
+ */
+#define MAX_PERIOD_NS (TIME_BASE_NS * 0x10000 - 1)
+
+static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
+ const struct pwm_state *state)
+{
+ struct ntxec_pwm *pwm = pwmchip_to_pwm(pwm_dev->chip);
+ unsigned int duty = state->duty_cycle;
+ unsigned int period = state->period;
+ int res = 0;
+
+ if (period > MAX_PERIOD_NS) {
+ dev_warn(pwm->dev,
+ "Period is not representable in 16 bits after division by %u: %u\n",
+ TIME_BASE_NS, period);
+ return -ERANGE;
+ }
+
+ period /= TIME_BASE_NS;
+ duty /= TIME_BASE_NS;
+
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
+ if (res)
+ return res;
+
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
+ if (res)
+ return res;
+
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
+ if (res)
+ return res;
+
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
+ if (res)
+ return res;
+
+ /*
+ * Writing a duty cycle of zone puts the device into a state where
+ * writing a higher duty cycle doesn't result in the brightness that it
+ * usually results in. This can be fixed by cycling the ENABLE register.
+ *
+ * As a workaround, write ENABLE=0 when the duty cycle is zero.
+ */
+ if (state->enabled && duty != 0) {
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
+ if (res)
+ return res;
+
+ /* Disable the auto-off timer */
+ res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
+ if (res)
+ return res;
+
+ return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
+ } else {
+ return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
+ }
+}
+
+static struct pwm_ops ntxec_pwm_ops = {
+ .apply = ntxec_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static int ntxec_pwm_probe(struct platform_device *pdev)
+{
+ struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
+ struct ntxec_pwm *pwm;
+ struct pwm_chip *chip;
+ int res;
+
+ pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ pwm->ec = ec;
+ pwm->dev = &pdev->dev;
+
+ chip = &pwm->chip;
+ chip->dev = &pdev->dev;
+ chip->ops = &ntxec_pwm_ops;
+ chip->base = -1;
+ chip->npwm = 1;
+
+ res = pwmchip_add(chip);
+ if (res < 0)
+ return res;
+
+ platform_set_drvdata(pdev, pwm);
+
+ return 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 struct platform_driver ntxec_pwm_driver = {
+ .driver = {
+ .name = "ntxec-pwm",
+ },
+ .probe = ntxec_pwm_probe,
+ .remove = ntxec_pwm_remove,
+};
+module_platform_driver(ntxec_pwm_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <[email protected]>");
+MODULE_DESCRIPTION("PWM driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.28.0
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
v3:
- Add Acked-by tag
v2:
- No changes
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 63996ab035217..fa173802000a0 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -712,6 +712,8 @@ patternProperties:
description: Broadcom Corporation (formerly NetLogic Microsystems)
"^netron-dy,.*":
description: Netron DY
+ "^netronix,.*":
+ description: Netronix, Inc.
"^netxeon,.*":
description: Shenzhen Netxeon Technology CO., LTD
"^neweast,.*":
--
2.28.0
Let's make sure I'll notice when there are patches for the NTXEC
drivers.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v3:
- Remove pwm and rtc bindings
v2:
- https://lore.kernel.org/lkml/[email protected]/
- No changes
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index d746519253c3a..f28f7cb890d05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12019,6 +12019,15 @@ F: include/net/netrom.h
F: include/uapi/linux/netrom.h
F: net/netrom/
+NETRONIX EMBEDDED CONTROLLER
+M: Jonathan Neuschäfer <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
+F: drivers/mfd/ntxec.c
+F: drivers/pwm/pwm-ntxec.c
+F: drivers/rtc/rtc-ntxec.c
+F: include/linux/mfd/ntxec.h
+
NETRONOME ETHERNET DRIVERS
M: Simon Horman <[email protected]>
R: Jakub Kicinski <[email protected]>
--
2.28.0
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 <[email protected]>
---
v3:
- Add email address to copyright line
- Remove OF compatible string and don't include linux/of_device.h
- Don't use a comma after sentinels
- Avoid ret |= ... pattern
- Move 8-bit register conversion to ntxec.h
- Relicense as GPLv2 or later
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Rework top-of-file comment [Lee Jones]
- Sort the #include lines [Alexandre Belloni]
- don't align = signs in struct initializers [Uwe Kleine-König]
- Switch to regmap
- Fix register number used to read minutes and seconds
- Prefix registers with NTXEC_REG_
- Add help text to the Kconfig option
- Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max
---
drivers/rtc/Kconfig | 8 +++
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 141 insertions(+)
create mode 100644 drivers/rtc/rtc-ntxec.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 48c536acd777f..ae8f3dc36c9a3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC
This driver can also be built as a module. If so, the module
will be called rtc-cros-ec.
+config RTC_DRV_NTXEC
+ tristate "Netronix embedded controller RTC driver"
+ depends on MFD_NTXEC
+ help
+ Say yes here if you want to support the RTC functionality of the
+ embedded controller found in certain e-book readers designed by the
+ ODM Netronix.
+
comment "on-CPU RTC drivers"
config RTC_DRV_ASM9260
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 880e08a409c3d..733479db18896 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o
obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o
+obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o
obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o
obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o
obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o
diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c
new file mode 100644
index 0000000000000..af23c7cc76544
--- /dev/null
+++ b/drivers/rtc/rtc-ntxec.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The Netronix embedded controller is a microcontroller found in some
+ * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
+ * battery monitoring, system power management, and PWM functionality.
+ *
+ * This driver implements access to the RTC time and date.
+ *
+ * Copyright 2020 Jonathan Neuschäfer <[email protected]>
+ */
+
+#include <linux/mfd/ntxec.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/types.h>
+
+struct ntxec_rtc {
+ struct device *dev;
+ struct ntxec *ec;
+};
+
+#define NTXEC_REG_WRITE_YEAR 0x10
+#define NTXEC_REG_WRITE_MONTH 0x11
+#define NTXEC_REG_WRITE_DAY 0x12
+#define NTXEC_REG_WRITE_HOUR 0x13
+#define NTXEC_REG_WRITE_MINUTE 0x14
+#define NTXEC_REG_WRITE_SECOND 0x15
+
+#define NTXEC_REG_READ_YM 0x20
+#define NTXEC_REG_READ_DH 0x21
+#define NTXEC_REG_READ_MS 0x23
+
+static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
+{
+ struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+ unsigned int value;
+ int res;
+
+ res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
+ if (res < 0)
+ return res;
+
+ tm->tm_year = (value >> 8) + 100;
+ tm->tm_mon = (value & 0xff) - 1;
+
+ res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
+ if (res < 0)
+ return res;
+
+ tm->tm_mday = value >> 8;
+ tm->tm_hour = value & 0xff;
+
+ res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
+ if (res < 0)
+ return res;
+
+ tm->tm_min = value >> 8;
+ tm->tm_sec = value & 0xff;
+
+ return 0;
+}
+
+static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
+{
+ struct ntxec_rtc *rtc = dev_get_drvdata(dev);
+ int res = 0;
+
+ res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
+ if (res)
+ return res;
+
+ res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
+ if (res)
+ return res;
+
+ res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
+ if (res)
+ return res;
+
+ res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
+ if (res)
+ return res;
+
+ res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
+ if (res)
+ return res;
+
+ return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
+}
+
+static const struct rtc_class_ops ntxec_rtc_ops = {
+ .read_time = ntxec_read_time,
+ .set_time = ntxec_set_time,
+};
+
+static int ntxec_rtc_probe(struct platform_device *pdev)
+{
+ struct rtc_device *dev;
+ struct ntxec_rtc *rtc;
+
+ rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+ if (!rtc)
+ return -ENOMEM;
+
+ rtc->dev = &pdev->dev;
+ rtc->ec = dev_get_drvdata(pdev->dev.parent);
+ platform_set_drvdata(pdev, rtc);
+
+ dev = devm_rtc_allocate_device(&pdev->dev);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ dev->ops = &ntxec_rtc_ops;
+ dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
+ dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */
+
+ return rtc_register_device(dev);
+}
+
+static struct platform_driver ntxec_rtc_driver = {
+ .driver = {
+ .name = "ntxec-rtc",
+ },
+ .probe = ntxec_rtc_probe,
+};
+module_platform_driver(ntxec_rtc_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <[email protected]>");
+MODULE_DESCRIPTION("RTC driver for Netronix EC");
+MODULE_LICENSE("GPL");
--
2.28.0
On Thu, 24 Sep 2020 21:24:53 +0200
Jonathan Neuschäfer <[email protected]> wrote:
> With this driver, mainline Linux can keep its time and date in sync with
> the vendor kernel.
>
> Advanced functionality like alarm and automatic power-on is not yet
> supported.
>
> Signed-off-by: Jonathan Neuschäfer <[email protected]>
> ---
>
> v3:
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
> - Relicense as GPLv2 or later
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Rework top-of-file comment [Lee Jones]
> - Sort the #include lines [Alexandre Belloni]
> - don't align = signs in struct initializers [Uwe Kleine-König]
> - Switch to regmap
> - Fix register number used to read minutes and seconds
> - Prefix registers with NTXEC_REG_
> - Add help text to the Kconfig option
> - Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max
> ---
> drivers/rtc/Kconfig | 8 +++
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 141 insertions(+)
> create mode 100644 drivers/rtc/rtc-ntxec.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 48c536acd777f..ae8f3dc36c9a3 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC
> This driver can also be built as a module. If so, the module
> will be called rtc-cros-ec.
>
> +config RTC_DRV_NTXEC
> + tristate "Netronix embedded controller RTC driver"
> + depends on MFD_NTXEC
> + help
> + Say yes here if you want to support the RTC functionality of the
> + embedded controller found in certain e-book readers designed by the
> + ODM Netronix.
> +
> comment "on-CPU RTC drivers"
>
> config RTC_DRV_ASM9260
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 880e08a409c3d..733479db18896 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
> obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
> obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o
> obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o
> +obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o
> obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o
> obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o
> obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o
> diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c
> new file mode 100644
> index 0000000000000..af23c7cc76544
> --- /dev/null
> +++ b/drivers/rtc/rtc-ntxec.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> + * battery monitoring, system power management, and PWM functionality.
> + *
> + * This driver implements access to the RTC time and date.
> + *
> + * Copyright 2020 Jonathan Neuschäfer <[email protected]>
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +#include <linux/types.h>
> +
> +struct ntxec_rtc {
> + struct device *dev;
> + struct ntxec *ec;
> +};
> +
> +#define NTXEC_REG_WRITE_YEAR 0x10
> +#define NTXEC_REG_WRITE_MONTH 0x11
> +#define NTXEC_REG_WRITE_DAY 0x12
> +#define NTXEC_REG_WRITE_HOUR 0x13
> +#define NTXEC_REG_WRITE_MINUTE 0x14
> +#define NTXEC_REG_WRITE_SECOND 0x15
> +
> +#define NTXEC_REG_READ_YM 0x20
> +#define NTXEC_REG_READ_DH 0x21
> +#define NTXEC_REG_READ_MS 0x23
> +
> +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int value;
> + int res;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_year = (value >> 8) + 100;
> + tm->tm_mon = (value & 0xff) - 1;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_mday = value >> 8;
> + tm->tm_hour = value & 0xff;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_min = value >> 8;
> + tm->tm_sec = value & 0xff;
> +
> + return 0;
> +}
> +
> +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + int res = 0;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> + if (res)
> + return res;
> +
> + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> +}
> +
> +static const struct rtc_class_ops ntxec_rtc_ops = {
> + .read_time = ntxec_read_time,
> + .set_time = ntxec_set_time,
> +};
> +
> +static int ntxec_rtc_probe(struct platform_device *pdev)
> +{
> + struct rtc_device *dev;
> + struct ntxec_rtc *rtc;
> +
> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> + if (!rtc)
> + return -ENOMEM;
> +
> + rtc->dev = &pdev->dev;
> + rtc->ec = dev_get_drvdata(pdev->dev.parent);
> + platform_set_drvdata(pdev, rtc);
> +
> + dev = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> +
> + dev->ops = &ntxec_rtc_ops;
> + dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */
> +
> + return rtc_register_device(dev);
> +}
> +
> +static struct platform_driver ntxec_rtc_driver = {
> + .driver = {
> + .name = "ntxec-rtc",
> + },
> + .probe = ntxec_rtc_probe,
> +};
> +module_platform_driver(ntxec_rtc_driver);
> +
I think module autoloading will not work without
MODULE_ALIAS("platform:ntxec-rtc");
Same for the pwm device.
> +MODULE_AUTHOR("Jonathan Neuschäfer <[email protected]>");
> +MODULE_DESCRIPTION("RTC driver for Netronix EC");
> +MODULE_LICENSE("GPL");
> --
> 2.28.0
>
>
Regards,
Andreas
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 <[email protected]>
---
v3:
- Remove interrupt-controller property from embedded-controller node
- subnodes of embedded-controller node in to the main node
v2:
- https://lore.kernel.org/lkml/[email protected]/
- Fix pwm-cells property (should be 2, not 1)
---
arch/arm/boot/dts/imx50-kobo-aura.dts | 17 ++++++++++++++++-
1 file changed, 16 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..2d1a59091a37c 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,24 @@ &i2c3 {
pinctrl-0 = <&pinctrl_i2c3>;
status = "okay";
- /* TODO: embedded controller at 0x43 */
+ embedded-controller@43 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ec>;
+ compatible = "netronix,ntxec";
+ reg = <0x43>;
+ system-power-controller;
+ interrupts-extended = <&gpio4 11 IRQ_TYPE_EDGE_FALLING>;
+ #pwm-cells = <2>;
+ };
};
&iomuxc {
+ pinctrl_ec: ec {
+ fsl,pins = <
+ MX50_PAD_CSPI_SS0__GPIO4_11 0x0 /* INT */
+ >;
+ };
+
pinctrl_gpiokeys: gpiokeys {
fsl,pins = <
MX50_PAD_CSPI_MISO__GPIO4_10 0x0
--
2.28.0
Hello Jonathan,
On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neusch?fer wrote:
> +#define NTXEC_REG_WRITE_YEAR 0x10
> +#define NTXEC_REG_WRITE_MONTH 0x11
> +#define NTXEC_REG_WRITE_DAY 0x12
> +#define NTXEC_REG_WRITE_HOUR 0x13
> +#define NTXEC_REG_WRITE_MINUTE 0x14
> +#define NTXEC_REG_WRITE_SECOND 0x15
> +
> +#define NTXEC_REG_READ_YM 0x20
> +#define NTXEC_REG_READ_DH 0x21
> +#define NTXEC_REG_READ_MS 0x23
Is this an official naming? I think at least ..._MS is a poor name.
Maybe consider ..._MINSEC instead and make the other two names a bit longer
for consistency?
> +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int value;
> + int res;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_year = (value >> 8) + 100;
> + tm->tm_mon = (value & 0xff) - 1;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_mday = value >> 8;
> + tm->tm_hour = value & 0xff;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_min = value >> 8;
> + tm->tm_sec = value & 0xff;
> +
> + return 0;
> +}
> +
> +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + int res = 0;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> + if (res)
> + return res;
> +
> + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
I wonder: Is this racy? If you write minute, does the seconds reset to
zero or something like that? Or can it happen, that after writing the
minute register and before writing the second register the seconds
overflow and you end up with the time set to a minute later than
intended? If so it might be worth to set the seconds to 0 at the start
of the function (with an explaining comment).
.read_time has a similar race. What happens if minutes overflow between
reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS?
> +static struct platform_driver ntxec_rtc_driver = {
> + .driver = {
> + .name = "ntxec-rtc",
> + },
> + .probe = ntxec_rtc_probe,
No .remove function?
> +};
> +module_platform_driver(ntxec_rtc_driver);
> +
> +MODULE_AUTHOR("Jonathan Neusch?fer <[email protected]>");
> +MODULE_DESCRIPTION("RTC driver for Netronix EC");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Jonathan,
On Thu, Sep 24, 2020 at 09:24:52PM +0200, Jonathan Neusch?fer wrote:
> The Netronix EC provides a PWM output which is used for the backlight
> on some ebook readers. This patches adds a driver for the PWM output.
>
> The .get_state callback is not implemented, because the PWM state can't
> be read back from the hardware.
>
> Signed-off-by: Jonathan Neusch?fer <[email protected]>
> ---
>
> v3:
> - Relicense as GPLv2 or later
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Fix bogus ?: in return line
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Various grammar and style improvements, as suggested by Uwe Kleine-K?nig,
> Lee Jones, and Alexandre Belloni
> - Switch to regmap
> - Prefix registers with NTXEC_REG_
> - Add help text to the Kconfig option
> - Use the .apply callback instead of the old API
> - Add a #define for the time base (125ns)
> - Don't change device state in .probe; this avoids multiple problems
> - Rework division and overflow check logic to perform divisions in 32 bits
> - Avoid setting duty cycle to zero, to work around a hardware quirk
> ---
> drivers/pwm/Kconfig | 8 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-ntxec.c | 161 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 170 insertions(+)
> create mode 100644 drivers/pwm/pwm-ntxec.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d335..530dfda38d65e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -350,6 +350,14 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_NTXEC
> + tristate "Netronix embedded controller PWM support"
> + depends on MFD_NTXEC
> + help
> + Say yes here if you want to support the PWM output of the embedded
> + controller found in certain e-book readers designed by the ODM
> + Netronix.
Is it only me who had to look up what ODM means? If not, maybe spell it
out?
> +
> config PWM_OMAP_DMTIMER
> tristate "OMAP Dual-Mode Timer PWM support"
> depends on OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a035577..1cc50dba22d1b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON) += pwm-meson.o
> obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
> obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o
> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
> +obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
> obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o
> obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
> new file mode 100644
> index 0000000000000..50da2dc14bb03
> --- /dev/null
> +++ b/drivers/pwm/pwm-ntxec.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> + * battery monitoring, system power management, and PWM functionality.
> + *
> + * This driver implements PWM output.
> + *
> + * Copyright 2020 Jonathan Neusch?fer <[email protected]>
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct ntxec_pwm {
> + struct device *dev;
> + struct ntxec *ec;
> + struct pwm_chip chip;
> +};
> +
> +static struct ntxec_pwm *pwmchip_to_pwm(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct ntxec_pwm, chip);
> +}
> +
> +#define NTXEC_REG_AUTO_OFF_HI 0xa1
> +#define NTXEC_REG_AUTO_OFF_LO 0xa2
> +#define NTXEC_REG_ENABLE 0xa3
> +#define NTXEC_REG_PERIOD_LOW 0xa4
> +#define NTXEC_REG_PERIOD_HIGH 0xa5
> +#define NTXEC_REG_DUTY_LOW 0xa6
> +#define NTXEC_REG_DUTY_HIGH 0xa7
> +
> +/*
> + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> + * measured in this unit.
> + */
> +#define TIME_BASE_NS 125
> +
> +/*
> + * The maximum input value (in nanoseconds) is determined by the time base and
> + * the range of the hardware registers that hold the converted value.
> + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> + */
> +#define MAX_PERIOD_NS (TIME_BASE_NS * 0x10000 - 1)
The maximal configurable period length is 0xffff, so I would have
expected MAX_PERIOD_NS to be 0xffff * TIME_BASE_NS?
> +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> + const struct pwm_state *state)
> +{
> + struct ntxec_pwm *pwm = pwmchip_to_pwm(pwm_dev->chip);
> + unsigned int duty = state->duty_cycle;
> + unsigned int period = state->period;
> + int res = 0;
> +
I assume your device only supports normal polarity? If so, please check
for it here and point out this limitation in the header (in the format
that is for example used in pwm-sifive.c to make it easy to grep for
that).
> + if (period > MAX_PERIOD_NS) {
> + dev_warn(pwm->dev,
> + "Period is not representable in 16 bits after division by %u: %u\n",
> + TIME_BASE_NS, period);
No error messages in .apply() please; this might spam the kernel log.
Also the expectation when a too big period is requested is to configure
for the biggest possible period. So just do:
if (period > MAX_PERIOD_NS) {
period = MAX_PERIOD_NS;
if (duty > period)
duty = period;
}
(or something equivalent).
> + return -ERANGE;
> + }
> +
> + period /= TIME_BASE_NS;
> + duty /= TIME_BASE_NS;
> +
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> + if (res)
> + return res;
> +
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> + if (res)
> + return res;
> +
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> + if (res)
> + return res;
> +
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> + if (res)
> + return res;
> +
> + /*
> + * Writing a duty cycle of zone puts the device into a state where
What is "zone"? A mixture of zero and one and so approximately 0.5?
> + * writing a higher duty cycle doesn't result in the brightness that it
> + * usually results in. This can be fixed by cycling the ENABLE register.
> + *
> + * As a workaround, write ENABLE=0 when the duty cycle is zero.
> + */
> + if (state->enabled && duty != 0) {
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> + if (res)
> + return res;
> +
> + /* Disable the auto-off timer */
> + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> + if (res)
> + return res;
> +
> + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> + } else {
> + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> + }
This code is wrong for state->enabled = false.
How does the PWM behave when .apply is called? Does it complete the
currently running period? Can it happen that when you switch from say
.duty_cycle = 900 * TIME_BASE_NS (0x384)
.period = 1800 * TIME_BASE_NS (0x708)
to
.duty_cycle = 300 * TIME_BASE_NS (0x12c)
.period = 600 * TIME_BASE_NS (0x258)
that a period with
.duty_cycle = 388 * TIME_BASE_NS (0x184)
.period = 1800 * TIME_BASE_NS (0x708)
(because only NTXEC_REG_PERIOD_HIGH was written when the new period
started) or something similar is emitted?
> +}
> +
> +static struct pwm_ops ntxec_pwm_ops = {
> + .apply = ntxec_pwm_apply,
Please implement a .get_state() callback. And enable PWM_DEBUG during
your tests.
> + .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;
Please don't call this variable pwm. I would expect that a variable with
this name is of type pwm_device. I would have called it "ddata" (and the
type would be named ntxec_pwm_ddata for me); another usual name is "priv".
> + 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);
If you do the platform_set_drvdata earlier you can just do
return pwmchip_add(chip);
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Sep 24, 2020 at 10:26 PM Jonathan Neuschäfer
<[email protected]> wrote:
>
> The Netronix embedded controller is a microcontroller found in some
> e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> battery monitoring, system power management, and PWM functionality.
>
> This driver implements register access and version detection.
>
> Third-party hardware documentation is available at:
>
> https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
>
> The EC supports interrupts, but the driver doesn't make use of them so
> far.
...
> +#include <asm/unaligned.h>
This usually goes after linux/*.h
(and actually not visible how it's being used, but see below first)
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
...
> +static void ntxec_poweroff(void)
> +{
> + int res;
> + u8 buf[] = {
> + NTXEC_REG_POWEROFF,
> + (NTXEC_POWEROFF_VALUE >> 8) & 0xff,
> + NTXEC_POWEROFF_VALUE & 0xff,
'& 0xff' parts are redundant. *u8 implies that. Fix in all cases.
Also I would rather see something like
buf[0] = _POWEROFF;
put_unaligned_be16(_VALUE, &buf[1]);
to explicitly show the endianess of the register values.
> + };
> + struct i2c_msg msgs[] = {
> + {
> + .addr = poweroff_restart_client->addr,
> + .flags = 0,
> + .len = sizeof(buf),
> + .buf = buf
It's slightly better to keep trailing commas in cases like this.
> + }
> + };
> +
> + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (res < 0)
> + dev_alert(&poweroff_restart_client->dev,
> + "Failed to power off (err = %d)\n", res);
alert? This needs to be explained.
> + /*
> + * The time from the register write until the host CPU is powered off
> + * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
> + * safely avoid returning from the poweroff handler.
> + */
> + msleep(5000);
> +}
> +
> +static int ntxec_restart(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + int res;
> + /*
> + * NOTE: The lower half of the reset value is not sent, because sending
> + * it causes an error
Why? Any root cause? Perhaps you need to send 0xffff ?
> + */
> + u8 buf[] = {
> + NTXEC_REG_RESET,
> + (NTXEC_RESET_VALUE >> 8) & 0xff,
Here you may still use put_unaligned_be16() but move the comment to be
before len hardcoded to sizeof(buf) - 1.
> + };
> + struct i2c_msg msgs[] = {
> + {
> + .addr = poweroff_restart_client->addr,
> + .flags = 0,
> + .len = sizeof(buf),
> + .buf = buf
> + }
> + };
> +
> + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (res < 0)
> + dev_alert(&poweroff_restart_client->dev,
> + "Failed to restart (err = %d)\n", res);
> +
> + return NOTIFY_DONE;
> +}
...
> +static int ntxec_probe(struct i2c_client *client)
> +{
> + struct ntxec *ec;
> + unsigned int version;
> + int res;
> +
> + ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> + if (!ec)
> + return -ENOMEM;
> +
> + ec->dev = &client->dev;
> +
> + ec->regmap = devm_regmap_init_i2c(client, ®map_config);
> + if (IS_ERR(ec->regmap)) {
> + dev_err(ec->dev, "Failed to set up regmap for device\n");
> + return res;
> + }
> +
> + /* Determine the firmware version */
> + res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
> + if (res < 0) {
> + dev_err(ec->dev, "Failed to read firmware version number\n");
> + return res;
> + }
> + dev_info(ec->dev,
> + "Netronix embedded controller version %04x detected.\n",
> + version);
This info level may confuse users if followed by an error path.
> + /* Bail out if we encounter an unknown firmware version */
> + switch (version) {
> + case 0xd726: /* found in Kobo Aura */
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + if (of_device_is_system_power_controller(ec->dev->of_node)) {
> + /*
> + * Set the 'powerkeep' bit. This is necessary on some boards
> + * in order to keep the system running.
> + */
> + res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
> + NTXEC_POWERKEEP_VALUE);
> + if (res < 0)
> + return res;
> + WARN_ON(poweroff_restart_client);
WARN_ON? All these alerts, WARNs, BUGs must be explained. Screaming to
the user is not good if it wasn't justified.
> + poweroff_restart_client = client;
> + if (pm_power_off)
> + dev_err(ec->dev, "pm_power_off already assigned\n");
> + else
> + pm_power_off = ntxec_poweroff;
> +
> + res = register_restart_handler(&ntxec_restart_handler);
> + if (res)
> + dev_err(ec->dev,
> + "Failed to register restart handler: %d\n", res);
> + }
> +
> + i2c_set_clientdata(client, ec);
> +
> + res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
> + ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
> + if (res)
> + dev_warn(ec->dev, "Failed to add subdevices: %d\n", res);
'warn' is inconsistent with 'return err'. Either do not return an
error, or mark a message as an error one.
And above with the restart handler has the same issue.
> + return res;
> +}
> +
> +static int ntxec_remove(struct i2c_client *client)
> +{
> + if (client == poweroff_restart_client) {
When it's not the case?
> + poweroff_restart_client = NULL;
> + pm_power_off = NULL;
> + unregister_restart_handler(&ntxec_restart_handler);
> + }
> +
> + return 0;
> +}
...
> +#include <linux/types.h>
> +
Missed
struct device;
struct regmap;
here.
> +struct ntxec {
> + struct device *dev;
> + struct regmap *regmap;
> +};
> +/*
> + * Some registers, such as the battery status register (0x41), are in
> + * big-endian, but others only have eight significant bits, which are in the
> + * first byte transmitted over I2C (the MSB of the big-endian value).
> + * This convenience function converts an 8-bit value to 16-bit for use in the
> + * second kind of register.
> + */
> +static inline u16 ntxec_reg8(u8 value)
> +{
> + return value << 8;
> +}
I'm wondering why __be16 is not used as returned type.
--
With Best Regards,
Andy Shevchenko
Hi,
On 24/09/2020 21:24:53+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.
>
> Signed-off-by: Jonathan Neusch?fer <[email protected]>
> ---
>
> v3:
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
> - Relicense as GPLv2 or later
I don't think you had to relicense. The kernel is GPL 2 only, you are
free to license your code under GPL 2 only if that is what you desire.
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Rework top-of-file comment [Lee Jones]
> - Sort the #include lines [Alexandre Belloni]
> - don't align = signs in struct initializers [Uwe Kleine-K?nig]
> - Switch to regmap
> - Fix register number used to read minutes and seconds
> - Prefix registers with NTXEC_REG_
> - Add help text to the Kconfig option
> - Use devm_rtc_allocate_device and rtc_register_device, set ->range_min and ->range_max
> ---
> drivers/rtc/Kconfig | 8 +++
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-ntxec.c | 132 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 141 insertions(+)
> create mode 100644 drivers/rtc/rtc-ntxec.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 48c536acd777f..ae8f3dc36c9a3 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1301,6 +1301,14 @@ config RTC_DRV_CROS_EC
> This driver can also be built as a module. If so, the module
> will be called rtc-cros-ec.
>
> +config RTC_DRV_NTXEC
> + tristate "Netronix embedded controller RTC driver"
> + depends on MFD_NTXEC
> + help
> + Say yes here if you want to support the RTC functionality of the
> + embedded controller found in certain e-book readers designed by the
> + ODM Netronix.
> +
> comment "on-CPU RTC drivers"
>
> config RTC_DRV_ASM9260
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 880e08a409c3d..733479db18896 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -111,6 +111,7 @@ obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
> obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
> obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o
> obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o
> +obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o
> obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o
> obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o
> obj-$(CONFIG_RTC_DRV_PALMAS) += rtc-palmas.o
> diff --git a/drivers/rtc/rtc-ntxec.c b/drivers/rtc/rtc-ntxec.c
> new file mode 100644
> index 0000000000000..af23c7cc76544
> --- /dev/null
> +++ b/drivers/rtc/rtc-ntxec.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> + * battery monitoring, system power management, and PWM functionality.
> + *
> + * This driver implements access to the RTC time and date.
> + *
> + * Copyright 2020 Jonathan Neusch?fer <[email protected]>
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +#include <linux/types.h>
> +
> +struct ntxec_rtc {
> + struct device *dev;
> + struct ntxec *ec;
> +};
> +
> +#define NTXEC_REG_WRITE_YEAR 0x10
> +#define NTXEC_REG_WRITE_MONTH 0x11
> +#define NTXEC_REG_WRITE_DAY 0x12
> +#define NTXEC_REG_WRITE_HOUR 0x13
> +#define NTXEC_REG_WRITE_MINUTE 0x14
> +#define NTXEC_REG_WRITE_SECOND 0x15
> +
> +#define NTXEC_REG_READ_YM 0x20
> +#define NTXEC_REG_READ_DH 0x21
> +#define NTXEC_REG_READ_MS 0x23
> +
> +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int value;
> + int res;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_year = (value >> 8) + 100;
> + tm->tm_mon = (value & 0xff) - 1;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_mday = value >> 8;
> + tm->tm_hour = value & 0xff;
> +
> + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> + if (res < 0)
> + return res;
> +
> + tm->tm_min = value >> 8;
> + tm->tm_sec = value & 0xff;
> +
> + return 0;
> +}
> +
> +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> + int res = 0;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> + if (res)
> + return res;
> +
> + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> + if (res)
> + return res;
> +
> + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> +}
> +
> +static const struct rtc_class_ops ntxec_rtc_ops = {
> + .read_time = ntxec_read_time,
> + .set_time = ntxec_set_time,
> +};
> +
> +static int ntxec_rtc_probe(struct platform_device *pdev)
> +{
> + struct rtc_device *dev;
> + struct ntxec_rtc *rtc;
> +
> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> + if (!rtc)
> + return -ENOMEM;
> +
> + rtc->dev = &pdev->dev;
> + rtc->ec = dev_get_drvdata(pdev->dev.parent);
> + platform_set_drvdata(pdev, rtc);
> +
> + dev = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> +
> + dev->ops = &ntxec_rtc_ops;
> + dev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + dev->range_max = 9025257599LL; /* 2255-12-31 23:59:59 */
> +
> + return rtc_register_device(dev);
> +}
> +
> +static struct platform_driver ntxec_rtc_driver = {
> + .driver = {
> + .name = "ntxec-rtc",
> + },
> + .probe = ntxec_rtc_probe,
> +};
> +module_platform_driver(ntxec_rtc_driver);
> +
> +MODULE_AUTHOR("Jonathan Neusch?fer <[email protected]>");
> +MODULE_DESCRIPTION("RTC driver for Netronix EC");
> +MODULE_LICENSE("GPL");
> --
> 2.28.0
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Fri, Sep 25, 2020 at 12:29:45PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 24, 2020 at 10:26 PM Jonathan Neuschäfer
> <[email protected]> wrote:
> >
> > The Netronix embedded controller is a microcontroller found in some
> > e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> > battery monitoring, system power management, and PWM functionality.
> >
> > This driver implements register access and version detection.
> >
> > Third-party hardware documentation is available at:
> >
> > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller
> >
> > The EC supports interrupts, but the driver doesn't make use of them so
> > far.
>
> ...
>
> > +#include <asm/unaligned.h>
>
> This usually goes after linux/*.h
> (and actually not visible how it's being used, but see below first)
I think it was a leftover from v1 before I used regmap for general
access to the registers. Will fix the ordering.
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/ntxec.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/reboot.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
>
> ...
>
> > +static void ntxec_poweroff(void)
> > +{
> > + int res;
> > + u8 buf[] = {
> > + NTXEC_REG_POWEROFF,
>
> > + (NTXEC_POWEROFF_VALUE >> 8) & 0xff,
> > + NTXEC_POWEROFF_VALUE & 0xff,
>
> '& 0xff' parts are redundant. *u8 implies that. Fix in all cases.
> Also I would rather see something like
>
> buf[0] = _POWEROFF;
> put_unaligned_be16(_VALUE, &buf[1]);
>
> to explicitly show the endianess of the register values.
Good idea.
> > + };
> > + struct i2c_msg msgs[] = {
> > + {
> > + .addr = poweroff_restart_client->addr,
> > + .flags = 0,
> > + .len = sizeof(buf),
>
> > + .buf = buf
>
> It's slightly better to keep trailing commas in cases like this.
Ok.
>
> > + }
> > + };
> > +
> > + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (res < 0)
>
> > + dev_alert(&poweroff_restart_client->dev,
> > + "Failed to power off (err = %d)\n", res);
>
> alert? This needs to be explained.
I copied the dev_alert from drivers/mfd/rn5t618.c.
Upon reconsideration, I'm not sure what the correct log level would be,
but _warn seems enough, or maybe _err is better
> > + /*
> > + * The time from the register write until the host CPU is powered off
> > + * has been observed to be about 2.5 to 3 seconds. Sleep long enough to
> > + * safely avoid returning from the poweroff handler.
> > + */
> > + msleep(5000);
> > +}
> > +
> > +static int ntxec_restart(struct notifier_block *nb,
> > + unsigned long action, void *data)
> > +{
> > + int res;
> > + /*
> > + * NOTE: The lower half of the reset value is not sent, because sending
> > + * it causes an error
>
> Why? Any root cause? Perhaps you need to send 0xffff ?
Unknown, because I don't have the EC firmware for analysis. The vendor
kernel sends 0xff00 and gets an error.
Sending 0xffff doesn't help.
> > + */
> > + u8 buf[] = {
> > + NTXEC_REG_RESET,
>
> > + (NTXEC_RESET_VALUE >> 8) & 0xff,
>
> Here you may still use put_unaligned_be16() but move the comment to be
> before len hardcoded to sizeof(buf) - 1.
Okay, will do.
>
> > + };
> > + struct i2c_msg msgs[] = {
> > + {
> > + .addr = poweroff_restart_client->addr,
> > + .flags = 0,
> > + .len = sizeof(buf),
> > + .buf = buf
> > + }
> > + };
> > +
> > + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (res < 0)
> > + dev_alert(&poweroff_restart_client->dev,
> > + "Failed to restart (err = %d)\n", res);
> > +
> > + return NOTIFY_DONE;
> > +}
>
> ...
An error in the i2c transfer here is an abnormal situation that should
in my opinion be logged, but I don't see what else the code can do here
to handle the error.
>
> > +static int ntxec_probe(struct i2c_client *client)
> > +{
> > + struct ntxec *ec;
> > + unsigned int version;
> > + int res;
> > +
> > + ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL);
> > + if (!ec)
> > + return -ENOMEM;
> > +
> > + ec->dev = &client->dev;
> > +
> > + ec->regmap = devm_regmap_init_i2c(client, ®map_config);
> > + if (IS_ERR(ec->regmap)) {
> > + dev_err(ec->dev, "Failed to set up regmap for device\n");
> > + return res;
> > + }
> > +
> > + /* Determine the firmware version */
> > + res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version);
> > + if (res < 0) {
> > + dev_err(ec->dev, "Failed to read firmware version number\n");
> > + return res;
> > + }
>
> > + dev_info(ec->dev,
> > + "Netronix embedded controller version %04x detected.\n",
> > + version);
>
> This info level may confuse users if followed by an error path.
Right. I suppose printing incompatible versions is still useful, so how
about something like the following?
/* Bail out if we encounter an unknown firmware version */
switch (version) {
case 0xd726: /* found in Kobo Aura */
dev_info(ec->dev,
"Netronix embedded controller version %04x detected.\n",
version);
break;
default:
dev_err(ec->dev,
"Netronix embedded controller version %04x is not supported.\n",
version);
return -ENODEV;
}
> > +
> > + if (of_device_is_system_power_controller(ec->dev->of_node)) {
> > + /*
> > + * Set the 'powerkeep' bit. This is necessary on some boards
> > + * in order to keep the system running.
> > + */
> > + res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP,
> > + NTXEC_POWERKEEP_VALUE);
> > + if (res < 0)
> > + return res;
>
> > + WARN_ON(poweroff_restart_client);
>
> WARN_ON? All these alerts, WARNs, BUGs must be explained. Screaming to
> the user is not good if it wasn't justified.
poweroff_restart_client being already set is not a situation that should
happen (and would indicate a bug in this driver, AFAICT), but I guess
the log message could be better in that case...
> > + poweroff_restart_client = client;
> > + if (pm_power_off)
> > + dev_err(ec->dev, "pm_power_off already assigned\n");
> > + else
> > + pm_power_off = ntxec_poweroff;
> > +
> > + res = register_restart_handler(&ntxec_restart_handler);
> > + if (res)
> > + dev_err(ec->dev,
> > + "Failed to register restart handler: %d\n", res);
> > + }
> > +
> > + i2c_set_clientdata(client, ec);
> > +
> > + res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices,
> > + ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL);
> > + if (res)
>
> > + dev_warn(ec->dev, "Failed to add subdevices: %d\n", res);
>
> 'warn' is inconsistent with 'return err'. Either do not return an
> error, or mark a message as an error one.
Okay, I'll change it to dev_err.
>
> And above with the restart handler has the same issue.
>
> > + return res;
> > +}
> > +
> > +static int ntxec_remove(struct i2c_client *client)
> > +{
>
> > + if (client == poweroff_restart_client) {
>
> When it's not the case?
The EC doesn't always need to provide poweroff/restart functionality,
and AFAIK, in some systems it doesn't. In those systems, ntxec_remove
would run with poweroff_restart_client == NULL.
In theory, there might also be two of it in the same system, of which
only one controls system poweroff/restart, but I'm not sure if that is
actually the case on any existing board design.
> > + poweroff_restart_client = NULL;
> > + pm_power_off = NULL;
> > + unregister_restart_handler(&ntxec_restart_handler);
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +#include <linux/types.h>
> > +
>
> Missed
>
> struct device;
> struct regmap;
>
> here.
I'll add them.
> > +struct ntxec {
> > + struct device *dev;
> > + struct regmap *regmap;
> > +};
>
> > +/*
> > + * Some registers, such as the battery status register (0x41), are in
> > + * big-endian, but others only have eight significant bits, which are in the
> > + * first byte transmitted over I2C (the MSB of the big-endian value).
> > + * This convenience function converts an 8-bit value to 16-bit for use in the
> > + * second kind of register.
> > + */
> > +static inline u16 ntxec_reg8(u8 value)
> > +{
> > + return value << 8;
> > +}
>
> I'm wondering why __be16 is not used as returned type.
I didn't think of it, but it's a good idea. Will do.
Thanks,
Jonathan Neuschäfer
On Fri, Sep 25, 2020 at 11:36:14AM +0200, Alexandre Belloni wrote:
> Hi,
>
> On 24/09/2020 21:24:53+0200, Jonathan Neuschäfer wrote:
...
> > v3:
...
> > - Relicense as GPLv2 or later
>
> I don't think you had to relicense. The kernel is GPL 2 only, you are
> free to license your code under GPL 2 only if that is what you desire.
I don't mind in this case.
On Fri, Sep 25, 2020 at 08:30:37AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
[...]
> > +config PWM_NTXEC
> > + tristate "Netronix embedded controller PWM support"
> > + depends on MFD_NTXEC
> > + help
> > + Say yes here if you want to support the PWM output of the embedded
> > + controller found in certain e-book readers designed by the ODM
> > + Netronix.
>
> Is it only me who had to look up what ODM means? If not, maybe spell it
> out?
I'm sure other readers will have the same problem. I'll spell it out.
> > +/*
> > + * The maximum input value (in nanoseconds) is determined by the time base and
> > + * the range of the hardware registers that hold the converted value.
> > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > + */
> > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0x10000 - 1)
>
> The maximal configurable period length is 0xffff, so I would have
> expected MAX_PERIOD_NS to be 0xffff * TIME_BASE_NS?
Due to the division rounding down, TIME_BASE_NS * 0x10000 - 1 would be
the highest input that results in a representable value after the
division, but I'm not sure it otherwise makes sense.
>
> > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > + const struct pwm_state *state)
> > +{
> > + struct ntxec_pwm *pwm = pwmchip_to_pwm(pwm_dev->chip);
> > + unsigned int duty = state->duty_cycle;
> > + unsigned int period = state->period;
> > + int res = 0;
> > +
>
> I assume your device only supports normal polarity? If so, please check
> for it here and point out this limitation in the header (in the format
> that is for example used in pwm-sifive.c to make it easy to grep for
> that).
I haven't seen any indication that it supports inverted polarity. I'll
point it out in the header comment, and add a check.
>
> > + if (period > MAX_PERIOD_NS) {
> > + dev_warn(pwm->dev,
> > + "Period is not representable in 16 bits after division by %u: %u\n",
> > + TIME_BASE_NS, period);
>
> No error messages in .apply() please; this might spam the kernel log.
>
> Also the expectation when a too big period is requested is to configure
> for the biggest possible period. So just do:
>
> if (period > MAX_PERIOD_NS) {
> period = MAX_PERIOD_NS;
>
> if (duty > period)
> duty = period;
> }
>
> (or something equivalent).
Okay, I'll adjust it.
> > + /*
> > + * Writing a duty cycle of zone puts the device into a state where
>
> What is "zone"? A mixture of zero and one and so approximately 0.5?
Oops, that's a typo. I just meant "zero".
> > + * writing a higher duty cycle doesn't result in the brightness that it
> > + * usually results in. This can be fixed by cycling the ENABLE register.
> > + *
> > + * As a workaround, write ENABLE=0 when the duty cycle is zero.
> > + */
> > + if (state->enabled && duty != 0) {
> > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > + if (res)
> > + return res;
> > +
> > + /* Disable the auto-off timer */
> > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > + if (res)
> > + return res;
> > +
> > + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > + } else {
> > + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > + }
>
> This code is wrong for state->enabled = false.
Why?
> How does the PWM behave when .apply is called? Does it complete the
> currently running period? Can it happen that when you switch from say
>
> .duty_cycle = 900 * TIME_BASE_NS (0x384)
> .period = 1800 * TIME_BASE_NS (0x708)
>
> to
>
> .duty_cycle = 300 * TIME_BASE_NS (0x12c)
> .period = 600 * TIME_BASE_NS (0x258)
>
> that a period with
>
> .duty_cycle = 388 * TIME_BASE_NS (0x184)
> .period = 1800 * TIME_BASE_NS (0x708)
>
> (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> started) or something similar is emitted?
Changes take effect after the low byte is written, so a result like 0x184
in the above example should not happen.
When the period and duty cycle are both changed, it temporarily results
in an inconsistent state:
- period = 1800ns, duty cycle = 900ns
- period = 600ns, duty cycle = 900ns (!)
- period = 600ns, duty cycle = 300ns
The inconsistent state of duty cycle > period is handled gracefully by
the EC and it outputs a 100% duty cycle, as far as I can tell.
I currently don't have a logic analyzer / oscilloscope to measure
whether we get full PWM periods, or some kind of glitch when the new
period starts in the middle of the last one.
> > +}
> > +
> > +static struct pwm_ops ntxec_pwm_ops = {
> > + .apply = ntxec_pwm_apply,
>
> Please implement a .get_state() callback. And enable PWM_DEBUG during
> your tests.
The device doesn't support reading back the PWM state. What should a
driver do in this case?
> > + .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;
>
> Please don't call this variable pwm. I would expect that a variable with
> this name is of type pwm_device. I would have called it "ddata" (and the
> type would be named ntxec_pwm_ddata for me); another usual name is "priv".
Ok, I'll rename it.
> > + chip->npwm = 1;
> > +
> > + res = pwmchip_add(chip);
> > + if (res < 0)
> > + return res;
> > +
> > + platform_set_drvdata(pdev, pwm);
>
> If you do the platform_set_drvdata earlier you can just do
>
> return pwmchip_add(chip);
Good idea, I'll do that.
Thanks,
Jonathan Neuschäfer
Hello Jonathan,
On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neusch?fer wrote:
> On Fri, Sep 25, 2020 at 08:30:37AM +0200, Uwe Kleine-K?nig wrote:
> > > + if (period > MAX_PERIOD_NS) {
> > > + dev_warn(pwm->dev,
> > > + "Period is not representable in 16 bits after division by %u: %u\n",
> > > + TIME_BASE_NS, period);
> >
> > No error messages in .apply() please; this might spam the kernel log.
> >
> > Also the expectation when a too big period is requested is to configure
> > for the biggest possible period. So just do:
> >
> > if (period > MAX_PERIOD_NS) {
> > period = MAX_PERIOD_NS;
> >
> > if (duty > period)
> > duty = period;
> > }
> >
> > (or something equivalent).
>
> Okay, I'll adjust it.
>
> > > + /*
> > > + * Writing a duty cycle of zone puts the device into a state where
> >
> > What is "zone"? A mixture of zero and one and so approximately 0.5?
>
> Oops, that's a typo. I just meant "zero".
>
> > > + * writing a higher duty cycle doesn't result in the brightness that it
> > > + * usually results in. This can be fixed by cycling the ENABLE register.
> > > + *
> > > + * As a workaround, write ENABLE=0 when the duty cycle is zero.
> > > + */
> > > + if (state->enabled && duty != 0) {
> > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > > + if (res)
> > > + return res;
> > > +
> > > + /* Disable the auto-off timer */
> > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > > + if (res)
> > > + return res;
> > > +
> > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > > + } else {
> > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > > + }
> >
> > This code is wrong for state->enabled = false.
>
> Why?
Hm, I wonder the same. Probably I just misunderstood the code, sorry :-\
> > How does the PWM behave when .apply is called? Does it complete the
> > currently running period? Can it happen that when you switch from say
> >
> > .duty_cycle = 900 * TIME_BASE_NS (0x384)
> > .period = 1800 * TIME_BASE_NS (0x708)
> >
> > to
> >
> > .duty_cycle = 300 * TIME_BASE_NS (0x12c)
> > .period = 600 * TIME_BASE_NS (0x258)
> >
> > that a period with
> >
> > .duty_cycle = 388 * TIME_BASE_NS (0x184)
> > .period = 1800 * TIME_BASE_NS (0x708)
> >
> > (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> > started) or something similar is emitted?
>
> Changes take effect after the low byte is written, so a result like 0x184
> in the above example should not happen.
>
> When the period and duty cycle are both changed, it temporarily results
> in an inconsistent state:
>
> - period = 1800ns, duty cycle = 900ns
> - period = 600ns, duty cycle = 900ns (!)
> - period = 600ns, duty cycle = 300ns
Does this always happen, or only if a new cycle starts at an unlucky
moment?
> The inconsistent state of duty cycle > period is handled gracefully by
> the EC and it outputs a 100% duty cycle, as far as I can tell.
OK.
> I currently don't have a logic analyzer / oscilloscope to measure
> whether we get full PWM periods, or some kind of glitch when the new
> period starts in the middle of the last one.
You can even check this with an LED using something like:
pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
pwm_apply(mypwm, {.enabled = false, ... });
. If the period is completed the LED is on for $big ns, if not the LED
is on for a timespan that is probably hardly noticable with the human
eye.
> > > +}
> > > +
> > > +static struct pwm_ops ntxec_pwm_ops = {
> > > + .apply = ntxec_pwm_apply,
> >
> > Please implement a .get_state() callback. And enable PWM_DEBUG during
> > your tests.
>
> The device doesn't support reading back the PWM state. What should a
> driver do in this case?
Document it as a limitation, please.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sat, Sep 26, 2020 at 12:32 AM Jonathan Neuschäfer
<[email protected]> wrote:
> On Fri, Sep 25, 2020 at 12:29:45PM +0300, Andy Shevchenko wrote:
> > On Thu, Sep 24, 2020 at 10:26 PM Jonathan Neuschäfer
> > <[email protected]> wrote:
...
> > > + dev_alert(&poweroff_restart_client->dev,
> > > + "Failed to power off (err = %d)\n", res);
> >
> > alert? This needs to be explained.
>
> I copied the dev_alert from drivers/mfd/rn5t618.c.
>
> Upon reconsideration, I'm not sure what the correct log level would be,
> but _warn seems enough, or maybe _err is better
It's up to you to decide, but crit/alert and similar has to be
justified (commit message / comment in the code).
...
> > > + /*
> > > + * NOTE: The lower half of the reset value is not sent, because sending
> > > + * it causes an error
> >
> > Why? Any root cause? Perhaps you need to send 0xffff ?
>
> Unknown, because I don't have the EC firmware for analysis. The vendor
> kernel sends 0xff00 and gets an error.
>
> Sending 0xffff doesn't help.
Maybe a slightly elaborated comment that it's copied from the vendor
kernel (which is?).
> > > + */
...
> > > + dev_info(ec->dev,
> > > + "Netronix embedded controller version %04x detected.\n",
> > > + version);
> >
> > This info level may confuse users if followed by an error path.
>
> Right. I suppose printing incompatible versions is still useful, so how
> about something like the following?
>
>
> /* Bail out if we encounter an unknown firmware version */
> switch (version) {
> case 0xd726: /* found in Kobo Aura */
> dev_info(ec->dev,
> "Netronix embedded controller version %04x detected.\n",
> version);
> break;
> default:
> dev_err(ec->dev,
> "Netronix embedded controller version %04x is not supported.\n",
> version);
> return -ENODEV;
> }
This is better.
...
> > > + WARN_ON(poweroff_restart_client);
> >
> > WARN_ON? All these alerts, WARNs, BUGs must be explained. Screaming to
> > the user is not good if it wasn't justified.
>
> poweroff_restart_client being already set is not a situation that should
> happen (and would indicate a bug in this driver, AFAICT), but I guess
> the log message could be better in that case...
As per above.
--
With Best Regards,
Andy Shevchenko
On Thu, 24 Sep 2020 21:24:50 +0200, Jonathan Neusch?fer wrote:
> This EC is found in e-book readers of multiple brands (e.g. Kobo,
> Tolino), and is typically implemented as a TI MSP430 microcontroller.
>
> It controls different functions of the system, such as power on/off,
> RTC, PWM for the backlight. The exact functionality provided can vary
> between boards.
>
> Signed-off-by: Jonathan Neusch?fer <[email protected]>
> ---
>
> v3:
> - Remove binding in text form patch description again
> - Add additionalProperties: false
> - Remove interrupt-controller property from example
> - Merge pwm/rtc nodes into main node
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Add the plaintext DT binding for comparison
> ---
> .../bindings/mfd/netronix,ntxec.yaml | 76 +++++++++++++++++++
> 1 file changed, 76 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/netronix,ntxec.yaml
>
Reviewed-by: Rob Herring <[email protected]>
On Tue, Sep 29, 2020 at 07:37:21PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 26, 2020 at 12:32 AM Jonathan Neuschäfer
> <[email protected]> wrote:
> > On Fri, Sep 25, 2020 at 12:29:45PM +0300, Andy Shevchenko wrote:
> > > On Thu, Sep 24, 2020 at 10:26 PM Jonathan Neuschäfer
> > > <[email protected]> wrote:
>
> ...
>
> > > > + dev_alert(&poweroff_restart_client->dev,
> > > > + "Failed to power off (err = %d)\n", res);
> > >
> > > alert? This needs to be explained.
> >
> > I copied the dev_alert from drivers/mfd/rn5t618.c.
> >
> > Upon reconsideration, I'm not sure what the correct log level would be,
> > but _warn seems enough, or maybe _err is better
>
> It's up to you to decide, but crit/alert and similar has to be
> justified (commit message / comment in the code).
Alright, thanks for explaining.
> > > > + /*
> > > > + * NOTE: The lower half of the reset value is not sent, because sending
> > > > + * it causes an error
> > >
> > > Why? Any root cause? Perhaps you need to send 0xffff ?
> >
> > Unknown, because I don't have the EC firmware for analysis. The vendor
> > kernel sends 0xff00 and gets an error.
> >
> > Sending 0xffff doesn't help.
>
> Maybe a slightly elaborated comment that it's copied from the vendor
> kernel (which is?).
Good idea, I'll do that.
> ...
>
> > > > + dev_info(ec->dev,
> > > > + "Netronix embedded controller version %04x detected.\n",
> > > > + version);
> > >
> > > This info level may confuse users if followed by an error path.
> >
> > Right. I suppose printing incompatible versions is still useful, so how
> > about something like the following?
> >
> >
> > /* Bail out if we encounter an unknown firmware version */
> > switch (version) {
> > case 0xd726: /* found in Kobo Aura */
> > dev_info(ec->dev,
> > "Netronix embedded controller version %04x detected.\n",
> > version);
> > break;
> > default:
> > dev_err(ec->dev,
> > "Netronix embedded controller version %04x is not supported.\n",
> > version);
> > return -ENODEV;
> > }
>
> This is better.
>
> ...
>
> > > > + WARN_ON(poweroff_restart_client);
> > >
> > > WARN_ON? All these alerts, WARNs, BUGs must be explained. Screaming to
> > > the user is not good if it wasn't justified.
> >
> > poweroff_restart_client being already set is not a situation that should
> > happen (and would indicate a bug in this driver, AFAICT), but I guess
> > the log message could be better in that case...
>
> As per above.
Okay, I'll rework the dev_alert/WARN_ON parts.
Thanks,
Jonathan Neuschäfer
On Mon, Sep 28, 2020 at 10:35:46AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
[...]
> > > > + if (state->enabled && duty != 0) {
> > > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + /* Disable the auto-off timer */
> > > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > > > + } else {
> > > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > > > + }
> > >
> > > This code is wrong for state->enabled = false.
> >
> > Why?
>
> Hm, I wonder the same. Probably I just misunderstood the code, sorry :-\
>
> > > How does the PWM behave when .apply is called? Does it complete the
> > > currently running period? Can it happen that when you switch from say
> > >
> > > .duty_cycle = 900 * TIME_BASE_NS (0x384)
> > > .period = 1800 * TIME_BASE_NS (0x708)
> > >
> > > to
> > >
> > > .duty_cycle = 300 * TIME_BASE_NS (0x12c)
> > > .period = 600 * TIME_BASE_NS (0x258)
> > >
> > > that a period with
> > >
> > > .duty_cycle = 388 * TIME_BASE_NS (0x184)
> > > .period = 1800 * TIME_BASE_NS (0x708)
> > >
> > > (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> > > started) or something similar is emitted?
> >
> > Changes take effect after the low byte is written, so a result like 0x184
> > in the above example should not happen.
> >
> > When the period and duty cycle are both changed, it temporarily results
> > in an inconsistent state:
> >
> > - period = 1800ns, duty cycle = 900ns
> > - period = 600ns, duty cycle = 900ns (!)
> > - period = 600ns, duty cycle = 300ns
>
> Does this always happen, or only if a new cycle starts at an unlucky
> moment?
Just based on thinking about the code, the register writes setting this
intermediate state would always happen, but I don't know if the state
changes are applied in the middle of a running period, or when the new
period starts, because I can't measure the signal in good enough detail
at the moment.
> > The inconsistent state of duty cycle > period is handled gracefully by
> > the EC and it outputs a 100% duty cycle, as far as I can tell.
>
> OK.
>
> > I currently don't have a logic analyzer / oscilloscope to measure
> > whether we get full PWM periods, or some kind of glitch when the new
> > period starts in the middle of the last one.
>
> You can even check this with an LED using something like:
>
> pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
> pwm_apply(mypwm, {.enabled = false, ... });
>
> . If the period is completed the LED is on for $big ns, if not the LED
> is on for a timespan that is probably hardly noticable with the human
> eye.
The longest configurable period is about 8ms, so it's not long enough to
see anything. However, after writing enable=0, it can take about a
second for the PWM signal to turn off... this hardware is a bit weird.
> > > > +}
> > > > +
> > > > +static struct pwm_ops ntxec_pwm_ops = {
> > > > + .apply = ntxec_pwm_apply,
> > >
> > > Please implement a .get_state() callback. And enable PWM_DEBUG during
> > > your tests.
> >
> > The device doesn't support reading back the PWM state. What should a
> > driver do in this case?
>
> Document it as a limitation, please.
Okay.
Thanks,
Jonathan Neuschäfer
On Thu, Sep 24, 2020 at 10:40:11PM +0200, Andreas Kemnade wrote:
> On Thu, 24 Sep 2020 21:24:53 +0200
> Jonathan Neuschäfer <[email protected]> wrote:
[...]
> > +static struct platform_driver ntxec_rtc_driver = {
> > + .driver = {
> > + .name = "ntxec-rtc",
> > + },
> > + .probe = ntxec_rtc_probe,
> > +};
> > +module_platform_driver(ntxec_rtc_driver);
> > +
> I think module autoloading will not work without
>
> MODULE_ALIAS("platform:ntxec-rtc");
>
> Same for the pwm device.
Right, I forgot that. Thanks for the reminder!
Jonathan
On Sat, Oct 3, 2020 at 2:36 AM Jonathan Neuschäfer
<[email protected]> wrote:
> On Mon, Sep 28, 2020 at 10:35:46AM +0200, Uwe Kleine-König wrote:
> > On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
...
> > You can even check this with an LED using something like:
> >
> > pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
> > pwm_apply(mypwm, {.enabled = false, ... });
> >
> > . If the period is completed the LED is on for $big ns, if not the LED
> > is on for a timespan that is probably hardly noticable with the human
> > eye.
>
> The longest configurable period is about 8ms, so it's not long enough to
> see anything. However, after writing enable=0, it can take about a
> second for the PWM signal to turn off... this hardware is a bit weird.
Sounds like you have 1/128 divisor and off/on is done on lower
frequency. (We saw PWMs with an additional 7-bit counter, reminds me
Crystal Cove PMIC PWM).
--
With Best Regards,
Andy Shevchenko
On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote:
> > +#define NTXEC_REG_WRITE_YEAR 0x10
> > +#define NTXEC_REG_WRITE_MONTH 0x11
> > +#define NTXEC_REG_WRITE_DAY 0x12
> > +#define NTXEC_REG_WRITE_HOUR 0x13
> > +#define NTXEC_REG_WRITE_MINUTE 0x14
> > +#define NTXEC_REG_WRITE_SECOND 0x15
> > +
> > +#define NTXEC_REG_READ_YM 0x20
> > +#define NTXEC_REG_READ_DH 0x21
> > +#define NTXEC_REG_READ_MS 0x23
>
> Is this an official naming? I think at least ..._MS is a poor name.
> Maybe consider ..._MINSEC instead and make the other two names a bit longer
> for consistency?
It's inofficial (the vendor kernel uses 0x10 etc. directly).
I'll pick longer names.
> > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > + unsigned int value;
> > + int res;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_year = (value >> 8) + 100;
> > + tm->tm_mon = (value & 0xff) - 1;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_mday = value >> 8;
> > + tm->tm_hour = value & 0xff;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_min = value >> 8;
> > + tm->tm_sec = value & 0xff;
> > +
> > + return 0;
> > +}
> > +
> > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > + int res = 0;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > + if (res)
> > + return res;
> > +
> > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
>
> I wonder: Is this racy? If you write minute, does the seconds reset to
> zero or something like that? Or can it happen, that after writing the
> minute register and before writing the second register the seconds
> overflow and you end up with the time set to a minute later than
> intended? If so it might be worth to set the seconds to 0 at the start
> of the function (with an explaining comment).
The setting the minutes does not reset the seconds, so I think this race
condition is possible. I'll add the workaround.
> .read_time has a similar race. What happens if minutes overflow between
> reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS?
Yes, we get read tearing in that case. It could even propagate all the
way to the year/month field, for example when the following time rolls
over:
A | B | C
2020-10-31 23:59:59
2020-11-01 00:00:00
- If the increment happens after reading C, we get 2020-10-31 23:59:59
- If the increment happens between reading B and C, we get 2020-10-31 23:00:00
- If the increment happens between reading A and B, we get 2020-10-01 00:00:00
- If the increment happens before reading A, we get 2020-11-01 00:00:00
... both of which are far from correct.
To mitigate this issue, I think something like the following is needed:
- Read year/month
- Read day/hour
- Read minute/second
- Read day/hour, compare with previously read value, restart on mismatch
- Read year/month, compare with previously read value, restart on mismatch
The order of the last two steps doesn't matter, as far as I can see, but
if I remove one of them, I can't catch all cases of read tearing.
> > +static struct platform_driver ntxec_rtc_driver = {
> > + .driver = {
> > + .name = "ntxec-rtc",
> > + },
> > + .probe = ntxec_rtc_probe,
>
> No .remove function?
I don't think it would serve a purpose in this driver. There are no
device-specific resources to release (no clocks to unprepare, for
example).
Thanks,
Jonathan Neuschäfer
On 04/10/2020 03:43:23+0200, Jonathan Neusch?fer wrote:
> > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > > + int res = 0;
> > > +
> > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> > > + if (res)
> > > + return res;
> > > +
> > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > > + if (res)
> > > + return res;
> > > +
> > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> >
> > I wonder: Is this racy? If you write minute, does the seconds reset to
> > zero or something like that? Or can it happen, that after writing the
> > minute register and before writing the second register the seconds
> > overflow and you end up with the time set to a minute later than
> > intended? If so it might be worth to set the seconds to 0 at the start
> > of the function (with an explaining comment).
>
> The setting the minutes does not reset the seconds, so I think this race
> condition is possible. I'll add the workaround.
>
Are you sure this happens? Usually, the seconds are not reset but the
internal 32768kHz counter is so you have a full second to write all the
registers.
> > .read_time has a similar race. What happens if minutes overflow between
> > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS?
>
> Yes, we get read tearing in that case. It could even propagate all the
> way to the year/month field, for example when the following time rolls
> over:
> A | B | C
> 2020-10-31 23:59:59
> 2020-11-01 00:00:00
>
> - If the increment happens after reading C, we get 2020-10-31 23:59:59
> - If the increment happens between reading B and C, we get 2020-10-31 23:00:00
> - If the increment happens between reading A and B, we get 2020-10-01 00:00:00
> - If the increment happens before reading A, we get 2020-11-01 00:00:00
>
> ... both of which are far from correct.
>
> To mitigate this issue, I think something like the following is needed:
>
> - Read year/month
> - Read day/hour
> - Read minute/second
> - Read day/hour, compare with previously read value, restart on mismatch
> - Read year/month, compare with previously read value, restart on mismatch
>
> The order of the last two steps doesn't matter, as far as I can see, but
> if I remove one of them, I can't catch all cases of read tearing.
>
Are you also sure this happens?
Only one comparison is necessary, the correct order would be:
- Read minute/second
- Read day/hour
- Read year/month
- Read minute/second, compare
If day/hour changes but not minute/second, it would mean that it took at
least an hour to read all the registers. At this point, I think you have
other problems and the exact time doesn't matter anymore.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello Jonathan,
On Sun, Oct 04, 2020 at 03:43:23AM +0200, Jonathan Neusch?fer wrote:
> On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-K?nig wrote:
> > > +static struct platform_driver ntxec_rtc_driver = {
> > > + .driver = {
> > > + .name = "ntxec-rtc",
> > > + },
> > > + .probe = ntxec_rtc_probe,
> >
> > No .remove function?
>
> I don't think it would serve a purpose in this driver. There are no
> device-specific resources to release (no clocks to unprepare, for
> example).
I had in mind that without a .remove callback the driver cannot detach.
but looking in the code (drivers/base/platform.c) this seems wrong.
So my concern can be considered void.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, 25 Sep 2020 at 07:10, Jonathan Neuschäfer <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
>
> v3:
> - Remove interrupt-controller property from embedded-controller node
> - subnodes of embedded-controller node in to the main node
>
> v2:
> - https://lore.kernel.org/lkml/[email protected]/
> - Fix pwm-cells property (should be 2, not 1)
> ---
> arch/arm/boot/dts/imx50-kobo-aura.dts | 17 ++++++++++++++++-
> 1 file changed, 16 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..2d1a59091a37c 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,24 @@ &i2c3 {
> pinctrl-0 = <&pinctrl_i2c3>;
> status = "okay";
>
> - /* TODO: embedded controller at 0x43 */
> + embedded-controller@43 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_ec>;
> + compatible = "netronix,ntxec";
> + reg = <0x43>;
> + system-power-controller;
> + interrupts-extended = <&gpio4 11 IRQ_TYPE_EDGE_FALLING>;
> + #pwm-cells = <2>;
> + };
> };
>
> &iomuxc {
> + pinctrl_ec: ec {
This should fail on dtschema check - pinctrl groups should end with "grp".
Best regards,
Krzysztof
On Sun, Oct 04, 2020 at 10:42:09AM +0200, Alexandre Belloni wrote:
> On 04/10/2020 03:43:23+0200, Jonathan Neuschäfer wrote:
> > > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
....
> > > > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
> > >
> > > I wonder: Is this racy? If you write minute, does the seconds reset to
> > > zero or something like that? Or can it happen, that after writing the
> > > minute register and before writing the second register the seconds
> > > overflow and you end up with the time set to a minute later than
> > > intended? If so it might be worth to set the seconds to 0 at the start
> > > of the function (with an explaining comment).
> >
> > The setting the minutes does not reset the seconds, so I think this race
> > condition is possible. I'll add the workaround.
> >
>
> Are you sure this happens? Usually, the seconds are not reset but the
> internal 32768kHz counter is so you have a full second to write all the
> registers.
I just checked it, and on this RTC, the phase / sub-second part is not
reset when the time is set.
> > > .read_time has a similar race. What happens if minutes overflow between
> > > reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS?
> >
> > Yes, we get read tearing in that case. It could even propagate all the
> > way to the year/month field, for example when the following time rolls
> > over:
> > A | B | C
> > 2020-10-31 23:59:59
> > 2020-11-01 00:00:00
> >
> > - If the increment happens after reading C, we get 2020-10-31 23:59:59
> > - If the increment happens between reading B and C, we get 2020-10-31 23:00:00
> > - If the increment happens between reading A and B, we get 2020-10-01 00:00:00
> > - If the increment happens before reading A, we get 2020-11-01 00:00:00
> >
> > ... both of which are far from correct.
> >
> > To mitigate this issue, I think something like the following is needed:
> >
> > - Read year/month
> > - Read day/hour
> > - Read minute/second
> > - Read day/hour, compare with previously read value, restart on mismatch
> > - Read year/month, compare with previously read value, restart on mismatch
> >
> > The order of the last two steps doesn't matter, as far as I can see, but
> > if I remove one of them, I can't catch all cases of read tearing.
> >
>
> Are you also sure this happens?
>
> Only one comparison is necessary, the correct order would be:
>
> - Read minute/second
> - Read day/hour
> - Read year/month
> - Read minute/second, compare
With this order, every one-second increment is detected, which I
previously tried to avoid. But I suppose it's fine because it simplifies
the logic and the window from first to last read should be short enough
anyway to be relatively unlikely to hit, and thus not cause a lot of retries.
> If day/hour changes but not minute/second, it would mean that it took at
> least an hour to read all the registers. At this point, I think you have
> other problems and the exact time doesn't matter anymore.
Indeed.
Thanks,
Jonathan Neuschäfer
On Wed, Oct 07, 2020 at 09:46:30AM +0200, Krzysztof Kozlowski wrote:
> On Fri, 25 Sep 2020 at 07:10, Jonathan Neuschäfer <[email protected]> wrote:
> >
> > Enable the Netronix EC on the Kobo Aura ebook reader.
...
> > &iomuxc {
> > + pinctrl_ec: ec {
>
> This should fail on dtschema check - pinctrl groups should end with "grp".
I missed that requirement. I think it's only stated in the i.MX pinctrl
bindings that have been converted to YAML.
I'll fix the names.
Thanks,
Jonathan Neuschäfer
On Thu, Sep 24, 2020 at 09:24:49PM +0200, Jonathan Neusch?fer wrote:
> Netronix, Inc. (http://www.netronixinc.com/) makes ebook reader board
> designs, which are for example used in Kobo and Tolino devices.
>
> An alternative prefix for Netronix would be "ntx", which is already used
> in code released by Netronix. It is shorter, but perhaps less clear.
>
> Signed-off-by: Jonathan Neusch?fer <[email protected]>
> Acked-by: Rob Herring <[email protected]>
minor nitpick: your S-o-b should be last in the tags area.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, 12 Oct 2020, Uwe Kleine-König wrote:
> On Thu, Sep 24, 2020 at 09:24:49PM +0200, Jonathan Neuschäfer wrote:
> > Netronix, Inc. (http://www.netronixinc.com/) makes ebook reader board
> > designs, which are for example used in Kobo and Tolino devices.
> >
> > An alternative prefix for Netronix would be "ntx", which is already used
> > in code released by Netronix. It is shorter, but perhaps less clear.
> >
> > Signed-off-by: Jonathan Neuschäfer <[email protected]>
> > Acked-by: Rob Herring <[email protected]>
>
> minor nitpick: your S-o-b should be last in the tags area.
I personally like to see tags in chronological order.
I doubt Rob Acked it before Jonathan originally signed it off.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog