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

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
Changes from v4:
- #gpio-cells changed from 2 (pin number, flags) to 3 (bank, pin number, flags). This is because two years ago we added device-tree fixing code into U-Boot [1] which adds reset-gpios to PCIe controller nodes if these GPIOs are controllable via MCU, and the code adds the gpio specifiers to have 3 cells. Implementing this driver with twocell specifier breaks PCIe functionality for users which have upgraded U-Boot
- added documentation for #interrupt-cells and #gpio-cells

Quoted 6 lines · Marek Behún

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

Reviewed-by Krzysztof Kozlowski <[email protected]>

Quoted 10 lines

---
Changes from v4:
- #gpio-cells changed from 2 (pin number, flags) to 3 (bank, pin number, flags). This is because two years ago we added device-tree fixing code into U-Boot [1] which adds reset-gpios to PCIe controller nodes if these GPIOs are controllable via MCU, and the code adds the gpio specifiers to have 3 cells. Implementing this driver with twocell specifier breaks PCIe functionality for users which have upgraded U-Boot
- added documentation for #interrupt-cells and #gpio-cells

Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline), work on fork of kernel (don't, instead use mainline) or you ignore some maintainers (really don't). Just use b4 and everything should be fine, although remember about b4 prep --auto-to-cc if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time, thus I will skip this patch entirely till you follow the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

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

+#include <linux/sysfs.h>

+module_i2c_driver(omnia_mcu_driver);

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

+#include <linux/i2c.h>
+#include <linux/if_ether.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>

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

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

+#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */

Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti:

Quoted 2 lines

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

...

drivers/platform/cznic/Makefile line 4

Quoted 6 lines

+++ b/drivers/platform/cznic/Makefile

drivers/platform/cznic/Makefile new file +4 -0

@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
+turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o  

'objs' is for user space. You need to use 'y'. Same applies to the entire series.

+ array_size.h
+ bits.h

+ string.h

Quoted 2 lines

+#include <linux/sysfs.h>

...

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

Quoted 3 lines

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

+	err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
+						       CMD_GET_FW_VERSION_APP,
+			     reply, sizeof(reply));  

Wouldn't be better to have a logical split?

	err = omnia_cmd_read(mcu->client,
			     bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP,
			     reply, sizeof(reply));

	err = omnia_cmd_read(mcu->client,
			     bootloader ? CMD_GET_FW_VERSION_BOOT
					: CMD_GET_FW_VERSION_APP,
			     reply, sizeof(reply));

?

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));  

What's wrong with dev_get_drvdata()?

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
+				char *buf)  

One line?

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

Quoted 9 lines

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

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

__ATTRIBUTE_GROUPS()

...

Quoted 8 lines

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

+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,
+	},
+};  

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 __TURRIS_OMNIA_MCU_H
+#define __TURRIS_OMNIA_MCU_H  

+ array_size.h

Quoted 5 lines

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

+#include <linux/i2c.h>
+#include <linux/if_ether.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>  

...

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

Quoted 3 lines

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

+static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
+				 void *reply, unsigned int len)
+{  

Why is this in the header?

Quoted 21 lines

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

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

...

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

Quoted 4 lines

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

+#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H
+#define __TURRIS_OMNIA_MCU_INTERFACE_H
+
+#include <linux/bits.h>  

+ bitfield.h

Quoted 2 lines

+#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */

Hi Andy,

thank you very much for the review. I have some notes and some questions, see below.

Quoted 1 line · On Sun, 24 Mar 2024 13:01:55 +0200 Andy Shevchenko <[email protected]>…

Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti:

Quoted 9 lines

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

Quoted 3 lines

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

+   err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
+                                                  CMD_GET_FW_VERSION_APP,
+                        reply, sizeof(reply));

Wouldn't be better to have a logical split?

      err = omnia_cmd_read(mcu->client,
                           bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP,
                           reply, sizeof(reply));

Changed for v6 to

Quoted 4 lines

      err = omnia_cmd_read(mcu->client,
                           bootloader ? CMD_GET_FW_VERSION_BOOT
                                      : CMD_GET_FW_VERSION_APP,
                           reply, sizeof(reply));

There are still some people wanting only 80 columns, and the whole driver is written that way.

Quoted 1 line

?

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
+                           char *buf)

