diff mbox

[4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

Message ID 1368807872-2601-5-git-send-email-t.figa@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa May 17, 2013, 4:24 p.m. UTC
SoC-specific driver might require additional save and restore of
registers. This patch adds pair of SoC-specific callbacks per pinctrl
device to account for this.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-samsung.c | 6 ++++++
 drivers/pinctrl/pinctrl-samsung.h | 3 +++
 2 files changed, 9 insertions(+)

Comments

Doug Anderson May 17, 2013, 7:24 p.m. UTC | #1
Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> +       if (ctrl->resume)
> +               ctrl->resume(drvdata);
> +

Having this resume at the beginning of the function seems right for
restoring eint settings like you do in patch #6.

...but if you need to try to handle the old s3c64xx and s5p64x0
concept of "restored" to actually take GPIOs out of powerdown mode
then you'll also need a callback at the end.

Does it make sense to add a second callback at the end of this function?


...since it's unclear how we'll handle s3c64xx/s5p64x0 (or even if I'm
misunderstanding and they're already handled somehow), I don't see any
problems with this patch, so...


On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>
Tomasz Figa May 17, 2013, 8:51 p.m. UTC | #2
On Friday 17 of May 2013 12:24:29 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > +       if (ctrl->resume)
> > +               ctrl->resume(drvdata);
> > +
> 
> Having this resume at the beginning of the function seems right for
> restoring eint settings like you do in patch #6.
> 
> ...but if you need to try to handle the old s3c64xx and s5p64x0
> concept of "restored" to actually take GPIOs out of powerdown mode
> then you'll also need a callback at the end.
> 
> Does it make sense to add a second callback at the end of this function?

Right. I haven't thought of this. It might make sense to add .resumed() 
callback as well to handle this.

I guess it could be added as a part of patches for S3C64xx-specific 
pinctrl suspend/resume, that I will post some day.

> ...since it's unclear how we'll handle s3c64xx/s5p64x0 (or even if I'm
> misunderstanding and they're already handled somehow), I don't see any
> problems with this patch, so...
> 
> 
> On exynos5250-snow (pinmux backported to 3.8):
> 
> Tested-by: Doug Anderson <dianders@chromium.org>
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Thanks.

Best regards,
Tomasz
Linus Walleij May 24, 2013, 9:07 a.m. UTC | #3
On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> SoC-specific driver might require additional save and restore of
> registers. This patch adds pair of SoC-specific callbacks per pinctrl
> device to account for this.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Patch applied for fixes.

Hm this is quite a lot of code for "fixes", can you confirm that
the system is really unusable without all these patches?

Yours,
Linus Walleij
Tomasz Figa May 24, 2013, 9:20 a.m. UTC | #4
Hi Linus,

On Friday 24 of May 2013 11:07:41 Linus Walleij wrote:
> On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > SoC-specific driver might require additional save and restore of
> > registers. This patch adds pair of SoC-specific callbacks per pinctrl
> > device to account for this.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Patch applied for fixes.

Thanks.

> Hm this is quite a lot of code for "fixes", can you confirm that
> the system is really unusable without all these patches?

Well, it is something that should have been already sent as a fix for 3.8, when 
this pin control driver was added, because since then suspend/resume has been 
broken on DT-enabled Exynos boards.

Without this, suspending the board and then trying to wake it up is rather 
unpredictable, leading usually to board reset, interrupt storm or at least 
some devices failing to resume.

Best regards,
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index 15db258..63ac22e 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -1010,6 +1010,9 @@  static void samsung_pinctrl_suspend_dev(
 				 reg, bank->pm_save[PINCFG_TYPE_FUNC]);
 		}
 	}
+
+	if (ctrl->suspend)
+		ctrl->suspend(drvdata);
 }
 
 /**
@@ -1026,6 +1029,9 @@  static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (ctrl->resume)
+		ctrl->resume(drvdata);
+
 	for (i = 0; i < ctrl->nr_banks; i++) {
 		struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
 		void __iomem *reg = virt_base + bank->pctl_offset;
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 9f5cc81..b316d9f 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -187,6 +187,9 @@  struct samsung_pin_ctrl {
 
 	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
 	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
+	void		(*suspend)(struct samsung_pinctrl_drv_data *);
+	void		(*resume)(struct samsung_pinctrl_drv_data *);
+
 	char		*label;
 };