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.
Attachment:
signature.asc
Description: PGP signature