One line?

80 columns...

Quoted 6 lines

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

Quoted 4 lines

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

+static const struct attribute_group *omnia_mcu_groups[] = {
+   &omnia_mcu_base_group,
+   NULL
+};

__ATTRIBUTE_GROUPS()

The next patches add more groups into this array, after the whole series it looks like this:

static const struct attribute_group *omnia_mcu_groups[] = {
&omnia_mcu_base_group, &omnia_mcu_gpio_group, &omnia_mcu_poweroff_group,
NULL
};

There is no macro for that.

Should I still use __ATTRIBUTE_GROUPS() in the first patch and than change it in the next one?

Quoted 5 lines

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

Quoted 3 lines

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

+static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
+                            void *reply, unsigned int len)
+{

Why is this in the header?

I considered it a helper function that should be defined in the header file, like the rest of the cmd helpers in this file. If you disagree, I will put it into the -base.c file.

Quoted 20 lines

Quoted 20 lines

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

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

Quoted 5 lines · Marek Behún

Hi Andy,

thank you very much for the review. I have some notes and some questions, see below.

Btw, I'll look into other patches next week.

Quoted 3 lines

There are still some people wanting only 80 columns, and the whole driver is written that way.

Hmm... Is it still a hard limit for drivers/platform/cznic for the _new_ code?

Quoted 3 lines

I considered it a helper function that should be defined in the header file, like the rest of the cmd helpers in this file. If you disagree, I will put it into the -base.c file.

I don't see the technical justification to hold it in the *.h rather than *.c. To me this one is big enough in C and likely in assembly to be copied to each user. Besides that aspect, it slows down the build a lot (mostly due to i2c.h inclusion which otherwise is not needed).

Quoted 5 lines · Marek Behún

Hi Andy,

thank you very much for the review. I have some notes and some questions, see below.

Btw, I'll look into other patches next week.

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

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

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

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

+}

Hi Marek,

I can't say I did a proper review but browsing through the code without proper understanding of the platform raised one small question :)

Quoted 11 lines · 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 <[email protected]>

...

Quoted 7 lines

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

This means the 'rising' and 'falling' should always be little-endian? Should the parameter types reflect this? Or should we see some cpu_to_le() calls? (Or, is this code just guaranteed to be always running on a le-machine?).

Quoted 31 lines

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

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

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

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

Quoted 5 lines

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

+
+	for (int i = 0; i < sizeof(u32); ++i) {
+		*rising |= src[2 * i] << (8 * i);
+		*falling |= src[2 * i + 1] << (8 * i);
+	}  

Also here I could expect seeing le_to_cpu() unless I am (again :]) missing something.

Quoted 63 lines · On Mon, 25 Mar 2024 11:10:04 +0200 Matti Vaittinen <[email protected]>…

Hi Marek,

I can't say I did a proper review but browsing through the code without proper understanding of the platform raised one small question :)

Quoted 11 lines · 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 <[email protected]>

...

Quoted 7 lines

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

This means the 'rising' and 'falling' should always be little-endian? Should the parameter types reflect this? Or should we see some cpu_to_le() calls? (Or, is this code just guaranteed to be always running on a le-machine?).

Quoted 31 lines

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

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

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

Quoted 8 lines

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

Quoted 5 lines

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

+
+	for (int i = 0; i < sizeof(u32); ++i) {
+		*rising |= src[2 * i] << (8 * i);
+		*falling |= src[2 * i + 1] << (8 * i);
+	}

Also here I could expect seeing le_to_cpu() unless I am (again :]) missing something.

I don't understand. The rising and falling variables have native endiannes, as they should have.

And the src and dst are u8 arrays, which contain two LE32 unsigned integers, but these integers are interleaved. I am converting then to two native unsigned integers byte by byte, so I am basically doing what a theoretical le32_interleaved_le32_to_cpu() would have done if it did exist. Putting another le*_to_cpu() would only break things.

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

+
+	i = sysfs_match_string(front_button_modes, buf);
+	if (i < 0)
+		return i;
+
+	val = i ? mask : 0;
+	err = omnia_ctl_cmd_locked(mcu, CMD_GENERAL_CONTROL, val, mask);
+	if (err)
+		return err;
+
+	return count;
+}
+static DEVICE_ATTR_RW(front_button_mode);

