Turris Omnia MCU driver

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

drivers/platform/cznic/Kconfig new file +6 -0

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

(quoted diff) +2 -0

+       return 0;
+}

+ return mode;

+}

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

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

+static const char *omnia_status_to_mcu_type(uint16_t status)

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

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

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

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

+ omnia_info_missing_feature(dev, "feature reading");

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

+#include <linux/i2c.h>

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

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

+};

Quoted 3 lines · Marek Behún

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

...

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

Quoted 5 lines

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

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

64 bit long --> 64-bit

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

Quoted 7 lines

Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu new file +6 -0

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

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

drivers/platform/cznic/Kconfig line 9

Quoted 4 lines

drivers/platform/cznic/Kconfig new file +3 -0

+menuconfig CZNIC_PLATFORMS
+       bool "Platform support for CZ.NIC's Turris hardware"  
 
+       depends on MACH_ARMADA_38X || COMPILE_TEST  

This...

Quoted 8 lines

drivers/platform/cznic/Kconfig new file +8 -0

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

Quoted 1 line

+ depends on MACH_ARMADA_38X || COMPILE_TEST

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

drivers/platform/cznic/Kconfig line 24

Quoted 7 lines

drivers/platform/cznic/Kconfig new file +6 -0

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

...

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

Quoted 2 lines

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

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

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

Quoted 4 lines

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

+{
+       u8 reply[OMNIA_FW_VERSION_LEN];
+       int err;

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

Quoted 10 lines

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

+
+       err = omnia_cmd_read(mcu->client,
+                            bootloader ? CMD_GET_FW_VERSION_BOOT
+                                       : CMD_GET_FW_VERSION_APP,
+                            reply, sizeof(reply));
+       if (err)
+               return err;  
 
+       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
+       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  

Hmm... I would rather use returned value

char *p;

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

return 0;

Quoted 3 lines

(quoted diff) +2 -0

+       return 0;
+}  

...

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

Quoted 7 lines

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

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

Do you need this?

Quoted 4 lines

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

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

Quoted 1 line

+ mode = 0;

return 0; ?

Quoted 1 line

+ return mode;

return a->mode; ?

Quoted 2 lines

+}

...

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

Quoted 11 lines

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

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

One line?

Quoted 6 lines

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

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

...

Quoted 1 line

+static const char *omnia_status_to_mcu_type(uint16_t status)

Why out of a sudden uint16_t instead of u16?

Quoted 12 lines

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

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

...

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

Quoted 2 lines

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

