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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YK4oGB5cZ/DhG5vm@sirena.org.uk>
Date:   Wed, 26 May 2021 11:51:04 +0100
From:   Mark Brown <broonie@...nel.org>
To:     cy_huang <u0084500@...il.com>
Cc:     lgirdwood@...il.com, robh+dt@...nel.org,
        linux-kernel@...r.kernel.org, cy_huang@...htek.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/2] regulator: rt6160: Add support for Richtek RT6160

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)