Add resource managed version of irq_create_mapping(), to help drivers automatically dispose a linux irq mapping when driver is detached.

The new function devm_irq_create_mapping() is not yet used, but the action function can be used in the FSL CAAM driver.

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

---
  drivers/crypto/caam/jr.c     |  8 ++----
  include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
  2 files changed, 56 insertions(+), 6 deletions(-)

drivers/crypto/caam/jr.c +2 -6

index 26eba7de3fb0..ad0295b055f8 100644
@@ -7,6 +7,7 @@
   * Copyright 2019, 2023 NXP
   */
 
+#include <linux/devm-helpers.h>
  #include <linux/of_irq.h>
  #include <linux/of_address.h>
  #include <linux/platform_device.h>

@@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
  	return error;
  }
 
-static void caam_jr_irq_dispose_mapping(void *data)
-{
-	irq_dispose_mapping((unsigned long)data);
-}
-
  /*
   * Probe routine for each detected JobR subsystem.
   */

@@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
  		return -EINVAL;
  	}
 
-	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
+	error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
  					 (void *)(unsigned long)jrpriv->irq);
  	if (error)
  		return error;

include/linux/devm-helpers.h +0 -0

index 74891802200d..3805551fd433 100644

include/linux/devm-helpers.h +52 -0

  static inline void devm_delayed_work_drop(void *res)

@@ -76,4 +78,56 @@ static inline int devm_work_autocancel(struct device *dev,
  	return devm_add_action(dev, devm_work_drop, w);
  }
 
+/**
+ * devm_irq_mapping_drop - devm action for disposing an irq mapping
+ * @res:	linux irq number cast to the void * type
+ *
+ * devm_irq_mapping_drop() can be used as an action parameter for the
+ * devm_add_action_or_reset() function in order to automatically dispose
+ * a linux irq mapping when a device driver is detached.
+ */
+static inline void devm_irq_mapping_drop(void *res)
+{
+	irq_dispose_mapping((unsigned int)(unsigned long)res);
+}
+
+/**
+ * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
+ * @dev:	Device which lifetime the mapping is bound to
+ * @domain:	domain owning this hardware interrupt or NULL for default domain
+ * @hwirq:	hardware irq number in that domain space
+ *
+ * Create an irq mapping to linux irq space which is automatically disposed when
+ * the driver is detached.
+ * devm_irq_create_mapping() can be used to omit the explicit
+ * irq_dispose_mapping() call when driver is detached.
+ *
+ * Returns a linux irq number on success, 0 if mapping could not be created, or
+ * a negative error number if devm action could not be added.
+ */
+static inline int devm_irq_create_mapping(struct device *dev,
+					  struct irq_domain *domain,
+					  irq_hw_number_t hwirq)
+{
+	unsigned int virq = irq_create_mapping(domain, hwirq);
+
+	if (!virq)
+		return 0;
+
+	/*
+	 * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
+	 * disabled. No need to register an action in that case.
+	 */
+	if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
+		int err;
+
+		err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
+					       (void *)(unsigned long)virq);
+		if (err)
+			return err;
+	}
+
+	return virq;
+}
+
  #endif

Quoted 50 lines · Marek Behún

Add resource managed version of irq_create_mapping(), to help drivers automatically dispose a linux irq mapping when driver is detached.

The new function devm_irq_create_mapping() is not yet used, but the action function can be used in the FSL CAAM driver.

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

---
  drivers/crypto/caam/jr.c     |  8 ++----
  include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
  2 files changed, 56 insertions(+), 6 deletions(-)

drivers/crypto/caam/jr.c +2 -6

index 26eba7de3fb0..ad0295b055f8 100644
@@ -7,6 +7,7 @@
   * Copyright 2019, 2023 NXP
   */
 
+#include <linux/devm-helpers.h>
  #include <linux/of_irq.h>
  #include <linux/of_address.h>
  #include <linux/platform_device.h>

@@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
  	return error;
  }
 
-static void caam_jr_irq_dispose_mapping(void *data)
-{
-	irq_dispose_mapping((unsigned long)data);
-}
-
  /*
   * Probe routine for each detected JobR subsystem.
   */

@@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
  		return -EINVAL;
  	}
 
-	error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
+	error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
  					 (void *)(unsigned long)jrpriv->irq);
  	if (error)
  		return error;

