Re: how to request gpiochip line which is only valid as an interrupt?

[Marek Behún <kabel@xxxxxxxxxx>]

On Tue, 11 Jun 2024 21:22:38 +0200
Bartosz Golaszewski <brgl@xxxxxxxx> wrote:

> On Tue, Jun 11, 2024 at 11:03 AM Marek Behún <kabel@xxxxxxxxxx> wrote:
> >
> > On Mon, 10 Jun 2024 16:01:55 +0200
> > Marek Behún <kabel@xxxxxxxxxx> wrote:
> >  
> > > Hello Bartosz,
> > >
> > > I would like to ask you if you could find some time to look at
> > >
> > >   [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for
> > >                   MCU provided TRNG
> > >
> > >   https://lore.kernel.org/soc/20240605161851.13911-7-kabel@xxxxxxxxxx/
> > >
> > > Andy Shevchenko added you to that conversation asking you about how to
> > > correctly do the following part:
> > >
> > >   irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx));
> > >
> > > I am writing this to give some more light into the problem. What is
> > > going on:
> > > - the turris-omnia-mcu driver provides a gpio chip with interrupts
> > > - some lines are gpio + irq, but some lines are interrupt only
> > > - later, after the gpiochip is registered, another part of the
> > >   turris-omnia-mcu driver wants to use one interrupt only line
> > >
> > > To use the gpiod_to_irq() function, I need gpio descriptor for that
> > > line. I can get that with gpiochip_get_desc(), since this is within the
> > > driver, I have access to the gpiochip. But this is semantically a
> > > little weird, because
> > >
> > >   1. gpiochip_get_desc() is supposed to be used by gpio driver, not
> > >      consumer (and the trng part of the turris-omnia-mcu code is a
> > >      consumer of the gpio)
> > >
> > >   2. reference counting?
> > >
> > > Looking at gpiolib, maybe the better function to use would be
> > > gpiochip_request_own_desc(). This also is defined in
> > > include/gpio/driver.c instead of include/gpio/consumer.c, but at least
> > > it's name suggests that it is used by code that also owns the
> > > gpiochip...
> > >
> > > One problem is that gpiochip_request_own_desc() won't work, because the
> > > gpiochip initializes valid masks for both gpios and irqs, and the
> > > gpiochip_request_own_desc() function calls gpiod_request_commit(),
> > > which executes the following code
> > >
> > >   if (guard.gc->request) {
> > >     offset = gpio_chip_hwgpio(desc);
> > >     if (gpiochip_line_is_valid(guard.gc, offset))
> > >       ret = guard.gc->request(guard.gc, offset);
> > >     else
> > >       ret = -EINVAL;
> > >     ...
> > >   }
> > >
> > > So if a gpiochip line is not valid GPIO, only valid IRQchip line, then
> > > the GPIO cannot be requested, even for interrupts.
> > >
> > > What is the proper solution here?
> > >
> > > Thank you
> > >
> > > Marek  
> >
> > Bart, Andy,
> >
> > it seems that if I write the mcu DT node interrupt property which
> > refers to self, i.e.:
> >
> >   mcu: system-controller@2a {
> >     ...
> >
> >     interrupts-extended = <&gpio1 11 IRQ_TYPE_NONE>,
> >                           <&mcu 13 IRQ_TYPE_NONE>;
> >     interrupt-names = "irq", "trng";
> >
> >     ...
> >   };
> >
> > it seems to work and I can use
> >
> >   irq = fwnode_irq_get_byname(dev_fwnode(dev), "trng");
> >
> > even if this is called from the mcu probe method.
> >
> > Do you think this is a proper solution?
> >
> > I find it a little bit weird that the mcu DT node refers to itself in
> > it's interrupt properties.
> >
> > Marek  
> 
> Do I understand correctly that this is an I2C device visible under a
> single address (and represented by a single device-tree node) that
> registers with several kernel subsystems (among others: GPIO and RNG)?

Indeed. Signle device-tree node, single I2C device at one address,
signle driver, several kernel subsystems. The gpiochip is registered as
first one, it provides interrupts, the subsequent things can use the
interrupts.

> If so then the interrupts should not be visible as a device property.

And that is how I have been doing this. But the question is how should
I request for the gpio descriptor? If the own interrupts are not
described device-tree property, I can't use fwnode_irq_get() /
of_irq_get().

Originally, I used the low-level irq_create_mapping(), passing it the
gpiochip's IRQ domain, something like:

  irq =  irq_create_mapping(mcu->gc.irq.domain, TRNG_HWIRQ);

Andy said [1] that

  This looks like some workaround against existing gpiod_to_irq(). Why
  do you need this?

I should not poke into gpiolib's internals like that.

So I changed it to

  irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, TRNG_HWIRQ));

But now Andy rightly says:

  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.

The gpiochip_get_desc() function doesn't request the GPIO, it doesn't
change it's flags nor anything.

There is the gpiochip_request_own_desc() function, which does
requesting, but the problem there is that this GPIO line cannot be
requested, because it is not a valid GPIO line, only a valid IRQ line
(according to relevant valid_masks).

> If you have access to the GPIO chip, can you simply call
> gpiochip_lock_as_irq() and then request the interrupt?

I don't quite understand. The gpiochip's irqchip is immutable and uses
the GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition.

This means that the .irq_request_resources method is set to
gpiochip_irq_reqres from gpiolib.c, which already calls
gpiochip_lock_as_irq().

So gpiochip_lock_as_irq() is called once I request the irq with
request_threaded_irq().

> Users can still read the value of this pin but won't be able to set
> direction to output.

Users are not supposed to read value of this pin, because it is not a
GPIO pin. The corresponding bit is not set in gpiochip.valid_mask.
It is for example impossible to export it in /sys/class/gpio.

This line is valid only as an IRQ (the
corresponding bit is set in gpiochip.irq.valid_mask).

Marek