[PATCH v8 0/9] Turris Omnia MCU driver

[PATCH v8 0/9] Turris Omnia MCU driver

[Marek Behún]

Hello Andy, Hans, Ilpo, Arnd, Gregory, and others,

this is v8 of the series adding Turris Omnia MCU driver.

This series depends on the immutable branch between LEDs and locking,
introducing devm_mutex_init(), see the PR
  https://lore.kernel.org/linux-leds/20240412084616.GR2399047@google.com/

See also cover letters for v1, v2, v3, v4, v5, v6 and v7:
  https://patchwork.kernel.org/project/linux-soc/cover/20230823161012.6986-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20230919103815.16818-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20231023143130.11602-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20231026161803.16750-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240323164359.21642-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240418121116.22184-1-kabel@kernel.org/
  https://patchwork.kernel.org/project/linux-soc/cover/20240424173809.7214-1-kabel@kernel.org/

Changes since v7:
- fixed wrong $id path in DT binding (patch 1)
- removed resource managed IRQ mapping disposal, which is not needed, as
  pointed out by Andy (patches 6, 7)
- added some more #includes (for linux/device.h, linux/interrupt.h,
  linux/hw_random.h) (patches 3, 6, 7)
- dropped the Fixes tags from the DT changes (patches 8, 9), with an
  explanation of this added into the commit message of patch 8, as
  suggested by Andrew

Marek Behún (9):
  dt-bindings: firmware: add cznic,turris-omnia-mcu binding
  platform: cznic: Add preliminary support for Turris Omnia MCU
  platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
  platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
  platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
  platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG
  platform: cznic: turris-omnia-mcu: Add support for digital message
    signing via debugfs
  ARM: dts: turris-omnia: Add MCU system-controller node
  ARM: dts: turris-omnia: Add GPIO key node for front button

 .../ABI/testing/debugfs-turris-omnia-mcu      |   13 +
 .../sysfs-bus-i2c-devices-turris-omnia-mcu    |  126 ++
 .../firmware/cznic,turris-omnia-mcu.yaml      |   86 ++
 MAINTAINERS                                   |    5 +
 .../dts/marvell/armada-385-turris-omnia.dts   |   35 +-
 drivers/platform/Kconfig                      |    2 +
 drivers/platform/Makefile                     |    1 +
 drivers/platform/cznic/Kconfig                |   51 +
 drivers/platform/cznic/Makefile               |    9 +
 .../platform/cznic/turris-omnia-mcu-base.c    |  439 +++++++
 .../platform/cznic/turris-omnia-mcu-debugfs.c |  207 ++++
 .../platform/cznic/turris-omnia-mcu-gpio.c    | 1048 +++++++++++++++++
 .../cznic/turris-omnia-mcu-sys-off-wakeup.c   |  258 ++++
 .../platform/cznic/turris-omnia-mcu-trng.c    |  101 ++
 .../cznic/turris-omnia-mcu-watchdog.c         |  123 ++
 drivers/platform/cznic/turris-omnia-mcu.h     |  188 +++
 include/linux/turris-omnia-mcu-interface.h    |  249 ++++
 17 files changed, 2940 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
 create mode 100644 Documentation/devicetree/bindings/firmware/cznic,turris-omnia-mcu.yaml
 create mode 100644 drivers/platform/cznic/Kconfig
 create mode 100644 drivers/platform/cznic/Makefile
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
 create mode 100644 include/linux/turris-omnia-mcu-interface.h

-- 
2.43.2

[PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

Add the basic skeleton for a new platform driver for the microcontroller
found on the Turris Omnia board.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 .../sysfs-bus-i2c-devices-turris-omnia-mcu    |  81 ++++
 MAINTAINERS                                   |   3 +
 drivers/platform/Kconfig                      |   2 +
 drivers/platform/Makefile                     |   1 +
 drivers/platform/cznic/Kconfig                |  26 ++
 drivers/platform/cznic/Makefile               |   4 +
 .../platform/cznic/turris-omnia-mcu-base.c    | 376 ++++++++++++++++++
 drivers/platform/cznic/turris-omnia-mcu.h     |  50 +++
 include/linux/turris-omnia-mcu-interface.h    | 249 ++++++++++++
 9 files changed, 792 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
 create mode 100644 drivers/platform/cznic/Kconfig
 create mode 100644 drivers/platform/cznic/Makefile
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
 create mode 100644 include/linux/turris-omnia-mcu-interface.h

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
new file mode 100644
index 000000000000..ff5e7cb00206
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -0,0 +1,81 @@
+What:		/sys/bus/i2c/devices/<mcu_device>/board_revision
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains board revision number.
+
+		Only available if board information is burned in the MCU (older
+		revisions have board information burned in the ATSHA204-A chip).
+
+		Format: %u.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/first_mac_address
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains device first MAC address. Each Turris Omnia is
+		allocated 3 MAC addresses. The two additional addresses are
+		computed from the first one by incrementing it.
+
+		Only available if board information is burned in the MCU (older
+		revisions have board information burned in the ATSHA204-A chip).
+
+		Format: %pM.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/fw_features
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Newer versions of the microcontroller firmware report the
+		features they support. These can be read from this file. If the
+		MCU firmware is too old, this file reads 0x0.
+
+		Format: 0x%x.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/fw_version_hash_application
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains the version hash (commit hash) of the application
+		part of the microcontroller firmware.
+
+		Format: %s.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/fw_version_hash_bootloader
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains the version hash (commit hash) of the bootloader
+		part of the microcontroller firmware.
+
+		Format: %s.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/mcu_type
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains the microcontroller type (STM32, GD32, MKL).
+
+		Format: %s.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/reset_selector
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains the selected factory reset level, determined by
+		how long the rear reset button was held by the user during board
+		reset.
+
+		Format: %i.
+
+What:		/sys/bus/i2c/devices/<mcu_device>/serial_number
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RO) Contains the 64 bit long board serial number in hexadecimal
+		format.
+
+		Only available if board information is burned in the MCU (older
+		revisions have board information burned in the ATSHA204-A chip).
+
+		Format: %016X.
diff --git a/MAINTAINERS b/MAINTAINERS
index 359ed2cadfc4..2aa178a9d53e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2139,6 +2139,7 @@ M:	Marek Behún <kabel@kernel.org>
 S:	Maintained
 W:	https://www.turris.cz/
 F:	Documentation/ABI/testing/debugfs-moxtet
+F:	Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
 F:	Documentation/ABI/testing/sysfs-bus-moxtet-devices
 F:	Documentation/ABI/testing/sysfs-firmware-turris-mox-rwtm
 F:	Documentation/devicetree/bindings/bus/moxtet.txt
@@ -2152,10 +2153,12 @@ F:	drivers/firmware/turris-mox-rwtm.c
 F:	drivers/gpio/gpio-moxtet.c
 F:	drivers/leds/leds-turris-omnia.c
 F:	drivers/mailbox/armada-37xx-rwtm-mailbox.c
+F:	drivers/platform/cznic/
 F:	drivers/watchdog/armada_37xx_wdt.c
 F:	include/dt-bindings/bus/moxtet.h
 F:	include/linux/armada-37xx-rwtm-mailbox.h
 F:	include/linux/moxtet.h
+F:	include/linux/turris-omnia-mcu-interface.h
 
 ARM/FARADAY FA526 PORT
 M:	Hans Ulli Kroll <ulli.kroll@googlemail.com>
diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index 868b20361769..fef907a94001 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -7,6 +7,8 @@ source "drivers/platform/goldfish/Kconfig"
 
 source "drivers/platform/chrome/Kconfig"
 
