lkml.org 
[lkml]   [2019]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver
    From
    Date
    On 06/03/2019 22:00, Martin Blumenstingl wrote:
    > Hi Neil,
    >
    > On Mon, Mar 4, 2019 at 11:40 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
    > [...]
    >> +#include <linux/clk.h>
    >> +#include <linux/delay.h>
    >> +#include <linux/io.h>
    >> +#include <linux/module.h>
    >> +#include <linux/of_device.h>
    >> +#include <linux/regmap.h>
    >> +#include <linux/mfd/syscon.h>
    > there's a "regmap" include right above. this driver doesn't use syscon
    > so this include can be dropped

    Forgot this one...

    >
    > [...]
    >> +static int phy_meson_g12a_usb2_exit(struct phy *phy)
    >> +{
    >> + struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
    >> +
    >> + return reset_control_reset(priv->reset);
    > do you know whether we should reset_control_assert here instead of
    > reset_control_reset?
    > the probe function below already uses reset_control_deassert, so the
    > current implementation is inconsistent. in v1 you replied with "Maybe
    > it would be better, indeed." - if there's a reason why
    > reset_control_assert doesn't work here then I would like to have a
    > comment stating why

    It's not clear yet, I implemented it safe here since in my tests, when
    I left the USB2 PHYs resetted, it was kept resetted on a soft system reset
    and the ROM was not able to setup the PHY correctly.

    So maybe it's wrong for power management, it's safer to simply to keep the
    PHYs unresetted when unused.

    >
    > Apart from these two this is looking good!
    > Human readable BIT/GENMASK #defines for the register bits would be
    > nice, but I'm not sure if you have the details to add these.

    I have the registers set in the doc, but it's much longer than copying
    the registers structs from the vendor kernel, so I postponed it.

    I'll try adding these, but for now it's low priority unless the PHY maintainer
    asks for them.

    Neil

    >
    >
    > Regards
    > Martin
    >

    \
     
     \ /
      Last update: 2019-03-07 09:42    [W:5.297 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site