On Mon, Jun 17, 2024 at 1:34 PM Marek Behún <kabel@xxxxxxxxxx> wrote:
>
> On Mon, 17 Jun 2024 12:42:41 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@xxxxxxxxxx> wrote:
> > > > On Mon, 17 Jun 2024 10:38:31 +0200
> > > > Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko
> > > > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@xxxxxxxxxx> 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
The reason to deprecate it was the fact that it's dangerous to use
from outside of the GPIO provider code. I actually plan to soon make
this function private to gpiolib, there's just one pinctrl driver left
to convert.
So maybe it's better to not use it here. Please keep in mind that
gpio_device_get_desc() doesn't increase the reference count of
gpio_device so you need to make sure it stays alive. But it seems to
be the case here as you're within the driver code still.
Bart