+source "drivers/platform/cznic/Kconfig"
+
 source "drivers/platform/mellanox/Kconfig"
 
 source "drivers/platform/olpc/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 41640172975a..8bf189264374 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_MIPS)		+= mips/
 obj-$(CONFIG_OLPC_EC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
 obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
+obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
new file mode 100644
index 000000000000..f8560ff9c1af
--- /dev/null
+++ b/drivers/platform/cznic/Kconfig
@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# For a description of the syntax of this configuration file,
+# see Documentation/kbuild/kconfig-language.rst.
+#
+
+menuconfig CZNIC_PLATFORMS
+	bool "Platform support for CZ.NIC's Turris hardware"
+	depends on MACH_ARMADA_38X || COMPILE_TEST
+	help
+	  Say Y here to be able to choose driver support for CZ.NIC's Turris
+	  devices. This option alone does not add any kernel code.
+
+if CZNIC_PLATFORMS
+
+config TURRIS_OMNIA_MCU
+	tristate "Turris Omnia MCU driver"
+	depends on MACH_ARMADA_38X || COMPILE_TEST
+	depends on I2C
+	help
+	  Say Y here to add support for the features implemented by the
+	  microcontroller on the CZ.NIC's Turris Omnia SOHO router.
+	  To compile this driver as a module, choose M here; the module will be
+	  called turris-omnia-mcu.
+
+endif # CZNIC_PLATFORMS
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
new file mode 100644
index 000000000000..31adca73bb94
--- /dev/null
+++ b/drivers/platform/cznic/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
+turris-omnia-mcu-y		:= turris-omnia-mcu-base.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
new file mode 100644
index 000000000000..8893c990853a
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/hex.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+#include "turris-omnia-mcu.h"
+
+#define OMNIA_FW_VERSION_LEN		20
+#define OMNIA_FW_VERSION_HEX_LEN	(2 * OMNIA_FW_VERSION_LEN + 1)
+#define OMNIA_BOARD_INFO_LEN		16
+
+int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
+		   unsigned int len)
+{
+	struct i2c_msg msgs[2];
+	int ret;
+
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].len = 1;
+	msgs[0].buf = &cmd;
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].len = len;
+	msgs[1].buf = reply;
+
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0)
+		return ret;
+	if (ret != ARRAY_SIZE(msgs))
+		return -EIO;
+
+	return 0;
+}
+
+static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
+				  u8 version[static OMNIA_FW_VERSION_HEX_LEN])
+{
+	u8 reply[OMNIA_FW_VERSION_LEN];
+	int err;
+
+	err = omnia_cmd_read(mcu->client,
+			     bootloader ? CMD_GET_FW_VERSION_BOOT
+					: CMD_GET_FW_VERSION_APP,
+			     reply, sizeof(reply));
+	if (err)
+		return err;
+
+	version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
+	bin2hex(version, reply, OMNIA_FW_VERSION_LEN);
+
+	return 0;
+}
+
+static ssize_t fw_version_hash_show(struct device *dev, char *buf,
+				    bool bootloader)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+	u8 version[OMNIA_FW_VERSION_HEX_LEN];
+	int err;
+
+	err = omnia_get_version_hash(mcu, bootloader, version);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%s\n", version);
+}
+
+static ssize_t fw_version_hash_application_show(struct device *dev,
+						struct device_attribute *a,
+						char *buf)
+{
+	return fw_version_hash_show(dev, buf, false);
+}
+static DEVICE_ATTR_RO(fw_version_hash_application);
+
+static ssize_t fw_version_hash_bootloader_show(struct device *dev,
+					       struct device_attribute *a,
+					       char *buf)
+{
+	return fw_version_hash_show(dev, buf, true);
+}
+static DEVICE_ATTR_RO(fw_version_hash_bootloader);
+
+static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
+				char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "0x%x\n", mcu->features);
+}
+static DEVICE_ATTR_RO(fw_features);
+
+static ssize_t mcu_type_show(struct device *dev, struct device_attribute *a,
+			     char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%s\n", mcu->type);
+}
+static DEVICE_ATTR_RO(mcu_type);
+
+static ssize_t reset_selector_show(struct device *dev,
+				   struct device_attribute *a, char *buf)
+{
+	int ret;
+
+	ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", ret);
+}
+static DEVICE_ATTR_RO(reset_selector);
+
+static ssize_t serial_number_show(struct device *dev,
+				  struct device_attribute *a, char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%016llX\n", mcu->board_serial_number);
+}
+static DEVICE_ATTR_RO(serial_number);
+
+static ssize_t first_mac_address_show(struct device *dev,
+				      struct device_attribute *a, char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%pM\n", mcu->board_first_mac);
+}
+static DEVICE_ATTR_RO(first_mac_address);
+
+static ssize_t board_revision_show(struct device *dev,
+				   struct device_attribute *a, char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%u\n", mcu->board_revision);
+}
+static DEVICE_ATTR_RO(board_revision);
+
+static struct attribute *omnia_mcu_base_attrs[] = {
+	&dev_attr_fw_version_hash_application.attr,
+	&dev_attr_fw_version_hash_bootloader.attr,
+	&dev_attr_fw_features.attr,
+	&dev_attr_mcu_type.attr,
+	&dev_attr_reset_selector.attr,
+	&dev_attr_serial_number.attr,
+	&dev_attr_first_mac_address.attr,
+	&dev_attr_board_revision.attr,
+	NULL
+};
+
+static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
+					    struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+	umode_t mode = a->mode;
+
+	if ((a == &dev_attr_serial_number.attr ||
+	     a == &dev_attr_first_mac_address.attr ||
+	     a == &dev_attr_board_revision.attr) &&
+	    !(mcu->features & FEAT_BOARD_INFO))
+		mode = 0;
+
+	return mode;
+}
+
+static const struct attribute_group omnia_mcu_base_group = {
+	.attrs = omnia_mcu_base_attrs,
+	.is_visible = omnia_mcu_base_attrs_visible,
+};
+
+static const struct attribute_group *omnia_mcu_groups[] = {
+	&omnia_mcu_base_group,
+	NULL
+};
+
+static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
+{
+	const char *type = bootloader ? "bootloader" : "application";
+	struct device *dev = &mcu->client->dev;
+	u8 version[OMNIA_FW_VERSION_HEX_LEN];
+	int err;
+
+	err = omnia_get_version_hash(mcu, bootloader, version);
+	if (err) {
+		dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
+			err);
+		return;
+	}
+
+	dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
+}
+
+static const char *omnia_status_to_mcu_type(uint16_t status)
+{
+	switch (status & STS_MCU_TYPE_MASK) {
+	case STS_MCU_TYPE_STM32:
+		return "STM32";
+	case STS_MCU_TYPE_GD32:
+		return "GD32";
+	case STS_MCU_TYPE_MKL:
+		return "MKL";
+	default:
+		return "unknown";
+	}
+}
+
+static void omnia_info_missing_feature(struct device *dev, const char *feature)
+{
+	dev_info(dev,
+		 "Your board's MCU firmware does not support the %s feature.\n",
+		 feature);
+}
+
+static int omnia_mcu_read_features(struct omnia_mcu *mcu)
+{
+	static const struct {
+		uint16_t mask;
+		const char *name;
+	} features[] = {
+		{ FEAT_EXT_CMDS,	   "extended control and status" },
+		{ FEAT_WDT_PING,	   "watchdog pinging" },
+		{ FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
+		{ FEAT_NEW_INT_API,	   "new interrupt API" },
+		{ FEAT_POWEROFF_WAKEUP,	   "poweroff and wakeup" },
+		{ FEAT_TRNG,		   "true random number generator" },
+	};
+	struct device *dev = &mcu->client->dev;
+	bool suggest_fw_upgrade = false;
+	int status;
+
+	/* status word holds MCU type, which we need below */
+	status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
+	if (status < 0)
+		return status;
+
+	/* check whether MCU firmware supports the CMD_GET_FEAUTRES command */
+	if (status & STS_FEATURES_SUPPORTED) {
+		__le32 reply;
+		int ret;
+
+		/* try read 32-bit features */
+		ret = omnia_cmd_read(mcu->client, CMD_GET_FEATURES, &reply,
+				     sizeof(reply));
+		if (ret) {
+			/* try read 16-bit features */
+			ret = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES);
+			if (ret < 0)
+				return ret;
+
+			mcu->features = ret;
+		} else {
+			mcu->features = le32_to_cpu(reply);
+
+			if (mcu->features & FEAT_FROM_BIT_16_INVALID)
+				mcu->features &= GENMASK(15, 0);
+		}
+	} else {
+		omnia_info_missing_feature(dev, "feature reading");
+		suggest_fw_upgrade = true;
+	}
+
+	mcu->type = omnia_status_to_mcu_type(status);
+	dev_info(dev, "MCU type %s%s\n", mcu->type,
+		 (mcu->features & FEAT_PERIPH_MCU) ?
+			", with peripheral resets wired" : "");
+
+	omnia_mcu_print_version_hash(mcu, true);
+
+	if (mcu->features & FEAT_BOOTLOADER)
+		dev_warn(dev,
+			 "MCU is running bootloader firmware. Was firmware upgrade interrupted?\n");
+	else
+		omnia_mcu_print_version_hash(mcu, false);
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(features); i++) {
+		if (mcu->features & features[i].mask)
+			continue;
+
+		omnia_info_missing_feature(dev, features[i].name);
+		suggest_fw_upgrade = true;
+	}
+
+	if (suggest_fw_upgrade)
+		dev_info(dev,
+			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
+
+	return 0;
+}
+
+static int omnia_mcu_read_board_info(struct omnia_mcu *mcu)
+{
+	u8 reply[1 + OMNIA_BOARD_INFO_LEN];
+	int err;
+
+	err = omnia_cmd_read(mcu->client, CMD_BOARD_INFO_GET, reply,
+			     sizeof(reply));
+	if (err)
+		return err;
+
+	if (reply[0] != OMNIA_BOARD_INFO_LEN)
+		return -EIO;
+
+	mcu->board_serial_number = get_unaligned_le64(&reply[1]);
+	memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);
+	mcu->board_revision = reply[15];
+
+	return 0;
+}
+
+static int omnia_mcu_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct omnia_mcu *mcu;
+	int err;
+
+	if (!client->irq)
+		return dev_err_probe(dev, -EINVAL, "IRQ resource not found\n");
+
+	mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
+	if (!mcu)
+		return -ENOMEM;
+
+	mcu->client = client;
+	i2c_set_clientdata(client, mcu);
+
+	err = omnia_mcu_read_features(mcu);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot determine MCU supported features\n");
+
+	if (mcu->features & FEAT_BOARD_INFO) {
+		err = omnia_mcu_read_board_info(mcu);
+		if (err)
+			return dev_err_probe(dev, err,
+					     "Cannot read board info\n");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_omnia_mcu_match[] = {
+	{ .compatible = "cznic,turris-omnia-mcu" },
+	{}
+};
+
+static struct i2c_driver omnia_mcu_driver = {
+	.probe		= omnia_mcu_probe,
+	.driver		= {
+		.name	= "turris-omnia-mcu",
+		.of_match_table = of_omnia_mcu_match,
+		.dev_groups = omnia_mcu_groups,
+	},
+};
+module_i2c_driver(omnia_mcu_driver);
+
+MODULE_AUTHOR("Marek Behun <kabel@kernel.org>");
+MODULE_DESCRIPTION("CZ.NIC's Turris Omnia MCU");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
new file mode 100644
index 000000000000..5b21fbca8fe1
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CZ.NIC's Turris Omnia MCU driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#ifndef __TURRIS_OMNIA_MCU_H
+#define __TURRIS_OMNIA_MCU_H
+
+#include <linux/i2c.h>
+#include <linux/if_ether.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+struct omnia_mcu {
+	struct i2c_client *client;
+	const char *type;
+	u32 features;
+
+	/* board information */
+	u64 board_serial_number;
+	u8 board_first_mac[ETH_ALEN];
+	u8 board_revision;
+};
+
+int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
+		   unsigned int len);
+
+static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
+{
+	__le16 reply;
+	int err;
+
+	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+	return err ?: le16_to_cpu(reply);
+}
+
+static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
+{
+	u8 reply;
+	int err;
+
+	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+	return err ?: reply;
+}
+
+#endif /* __TURRIS_OMNIA_MCU_H */
diff --git a/include/linux/turris-omnia-mcu-interface.h b/include/linux/turris-omnia-mcu-interface.h
new file mode 100644
index 000000000000..88f8490b5897
--- /dev/null
+++ b/include/linux/turris-omnia-mcu-interface.h
@@ -0,0 +1,249 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CZ.NIC's Turris Omnia MCU I2C interface commands definitions
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H
+#define __TURRIS_OMNIA_MCU_INTERFACE_H
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+
+enum omnia_commands_e {
+	CMD_GET_STATUS_WORD		= 0x01, /* slave sends status word back */
+	CMD_GENERAL_CONTROL		= 0x02,
+	CMD_LED_MODE			= 0x03, /* default/user */
+	CMD_LED_STATE			= 0x04, /* LED on/off */
+	CMD_LED_COLOR			= 0x05, /* LED number + RED + GREEN + BLUE */
+	CMD_USER_VOLTAGE		= 0x06,
+	CMD_SET_BRIGHTNESS		= 0x07,
+	CMD_GET_BRIGHTNESS		= 0x08,
+	CMD_GET_RESET			= 0x09,
+	CMD_GET_FW_VERSION_APP		= 0x0A, /* 20B git hash number */
+	CMD_SET_WATCHDOG_STATE		= 0x0B, /* 0 - disable
+						 * 1 - enable / ping
+						 * after boot watchdog is started
+						 * with 2 minutes timeout
+						 */
+
+	/* CMD_WATCHDOG_STATUS		= 0x0C, not implemented anymore */
+
+	CMD_GET_WATCHDOG_STATE		= 0x0D,
+	CMD_GET_FW_VERSION_BOOT		= 0x0E, /* 20B git hash number */
+	CMD_GET_FW_CHECKSUM		= 0x0F, /* 4B length, 4B checksum */
+
+	/* available if FEATURES_SUPPORTED bit set in status word */
+	CMD_GET_FEATURES		= 0x10,
+
+	/* available if EXT_CMD bit set in features */
+	CMD_GET_EXT_STATUS_DWORD	= 0x11,
+	CMD_EXT_CONTROL			= 0x12,
+	CMD_GET_EXT_CONTROL_STATUS	= 0x13,
+
+	/* available if NEW_INT_API bit set in features */
+	CMD_GET_INT_AND_CLEAR		= 0x14,
+	CMD_GET_INT_MASK		= 0x15,
+	CMD_SET_INT_MASK		= 0x16,
+
+	/* available if FLASHING bit set in features */
+	CMD_FLASH			= 0x19,
+
+	/* available if WDT_PING bit set in features */
+	CMD_SET_WDT_TIMEOUT		= 0x20,
+	CMD_GET_WDT_TIMELEFT		= 0x21,
+
+	/* available if POWEROFF_WAKEUP bit set in features */
+	CMD_SET_WAKEUP			= 0x22,
+	CMD_GET_UPTIME_AND_WAKEUP	= 0x23,
+	CMD_POWER_OFF			= 0x24,
+
+	/* available if USB_OVC_PROT_SETTING bit set in features */
+	CMD_SET_USB_OVC_PROT		= 0x25,
+	CMD_GET_USB_OVC_PROT		= 0x26,
+
+	/* available if TRNG bit set in features */
+	CMD_TRNG_COLLECT_ENTROPY	= 0x28,
+
+	/* available if CRYPTO bit set in features */
+	CMD_CRYPTO_GET_PUBLIC_KEY	= 0x29,
+	CMD_CRYPTO_SIGN_MESSAGE		= 0x2A,
+	CMD_CRYPTO_COLLECT_SIGNATURE	= 0x2B,
+
+	/* available if BOARD_INFO it set in features */
+	CMD_BOARD_INFO_GET		= 0x2C,
+	CMD_BOARD_INFO_BURN		= 0x2D,
+
+	/* available only at address 0x2b (led-controller) */
+	/* available only if LED_GAMMA_CORRECTION bit set in features */
+	CMD_SET_GAMMA_CORRECTION	= 0x30,
+	CMD_GET_GAMMA_CORRECTION	= 0x31,
+
+	/* available only at address 0x2b (led-controller) */
+	/* available only if PER_LED_CORRECTION bit set in features */
+	/* available only if FROM_BIT_16_INVALID bit NOT set in features */
+	CMD_SET_LED_CORRECTIONS		= 0x32,
+	CMD_GET_LED_CORRECTIONS		= 0x33,
+};
+
+enum omnia_flashing_commands_e {
+	FLASH_CMD_UNLOCK		= 0x01,
+	FLASH_CMD_SIZE_AND_CSUM		= 0x02,
+	FLASH_CMD_PROGRAM		= 0x03,
+	FLASH_CMD_RESET			= 0x04,
+};
+
+enum omnia_sts_word_e {
+	STS_MCU_TYPE_MASK			= GENMASK(1, 0),
+	STS_MCU_TYPE_STM32			= 0 << 0,
+	STS_MCU_TYPE_GD32			= 1 << 0,
+	STS_MCU_TYPE_MKL			= 2 << 0,
+	STS_FEATURES_SUPPORTED			= BIT(2),
+	STS_USER_REGULATOR_NOT_SUPPORTED	= BIT(3),
+	STS_CARD_DET				= BIT(4),
+	STS_MSATA_IND				= BIT(5),
+	STS_USB30_OVC				= BIT(6),
+	STS_USB31_OVC				= BIT(7),
+	STS_USB30_PWRON				= BIT(8),
+	STS_USB31_PWRON				= BIT(9),
+	STS_ENABLE_4V5				= BIT(10),
+	STS_BUTTON_MODE				= BIT(11),
+	STS_BUTTON_PRESSED			= BIT(12),
+	STS_BUTTON_COUNTER_MASK			= GENMASK(15, 13)
+};
+
+enum omnia_ctl_byte_e {
+	CTL_LIGHT_RST		= BIT(0),
+	CTL_HARD_RST		= BIT(1),
+	/* BIT(2) is currently reserved */
+	CTL_USB30_PWRON		= BIT(3),
+	CTL_USB31_PWRON		= BIT(4),
+	CTL_ENABLE_4V5		= BIT(5),
+	CTL_BUTTON_MODE		= BIT(6),
+	CTL_BOOTLOADER		= BIT(7)
+};
+
+enum omnia_features_e {
+	FEAT_PERIPH_MCU			= BIT(0),
+	FEAT_EXT_CMDS			= BIT(1),
+	FEAT_WDT_PING			= BIT(2),
+	FEAT_LED_STATE_EXT_MASK		= GENMASK(4, 3),
+	FEAT_LED_STATE_EXT		= 1 << 3,
+	FEAT_LED_STATE_EXT_V32		= 2 << 3,
+	FEAT_LED_GAMMA_CORRECTION	= BIT(5),
+	FEAT_NEW_INT_API		= BIT(6),
+	FEAT_BOOTLOADER			= BIT(7),
+	FEAT_FLASHING			= BIT(8),
+	FEAT_NEW_MESSAGE_API		= BIT(9),
+	FEAT_BRIGHTNESS_INT		= BIT(10),
+	FEAT_POWEROFF_WAKEUP		= BIT(11),
+	FEAT_CAN_OLD_MESSAGE_API	= BIT(12),
+	FEAT_TRNG			= BIT(13),
+	FEAT_CRYPTO			= BIT(14),
+	FEAT_BOARD_INFO			= BIT(15),
+
+	/*
+	 * Orginally the features command replied only 16 bits. If more were
+	 * read, either the I2C transaction failed or 0xff bytes were sent.
+	 * Therefore to consider bits 16 - 31 valid, one bit (20) was reserved
+	 * to be zero.
+	 */
+
+	/* Bits 16 - 19 correspond to bits 0 - 3 of status word */
+	FEAT_MCU_TYPE_MASK		= GENMASK(17, 16),
+	FEAT_MCU_TYPE_STM32		= 0 << 16,
+	FEAT_MCU_TYPE_GD32		= 1 << 16,
+	FEAT_MCU_TYPE_MKL		= 2 << 16,
+	FEAT_FEATURES_SUPPORTED		= BIT(18),
+	FEAT_USER_REGULATOR_NOT_SUPPORTED = BIT(19),
+
+	/* must not be set */
+	FEAT_FROM_BIT_16_INVALID	= BIT(20),
+
+	FEAT_PER_LED_CORRECTION		= BIT(21),
+	FEAT_USB_OVC_PROT_SETTING	= BIT(22),
+};
+
+enum omnia_ext_sts_dword_e {
+	EXT_STS_SFP_nDET		= BIT(0),
+	EXT_STS_LED_STATES_MASK		= GENMASK(31, 12),
+	EXT_STS_WLAN0_MSATA_LED		= BIT(12),
+	EXT_STS_WLAN1_LED		= BIT(13),
+	EXT_STS_WLAN2_LED		= BIT(14),
+	EXT_STS_WPAN0_LED		= BIT(15),
+	EXT_STS_WPAN1_LED		= BIT(16),
+	EXT_STS_WPAN2_LED		= BIT(17),
+	EXT_STS_WAN_LED0		= BIT(18),
+	EXT_STS_WAN_LED1		= BIT(19),
+	EXT_STS_LAN0_LED0		= BIT(20),
+	EXT_STS_LAN0_LED1		= BIT(21),
+	EXT_STS_LAN1_LED0		= BIT(22),
+	EXT_STS_LAN1_LED1		= BIT(23),
+	EXT_STS_LAN2_LED0		= BIT(24),
+	EXT_STS_LAN2_LED1		= BIT(25),
+	EXT_STS_LAN3_LED0		= BIT(26),
+	EXT_STS_LAN3_LED1		= BIT(27),
+	EXT_STS_LAN4_LED0		= BIT(28),
+	EXT_STS_LAN4_LED1		= BIT(29),
+	EXT_STS_LAN5_LED0		= BIT(30),
+	EXT_STS_LAN5_LED1		= BIT(31),
+};
+
+enum omnia_ext_ctl_e {
+	EXT_CTL_nRES_MMC		= BIT(0),
+	EXT_CTL_nRES_LAN		= BIT(1),
+	EXT_CTL_nRES_PHY		= BIT(2),
+	EXT_CTL_nPERST0			= BIT(3),
+	EXT_CTL_nPERST1			= BIT(4),
+	EXT_CTL_nPERST2			= BIT(5),
+	EXT_CTL_PHY_SFP			= BIT(6),
+	EXT_CTL_PHY_SFP_AUTO		= BIT(7),
+	EXT_CTL_nVHV_CTRL		= BIT(8),
+};
+
+enum omnia_int_e {
+	INT_CARD_DET		= BIT(0),
+	INT_MSATA_IND		= BIT(1),
+	INT_USB30_OVC		= BIT(2),
+	INT_USB31_OVC		= BIT(3),
+	INT_BUTTON_PRESSED	= BIT(4),
+	INT_SFP_nDET		= BIT(5),
+	INT_BRIGHTNESS_CHANGED	= BIT(6),
+	INT_TRNG		= BIT(7),
+	INT_MESSAGE_SIGNED	= BIT(8),
+
+	INT_LED_STATES_MASK	= GENMASK(31, 12),
+	INT_WLAN0_MSATA_LED	= BIT(12),
+	INT_WLAN1_LED		= BIT(13),
+	INT_WLAN2_LED		= BIT(14),
+	INT_WPAN0_LED		= BIT(15),
+	INT_WPAN1_LED		= BIT(16),
+	INT_WPAN2_LED		= BIT(17),
+	INT_WAN_LED0		= BIT(18),
+	INT_WAN_LED1		= BIT(19),
+	INT_LAN0_LED0		= BIT(20),
+	INT_LAN0_LED1		= BIT(21),
+	INT_LAN1_LED0		= BIT(22),
+	INT_LAN1_LED1		= BIT(23),
+	INT_LAN2_LED0		= BIT(24),
+	INT_LAN2_LED1		= BIT(25),
+	INT_LAN3_LED0		= BIT(26),
+	INT_LAN3_LED1		= BIT(27),
+	INT_LAN4_LED0		= BIT(28),
+	INT_LAN4_LED1		= BIT(29),
+	INT_LAN5_LED0		= BIT(30),
+	INT_LAN5_LED1		= BIT(31),
+};
+
+enum omnia_cmd_poweroff_e {
+	CMD_POWER_OFF_POWERON_BUTTON	= BIT(0),
+	CMD_POWER_OFF_MAGIC		= 0xdead,
+};
+
+enum cmd_usb_ovc_prot_e {
+	CMD_xET_USB_OVC_PROT_PORT_MASK	= GENMASK(3, 0),
+	CMD_xET_USB_OVC_PROT_ENABLE	= BIT(4),
+};
+
+#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */
-- 
2.43.2

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> Add the basic skeleton for a new platform driver for the microcontroller
> found on the Turris Omnia board.

...

> +What:          /sys/bus/i2c/devices/<mcu_device>/serial_number
> +Date:          July 2024
> +KernelVersion: 6.10
> +Contact:       Marek Behún <kabel@kernel.org>
> +Description:   (RO) Contains the 64 bit long board serial number in hexadecimal

64 bit long --> 64-bit

> +               format.
> +
> +               Only available if board information is burned in the MCU (older
> +               revisions have board information burned in the ATSHA204-A chip).
> +
> +               Format: %016X.

It's strange to use capitalized hexadecimal here and not in other
files, but maybe it's something special about "serial number"? Dunno.

...

> +menuconfig CZNIC_PLATFORMS
> +       bool "Platform support for CZ.NIC's Turris hardware"

> +       depends on MACH_ARMADA_38X || COMPILE_TEST

This...

> +       help
> +         Say Y here to be able to choose driver support for CZ.NIC's Turris
> +         devices. This option alone does not add any kernel code.
> +
> +if CZNIC_PLATFORMS
> +
> +config TURRIS_OMNIA_MCU
> +       tristate "Turris Omnia MCU driver"

> +       depends on MACH_ARMADA_38X || COMPILE_TEST

...or this dependency is redundant. I think one would expect that
these platforms will not be always based on the same platform, hence I
would drop the former and leave the latter. But you should know better
than me.

> +       depends on I2C
> +       help
> +         Say Y here to add support for the features implemented by the
> +         microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> +         To compile this driver as a module, choose M here; the module will be
> +         called turris-omnia-mcu.

...

> +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])

Interesting format of the last parameter. Does it make any difference
to the compiler if you use u8 *version?

> +{
> +       u8 reply[OMNIA_FW_VERSION_LEN];
> +       int err;
> +
> +       err = omnia_cmd_read(mcu->client,
> +                            bootloader ? CMD_GET_FW_VERSION_BOOT
> +                                       : CMD_GET_FW_VERSION_APP,
> +                            reply, sizeof(reply));
> +       if (err)
> +               return err;

> +       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> +       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);

Hmm... I would rather use returned value

char *p;

p = bin2hex(...);
*p = '\0';

return 0;

> +       return 0;
> +}

...

> +static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
> +                                           struct attribute *a, int n)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct omnia_mcu *mcu = dev_get_drvdata(dev);

> +       umode_t mode = a->mode;

Do you need this?

> +       if ((a == &dev_attr_serial_number.attr ||
> +            a == &dev_attr_first_mac_address.attr ||
> +            a == &dev_attr_board_revision.attr) &&
> +           !(mcu->features & FEAT_BOARD_INFO))

> +               mode = 0;

return 0; ?

> +       return mode;

return a->mode; ?

> +}

...

> +static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
> +{
> +       const char *type = bootloader ? "bootloader" : "application";
> +       struct device *dev = &mcu->client->dev;
> +       u8 version[OMNIA_FW_VERSION_HEX_LEN];
> +       int err;
> +
> +       err = omnia_get_version_hash(mcu, bootloader, version);
> +       if (err) {
> +               dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> +                       err);

One  line?

> +               return;
> +       }
> +
> +       dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
> +}

...

> +static const char *omnia_status_to_mcu_type(uint16_t status)

Why out of a sudden uint16_t instead of u16?

> +{
> +       switch (status & STS_MCU_TYPE_MASK) {
> +       case STS_MCU_TYPE_STM32:
> +               return "STM32";
> +       case STS_MCU_TYPE_GD32:
> +               return "GD32";
> +       case STS_MCU_TYPE_MKL:
> +               return "MKL";
> +       default:
> +               return "unknown";
> +       }
> +}

...

> +       static const struct {
> +               uint16_t mask;

Ditto.

> +               const char *name;
> +       } features[] = {
> +               { FEAT_EXT_CMDS,           "extended control and status" },
> +               { FEAT_WDT_PING,           "watchdog pinging" },
> +               { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
> +               { FEAT_NEW_INT_API,        "new interrupt API" },
> +               { FEAT_POWEROFF_WAKEUP,    "poweroff and wakeup" },
> +               { FEAT_TRNG,               "true random number generator" },
> +       };

...

> +               omnia_info_missing_feature(dev, "feature reading");

Tautology. Read the final message. I believe you wanted to pass just
"reading" here.

...

> +       memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);

There is an API ether_copy_addr() or so, don't remember by heart.
You also need an include for ETH_ALEN definition.

...

> +#include <linux/i2c.h>

No users of this, you may replace with

struct i2c_client;

Am I right?

...

> +       CMD_GET_FW_VERSION_BOOT         = 0x0E, /* 20B git hash number */

Git

...

> +       /* available only at address 0x2b (led-controller) */

LED-controller

...

> +enum omnia_ctl_byte_e {
> +       CTL_LIGHT_RST           = BIT(0),
> +       CTL_HARD_RST            = BIT(1),
> +       /* BIT(2) is currently reserved */
> +       CTL_USB30_PWRON         = BIT(3),
> +       CTL_USB31_PWRON         = BIT(4),
> +       CTL_ENABLE_4V5          = BIT(5),
> +       CTL_BUTTON_MODE         = BIT(6),
> +       CTL_BOOTLOADER          = BIT(7)

Keep trailing comma as it might be extended (theoretically). And you
do the similar in other enums anyway.

> +};

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

On Tue, 30 Apr 2024 15:53:51 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Add the basic skeleton for a new platform driver for the microcontroller
> > found on the Turris Omnia board.  
> 
> ...
> 
> > +What:          /sys/bus/i2c/devices/<mcu_device>/serial_number
> > +Date:          July 2024
> > +KernelVersion: 6.10
> > +Contact:       Marek Behún <kabel@kernel.org>
> > +Description:   (RO) Contains the 64 bit long board serial number in hexadecimal  
> 
> 64 bit long --> 64-bit