+       static const struct {
+               uint16_t mask;  

Ditto.

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

Quoted 9 lines

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

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

...

Quoted 1 line

+ omnia_info_missing_feature(dev, "feature reading");

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

...

Quoted 1 line

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

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

...

Quoted 1 line

+#include <linux/i2c.h>

No users of this, you may replace with

struct i2c_client;

Am I right?

...

Quoted 2 lines

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

Git

...

Quoted 1 line

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

LED-controller

...

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

Quoted 9 lines

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

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

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

Quoted 21 lines

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

Quoted 13 lines

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

+{
+       u8 reply[OMNIA_FW_VERSION_LEN];
+       int err;
+
+       err = omnia_cmd_read(mcu->client,
+                            bootloader ? CMD_GET_FW_VERSION_BOOT
+                                       : CMD_GET_FW_VERSION_APP,
+                            reply, sizeof(reply));
+       if (err)
+               return err;  
 
+       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
+       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  

Hmm... I would rather use returned value

char *p;

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

Quoted 1 line

return 0;

OK. I guess

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

would be too crazy, right?

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

Quoted 5 lines

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

Quoted 2 lines

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

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

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

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

Quoted 21 lines

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

Quoted 13 lines

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

+{
+       u8 reply[OMNIA_FW_VERSION_LEN];
+       int err;
+
+       err = omnia_cmd_read(mcu->client,
+                            bootloader ? CMD_GET_FW_VERSION_BOOT
+                                       : CMD_GET_FW_VERSION_APP,
+                            reply, sizeof(reply));
+       if (err)
+               return err;  
 
+       version[OMNIA_FW_VERSION_HEX_LEN - 1] = '\0';
+       bin2hex(version, reply, OMNIA_FW_VERSION_LEN);  

Hmm... I would rather use returned value

char *p;

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

Quoted 1 line

return 0;

OK. I guess

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

would be too crazy, right?

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+       return 0;
+}  

Quoted 4 lines

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

Quoted 2 lines

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

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

One line?

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

Quoted 4 lines

Quoted 1 line

+ omnia_info_missing_feature(dev, "feature reading");

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

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

Quoted 4 lines

Quoted 1 line

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

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

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

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

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

Quoted 10 lines

Quoted 9 lines

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

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

Quoted 2 lines

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

ctl_byt is 8-bit, so this enum really can't be extended.

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

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

...

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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

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

Quoted 5 lines

Quoted 2 lines

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

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

Really?

Quoted 6 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

One line?

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

Then better to have a logical split then?

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

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+ omnia_info_missing_feature(dev, "feature reading");

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

Quoted 9 lines

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

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

...the 'reading' feature

may be a good compromise.

...

Quoted 15 lines

Quoted 4 lines

Quoted 1 line

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

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

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

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

Quoted 2 lines

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

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

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

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

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

Quoted 4 lines

Quoted 2 lines

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

ctl_byt is 8-bit, so this enum really can't be extended.

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

Quoted 2 lines

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

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

BIT(31) | BIT(1)

as a new value?

From Linux perspective these are not terminating lines.

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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

Quoted 2 lines

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

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

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

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

Really?

Indeed:

  extern void a(char *x);

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

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

      b(x);

  }

says:
test.c: In function ‘c’:
test.c:9:9: warning: ‘b’ accessing 10 bytes in a region of size 5 [-Wstringop-overflow=]

      9 |         b(x);
        |         ^~~~
  test.c:9:9: note: referencing argument 1 of type ‘char[10]’
  test.c:3:13: note: in a call to function ‘b’
      3 | static void b(char x[static 10]) {
        |

Quoted 18 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

+ omnia_info_missing_feature(dev, "feature reading");

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

Quoted 9 lines

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

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

Quoted 3 lines

...the 'reading' feature

may be a good compromise.

I have rewritten it to use another dev_info:
dev_info(dev,

           "Your board's MCU firmware does not support feature reading.\n");

Quoted 5 lines · Marek Behún

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

...

Quoted 19 lines

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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

Quoted 2 lines

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

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

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

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

Really?

Indeed:

  extern void a(char *x);

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

Quoted 4 lines

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

      b(x);

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

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

Quoted 19 lines

Quoted 19 lines

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

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

Really?

Indeed:

  extern void a(char *x);

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

Quoted 6 lines

Quoted 4 lines

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

      b(x);

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

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

and then I use this function:

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

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

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

Quoted 7 lines · Marek Behún

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Marek Behún

Quoted 1 line · On Tue, 30 Apr 2024 15:53:51 +0300 Andy Shevchenko <[email protected]>…

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

...

Quoted 34 lines

Quoted 19 lines

Quoted 19 lines

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

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

Really?

Indeed:

  extern void a(char *x);

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

Quoted 6 lines

Quoted 4 lines

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

      b(x);

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

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

and then I use this function:

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

Quoted 4 lines

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

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

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

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

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

+	if (status & STS_FEATURES_SUPPORTED) {
+		__le32 reply;
+		int ret;
+
+		/* try read 32-bit features */
+		ret = omnia_cmd_read(mcu->client, CMD_GET_FEATURES, &reply,
+				     sizeof(reply));
+		if (ret) {
+			/* try read 16-bit features */
+			ret = omnia_cmd_read_u16(mcu->client, CMD_GET_FEATURES);

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

+	} else {
+		omnia_info_missing_feature(dev, "feature reading");
+		suggest_fw_upgrade = true;
+	}
+
+	mcu->type = omnia_status_to_mcu_type(status);
+	dev_info(dev, "MCU type %s%s\n", mcu->type,
+		 (mcu->features & FEAT_PERIPH_MCU) ?
+			", with peripheral resets wired" : "");
+
+	omnia_mcu_print_version_hash(mcu, true);
+
+	if (mcu->features & FEAT_BOOTLOADER)
+		dev_warn(dev,
+			 "MCU is running bootloader firmware. Was firmware upgrade interrupted?\n");
+	else
+		omnia_mcu_print_version_hash(mcu, false);
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(features); i++) {
+		if (mcu->features & features[i].mask)
+			continue;
+
+		omnia_info_missing_feature(dev, features[i].name);
+		suggest_fw_upgrade = true;
+	}
+
+	if (suggest_fw_upgrade)
+		dev_info(dev,
+			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
+
+	return 0;
+}
 
+int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
+		   unsigned int len);
+
+static inline int omnia_cmd_read_u16(const struct i2c_client *client, u8 cmd)
+{
+	__le16 reply;
+	int err;
+
+	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+	return err ?: le16_to_cpu(reply);
+}
+
+static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
+{
+	u8 reply;
+	int err;
+
+	err = omnia_cmd_read(client, cmd, &reply, sizeof(reply));
+
+	return err ?: reply;
+}
+
+#endif /* __TURRIS_OMNIA_MCU_H */

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

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

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

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

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

+	FEAT_LED_GAMMA_CORRECTION	= BIT(5),
+	FEAT_NEW_INT_API		= BIT(6),
+	FEAT_BOOTLOADER			= BIT(7),
+	FEAT_FLASHING			= BIT(8),
+	FEAT_NEW_MESSAGE_API		= BIT(9),
+	FEAT_BRIGHTNESS_INT		= BIT(10),
+	FEAT_POWEROFF_WAKEUP		= BIT(11),
+	FEAT_CAN_OLD_MESSAGE_API	= BIT(12),
+	FEAT_TRNG			= BIT(13),
+	FEAT_CRYPTO			= BIT(14),
+	FEAT_BOARD_INFO			= BIT(15),
+
+	/*
+	 * Orginally the features command replied only 16 bits. If more were

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

drivers/platform/cznic/turris-omnia-mcu-base.c +14 -11

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

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

+#define FRONT_BUTTON_RELEASE_DELAY 50 /* ms */

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

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

+};

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

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

