lkml.org 
[lkml]   [2019]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2019-12-19 13:44    [W:0.064 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site