Turris Omnia MCU driver

Add binding for cznic,turris-omnia-mcu, the device-tree node representing the system-controller features provided by the MCU on the Turris Omnia router.

Signed-off-by Marek Behún <[email protected]>

---

Add a new platform driver that exposes the features provided by the microcontroller on the Turris Omnia board.

This commit adds the basic skeleton for the driver.

+turris-omnia-mcu-objs := turris-omnia-mcu-base.o

drivers/platform/cznic/turris-omnia-mcu-base.c new file +6 -0

+	err = omnia_get_version_hash(mcu, bootloader, version);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%s\n", version);
+}

+ATTRIBUTE_GROUPS(omnia_mcu);

drivers/platform/cznic/turris-omnia-mcu-base.c new file +3 -0

+	status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
+	if (status < 0)
+		return status;

drivers/platform/cznic/turris-omnia-mcu-base.c new file +4 -0

+			omnia_info_missing_feature(dev, features[i].name);
+			suggest_fw_upgrade = true;
+		}
+	}

(quoted diff) +1 -0

+	}

+};

(quoted diff) +2 -0

+	{ }
+};

+module_i2c_driver(omnia_mcu_driver);

+#include <asm/byteorder.h>

+#include <linux/i2c.h>

+#include <linux/turris-omnia-mcu-interface.h>

+}

Quoted 4 lines · Marek Behún

Add a new platform driver that exposes the features provided by the microcontroller on the Turris Omnia board.

This commit adds the basic skeleton for the driver.

Read "Submitting Patches" documentation (find "This patch" in it) and amend your commit message accordingly.

Review

Quoted 2 lines

(quoted diff) +2 -0

+Date:		August 2023
+KernelVersion:	6.6  

Wrong and outdated. Use phb-crystall-ball to find the closest possible values for both parameters here.

Ditto for all others with the same issue.

Note, it likely might be part of v6.7, not earlier.

Quoted 4 lines

obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
+obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/

Why not ordered (to some extent) here (as you did in the other file)?

Quoted 1 line

+turris-omnia-mcu-objs := turris-omnia-mcu-base.o

objs is for user space tools. Kernel code should use m,y.

drivers/platform/cznic/turris-omnia-mcu-base.c line 9

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+#include <linux/hex.h>
+#include <linux/module.h>  

Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...

Quoted 12 lines

+static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,

drivers/platform/cznic/turris-omnia-mcu-base.c new file +6 -0

+				  u8 *version)
+{
+	u8 reply[20];
+	int ret;
+
+	ret = omnia_cmd_read(mcu->client, bootloader ?

CMD_GET_FW_VERSION_BOOT :
+n average development ti
CMD_GET_FW_VERSION_APP,

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+			     reply, sizeof(reply));  

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (ret < 0)  

Can it return positive value? What would be the meaning of it?

drivers/platform/cznic/turris-omnia-mcu-base.c line 25

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+		return ret;  
 
+	version[40] = '\0';  

How do you know the version has enough space?

Quoted 11 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +9 -0

+	bin2hex(version, reply, 20);
+
+	return 0;
+}
+
+static ssize_t fw_version_hash_show(struct device *dev, char *buf,
+				    bool bootloader)
+{
+	struct omnia_mcu *mcu =

i2c_get_clientdata(to_i2c_client(dev));

(quoted diff) +1 -0

+	u8 version[41];  

Review

Quoted 1 line

(quoted diff) +1 -0

+	int err;  

In two near functions you already inconsistent in the naming of the return code variable. Be consistent across all your code, i.e. choose one name and use it everywhere.

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +6 -0

+	err = omnia_get_version_hash(mcu, bootloader, version);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%s\n", version);
+}  

...

drivers/platform/cznic/turris-omnia-mcu-base.c line 66

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	return sysfs_emit(buf, "%#x\n", mcu->features);  

"0" will be printed differently, is this on purpose?

Review

Quoted 4 lines

(quoted diff) +1 -0

+	int ret = omnia_cmd_read_u8(to_i2c_client(dev),

CMD_GET_RESET); +

(quoted diff) +2 -0

+	if (ret < 0)
+		return ret;  

Better from maintenance perspective to have

	int ret;

	ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
	if (ret < 0)
		return ret;

drivers/platform/cznic/turris-omnia-mcu-base.c line 97

Quoted 8 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +7 -0

+static struct attribute *omnia_mcu_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,  
 
+	NULL,  

No comma for the terminator line. It will make your code robust at compile time against some misplaced entries.

drivers/platform/cznic/turris-omnia-mcu-base.c line 99

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+};  
 
+  

Unneeded blank line.

Quoted 1 line

+ATTRIBUTE_GROUPS(omnia_mcu);

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	u8 version[41];  

This magic sizes should go away. Use predefined constant, or allocate on the heap, depending on the case(s) you have.

drivers/platform/cznic/turris-omnia-mcu-base.c line 154

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	int status;  

My gosh, it's a _third_ name for the same!

Quoted 4 lines

(quoted diff) +1 -0

+	status = omnia_cmd_read_u16(mcu->client,

CMD_GET_STATUS_WORD);

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+	if (status < 0)
+		return status;  

...

drivers/platform/cznic/turris-omnia-mcu-base.c line 186

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	for (int i = 0; i < ARRAY_SIZE(features); ++i) {  

Why signed?

Why pre-increment?

drivers/platform/cznic/turris-omnia-mcu-base.c line 187

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+		if (!(mcu->features & features[i].mask)) {  

Wouldn't be better

		if (mcu->features & features[i].mask)
			continue;

?

Quoted 5 lines

(quoted diff) +1 -0

+			omnia_info_missing_feature(dev,

features[i].name);

drivers/platform/cznic/turris-omnia-mcu-base.c new file +3 -0

+			suggest_fw_upgrade = true;
+		}
+	}  

...

Quoted 3 lines

(quoted diff) +2 -0

