linux-kernel - Re: [PATCH 3/4] regulator: mpq7920: add mpq7920 regulator driver

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 19 Dec 2019 12:44:02 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Saravanan Sekar <sravanhome@...il.com>
Cc:     lgirdwood@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        mripard@...nel.org, heiko@...ech.de, shawnguo@...nel.org,
        laurent.pinchart@...asonboard.com, icenowy@...c.io,
        mchehab+samsung@...nel.org, davem@...emloft.net,
        gregkh@...uxfoundation.org, Jonathan.Cameron@...wei.com,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/4] regulator: mpq7920: add mpq7920 regulator driver

On Thu, Dec 19, 2019 at 11:37:20AM +0100, Saravanan Sekar wrote:

This looks pretty good, a few small issues below:

> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * mpq7920.c  -  mps mpq7920
> + *
> + * Copyright 2019 Monolithic Power Systems, Inc

Please keep the entire comment a C++ one so things look more
intentional.

> +static int mpq7920_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> +{
> +	unsigned int ramp_val = (ramp_delay <= 4000) ? 3 : 2;
> +
> +	return regmap_update_bits(rdev->regmap, MPQ7920_REG_CTL0,
> +				  MPQ7920_MASK_DVS_SLEWRATE, ramp_val << 6);
> +}

This should validate the input.  Please also avoid abusing the ternery
operator like this, just write normal logic statements to make the code
more readable.

> +	struct regulator_desc *rdesc;
> +	struct regulator_ops *ops;
> +
> +	for (i = 0; i < MPQ7920_MAX_REGULATORS; i++) {
> +		rdesc = &info->rdesc[i];
> +		ops = rdesc->ops;
> +		if (rdesc->curr_table) {
> +			ops->get_current_limit =
> +				regulator_get_current_limit_regmap;
> +			ops->set_current_limit =
> +				regulator_set_current_limit_regmap;
> +		}

It would be better to make these constant at build time rather than
patching at runtime, that lets things like static checkers do their
thing more easily.

> +	ret = mpq7920_regulator_register(info, &config);
> +	if (ret < 0)
> +		dev_err(dev, "Failed to register regulator!\n");

This function has one caller, just inline it.

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