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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux