Re: [PATCH v4 2/2] regulator: rt6160: Add support for Richtek RT6160

[ChiYuan Huang]

HI:

Mark Brown <broonie@xxxxxxxxxx> 於 2021年5月26日 週三 下午6:50寫道：
>
> 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.
>
There's a independent 'vsel' pin. It depend on user's HW design.
And that's why there's a 'richtek,vsel_active_low' property.
Its normal application is to use vsel high active level, and it means
the opposite level can be used for the suspend voltage

And there're also two voltage registers for vsel level high and low.
> > +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()
>
I didn't notice there's the regulator_set_ramp_delay_regmap API that
can be used in kernel 5.13.
Ack in next.
> > +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.
>
Ack in next. Change the if-else to switch case. Thx.
> > +     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?
>
The IC is designed for low IQ.
So from the driver probe, I only need to keep 'enable' pin high.
Or if user specify the 'enable' gpio, it will block i2c communication,
register also be reset,
and add more delay time on enable/disable.
That's why 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.
Sure, change it to dev_err(&i2c->dev, "Failed to init regmap (%d)\n",
PTR_ERR(regmap));
Ack in next, thx.