+ }

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

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

+/**
+ * omnia_mask_interleave - Interleaves the bytes from @rising and @falling

+ *     @dst: the destination u8 array of interleaved bytes
+ *     @rising: rising mask
+ *     @falling: falling mask

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

+ * interleaved format. The rationale behind it is that the low-indexed bits are
+ * more important - in many cases, the user will be interested only in
+ * interrupts with indexes 0 to 7, and so the system can stop reading after
+ * first 2 bytes (r0, f0), to save time on the slow I2C bus.
+ *
+ * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
+ * and use an appropriate bitmap_* function once such a function exists.
+ */

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

-#include <linux/i2c.h>

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

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

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

Quoted 10 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

...

Quoted 39 lines

drivers/platform/cznic/turris-omnia-mcu-base.c +18 -12

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

Quoted 1 line

        return 0;

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

Quoted 1 line

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

Do you still need this?

...

Quoted 1 line

+#define FRONT_BUTTON_RELEASE_DELAY 50 /* ms */

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

_MS here.

Quoted 38 lines

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

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

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

Quoted 4 lines

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

+       [52] = "gpio%u.MiniPCIe1 nPERST",
+       [53] = "gpio%u.MiniPCIe2 nPERST",
+       [54] = "gpio%u.WAN PHY SFP mux",
+};

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

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

Quoted 6 lines

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

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

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

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

Quoted 7 lines

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

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

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

Quoted 4 lines

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

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

...

Quoted 3 lines

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

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

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

Quoted 3 lines

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

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

Why not assign these inside the definition line?

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

Quoted 6 lines

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

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

One line?

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

Quoted 3 lines

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

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

Ditto?

Quoted 1 line

+ }

...

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

Quoted 9 lines

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

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

Assignments in the definition line?

Quoted 11 lines

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

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

...

Quoted 5 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

Quoted 7 lines

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

an interrupt
a pending
int --> in ?

Quoted 8 lines

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

Quoted 6 lines

drivers/platform/cznic/turris-omnia-mcu-gpio.c new file +6 -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);
+       }

I will look at this later on,

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

Quoted 11 lines

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

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

and into this.

...

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

Quoted 12 lines

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

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

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

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

and most likely you can drop minmax.h.

Quoted 1 line

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

2 is not needed.

Quoted 1 line

-#include <linux/i2c.h>

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

Quoted 7 lines

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

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

...

Quoted 1 line

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

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+       if (err)
+               return err;

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

Quoted 38 lines

Quoted 38 lines

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

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

Quoted 8 lines

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

Quoted 4 lines

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

+       [52] = "gpio%u.MiniPCIe1 nPERST",
+       [53] = "gpio%u.MiniPCIe2 nPERST",
+       [54] = "gpio%u.WAN PHY SFP mux",
+};

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

Are you saying that I should dynamically create another array just to pass it to the gpiochip's names pointer?