+		dev_info(dev,
+			 "Consider upgrading MCU firmware with the

omnia-mcutool utility.\n");

You have so-o many dev_info() calls, are you sure you not abusing use of that?

...

drivers/platform/cznic/turris-omnia-mcu-base.c line 217

Quoted 5 lines

(quoted diff) +2 -0

+	if (ret) {
+		dev_err(dev, "Cannot determine MCU supported

features: %d\n",

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+			ret);
+		return ret;  

		return dev_err_probe(...);

Quoted 1 line

(quoted diff) +1 -0

+	}  

...

drivers/platform/cznic/turris-omnia-mcu-base.c line 223

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +4 -0

+	if (!client->irq) {
+		dev_err(dev, "IRQ resource not found\n");
+		return -ENODATA;
+	}  

Why you need to allocate memory, go through the initial checks, etc and then fail? What's wrong with checking this first?

Why -ENODATA?!

drivers/platform/cznic/turris-omnia-mcu-base.c line 229

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+static const struct of_device_id of_omnia_mcu_match[] = {
+	{ .compatible = "cznic,turris-omnia-mcu", },  

Inner comma is not needed.

drivers/platform/cznic/turris-omnia-mcu-base.c line 230

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	{},  

No comma for the terminator entry.

Quoted 1 line

+};

...

drivers/platform/cznic/turris-omnia-mcu-base.c line 234

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+static const struct i2c_device_id omnia_mcu_id[] = {
+	{ "turris-omnia-mcu", 0 },  

', 0' part is redundant.

Quoted 2 lines

(quoted diff) +2 -0

+	{ }
+};  

...

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +9 -0

+static struct i2c_driver omnia_mcu_driver = {
+	.probe		= omnia_mcu_probe,
+	.id_table	= omnia_mcu_id,
+	.driver		= {
+		.name	= "turris-omnia-mcu",
+		.of_match_table = of_omnia_mcu_match,
+		.dev_groups = omnia_mcu_groups,
+	},
+};  

Quoted 1 line

+

Redundant blank line.

Quoted 1 line

+module_i2c_driver(omnia_mcu_driver);

...

drivers/platform/cznic/turris-omnia-mcu.h line 9

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu.h new file +2 -0

+#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
+#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H  

WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!

Quoted 1 line

+#include <asm/byteorder.h>

This usually goes after linux/*.h.

Quoted 2 lines

+#include <linux/i2c.h>

Missing types.h, errno.h, ...

+ blank line?

Quoted 1 line

+#include <linux/turris-omnia-mcu-interface.h>

...

drivers/platform/cznic/turris-omnia-mcu.h line 37

Quoted 3 lines

(quoted diff) +1 -0

+	ret = i2c_transfer(client->adapter, msgs,

ARRAY_SIZE(msgs));

drivers/platform/cznic/turris-omnia-mcu.h new file +1 -0

+	if (likely(ret == ARRAY_SIZE(msgs)))  

Why likely()? Please, justify.

drivers/platform/cznic/turris-omnia-mcu.h line 42

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu.h new file +5 -0

+		return len;  
 
+	else if (ret < 0)
+		return ret;
+	else
+		return -EIO;  

In both cases the 'else' is redundant. Moreover, the usual pattern is to check for the errors first.

Quoted 1 line

+}

...

include/linux/turris-omnia-mcu-interface.h line 9

Quoted 2 lines

include/linux/turris-omnia-mcu-interface.h new file +2 -0

+#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
+#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H  

My gosh!

include/linux/turris-omnia-mcu-interface.h line 13

Quoted 3 lines

include/linux/turris-omnia-mcu-interface.h new file +2 -0

+#include <linux/bits.h>  
 
+enum commands_e {  

Are you sure the name is unique enough / properly namespaced? Same Q to all enums.

...

include/linux/turris-omnia-mcu-interface.h line 96

Quoted 1 line

include/linux/turris-omnia-mcu-interface.h new file +1 -0

+	/*CTL_RESERVED		= BIT(2),*/  

Missing spaces?
Also, needs a comment why this is commented out.

include/linux/turris-omnia-mcu-interface.h line 101

Quoted 1 line

include/linux/turris-omnia-mcu-interface.h new file +1 -0

+	CTL_BOOTLOADER		= BIT(7)  

Add trailing comma.

Quoted 1 line · On Tue, 19 Sep 2023 15:29:08 +0300 Andy Shevchenko <[email protected]> wrote:

On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:

Quoted 6 lines

Quoted 4 lines

obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
+obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/

Why not ordered (to some extent) here (as you did in the other file)?

Where should I put it? The other entries are not ordered, see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2

Quoted 3 lines

Quoted 1 line

+turris-omnia-mcu-objs := turris-omnia-mcu-base.o

objs is for user space tools. Kernel code should use m,y.

I don't understand. There are plenty driver using -objs. The driver consists of multiple C source files. I thought this should be done as
obj-$(CONFIG_XXX) += xxx.o
xxx-objs := xxx-a.o xxx-b.o xxx-c.o

If you choose CONFIG_XXX = y, then the driver will be compiled into kernel, if you choose CONFIG_XXX = m, then the driver will be a module xxx.ko.

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 9

Quoted 2 lines

drivers/platform/cznic/turris-omnia-mcu-base.c new file +2 -0

+#include <linux/hex.h>
+#include <linux/module.h>  

Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...

i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the the .c file also include all those headers if it includes a local header already?

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (ret < 0)  

Can it return positive value? What would be the meaning of it?

Originally I had it return 0 on success, but recently I've had a discussion about this same thing with LED subsystem maintainer about some LED patches, Lee Jones, who requested the _read function to return number of bytes read on success. See at
https://lore.kernel.org/linux-leds/[email protected]/
where he says:
Read and write APIs, even abstracted ones such as these generally return the number of bytes successfully read and written respectively. If you are going to normalise, then please do so against this standard.
I do not agree with him, since this is a local command accessor which only succeeds if all the bytes are read/written as requested. But I adjusted my code as per his request since he is the maintainer.

I will change the code back so that it returns 0 on success instead of the number of bytes written.

Quoted 1 line

Quoted 1 line

(quoted diff) +1 -0

+		return ret;  

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 25

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	version[40] = '\0';  

How do you know the version has enough space?

The function is only use within the file, and both users know how much space it needs. But ok, I shall enforce it with the static keyword as
static int
omnia_get_version_hash(...,

			 u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);

Would that be okay?

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	int err;  

In two near functions you already inconsistent in the naming of the return code variable. Be consistent across all your code, i.e. choose one name and use it everywhere.

I use
ret - when the return value can also be positive and mean something err - when the return value is either 0 (no error) or negative errno
Is this inconsistent?

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 154

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	int status;  

My gosh, it's a _third_ name for the same!

The variable holds value of the status register, which is needed at another point in this function. I thought naming it just "ret" would cause confusions... Do you really propose to rename it such?

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 186

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	for (int i = 0; i < ARRAY_SIZE(features); ++i) {  

Why signed?

Because it is shorter and makes no difference and there are tons of
for (int i = 0; i < ARRAY_SIZE(x); i++)
in the kernel already. But if you really want, I can change it. Please let me know.

Quoted 1 line

Why pre-increment?

Again, it makes no difference, is widely used in the kernel and I personally prefer it. But I will change it if you want.

Quoted 6 lines

Quoted 3 lines

(quoted diff) +2 -0

+		dev_info(dev,
+			 "Consider upgrading MCU firmware with the

omnia-mcutool utility.\n");

You have so-o many dev_info() calls, are you sure you not abusing use of that?

I want the users to upgrade the MCU firmware. These infos are only printed during probe time, and won't be printed at all if the firmware is upgraded. Is this a problem?

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu.h line 37

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu.h new file +1 -0

+	if (likely(ret == ARRAY_SIZE(msgs)))  

Why likely()? Please, justify.

Becuase it is unlikely the I2C transaction will fail. In most cases, it does not.

Quoted 1 line

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu.h new file +1 -0

+		return len;  

Quoted 4 lines

Quoted 1 line

+enum commands_e {

Are you sure the name is unique enough / properly namespaced? Same Q to all enums.

I can change it. I wanted it to be compatible with the header in the firmware repository.

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 19 Sep 2023 15:29:08 +0300 Andy Shevchenko <[email protected]> wrote:

On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:

...

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

obj-$(CONFIG_GOLDFISH) += goldfish/
obj-$(CONFIG_CHROME_PLATFORMS) += chrome/
obj-$(CONFIG_SURFACE_PLATFORMS) += surface/
+obj-$(CONFIG_CZNIC_PLATFORMS) += cznic/

Quoted 4 lines

Quoted 1 line

Why not ordered (to some extent) here (as you did in the other file)?

Where should I put it? The other entries are not ordered, see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2

With the visible context above, at least after chrome already much better, no?

Quoted 5 lines

Quoted 1 line

Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...

i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the the .c file also include all those headers if it includes a local header already?

For generic ones, yes.

Quoted 4 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (ret < 0)    

Can it return positive value? What would be the meaning of it?

Quoted 13 lines

Originally I had it return 0 on success, but recently I've had a discussion about this same thing with LED subsystem maintainer about some LED patches, Lee Jones, who requested the _read function to return number of bytes read on success. See at
https://lore.kernel.org/linux-leds/[email protected]/
where he says:
Read and write APIs, even abstracted ones such as these generally return the number of bytes successfully read and written respectively. If you are going to normalise, then please do so against this standard.
I do not agree with him, since this is a local command accessor which only succeeds if all the bytes are read/written as requested. But I adjusted my code as per his request since he is the maintainer.

This is strange. For example, regmap APIs never returns amount of data written or read. I think it's solely depends on the API. It might be useful for i²c APIs, in case you can do something about it. but if you have wrappers on top of that already (meaning not using directly the i2c_*() calls, I dunno the positive return is anyhow useful.

Quoted 7 lines

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 154

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	int status;    

My gosh, it's a _third_ name for the same!

The variable holds value of the status register, which is needed at another point in this function. I thought naming it just "ret" would cause confusions... Do you really propose to rename it such?

It's not bad per se, just put yourself on the place of somebody who would like to understand the semantics of all these variables. Maybe you can add a comment on top explaining that (in short)?

Quoted 8 lines

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu-base.c line 186

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-base.c new file +1 -0

+	for (int i = 0; i < ARRAY_SIZE(features); ++i) {    

Why signed?

Because it is shorter and makes no difference and there are tons of
for (int i = 0; i < ARRAY_SIZE(x); i++)
in the kernel already. But if you really want, I can change it. Please let me know.

It just makes harder to get the code, because of possible unneeded questions like "if it's a signed type, maybe it's on purpose?"

Quoted 4 lines

Quoted 1 line

Why pre-increment?

Again, it makes no difference, is widely used in the kernel and I personally prefer it. But I will change it if you want.

Pre-increment usually also a sign of something special about the iterator variable. In non-special cases we use post-increment. And if you count the use of each you will see the difference.

Quoted 6 lines

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu.h line 37

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu.h new file +1 -0

+	if (likely(ret == ARRAY_SIZE(msgs)))    

Why likely()? Please, justify.

Becuase it is unlikely the I2C transaction will fail. In most cases, it does not.

Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?

...

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+enum commands_e {

Are you sure the name is unique enough / properly namespaced? Same Q to all enums.

Quoted 2 lines

I can change it. I wanted it to be compatible with the header in the firmware repository.

Are these being generated, or also statically written in that repo? If the latter, I think better to use more unique naming schema in the kernel.

Quoted 4 lines · On Tue, 19 Sep 2023 21:27:04 +0300 Andy Shevchenko <[email protected]> wrote:

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 19 Sep 2023 15:29:08 +0300 Andy Shevchenko <[email protected]> wrote:

On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

drivers/platform/cznic/turris-omnia-mcu.h line 37

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu.h new file +1 -0

+	if (likely(ret == ARRAY_SIZE(msgs)))    

Why likely()? Please, justify.

Quoted 4 lines

Quoted 2 lines

Becuase it is unlikely the I2C transaction will fail. In most cases, it does not.

Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?

Compiler optimization (one branch avoided). But I guess this isn't a hot path, since I2C is insanely slow anyway. OK, I shall remove the likely() usage.

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

This include:

- 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

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +5 -0

+	switch (cmd) {
+	case CMD_GENERAL_CONTROL:
+		buf[1] = val;
+		buf[2] = mask;
+		len = 3;

(quoted diff) +1 -0

+	}

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +10 -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;
+
+		mutex_lock(&mcu->lock);
+		val = omnia_cmd_read_bit(mcu->client,
+					 CMD_GET_EXT_CONTROL_STATUS,

(quoted diff) +2 -0

+		return GPIO_LINE_DIRECTION_IN;
+}

(quoted diff) +2 -0

+		return -EOPNOTSUPP;
+}

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +11 -0

+	if (!gpio->ctl_cmd)
+		return;
+
+	mask = gpio->ctl_bit;
+	val = value ? mask : 0;
+
+	err = omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
+	if (err)
+		dev_err(&mcu->client->dev, "Cannot set GPIO %u: %d\n",
+			offset, err);
+}

(quoted diff) +2 -0

+		return true;
+}

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	}
+
+	return 0;
+}

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +5 -0

+		mcu->both = rising & falling;
+		mcu->is_cached &= mcu->both;
+	}
+
+	mutex_unlock(&mcu->lock);

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +10 -0

