Hi Kaneko-san,
On Mon, Jan 15, 2018 at 7:20 PM, Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx> wrote:
> From: Hien Dang <hien.dang.eb@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@xxxxxxxxxxx>
> [Modify structure of the bank info to simplify a saving registers]
> [Remove DEV_PM_OPS macro]
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxx
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
