Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

["Marek Behún" <kabel@kernel.org>]

On Mon, 17 Jun 2024 12:42:41 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote:  
> > > On Mon, 17 Jun 2024 10:38:31 +0200
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:  
> > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:  
> > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> wrote:  
> > > > > >
> > > > > > 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.  
> 
> ...
> 
> > > > > > +       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.  
> > >
> > > 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  
> >
> > 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