+	for (int i = 0; i < ngpios; ++i) {
+		const struct omnia_gpio *gpio = &omnia_gpios[i];
+
+		if (!gpio->int_bit)
+			continue;
+
+		if (omnia_gpio_available(mcu, gpio))
+			set_bit(i, valid_mask);
+	}
+}

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +2 -0

+	if (ret < 0)
+		return ret;

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id)
+{
+	struct omnia_mcu *mcu = dev_id;
+	irqreturn_t res = IRQ_NONE;

+}

+}

(quoted diff) +1 -0

+	}

+}

drivers/platform/cznic/turris-omnia-mcu.h +1 -0

+	bool button_pressed_emul;
 };

(quoted diff) +1 -0

+	return ret < 0 ? ret : 0;

Quoted 6 lines · Marek Behún

- 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

Can the main driver provide a regmap and all other use it?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (gpio->ctl_cmd)
+		return -EOPNOTSUPP;  

I believe internally we use ENOTSUPP.
Ditto for all cases like this.

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 439

Quoted 11 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +8 -0

+	mutex_lock(&mcu->lock);
+
+	if (ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu,
  CMD_GENERAL_CONTROL, ctl,
+					     ctl_mask);  
 
+	if (!err && ext_ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL,
  ext_ctl,
+					     ext_ctl_mask);  

