Turris Omnia MCU driver

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

+#include <linux/types.h>

+#include "turris-omnia-mcu.h"

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-gpio.c new file +4 -0

+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bug.h>

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

+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include <asm/unaligned.h>
+
+#include "turris-omnia-mcu.h"

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

+       }
+
+       return 0;
+}

+ int i;

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

+       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
+               unsigned long *field, *field_mask;
+               u8 bit = omnia_gpios[i].ctl_bit;
+
+               switch (omnia_gpios[i].ctl_cmd) {
+               case OMNIA_CMD_GENERAL_CONTROL:
+                       field = &ctl;
+                       field_mask = &ctl_mask;
+                       break;

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

+                       __set_bit(bit, field_mask);
+                       __assign_bit(bit, field, test_bit(i, bits));
+               }
+       }

(quoted diff) +2 -0

+               return -EINVAL;
+       }

+/**
+ * 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 an interrupt mask and reports a pending interrupt bitmap in
+ * this interleaved format. The rationale behind this 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 +4 -0

+               dst[2 * i] = rising >> (8 * i);
+               dst[2 * i + 1] = falling >> (8 * i);
+       }
+}

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

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

On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <[email protected]> wrote:

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

Why so many spaces before @? One is enough.

Quoted 11 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 an interrupt mask and reports a pending interrupt bitmap in
+ * this interleaved format. The rationale behind this 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.

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

Quoted 6 lines

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

+static void
+omnia_mask_interleave(u8 *dst, unsigned long rising, unsigned long falling)

But rising and failing should be either u64 or unsigned long *.

Review

Quoted 2 lines

(quoted diff) +2 -0

+{
+       for (int i = 0; i < sizeof(u32); ++i) {

In other cases you use:
1) unsigned
2) post-increment

What makes this one special?

Quoted 4 lines

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

+               dst[2 * i] = rising >> (8 * i);
+               dst[2 * i + 1] = falling >> (8 * i);
+       }
+}

...

Quoted 8 lines

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

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

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

Quoted 10 lines

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

+static void omnia_mask_deinterleave(const u8 *src, unsigned long *rising,
+                                   unsigned long *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);
+       }
+}

Same comments as per above function.

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

+#include <linux/i2c.h>

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

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

+ struct device *dev = kobj_to_dev(kobj);

+#include <linux/rtc.h>

#include <linux/types.h>
#include <linux/workqueue.h>
#include <asm/byteorder.h>
+#include <asm/unaligned.h>

Quoted 1 line

+ struct device *dev = kobj_to_dev(kobj);

Is this declared in device.h or elsewhere?

Add support for the watchdog mechanism provided by the MCU.

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

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

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

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Quoted 8 lines · On Wed, 5 Jun 2024 22:00:20 +0300 Andy Shevchenko <[email protected]>…

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Note that I can't use gpiochip_request_own_desc(), nor any other function that calls gpio_request_commit() (like gpiod_get()), because that checks for gpiochip_line_is_valid(), and this returns false for the TRNG line, cause that line is not a GPIO line, but interrupt only line.

That is why I used
irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
until v7, with no reference to gpio descriptors, since this line is not a GPIO line.

We have discussed this back in April, in the thread
https://lore.kernel.org/soc/[email protected]/
where we concluded that
irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
is better...

Quoted 23 lines · Marek Behún

Quoted 8 lines · On Wed, 5 Jun 2024 22:00:20 +0300 Andy Shevchenko <[email protected]>…

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");    

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Note that I can't use gpiochip_request_own_desc(), nor any other function that calls gpio_request_commit() (like gpiod_get()), because that checks for gpiochip_line_is_valid(), and this returns false for the TRNG line, cause that line is not a GPIO line, but interrupt only line.

That is why I used
irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
until v7, with no reference to gpio descriptors, since this line is not a GPIO line.

Quoted 5 lines

We have discussed this back in April, in the thread
https://lore.kernel.org/soc/[email protected]/
where we concluded that
irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx));
is better...

That's fine to not use other APIs, the problem here is with reference counting on the GPIO device. The API you could use is gpio_device_get_desc(). But you need to have a GPIO device pointer somewhere in your driver being available.

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

Quoted 2 lines

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

+#include <linux/bitfield.h>
+#include <linux/completion.h>  

+ errno.h

Quoted 4 lines · On Wed, 5 Jun 2024 22:00:20 +0300 Andy Shevchenko <[email protected]>…

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

Quoted 2 lines

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

+#include <linux/bitfield.h>
+#include <linux/completion.h>

+ errno.h

I use -EIO, -EINVAL and -ENOMEM in turris-omnia-mcu-base.c, -EINVAL, -ENOTSUPP in turris-omnia-mcu-gpio.c.

Should I include errno.h in those also?

Or is this only needed for -ERESTARTSYS?

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

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

Quoted 2 lines

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

+#include <linux/bitfield.h>
+#include <linux/completion.h>

+ errno.h

drivers/platform/cznic/turris-omnia-mcu-trng.c line 18

Quoted 10 lines

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

+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/string.h>
 
+#include <linux/turris-omnia-mcu-interface.h>

As per other patches.

Quoted 3 lines

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

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

...

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Quoted 36 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

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

Quoted 2 lines

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

+#include <linux/bitfield.h>
+#include <linux/completion.h>  

+ errno.h

drivers/platform/cznic/turris-omnia-mcu-trng.c line 18

Quoted 10 lines

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

+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/string.h>  
 
+#include <linux/turris-omnia-mcu-interface.h>  

As per other patches.

Quoted 3 lines

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

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

...

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine.

Quoted 48 lines · On Mon, 17 Jun 2024 10:38:31 +0200 Bartosz Golaszewski <[email protected]> wrote:

Quoted 36 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

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

Quoted 2 lines

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

+#include <linux/bitfield.h>
+#include <linux/completion.h>

+ errno.h

drivers/platform/cznic/turris-omnia-mcu-trng.c line 18

Quoted 10 lines

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

+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/string.h>
 
+#include <linux/turris-omnia-mcu-interface.h>

As per other patches.

Quoted 3 lines

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

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

...

Quoted 9 lines

drivers/platform/cznic/turris-omnia-mcu-trng.c line 73

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Quoted 4 lines

Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine.

But this is already being done.

The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:

  static const struct irq_chip omnia_mcu_irq_chip = {
    ...
    GPIOCHIP_IRQ_RESOURCE_HELPERS,
  };

This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are used as irq_request_resources() and irq_release_resources().

The gpiochip_reqres_irq() code increases the module refcount and even locks the line as IRQ:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732

Marek

Quoted 12 lines · Marek Behún

Quoted 10 lines · On Mon, 17 Jun 2024 10:38:31 +0200 Bartosz Golaszewski <[email protected]> wrote:

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

Quoted 26 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");

Quoted 8 lines

Quoted 3 lines

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine.

But this is already being done.

The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:

  static const struct irq_chip omnia_mcu_irq_chip = {
    ...
    GPIOCHIP_IRQ_RESOURCE_HELPERS,
  };

This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are used as irq_request_resources() and irq_release_resources().

Quoted 4 lines

The gpiochip_reqres_irq() code increases the module refcount and even locks the line as IRQ:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732

Andy: what is the issue here then exactly?

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Marek Behún

Quoted 10 lines · On Mon, 17 Jun 2024 10:38:31 +0200 Bartosz Golaszewski <[email protected]> wrote:

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

Quoted 31 lines

Quoted 30 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");  

Quoted 8 lines

Quoted 3 lines

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine.

But this is already being done.

The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:

  static const struct irq_chip omnia_mcu_irq_chip = {
    ...
    GPIOCHIP_IRQ_RESOURCE_HELPERS,
  };

This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are used as irq_request_resources() and irq_release_resources().

The gpiochip_reqres_irq() code increases the module refcount and even locks the line as IRQ:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732

Quoted 1 line

Andy: what is the issue here then exactly?

The function in use is marked as DEPRECATED. If you are fine with its usage in this case, I have no issues with it.
If you want it to be replaced with the respective gpio_device_get_desc(), it's fine, but then the question is how properly get a pointer to GPIO device object.

Quoted 48 lines · On Mon, 17 Jun 2024 12:42:41 +0200 Andy Shevchenko <[email protected]>…

Quoted 13 lines · Bartosz Golaszewski

Quoted 12 lines · Marek Behún

Quoted 10 lines · On Mon, 17 Jun 2024 10:38:31 +0200 Bartosz Golaszewski <[email protected]> wrote:

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Marek Behún

Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral.

+Cc: Bart for gpiochip_get_desc() usage.

...

Quoted 31 lines

Quoted 30 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

(quoted diff) +4 -0

+       irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_INT_TRNG)];
+       irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
+       if (irq < 0)
+               return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n");

Quoted 8 lines

Quoted 3 lines

Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term.

Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine.

But this is already being done.

The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition:

  static const struct irq_chip omnia_mcu_irq_chip = {
    ...
    GPIOCHIP_IRQ_RESOURCE_HELPERS,
  };

This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are used as irq_request_resources() and irq_release_resources().

The gpiochip_reqres_irq() code increases the module refcount and even locks the line as IRQ:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732

Quoted 8 lines

Quoted 1 line

Andy: what is the issue here then exactly?

The function in use is marked as DEPRECATED. If you are fine with its usage in this case, I have no issues with it.
If you want it to be replaced with the respective gpio_device_get_desc(), it's fine, but then the question is how properly get a pointer to GPIO device object.

Aha, I did not notice that the function is deprecated.

What about

  irq = gpiod_to_irq(gpio_device_get_desc(mcu->gc.gpiodev, irq_idx));

?

Note: I would prefer
irq_create_mapping(mcu->gc.irq.domain, irq_idx)
since the irq_idx line is not a valid GPIO line and at this part of the driver the fact that the IRQs are provided through a gpiochip are semantically irrelevant (we are interested in "an IRQ", not "an IRQ from a GPIO").

Marek

drivers/platform/cznic/turris-omnia-mcu-trng.c line 36

Quoted 4 lines · Marek Behún

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

+static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv;  

Please don't cast rng->priv in this manner. Please take a look at drivers/char/hw_random/bcm2835-rng.c for how it should be done.

Thanks,

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

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

Quoted 4 lines · Marek Behún

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

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

Thank you!
There are a few small issues here and there, but overall LGTM. The only one main question is what to do with gpiochip_get_desc(). I Cc'ed Bart for this.