Re: [PATCH v2 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver

[Neil Armstrong <narmstrong@baylibre.com>]

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
>