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

[Vinod Koul]

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