ok

> 
> > +               format.
> > +
> > +               Only available if board information is burned in the MCU (older
> > +               revisions have board information burned in the ATSHA204-A chip).
> > +
> > +               Format: %016X.  
> 
> It's strange to use capitalized hexadecimal here and not in other
> files, but maybe it's something special about "serial number"? Dunno.

Yes, the serial number is printed with captial hex letters.

> > +menuconfig CZNIC_PLATFORMS
> > +       bool "Platform support for CZ.NIC's Turris hardware"  
> 
> > +       depends on MACH_ARMADA_38X || COMPILE_TEST  
> 
> This...
> 
> > +       help
> > +         Say Y here to be able to choose driver support for CZ.NIC's Turris
> > +         devices. This option alone does not add any kernel code.
> > +
> > +if CZNIC_PLATFORMS
> > +
> > +config TURRIS_OMNIA_MCU
> > +       tristate "Turris Omnia MCU driver"  
> 
> > +       depends on MACH_ARMADA_38X || COMPILE_TEST  
> 
> ...or this dependency is redundant. I think one would expect that
> these platforms will not be always based on the same platform, hence I
> would drop the former and leave the latter. But you should know better
> than me.

ok

> 
> > +       depends on I2C
> > +       help
> > +         Say Y here to add support for the features implemented by the
> > +         microcontroller on the CZ.NIC's Turris Omnia SOHO router.
> > +         To compile this driver as a module, choose M here; the module will be
> > +         called turris-omnia-mcu.  
> 
> ...
> 
> > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])  
> 
> Interesting format of the last parameter. Does it make any difference
> to the compiler if you use u8 *version?

The compiler will warn if an array with not enough space is passed as
argument.

> 
> > +{
> > +       u8 reply[OMNIA_FW_VERSION_LEN];
> > +       int err;
> > +
> > +       err = omnia_cmd_read(mcu->client,
> > +                            bootloader ? CMD_GET_FW_VERSION_BOOT
> > +                                       : CMD_GET_FW_VERSION_APP,
> > +                            reply, sizeof(reply));
> > +       if (err)
> > +               return err;  
> 
> > +       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > +       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  
> 
> Hmm... I would rather use returned value
> 
> char *p;
> 
> p = bin2hex(...);
> *p = '\0';
> 
> return 0;

OK. I guess

  *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';

would be too crazy, right?

> 
> > +       return 0;
> > +}  
> 
> ...
> 
> > +static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
> > +                                           struct attribute *a, int n)
> > +{
> > +       struct device *dev = kobj_to_dev(kobj);
> > +       struct omnia_mcu *mcu = dev_get_drvdata(dev);  
> 
> > +       umode_t mode = a->mode;  
> 
> Do you need this?
> 
> > +       if ((a == &dev_attr_serial_number.attr ||
> > +            a == &dev_attr_first_mac_address.attr ||
> > +            a == &dev_attr_board_revision.attr) &&
> > +           !(mcu->features & FEAT_BOARD_INFO))  
> 
> > +               mode = 0;  
> 
> return 0; ?
> 
> > +       return mode;  
> 
> return a->mode; ?

ok

> 
> > +}  
> 
> ...
> 
> > +static void omnia_mcu_print_version_hash(struct omnia_mcu *mcu, bool bootloader)
> > +{
> > +       const char *type = bootloader ? "bootloader" : "application";
> > +       struct device *dev = &mcu->client->dev;
> > +       u8 version[OMNIA_FW_VERSION_HEX_LEN];
> > +       int err;
> > +
> > +       err = omnia_get_version_hash(mcu, bootloader, version);
> > +       if (err) {
> > +               dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > +                       err);  
> 
> One  line?

I'd like to keep this driver to 80 columns.

> 
> > +               return;
> > +       }
> > +
> > +       dev_info(dev, "MCU %s firmware version hash: %s\n", type, version);
> > +}  
> 
> ...
> 
> > +static const char *omnia_status_to_mcu_type(uint16_t status)  
> 
> Why out of a sudden uint16_t instead of u16?

This was a mistake, thanks.

> > +{
> > +       switch (status & STS_MCU_TYPE_MASK) {
> > +       case STS_MCU_TYPE_STM32:
> > +               return "STM32";
> > +       case STS_MCU_TYPE_GD32:
> > +               return "GD32";
> > +       case STS_MCU_TYPE_MKL:
> > +               return "MKL";
> > +       default:
> > +               return "unknown";
> > +       }
> > +}  
> 
> ...
> 
> > +       static const struct {
> > +               uint16_t mask;  
> 
> Ditto.
> 
> > +               const char *name;
> > +       } features[] = {
> > +               { FEAT_EXT_CMDS,           "extended control and status" },
> > +               { FEAT_WDT_PING,           "watchdog pinging" },
> > +               { FEAT_LED_STATE_EXT_MASK, "peripheral LED pins reading" },
> > +               { FEAT_NEW_INT_API,        "new interrupt API" },
> > +               { FEAT_POWEROFF_WAKEUP,    "poweroff and wakeup" },
> > +               { FEAT_TRNG,               "true random number generator" },
> > +       };  
> 
> ...
> 
> > +               omnia_info_missing_feature(dev, "feature reading");  
> 
> Tautology. Read the final message. I believe you wanted to pass just
> "reading" here.

No, I actually wanted it to print
  Your board's MCU firmware does not support the feature reading
  feature.
as in the feature "feature reading" is not supported.
I guess I could change it to
  Your board's MCU firmware does not support the feature reading.
but that would complicate the code: either I would need to add
" feature" suffix to all the features[].name, or duplicate the
info string from the omnia_info_missing_feature() function.

> ...
> 
> > +       memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);  
> 
> There is an API ether_copy_addr() or so, don't remember by heart.
> You also need an include for ETH_ALEN definition.

Doc for ether_addr_copy says:
  Please note: dst & src must both be aligned to u16.
since the code does:
  u16 *a = (u16 *)dst;
  const u16 *b = (const u16 *)src;

  a[0] = b[0];
  a[1] = b[1];
  a[2] = b[2]

Since I am copying from &reply[9], which is not u16-aligned, this won't
work.

> ...
> 
> > +#include <linux/i2c.h>  
> 
> No users of this, you may replace with
> 
> struct i2c_client;
> 
> Am I right?

OK.

> 
> ...
> 
> > +       CMD_GET_FW_VERSION_BOOT         = 0x0E, /* 20B git hash
> > number */  
> 
> Git

OK.

> ...
> 
> > +       /* available only at address 0x2b (led-controller) */  
> 
> LED-controller

OK

> 
> ...
> 
> > +enum omnia_ctl_byte_e {
> > +       CTL_LIGHT_RST           = BIT(0),
> > +       CTL_HARD_RST            = BIT(1),
> > +       /* BIT(2) is currently reserved */
> > +       CTL_USB30_PWRON         = BIT(3),
> > +       CTL_USB31_PWRON         = BIT(4),
> > +       CTL_ENABLE_4V5          = BIT(5),
> > +       CTL_BUTTON_MODE         = BIT(6),
> > +       CTL_BOOTLOADER          = BIT(7)  
> 
> Keep trailing comma as it might be extended (theoretically). And you
> do the similar in other enums anyway.

ctl_byt is 8-bit, so this enum really can't be extended. In fact I need
to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.

Thanks, Andy.

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Dan Carpenter]

On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > +{
> > > +       u8 reply[OMNIA_FW_VERSION_LEN];
> > > +       int err;
> > > +
> > > +       err = omnia_cmd_read(mcu->client,
> > > +                            bootloader ? CMD_GET_FW_VERSION_BOOT
> > > +                                       : CMD_GET_FW_VERSION_APP,
> > > +                            reply, sizeof(reply));
> > > +       if (err)
> > > +               return err;  
> > 
> > > +       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > > +       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  
> > 
> > Hmm... I would rather use returned value
> > 
> > char *p;
> > 
> > p = bin2hex(...);
> > *p = '\0';
> > 
> > return 0;
> 
> OK. I guess
> 
>   *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
> 
> would be too crazy, right?

Indeed.  Please don't do that.  0_0

regards,
dan carpenter

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> On Tue, 30 Apr 2024 15:53:51 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > 
> > Interesting format of the last parameter. Does it make any difference
> > to the compiler if you use u8 *version?
> 
> The compiler will warn if an array with not enough space is passed as
> argument.

Really?

> > > +{
> > > +       u8 reply[OMNIA_FW_VERSION_LEN];
> > > +       int err;
> > > +
> > > +       err = omnia_cmd_read(mcu->client,
> > > +                            bootloader ? CMD_GET_FW_VERSION_BOOT
> > > +                                       : CMD_GET_FW_VERSION_APP,
> > > +                            reply, sizeof(reply));
> > > +       if (err)
> > > +               return err;  
> > 
> > > +       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
> > > +       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  
> > 
> > Hmm... I would rather use returned value
> > 
> > char *p;
> > 
> > p = bin2hex(...);
> > *p = '\0';
> > 
> > return 0;
> 
> OK. I guess
> 
>   *bin2hex(version, reply, OMNIA_FW_VERSION_LEN) = '\0';
> 
> would be too crazy, right?

Yes, it's not a Python :-)

> > > +       return 0;
> > > +}  

...

> > > +               dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > > +                       err);  
> > 
> > One  line?
> 
> I'd like to keep this driver to 80 columns.

Then better to have a logical split then?

			dev_err(dev, "Cannot read MCU %s firmware version: %d\n",
				type, err);

...

> > > +               omnia_info_missing_feature(dev, "feature reading");  
> > 
> > Tautology. Read the final message. I believe you wanted to pass just
> > "reading" here.
> 
> No, I actually wanted it to print
>   Your board's MCU firmware does not support the feature reading
>   feature.
> as in the feature "feature reading" is not supported.
> I guess I could change it to
>   Your board's MCU firmware does not support the feature reading.
> but that would complicate the code: either I would need to add
> " feature" suffix to all the features[].name, or duplicate the
> info string from the omnia_info_missing_feature() function.

From point of a mere user (as I am towards this driver) I consider
the tautology confusing.

	...the 'reading' feature

may be a good compromise.

...

> > > +       memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);  
> > 
> > There is an API ether_copy_addr() or so, don't remember by heart.
> > You also need an include for ETH_ALEN definition.
> 
> Doc for ether_addr_copy says:
>   Please note: dst & src must both be aligned to u16.
> since the code does:
>   u16 *a = (u16 *)dst;
>   const u16 *b = (const u16 *)src;
> 
>   a[0] = b[0];
>   a[1] = b[1];
>   a[2] = b[2]
> 
> Since I am copying from &reply[9], which is not u16-aligned, this won't
> work.

It would work on architectures that support misaligned accesses, but in general
you are right. Perhaps a comment on top to avoid "cleanup" patches like this?

...