include/linux/devm-helpers.h +0 -0

index 74891802200d..3805551fd433 100644

include/linux/devm-helpers.h line 29

Quoted 6 lines

include/linux/devm-helpers.h +2 -0

@@ -24,6 +24,8 @@
   */
 
  #include <linux/device.h>
+#include <linux/kconfig.h>
+#include <linux/irqdomain.h>
  #include <linux/workqueue.h>  

My confidence level is not terribly high today, so I am likely to accept just about any counter arguments :) But ... More I think of this whole header, less convinced I am that this (the header) is a great idea. I wonder who has authored a concept like this... :rolleyes:

Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in one header has potential to be including a lot of unneeded stuff to the users. I am under impression this can be bad for example for the build times.

I think that ideally the devm-APIs should live close to their non-devm counterparts, and this header should be just used as a last resort, when all the other options fail :) May I assume all other options have failed for the IRQ stuff?

Well, I will leave the big picture to the bigger minds. When just looking at the important things like the function names and coding style
- this change looks Ok to me ;)

+/**
+ * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
+ * @dev: Device which lifetime the mapping is bound to
+ * @domain: domain owning this hardware interrupt or NULL for default domain

+ * @hwirq: hardware irq number in that domain space
+ *
+ * Create an irq mapping to linux irq space which is automatically disposed when
+ * the driver is detached.
+ * devm_irq_create_mapping() can be used to omit the explicit
+ * irq_dispose_mapping() call when driver is detached.
+ *
+ * Returns a linux irq number on success, 0 if mapping could not be created, or

include/linux/devm-helpers.h +17 -0

+
+	/*
+	 * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
+	 * disabled. No need to register an action in that case.
+	 */
+	if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
+		int err;
+
+		err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
+					       (void *)(unsigned long)virq);
+		if (err)
+			return err;
+	}
+
+	return virq;
+}
+
 #endif

Signature · 1 line

2.43.2

Quoted 4 lines · Marek Behún

+/**
+ * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
+ * @dev: Device which lifetime the mapping is bound to
+ * @domain: domain owning this hardware interrupt or NULL for default domain

Quoted 8 lines

+ * @hwirq: hardware irq number in that domain space
+ *
+ * Create an irq mapping to linux irq space which is automatically disposed when
+ * the driver is detached.
+ * devm_irq_create_mapping() can be used to omit the explicit
+ * irq_dispose_mapping() call when driver is detached.
+ *
+ * Returns a linux irq number on success, 0 if mapping could not be created, or

                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

include/linux/devm-helpers.h static inline int devm_work_autocancel(struct device *dev, · line 115

Quoted 10 lines

+ * a negative error number if devm action could not be added.
+ */

include/linux/devm-helpers.h +8 -0

