Re: [PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

On Fri, Jan 11 2019 at 15:55 -0700, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-01-07 10:48:36)
> > On Thu, Dec 20 2018 at 13:19 -0700, Stephen Boyd wrote:
> > >Quoting Lina Iyer (2018-12-19 14:11:01)
> >
> > >> +static const struct pdc_gpio_pin_data sdm845_gpio_data = {
> > >> +       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
> > >> +       .map = sdm845_gpio_pdc_map,
> > >> +};
> > >> +
> > >> +const struct of_device_id pdc_gpio_match_table[] = {
> > >> +       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },
> > >
> > >Why not qcom,sdm845-pdc?
> > >
> > The compatible matches the compatible specified in the PDC driver. Not
> > sure why the 'sdm' was left out at that time.
>
> Are you going to add sdm?
Realized this is a bug. Will fix.
> > >> +       { },
> > >> +};
> > >
> > >I wonder why we wouldn't just put this all into the qcom-pdc.c file at
> > >the bottom and then have that IRQCHIP_DECLARE() macros call small
> > >functions that pass the pdc to gpio mapping table to qcom_pdc_init()
> > >that takes a third argument?
> > >
> > We could. I feel we would be adding tables like this and it just
> > clutters up the driver file. May be in the future we could move to
> > target specific data file like the gpios, but that could be excessive
> > too. Thought this might be a compromise. I am open to change.
>
> Ok. The benefit to my approach is that we don't do the string match
> twice. We do it once and sacrifice a little code space to jump to the
> real init function with the data we want. We can then put those chip
> tables inside some #ifdef to save space and allow configurations to turn
> off everything in EXPERT mode but leave everything default enabled
> otherwise.
Sure. I will use this idea.

> > >I really hope that in the future all the gpios can be wakeup capable so
> > >that we don't need to have the table at all!
> > >
> > I doubt there are plans to support that in hardware. We should plan for
> > supporting tables like this for other chipsets based on the PDC
> > architecture.
>
> Uh oh. That's sad.