> > > +enum omnia_ctl_byte_e {
> > > +       CTL_LIGHT_RST           = BIT(0),
> > > +       CTL_HARD_RST            = BIT(1),
> > > +       /* BIT(2) is currently reserved */
> > > +       CTL_USB30_PWRON         = BIT(3),
> > > +       CTL_USB31_PWRON         = BIT(4),
> > > +       CTL_ENABLE_4V5          = BIT(5),
> > > +       CTL_BUTTON_MODE         = BIT(6),
> > > +       CTL_BOOTLOADER          = BIT(7)  
> > 
> > Keep trailing comma as it might be extended (theoretically). And you
> > do the similar in other enums anyway.
> 
> ctl_byt is 8-bit, so this enum really can't be extended.

I understand that (even before writing the previous reply).

> In fact I need
> to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.

Who prevents from having in a new firmware let's say

 BIT(31) | BIT(1)

as a new value?

From Linux perspective these are not terminating lines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > On Tue, 30 Apr 2024 15:53:51 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> 
> ...
> 
> > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > 
> > > Interesting format of the last parameter. Does it make any difference
> > > to the compiler if you use u8 *version?
> > 
> > The compiler will warn if an array with not enough space is passed as
> > argument.
> 
> Really?

Indeed:

  extern void a(char *x);

  static void b(char x[static 10]) {
      a(x);
  }

  void c(void) {
      char x[5] = "abcd";
      b(x);
  }

says:
  test.c: In function ‘c’:
  test.c:9:9: warning: ‘b’ accessing 10 bytes in a region of size 5 [-Wstringop-overflow=]
      9 |         b(x);
        |         ^~~~
  test.c:9:9: note: referencing argument 1 of type ‘char[10]’
  test.c:3:13: note: in a call to function ‘b’
      3 | static void b(char x[static 10]) {
        |

...

> > > > +               dev_err(dev, "Cannot read MCU %s firmware version: %d\n", type,
> > > > +                       err);  
> > > 
> > > One  line?
> > 
> > I'd like to keep this driver to 80 columns.
> 
> Then better to have a logical split then?
> 
> 			dev_err(dev, "Cannot read MCU %s firmware version: %d\n",
> 				type, err);

OK

> > > > +               omnia_info_missing_feature(dev, "feature reading");  
> > > 
> > > Tautology. Read the final message. I believe you wanted to pass just
> > > "reading" here.
> > 
> > No, I actually wanted it to print
> >   Your board's MCU firmware does not support the feature reading
> >   feature.
> > as in the feature "feature reading" is not supported.
> > I guess I could change it to
> >   Your board's MCU firmware does not support the feature reading.
> > but that would complicate the code: either I would need to add
> > " feature" suffix to all the features[].name, or duplicate the
> > info string from the omnia_info_missing_feature() function.
> 
> From point of a mere user (as I am towards this driver) I consider
> the tautology confusing.
> 
> 	...the 'reading' feature
> 
> may be a good compromise.

I have rewritten it to use another dev_info:
  dev_info(dev,
           "Your board's MCU firmware does not support feature reading.\n");

> 
> ...
> 
> > > > +       memcpy(mcu->board_first_mac, &reply[9], ETH_ALEN);  
> > > 
> > > There is an API ether_copy_addr() or so, don't remember by heart.
> > > You also need an include for ETH_ALEN definition.
> > 
> > Doc for ether_addr_copy says:
> >   Please note: dst & src must both be aligned to u16.
> > since the code does:
> >   u16 *a = (u16 *)dst;
> >   const u16 *b = (const u16 *)src;
> > 
> >   a[0] = b[0];
> >   a[1] = b[1];
> >   a[2] = b[2]
> > 
> > Since I am copying from &reply[9], which is not u16-aligned, this won't
> > work.
> 
> It would work on architectures that support misaligned accesses, but in general
> you are right. Perhaps a comment on top to avoid "cleanup" patches like this?


OK
 
> > > > +enum omnia_ctl_byte_e {
> > > > +       CTL_LIGHT_RST           = BIT(0),
> > > > +       CTL_HARD_RST            = BIT(1),
> > > > +       /* BIT(2) is currently reserved */
> > > > +       CTL_USB30_PWRON         = BIT(3),
> > > > +       CTL_USB31_PWRON         = BIT(4),
> > > > +       CTL_ENABLE_4V5          = BIT(5),
> > > > +       CTL_BUTTON_MODE         = BIT(6),
> > > > +       CTL_BOOTLOADER          = BIT(7)  
> > > 
> > > Keep trailing comma as it might be extended (theoretically). And you
> > > do the similar in other enums anyway.
> > 
> > ctl_byt is 8-bit, so this enum really can't be extended.
> 
> I understand that (even before writing the previous reply).
> 
> > In fact I need
> > to drop the last comma from omnia_ext_sts_dword_e and omnia_int_e.
> 
> Who prevents from having in a new firmware let's say
> 
>  BIT(31) | BIT(1)
> 
> as a new value?
> 
> From Linux perspective these are not terminating lines.

OK

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > >
> > > > Interesting format of the last parameter. Does it make any difference
> > > > to the compiler if you use u8 *version?
> > >
> > > The compiler will warn if an array with not enough space is passed as
> > > argument.
> >
> > Really?
>
> Indeed:
>
>   extern void a(char *x);
>
>   static void b(char x[static 10]) {
>       a(x);
>   }
>
>   void c(void) {
>       char x[5] = "abcd";

>       b(x);

It's not the example I was talking about. Here should be a(x).

>   }
>
> says:
>   test.c: In function ‘c’:
>   test.c:9:9: warning: ‘b’ accessing 10 bytes in a region of size 5 [-Wstringop-overflow=]
>       9 |         b(x);
>         |         ^~~~
>   test.c:9:9: note: referencing argument 1 of type ‘char[10]’
>   test.c:3:13: note: in a call to function ‘b’
>       3 | static void b(char x[static 10]) {
>         |

...

> > > > > +               omnia_info_missing_feature(dev, "feature reading");
> > > >
> > > > Tautology. Read the final message. I believe you wanted to pass just
> > > > "reading" here.
> > >
> > > No, I actually wanted it to print
> > >   Your board's MCU firmware does not support the feature reading
> > >   feature.
> > > as in the feature "feature reading" is not supported.
> > > I guess I could change it to
> > >   Your board's MCU firmware does not support the feature reading.
> > > but that would complicate the code: either I would need to add
> > > " feature" suffix to all the features[].name, or duplicate the
> > > info string from the omnia_info_missing_feature() function.
> >
> > From point of a mere user (as I am towards this driver) I consider
> > the tautology confusing.
> >
> >       ...the 'reading' feature
> >
> > may be a good compromise.
>
> I have rewritten it to use another dev_info:
>   dev_info(dev,
>            "Your board's MCU firmware does not support feature reading.\n");

OK! As long as there are no two messages in a row for the same error.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> 
> ...
> 
> > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > >
> > > > > Interesting format of the last parameter. Does it make any difference
> > > > > to the compiler if you use u8 *version?
> > > >
> > > > The compiler will warn if an array with not enough space is passed as
> > > > argument.
> > >
> > > Really?
> >
> > Indeed:
> >
> >   extern void a(char *x);
> >
> >   static void b(char x[static 10]) {
> >       a(x);
> >   }
> >
> >   void c(void) {
> >       char x[5] = "abcd";
> 
> >       b(x);
> 
> It's not the example I was talking about. Here should be a(x).

Somehow I got lost. Let's return to my function, where I have
  int omnia_get_version_hash(..., u8 version[static 40]);

and then I use this function:

  u8 version[40];
  omnia_get_version_hash(..., version);

If somehow I made a mistake and declared the version array shorter:
  u8 version[20];
  omnia_get_version_hash(..., version);
I would get a warning.

So the purpose is to get warned if the compiler can prove that there is
not enough space in the destination buffer.

Marek

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

On Thu, May 2, 2024 at 10:18 PM Marek Behún <kabel@kernel.org> wrote:
> On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> > On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > > >
> > > > > > Interesting format of the last parameter. Does it make any difference
> > > > > > to the compiler if you use u8 *version?
> > > > >
> > > > > The compiler will warn if an array with not enough space is passed as
> > > > > argument.
> > > >
> > > > Really?
> > >
> > > Indeed:
> > >
> > >   extern void a(char *x);
> > >
> > >   static void b(char x[static 10]) {
> > >       a(x);
> > >   }
> > >
> > >   void c(void) {
> > >       char x[5] = "abcd";
> >
> > >       b(x);
> >
> > It's not the example I was talking about. Here should be a(x).
>
> Somehow I got lost. Let's return to my function, where I have
>   int omnia_get_version_hash(..., u8 version[static 40]);
>
> and then I use this function:
>
>   u8 version[40];
>   omnia_get_version_hash(..., version);
>
> If somehow I made a mistake and declared the version array shorter:
>   u8 version[20];
>   omnia_get_version_hash(..., version);
> I would get a warning.

Yes. Would you get the same warning if you replace the parameter to a pointer?

> So the purpose is to get warned if the compiler can prove that there is
> not enough space in the destination buffer.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

On Fri, May 03, 2024 at 06:59:20AM +0300, Andy Shevchenko wrote:
> On Thu, May 2, 2024 at 10:18 PM Marek Behún <kabel@kernel.org> wrote:
> > On Thu, May 02, 2024 at 09:47:25PM +0300, Andy Shevchenko wrote:
> > > On Thu, May 2, 2024 at 9:40 PM Marek Behún <kabel@kernel.org> wrote:
> > > > On Tue, Apr 30, 2024 at 06:17:45PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Apr 30, 2024 at 04:05:07PM +0200, Marek Behún wrote:
> > > > > > On Tue, 30 Apr 2024 15:53:51 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> 
> ...
> 
> > > > > > > > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> > > > > > > > +                                 u8 version[static OMNIA_FW_VERSION_HEX_LEN])
> > > > > > >
> > > > > > > Interesting format of the last parameter. Does it make any difference
> > > > > > > to the compiler if you use u8 *version?
> > > > > >
> > > > > > The compiler will warn if an array with not enough space is passed as
> > > > > > argument.
> > > > >
> > > > > Really?
> > > >
> > > > Indeed:
> > > >
> > > >   extern void a(char *x);
> > > >
> > > >   static void b(char x[static 10]) {
> > > >       a(x);
> > > >   }
> > > >
> > > >   void c(void) {
> > > >       char x[5] = "abcd";
> > >
> > > >       b(x);
> > >
> > > It's not the example I was talking about. Here should be a(x).
> >
> > Somehow I got lost. Let's return to my function, where I have
> >   int omnia_get_version_hash(..., u8 version[static 40]);
> >
> > and then I use this function:
> >
> >   u8 version[40];
> >   omnia_get_version_hash(..., version);
> >
> > If somehow I made a mistake and declared the version array shorter:
> >   u8 version[20];
> >   omnia_get_version_hash(..., version);
> > I would get a warning.
> 
> Yes. Would you get the same warning if you replace the parameter to a pointer?

If you mean instead of declaring
  u8 version[20];
if I did
  u8 *version;
  omnia_get_version_hash(..., version);
then I will get a different warning:
  version is used uninitialized

And if you mean that instead of the
  char version[static 40]
in the argument list of the function I used
  char *version,
then no, GCC spits no warning.

Marek

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Ilpo Järvinen]

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

On Tue, 30 Apr 2024, Marek Behún wrote:

> Add the basic skeleton for a new platform driver for the microcontroller
> found on the Turris Omnia board.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>

> +	struct device *dev = &mcu->client->dev;
> +	bool suggest_fw_upgrade = false;
> +	int status;
> +
> +	/* status word holds MCU type, which we need below */
> +	status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
> +	if (status < 0)
> +		return status;
> +
> +	/* check whether MCU firmware supports the CMD_GET_FEAUTRES command */

FEATURES

> +	if (status & STS_FEATURES_SUPPORTED) {
> +		__le32 reply;
> +		int ret;
> +
> +		/* try read 32-bit features */
> +		ret = omnia_cmd_read(mcu->client, CMD_GET_FEATURES, &reply,
> +				     sizeof(reply));
> +		if (ret) {
> +			/* try read 16-bit features */
> +			ret = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES);
> +			if (ret < 0)
> +				return ret;
> +
> +			mcu->features = ret;
> +		} else {
> +			mcu->features = le32_to_cpu(reply);
> +
> +			if (mcu->features & FEAT_FROM_BIT_16_INVALID)
> +				mcu->features &= GENMASK(15, 0);
> +		}

I'm not a big fan of the inconsistency here when it comes to who handles 
the endianness, perhaps there is a good reason for that but it looks a bit 
odd to have it done in a different way for 32-bit and 16-bit.

> +	} else {
> +		omnia_info_missing_feature(dev, "feature reading");
> +		suggest_fw_upgrade = true;
> +	}
> +
> +	mcu->type = omnia_status_to_mcu_type(status);
> +	dev_info(dev, "MCU type %s%s\n", mcu->type,
> +		 (mcu->features & FEAT_PERIPH_MCU) ?
> +			", with peripheral resets wired" : "");
> +
> +	omnia_mcu_print_version_hash(mcu, true);
> +
> +	if (mcu->features & FEAT_BOOTLOADER)
> +		dev_warn(dev,
> +			 "MCU is running bootloader firmware. Was firmware upgrade interrupted?\n");
> +	else
> +		omnia_mcu_print_version_hash(mcu, false);
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(features); i++) {
> +		if (mcu->features & features[i].mask)
> +			continue;
> +
> +		omnia_info_missing_feature(dev, features[i].name);
> +		suggest_fw_upgrade = true;
> +	}
> +
> +	if (suggest_fw_upgrade)
> +		dev_info(dev,
> +			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
> +
> +	return 0;
> +}

> +int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
> +		   unsigned int len);
> +
> +static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
> +{
> +	__le16 reply;
> +	int err;
> +
> +	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
> +
> +	return err ?: le16_to_cpu(reply);
> +}
> +
> +static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
> +{
> +	u8 reply;
> +	int err;
> +
> +	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
> +
> +	return err ?: reply;
> +}
> +
> +#endif /* __TURRIS_OMNIA_MCU_H */


> diff --git a/include/linux/turris-omnia-mcu-interface.h b/include/linux/turris-omnia-mcu-interface.h
> new file mode 100644
> index 000000000000..88f8490b5897
> --- /dev/null
> +++ b/include/linux/turris-omnia-mcu-interface.h
> @@ -0,0 +1,249 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * CZ.NIC's Turris Omnia MCU I2C interface commands definitions
> + *
> + * 2024 by Marek Behún <kabel@kernel.org>
> + */
> +
> +#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H
> +#define __TURRIS_OMNIA_MCU_INTERFACE_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +
> +enum omnia_commands_e {
> +	CMD_GET_STATUS_WORD		= 0x01, /* slave sends status word back */
> +	CMD_GENERAL_CONTROL		= 0x02,
> +	CMD_LED_MODE			= 0x03, /* default/user */
> +	CMD_LED_STATE			= 0x04, /* LED on/off */
> +	CMD_LED_COLOR			= 0x05, /* LED number + RED + GREEN + BLUE */
> +	CMD_USER_VOLTAGE		= 0x06,
> +	CMD_SET_BRIGHTNESS		= 0x07,
> +	CMD_GET_BRIGHTNESS		= 0x08,
> +	CMD_GET_RESET			= 0x09,
> +	CMD_GET_FW_VERSION_APP		= 0x0A, /* 20B git hash number */
> +	CMD_SET_WATCHDOG_STATE		= 0x0B, /* 0 - disable
> +						 * 1 - enable / ping
> +						 * after boot watchdog is started
> +						 * with 2 minutes timeout
> +						 */
> +
> +	/* CMD_WATCHDOG_STATUS		= 0x0C, not implemented anymore */
> +
> +	CMD_GET_WATCHDOG_STATE		= 0x0D,
> +	CMD_GET_FW_VERSION_BOOT		= 0x0E, /* 20B git hash number */
> +	CMD_GET_FW_CHECKSUM		= 0x0F, /* 4B length, 4B checksum */
> +
> +	/* available if FEATURES_SUPPORTED bit set in status word */
> +	CMD_GET_FEATURES		= 0x10,
> +
> +	/* available if EXT_CMD bit set in features */
> +	CMD_GET_EXT_STATUS_DWORD	= 0x11,
> +	CMD_EXT_CONTROL			= 0x12,
> +	CMD_GET_EXT_CONTROL_STATUS	= 0x13,
> +
> +	/* available if NEW_INT_API bit set in features */
> +	CMD_GET_INT_AND_CLEAR		= 0x14,
> +	CMD_GET_INT_MASK		= 0x15,
> +	CMD_SET_INT_MASK		= 0x16,
> +
> +	/* available if FLASHING bit set in features */
> +	CMD_FLASH			= 0x19,
> +
> +	/* available if WDT_PING bit set in features */
> +	CMD_SET_WDT_TIMEOUT		= 0x20,
> +	CMD_GET_WDT_TIMELEFT		= 0x21,
> +
> +	/* available if POWEROFF_WAKEUP bit set in features */
> +	CMD_SET_WAKEUP			= 0x22,
> +	CMD_GET_UPTIME_AND_WAKEUP	= 0x23,
> +	CMD_POWER_OFF			= 0x24,
> +
> +	/* available if USB_OVC_PROT_SETTING bit set in features */
> +	CMD_SET_USB_OVC_PROT		= 0x25,
> +	CMD_GET_USB_OVC_PROT		= 0x26,
> +
> +	/* available if TRNG bit set in features */
> +	CMD_TRNG_COLLECT_ENTROPY	= 0x28,
> +
> +	/* available if CRYPTO bit set in features */
> +	CMD_CRYPTO_GET_PUBLIC_KEY	= 0x29,
> +	CMD_CRYPTO_SIGN_MESSAGE		= 0x2A,
> +	CMD_CRYPTO_COLLECT_SIGNATURE	= 0x2B,
> +
> +	/* available if BOARD_INFO it set in features */
> +	CMD_BOARD_INFO_GET		= 0x2C,
> +	CMD_BOARD_INFO_BURN		= 0x2D,
> +
> +	/* available only at address 0x2b (led-controller) */
> +	/* available only if LED_GAMMA_CORRECTION bit set in features */
> +	CMD_SET_GAMMA_CORRECTION	= 0x30,
> +	CMD_GET_GAMMA_CORRECTION	= 0x31,
> +
> +	/* available only at address 0x2b (led-controller) */
> +	/* available only if PER_LED_CORRECTION bit set in features */
> +	/* available only if FROM_BIT_16_INVALID bit NOT set in features */
> +	CMD_SET_LED_CORRECTIONS		= 0x32,
> +	CMD_GET_LED_CORRECTIONS		= 0x33,
> +};
> +
> +enum omnia_flashing_commands_e {
> +	FLASH_CMD_UNLOCK		= 0x01,
> +	FLASH_CMD_SIZE_AND_CSUM		= 0x02,
> +	FLASH_CMD_PROGRAM		= 0x03,
> +	FLASH_CMD_RESET			= 0x04,
> +};

I'm bit worried about many items above, they seem generic enough they 
could trigger name conflict at some point. Could these all defines be 
prefixed so there's no risk of collisions.

> +enum omnia_sts_word_e {
> +	STS_MCU_TYPE_MASK			= GENMASK(1, 0),
> +	STS_MCU_TYPE_STM32			= 0 << 0,
> +	STS_MCU_TYPE_GD32			= 1 << 0,
> +	STS_MCU_TYPE_MKL			= 2 << 0,

These are FIELD_PREP(STS_MCU_TYPE_MASK, x), although I suspect you need to 
use FIELD_PREP_CONST() in this context. If neither ends up working in this 
context, then leave it as is.

> +	STS_FEATURES_SUPPORTED			= BIT(2),
> +	STS_USER_REGULATOR_NOT_SUPPORTED	= BIT(3),
> +	STS_CARD_DET				= BIT(4),
> +	STS_MSATA_IND				= BIT(5),
> +	STS_USB30_OVC				= BIT(6),
> +	STS_USB31_OVC				= BIT(7),
> +	STS_USB30_PWRON				= BIT(8),
> +	STS_USB31_PWRON				= BIT(9),
> +	STS_ENABLE_4V5				= BIT(10),
> +	STS_BUTTON_MODE				= BIT(11),
> +	STS_BUTTON_PRESSED			= BIT(12),
> +	STS_BUTTON_COUNTER_MASK			= GENMASK(15, 13)
> +};
> +
> +enum omnia_ctl_byte_e {
> +	CTL_LIGHT_RST		= BIT(0),
> +	CTL_HARD_RST		= BIT(1),
> +	/* BIT(2) is currently reserved */
> +	CTL_USB30_PWRON		= BIT(3),
> +	CTL_USB31_PWRON		= BIT(4),
> +	CTL_ENABLE_4V5		= BIT(5),
> +	CTL_BUTTON_MODE		= BIT(6),
> +	CTL_BOOTLOADER		= BIT(7)
> +};
> +
> +enum omnia_features_e {
> +	FEAT_PERIPH_MCU			= BIT(0),
> +	FEAT_EXT_CMDS			= BIT(1),
> +	FEAT_WDT_PING			= BIT(2),
> +	FEAT_LED_STATE_EXT_MASK		= GENMASK(4, 3),
> +	FEAT_LED_STATE_EXT		= 1 << 3,
> +	FEAT_LED_STATE_EXT_V32		= 2 << 3,

Ditto.

> +	FEAT_LED_GAMMA_CORRECTION	= BIT(5),
> +	FEAT_NEW_INT_API		= BIT(6),
> +	FEAT_BOOTLOADER			= BIT(7),
> +	FEAT_FLASHING			= BIT(8),
> +	FEAT_NEW_MESSAGE_API		= BIT(9),
> +	FEAT_BRIGHTNESS_INT		= BIT(10),
> +	FEAT_POWEROFF_WAKEUP		= BIT(11),
> +	FEAT_CAN_OLD_MESSAGE_API	= BIT(12),
> +	FEAT_TRNG			= BIT(13),
> +	FEAT_CRYPTO			= BIT(14),
> +	FEAT_BOARD_INFO			= BIT(15),
> +
> +	/*
> +	 * Orginally the features command replied only 16 bits. If more were
> +	 * read, either the I2C transaction failed or 0xff bytes were sent.
> +	 * Therefore to consider bits 16 - 31 valid, one bit (20) was reserved
> +	 * to be zero.
> +	 */
> +
> +	/* Bits 16 - 19 correspond to bits 0 - 3 of status word */
> +	FEAT_MCU_TYPE_MASK		= GENMASK(17, 16),
> +	FEAT_MCU_TYPE_STM32		= 0 << 16,
> +	FEAT_MCU_TYPE_GD32		= 1 << 16,
> +	FEAT_MCU_TYPE_MKL		= 2 << 16,

Ditto.


-- 
 i.

Re: [PATCH v8 2/9] platform: cznic: Add preliminary support for Turris Omnia MCU

[Marek Behún]

Thanks, Ilpo, I've applied all your suggestions.

[PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

Add support for GPIOs connected to the MCU on the Turris Omnia board.

This includes:
- front button pin
- enable pins for USB regulators
- MiniPCIe / mSATA card presence pins in MiniPCIe port 0
- LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
- on board revisions 32+ also various peripheral resets and another
  voltage regulator enable pin

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 .../sysfs-bus-i2c-devices-turris-omnia-mcu    |   16 +
 drivers/platform/cznic/Kconfig                |   15 +
 drivers/platform/cznic/Makefile               |    1 +
 .../platform/cznic/turris-omnia-mcu-base.c    |   33 +-
 .../platform/cznic/turris-omnia-mcu-gpio.c    | 1048 +++++++++++++++++
 drivers/platform/cznic/turris-omnia-mcu.h     |   69 +-
 6 files changed, 1166 insertions(+), 16 deletions(-)
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c

diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
index ff5e7cb00206..b27b60d00c87 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -22,6 +22,22 @@ Description:	(RO) Contains device first MAC address. Each Turris Omnia is
 
 		Format: %pM.
 
+What:		/sys/bus/i2c/devices/<mcu_device>/front_button_mode
+Date:		July 2024
+KernelVersion:	6.10
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RW) The front button on the Turris Omnia router can be
+		configured either to change the intensity of all the LEDs on the
+		front panel, or to send the press event to the CPU as an
+		interrupt.
+
+		This file switches between these two modes:
+		- "mcu" makes the button press event be handled by the MCU to
+		  change the LEDs panel intensity.
+		- "cpu" makes the button press event be handled by the CPU.
+
+		Format: %s.
+
 What:		/sys/bus/i2c/devices/<mcu_device>/fw_features
 Date:		July 2024
 KernelVersion:	6.10
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index f8560ff9c1af..3a8c3edcd7e6 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -17,9 +17,24 @@ config TURRIS_OMNIA_MCU
 	tristate "Turris Omnia MCU driver"
 	depends on MACH_ARMADA_38X || COMPILE_TEST
 	depends on I2C
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	help
 	  Say Y here to add support for the features implemented by the
 	  microcontroller on the CZ.NIC's Turris Omnia SOHO router.
+	  The features include:
+	  - GPIO pins
+	    - to get front button press events (the front button can be
+	      configured either to generate press events to the CPU or to change
+	      front LEDs panel brightness)
+	    - to enable / disable USB port voltage regulators and to detect
+	      USB overcurrent
+	    - to detect MiniPCIe / mSATA card presence in MiniPCIe port 0
+	    - to configure resets of various peripherals on board revisions 32+
+	    - to enable / disable the VHV voltage regulator to the SOC in order
+	      to be able to program SOC's OTP on board revisions 32+
+	    - to get input from the LED output pins of the WAN ethernet PHY, LAN
+	      switch and MiniPCIe ports
 	  To compile this driver as a module, choose M here; the module will be
 	  called turris-omnia-mcu.
 
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index 31adca73bb94..53fd8f1777a3 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
 turris-omnia-mcu-y		:= turris-omnia-mcu-base.o
+turris-omnia-mcu-y		+= turris-omnia-mcu-gpio.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 8893c990853a..708b084cb5da 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -22,25 +22,31 @@
 #define OMNIA_FW_VERSION_HEX_LEN	(2 * OMNIA_FW_VERSION_LEN + 1)
 #define OMNIA_BOARD_INFO_LEN		16
 
-int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
-		   unsigned int len)
+int omnia_cmd_write_read(const struct i2c_client *client,
+			 void *cmd, unsigned int cmd_len,
+			 void *reply, unsigned int reply_len)
 {
 	struct i2c_msg msgs[2];
-	int ret;
+	int ret, num;
 
 	msgs[0].addr = client->addr;
 	msgs[0].flags = 0;
-	msgs[0].len = 1;
-	msgs[0].buf = &cmd;
-	msgs[1].addr = client->addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = reply;
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	msgs[0].len = cmd_len;
+	msgs[0].buf = cmd;
+	num = 1;
+
+	if (reply_len) {
+		msgs[1].addr = client->addr;
+		msgs[1].flags = I2C_M_RD;
+		msgs[1].len = reply_len;
+		msgs[1].buf = reply;
+		num++;
+	}
+
+	ret = i2c_transfer(client->adapter, msgs, num);
 	if (ret < 0)
 		return ret;
-	if (ret != ARRAY_SIZE(msgs))
+	if (ret != num)
 		return -EIO;
 
 	return 0;
@@ -188,6 +194,7 @@ static const struct attribute_group omnia_mcu_base_group = {
 
 static const struct attribute_group *omnia_mcu_groups[] = {
 	&omnia_mcu_base_group,
+	&omnia_mcu_gpio_group,
 	NULL
 };
 
@@ -353,7 +360,7 @@ static int omnia_mcu_probe(struct i2c_client *client)
 					     "Cannot read board info\n");
 	}
 
-	return 0;
+	return omnia_mcu_register_gpiochip(mcu);
 }
 
 static const struct of_device_id of_omnia_mcu_match[] = {
diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
new file mode 100644
index 000000000000..bcc12bd1453c
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -0,0 +1,1048 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU GPIO and IRQ driver
+ *
+ * 2024 by Marek Behún <kabel@kernel.org>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bug.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/devm-helpers.h>
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include <asm/unaligned.h>
+
+#include "turris-omnia-mcu.h"
+
+#define CMD_INT_ARG_LEN			8
+#define FRONT_BUTTON_RELEASE_DELAY	50 /* ms */
+
+static const char * const omnia_mcu_gpio_templates[64] = {
+	/* GPIOs with value read from the 16-bit wide status */
+	[4]  = "gpio%u.MiniPCIe0 Card Detect",
+	[5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
+	[6]  = "gpio%u.Front USB3 port over-current",
+	[7]  = "gpio%u.Rear USB3 port over-current",
+	[8]  = "gpio%u.Front USB3 port power",
+	[9]  = "gpio%u.Rear USB3 port power",
+	[12] = "gpio%u.Front Button",
+
+	/* GPIOs with value read from the 32-bit wide extended status */
+	[16] = "gpio%u.SFP nDET",
+	[28] = "gpio%u.MiniPCIe0 LED",
+	[29] = "gpio%u.MiniPCIe1 LED",
+	[30] = "gpio%u.MiniPCIe2 LED",
+	[31] = "gpio%u.MiniPCIe0 PAN LED",
+	[32] = "gpio%u.MiniPCIe1 PAN LED",
+	[33] = "gpio%u.MiniPCIe2 PAN LED",
+	[34] = "gpio%u.WAN PHY LED0",
+	[35] = "gpio%u.WAN PHY LED1",
+	[36] = "gpio%u.LAN switch p0 LED0",
+	[37] = "gpio%u.LAN switch p0 LED1",
+	[38] = "gpio%u.LAN switch p1 LED0",
+	[39] = "gpio%u.LAN switch p1 LED1",
+	[40] = "gpio%u.LAN switch p2 LED0",
+	[41] = "gpio%u.LAN switch p2 LED1",
+	[42] = "gpio%u.LAN switch p3 LED0",
+	[43] = "gpio%u.LAN switch p3 LED1",
+	[44] = "gpio%u.LAN switch p4 LED0",
+	[45] = "gpio%u.LAN switch p4 LED1",
+	[46] = "gpio%u.LAN switch p5 LED0",
+	[47] = "gpio%u.LAN switch p5 LED1",
+
+	/* GPIOs with value read from the 16-bit wide extended control status */
+	[48] = "gpio%u.eMMC nRESET",
+	[49] = "gpio%u.LAN switch nRESET",
+	[50] = "gpio%u.WAN PHY nRESET",
+	[51] = "gpio%u.MiniPCIe0 nPERST",
+	[52] = "gpio%u.MiniPCIe1 nPERST",
+	[53] = "gpio%u.MiniPCIe2 nPERST",
+	[54] = "gpio%u.WAN PHY SFP mux",
+};
+
+#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \
+	{								\
+		.cmd = _cmd,						\
+		.ctl_cmd = _ctl_cmd,					\
+		.bit = _bit,						\
+		.ctl_bit = _ctl_bit,					\
+		.int_bit = _int_bit,					\
+		.feat = _feat,						\
+		.feat_mask = _feat_mask,				\
+	}
+#define _DEF_GPIO_STS(_name) \
+	_DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0)
+#define _DEF_GPIO_CTL(_name) \
+	_DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \
+		  CTL_ ## _name, 0, 0, 0)
+#define _DEF_GPIO_EXT_STS(_name, _feat) \
+	_DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+		  INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \
+		  FEAT_ ## _feat | FEAT_EXT_CMDS)
+#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \
+	_DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+		  INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \
+		  FEAT_LED_STATE_EXT_MASK)
+#define _DEF_GPIO_EXT_STS_LEDALL(_name) \
+	_DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
+		  INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0)
+#define _DEF_GPIO_EXT_CTL(_name, _feat) \
+	_DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \
+		  EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \
+		  FEAT_ ## _feat | FEAT_EXT_CMDS, \
+		  FEAT_ ## _feat | FEAT_EXT_CMDS)
+#define _DEF_INT(_name) \
+	_DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0)
+
+static const struct omnia_gpio {
+	u8 cmd, ctl_cmd;
+	u32 bit, ctl_bit;
+	u32 int_bit;
+	u16 feat, feat_mask;
+} omnia_gpios[64] = {
+	/* GPIOs with value read from the 16-bit wide status */
+	[4]  = _DEF_GPIO_STS(CARD_DET),
+	[5]  = _DEF_GPIO_STS(MSATA_IND),
+	[6]  = _DEF_GPIO_STS(USB30_OVC),
+	[7]  = _DEF_GPIO_STS(USB31_OVC),
+	[8]  = _DEF_GPIO_CTL(USB30_PWRON),
+	[9]  = _DEF_GPIO_CTL(USB31_PWRON),
+
+	/* brightness changed interrupt, no GPIO */
+	[11] = _DEF_INT(BRIGHTNESS_CHANGED),
+
+	[12] = _DEF_GPIO_STS(BUTTON_PRESSED),
+
+	/* TRNG interrupt, no GPIO */
+	[13] = _DEF_INT(TRNG),
+
+	/* MESSAGE_SIGNED interrupt, no GPIO */
+	[14] = _DEF_INT(MESSAGE_SIGNED),
+
+	/* GPIOs with value read from the 32-bit wide extended status */
+	[16] = _DEF_GPIO_EXT_STS(SFP_nDET, PERIPH_MCU),
+	[28] = _DEF_GPIO_EXT_STS_LEDALL(WLAN0_MSATA_LED),
+	[29] = _DEF_GPIO_EXT_STS_LEDALL(WLAN1_LED),
+	[30] = _DEF_GPIO_EXT_STS_LEDALL(WLAN2_LED),
+	[31] = _DEF_GPIO_EXT_STS_LED(WPAN0_LED, EXT),
+	[32] = _DEF_GPIO_EXT_STS_LED(WPAN1_LED, EXT),
+	[33] = _DEF_GPIO_EXT_STS_LED(WPAN2_LED, EXT),
+	[34] = _DEF_GPIO_EXT_STS_LEDALL(WAN_LED0),
+	[35] = _DEF_GPIO_EXT_STS_LED(WAN_LED1, EXT_V32),
+	[36] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED0),
+	[37] = _DEF_GPIO_EXT_STS_LEDALL(LAN0_LED1),
+	[38] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED0),
+	[39] = _DEF_GPIO_EXT_STS_LEDALL(LAN1_LED1),
+	[40] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED0),
+	[41] = _DEF_GPIO_EXT_STS_LEDALL(LAN2_LED1),
+	[42] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED0),
+	[43] = _DEF_GPIO_EXT_STS_LEDALL(LAN3_LED1),
+	[44] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED0),
+	[45] = _DEF_GPIO_EXT_STS_LEDALL(LAN4_LED1),
+	[46] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED0),
+	[47] = _DEF_GPIO_EXT_STS_LEDALL(LAN5_LED1),
+
+	/* GPIOs with value read from the 16-bit wide extended control status */
+	[48] = _DEF_GPIO_EXT_CTL(nRES_MMC, PERIPH_MCU),
+	[49] = _DEF_GPIO_EXT_CTL(nRES_LAN, PERIPH_MCU),
+	[50] = _DEF_GPIO_EXT_CTL(nRES_PHY, PERIPH_MCU),
+	[51] = _DEF_GPIO_EXT_CTL(nPERST0, PERIPH_MCU),
+	[52] = _DEF_GPIO_EXT_CTL(nPERST1, PERIPH_MCU),
+	[53] = _DEF_GPIO_EXT_CTL(nPERST2, PERIPH_MCU),
+	[54] = _DEF_GPIO_EXT_CTL(PHY_SFP, PERIPH_MCU),
+};
+
+/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
+static const u8 omnia_int_to_gpio_idx[32] = {
+	[__bf_shf(INT_CARD_DET)]		= 4,
+	[__bf_shf(INT_MSATA_IND)]		= 5,
+	[__bf_shf(INT_USB30_OVC)]		= 6,
+	[__bf_shf(INT_USB31_OVC)]		= 7,
+	[__bf_shf(INT_BUTTON_PRESSED)]		= 12,
+	[__bf_shf(INT_TRNG)]			= 13,
+	[__bf_shf(INT_MESSAGE_SIGNED)]		= 14,
+	[__bf_shf(INT_SFP_nDET)]		= 16,
+	[__bf_shf(INT_BRIGHTNESS_CHANGED)]	= 11,
+	[__bf_shf(INT_WLAN0_MSATA_LED)]		= 28,
+	[__bf_shf(INT_WLAN1_LED)]		= 29,
+	[__bf_shf(INT_WLAN2_LED)]		= 30,
+	[__bf_shf(INT_WPAN0_LED)]		= 31,
+	[__bf_shf(INT_WPAN1_LED)]		= 32,
+	[__bf_shf(INT_WPAN2_LED)]		= 33,
+	[__bf_shf(INT_WAN_LED0)]		= 34,
+	[__bf_shf(INT_WAN_LED1)]		= 35,
+	[__bf_shf(INT_LAN0_LED0)]		= 36,
+	[__bf_shf(INT_LAN0_LED1)]		= 37,
+	[__bf_shf(INT_LAN1_LED0)]		= 38,
+	[__bf_shf(INT_LAN1_LED1)]		= 39,
+	[__bf_shf(INT_LAN2_LED0)]		= 40,
+	[__bf_shf(INT_LAN2_LED1)]		= 41,
+	[__bf_shf(INT_LAN3_LED0)]		= 42,
+	[__bf_shf(INT_LAN3_LED1)]		= 43,
+	[__bf_shf(INT_LAN4_LED0)]		= 44,
+	[__bf_shf(INT_LAN4_LED1)]		= 45,
+	[__bf_shf(INT_LAN5_LED0)]		= 46,
+	[__bf_shf(INT_LAN5_LED1)]		= 47,
+};
+
+/* index of PHY_SFP GPIO in the omnia_gpios array */
+#define OMNIA_GPIO_PHY_SFP_OFFSET	54
+
+static int omnia_ctl_cmd_locked(struct omnia_mcu *mcu, u8 cmd, u16 val,
+				u16 mask)
+{
+	size_t len;
+	u8 buf[5];
+
+	buf[0] = cmd;
+
+	switch (cmd) {
+	case CMD_GENERAL_CONTROL:
+		buf[1] = val;
+		buf[2] = mask;
+		len = 3;
+		break;
+
+	case CMD_EXT_CONTROL:
+		put_unaligned_le16(val, &buf[1]);
+		put_unaligned_le16(mask, &buf[3]);
+		len = 5;
+		break;
+
+	default:
+		BUG();
+	}
+
+	return omnia_cmd_write(mcu->client, buf, len);
+}
+
+static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask)
+{
+	guard(mutex)(&mcu->lock);
+
+	return omnia_ctl_cmd_locked(mcu, cmd, val, mask);
+}
+
+static int omnia_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	if (!omnia_gpios[offset].cmd)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int omnia_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) {
+		int val;
+
+		scoped_guard(mutex, &mcu->lock)
+			val = omnia_cmd_read_bit(mcu->client,
+						 CMD_GET_EXT_CONTROL_STATUS,
+						 EXT_CTL_PHY_SFP_AUTO);
+
+		if (val < 0)
+			return val;
+
+		if (val)
+			return GPIO_LINE_DIRECTION_IN;
+
+		return GPIO_LINE_DIRECTION_OUT;
+	}
+
+	if (omnia_gpios[offset].ctl_cmd)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static int omnia_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	const struct omnia_gpio *gpio = &omnia_gpios[offset];
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
+		return omnia_ctl_cmd(mcu, CMD_EXT_CONTROL, EXT_CTL_PHY_SFP_AUTO,
+				     EXT_CTL_PHY_SFP_AUTO);
+
+	if (gpio->ctl_cmd)
+		return -ENOTSUPP;
+
+	return 0;
+}
+
+static int omnia_gpio_direction_output(struct gpio_chip *gc,
+				       unsigned int offset, int value)
+{
+	const struct omnia_gpio *gpio = &omnia_gpios[offset];
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	u16 val, mask;
+
+	if (!gpio->ctl_cmd)
+		return -ENOTSUPP;
+
+	mask = gpio->ctl_bit;
+	val = value ? mask : 0;
+
+	if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
+		mask |= EXT_CTL_PHY_SFP_AUTO;
+
+	return omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
+}
+
+static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	const struct omnia_gpio *gpio = &omnia_gpios[offset];
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	/*
+	 * If firmware does not support the new interrupt API, we are informed
+	 * of every change of the status word by an interrupt from MCU and save
+	 * its value in the interrupt service routine. Simply return the saved
+	 * value.
+	 */
+	if (gpio->cmd == CMD_GET_STATUS_WORD &&
+	    !(mcu->features & FEAT_NEW_INT_API))
+		return !!(mcu->last_status & gpio->bit);
+
+	guard(mutex)(&mcu->lock);
+
+	/*
+	 * If firmware does support the new interrupt API, we may have cached
+	 * the value of a GPIO in the interrupt service routine. If not, read
+	 * the relevant bit now.
+	 */
+	if (gpio->int_bit && (mcu->is_cached & gpio->int_bit))
+		return !!(mcu->cached & gpio->int_bit);
+
+	return omnia_cmd_read_bit(mcu->client, gpio->cmd, gpio->bit);
+}
+
+static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+				   unsigned long *bits)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	u32 sts_bits, ext_sts_bits, ext_ctl_bits;
+	int err, i;
+
+	sts_bits = 0;
+	ext_sts_bits = 0;
+	ext_ctl_bits = 0;
+
+	/* determine which bits to read from the 3 possible commands */
+	for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+		if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
+			sts_bits |= omnia_gpios[i].bit;
+		else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
+			ext_sts_bits |= omnia_gpios[i].bit;
+		else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
+			ext_ctl_bits |= omnia_gpios[i].bit;
+	}
+
+	guard(mutex)(&mcu->lock);
+
+	if (mcu->features & FEAT_NEW_INT_API) {
+		/* read relevant bits from status */
+		err = omnia_cmd_read_bits(mcu->client, CMD_GET_STATUS_WORD,
+					  sts_bits, &sts_bits);
+		if (err)
+			return err;
+	} else {
+		/*
+		 * Use status word value cached in the interrupt service routine
+		 * if firmware does not support the new interrupt API.
+		 */
+		sts_bits = mcu->last_status;
+	}
+
+	/* read relevant bits from extended status */
+	err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_STATUS_DWORD,
+				  ext_sts_bits, &ext_sts_bits);
+	if (err)
+		return err;
+
+	/* read relevant bits from extended control */
+	err = omnia_cmd_read_bits(mcu->client, CMD_GET_EXT_CONTROL_STATUS,
+				  ext_ctl_bits, &ext_ctl_bits);
+	if (err)
+		return err;
+
+	/* assign relevant bits in result */
+	for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+		if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
+			__assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
+		else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
+			__assign_bit(i, bits, ext_sts_bits &
+					      omnia_gpios[i].bit);
+		else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
+			__assign_bit(i, bits, ext_ctl_bits &
+					      omnia_gpios[i].bit);
+	}
+
+	return 0;
+}
+
+static void omnia_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	const struct omnia_gpio *gpio = &omnia_gpios[offset];
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	u16 val, mask;
+
+	if (!gpio->ctl_cmd)
+		return;
+
+	mask = gpio->ctl_bit;
+	val = value ? mask : 0;
+
+	omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
+}
+
+static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	u16 ext_ctl, ext_ctl_mask;
+	u8 ctl, ctl_mask;
+	int i;
+
+	ctl = 0;
+	ctl_mask = 0;
+	ext_ctl = 0;
+	ext_ctl_mask = 0;
+
+	for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+		if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) {
+			ctl_mask |= omnia_gpios[i].ctl_bit;
+			if (test_bit(i, bits))
+				ctl |= omnia_gpios[i].ctl_bit;
+		} else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) {
+			ext_ctl_mask |= omnia_gpios[i].ctl_bit;
+			if (test_bit(i, bits))
+				ext_ctl |= omnia_gpios[i].ctl_bit;
+		}
+	}
+
+	guard(mutex)(&mcu->lock);
+
+	if (ctl_mask)
+		omnia_ctl_cmd_locked(mcu, CMD_GENERAL_CONTROL, ctl, ctl_mask);
+
+	if (ext_ctl_mask)
+		omnia_ctl_cmd_locked(mcu, CMD_EXT_CONTROL, ext_ctl,
+				     ext_ctl_mask);
+}
+
+static bool omnia_gpio_available(struct omnia_mcu *mcu,
+				 const struct omnia_gpio *gpio)
+{
+	if (gpio->feat_mask)
+		return (mcu->features & gpio->feat_mask) == gpio->feat;
+
+	if (gpio->feat)
+		return mcu->features & gpio->feat;
+
+	return true;
+}
+
+static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
+				      unsigned long *valid_mask,
+				      unsigned int ngpios)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	for (unsigned int i = 0; i < ngpios; i++) {
+		const struct omnia_gpio *gpio = &omnia_gpios[i];
+
+		if (gpio->cmd || gpio->int_bit)
+			__assign_bit(i, valid_mask,
+				     omnia_gpio_available(mcu, gpio));
+		else
+			__clear_bit(i, valid_mask);
+	}
+
+	return 0;
+}
+
+static int omnia_gpio_of_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec,
+			       u32 *flags)
+{
+	u32 bank, gpio;
+
+	if (WARN_ON(gpiospec->args_count != 3))
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	bank = gpiospec->args[0];
+	gpio = gpiospec->args[1];
+
+	switch (bank) {
+	case 0:
+		return gpio < 16 ? gpio : -EINVAL;
+	case 1:
+		return gpio < 32 ? 16 + gpio : -EINVAL;
+	case 2:
+		return gpio < 16 ? 48 + gpio : -EINVAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static void omnia_irq_shutdown(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 bit = omnia_gpios[hwirq].int_bit;
+
+	mcu->rising &= ~bit;
+	mcu->falling &= ~bit;
+}
+
+static void omnia_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 bit = omnia_gpios[hwirq].int_bit;
+
+	if (!omnia_gpios[hwirq].cmd)
+		mcu->rising &= ~bit;
+	mcu->mask &= ~bit;
+	gpiochip_disable_irq(gc, hwirq);
+}
+
+static void omnia_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 bit = omnia_gpios[hwirq].int_bit;
+
+	gpiochip_enable_irq(gc, hwirq);
+	mcu->mask |= bit;
+	if (!omnia_gpios[hwirq].cmd)
+		mcu->rising |= bit;
+}
+
+static int omnia_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct device *dev = &mcu->client->dev;
+	u32 bit = omnia_gpios[hwirq].int_bit;
+
+	if (!(type & IRQ_TYPE_EDGE_BOTH)) {
+		dev_err(dev, "irq %u: unsupported type %u\n", d->irq, type);
+		return -EINVAL;
+	}
+
+	if (type & IRQ_TYPE_EDGE_RISING)
+		mcu->rising |= bit;
+	else
+		mcu->rising &= ~bit;
+
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		mcu->falling |= bit;
+	else
+		mcu->falling &= ~bit;
+
+	return 0;
+}
+
+static void omnia_irq_bus_lock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	/* nothing to do if MCU firmware does not support new interrupt API */
+	if (!(mcu->features & FEAT_NEW_INT_API))
+		return;
+
+	mutex_lock(&mcu->lock);
+}
+
+/**
+ * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
+ *	@dst: the destination u8 array of interleaved bytes
+ *	@rising: rising mask
+ *	@falling: falling mask
+ *
+ * Interleaves the little-endian bytes from @rising and @falling words.
+ *
+ * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
+ * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
+ *
+ * The MCU receives interrupt mask and reports pending interrupt bitmap int this
+ * interleaved format. The rationale behind it is that the low-indexed bits are
+ * more important - in many cases, the user will be interested only in
+ * interrupts with indexes 0 to 7, and so the system can stop reading after
+ * first 2 bytes (r0, f0), to save time on the slow I2C bus.
+ *
+ * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
+ * and use an appropriate bitmap_* function once such a function exists.
+ */
+static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling)
+{
+	for (int i = 0; i < sizeof(u32); ++i) {
+		dst[2 * i] = rising >> (8 * i);
+		dst[2 * i + 1] = falling >> (8 * i);
+	}
+}
+
+/**
+ * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling
+ *	@src: the source u8 array containing the interleaved bytes
+ *	@rising: pointer where to store the rising mask gathered from @src
+ *	@falling: pointer where to store the falling mask gathered from @src
+ *
+ * This is the inverse function to omnia_mask_interleave.
+ */
+static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling)
+{
+	*rising = *falling = 0;
+
+	for (int i = 0; i < sizeof(u32); ++i) {
+		*rising |= src[2 * i] << (8 * i);
+		*falling |= src[2 * i + 1] << (8 * i);
+	}
+}
+
+static void omnia_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	struct device *dev = &mcu->client->dev;
+	u8 cmd[1 + CMD_INT_ARG_LEN];
+	u32 rising, falling;
+	int err;
+
+	/* nothing to do if MCU firmware does not support new interrupt API */
+	if (!(mcu->features & FEAT_NEW_INT_API))
+		return;
+
+	cmd[0] = CMD_SET_INT_MASK;
+
+	rising = mcu->rising & mcu->mask;
+	falling = mcu->falling & mcu->mask;
+
+	/* interleave the rising and falling bytes into the command arguments */
+	omnia_mask_interleave(&cmd[1], rising, falling);
+
+	dev_dbg(dev, "set int mask %8ph\n", &cmd[1]);
+
+	err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+	if (err) {
+		dev_err(dev, "Cannot set mask: %d\n", err);
+		goto unlock;
+	}
+
+	/*
+	 * Remember which GPIOs have both rising and falling interrupts enabled.
+	 * For those we will cache their value so that .get() method is faster.
+	 * We also need to forget cached values of GPIOs that aren't cached
+	 * anymore.
+	 */
+	mcu->both = rising & falling;
+	mcu->is_cached &= mcu->both;
+
+unlock:
+	mutex_unlock(&mcu->lock);
+}
+
+static const struct irq_chip omnia_mcu_irq_chip = {
+	.name			= "Turris Omnia MCU interrupts",
+	.irq_shutdown		= omnia_irq_shutdown,
+	.irq_mask		= omnia_irq_mask,
+	.irq_unmask		= omnia_irq_unmask,
+	.irq_set_type		= omnia_irq_set_type,
+	.irq_bus_lock		= omnia_irq_bus_lock,
+	.irq_bus_sync_unlock	= omnia_irq_bus_sync_unlock,
+	.flags			= IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
+				      unsigned long *valid_mask,
+				      unsigned int ngpios)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+
+	for (unsigned int i = 0; i < ngpios; i++) {
+		const struct omnia_gpio *gpio = &omnia_gpios[i];
+
+		if (gpio->int_bit)
+			__assign_bit(i, valid_mask,
+				     omnia_gpio_available(mcu, gpio));
+		else
+			__clear_bit(i, valid_mask);
+	}
+}
+
+static int omnia_irq_init_hw(struct gpio_chip *gc)
+{
+	struct omnia_mcu *mcu = gpiochip_get_data(gc);
+	u8 cmd[1 + CMD_INT_ARG_LEN] = {};
+
+	cmd[0] = CMD_SET_INT_MASK;
+
+	return omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+}
+
+/*
+ * Determine how many bytes we need to read from the reply to the
+ * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked interrupts.
+ */
+static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling)
+{
+	size_t rlen = 0, flen = 0;
+
+	if (rising)
+		rlen = ((__fls(rising) >> 3) << 1) + 1;
+
+	if (falling)
+		flen = ((__fls(falling) >> 3) << 1) + 2;
+
+	return max(rlen, flen);
+}
+
+static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
+				       unsigned long *pending)
+{
+	struct device *dev = &mcu->client->dev;
+	u8 reply[CMD_INT_ARG_LEN] = {};
+	u32 rising, falling;
+	size_t len;
+	int err;
+
+	len = omnia_irq_compute_pending_length(mcu->rising & mcu->mask,
+					       mcu->falling & mcu->mask);
+	if (!len)
+		return false;
+
+	guard(mutex)(&mcu->lock);
+
+	err = omnia_cmd_read(mcu->client, CMD_GET_INT_AND_CLEAR, reply,
+			     len);
+	if (err) {
+		dev_err(dev, "Cannot read pending IRQs: %d\n", err);
+		return false;
+	}
+
+	/* deinterleave the reply bytes into rising and falling */
+	omnia_mask_deinterleave(reply, &rising, &falling);
+
+	rising &= mcu->mask;
+	falling &= mcu->mask;
+	*pending = rising | falling;
+
+	/* cache values for GPIOs that have both edges enabled */
+	mcu->is_cached &= ~(rising & falling);
+	mcu->is_cached |= mcu->both & (rising ^ falling);
+	mcu->cached = (mcu->cached | rising) & ~falling;
+
+	return true;
+}
+
+static int omnia_read_status_word_old_fw(struct omnia_mcu *mcu, u16 *status)
+{
+	int ret;
+
+	ret = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Old firmware has a bug wherein it never resets the USB port
+	 * overcurrent bits back to zero. Ignore them.
+	 */
+	*status = ret & ~(STS_USB30_OVC | STS_USB31_OVC);
+
+	return 0;
+}
+
+static void button_release_emul_fn(struct work_struct *work)
+{
+	struct omnia_mcu *mcu = container_of(to_delayed_work(work),
+					     struct omnia_mcu,
+					     button_release_emul_work);
+
+	mcu->button_pressed_emul = false;
+	generic_handle_irq_safe(mcu->client->irq);
+}
+
+static void fill_int_from_sts(u32 *rising, u32 *falling, u16 rising_sts,
+			      u16 falling_sts, u16 sts_bit, u32 int_bit)
+{
+	if (rising_sts & sts_bit)
+		*rising |= int_bit;
+	if (falling_sts & sts_bit)
+		*falling |= int_bit;
+}
+
+static bool omnia_irq_read_pending_old(struct omnia_mcu *mcu,
+				       unsigned long *pending)
+{
+	struct device *dev = &mcu->client->dev;
+	u16 status, rising_sts, falling_sts;
+	u32 rising, falling;
+	int ret;
+
+	guard(mutex)(&mcu->lock);
+
+	ret = omnia_read_status_word_old_fw(mcu, &status);
+	if (ret < 0) {
+		dev_err(dev, "Cannot read pending IRQs: %d\n", ret);
+		return false;
+	}
+
+	/*
+	 * The old firmware triggers an interrupt whenever status word changes,
+	 * but does not inform about which bits rose or fell. We need to compute
+	 * this here by comparing with the last status word value.
+	 *
+	 * The STS_BUTTON_PRESSED bit needs special handling, because the old
+	 * firmware clears the STS_BUTTON_PRESSED bit on successful completion
+	 * of the CMD_GET_STATUS_WORD command, resulting in another interrupt:
+	 * - first we get an interrupt, we read the status word where
+	 *   STS_BUTTON_PRESSED is present,
+	 * - MCU clears the STS_BUTTON_PRESSED bit because we read the status
+	 *   word,
+	 * - we get another interrupt because the status word changed again
+	 *   (STS_BUTTON_PRESSED was cleared).
+	 *
+	 * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call
+	 * the gpiochip's .get() method after an edge event on a requested GPIO
+	 * occurs.
+	 *
+	 * We ensure that the .get() method reads 1 for the button GPIO for some
+	 * time.
+	 */
+
+	if (status & STS_BUTTON_PRESSED) {
+		mcu->button_pressed_emul = true;
+		mod_delayed_work(system_wq, &mcu->button_release_emul_work,
+				 msecs_to_jiffies(FRONT_BUTTON_RELEASE_DELAY));
+	} else if (mcu->button_pressed_emul) {
+		status |= STS_BUTTON_PRESSED;
+	}
+
+	rising_sts = ~mcu->last_status & status;
+	falling_sts = mcu->last_status & ~status;
+
+	mcu->last_status = status;
+
+	/*
+	 * Fill in the relevant interrupt bits from status bits for CARD_DET,
+	 * MSATA_IND and BUTTON_PRESSED.
+	 */
+	rising = 0;
+	falling = 0;
+	fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+			  STS_CARD_DET, INT_CARD_DET);
+	fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+			  STS_MSATA_IND, INT_MSATA_IND);
+	fill_int_from_sts(&rising, &falling, rising_sts, falling_sts,
+			  STS_BUTTON_PRESSED, INT_BUTTON_PRESSED);
+
+	/* Use only bits that are enabled */
+	rising &= mcu->rising & mcu->mask;
+	falling &= mcu->falling & mcu->mask;
+	*pending = rising | falling;
+
+	return true;
+}
+
+static bool omnia_irq_read_pending(struct omnia_mcu *mcu,
+				   unsigned long *pending)
+{
+	if (mcu->features & FEAT_NEW_INT_API)
+		return omnia_irq_read_pending_new(mcu, pending);
+	else
+		return omnia_irq_read_pending_old(mcu, pending);
+}
+
+static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id)
+{
+	struct omnia_mcu *mcu = dev_id;
+	struct irq_domain *domain;
+	unsigned long pending;
+	int i;
+
+	if (!omnia_irq_read_pending(mcu, &pending))
+		return IRQ_NONE;
+
+	domain = mcu->gc.irq.domain;
+
+	for_each_set_bit(i, &pending, 32) {
+		unsigned int nested_irq;
+
+		nested_irq = irq_find_mapping(domain, omnia_int_to_gpio_idx[i]);
+
+		handle_nested_irq(nested_irq);
+	}
+
+	return IRQ_RETVAL(pending);
+}
+
+static const char * const front_button_modes[2] = { "mcu", "cpu" };
+
+static ssize_t front_button_mode_show(struct device *dev,
+				      struct device_attribute *a, char *buf)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+	int val;
+
+	if (mcu->features & FEAT_NEW_INT_API) {
+		val = omnia_cmd_read_bit(mcu->client, CMD_GET_STATUS_WORD,
+					 STS_BUTTON_MODE);
+		if (val < 0)
+			return val;
+	} else {
+		val = !!(mcu->last_status & STS_BUTTON_MODE);
+	}
+
+	return sysfs_emit(buf, "%s\n", front_button_modes[val]);
+}
+
+static ssize_t front_button_mode_store(struct device *dev,
+				       struct device_attribute *a,
+				       const char *buf, size_t count)
+{
+	struct omnia_mcu *mcu = dev_get_drvdata(dev);
+	int err, i;
+
+	i = sysfs_match_string(front_button_modes, buf);
+	if (i < 0)
+		return i;
+
+	err = omnia_ctl_cmd_locked(mcu, CMD_GENERAL_CONTROL,
+				   i ? CTL_BUTTON_MODE : 0,
+				   CTL_BUTTON_MODE);
+	if (err)
+		return err;
+
+	return count;
+}
+static DEVICE_ATTR_RW(front_button_mode);
+
+static struct attribute *omnia_mcu_gpio_attrs[] = {
+	&dev_attr_front_button_mode.attr,
+	NULL
+};
+
+const struct attribute_group omnia_mcu_gpio_group = {
+	.attrs = omnia_mcu_gpio_attrs,
+};
+
+int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu)
+{
+	bool new_api = mcu->features & FEAT_NEW_INT_API;
+	struct device *dev = &mcu->client->dev;
+	unsigned long irqflags;
+	int err;
+
+	err = devm_mutex_init(dev, &mcu->lock);
+	if (err)
+		return err;
+
+	mcu->gc.request = omnia_gpio_request;
+	mcu->gc.get_direction = omnia_gpio_get_direction;
+	mcu->gc.direction_input = omnia_gpio_direction_input;
+	mcu->gc.direction_output = omnia_gpio_direction_output;
+	mcu->gc.get = omnia_gpio_get;
+	mcu->gc.get_multiple = omnia_gpio_get_multiple;
+	mcu->gc.set = omnia_gpio_set;
+	mcu->gc.set_multiple = omnia_gpio_set_multiple;
+	mcu->gc.init_valid_mask = omnia_gpio_init_valid_mask;
+	mcu->gc.can_sleep = true;
+	mcu->gc.names = omnia_mcu_gpio_templates;
+	mcu->gc.base = -1;
+	mcu->gc.ngpio = ARRAY_SIZE(omnia_gpios);
+	mcu->gc.label = "Turris Omnia MCU GPIOs";
+	mcu->gc.parent = dev;
+	mcu->gc.owner = THIS_MODULE;
+	mcu->gc.of_gpio_n_cells = 3;
+	mcu->gc.of_xlate = omnia_gpio_of_xlate;
+
+	gpio_irq_chip_set_chip(&mcu->gc.irq, &omnia_mcu_irq_chip);
+	/* This will let us handle the parent IRQ in the driver */
+	mcu->gc.irq.parent_handler = NULL;
+	mcu->gc.irq.num_parents = 0;
+	mcu->gc.irq.parents = NULL;
+	mcu->gc.irq.default_type = IRQ_TYPE_NONE;
+	mcu->gc.irq.handler = handle_bad_irq;
+	mcu->gc.irq.threaded = true;
+	if (new_api)
+		mcu->gc.irq.init_hw = omnia_irq_init_hw;
+	mcu->gc.irq.init_valid_mask = omnia_irq_init_valid_mask;
+
+	err = devm_gpiochip_add_data(dev, &mcu->gc, mcu);
+	if (err)
+		return dev_err_probe(dev, err, "Cannot add GPIO chip\n");
+
+	/*
+	 * Before requesting the interrupt, if firmware does not support the new
+	 * interrupt API, we need to cache the value of the status word, so that
+	 * when it changes, we may compare the new value with the cached one in
+	 * the interrupt handler.
+	 */
+	if (!new_api) {
+		err = omnia_read_status_word_old_fw(mcu, &mcu->last_status);
+		if (err < 0)
+			return dev_err_probe(dev, err,
+					     "Cannot read status word\n");
+
+		INIT_DELAYED_WORK(&mcu->button_release_emul_work,
+				  button_release_emul_fn);
+	}
+
+	irqflags = IRQF_ONESHOT;
+	if (new_api)
+		irqflags |= IRQF_TRIGGER_LOW;
+	else
+		irqflags |= IRQF_TRIGGER_FALLING;
+
+	err = devm_request_threaded_irq(dev, mcu->client->irq, NULL,
+					omnia_irq_thread_handler, irqflags,
+					"turris-omnia-mcu", mcu);
+	if (err)
+		return dev_err_probe(dev, err, "Cannot request IRQ\n");
+
+	if (!new_api) {
+		/*
+		 * The button_release_emul_work has to be initialized before the
+		 * thread is requested, and on driver remove it needs to be
+		 * canceled before the thread is freed. Therefore we can't use
+		 * devm_delayed_work_autocancel() directly, because the order
+		 *   devm_delayed_work_autocancel();
+		 *   devm_request_threaded_irq();
+		 * would cause improper release order:
+		 *   free_irq();
+		 *   cancel_delayed_work_sync();
+		 * Instead we first initialize the work above, and only now
+		 * after IRQ is requested we add the work devm action.
+		 */
+		err = devm_add_action(dev, devm_delayed_work_drop,
+				      &mcu->button_release_emul_work);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index 5b21fbca8fe1..e8d9bf1c8fdc 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -8,9 +8,12 @@
 #ifndef __TURRIS_OMNIA_MCU_H
 #define __TURRIS_OMNIA_MCU_H
 
-#include <linux/i2c.h>
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
 #include <linux/if_ether.h>
+#include <linux/mutex.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 #include <asm/byteorder.h>
 
 struct omnia_mcu {
@@ -22,10 +25,66 @@ struct omnia_mcu {
 	u64 board_serial_number;
 	u8 board_first_mac[ETH_ALEN];
 	u8 board_revision;
+
+	/* GPIO chip */
+	struct gpio_chip gc;
+	struct mutex lock;
+	u32 mask, rising, falling, both, cached, is_cached;
+	/* Old MCU firmware handling needs the following */
+	struct delayed_work button_release_emul_work;
+	u16 last_status;
+	bool button_pressed_emul;
 };
 
-int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
-		   unsigned int len);
+int omnia_cmd_write_read(const struct i2c_client *client,
+			 void *cmd, unsigned int cmd_len,
+			 void *reply, unsigned int reply_len);
+
+static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
+				  size_t len)
+{
+	return omnia_cmd_write_read(client, cmd, len, NULL, 0);
+}
+
+static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
+				 void *reply, unsigned int len)
+{
+	return omnia_cmd_write_read(client, &cmd, 1, reply, len);
+}
+
+/* Returns 0 on success */
+static inline int omnia_cmd_read_bits(const struct i2c_client *client, u8 cmd,
+				      u32 bits, u32 *dst)
+{
+	__le32 reply;
+	int err;
+
+	if (!bits) {
+		*dst = 0;
+		return 0;
+	}
+
+	err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
+	if (err)
+		return err;
+
+	*dst = le32_to_cpu(reply) & bits;
+
+	return 0;
+}
+
+static inline int omnia_cmd_read_bit(const struct i2c_client *client, u8 cmd,
+				     u32 bit)
+{
+	u32 reply;
+	int err;
+
+	err = omnia_cmd_read_bits(client, cmd, bit, &reply);
+	if (err)
+		return err;
+
+	return !!reply;
+}
 
 static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
 {
@@ -47,4 +106,8 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
 	return err ?: reply;
 }
 