Can it be

	if (err)
		goto out_unlock;

	if (_mask)
		...

Review

Quoted 5 lines

(quoted diff) +3 -0

+	mutex_unlock(&mcu->lock);  
 
+	if (err)
+		dev_err(&mcu->client->dev, "Cannot set GPIOs:
  %d\n", err);  

How is useful this one?

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 464

Quoted 7 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +6 -0

+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);  
 
+	bitmap_zero(valid_mask, ngpios);  

No need.

Also do you have bitops.h included?

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 673

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);  

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 882

Quoted 8 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +8 -0

+static struct attribute *omnia_mcu_gpio_attrs[] = {
+	&dev_attr_front_button_mode.attr,
+	NULL,
+};
+
+static const struct attribute_group omnia_mcu_gpio_group = {
+	.attrs = omnia_mcu_gpio_attrs,
+};  

ATTRIBUTE_GROUPS() ?

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 959

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +1 -0

+	err = devm_device_add_group(dev, &omnia_mcu_gpio_group);  

No way, no-one should use the API scheduled for removal. What's wrong with .dev_groups ?

...

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 972

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +5 -0

+void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
+{
+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);  

Wrong order?

drivers/platform/cznic/turris-omnia-mcu.h line 25

Quoted 7 lines

struct omnia_mcu {

drivers/platform/cznic/turris-omnia-mcu.h +3 -0

 	struct i2c_client *client;
 	const char *type;
 	u16 features;
+
+	/* GPIO chip */
+	struct gpio_chip gc;  

Making this a first member may lead to the better code. Check with
bloat-o-meter.

Quoted 1 line · On Tue, 19 Sep 2023 16:00:39 +0300 Andy Shevchenko <[email protected]> wrote:

On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:

Quoted 1 line

Ditto for all cases like this.

checkpatch warns:
ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

Linus suggested using guards, so I will refactor this away.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		dev_err(&mcu->client->dev, "Cannot set GPIOs:
  %d\n", err);  

How is useful this one?

The function does not return, this way we will know something has gone wrong. I am used to do this from the networking subsystem, where people at least from mv88e6xxx wanted such behavior. Do you want me to drop this?

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	bitmap_zero(valid_mask, ngpios);  

No need.

Also do you have bitops.h included?

bitmap.h actually for this

Quoted 7 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 673

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);  

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);

Am I understanding you correctly?

But then I would need to store the mask in driver data as a 64-bit value with half the data not used. Also the CPU is 32-bit.

Quoted 1 line

ATTRIBUTE_GROUPS() ?

I only want to create ane attribute_group. (I am using devm_device_add_group(), not devm_device_add_groups()).

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 959

Quoted 1 line

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +1 -0

+	err = devm_device_add_group(dev, &omnia_mcu_gpio_group);  

No way, no-one should use the API scheduled for removal. What's wrong with .dev_groups ?

Can I add them conditionally? GPIO chip is always created, but the patch 4/7 only adds the attributes conditionally. Is it possible via .dev_groups?

Quoted 8 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 972

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +5 -0

+void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
+{
+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);  

Wrong order?

No, the mutex may be used in the work. Can't destroy it first. Or am I misunderstanding something?

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu.h line 25

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu.h +3 -0

 	struct i2c_client *client;
 	const char *type;
 	u16 features;
+
+	/* GPIO chip */
+	struct gpio_chip gc;  

Making this a first member may lead to the better code. Check with
bloat-o-meter.

kabel@dellmb ~/linux $ ./scripts/bloat-o-meter \
turris-omnia-mcu.o turris-omnia-mcu-gpiochip-first.o
add/remove: 0/0 grow/shrink: 9/0 up/down: 52/0 (52)

Function                                     old     new   delta
omnia_mcu_register_gpiochip                  840     852     +12
omnia_mcu_probe                              696     708     +12
omnia_mcu_unregister_gpiochip                 20      24      +4
omnia_irq_bus_sync_unlock                    256     260      +4
omnia_irq_bus_lock                            36      40      +4
omnia_gpio_get_multiple                      872     876      +4
omnia_gpio_get                               372     376      +4
fw_features_show                              28      32      +4
front_button_mode_show                       260     264      +4

Total: Before=10468, After=10520, chg +0.50%

Seems the code grew when I swapped it.

Quoted 2 lines

checkpatch warns:
ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

So, fix checkpatch then. It has false positives, because it doesn't know if the actual error code will sneak outside the kernel or not. But in the subsystem we internally use ENOTSUPP.

Quoted 17 lines

Quoted 7 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 673

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);    

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);

Quoted 4 lines

Am I understanding you correctly?

But then I would need to store the mask in driver data as a 64-bit value with half the data not used. Also the CPU is 32-bit.

If you use proper bitmaps, perhaps this will be easier. You can use one for each and merge them whenever you want (with bitmap_or() call) or split (with bitmap_and() respectively):

bitmap_or(full, raising, failing); // merge bitmap_and(raising, full, rasing_mask); // split

Quoted 9 lines

Quoted 8 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 972

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +5 -0

+void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
+{
+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);    

Wrong order?

Quoted 2 lines

No, the mutex may be used in the work. Can't destroy it first. Or am I misunderstanding something?

