linux-kernel - Re: [PATCH 8/9] regulator: bd71837: BD71837 PMIC 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180524141427.GU4828@sirena.org.uk>
Date:   Thu, 24 May 2018 15:14:27 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
        mark.rutland@....com, lee.jones@...aro.org, lgirdwood@...il.com,
        mazziesaccount@...il.com, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        mikko.mutanen@...rohmeurope.com, heikki.haikola@...rohmeurope.com
Subject: Re: [PATCH 8/9] regulator: bd71837: BD71837 PMIC regulator driver

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()

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