Re: [PATCH 4/8] regulator: stpmu1: add stpmu1 regulator driver

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jul 05, 2018 at 03:14:23PM +0000, Pascal PAILLET-LME wrote:

>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_PROTECTION_CONSUMER) += protection-consumer.o
>  
>  obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o
>  obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o

Looks like this got included by mistake...

> --- /dev/null
> +++ b/drivers/regulator/stpmu1_regulator.c
> @@ -0,0 +1,919 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved

All rights reserved and GPL :)

Please also make the entire comment a C++ one so the block looks more
intentional.

> +static unsigned int stpmu1_map_mode(unsigned int mode)
> +{
> +	return mode == REGULATOR_MODE_STANDBY ?
> +		REGULATOR_MODE_STANDBY : REGULATOR_MODE_NORMAL;
> +}

This looks confused - what's going on here?  Normally a map_mode()
operation would be translating values in DT but this translates
everything that isn't standby into normal which suggests there's an
error checking problem.

> +static int stpmu1_ldo3_list_voltage(struct regulator_dev *rdev,
> +				    unsigned int sel)
> +{
> +	struct stpmu1_regulator *regul = rdev_get_drvdata(rdev);
> +
> +	if (sel == 0)
> +		return regulator_list_voltage_linear_range(rdev, 1);
> +
> +	if (sel < 31)
> +		return regulator_list_voltage_linear_range(rdev, sel);
> +
> +	if (sel == 31)
> +		return stpmu1_get_voltage_regmap(regul->voltage_ref_reg) / 2;
> +
> +	return -EINVAL;
> +}

The only thing that's going on here that's odd and that couldn't be
represented with a helper is selector 31 which looks to be some sort of
divided bypass mode - can you explain what this represents in hardware
terms please?

> +static int stpmu1_ldo3_get_voltage(struct regulator_dev *rdev)
> +{
> +	int sel = regulator_get_voltage_sel_regmap(rdev);
> +
> +	if (sel < 0)
> +		return -EINVAL;
> +
> +	return stpmu1_ldo3_list_voltage(rdev, (unsigned int)sel);
> +}

This is just the standard core behaviour, no need for this.

> +static int stpmu1_fixed_regul_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct stpmu1_regulator *regul = rdev_get_drvdata(rdev);
> +	int id = rdev_get_id(rdev);
> +
> +	/* VREF_DDR voltage is equal to Buck2/2 */
> +	if (id == STPMU1_VREF_DDR)
> +		return stpmu1_get_voltage_regmap(regul->voltage_ref_reg) / 2;
> +
> +	/* For all other case , rely on fixed value defined by Hw settings */
> +	return regul->cfg->desc.fixed_uV;
> +}

It'd be better to just use a separate set of operations for the DDR
regulator rather than have a conditional here, less code and makes it
clearer that this one is a special case.

> +static void update_regulator_constraints(int index,
> +					 struct regulator_init_data *init_data)
> +{
> +	struct stpmu1_regulator_cfg *cfg = &stpmu1_regulator_cfgs[index];
> +	struct regulator_desc *desc = &cfg->desc;
> +
> +	init_data->constraints.valid_ops_mask |=
> +		cfg->valid_ops_mask;
> +	init_data->constraints.valid_modes_mask |=
> +		cfg->valid_modes_mask;

Drivers should never be modifying their constraints, this is a no no.

> +	/*
> +	 * if all constraints are not specified in DT , ensure Hw
> +	 * constraints are met
> +	 */
> +	if (desc->n_voltages > 1) {

Drivers shouldn't be doing this either.  The API will not allow any
modifications of hardware state without constraints so unless the
bootloader is leaving things in a broken state we should be fine.

> +	if (!init_data->constraints.ramp_delay)
> +		init_data->constraints.ramp_delay = PMIC_RAMP_SLOPE_UV_PER_US;
> +
> +	if (!init_data->constraints.enable_time)
> +		init_data->constraints.enable_time = PMIC_ENABLE_TIME_US;

If these are hard constraints we know from the chip design they should
be being set in the descriptor.  The constraints are there to override
if either they're board dependent or the board needs something longer
than the chip default.

> +	/* LDO3 and VREF_DDR can use buck2 as reference voltage */
> +	if (regul->regul_id == STPMU1_LDO3 ||
> +	    regul->regul_id == STPMU1_VREF_DDR) {
> +		if (!buck2) {
> +			dev_err(&pdev->dev,
> +				"Error in PMIC regulator settings: Buck2 is not defined prior to LDO3 or VREF_DDR regulators\n"
> +				);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		regul->voltage_ref_reg = buck2;
> +	}

Can or do?  Usually this would be hooked up in the DT (with the
regulator specifying a supply name in the desc).

> +	np = pdev->dev.of_node;
> +	if (!np) {
> +		dev_err(&pdev->dev, "regulators node not found\n");
> +		return -EINVAL;
> +	}
> +

May as well let the driver probe?

> +	/* Register all defined (from DT) regulators to Regulator Framework */
> +	for (i = 0; i < ARRAY_SIZE(stpmu1_regulator_cfgs); i++) {
> +		/* Parse DT & find regulators to register */
> +		init_data = stpmu1_regulators_matches[i].init_data;

You should register everything that's physically there unconditionally,
there's no harm in having a regulator registered that's not used and it
might be useful for debugging purposes.

> +static const struct of_device_id of_pmic_regulator_match[] = {
> +	{ .compatible = "st,stpmu1-regulators" },
> +	{ },
> +};

This is part of a MFD for a single chip, why do we need a specific
compatible string here rather than just enumerating from the base
registration of the device?

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