I mean you are using a lot of devm(), can mutex be used in IRQ or whatever that can be triggered after this call?

On Thu, 21 Sep 2023 12:55:08 +0300
Andy Shevchenko <[email protected]> wrote:

Quoted 22 lines

Quoted 17 lines

Quoted 7 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 673

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);    

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);

Quoted 4 lines

Am I understanding you correctly?

But then I would need to store the mask in driver data as a 64-bit value with half the data not used. Also the CPU is 32-bit.

Quoted 6 lines

If you use proper bitmaps, perhaps this will be easier. You can use one for each and merge them whenever you want (with bitmap_or() call) or split (with bitmap_and() respectively):

bitmap_or(full, raising, failing); // merge bitmap_and(raising, full, rasing_mask); // split

Hmm. But then what? I or the result and use it as pending interrupt bitmap, to be iterated over. The indexes of the bits correspond to the constants in the MCU API.

So after your suggestion I have rising and falling containgin
rising = 00rr00rr00rr00rr; /* r means rising bits */ falling = 00ff00ff00ff00ff; /* f means falling bits */ pending = rising | falling;
which means:
pending = pp00pp00pp00pp; /* p means pending bits */
But these bit positions do not correspond to the interrupt number anymore.

I still think the de-interleaving of the buffer from
rr ff rr ff rr ff rr ff
into two words:
rising = rrrrrrrr;
falling = ffffffff;
is simpler...

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 972

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +3 -0

+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);    

Wrong order?

Quoted 5 lines

Quoted 2 lines

No, the mutex may be used in the work. Can't destroy it first. Or am I misunderstanding something?

I mean you are using a lot of devm(), can mutex be used in IRQ or whatever that can be triggered after this call?

OK, I think I need to free the irq before canceling the work. Thank you!

Quoted 2 lines · Marek Behún

On Thu, 21 Sep 2023 12:55:08 +0300
Andy Shevchenko <[email protected]> wrote:

...

Quoted 42 lines

Quoted 22 lines

Quoted 17 lines

Quoted 7 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 673

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);      

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);

Quoted 4 lines

Am I understanding you correctly?

But then I would need to store the mask in driver data as a 64-bit value with half the data not used. Also the CPU is 32-bit.

Quoted 6 lines

If you use proper bitmaps, perhaps this will be easier. You can use one for each and merge them whenever you want (with bitmap_or() call) or split (with bitmap_and() respectively):

bitmap_or(full, raising, failing); // merge bitmap_and(raising, full, rasing_mask); // split

Hmm. But then what? I or the result and use it as pending interrupt bitmap, to be iterated over. The indexes of the bits correspond to the constants in the MCU API.

So after your suggestion I have rising and falling containgin
rising = 00rr00rr00rr00rr; /* r means rising bits */ falling = 00ff00ff00ff00ff; /* f means falling bits */ pending = rising | falling;
which means:
pending = pp00pp00pp00pp; /* p means pending bits */
But these bit positions do not correspond to the interrupt number anymore.

Quoted 6 lines

I still think the de-interleaving of the buffer from
rr ff rr ff rr ff rr ff
into two words:
rising = rrrrrrrr;
falling = ffffffff;
is simpler...

There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's made there. But before going that way, check on
https://lore.kernel.org/all/[email protected]/
That APIs you would need I am pretty sure.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 972

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +3 -0

+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);      

Wrong order?

No, the mutex may be used in the work. Can't destroy it first. Or am I misunderstanding something?

Quoted 4 lines

Quoted 2 lines

I mean you are using a lot of devm(), can mutex be used in IRQ or whatever that can be triggered after this call?

OK, I think I need to free the irq before canceling the work. Thank you!

Can you rather switch everything to be devm managed?

Quoted 3 lines · On Fri, 22 Sep 2023 17:18:56 +0300 Andy Shevchenko <[email protected]> wrote:

Quoted 2 lines · Marek Behún

On Thu, 21 Sep 2023 12:55:08 +0300
Andy Shevchenko <[email protected]> wrote:

Quoted 49 lines

Quoted 42 lines

Quoted 22 lines

Quoted 17 lines

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
+		 (reply[6] << 24);
+	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
+		  (reply[7] << 24);      

With a help of two masks, you can access to the both edges as to
64-bit value and simplify the code.

Huh? As in
rising = reply & 0x00ff00ff00ff00ff;
falling = reply & 0xff00ff00ff00ff00;
?
But then I can't or the rising bit with the corresponding falling bit to get pending...
Or I guess i can with:
pending = rising & (pending >> 8);

Quoted 4 lines

Am I understanding you correctly?

But then I would need to store the mask in driver data as a 64-bit value with half the data not used. Also the CPU is 32-bit.

Quoted 6 lines

If you use proper bitmaps, perhaps this will be easier. You can use one for each and merge them whenever you want (with bitmap_or() call) or split (with bitmap_and() respectively):

bitmap_or(full, raising, failing); // merge bitmap_and(raising, full, rasing_mask); // split

Hmm. But then what? I or the result and use it as pending interrupt bitmap, to be iterated over. The indexes of the bits correspond to the constants in the MCU API.

So after your suggestion I have rising and falling containgin
rising = 00rr00rr00rr00rr; /* r means rising bits */ falling = 00ff00ff00ff00ff; /* f means falling bits */ pending = rising | falling;
which means:
pending = pp00pp00pp00pp; /* p means pending bits */
But these bit positions do not correspond to the interrupt number anymore.

Quoted 6 lines

I still think the de-interleaving of the buffer from
rr ff rr ff rr ff rr ff
into two words:
rising = rrrrrrrr;
falling = ffffffff;
is simpler...

Quoted 4 lines

There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's made there. But before going that way, check on
https://lore.kernel.org/all/[email protected]/
That APIs you would need I am pretty sure.

Andy, thank you for patience in reviewing this.

Hmm. I like the names, scatter and gather. In the firmware, I used interleave and deinterleave, see
https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.c#L360

But those functions work bit-wise. I realize that the I2C transfers in the driver are so slow that such bit-wise cycling over a bitmap won't matter much, but I still find my original proposal more simple and straight-forward. But I will cave if you insist. Please let me know (and can I then send your local patch in the series?)

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+	if (!(mcu->features & FEAT_NEW_INT_API))
+
  cancel_delayed_work_sync(&mcu->button_release_emul_work); +
+	mutex_destroy(&mcu->lock);      

Wrong order?

No, the mutex may be used in the work. Can't destroy it first. Or am I misunderstanding something?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

