Message-ID: <20201116183833.GC4739@sirena.org.uk>
Date:   Mon, 16 Nov 2020 18:38:33 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     linuxarm@...wei.com, mauro.chehab@...wei.com,
        John Stultz <john.stultz@...aro.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mayulong <mayulong1@...wei.com>,
        YueHaibing <yuehaibing@...wei.com>, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging

On Mon, Nov 16, 2020 at 01:59:30PM +0100, Mauro Carvalho Chehab wrote:

> This driver is ready for mainstream. Move it out of staging.

There's quite a few issues here, to be honest I'm disappointed some of
them weren't caught during staging review, this needs fairly substantial
work and there's signs that this also applies to at least the MFD
portion.

This also appears to be missing a DT binding document, binding
documentation is required for anything with DT support.

> +config REGULATOR_HI6421V600
> +	tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
> +	depends on MFD_HI6421_SPMI && OF
> +	depends on REGULATOR

This is inside an if REGULATOR block, as with all the other regulator
drivers it doesn't need a dependency on REGULATOR.

> --- /dev/null
> +++ b/drivers/regulator/hi6421v600-regulator.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for regulators in Hisi IC

Please make the entire comment a C++ one so things look more
intentional.

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mfd/hi6421-spmi-pmic.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/spmi.h>
> +#include <linux/time.h>
> +#include <linux/uaccess.h>

Are we sure about *all* these includes?

> +#define rdev_dbg(rdev, fmt, arg...)	\
> +		 pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)

If it is useful to copy this then put it in a header rather than
cut'n'pasting it.  I'm not sure I see a pressing need for most of the
trace here, it looks to be duplicating a lot of things the core does for
you.

> +static DEFINE_MUTEX(enable_mutex);

Drivers shouldn't be declaring global variables, if this is required it
should be allocated in driver data.

> +/*
> + * helper function to ensure when it returns it is at least 'delay_us'
> + * microseconds after 'since'.
> + */
> +
> +static int hi6421_spmi_regulator_is_enabled(struct regulator_dev *rdev)

The helper function appears to have been removed?

> +{
> +	struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> +	struct hi6421_spmi_pmic *pmic = sreg->pmic;
> +	u32 reg_val;
> +
> +	reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
> +
> +	rdev_dbg(rdev,
> +		 "enable_reg=0x%x, val= 0x%x, enable_state=%d\n",
> +		 rdev->desc->enable_reg,
> +		 reg_val, (reg_val & rdev->desc->enable_mask));
> +
> +	return ((reg_val & rdev->desc->enable_mask) != 0);
> +}

It looks like it would be less code overall to just implement a regmap
for the MFD, even if it were only used in this driver - almost
everything in here is just a carbon copy of standard helpers that the
core provides for regmap devices.  Doing it in the MFD would be more
idiomatic for everything though.

> +static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> +	struct hi6421_spmi_pmic *pmic = sreg->pmic;
> +
> +	/* cannot enable more than one regulator at one time */
> +	mutex_lock(&enable_mutex);
> +	usleep_range(HISI_REGS_ENA_PROTECT_TIME,
> +		     HISI_REGS_ENA_PROTECT_TIME + 1000);
> +
> +	/* set enable register */
> +	rdev_dbg(rdev,
> +		 "off_on_delay=%d us, enable_reg=0x%x, enable_mask=0x%x\n",
> +		 rdev->desc->off_on_delay, rdev->desc->enable_reg,
> +		 rdev->desc->enable_mask);
> +
> +	hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg,
> +			     rdev->desc->enable_mask,
> +			     rdev->desc->enable_mask);

This is the one operation which is doing anything unusual and which I'd
expect to be open coded in the driver - obviously this is a pretty
simplistic implementation but it will work though as indicated above it
shouldn't be using a global, the mutex should be in driver data.  I
guess you could use the mutex to protect a timestamp and use that to
figure out if a delay is actually needed?

> +static int hi6421_spmi_dt_parse(struct platform_device *pdev,
> +				struct hi6421v600_regulator *sreg,
> +			 struct regulator_desc *rdesc)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned int *v_table;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "reg", &rdesc->enable_reg);
> +	if (ret) {
> +		dev_err(dev, "missing reg property\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "vsel-reg", &rdesc->vsel_reg);

There is no way this stuff should be being parsed out of DT, we should
know the register map for the device and what regulators it has based
purely off knowing what device we have.  Baking the register map into
the ABI is bad practice, it prevents us from improving our support for
the hardware without going and updating people's DTs.

> +	/*
> +	 * Not all regulators work on idle mode
> +	 */
> +	ret = of_property_read_u32(np, "idle-mode-mask", &sreg->eco_mode_mask);
> +	if (ret) {
> +		dev_dbg(dev, "LDO doesn't support economy mode.\n");
> +		sreg->eco_mode_mask = 0;
> +		sreg->eco_uA = 0;
> +	} else {
> +		ret = of_property_read_u32(np, "eco-microamp", &sreg->eco_uA);

There's no conditional code to not register the mode operations if the
mode information is not available (and again this should be done based 
on knowing the properties of the device, this is unlikely to be system
dependent).

> +static int hi6421_spmi_regulator_probe_ldo(struct platform_device *pdev,
> +                                          struct device_node *np,
> +                                          struct hi6421_spmi_pmic *pmic)
> +{

This probe code looks very different to other regulator drivers, this
alone should have been a warning that the driver needs some substantial
refactoring here.  As indicated information about what regulators are
present on devices and their properties should be in the driver not the
DT, the driver should just be able to register them unconditionally and
use of_match and regulators_node to allow the core to find any
constraints that are provided by the platform.

> +	constraint->valid_modes_mask = REGULATOR_MODE_NORMAL;
> +	if (sreg->eco_mode_mask) {
> +		constraint->valid_modes_mask |= REGULATOR_MODE_IDLE;
> +		constraint->valid_ops_mask |= REGULATOR_CHANGE_MODE;
> +	}

This is absolutely not OK, a regulator driver should *not* be modifying
the constraints that the machine has set.  If it is safe to change modes
on a platform and the system integrator wants to do that then they will
set the constraints appropriately, there is no way the regulator driver
can tell what is appropriate on a given system.  The fact that the
driver is including machine.h at all ought to have been an indicator
that there's an abstraction problem here.

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

Powered by blists - more mailing lists