+extern const struct attribute_group omnia_mcu_gpio_group;
+
+int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
+
 #endif /* __TURRIS_OMNIA_MCU_H */
-- 
2.43.2

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
>
> Add support for GPIOs connected to the MCU on the Turris Omnia board.
>
> This includes:
> - front button pin
> - enable pins for USB regulators
> - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> - on board revisions 32+ also various peripheral resets and another
>   voltage regulator enable pin

...

> -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
> -                  unsigned int len)
> +int omnia_cmd_write_read(const struct i2c_client *client,
> +                        void *cmd, unsigned int cmd_len,
> +                        void *reply, unsigned int reply_len)
>  {
>         struct i2c_msg msgs[2];
> -       int ret;
> +       int ret, num;
>
>         msgs[0].addr = client->addr;
>         msgs[0].flags = 0;
> -       msgs[0].len = 1;
> -       msgs[0].buf = &cmd;
> -       msgs[1].addr = client->addr;
> -       msgs[1].flags = I2C_M_RD;
> -       msgs[1].len = len;
> -       msgs[1].buf = reply;
> -
> -       ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +       msgs[0].len = cmd_len;
> +       msgs[0].buf = cmd;
> +       num = 1;
> +
> +       if (reply_len) {
> +               msgs[1].addr = client->addr;
> +               msgs[1].flags = I2C_M_RD;
> +               msgs[1].len = reply_len;
> +               msgs[1].buf = reply;
> +               num++;
> +       }
> +
> +       ret = i2c_transfer(client->adapter, msgs, num);
>         if (ret < 0)
>                 return ret;
> -       if (ret != ARRAY_SIZE(msgs))
> +       if (ret != num)
>                 return -EIO;
>
>         return 0;

Hold on, you are "patching" the code you just brought by the previous
patch?! No, just do from the beginning how it should be at the end.

...

> +#include <linux/devm-helpers.h>

Do you still need this?

...

> +#define FRONT_BUTTON_RELEASE_DELAY     50 /* ms */

Use proper unit suffix instead of a comment like many others do.

_MS here.

...

> +static const char * const omnia_mcu_gpio_templates[64] = {
> +       /* GPIOs with value read from the 16-bit wide status */
> +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> +       [6]  = "gpio%u.Front USB3 port over-current",
> +       [7]  = "gpio%u.Rear USB3 port over-current",
> +       [8]  = "gpio%u.Front USB3 port power",
> +       [9]  = "gpio%u.Rear USB3 port power",
> +       [12] = "gpio%u.Front Button",
> +
> +       /* GPIOs with value read from the 32-bit wide extended status */
> +       [16] = "gpio%u.SFP nDET",
> +       [28] = "gpio%u.MiniPCIe0 LED",
> +       [29] = "gpio%u.MiniPCIe1 LED",
> +       [30] = "gpio%u.MiniPCIe2 LED",
> +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> +       [34] = "gpio%u.WAN PHY LED0",
> +       [35] = "gpio%u.WAN PHY LED1",
> +       [36] = "gpio%u.LAN switch p0 LED0",
> +       [37] = "gpio%u.LAN switch p0 LED1",
> +       [38] = "gpio%u.LAN switch p1 LED0",
> +       [39] = "gpio%u.LAN switch p1 LED1",
> +       [40] = "gpio%u.LAN switch p2 LED0",
> +       [41] = "gpio%u.LAN switch p2 LED1",
> +       [42] = "gpio%u.LAN switch p3 LED0",
> +       [43] = "gpio%u.LAN switch p3 LED1",
> +       [44] = "gpio%u.LAN switch p4 LED0",
> +       [45] = "gpio%u.LAN switch p4 LED1",
> +       [46] = "gpio%u.LAN switch p5 LED0",
> +       [47] = "gpio%u.LAN switch p5 LED1",
> +
> +       /* GPIOs with value read from the 16-bit wide extended control status */
> +       [48] = "gpio%u.eMMC nRESET",
> +       [49] = "gpio%u.LAN switch nRESET",
> +       [50] = "gpio%u.WAN PHY nRESET",
> +       [51] = "gpio%u.MiniPCIe0 nPERST",
> +       [52] = "gpio%u.MiniPCIe1 nPERST",
> +       [53] = "gpio%u.MiniPCIe2 nPERST",
> +       [54] = "gpio%u.WAN PHY SFP mux",
> +};

You may reduce the memory footprint of these just by doing "gpio%u."
part(s) outside. Here compiler won't compress this (as in the case of
repetitive string literals),