I mean you are using a lot of devm(), can mutex be used in IRQ or whatever that can be triggered after this call?

OK, I think I need to free the irq before canceling the work. Thank you!

Can you rather switch everything to be devm managed?

There are no devm_ calls for mutex and work initialization. Are you suggesting that I should write a release function for the gpio sub-driver? Something like

static void omnia_gpiochip_release(dev, res)
{
cancel_work();
mutex_destroy();
}

int omnia_mcu_register_gpiochip(mcu)
{
...
x = devres_alloc(omnia_gpiochip_release);
devres_add(dev, x);
...
}

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 439

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +8 -0

+	mutex_lock(&mcu->lock);
+
+	if (ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
+					     ctl_mask);  
 
+	if (!err && ext_ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
+					     ext_ctl_mask);  

Can it be

	if (err)
		goto out_unlock;

	if (_mask)
		...

?

On Tue, 19 Sep 2023 16:00:39 +0300
Andy Shevchenko <[email protected]> wrote:

Quoted 12 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 439

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +8 -0

+	mutex_lock(&mcu->lock);
+
+	if (ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
+					     ctl_mask);  
 
+	if (!err && ext_ctl_mask)
+		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
+					     ext_ctl_mask);  

Can it be

Quoted 7 lines

	if (err)
		goto out_unlock;

	if (_mask)
		...

?

Hi Andy,

so I am refactoring this to use guard(mutex), but now I have this:

	guard(mutex, &mcu->lock);

	if (ctl_mask) {
		err = ...;
		if (err)
			goto out_err;
	}

	if (ext_ctl_mask) {
		err = ...;
		if (err)
			goto out_err;
	}

	return;
out_err:
	dev_err(dev, "Cannot set GPIOs: %d\n", err);

which clearly is not any better... or at least the original

if (!err && ext_ctl_mask)
is better IMO.

Compare with:

	guard(mutex, &mcu->lock);

	if (ctl_mask)
		err = ...;

	if (!err && ext_ctl_mask)
		err = ...;

	if (err)
		dev_err(dev, "Cannot set GPIOs: %d\n", err);

Do you have a better suggestion?

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

This include:
- 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 <[email protected]>

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +5 -0

@@ -1,3 +1,19 @@
+What:          /sys/bus/i2c/devices/<mcu_device>/front_button_mode
+Date:          August 2023
+KernelVersion: 6.6
+Contact:       Marek Behún <[email protected]>
+Description:   (RW) The front button on the Turris Omnia router can be

drivers/platform/cznic/turris-omnia-mcu-base.c +23 -0

+#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)

drivers/platform/cznic/turris-omnia-mcu-base.c +2 -0

+static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{

drivers/platform/cznic/turris-omnia-mcu-base.c +4 -0

+static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+                                  unsigned long *bits)
+{
+       mutex_lock(&mcu->lock);

drivers/platform/cznic/turris-omnia-mcu-base.c +2 -0

+               if (err)
+                       goto err_unlock;

drivers/platform/cznic/turris-omnia-mcu-base.c +2 -0

+       if (err)
+               goto err_unlock;

drivers/platform/cznic/turris-omnia-mcu-base.c +6 -0

+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);
+

drivers/platform/cznic/turris-omnia-mcu-base.c +6 -0

+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);
+

drivers/platform/cznic/turris-omnia-mcu-base.c +5 -0

+static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
+                                      unsigned long *pending)
+{
+       u32 rising, falling;
+       u8 reply[8] = {};

+ mutex_lock(&mcu->lock);

drivers/platform/cznic/turris-omnia-mcu-base.c +9 -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);
+}

drivers/platform/cznic/turris-omnia-mcu-base.c +20 -0

+       /*
+        * 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

+ mcu->gc.names = omnia_mcu_gpio_names;

On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <[email protected]> wrote:

drivers/platform/cznic/turris-omnia-mcu-gpio.c line 532

Quoted 4 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +4 -0

+       if (type & IRQ_TYPE_EDGE_RISING)
+               mcu->rising |= bit;
+       else
+               mcu->rising &= ~bit;

And with bits in place of bitmasks these would be

if ()
__set_bit(bit, mcu->rising);
else
__clear_bit(bit, mcu->rising);

Quoted 15 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +12 -0

@@ -1,3 +1,19 @@
+What:          /sys/bus/i2c/devices/<mcu_device>/front_button_mode
+Date:          August 2023
+KernelVersion: 6.6
+Contact:       Marek Behún <[email protected]>
+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.

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu line 15

Quoted 3 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +3 -0

+               - "cpu" makes the button press event be handled by the CPU.
+
+               Format: %s.  

I understand what this does, but why does this have to be configured at runtime from userspace sysfs? Why can't this just be a property in device tree or ACPI (etc)? I don't imagine a user is going to change this back and forth are they? They likely want one or another.

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu line 19

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +4 -0

+static const struct omnia_gpio {
+       u8 cmd, ctl_cmd;
+       u32 bit, ctl_bit;
+       u32 int_bit;  

If they are really just ONE bit I would actually use an int, encode the bit number rather than the mask, and then use BIT( ..->int_bit) from <linux/bits.h> everywhere I need a mask. No strong preference though.

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu

Quoted 5 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +5 -0

+/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
+static const unsigned int omnia_int_to_gpio_idx[32] = {
+       [ilog2(INT_CARD_DET)]           = 4,
+       [ilog2(INT_MSATA_IND)]          = 5,
+       [ilog2(INT_USB30_OVC)]          = 6,  

ilog2 is a bit unituitive for a programmer, are you a schooled mathematician Marek? ;)

It is also a consequence of using a bitmask rather than a bit number in the struct, all ilog2 does is fls-1. So what about just putting the bit number in the struct and then all of these assignments will look natural as well.

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +10 -0

+static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
+                                      unsigned long *pending)
+{
+       u32 rising, falling;
+       u8 reply[8] = {};
+       size_t len;
+       int err;
+
+       /*
+        * Determine how many bytes we need to read from the reply to the

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu

Quoted 5 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +5 -0

+        * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked
+        * interrupts.
+        */
+       len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1,
+                 ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2);  

Really hard to read, and again easier if we store the bit number rather than the bitmask.

Quoted 9 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +9 -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);
+}  

(...)

Quoted 20 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +20 -0

+       /*
+        * 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

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +10 -0

+        * time.
+        */
+
+       if (status & STS_BUTTON_PRESSED) {
+               mcu->button_pressed_emul = true;
+               mod_delayed_work(system_wq, &mcu->button_release_emul_work,
+                                msecs_to_jiffies(50));
+       } else if (mcu->button_pressed_emul) {
+               status |= STS_BUTTON_PRESSED;
+       }  

