On Wed, May 26, 2021 at 01:47:48PM +0800, cy_huang wrote:

This looks mostly good, a few small issues below:

> +static int rt6160_set_suspend_voltage(struct regulator_dev *rdev, int uV)
> +{
> +	struct rt6160_priv *priv = rdev_get_drvdata(rdev);
> +	struct regmap *regmap = rdev_get_regmap(rdev);
> +	unsigned int reg = RT6160_REG_VSELH;
> +	int vsel;
> +
> +	vsel = regulator_map_voltage_linear(rdev, uV, uV);
> +	if (vsel < 0)
> +		return vsel;
> +
> +	if (priv->vsel_active_low)
> +		reg = RT6160_REG_VSELL;
> +
> +	return regmap_update_bits(regmap, reg, RT6160_VSEL_MASK, vsel);
> +}

This seems to just be updating the normal voltage configuration
regulator, the suspend mode operations are there for devices that
have a hardware suspend mode that's entered as part of the very
low level system suspend process.

> +static int rt6160_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> +{
> +	struct regmap *regmap = rdev_get_regmap(rdev);
> +	unsigned int ramp_value = RT6160_RAMPRATE_1VMS;
> +
> +	switch (ramp_delay) {
> +	case 1 ... 1000:
> +		ramp_value = RT6160_RAMPRATE_1VMS;
> +		break;

This looks like it could be converted to regulator_set_ramp_delay_regmap()

> +static unsigned int rt6160_of_map_mode(unsigned int mode)
> +{
> +	if (mode == RT6160_MODE_FPWM)
> +		return REGULATOR_MODE_FAST;
> +	else if (mode == RT6160_MODE_AUTO)
> +		return REGULATOR_MODE_NORMAL;
> +

This would be more idiomatically written as a switch statement.

> +	enable_gpio = devm_gpiod_get_optional(&i2c->dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(enable_gpio)) {
> +		dev_err(&i2c->dev, "Failed to get 'enable' gpio\n");
> +		return PTR_ERR(enable_gpio);
> +	}

There's no other references to enable_gpio?

> +	regmap = devm_regmap_init_i2c(i2c, &rt6160_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&i2c->dev, "Failed to init regmap\n");
> +		return PTR_ERR(regmap);
> +	}

It's better to print the error code to help anyone who runs into
issues figure out what's wrong.