Quoted 8 lines

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

Quoted 6 lines

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

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

One line?

That would be 81 columns, which I would like to avoid.

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

Quoted 7 lines

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

Quoted 3 lines

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

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

Ditto?

Quoted 1 line

+ }

Quoted 2 lines · Marek Behún

Quoted 1 line · Andy Shevchenko

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

...

Quoted 47 lines

Quoted 46 lines

Quoted 32 lines

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

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

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

Quoted 10 lines

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

+
+       /* GPIOs with value read from the 16-bit wide extended control status */
+       [48] = "gpio%u.eMMC nRESET",
+       [49] = "gpio%u.LAN switch nRESET",
+       [50] = "gpio%u.WAN PHY nRESET",
+       [51] = "gpio%u.MiniPCIe0 nPERST",
+       [52] = "gpio%u.MiniPCIe1 nPERST",
+       [53] = "gpio%u.MiniPCIe2 nPERST",
+       [54] = "gpio%u.WAN PHY SFP mux",
+};

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

Quoted 2 lines

Are you saying that I should dynamically create another array just to pass it to the gpiochip's names pointer?

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

...

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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

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

Quoted 3 lines

Quoted 1 line

One line?

That would be 81 columns, which I would like to avoid.

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

Quoted 2 lines

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

Make sense as well.

Quoted 7 lines

Quoted 7 lines

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

Quoted 3 lines

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

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

Ditto?

Quoted 1 line

+ }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Marek Behún

Quoted 1 line · Andy Shevchenko

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

Quoted 47 lines

Quoted 47 lines

Quoted 46 lines

Quoted 32 lines

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

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

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

Quoted 10 lines

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

+
+       /* GPIOs with value read from the 16-bit wide extended control status */
+       [48] = "gpio%u.eMMC nRESET",
+       [49] = "gpio%u.LAN switch nRESET",
+       [50] = "gpio%u.WAN PHY nRESET",
+       [51] = "gpio%u.MiniPCIe0 nPERST",
+       [52] = "gpio%u.MiniPCIe1 nPERST",
+       [53] = "gpio%u.MiniPCIe2 nPERST",
+       [54] = "gpio%u.WAN PHY SFP mux",
+};

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

Quoted 8 lines

Quoted 2 lines

Are you saying that I should dynamically create another array just to pass it to the gpiochip's names pointer?

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

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

dev = device_create_with_groups(&gpio_class, &gdev->dev,

                                  MKDEV(0, 0), data, gpio_groups,
                                  ioname ? ioname : "gpio%u",
                                  desc_to_gpio(desc));

The ioname variable contains the string.

This is then visible in sysfs:

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

Quoted 4 lines · Marek Behún

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Marek Behún

Quoted 1 line · Andy Shevchenko

On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <[email protected]> wrote:

...

Quoted 66 lines

Quoted 47 lines

Quoted 47 lines

Quoted 46 lines

Quoted 32 lines

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

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

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

Quoted 10 lines

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

+
+       /* GPIOs with value read from the 16-bit wide extended control status */
+       [48] = "gpio%u.eMMC nRESET",
+       [49] = "gpio%u.LAN switch nRESET",
+       [50] = "gpio%u.WAN PHY nRESET",
+       [51] = "gpio%u.MiniPCIe0 nPERST",
+       [52] = "gpio%u.MiniPCIe1 nPERST",
+       [53] = "gpio%u.MiniPCIe2 nPERST",
+       [54] = "gpio%u.WAN PHY SFP mux",
+};

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

Quoted 8 lines

Quoted 2 lines

Are you saying that I should dynamically create another array just to pass it to the gpiochip's names pointer?

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

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

dev = device_create_with_groups(&gpio_class, &gdev->dev,

                                  MKDEV(0, 0), data, gpio_groups,
                                  ioname ? ioname : "gpio%u",
                                  desc_to_gpio(desc));

The ioname variable contains the string.

Quoted 8 lines

This is then visible in sysfs:

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

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

Quoted 1 line

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

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

On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:

Quoted 4 lines

Quoted 1 line

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

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

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

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

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

I could probably do something like

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

Then the 3 instances would become

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

What do you think?

Quoted 1 line · Marek Behún

On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:

...

Quoted 25 lines

Quoted 4 lines

Quoted 1 line

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

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

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

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

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

I could probably do something like

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

Then the 3 instances would become

Quoted 6 lines

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

What do you think?

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