...

> +static const struct omnia_gpio {
> +       u8 cmd, ctl_cmd;
> +       u32 bit, ctl_bit;
> +       u32 int_bit;
> +       u16 feat, feat_mask;
> +} omnia_gpios[64] = {

It's much better to decouple definition and assignment and put
definition _before_ the macros that fill it.

> +};

...

> +               scoped_guard(mutex, &mcu->lock)
> +                       val = omnia_cmd_read_bit(mcu->client,
> +                                                CMD_GET_EXT_CONTROL_STATUS,
> +                                                EXT_CTL_PHY_SFP_AUTO);
> +
> +               if (val < 0)
> +                       return val;

I would move that under guard  for the sake of better readability
(usual pattern in place). It's anyway a slow path and one branch under
the mutex won't affect anything.

> +               if (val)
> +                       return GPIO_LINE_DIRECTION_IN;
> +
> +               return GPIO_LINE_DIRECTION_OUT;

...

> +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +       u32 sts_bits, ext_sts_bits, ext_ctl_bits;
> +       int err, i;

> +       sts_bits = 0;
> +       ext_sts_bits = 0;
> +       ext_ctl_bits = 0;

Why not assign these inside the definition line?

...

> +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> +               if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
> +                       __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
> +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
> +                       __assign_bit(i, bits, ext_sts_bits &
> +                                             omnia_gpios[i].bit);

