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