>
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct dwapb_context {
> > + u32 data[DWAPB_MAX_PORTS];
> > + u32 dir[DWAPB_MAX_PORTS];
> > + u32 ext[DWAPB_MAX_PORTS];
> > + u32 int_en;
> > + u32 int_mask;
> > + u32 int_
> > + if (pp->idx == 0 &&
> > + of_property_read_bool(port_np, "interrupt-controller")) {
> > + pp->irq = irq_of_parse_and_map(port_np, 0);
> > + if (!pp->irq) {
> > + dev_warn(dev, "no irq for bank %s\n",
> > +
> > + struct bgpio_chip *bgc = to_bgpio_chip(gc);
> > + struct dwapb_gpio_port *port =
> > + container_of(bgc, struct dwapb_gpio_port, bgc);
>
> Does it make sense to introduce an inline helper like
>
> static inline struct dwapb_gpio_port *to_dwapb_gpio_port(struct bgpio_ch
>
> > Reviewed-by: Hock Leong Kweh
>
> You still keep that guy as reviewer in a whole series, however I didn't see a
> word from him here. How is it possible?
In our internal review, he gave me a lot of suggestions.
> > + for (i = 0; i < gpio->nr_ports; i++) {
> > + unsigned int of
>
> > >
> > > > > static int dwapb_gpio_probe(struct platform_device *pdev) {
> > > > > + int i;
> > > > > struct resource *res;
> > > > > struct dwapb_gpio *gpio;
> > > > > - struct device_node *np;
> > > > > int err;
> > > > > - unsigned int offs = 0;
> > > > > +
> >
> > > > static int dwapb_gpio_probe(struct platform_device *pdev) {
> > > > + int i;
> > > > struct resource *res;
> > > > struct dwapb_gpio *gpio;
> > > > - struct device_node *np;
> > > > int err;
> > > > - unsigned int offs = 0;
> > > > + str
> On Tue, 9 Sep 2014, Weike Chen wrote:
>
> >
> > struct dwapb_gpio;
> > +struct dwapb_context;
> >
> > struct dwapb_gpio_port {
> > struct bgpio_chip bgc;
> > boolis_registered;
> > struct dwapb_gpio *gpio;
> > + struct dwapb_context*ctx;
>
> A
> >
> > Hi Alvin,
> >
> > I did a quick test and this looks like it works for me (with device tree).
> > I had a couple of small fixes below.
> It is very appreciated to help testing.
>
>
> > Alan
> >
> > >
> > > - port->bgc.gc.ngpio = ngpio;
> > > - port->bgc.gc.of_node = port_np;
> > > +#ifdef
>
> Hi Alvin,
>
> I did a quick test and this looks like it works for me (with device tree).
> I had a couple of small fixes below.
It is very appreciated to help testing.
> Alan
>
> >
> > - port->bgc.gc.ngpio = ngpio;
> > - port->bgc.gc.of_node = port_np;
> > +#ifdef CONFIG_OF_GPIO
> > +
> > > You cover this specific dependencies with inline ifdefs, but you
> > > lose the CONFIG_OF depends by dropping it, and there are no such
> > > checks in the probe routine. Assumptions of OF are not limited to probe in
> this driver.
> > >
> > > While I would like to see this assumption properl
> >@@ -136,7 +136,6 @@ config GPIO_DWAPB
> > tristate "Synopsys DesignWare APB GPIO driver"
> > select GPIO_GENERIC
> > select GENERIC_IRQ_CHIP
> >-depends on OF_GPIO
>
> You cover this specific dependencies with inline ifdefs, but you lose the
> CONFIG_OF depends by dropping it, a
> On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
> > > irq = irq_of_parse_and_map(node, 0); If (!irq) {
> > > pp->irq = -1;
> > > return;
> > > } else {
> > > pp->irq = irq;
> > > }
> > > Then the code looks strange.
> > >
> > > How do you think?
> >
> > If I understood correctly yo
> >
> > +#ifdef CONFIG_OF_GPIO
> > +
> > +static struct dwapb_platform_data *
> > +dwapb_gpio_get_pdata_of(struct device *dev) {
> > + struct device_node *node, *port_np;
> > + struct dwapb_platform_data *pdata;
> > + struct dwapb_port_property *pp;
> > + int nports;
> > + int i;
> > +
>
> > Since the 'debounce' feature also needs read/write, if we splite this
> > patch, then for 'debounce', One patch use readl/writel, and another
> > patch change to dwapb_read/write. It seems duplicate since the previous
> patch use readl/writel and the fllowing one change it immediately.
> > Sinc
> > >
> > > - irq_set_chained_handler(irq, dwapb_irq_handler);
> > > - irq_set_handler_data(irq, gpio);
> > > + if (!pp->irq_shared) {
> > > + irq_set_chained_handler(pp->irq, dwapb_irq_handler);
> > > + irq_set_handler_data(pp->irq, gpio);
> > > + } else {
> > > + /*
> > >
> > -static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> > +static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)
>
> What about dwapb_do_irq() ?
OK, I will improve it.
> > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
> > + u32 worked;
> > + struct dwapb_gpi
> Subject: Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
>
> On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> > This patch enables 'debounce' for the designware GPIO, and it is based
> > on Josef Ahmad's previous work.
>
> Can we split dwapb_read/write introducing and changing from
>
> On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote:
> > This patch enables suspend and resume mode for the power management,
> > and it is based on Josef Ahmad's previous work.
> >
> > Reviewed-by: Hock Leong Kweh
> > Reviewed-by: Shevchenko, Andriy
>
> I have to recall my reviwed-by tag s
> >>
> >> Insert this into the dynamically allocated per-port or chip struct instead.
> >>
> > How about the following?
> >
> > static struct dwapb_context {
> > u32 data[DWAPB_MAX_PORTS];
> > u32 dir[DWAPB_MAX_PORTS];
> > u32 ext[DWAPB_MAX_PORTS];
> > u32 int_en;
>
>
> > +#if defined CONFIG_PM_SLEEP
>
> I wonder whether it's worth #ifdef:in out such things, it clutters the place.
OK. I will use '#ifdef'.
>
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
>
> Call the struct:
>
> struct dwapb
> > Since we enable this module not only support OF devices, but also support
> MFD devices, so we remove OF_GPIO dependenc
> > For 'PCI', the original code is also not depended on PCI, and this patch
> > also
> not, do you think it is necessary?
>
> if not PCI then you should add something likw
>
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -136,7 +136,6 @@ config GPIO_DWAPB
> > tristate "Synopsys DesignWare APB GPIO driver"
> > select GPIO_GENERIC
> > select GENERIC_IRQ_CHIP
> > - depends on OF_GPIO
> you need either OF_GPIO or PCI
Since we enable t
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
> > + unsigned long data;
> > + unsigned long dir;
> > + unsigned long int_en;
> > + unsigned long int_mask;
> > + unsigned long int_type;
> > + unsigned long int_pol;
> > +
> >
> > I don't understand the reason for adding dwapb_read and dwapb_write here.
> > The rest of the driver is using readl and writel. I'd rather not see
> > two different methods being used in the same driver for register access.
> > Maybe I'm missing something, but if we need to add dwapb_read/
>
> Hi Weike,
>
> I tried out these patches on the current master branch (v3.17-rc2-9-g68e3702)
> with a socfpga cyclone5 board.
>
> If I apply all 3 patches, the kernel doesn't boot.
>
> If I rebuild with only the first patch, I get only one gpio block showing up
> (should
> have 3 for this b
25 matches
Mail list logo
