Re: [PATCH net-next v3 1/4] phylib: Add device reset delay support

Hi Geert,

On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote:

Hi Richard,

On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner [off-list ref] wrote:

quoted

From: Richard Leitner <redacted>

Some PHYs need a minimum time after the reset gpio was asserted and/or
deasserted. To ensure we meet these timing requirements add two new
optional devicetree parameters for the phy: reset-delay-us and
reset-post-delay-us.

Thanks for your patch!

quoted

This patch depends on the "phylib: Add device reset GPIO support" patch
submitted by Geert Uytterhoeven/Sergei Shtylyov, see:
        https://patchwork.kernel.org/patch/10090149/

The above paragraph belongs under the "---" line below, as it is not intended
to be preserved in the eternal git history.

Ok. Thanks. That makes sense. I'll take it into account for v4.

quoted

Signed-off-by: Richard Leitner <redacted>

Reviewed-by: Geert Uytterhoeven <redacted>

Although I have a few suggestions below:

Thank you for your review and suggestions (they make the code look way
more neater). I'll adapt v4 accordingly.

quoted

--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c

@@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove);

 void mdio_device_reset(struct mdio_device *mdiodev, int value)
 {
-       if (mdiodev->reset)
-               gpiod_set_value(mdiodev->reset, value);
+       if (!mdiodev->reset)
+               return;
+
+       gpiod_set_value(mdiodev->reset, value);
+
+       if (value && mdiodev->reset_delay)
+               usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100);
+       else if (!value && mdiodev->reset_post_delay)
+               usleep_range(mdiodev->reset_post_delay,
+                            mdiodev->reset_post_delay + 100);

I think this can be written simpler using e.g.:

        unsigned int delay;

        ...
        delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay;
        if (delay)
                usleep_range(delay, delay + 100);

Perhaps the range extension should be relative, e.g.
"delay + min(delay / 10, 100)"?

quoted

--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c

@@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
        if (of_property_read_bool(child, "broken-turn-around"))
                mdio->phy_ignore_ta_mask |= 1 << addr;

+       if (of_property_read_u32(child, "reset-delay-us",
+                                &phy->mdio.reset_delay))
+               phy->mdio.reset_delay = 0;
+
+       if (of_property_read_u32(child, "reset-post-delay-us",
+                                &phy->mdio.reset_post_delay))
+               phy->mdio.reset_post_delay = 0;

If of_property_read_u32() fails, it doesn't write to its output parameter.
As the structure should be zeroed during allocation, you can just write:

        of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay);
        of_property_read_u32(child, "reset-post-delay-us",
                             &phy->mdio.reset_post_delay);

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html