linux-kernel - Re: [PATCH 1/8] phy: phy-hi3670-usb3: move driver from staging into phy

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 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201130103023.GM8403@vkoul-mobl>
Date:   Mon, 30 Nov 2020 16:00:23 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>, linuxarm@...wei.com,
        mauro.chehab@...wei.com, John Stultz <john.stultz@...aro.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Alex Dewar <alex.dewar90@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Yu Chen <chenyu56@...wei.com>, devel@...verdev.osuosl.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/8] phy: phy-hi3670-usb3: move driver from staging into
 phy

On 16-11-20, 13:59, Mauro Carvalho Chehab wrote:

> +#define CTRL7_USB2_REFCLKSEL_MASK	(3 << 3)
> +#define CTRL7_USB2_REFCLKSEL_ABB	(3 << 3)
> +#define CTRL7_USB2_REFCLKSEL_PAD	(2 << 3)

This should use GENMASK()
> +
> +#define CFG50_USB3_PHY_TEST_POWERDOWN	BIT(23)
> +
> +#define CFG54_USB31PHY_CR_ADDR_MASK	(0xFFFF)
> +#define CFG54_USB31PHY_CR_ADDR_SHIFT	(16)

We can skip this by using FIELD_GET/FIELD_SET macros and only define
register fields.

> +static int hi3670_phy_cr_start(struct regmap *usb31misc, int direction)
> +{
> +	int ret;
> +
> +	if (direction)
> +		ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> +					 CFG54_USB31PHY_CR_WR_EN,
> +					 CFG54_USB31PHY_CR_WR_EN);
> +	else
> +		ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> +					 CFG54_USB31PHY_CR_RD_EN,
> +					 CFG54_USB31PHY_CR_RD_EN);

how about:
        if direction
                reg = CFG54_USB31PHY_CR_WR_EN;
        else
                reg = CFG54_USB31PHY_CR_RD_EN;

        regmap_update_bits(usb31misc, USB_MISC_CFG54, reg, reg);
 
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = hi3670_phy_cr_clk(usb31misc);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> +				 CFG54_USB31PHY_CR_RD_EN | CFG54_USB31PHY_CR_WR_EN, 0);
> +
> +	return ret;

        return regmap_update_bits()

> +static int hi3670_phy_cr_wait_ack(struct regmap *usb31misc)
> +{
> +	u32 reg;
> +	int retry = 100000;
> +	int ret;
> +
> +	while (retry-- > 0) {
> +		ret = regmap_read(usb31misc, USB_MISC_CFG54, &reg);
> +		if (ret)
> +			return ret;
> +		if ((reg & CFG54_USB31PHY_CR_ACK) == CFG54_USB31PHY_CR_ACK)
> +			return 0;
> +
> +		ret = hi3670_phy_cr_clk(usb31misc);
> +		if (ret)
> +			return ret;

No delay in between reads..? maybe add a small delay and reduce the
retries?

> +static int hi3670_phy_cr_set_addr(struct regmap *usb31misc, u32 addr)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	ret = regmap_read(usb31misc, USB_MISC_CFG54, &reg);
> +	if (ret)
> +		return ret;
> +
> +	reg &= ~(CFG54_USB31PHY_CR_ADDR_MASK << CFG54_USB31PHY_CR_ADDR_SHIFT);
> +	reg |= ((addr & CFG54_USB31PHY_CR_ADDR_MASK) << CFG54_USB31PHY_CR_ADDR_SHIFT);
> +	ret = regmap_write(usb31misc, USB_MISC_CFG54, reg);

regmap_update_bits() ?

> +static int hi3670_is_abbclk_seleted(struct hi3670_priv *priv)
> +{
> +	u32 reg;
> +
> +	if (!priv->sctrl) {
> +		dev_err(priv->dev, "priv->sctrl is null!\n");
> +		return 1;
> +	}
> +
> +	if (regmap_read(priv->sctrl, SCTRL_SCDEEPSLEEPED, &reg)) {
> +		dev_err(priv->dev, "SCTRL_SCDEEPSLEEPED read failed!\n");
> +		return 1;

Not a -ve error code?
-- 
~Vinod