+static inline int devm_irq_create_mapping(struct device *dev,
+					  struct irq_domain *domain,
+					  irq_hw_number_t hwirq)
+{
+	unsigned int virq = irq_create_mapping(domain, hwirq);
+
+	if (!virq)
+		return 0;  

What is the point of returning zero instead of an error code? Neither of the callers that are introduced later in the patchset use this.

I understand that it matches some of the other legacy irq function behaviors, but I think we are trying to move away from that because it just leads to bugs.

Since we don't need the zero now, let's wait until we have a user before introducing this behavior. Then we can add a new function that returns zero, but we'll still encourage people to use the standard error code function where possible. And at the same time, when we do introduce the zero is an error code, function you should contact [email protected] so someone an write a static checker rule to detect the bugs that result from it.

Quoted 33 lines · On Tue, 26 Mar 2024 12:00:25 +0300 Dan Carpenter <[email protected]>…

Quoted 4 lines · Marek Behún

+/**
+ * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
+ * @dev: Device which lifetime the mapping is bound to
+ * @domain: domain owning this hardware interrupt or NULL for default domain

Quoted 8 lines

+ * @hwirq: hardware irq number in that domain space
+ *
+ * Create an irq mapping to linux irq space which is automatically disposed when
+ * the driver is detached.
+ * devm_irq_create_mapping() can be used to omit the explicit
+ * irq_dispose_mapping() call when driver is detached.
+ *
+ * Returns a linux irq number on success, 0 if mapping could not be created, or

                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

include/linux/devm-helpers.h static inline int devm_work_autocancel(struct device *dev, · line 115

Quoted 10 lines

+ * a negative error number if devm action could not be added.
+ */

include/linux/devm-helpers.h +8 -0

+static inline int devm_irq_create_mapping(struct device *dev,
+					  struct irq_domain *domain,
+					  irq_hw_number_t hwirq)
+{
+	unsigned int virq = irq_create_mapping(domain, hwirq);
+
+	if (!virq)
+		return 0;  

What is the point of returning zero instead of an error code? Neither of the callers that are introduced later in the patchset use this.

I understand that it matches some of the other legacy irq function behaviors, but I think we are trying to move away from that because it just leads to bugs.

Quoted 7 lines

Since we don't need the zero now, let's wait until we have a user before introducing this behavior. Then we can add a new function that returns zero, but we'll still encourage people to use the standard error code function where possible. And at the same time, when we do introduce the zero is an error code, function you should contact [email protected] so someone an write a static checker rule to detect the bugs that result from it.

Hi Dan,

the first user of this function is the very next patch of this series, and it does this:

(quoted diff) +4 -0

+	irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+	if (irq <= 0)
+		return dev_err_probe(dev, irq ?: -ENXIO,
+				     "Cannot map MESSAGE_SIGNED IRQ\n");

So it handles !irq as -ENXIO.

I looked into several users who do
virq = irq_create_mapping()
and then reutrn errno if !virq:

git grep -A 3 'virq = irq_create_mapping'

Some return -ENOMEM, some -ENXIO, some -EINVAL.

What do you think?

A few drivers register a devm action to remove a debugfs directory, implementing a one-liner function that calls debufs_remove_recursive(). Help drivers avoid this repeated implementations by adding managed version of debugfs directory create function.

Use the new function devm_debugfs_create_dir() in the following drivers:

   drivers/crypto/caam/ctrl.c
   drivers/gpu/drm/bridge/ti-sn65dsi86.c
   drivers/hwmon/hp-wmi-sensors.c
   drivers/hwmon/mr75203.c
   drivers/hwmon/pmbus/pmbus_core.c

A few drivers register a devm action to remove a debugfs directory, implementing a one-liner function that calls debufs_remove_recursive(). Help drivers avoid this repeated implementations by adding managed version of debugfs directory create function.

Use the new function devm_debugfs_create_dir() in the following drivers:

   drivers/crypto/caam/ctrl.c
   drivers/gpu/drm/bridge/ti-sn65dsi86.c
   drivers/hwmon/hp-wmi-sensors.c
   drivers/hwmon/mr75203.c
   drivers/hwmon/pmbus/pmbus_core.c

Also use the action function devm_debugfs_dir_recursive_drop() in driver
drivers/gpio/gpio-mockup.c

As per Dan Williams' request [1], do not touch the driver
drivers/cxl/mem.c

[1] https://lore.kernel.org/linux-gpio/65d7918b358a5_1ee3129432@dwillia2-mobl3.amr.corp.intel.com.notmuch/

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

---
  drivers/crypto/caam/ctrl.c            | 16 +++--------
  drivers/gpio/gpio-mockup.c            | 11 ++------
  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 ++-------
  drivers/hwmon/hp-wmi-sensors.c        | 15 ++--------
  drivers/hwmon/mr75203.c               | 15 ++++------
  drivers/hwmon/pmbus/pmbus_core.c      | 16 ++++-------
  include/linux/devm-helpers.h          | 40 +++++++++++++++++++++++++++
  7 files changed, 61 insertions(+), 65 deletions(-)

drivers/gpio/gpio-mockup.c +1 -7

index 455eecf6380e..adbe0fe09490 100644
@@ -12,6 +12,7 @@
  #include <linux/cleanup.h>
  #include <linux/debugfs.h>
  #include <linux/device.h>
+#include <linux/devm-helpers.h>
  #include <linux/gpio/driver.h>
  #include <linux/interrupt.h>
  #include <linux/irq.h>

@@ -390,13 +391,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
  	}
  }
 
