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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]