| Message ID | 20240102-j7200-pcie-s2r-v3-2-5c2e4a3fac1f@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: > No need to check the pointer returned by platform_get_drvdata(), as > platform_set_drvdata() is called during the probe. This patch should go _after_ the next one, otherwise the commit message doesn't tell full story and the code change bring a potential regression.
On 2/15/24 16:27, Andy Shevchenko wrote: > On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: >> No need to check the pointer returned by platform_get_drvdata(), as >> platform_set_drvdata() is called during the probe. > > This patch should go _after_ the next one, otherwise the commit message doesn't > tell full story and the code change bring a potential regression. > Hello Andy, I'm ok to move this patch after the next one. But for my understanding, could you explain me why changing the order is important in this case ? Regards,
On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote: > On 2/15/24 16:27, Andy Shevchenko wrote: > > On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: > >> No need to check the pointer returned by platform_get_drvdata(), as > >> platform_set_drvdata() is called during the probe. > > > > This patch should go _after_ the next one, otherwise the commit message doesn't > > tell full story and the code change bring a potential regression. > > Hello Andy, > > I'm ok to move this patch after the next one. > But for my understanding, could you explain me why changing the order is > important in this case ? Old PM calls obviously can be called in different circumstances and these checks are important. Just squash these two patches to avoid additional churn and we are done.
On 2/16/24 16:08, Andy Shevchenko wrote: > On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote: >> On 2/15/24 16:27, Andy Shevchenko wrote: >>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: >>>> No need to check the pointer returned by platform_get_drvdata(), as >>>> platform_set_drvdata() is called during the probe. >>> >>> This patch should go _after_ the next one, otherwise the commit message doesn't >>> tell full story and the code change bring a potential regression. >> >> Hello Andy, >> >> I'm ok to move this patch after the next one. >> But for my understanding, could you explain me why changing the order is >> important in this case ? > > Old PM calls obviously can be called in different circumstances and these > checks are important. > > Just squash these two patches to avoid additional churn and we are done. You mean invert the order instead of squash.
On Wed, Feb 21, 2024 at 12:01:43PM +0100, Thomas Richard wrote: > On 2/16/24 16:08, Andy Shevchenko wrote: > > On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote: > >> On 2/15/24 16:27, Andy Shevchenko wrote: > >>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: > >>>> No need to check the pointer returned by platform_get_drvdata(), as > >>>> platform_set_drvdata() is called during the probe. > >>> > >>> This patch should go _after_ the next one, otherwise the commit message doesn't > >>> tell full story and the code change bring a potential regression. > >> > >> Hello Andy, > >> > >> I'm ok to move this patch after the next one. > >> But for my understanding, could you explain me why changing the order is > >> important in this case ? > > > > Old PM calls obviously can be called in different circumstances and these > > checks are important. > > > > Just squash these two patches to avoid additional churn and we are done. > > You mean invert the order instead of squash. Either would work, but see how much churn in terms of changing just changed lines it adds.
On 2/21/24 14:13, Andy Shevchenko wrote: > On Wed, Feb 21, 2024 at 12:01:43PM +0100, Thomas Richard wrote: >> On 2/16/24 16:08, Andy Shevchenko wrote: >>> On Fri, Feb 16, 2024 at 08:59:47AM +0100, Thomas Richard wrote: >>>> On 2/15/24 16:27, Andy Shevchenko wrote: >>>>> On Thu, Feb 15, 2024 at 04:17:47PM +0100, Thomas Richard wrote: >>>>>> No need to check the pointer returned by platform_get_drvdata(), as >>>>>> platform_set_drvdata() is called during the probe. >>>>> >>>>> This patch should go _after_ the next one, otherwise the commit message doesn't >>>>> tell full story and the code change bring a potential regression. >>>> >>>> Hello Andy, >>>> >>>> I'm ok to move this patch after the next one. >>>> But for my understanding, could you explain me why changing the order is >>>> important in this case ? >>> >>> Old PM calls obviously can be called in different circumstances and these >>> checks are important. >>> >>> Just squash these two patches to avoid additional churn and we are done. >> >> You mean invert the order instead of squash. > > Either would work, but see how much churn in terms of changing just changed > lines it adds. OK thanks. I'll squash the two patches. And I'll add a comment which explains that I dropped some dead code. Regards,
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 19cc0db771a5..02eabd28d46e 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1693,11 +1693,7 @@ static void pcs_restore_context(struct pcs_device *pcs) static int pinctrl_single_suspend(struct platform_device *pdev, pm_message_t state) { - struct pcs_device *pcs; - - pcs = platform_get_drvdata(pdev); - if (!pcs) - return -EINVAL; + struct pcs_device *pcs = platform_get_drvdata(pdev); if (pcs->flags & PCS_CONTEXT_LOSS_OFF) { int ret; @@ -1712,11 +1708,7 @@ static int pinctrl_single_suspend(struct platform_device *pdev, static int pinctrl_single_resume(struct platform_device *pdev) { - struct pcs_device *pcs; - - pcs = platform_get_drvdata(pdev); - if (!pcs) - return -EINVAL; + struct pcs_device *pcs = platform_get_drvdata(pdev); if (pcs->flags & PCS_CONTEXT_LOSS_OFF) pcs_restore_context(pcs);
No need to check the pointer returned by platform_get_drvdata(), as platform_set_drvdata() is called during the probe. Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/pinctrl/pinctrl-single.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)