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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel