[PATCH v2 08/13] dm: irq: Add support for requesting interrupts

[Bin Meng]

Hi Simon,

On Tue, Feb 4, 2020 at 8:19 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 3 Feb 2020 at 10:05, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Dec 22, 2019 at 2:16 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > At present driver model supports the IRQ uclass but there is no way to
> > > request a particular interrupt for a driver.
> > >
> > > Add a mechanism, similar to clock and reset, to read the interrupts
> > > required by a device from the device tree and to request those interrupts.
> > >
> > > U-Boot itself does not have interrupt-driven handlers, so just provide a
> > > means to read and clear an interrupt. This can be useful to handle
> > > peripherals which must use an interrupt to determine when data is
> > > available, for example.
> > >
> > > Bring over the basic binding file as well, from Linux v5.4. Note that the
> > > older binding is not supported in U-Boot; the newer 'special form' must be
> > > used.
> > >
> > > Add a simple test of the new functionality.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > >
> > > Changes in v2: None
> > >
> > >  arch/sandbox/dts/test.dts                     |   5 +-
> > >  .../interrupt-controller/interrupts.txt       | 131 ++++++++++++++++++
> > >  drivers/misc/irq-uclass.c                     | 116 ++++++++++++++++
> > >  drivers/misc/irq_sandbox.c                    |  41 ++++++
> > >  include/irq.h                                 | 115 +++++++++++++++
> > >  test/dm/irq.c                                 |  31 +++++
> > >  6 files changed, 438 insertions(+), 1 deletion(-)
> > >  create mode 100644 doc/device-tree-bindings/interrupt-controller/interrupts.txt
> > >
>
> [..]
> > > diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> > > index 38498a66a4..7d65192858 100644
> > > --- a/drivers/misc/irq-uclass.c
> > > +++ b/drivers/misc/irq-uclass.c
> > > @@ -4,8 +4,11 @@
> > >   * Written by Simon Glass <sjg at chromium.org>
> > >   */
> > >
> > > +#define LOG_CATEGORY UCLASS_IRQ
> > > +
> > >  #include <common.h>
> > >  #include <dm.h>
> > > +#include <dt-structs.h>
> > >  #include <irq.h>
> > >  #include <dm/device-internal.h>
> > >
> > > @@ -49,6 +52,119 @@ int irq_restore_polarities(struct udevice *dev)
> > >         return ops->restore_polarities(dev);
> > >  }
> > >
> > > +int irq_read_and_clear(struct irq *irq)
> > > +{
> > > +       const struct irq_ops *ops = irq_get_ops(irq->dev);
> > > +
> > > +       if (!ops->read_and_clear)
> > > +               return -ENOSYS;
> > > +
> > > +       return ops->read_and_clear(irq);
> > > +}
> > > +
> > > +#if CONFIG_IS_ENABLED(OF_PLATDATA)
> > > +int irq_get_by_index_platdata(struct udevice *dev, int index,
> > > +                             struct phandle_1_arg *cells, struct irq *irq)
> > > +{
> > > +       int ret;
> > > +
> > > +       if (index != 0)
> > > +               return -ENOSYS;
> > > +       ret = uclass_get_device(UCLASS_IRQ, 0, &irq->dev);
> > > +       if (ret)
> > > +               return ret;
> > > +       irq->id = cells[0].arg[0];
> > > +
> > > +       return 0;
> > > +}
> > > +#else
> > > +static int irq_of_xlate_default(struct irq *irq,
> > > +                               struct ofnode_phandle_args *args)
> > > +{
> > > +       log_debug("(irq=%p)\n", irq);
> > > +
> > > +       if (args->args_count > 1) {
> > > +               log_debug("Invaild args_count: %d\n", args->args_count);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       if (args->args_count)
> > > +               irq->id = args->args[0];
> > > +       else
> > > +               irq->id = 0;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int irq_get_by_index_tail(int ret, ofnode node,
> >
> > It's odd to pass the return value of some other functions to this
> > function and check it here. Can we please remove ret, and check its
> > value at where we get that?
>
> This pattern is similar to how it is done in DM core. It routes all
> return paths through a single function where you can do error
> checking, etc. So I think this is better than the alternative. The
> only strange thing here is that this function is only used one. I did
> this since I suspected there would be others coming. But I could
> inline if it you like..

If that's the way how other DM codes do, I am okay with that.

>
> [..]
>
> > > +static int sandbox_irq_of_xlate(struct irq *irq,
> > > +                               struct ofnode_phandle_args *args)
> > > +{
> > > +       irq->id = args->args[0];
> > > +
> > > +       return 0;
> > > +}
> >
> > This
>
> ...?

I must have been working too long yesterday ...

Regards,
Bin