I feel a bit icky about this, would be best if Dmitry Torokhov review such input subsystem-related things.

It looks like a reimplementation of drivers/input/keyboard/gpio_keys.c so why not just connect a gpio-keys device using either device tree or a software node? (Dmitry knows all about how to create proper software nodes for stuff like this, DT is self-evident.)

Quoted 1 line

+ mcu->gc.names = omnia_mcu_gpio_names;

Do we really support format strings in these names? I don't think so, or I'm wrong about my own code... (Which wouldn't be the first time.)

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu

Quoted 4 lines

#include <asm/byteorder.h>

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +2 -0

+#include <linux/gpio/driver.h>
 #include <linux/i2c.h>
+#include <linux/log2.h>  

I think we want to get rid of log2.h from this driver.

I could use ffs(x) instead of ilog2(x) + 1.

Quoted 1 line · On Thu, 21 Sep 2023 21:45:57 +0200 Marek Behún <[email protected]> wrote:

I could use ffs(x) instead of ilog2(x) + 1.

Pardon me, I meant fls(). Or maybe get_bitmask_order() from linux/bitops.h.

Add support for true board poweroff (MCU can disable all unnecessary voltage regulators) and wakeup at a specified time, implemented via a RTC driver so that the rtcwake utility can be used to configure it.

Signed-off-by Marek Behún <[email protected]>

---
 .../sysfs-bus-i2c-devices-turris-omnia-mcu    |  16 ++
 drivers/platform/cznic/Kconfig                |   4 +
 drivers/platform/cznic/Makefile               |   1 +
 .../platform/cznic/turris-omnia-mcu-base.c    |   4 +
 .../platform/cznic/turris-omnia-mcu-gpio.c    |   2 -
 .../cznic/turris-omnia-mcu-sys-off-wakeup.c   | 245 ++++++++++++++++++
 drivers/platform/cznic/turris-omnia-mcu.h     |  19 ++
 7 files changed, 289 insertions(+), 2 deletions(-)
 create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu +16 -0

index c0efdf1b3803..d668b8e999e6 100644
@@ -14,6 +14,22 @@ Description:	(RW) The front button on the Turris Omnia router can be
 
 		Format: %s.
 
+What:		/sys/bus/i2c/devices/<mcu_device>/front_button_poweron
+Date:		August 2023
+KernelVersion:	6.6
+Contact:	Marek Behún <[email protected]>
+Description:	(RW) Newer versions of the microcontroller firmware of the
+		Turris Omnia router support powering off the router into true
+		low power mode. The router can be powered on by pressing the
+		front button.
+
+		This file configures whether front button power on is enabled.
+
+		This file is present only if the power off feature is supported
+		by the firmware.
+
+		Format: %i.
+
 What:		/sys/bus/i2c/devices/<mcu_device>/fw_features
 Date:		August 2023
 KernelVersion:	6.6

drivers/platform/cznic/Kconfig +4 -0

index 3a8c3edcd7e6..0a752aa654fa 100644
@@ -19,10 +19,14 @@ config TURRIS_OMNIA_MCU
 	depends on I2C
 	select GPIOLIB
 	select GPIOLIB_IRQCHIP
+	select RTC_CLASS
 	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:
+	  - board poweroff into true low power mode (with voltage regulators
+	    disabled) and the ability to configure wake up from this mode (via
+	    rtcwake)
 	  - 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

drivers/platform/cznic/Makefile +1 -0

index a6177f5b4fff..6f1470d1f673 100644
@@ -3,3 +3,4 @@
 obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
 turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o
 turris-omnia-mcu-objs		+= turris-omnia-mcu-gpio.o
+turris-omnia-mcu-objs		+= turris-omnia-mcu-sys-off-wakeup.o

drivers/platform/cznic/turris-omnia-mcu-base.c +4 -0

index 2954151468e5..b507f81ebe0a 100644
@@ -226,6 +226,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = omnia_mcu_register_sys_off_and_wakeup(mcu);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 

drivers/platform/cznic/turris-omnia-mcu-gpio.c +0 -2

index e8d5eeb4eb31..3d34a32bc7b0 100644
@@ -5,8 +5,6 @@
  * 2023 by Marek Behún <[email protected]>
  */
 
-#include <asm/unaligned.h>
-
 #include "turris-omnia-mcu.h"
 
 static const char * const omnia_mcu_gpio_names[64] = {

drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c new file +245 -0

new file mode 100644
index 000000000000..07ae92d819e3
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU system off and RTC wakeup driver
+ *
+ * This is not a true RTC driver (in the sense that it does not provide a
+ * real-time clock), rather the MCU implements a wakeup from powered off state
+ * at a specified time relative to MCU boot, and we expose this feature via RTC
+ * alarm, so that it can be used via the rtcwake command, which is the standard
+ * Linux command for this.
+ *
+ * 2023 by Marek Behún <[email protected]>
+ */
+
+#include <linux/crc32.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
+
+#include "turris-omnia-mcu.h"
+
+static int omnia_get_uptime_wakeup(const struct i2c_client *client, u32 *uptime,
+				   u32 *wakeup)
+{
+	u32 reply[2];
+	int err;
+
+	err = omnia_cmd_read(client, CMD_GET_UPTIME_AND_WAKEUP, reply,
+			     sizeof(reply));
+	if (err)
+		return err;
+
+	if (uptime)
+		*uptime = le32_to_cpu(reply[0]);
+
+	if (wakeup)
+		*wakeup = le32_to_cpu(reply[1]);
+
+	return 0;
+}
+
+static int omnia_read_time(struct device *dev, struct rtc_time *tm)
+{
+	u32 uptime;
+	int err;
+
+	err = omnia_get_uptime_wakeup(to_i2c_client(dev), &uptime, NULL);
+	if (err)
+		return err;
+
+	rtc_time64_to_tm(uptime, tm);
+
+	return 0;
+}
+
+static int omnia_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_mcu *mcu = i2c_get_clientdata(client);
+	u32 wakeup;
+	int err;
+
+	err = omnia_get_uptime_wakeup(client, NULL, &wakeup);
+	if (err)
+		return err;
+
+	alrm->enabled = !!wakeup;
+	rtc_time64_to_tm(wakeup ?: mcu->rtc_alarm, &alrm->time);
+
+	return 0;
+}
+
+static int omnia_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_mcu *mcu = i2c_get_clientdata(client);
+
+	mcu->rtc_alarm = rtc_tm_to_time64(&alrm->time);
+
+	if (alrm->enabled)
+		return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
+					   mcu->rtc_alarm);
+	else
+		return 0;
+}
+
+static int omnia_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_mcu *mcu = i2c_get_clientdata(client);
+
+	return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
+				   enabled ? mcu->rtc_alarm : 0);
+}
+
+static const struct rtc_class_ops omnia_rtc_ops = {
+	.read_time		= omnia_read_time,
+	.read_alarm		= omnia_read_alarm,
+	.set_alarm		= omnia_set_alarm,
+	.alarm_irq_enable	= omnia_alarm_irq_enable,
+};
+
+static int omnia_power_off(struct sys_off_data *data)
+{
+	struct omnia_mcu *mcu = data->cb_data;
+	u8 cmd[9];
+	u32 tmp;
+	u16 arg;
+	int err;
+
+	if (mcu->front_button_poweron)
+		arg = CMD_POWER_OFF_POWERON_BUTTON;
+	else
+		arg = 0;
+
+	cmd[0] = CMD_POWER_OFF;
+	put_unaligned_le16(CMD_POWER_OFF_MAGIC, &cmd[1]);
+	put_unaligned_le16(arg, &cmd[3]);
+
+	tmp = cpu_to_be32(get_unaligned_le32(&cmd[1]));
+	put_unaligned_le32(crc32_be(0xffffffff, (void *)&tmp, sizeof(tmp)),
+			   &cmd[5]);
+
+	err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+	if (err)
+		dev_err(&mcu->client->dev,
+			"Unable to send the poweroff command: %d\n", err);
+
+	return NOTIFY_DONE;
+}
+
+static int omnia_restart(struct sys_off_data *data)
+{
+	struct omnia_mcu *mcu = data->cb_data;
+	u8 cmd[3];
+	int err;
+
+	cmd[0] = CMD_GENERAL_CONTROL;
+
+	if (reboot_mode == REBOOT_HARD)
+		cmd[1] = cmd[2] = CTL_HARD_RST;
+	else
+		cmd[1] = cmd[2] = CTL_LIGHT_RST;
+
+	err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
+	if (err)
+		dev_err(&mcu->client->dev,
+			"Unable to send the restart command: %d\n", err);
+
+	/*
+	 * MCU needs a little bit to process the I2C command, otherwise it will
+	 * do a light reset based on SOC SYSRES_OUT pin.
+	 */
+	mdelay(1);
+
+	return NOTIFY_DONE;
+}
+
+static ssize_t front_button_poweron_show(struct device *dev,
+					 struct device_attribute *a, char *buf)
+{
+	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+
+	return sysfs_emit(buf, "%d\n", mcu->front_button_poweron);
+}
+
+static ssize_t front_button_poweron_store(struct device *dev,
+					  struct device_attribute *a,
+					  const char *buf, size_t count)
+{
+	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+	bool val;
+
+	if (kstrtobool(buf, &val) < 0)
+		return -EINVAL;
+
+	mcu->front_button_poweron = val;
+
+	return count;
+}
+static DEVICE_ATTR_RW(front_button_poweron);
+
+static struct attribute *omnia_mcu_poweroff_attrs[] = {
+	&dev_attr_front_button_poweron.attr,
+	NULL,
+};
+
+static const struct attribute_group omnia_mcu_poweroff_group = {
+	.attrs = omnia_mcu_poweroff_attrs,
+};
+
+int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu)
+{
+	struct device *dev = &mcu->client->dev;
+	int err;
+
+	/* MCU restart is always available */
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    omnia_restart, mcu);
+	if (err) {
+		dev_err(dev, "Cannot register system restart handler: %d\n",
+			err);
+		return err;
+	}
+
+	/*
+	 * poweroff and wakeup are available only if POWEROFF_WAKEUP feature is
+	 * present
+	 */
+	if (!(mcu->features & FEAT_POWEROFF_WAKEUP))
+		return 0;
+
+	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
+					    SYS_OFF_PRIO_FIRMWARE,
+					    omnia_power_off, mcu);
+	if (err) {
+		dev_err(dev, "Cannot register system power off handler: %d\n",
+			err);
+		return err;
+	}
+
+	mcu->rtcdev = devm_rtc_allocate_device(dev);
+	if (IS_ERR(mcu->rtcdev)) {
+		err = PTR_ERR(mcu->rtcdev);
+		dev_err(dev, "Cannot allocate RTC device: %d\n", err);
+		return err;
+	}
+
+	mcu->rtcdev->ops = &omnia_rtc_ops;
+	mcu->rtcdev->range_max = U32_MAX;
+
+	err = devm_rtc_register_device(mcu->rtcdev);
+	if (err) {
+		dev_err(dev, "Cannot register RTC device: %d\n", err);
+		return err;
+	}
+
+	mcu->front_button_poweron = true;
+
+	err = devm_device_add_group(dev, &omnia_mcu_poweroff_group);
+	if (err)
+		dev_err(dev, "Cannot add poweroff sysfs attribute group: %d\n",
+			err);
+
+	return err;
+}