-static void gpio_mockup_debugfs_cleanup(void *data)
-{
-	struct gpio_mockup_chip *chip = data;
-
-	debugfs_remove_recursive(chip->dbg_dir);
-}
-
  static void gpio_mockup_dispose_mappings(void *data)
  {
  	struct gpio_mockup_chip *chip = data;

drivers/gpio/gpio-mockup.c +0 -0

  }
 
  static const struct of_device_id gpio_mockup_of_match[] = {

drivers/hwmon/mr75203.c +1 -4

index 50a8b9c3f94d..50f348fca108 100644
@@ -10,6 +10,7 @@
  #include <linux/bits.h>
  #include <linux/clk.h>
  #include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
  #include <linux/hwmon.h>
  #include <linux/kstrtox.h>
  #include <linux/module.h>

@@ -216,17 +217,11 @@ static const struct file_operations pvt_ts_coeff_j_fops = {
  	.llseek = default_llseek,
  };
 
-static void devm_pvt_ts_dbgfs_remove(void *data)
-{
-	struct pvt_device *pvt = (struct pvt_device *)data;
-

drivers/hwmon/mr75203.c +1 -1

  	debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir,
  			   &pvt->ts_coeff.h);

@@ -237,7 +232,7 @@ static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
  	debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
  			    &pvt_ts_coeff_j_fops);
 
-	return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
+	return 0;
  }

Review

Quoted 3 lines

(quoted diff) +2 -1

-	return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip);
+	return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
+					chip->dbg_dir);  

This look strange. Shouldn't the debugfs_create_dir() call in gpio_mockup_debugfs_setup() be changed, instead?

(I've not look in the previous version if something was said about it, so apologies if already discussed)

Review

Quoted 6 lines

  static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
  {

(quoted diff) +3 -1

-	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+	pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(pvt->dbgfs_dir))
+		return PTR_ERR(pvt->dbgfs_dir);  

Not sure if the test and error handling should be added here. *If I'm correct*, functions related to debugfs already handle this case and just do nothing. And failure in debugfs related code is not considered as something that need to be reported and abort a probe function.

Maybe the same other (already existing) tests in this patch should be removed as well, in a separated patch.

On Sat, 23 Mar 2024 22:10:40 +0100
Christophe JAILLET <[email protected]> wrote:

Quoted 12 lines

Review

Quoted 6 lines

   static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
   {

(quoted diff) +3 -1

-	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+	pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(pvt->dbgfs_dir))
+		return PTR_ERR(pvt->dbgfs_dir);

Not sure if the test and error handling should be added here. *If I'm correct*, functions related to debugfs already handle this case and just do nothing. And failure in debugfs related code is not considered as something that need to be reported and abort a probe function.

Quoted 2 lines

Maybe the same other (already existing) tests in this patch should be removed as well, in a separated patch.

Functions related to debugfs maybe do, but devm_ resource management functions may fail to allocate release structure, and those errors need to be handled, AFAIK.

Marek

Le 23/03/2024 à 22:25, Marek Behún a écrit :

Quoted 3 lines

On Sat, 23 Mar 2024 22:10:40 +0100
Christophe JAILLET <[email protected]> wrote:

...

Quoted 15 lines

Quoted 14 lines

Review

Quoted 6 lines

   static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
   {

(quoted diff) +3 -1

-	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+	pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(pvt->dbgfs_dir))
+		return PTR_ERR(pvt->dbgfs_dir);  

Not sure if the test and error handling should be added here. *If I'm correct*, functions related to debugfs already handle this case and just do nothing. And failure in debugfs related code is not considered as something that need to be reported and abort a probe function.

Maybe the same other (already existing) tests in this patch should be removed as well, in a separated patch.

Quoted 3 lines

Functions related to debugfs maybe do, but devm_ resource management functions may fail to allocate release structure, and those errors need to be handled, AFAIK.

I would say no.
If this memory allocation fails, then debugfs_create_dir() will not be called, but that's not a really big deal if the driver itself can still run normally without it.

Up to you to leave it as-is or remove what I think is a useless error handling.
At least, maybe it could be said in the commit log, so that maintainers can comment on it, if they don't spot the error handling you introduce.

CJ

Quoted 3 lines

Marek

Review

Quoted 6 lines

  static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
  {

(quoted diff) +3 -1

-	pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+	pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+	if (IS_ERR(pvt->dbgfs_dir))
+		return PTR_ERR(pvt->dbgfs_dir);

Not sure if the test and error handling should be added here.