Instead of a busy waiting while loop using udelay
use readl_poll_timeout function to check the condition
is met or timeout occurs in crport_handshake function.
readl_poll_timeout is called in non atomic context so
it safe to sleep until the condition is met.
Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
Signed-off-by: Anand Moon <[email protected]>
---
Changes v4:
Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
--Fix the commit message.
--Fix the error timeout condition for -ETIMEDOUT
---
Changes v3:
--Fix the commit message.
--Drop the variable, used the value directly.
Changes v2:
--used the default timeout values.
--Added missing Fixed tags.
---
drivers/phy/samsung/phy-exynos5-usbdrd.c | 39 ++++++++----------------
1 file changed, 12 insertions(+), 27 deletions(-)
diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
index 7f6279fb4f8f..ad81aa65cdff 100644
--- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
+++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
@@ -16,6 +16,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_device.h>
+#include <linux/iopoll.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/mutex.h>
@@ -556,41 +557,25 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
u32 val, u32 cmd)
{
- u32 usec = 100;
unsigned int result;
+ int err;
writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
- do {
- result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
- if (result & PHYREG1_CR_ACK)
- break;
-
- udelay(1);
- } while (usec-- > 0);
-
- if (!usec) {
- dev_err(phy_drd->dev,
- "CRPORT handshake timeout1 (0x%08x)\n", val);
- return -ETIME;
+ err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+ result, (result & PHYREG1_CR_ACK), 1, 100);
+ if (err == -ETIMEDOUT) {
+ dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
+ return err;
}
- usec = 100;
-
writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
- do {
- result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
- if (!(result & PHYREG1_CR_ACK))
- break;
-
- udelay(1);
- } while (usec-- > 0);
-
- if (!usec) {
- dev_err(phy_drd->dev,
- "CRPORT handshake timeout2 (0x%08x)\n", val);
- return -ETIME;
+ err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
+ result, !(result & PHYREG1_CR_ACK), 1, 100);
+ if (err == -ETIMEDOUT) {
+ dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
+ return err;
}
return 0;
--
2.27.0
On 13-07-20, 07:42, Anand Moon wrote:
> Instead of a busy waiting while loop using udelay
> use readl_poll_timeout function to check the condition
> is met or timeout occurs in crport_handshake function.
> readl_poll_timeout is called in non atomic context so
> it safe to sleep until the condition is met.
>
> Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
> Signed-off-by: Anand Moon <[email protected]>
> ---
> Changes v4:
> Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
> --Fix the commit message.
> --Fix the error timeout condition for -ETIMEDOUT
> ---
> Changes v3:
> --Fix the commit message.
> --Drop the variable, used the value directly.
> Changes v2:
> --used the default timeout values.
> --Added missing Fixed tags.
> ---
> drivers/phy/samsung/phy-exynos5-usbdrd.c | 39 ++++++++----------------
> 1 file changed, 12 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index 7f6279fb4f8f..ad81aa65cdff 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> @@ -16,6 +16,7 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> +#include <linux/iopoll.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/mutex.h>
> @@ -556,41 +557,25 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
> static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
> u32 val, u32 cmd)
> {
> - u32 usec = 100;
> unsigned int result;
> + int err;
>
> writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>
> - do {
> - result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> - if (result & PHYREG1_CR_ACK)
> - break;
> -
> - udelay(1);
> - } while (usec-- > 0);
> -
> - if (!usec) {
> - dev_err(phy_drd->dev,
> - "CRPORT handshake timeout1 (0x%08x)\n", val);
> - return -ETIME;
> + err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
> + result, (result & PHYREG1_CR_ACK), 1, 100);
pls align this line to opening brace of preceding line:
err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
result, (result & PHYREG1_CR_ACK), 1, 100);
This is recommended way of splitting lines, see
Documentation/process/coding-style.rst and run checkpatch.pl with
--strict option
thanks
--
~Vinod
Hi Vinod,
On Thu, 16 Jul 2020 at 11:20, Vinod Koul <[email protected]> wrote:
>
> On 13-07-20, 07:42, Anand Moon wrote:
> > Instead of a busy waiting while loop using udelay
> > use readl_poll_timeout function to check the condition
> > is met or timeout occurs in crport_handshake function.
> > readl_poll_timeout is called in non atomic context so
> > it safe to sleep until the condition is met.
> >
> > Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
> > Signed-off-by: Anand Moon <[email protected]>
> > ---
> > Changes v4:
> > Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
> > --Fix the commit message.
> > --Fix the error timeout condition for -ETIMEDOUT
> > ---
> > Changes v3:
> > --Fix the commit message.
> > --Drop the variable, used the value directly.
> > Changes v2:
> > --used the default timeout values.
> > --Added missing Fixed tags.
> > ---
> > drivers/phy/samsung/phy-exynos5-usbdrd.c | 39 ++++++++----------------
> > 1 file changed, 12 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > index 7f6279fb4f8f..ad81aa65cdff 100644
> > --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/of_device.h>
> > +#include <linux/iopoll.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/mutex.h>
> > @@ -556,41 +557,25 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
> > static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
> > u32 val, u32 cmd)
> > {
> > - u32 usec = 100;
> > unsigned int result;
> > + int err;
> >
> > writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >
> > - do {
> > - result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> > - if (result & PHYREG1_CR_ACK)
> > - break;
> > -
> > - udelay(1);
> > - } while (usec-- > 0);
> > -
> > - if (!usec) {
> > - dev_err(phy_drd->dev,
> > - "CRPORT handshake timeout1 (0x%08x)\n", val);
> > - return -ETIME;
> > + err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
> > + result, (result & PHYREG1_CR_ACK), 1, 100);
>
> pls align this line to opening brace of preceding line:
>
> err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
> result, (result & PHYREG1_CR_ACK), 1, 100);
>
> This is recommended way of splitting lines, see
> Documentation/process/coding-style.rst and run checkpatch.pl with
> --strict option
Ok, I will do this, just waiting for some more feedback on these changes.
>
> thanks
> --
> ~Vinod
-Anand
On Mon, Jul 13, 2020 at 07:42:43AM +0000, Anand Moon wrote:
> Instead of a busy waiting while loop using udelay
> use readl_poll_timeout function to check the condition
> is met or timeout occurs in crport_handshake function.
> readl_poll_timeout is called in non atomic context so
> it safe to sleep until the condition is met.
>
> Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
There is no bug in original code so Fixes tag is not appropriate. Remove
it please.
Best regards,
Krzysztof
> Signed-off-by: Anand Moon <[email protected]>
> ---
> Changes v4:
> Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
> --Fix the commit message.
> --Fix the error timeout condition for -ETIMEDOUT
> ---
> Changes v3:
> --Fix the commit message.
> --Drop the variable, used the value directly.
> Changes v2:
> --used the default timeout values.
> --Added missing Fixed tags.
> ---
> drivers/phy/samsung/phy-exynos5-usbdrd.c | 39 ++++++++----------------
> 1 file changed, 12 insertions(+), 27 deletions(-)
>
Hi Krzysztof,
On Mon, 20 Jul 2020 at 13:20, Krzysztof Kozlowski <[email protected]> wrote:
>
> On Mon, Jul 13, 2020 at 07:42:43AM +0000, Anand Moon wrote:
> > Instead of a busy waiting while loop using udelay
> > use readl_poll_timeout function to check the condition
> > is met or timeout occurs in crport_handshake function.
> > readl_poll_timeout is called in non atomic context so
> > it safe to sleep until the condition is met.
> >
> > Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
>
> There is no bug in original code so Fixes tag is not appropriate. Remove
> it please.
>
Thanks for your review. Ok I will do that.
> Best regards,
> Krzysztof
>
-Anand
> > Signed-off-by: Anand Moon <[email protected]>
> > ---
> > Changes v4:
> > Rebased on to of patch [0] https://patchwork.kernel.org/patch/11651673/
> > --Fix the commit message.
> > --Fix the error timeout condition for -ETIMEDOUT
> > ---
> > Changes v3:
> > --Fix the commit message.
> > --Drop the variable, used the value directly.
> > Changes v2:
> > --used the default timeout values.
> > --Added missing Fixed tags.
> > ---
> > drivers/phy/samsung/phy-exynos5-usbdrd.c | 39 ++++++++----------------
> > 1 file changed, 12 insertions(+), 27 deletions(-)
> >