drivers/platform/cznic/turris-omnia-mcu.h +19 -0

index b5802240aa8d..c40ac07be5f5 100644
@@ -9,10 +9,12 @@
 #define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
 
 #include <asm/byteorder.h>
+#include <asm/unaligned.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/log2.h>
 #include <linux/mutex.h>
+#include <linux/rtc.h>
 #include <linux/turris-omnia-mcu-interface.h>
 #include <linux/workqueue.h>
 

@@ -29,6 +31,11 @@ struct omnia_mcu {
 	u16 last_status;
 	struct delayed_work button_release_emul_work;
 	bool button_pressed_emul;
+
+	/* RTC device for configuring wake-up */
+	struct rtc_device *rtcdev;
+	u32 rtc_alarm;
+	bool front_button_poweron;
 };
 
 static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,

@@ -39,6 +46,17 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
 	return ret < 0 ? ret : 0;
 }
 
+static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
+				      u32 val)
+{
+	u8 buf[5];
+
+	buf[0] = cmd;
+	put_unaligned_le32(val, &buf[1]);
+
+	return omnia_cmd_write(client, buf, sizeof(buf));
+}
+
 static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
 				 unsigned int len)
 {

@@ -117,5 +135,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
 
 int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
 void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu);
+int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
 
 #endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */