On Tue, 2014-09-16 at 02:22 -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.
>
Few comments below.
> Reviewed-by: Hock Leong Kweh <hock.leong.kweh@xxxxxxxxx>
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?
> Signed-off-by: Weike Chen <alvin.chen@xxxxxxxxx>
> ---
> drivers/gpio/gpio-dwapb.c | 109 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 109 insertions(+)
>
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 9a76e3c..14d83af 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -50,11 +50,14 @@
> #define GPIO_SWPORT_DDR_SIZE (GPIO_SWPORTB_DDR - GPIO_SWPORTA_DDR)
>
> struct dwapb_gpio;
> +struct dwapb_context;
>
> struct dwapb_gpio_port {
> struct bgpio_chip bgc;
> bool is_registered;
> struct dwapb_gpio *gpio;
> + struct dwapb_context *ctx;
> + unsigned int idx;
> };
>
> struct dwapb_gpio {
> @@ -377,6 +380,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
>
> port = &gpio->ports[offs];
> port->gpio = gpio;
> + port->idx = pp->idx;
>
> dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_SIZE);
> set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_SIZE);
> @@ -584,10 +588,115 @@ static const struct of_device_id dwapb_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, dwapb_of_match);
>
> +#ifdef CONFIG_PM_SLEEP
> +/* Store GPIO context across system-wide suspend/resume transitions */
> +struct dwapb_context {
> + u32 data;
> + u32 dir;
> + u32 ext;
> + u32 int_en;
> + u32 int_mask;
> + u32 int_type;
> + u32 int_pol;
> + u32 int_deb;
> +};
> +
> +static int dwapb_gpio_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(&bgc->lock, flags);
> + for (i = 0; i < gpio->nr_ports; i++) {
> + unsigned int offset;
> + unsigned int idx = gpio->ports[i].idx;
> + struct dwapb_context *ctx = gpio->ports[i].ctx;
> +
> + if (!ctx) {
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + gpio->ports[i].ctx = ctx;
> + }
I don't think it's a right place to allocate this resource, especially
with devm_ helper. Can you move this to probe() stage?
Or you even can embed contex structure inside chip one with #ifdef
CONFIG_PM_SLEEP.
Maybe others could comment on this.
> +
> + offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
No need to have parentheses here. Check the code below as well.
> + ctx->dir = dwapb_read(gpio, offset);
> +
> + offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
> + ctx->data = dwapb_read(gpio, offset);
> +
> + offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
> + ctx->ext = dwapb_read(gpio, offset);
> +
> + /* Only port A can provide interrupts */
> + if (idx == 0) {
> + ctx->int_mask = dwapb_read(gpio, GPIO_INTMASK);
> + ctx->int_en = dwapb_read(gpio, GPIO_INTEN);
> + ctx->int_pol = dwapb_read(gpio, GPIO_INT_POLARITY);
> + ctx->int_type = dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
> + ctx->int_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
> +
> + /* Mask out interrupts */
> + dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> + }
> + }
> + spin_unlock_irqrestore(&bgc->lock, flags);
> +
> + return 0;
> +}
> +
> +static int dwapb_gpio_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
> + struct bgpio_chip *bgc = &gpio->ports[0].bgc;
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(&bgc->lock, flags);
> + for (i = 0; i < gpio->nr_ports; i++) {
> + unsigned int offset;
> + unsigned int idx = gpio->ports[i].idx;
> + struct dwapb_context *ctx = gpio->ports[i].ctx;
> +
> + BUG_ON(ctx == 0);
> +
> + offset = GPIO_SWPORTA_DR + (idx * GPIO_SWPORT_DR_SIZE);
> + dwapb_write(gpio, offset, ctx->data);
> +
> + offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
> + dwapb_write(gpio, offset, ctx->dir);
> +
> + offset = GPIO_EXT_PORTA + (idx * GPIO_EXT_PORT_SIZE);
> + dwapb_write(gpio, offset, ctx->ext);
> +
> + /* Only port A can provide interrupts */
> + if (idx == 0) {
> + dwapb_write(gpio, GPIO_INTTYPE_LEVEL, ctx->int_type);
> + dwapb_write(gpio, GPIO_INT_POLARITY, ctx->int_pol);
> + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ctx->int_deb);
> + dwapb_write(gpio, GPIO_INTEN, ctx->int_en);
> + dwapb_write(gpio, GPIO_INTMASK, ctx->int_mask);
> +
> + /* Clear out spurious interrupts */
> + dwapb_write(gpio, GPIO_PORTA_EOI, 0xffffffff);
> + }
> + }
> + spin_unlock_irqrestore(&bgc->lock, flags);
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend,
> + dwapb_gpio_resume);
> +
> static struct platform_driver dwapb_gpio_driver = {
> .driver = {
> .name = "gpio-dwapb",
> .owner = THIS_MODULE,
> + .pm = &dwapb_gpio_pm_ops,
> .of_match_table = of_match_ptr(dwapb_of_match),
> },
> .probe = dwapb_gpio_probe,
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
