lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 6 Mar 2019 22:00:39 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Neil Armstrong <narmstrong@...libre.com>
Cc:     gregkh@...uxfoundation.org, hminas@...opsys.com, balbi@...nel.org,
        kishon@...com, linux-amlogic@...ts.infradead.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver

Hi Neil,

On Mon, Mar 4, 2019 at 11:40 AM Neil Armstrong <narmstrong@...libre.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

[...]
> +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

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.


Regards
Martin

Powered by blists - more mailing lists