Re: [PATCH 8/9] regulator: bd71837: BD71837 PMIC regulator driver

[Mark Brown <broonie@kernel.org>]

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

On Thu, May 24, 2018 at 09:00:36AM +0300, Matti Vaittinen wrote:

> @@ -0,0 +1,683 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +/*
> + * bd71837-regulator.c ROHM BD71837MWV regulator driver
> + */
> +#include <linux/kernel.h>

Make the entire comment block a C++ comment so it looks more intentional
and add a blank line before the headers for legibility.

> +static int bd71837_regulator_set_regmap(struct regulator_dev *rdev, int set)
> +{
> +	int ret = -EINVAL;
> +	struct bd71837_pmic *pmic = rdev->reg_data;
> +
> +	if (pmic) {
> +		mutex_lock(&pmic->mtx);
> +		if (!set)
> +			ret = regulator_disable_regmap(rdev);
> +		else
> +			ret = regulator_enable_regmap(rdev);
> +		mutex_unlock(&pmic->mtx);
> +	}

This looks very weird - why might we not have a parent PMIC, what is the
lock doing and what is this wrapper function intended to do?  Similar
issues apply to the voltage functions, if there's any need for this it
needs to be better documented but it really doesn't look like a good
idea.

> +	err =
> +	    regmap_update_bits(pmic->mfd->regmap, BD71837_REG_REGLOCK,
> +			       (REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
> +	if (err) {
> +		dev_err(&pmic->pdev->dev, "Failed to unlock PMIC (%d)\n", err);
> +		goto err;
> +	} else
> +		dev_dbg(&pmic->pdev->dev, "%s: Unlocked lock register 0x%x\n",
> +			__func__, BD71837_REG_REGLOCK);

There's loads of coding style problems with this code, please refer to
the coding style - indentation is weird and if there's { } on one side
of an else it should be on both.

> +		rdev = regulator_register(desc, &config);
> +		if (IS_ERR(rdev)) {

devm_regulator_regster()

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