One line?

> +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
> +                       __assign_bit(i, bits, ext_ctl_bits &
> +                                             omnia_gpios[i].bit);

Ditto?

> +       }

...

> +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +       u16 ext_ctl, ext_ctl_mask;
> +       u8 ctl, ctl_mask;
> +       int i;
> +
> +       ctl = 0;
> +       ctl_mask = 0;
> +       ext_ctl = 0;
> +       ext_ctl_mask = 0;

Assignments in the definition line?

...

> +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> +               if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) {
> +                       ctl_mask |= omnia_gpios[i].ctl_bit;
> +                       if (test_bit(i, bits))
> +                               ctl |= omnia_gpios[i].ctl_bit;
> +               } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) {
> +                       ext_ctl_mask |= omnia_gpios[i].ctl_bit;
> +                       if (test_bit(i, bits))
> +                               ext_ctl |= omnia_gpios[i].ctl_bit;
> +               }
> +       }

...

> +/**
> + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
> + *     @dst: the destination u8 array of interleaved bytes
> + *     @rising: rising mask
> + *     @falling: falling mask
> + *
> + * Interleaves the little-endian bytes from @rising and @falling words.
> + *
> + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
> + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
> + *
> + * The MCU receives interrupt mask and reports pending interrupt bitmap int this

an interrupt
a pending
int --> in ?

> + * interleaved format. The rationale behind it is that the low-indexed bits are
> + * more important - in many cases, the user will be interested only in
> + * interrupts with indexes 0 to 7, and so the system can stop reading after
> + * first 2 bytes (r0, f0), to save time on the slow I2C bus.
> + *
> + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> + * and use an appropriate bitmap_* function once such a function exists.
> + */
> +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling)
> +{
> +       for (int i = 0; i < sizeof(u32); ++i) {
> +               dst[2 * i] = rising >> (8 * i);
> +               dst[2 * i + 1] = falling >> (8 * i);
> +       }

I will look at this later on,

> +}

> +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling)
> +{
> +       *rising = *falling = 0;
> +
> +       for (int i = 0; i < sizeof(u32); ++i) {
> +               *rising |= src[2 * i] << (8 * i);
> +               *falling |= src[2 * i + 1] << (8 * i);
> +       }
> +}

and into this.

...

> +static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling)
> +{
> +       size_t rlen = 0, flen = 0;
> +
> +       if (rising)
> +               rlen = ((__fls(rising) >> 3) << 1) + 1;
> +
> +       if (falling)
> +               flen = ((__fls(falling) >> 3) << 1) + 2;
> +
> +       return max(rlen, flen);
> +}

I am not sure why you need this, but it can be done easily

x = ((__fls(falling | rising) >> 3) << 1) + 1;
if (falling)
  return x + 1;
return x;

and most likely you can drop minmax.h.

...

> +static const char * const front_button_modes[2] = { "mcu", "cpu" };

2 is not needed.

...

> -#include <linux/i2c.h>

??? That is exactly the point to have things done from the beginning.

> +#include <linux/bitops.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/if_ether.h>
> +#include <linux/mutex.h>
>  #include <linux/types.h>
> +#include <linux/workqueue.h>
>  #include <asm/byteorder.h>

...

> +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);

Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
in a few places.


> +       if (err)
> +               return err;

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > Add support for GPIOs connected to the MCU on the Turris Omnia board.
> >
> > This includes:
> > - front button pin
> > - enable pins for USB regulators
> > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> > - on board revisions 32+ also various peripheral resets and another
> >   voltage regulator enable pin
> 
> ...
> 
> > -int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
> > -                  unsigned int len)
> > +int omnia_cmd_write_read(const struct i2c_client *client,
> > +                        void *cmd, unsigned int cmd_len,
> > +                        void *reply, unsigned int reply_len)
> >  {
> >         struct i2c_msg msgs[2];
> > -       int ret;
> > +       int ret, num;
> >
> >         msgs[0].addr = client->addr;
> >         msgs[0].flags = 0;
> > -       msgs[0].len = 1;
> > -       msgs[0].buf = &cmd;
> > -       msgs[1].addr = client->addr;
> > -       msgs[1].flags = I2C_M_RD;
> > -       msgs[1].len = len;
> > -       msgs[1].buf = reply;
> > -
> > -       ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +       msgs[0].len = cmd_len;
> > +       msgs[0].buf = cmd;
> > +       num = 1;
> > +
> > +       if (reply_len) {
> > +               msgs[1].addr = client->addr;
> > +               msgs[1].flags = I2C_M_RD;
> > +               msgs[1].len = reply_len;
> > +               msgs[1].buf = reply;
> > +               num++;
> > +       }
> > +
> > +       ret = i2c_transfer(client->adapter, msgs, num);
> >         if (ret < 0)
> >                 return ret;
> > -       if (ret != ARRAY_SIZE(msgs))
> > +       if (ret != num)
> >                 return -EIO;
> >
> >         return 0;
> 
> Hold on, you are "patching" the code you just brought by the previous
> patch?! No, just do from the beginning how it should be at the end.

Fixed.

> > +#include <linux/devm-helpers.h>
> 
> Do you still need this?

Yes, for devm_delayed_work_drop().

> ...
> 
> > +#define FRONT_BUTTON_RELEASE_DELAY     50 /* ms */
> 
> Use proper unit suffix instead of a comment like many others do.
> 
> _MS here.

Fixed.

> > +static const char * const omnia_mcu_gpio_templates[64] = {
> > +       /* GPIOs with value read from the 16-bit wide status */
> > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > +       [6]  = "gpio%u.Front USB3 port over-current",
> > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > +       [8]  = "gpio%u.Front USB3 port power",
> > +       [9]  = "gpio%u.Rear USB3 port power",
> > +       [12] = "gpio%u.Front Button",
> > +
> > +       /* GPIOs with value read from the 32-bit wide extended status */
> > +       [16] = "gpio%u.SFP nDET",
> > +       [28] = "gpio%u.MiniPCIe0 LED",
> > +       [29] = "gpio%u.MiniPCIe1 LED",
> > +       [30] = "gpio%u.MiniPCIe2 LED",
> > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > +       [34] = "gpio%u.WAN PHY LED0",
> > +       [35] = "gpio%u.WAN PHY LED1",
> > +       [36] = "gpio%u.LAN switch p0 LED0",
> > +       [37] = "gpio%u.LAN switch p0 LED1",
> > +       [38] = "gpio%u.LAN switch p1 LED0",
> > +       [39] = "gpio%u.LAN switch p1 LED1",
> > +       [40] = "gpio%u.LAN switch p2 LED0",
> > +       [41] = "gpio%u.LAN switch p2 LED1",
> > +       [42] = "gpio%u.LAN switch p3 LED0",
> > +       [43] = "gpio%u.LAN switch p3 LED1",
> > +       [44] = "gpio%u.LAN switch p4 LED0",
> > +       [45] = "gpio%u.LAN switch p4 LED1",
> > +       [46] = "gpio%u.LAN switch p5 LED0",
> > +       [47] = "gpio%u.LAN switch p5 LED1",
> > +
> > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > +       [48] = "gpio%u.eMMC nRESET",
> > +       [49] = "gpio%u.LAN switch nRESET",
> > +       [50] = "gpio%u.WAN PHY nRESET",
> > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > +       [54] = "gpio%u.WAN PHY SFP mux",
> > +};
> 
> You may reduce the memory footprint of these just by doing "gpio%u."
> part(s) outside. Here compiler won't compress this (as in the case of
> repetitive string literals),

Are you saying that I should dynamically create another array just to
pass it to the gpiochip's names pointer?
 
> > +static const struct omnia_gpio {
> > +       u8 cmd, ctl_cmd;
> > +       u32 bit, ctl_bit;
> > +       u32 int_bit;
> > +       u16 feat, feat_mask;
> > +} omnia_gpios[64] = {
> 
> It's much better to decouple definition and assignment and put
> definition _before_ the macros that fill it.

Fixed.

> > +               scoped_guard(mutex, &mcu->lock)
> > +                       val = omnia_cmd_read_bit(mcu->client,
> > +                                                CMD_GET_EXT_CONTROL_STATUS,
> > +                                                EXT_CTL_PHY_SFP_AUTO);
> > +
> > +               if (val < 0)
> > +                       return val;
> 
> I would move that under guard  for the sake of better readability
> (usual pattern in place). It's anyway a slow path and one branch under
> the mutex won't affect anything.

OK.

> > +               if (val)
> > +                       return GPIO_LINE_DIRECTION_IN;
> > +
> > +               return GPIO_LINE_DIRECTION_OUT;
> 
> ...
> 
> > +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> > +       u32 sts_bits, ext_sts_bits, ext_ctl_bits;
> > +       int err, i;
> 
> > +       sts_bits = 0;
> > +       ext_sts_bits = 0;
> > +       ext_ctl_bits = 0;
> 
> Why not assign these inside the definition line?

OK.

> > +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> > +               if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
> > +                       __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
> > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
> > +                       __assign_bit(i, bits, ext_sts_bits &
> > +                                             omnia_gpios[i].bit);
> 
> One line?

That would be 81 columns, which I would like to avoid.
I can remove the _bits suffix from these variables, though. What do you
think?

> > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
> > +                       __assign_bit(i, bits, ext_ctl_bits &
> > +                                             omnia_gpios[i].bit);
> 
> Ditto?
> 
> > +       }
> 
> ...
> 
> > +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> > +       u16 ext_ctl, ext_ctl_mask;
> > +       u8 ctl, ctl_mask;
> > +       int i;
> > +
> > +       ctl = 0;
> > +       ctl_mask = 0;
> > +       ext_ctl = 0;
> > +       ext_ctl_mask = 0;
> 
> Assignments in the definition line?

OK

> > +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> > +               if (omnia_gpios[i].ctl_cmd == CMD_GENERAL_CONTROL) {
> > +                       ctl_mask |= omnia_gpios[i].ctl_bit;
> > +                       if (test_bit(i, bits))
> > +                               ctl |= omnia_gpios[i].ctl_bit;
> > +               } else if (omnia_gpios[i].ctl_cmd == CMD_EXT_CONTROL) {
> > +                       ext_ctl_mask |= omnia_gpios[i].ctl_bit;
> > +                       if (test_bit(i, bits))
> > +                               ext_ctl |= omnia_gpios[i].ctl_bit;
> > +               }
> > +       }
> 
> ...
> 
> > +/**
> > + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
> > + *     @dst: the destination u8 array of interleaved bytes
> > + *     @rising: rising mask
> > + *     @falling: falling mask
> > + *
> > + * Interleaves the little-endian bytes from @rising and @falling words.
> > + *
> > + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
> > + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
> > + *
> > + * The MCU receives interrupt mask and reports pending interrupt bitmap int this
> 
> an interrupt
> a pending
> int --> in ?

Thx.

> > + * interleaved format. The rationale behind it is that the low-indexed bits are
> > + * more important - in many cases, the user will be interested only in
> > + * interrupts with indexes 0 to 7, and so the system can stop reading after
> > + * first 2 bytes (r0, f0), to save time on the slow I2C bus.
> > + *
> > + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> > + * and use an appropriate bitmap_* function once such a function exists.
> > + */
> > +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling)
> > +{
> > +       for (int i = 0; i < sizeof(u32); ++i) {
> > +               dst[2 * i] = rising >> (8 * i);
> > +               dst[2 * i + 1] = falling >> (8 * i);
> > +       }
> 
> I will look at this later on,
> 
> > +}
> 
> > +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling)
> > +{
> > +       *rising = *falling = 0;
> > +
> > +       for (int i = 0; i < sizeof(u32); ++i) {
> > +               *rising |= src[2 * i] << (8 * i);
> > +               *falling |= src[2 * i + 1] << (8 * i);
> > +       }
> > +}
> 
> and into this.
> 
> ...
> 
> > +static size_t omnia_irq_compute_pending_length(u32 rising, u32 falling)
> > +{
> > +       size_t rlen = 0, flen = 0;
> > +
> > +       if (rising)
> > +               rlen = ((__fls(rising) >> 3) << 1) + 1;
> > +
> > +       if (falling)
> > +               flen = ((__fls(falling) >> 3) << 1) + 2;
> > +
> > +       return max(rlen, flen);
> > +}
> 
> I am not sure why you need this, but it can be done easily
> 
> x = ((__fls(falling | rising) >> 3) << 1) + 1;
> if (falling)
>   return x + 1;
> return x;
> 
> and most likely you can drop minmax.h.

This will return different results for for example when
  rising = 0x100
  falling = 0x10
where we need to read only 3 bytes, but your version will say that we
need 4 bytes.

> > +static const char * const front_button_modes[2] = { "mcu", "cpu" };
> 
> 2 is not needed.

OK

> > -#include <linux/i2c.h>
> 
> ??? That is exactly the point to have things done from the beginning.

I must have somehow missed this before sending, I see that I've already
fixed it in my working branch.

