Message ID | 1516040426-15507-1-git-send-email-ykaneko0929@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Hi Kaneko-san, On Mon, Jan 15, 2018 at 7:20 PM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote: > From: Hien Dang <hien.dang.eb@renesas.com> > > This patch adds an implementation that saves and restores the state of > GPIO configuration on suspend and resume. > > Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > [Modify structure of the bank info to simplify a saving registers] > [Remove DEV_PM_OPS macro] > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > --- > > This patch is based on the for-next branch of linux-gpio tree. > > v2 [Yoshihiro Kaneko] > * Modify structure of the bank info as suggested by Geert Uytterhoeven > > v3 [Yoshihiro Kaneko] > * Remove DEV_PM_OPS macro as suggested by Vladimir Zapolskiy Thanks for the updates! It looks like not much is left from the original patches... > --- a/drivers/gpio/gpio-rcar.c > +++ b/drivers/gpio/gpio-rcar.c > @@ -31,6 +31,16 @@ > #include <linux/spinlock.h> > #include <linux/slab.h> > > +struct gpio_rcar_bank_info { > + u32 iointsel; > + u32 inoutsel; > + u32 outdt; > + u32 active_high_rising_edge; > + u32 level_trigger; > + u32 both; > + u32 intmsk; > +}; > + > struct gpio_rcar_priv { > void __iomem *base; > spinlock_t lock; > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { > unsigned int irq_parent; > bool has_both_edge_trigger; > bool needs_clk; > + struct gpio_rcar_bank_info bank_info; > }; > > #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */ > @@ -531,11 +542,65 @@ static int gpio_rcar_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int gpio_rcar_suspend(struct device *dev) > +{ > + struct gpio_rcar_priv *p = dev_get_drvdata(dev); > + > + p->bank_info.iointsel = gpio_rcar_read(p, IOINTSEL); > + p->bank_info.inoutsel = gpio_rcar_read(p, INOUTSEL); > + p->bank_info.outdt = gpio_rcar_read(p, OUTDT); > + p->bank_info.intmsk = gpio_rcar_read(p, INTMSK); Given you just store values read from the hardware registers, why not name all fields below according to the registers' names, like you did for the above? Especially for active_high_rising_edge and level_trigger, as the semantics are inverted, compared to the POSNEG resp. EDGLEVEL registers. > + p->bank_info.active_high_rising_edge = gpio_rcar_read(p, POSNEG); > + p->bank_info.level_trigger = gpio_rcar_read(p, EDGLEVEL); > + p->bank_info.both = gpio_rcar_read(p, BOTHEDGE); R-Car Gen1 does not have the BOTHEDGE. Hence it should be read only if p->has_both_edge_trigger is true; However, do we need to save/restore the configuration on all supported SoCs? Shouldn't it depend on PSCI, like is done in drivers/pinctrl/sh-pfc/core.c and drivers/clk/renesas/renesas-cpg-mssr.c? > + > + return 0; > +} > + > +static int gpio_rcar_resume(struct device *dev) > +{ > + struct gpio_rcar_priv *p = dev_get_drvdata(dev); > + int offset; unsigned int > + u32 mask; > + > + for (offset = 0; offset < p->gpio_chip.ngpio; offset++) { > + mask = BIT(offset); > + /* I/O pin */ > + if (!(p->bank_info.iointsel & mask)) { > + if (p->bank_info.inoutsel & mask) > + gpio_rcar_direction_output( > + &p->gpio_chip, offset, > + !!(p->bank_info.outdt & mask)); > + else > + gpio_rcar_direction_input(&p->gpio_chip, > + offset); > + /* Interrupt pin */ The comment above is not properly indented, and should be moved below the "else" statement. > + } else { > + gpio_rcar_config_interrupt_input_mode( > + p, > + offset, > + !(p->bank_info.active_high_rising_edge & mask), > + !!(p->bank_info.level_trigger & mask), This logic seems to be inverted, shouldn't it be !(p->bank_info.level_trigger & mask), ? > + !!(p->bank_info.both & mask)); > + > + if (p->bank_info.intmsk & mask) > + gpio_rcar_write(p, MSKCLR, mask); > + } > + } > + > + return 0; > +} > +#endif /* CONFIG_PM_SLEEP*/ > + > +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, gpio_rcar_resume); > + > static struct platform_driver gpio_rcar_device_driver = { > .probe = gpio_rcar_probe, > .remove = gpio_rcar_remove, > .driver = { > .name = "gpio_rcar", > + .pm = &gpio_rcar_pm_ops, > .of_match_table = of_match_ptr(gpio_rcar_of_table), > } > }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index e76de57..88fbb3e 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -31,6 +31,16 @@ #include <linux/spinlock.h> #include <linux/slab.h> +struct gpio_rcar_bank_info { + u32 iointsel; + u32 inoutsel; + u32 outdt; + u32 active_high_rising_edge; + u32 level_trigger; + u32 both; + u32 intmsk; +}; + struct gpio_rcar_priv { void __iomem *base; spinlock_t lock; @@ -41,6 +51,7 @@ struct gpio_rcar_priv { unsigned int irq_parent; bool has_both_edge_trigger; bool needs_clk; + struct gpio_rcar_bank_info bank_info; }; #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */ @@ -531,11 +542,65 @@ static int gpio_rcar_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_SLEEP +static int gpio_rcar_suspend(struct device *dev) +{ + struct gpio_rcar_priv *p = dev_get_drvdata(dev); + + p->bank_info.iointsel = gpio_rcar_read(p, IOINTSEL); + p->bank_info.inoutsel = gpio_rcar_read(p, INOUTSEL); + p->bank_info.outdt = gpio_rcar_read(p, OUTDT); + p->bank_info.intmsk = gpio_rcar_read(p, INTMSK); + p->bank_info.active_high_rising_edge = gpio_rcar_read(p, POSNEG); + p->bank_info.level_trigger = gpio_rcar_read(p, EDGLEVEL); + p->bank_info.both = gpio_rcar_read(p, BOTHEDGE); + + return 0; +} + +static int gpio_rcar_resume(struct device *dev) +{ + struct gpio_rcar_priv *p = dev_get_drvdata(dev); + int offset; + u32 mask; + + for (offset = 0; offset < p->gpio_chip.ngpio; offset++) { + mask = BIT(offset); + /* I/O pin */ + if (!(p->bank_info.iointsel & mask)) { + if (p->bank_info.inoutsel & mask) + gpio_rcar_direction_output( + &p->gpio_chip, offset, + !!(p->bank_info.outdt & mask)); + else + gpio_rcar_direction_input(&p->gpio_chip, + offset); + /* Interrupt pin */ + } else { + gpio_rcar_config_interrupt_input_mode( + p, + offset, + !(p->bank_info.active_high_rising_edge & mask), + !!(p->bank_info.level_trigger & mask), + !!(p->bank_info.both & mask)); + + if (p->bank_info.intmsk & mask) + gpio_rcar_write(p, MSKCLR, mask); + } + } + + return 0; +} +#endif /* CONFIG_PM_SLEEP*/ + +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, gpio_rcar_resume); + static struct platform_driver gpio_rcar_device_driver = { .probe = gpio_rcar_probe, .remove = gpio_rcar_remove, .driver = { .name = "gpio_rcar", + .pm = &gpio_rcar_pm_ops, .of_match_table = of_match_ptr(gpio_rcar_of_table), } };