> > +#include <linux/bitops.h>
> > +#include <linux/gpio/driver.h>
> >  #include <linux/if_ether.h>
> > +#include <linux/mutex.h>
> >  #include <linux/types.h>
> > +#include <linux/workqueue.h>
> >  #include <asm/byteorder.h>
> 
> ...
> 
> > +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> 
> Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
> in a few places.
> 
> 
> > +       if (err)
> > +               return err;
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote:
> On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > +static const char * const omnia_mcu_gpio_templates[64] = {
> > > +       /* GPIOs with value read from the 16-bit wide status */
> > > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > > +       [6]  = "gpio%u.Front USB3 port over-current",
> > > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > > +       [8]  = "gpio%u.Front USB3 port power",
> > > +       [9]  = "gpio%u.Rear USB3 port power",
> > > +       [12] = "gpio%u.Front Button",
> > > +
> > > +       /* GPIOs with value read from the 32-bit wide extended status */
> > > +       [16] = "gpio%u.SFP nDET",
> > > +       [28] = "gpio%u.MiniPCIe0 LED",
> > > +       [29] = "gpio%u.MiniPCIe1 LED",
> > > +       [30] = "gpio%u.MiniPCIe2 LED",
> > > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > > +       [34] = "gpio%u.WAN PHY LED0",
> > > +       [35] = "gpio%u.WAN PHY LED1",
> > > +       [36] = "gpio%u.LAN switch p0 LED0",
> > > +       [37] = "gpio%u.LAN switch p0 LED1",
> > > +       [38] = "gpio%u.LAN switch p1 LED0",
> > > +       [39] = "gpio%u.LAN switch p1 LED1",
> > > +       [40] = "gpio%u.LAN switch p2 LED0",
> > > +       [41] = "gpio%u.LAN switch p2 LED1",
> > > +       [42] = "gpio%u.LAN switch p3 LED0",
> > > +       [43] = "gpio%u.LAN switch p3 LED1",
> > > +       [44] = "gpio%u.LAN switch p4 LED0",
> > > +       [45] = "gpio%u.LAN switch p4 LED1",
> > > +       [46] = "gpio%u.LAN switch p5 LED0",
> > > +       [47] = "gpio%u.LAN switch p5 LED1",
> > > +
> > > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > > +       [48] = "gpio%u.eMMC nRESET",
> > > +       [49] = "gpio%u.LAN switch nRESET",
> > > +       [50] = "gpio%u.WAN PHY nRESET",
> > > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > > +       [54] = "gpio%u.WAN PHY SFP mux",
> > > +};
> > 
> > You may reduce the memory footprint of these just by doing "gpio%u."
> > part(s) outside. Here compiler won't compress this (as in the case of
> > repetitive string literals),
> 
> Are you saying that I should dynamically create another array just to
> pass it to the gpiochip's names pointer?

I have looked into this again and now I'm puzzled how you tested this.
To me it seems that those gpio%u will go as a fixed string to the user space,
there is no %u --> number substitution happens. Moreover, this data anyway
is redundant. Userspace and debugfs all have line numbers being printed.

...

> > > +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> > > +               if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
> > > +                       __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
> > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
> > > +                       __assign_bit(i, bits, ext_sts_bits &
> > > +                                             omnia_gpios[i].bit);
> > 
> > One line?
> 
> That would be 81 columns, which I would like to avoid.

81?! It's fine! Even documentation allows that for the readability.

> I can remove the _bits suffix from these variables, though. What do you
> think?

Make sense as well.

> > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
> > > +                       __assign_bit(i, bits, ext_ctl_bits &
> > > +                                             omnia_gpios[i].bit);
> > 
> > Ditto?
> > 
> > > +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote:
> On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote:
> > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> 
> ...
> 
> > > > +static const char * const omnia_mcu_gpio_templates[64] = {
> > > > +       /* GPIOs with value read from the 16-bit wide status */
> > > > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > > > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > > > +       [6]  = "gpio%u.Front USB3 port over-current",
> > > > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > > > +       [8]  = "gpio%u.Front USB3 port power",
> > > > +       [9]  = "gpio%u.Rear USB3 port power",
> > > > +       [12] = "gpio%u.Front Button",
> > > > +
> > > > +       /* GPIOs with value read from the 32-bit wide extended status */
> > > > +       [16] = "gpio%u.SFP nDET",
> > > > +       [28] = "gpio%u.MiniPCIe0 LED",
> > > > +       [29] = "gpio%u.MiniPCIe1 LED",
> > > > +       [30] = "gpio%u.MiniPCIe2 LED",
> > > > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > > > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > > > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > > > +       [34] = "gpio%u.WAN PHY LED0",
> > > > +       [35] = "gpio%u.WAN PHY LED1",
> > > > +       [36] = "gpio%u.LAN switch p0 LED0",
> > > > +       [37] = "gpio%u.LAN switch p0 LED1",
> > > > +       [38] = "gpio%u.LAN switch p1 LED0",
> > > > +       [39] = "gpio%u.LAN switch p1 LED1",
> > > > +       [40] = "gpio%u.LAN switch p2 LED0",
> > > > +       [41] = "gpio%u.LAN switch p2 LED1",
> > > > +       [42] = "gpio%u.LAN switch p3 LED0",
> > > > +       [43] = "gpio%u.LAN switch p3 LED1",
> > > > +       [44] = "gpio%u.LAN switch p4 LED0",
> > > > +       [45] = "gpio%u.LAN switch p4 LED1",
> > > > +       [46] = "gpio%u.LAN switch p5 LED0",
> > > > +       [47] = "gpio%u.LAN switch p5 LED1",
> > > > +
> > > > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > > > +       [48] = "gpio%u.eMMC nRESET",
> > > > +       [49] = "gpio%u.LAN switch nRESET",
> > > > +       [50] = "gpio%u.WAN PHY nRESET",
> > > > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > > > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > > > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > > > +       [54] = "gpio%u.WAN PHY SFP mux",
> > > > +};
> > > 
> > > You may reduce the memory footprint of these just by doing "gpio%u."
> > > part(s) outside. Here compiler won't compress this (as in the case of
> > > repetitive string literals),
> > 
> > Are you saying that I should dynamically create another array just to
> > pass it to the gpiochip's names pointer?
> 
> I have looked into this again and now I'm puzzled how you tested this.
> To me it seems that those gpio%u will go as a fixed string to the user space,
> there is no %u --> number substitution happens. Moreover, this data anyway
> is redundant. Userspace and debugfs all have line numbers being printed.
> 

It gets substituted in drivers/gpio/gpiolib-sysfs.c, function
gpiod_export():

  dev = device_create_with_groups(&gpio_class, &gdev->dev,
                                  MKDEV(0, 0), data, gpio_groups,
				  ioname ? ioname : "gpio%u",
				  desc_to_gpio(desc));

The ioname variable contains the string.

This is then visible in sysfs:

  $ cd /sys/class/gpio
  $ echo 560 >export
  $ ls
  ...
  gpio560.eMMC nRESET
  ...


> ...
> 
> > > > +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> > > > +               if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
> > > > +                       __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
> > > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
> > > > +                       __assign_bit(i, bits, ext_sts_bits &
> > > > +                                             omnia_gpios[i].bit);
> > > 
> > > One line?
> > 
> > That would be 81 columns, which I would like to avoid.
> 
> 81?! It's fine! Even documentation allows that for the readability.

OK

> > I can remove the _bits suffix from these variables, though. What do you
> > think?
> 
> Make sense as well.
> 
> > > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
> > > > +                       __assign_bit(i, bits, ext_ctl_bits &
> > > > +                                             omnia_gpios[i].bit);
> > > 
> > > Ditto?
> > > 
> > > > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Sun, May 5, 2024 at 11:18 AM Marek Behún <kabel@kernel.org> wrote:
> On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote:
> > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote:
> > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:

...

> > > > > +static const char * const omnia_mcu_gpio_templates[64] = {
> > > > > +       /* GPIOs with value read from the 16-bit wide status */
> > > > > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > > > > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > > > > +       [6]  = "gpio%u.Front USB3 port over-current",
> > > > > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > > > > +       [8]  = "gpio%u.Front USB3 port power",
> > > > > +       [9]  = "gpio%u.Rear USB3 port power",
> > > > > +       [12] = "gpio%u.Front Button",
> > > > > +
> > > > > +       /* GPIOs with value read from the 32-bit wide extended status */
> > > > > +       [16] = "gpio%u.SFP nDET",
> > > > > +       [28] = "gpio%u.MiniPCIe0 LED",
> > > > > +       [29] = "gpio%u.MiniPCIe1 LED",
> > > > > +       [30] = "gpio%u.MiniPCIe2 LED",
> > > > > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > > > > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > > > > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > > > > +       [34] = "gpio%u.WAN PHY LED0",
> > > > > +       [35] = "gpio%u.WAN PHY LED1",
> > > > > +       [36] = "gpio%u.LAN switch p0 LED0",
> > > > > +       [37] = "gpio%u.LAN switch p0 LED1",
> > > > > +       [38] = "gpio%u.LAN switch p1 LED0",
> > > > > +       [39] = "gpio%u.LAN switch p1 LED1",
> > > > > +       [40] = "gpio%u.LAN switch p2 LED0",
> > > > > +       [41] = "gpio%u.LAN switch p2 LED1",
> > > > > +       [42] = "gpio%u.LAN switch p3 LED0",
> > > > > +       [43] = "gpio%u.LAN switch p3 LED1",
> > > > > +       [44] = "gpio%u.LAN switch p4 LED0",
> > > > > +       [45] = "gpio%u.LAN switch p4 LED1",
> > > > > +       [46] = "gpio%u.LAN switch p5 LED0",
> > > > > +       [47] = "gpio%u.LAN switch p5 LED1",
> > > > > +
> > > > > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > > > > +       [48] = "gpio%u.eMMC nRESET",
> > > > > +       [49] = "gpio%u.LAN switch nRESET",
> > > > > +       [50] = "gpio%u.WAN PHY nRESET",
> > > > > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > > > > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > > > > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > > > > +       [54] = "gpio%u.WAN PHY SFP mux",
> > > > > +};
> > > >
> > > > You may reduce the memory footprint of these just by doing "gpio%u."
> > > > part(s) outside. Here compiler won't compress this (as in the case of
> > > > repetitive string literals),
> > >
> > > Are you saying that I should dynamically create another array just to
> > > pass it to the gpiochip's names pointer?
> >
> > I have looked into this again and now I'm puzzled how you tested this.
> > To me it seems that those gpio%u will go as a fixed string to the user space,
> > there is no %u --> number substitution happens. Moreover, this data anyway
> > is redundant. Userspace and debugfs all have line numbers being printed.
> >
>
> It gets substituted in drivers/gpio/gpiolib-sysfs.c, function
> gpiod_export():
>
>   dev = device_create_with_groups(&gpio_class, &gdev->dev,
>                                   MKDEV(0, 0), data, gpio_groups,
>                                   ioname ? ioname : "gpio%u",
>                                   desc_to_gpio(desc));
>
> The ioname variable contains the string.
>
> This is then visible in sysfs:
>
>   $ cd /sys/class/gpio
>   $ echo 560 >export
>   $ ls
>   ...
>   gpio560.eMMC nRESET
>   ...

Interesting. But before giving my conclusion on this, what is the
output of /sys/kernel/debug/gpio and `gpioinfo` (the latter from
libgpiod)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

On Sun, May 05, 2024 at 04:34:28PM +0300, Andy Shevchenko wrote:
> On Sun, May 5, 2024 at 11:18 AM Marek Behún <kabel@kernel.org> wrote:
> > On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote:
> > > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote:
> > > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@kernel.org> wrote:
> 
> ...
> 
> > > > > > +static const char * const omnia_mcu_gpio_templates[64] = {
> > > > > > +       /* GPIOs with value read from the 16-bit wide status */
> > > > > > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > > > > > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > > > > > +       [6]  = "gpio%u.Front USB3 port over-current",
> > > > > > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > > > > > +       [8]  = "gpio%u.Front USB3 port power",
> > > > > > +       [9]  = "gpio%u.Rear USB3 port power",
> > > > > > +       [12] = "gpio%u.Front Button",
> > > > > > +
> > > > > > +       /* GPIOs with value read from the 32-bit wide extended status */
> > > > > > +       [16] = "gpio%u.SFP nDET",
> > > > > > +       [28] = "gpio%u.MiniPCIe0 LED",
> > > > > > +       [29] = "gpio%u.MiniPCIe1 LED",
> > > > > > +       [30] = "gpio%u.MiniPCIe2 LED",
> > > > > > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > > > > > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > > > > > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > > > > > +       [34] = "gpio%u.WAN PHY LED0",
> > > > > > +       [35] = "gpio%u.WAN PHY LED1",
> > > > > > +       [36] = "gpio%u.LAN switch p0 LED0",
> > > > > > +       [37] = "gpio%u.LAN switch p0 LED1",
> > > > > > +       [38] = "gpio%u.LAN switch p1 LED0",
> > > > > > +       [39] = "gpio%u.LAN switch p1 LED1",
> > > > > > +       [40] = "gpio%u.LAN switch p2 LED0",
> > > > > > +       [41] = "gpio%u.LAN switch p2 LED1",
> > > > > > +       [42] = "gpio%u.LAN switch p3 LED0",
> > > > > > +       [43] = "gpio%u.LAN switch p3 LED1",
> > > > > > +       [44] = "gpio%u.LAN switch p4 LED0",
> > > > > > +       [45] = "gpio%u.LAN switch p4 LED1",
> > > > > > +       [46] = "gpio%u.LAN switch p5 LED0",
> > > > > > +       [47] = "gpio%u.LAN switch p5 LED1",
> > > > > > +
> > > > > > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > > > > > +       [48] = "gpio%u.eMMC nRESET",
> > > > > > +       [49] = "gpio%u.LAN switch nRESET",
> > > > > > +       [50] = "gpio%u.WAN PHY nRESET",
> > > > > > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > > > > > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > > > > > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > > > > > +       [54] = "gpio%u.WAN PHY SFP mux",
> > > > > > +};
> > > > >
> > > > > You may reduce the memory footprint of these just by doing "gpio%u."
> > > > > part(s) outside. Here compiler won't compress this (as in the case of
> > > > > repetitive string literals),
> > > >
> > > > Are you saying that I should dynamically create another array just to
> > > > pass it to the gpiochip's names pointer?
> > >
> > > I have looked into this again and now I'm puzzled how you tested this.
> > > To me it seems that those gpio%u will go as a fixed string to the user space,
> > > there is no %u --> number substitution happens. Moreover, this data anyway
> > > is redundant. Userspace and debugfs all have line numbers being printed.
> > >
> >
> > It gets substituted in drivers/gpio/gpiolib-sysfs.c, function
> > gpiod_export():
> >
> >   dev = device_create_with_groups(&gpio_class, &gdev->dev,
> >                                   MKDEV(0, 0), data, gpio_groups,
> >                                   ioname ? ioname : "gpio%u",
> >                                   desc_to_gpio(desc));
> >
> > The ioname variable contains the string.
> >
> > This is then visible in sysfs:
> >
> >   $ cd /sys/class/gpio
> >   $ echo 560 >export
> >   $ ls
> >   ...
> >   gpio560.eMMC nRESET
> >   ...
> 
> Interesting. But before giving my conclusion on this, what is the
> output of /sys/kernel/debug/gpio and `gpioinfo` (the latter from
> libgpiod)?

It prints %u, as you suspected and I see you already posted patch to
avoid this. I'm dropping the "gpio%u." prefixes from GPIO line names.

Marek

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
... 
> > +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> 
> Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
> in a few places.

It is used 3 times:
  rlen = ((__fls(rising) >> 3) << 1) + 1;
  flen = ((__fls(falling) >> 3) << 1) + 2;
  err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);

The last one is not compatible with the first two (because of the "<< 1").

The first two instances are contained within a function that is dedicated
to "computing needed reply length".

I could probably do something like

  static inline unsigned int
  omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset)
  {
          return ((__fls(mask) >> 3) << interleaved) + 1 + offset;
  }

Then the 3 instances would become

  rlen = omnia_compute_reply_len(rising, true, 0);
  flen = omnia_compute_reply_len(falling, true, 1);
  err = omnia_cmd_read(client, cmd, &reply,
                       omnia_compute_reply_len(bits, false, 0));

What do you think?

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Fri, May 03, 2024 at 10:43:06AM +0200, Marek Behún wrote:
> On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:

...

> > > +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> > 
> > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
> > in a few places.
> 
> It is used 3 times:
>   rlen = ((__fls(rising) >> 3) << 1) + 1;
>   flen = ((__fls(falling) >> 3) << 1) + 2;
>   err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> 
> The last one is not compatible with the first two (because of the "<< 1").
> 
> The first two instances are contained within a function that is dedicated
> to "computing needed reply length".
> 
> I could probably do something like
> 
>   static inline unsigned int
>   omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset)
>   {
>           return ((__fls(mask) >> 3) << interleaved) + 1 + offset;
>   }
> 
> Then the 3 instances would become
> 
>   rlen = omnia_compute_reply_len(rising, true, 0);
>   flen = omnia_compute_reply_len(falling, true, 1);
>   err = omnia_cmd_read(client, cmd, &reply,
>                        omnia_compute_reply_len(bits, false, 0));
> 
> What do you think?

Fine, but think about these bit operations, I believe it can be optimised.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún]

On Fri, May 03, 2024 at 08:33:25PM +0300, Andy Shevchenko wrote:
> On Fri, May 03, 2024 at 10:43:06AM +0200, Marek Behún wrote:
> > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > +       err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> > > 
> > > Perhaps a helper for this (__fls(x) >> 3 + (y)) ? It seems it's used
> > > in a few places.
> > 
> > It is used 3 times:
> >   rlen = ((__fls(rising) >> 3) << 1) + 1;
> >   flen = ((__fls(falling) >> 3) << 1) + 2;
> >   err = omnia_cmd_read(client, cmd, &reply, (__fls(bits) >> 3) + 1);
> > 
> > The last one is not compatible with the first two (because of the "<< 1").
> > 
> > The first two instances are contained within a function that is dedicated
> > to "computing needed reply length".
> > 
> > I could probably do something like
> > 
> >   static inline unsigned int
> >   omnia_compute_reply_len(uin32_t mask, bool interleaved, unsigned int offset)
> >   {
> >           return ((__fls(mask) >> 3) << interleaved) + 1 + offset;
> >   }
> > 
> > Then the 3 instances would become
> > 
> >   rlen = omnia_compute_reply_len(rising, true, 0);
> >   flen = omnia_compute_reply_len(falling, true, 1);
> >   err = omnia_cmd_read(client, cmd, &reply,
> >                        omnia_compute_reply_len(bits, false, 0));
> > 
> > What do you think?
> 
> Fine, but think about these bit operations, I believe it can be optimised.

I will try something, altough I fear it might need 64-bit operations,
which may be even more suboptimal since the driver is supposed to run on
armv7.

Also, isn't this premature optimization? Since the computed length is then
passed as a parameter to I2C reading, and the I2C bus is orders of
magnitudes slower and the handling of I2C requests in Marvell I2C driver
will kill any optimization I do here...