[PATCH v4 2/3] regulator: Add RPMI voltage service

[Mark Brown]

> The RPMI specification defines a voltage service group which can be
> accessed via SBI MPXY extension or dedicated S-mode RPMI transport.

> Add a mailbox client based regulator driver for the RISC-V RPMI voltage
> service group. The driver enumerates the voltage domains advertised by
> the platform microcontroller, reads their supported voltage levels and
> registers each of them as a regulator.

> +++ b/drivers/regulator/riscv-rpmi-regulator.c
> @@ -0,0 +1,908 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * RISC-V RPMI Based Regulator Driver through SBI MPXY
> + *

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

> +static int rpmi_reg_setup(struct rpmi_reg_domain *mpxy_reg)
> +{
> +	struct regulation_constraints *constraints = &mpxy_reg->init_data.constraints;
> +	struct rpmi_reg_level_linear *linear_level;
> +	struct linear_range *linear_ranges;
> +	u32 i, linear_index, n_step, top;
> +	u32 min_uV = U32_MAX, max_uV = 0;
> +
> +	mpxy_reg->desc.name = devm_kasprintf(mpxy_reg->dev, GFP_KERNEL, "%s", mpxy_reg->name);

Do we need to take this copy?

> +	/*
> +	 * Everything a regulator constraint would describe is already known:
> +	 * the levels come from VOLT_GET_SUPPORTED_LEVELS and the always-on
> +	 * capability from the VOLT_GET_ATTRIBUTES flags. Build the constraints
> +	 * from that rather than from a device tree node, which would only be a
> +	 * second copy of the same facts. Without them the core leaves
> +	 * REGULATOR_CHANGE_VOLTAGE clear and refuses every set_voltage().
> +	 */
> +	constraints->name = mpxy_reg->desc.name;
> +	constraints->min_uV = min_uV;
> +	constraints->max_uV = max_uV;
> +	constraints->always_on = mpxy_reg->always_on;
> +	constraints->valid_ops_mask = REGULATOR_CHANGE_VOLTAGE;

No, the driver should not be specifying any constraints.  Those should
come from the platform.  These regulators are described in DT, their
constraints should come from DT as normal.

> +		ret = rpmi_reg_get_attrs(rpmi_reg);
> +		if (ret) {
> +			dev_warn(rpmi_reg->dev,
> +				 "voltage domain %d initialization failed\n",
> +				 rpmi_reg->id);

id is unsigned.

> +
> +	/*
> +	 * One line for the lot. A domain that did not make it has already said
> +	 * so above, so naming each one that did only buys a count.
> +	 */
> +	dev_info(dev, "%u MPXY voltage domains registered\n", registered);

The core is fairly chatty...

-- 
Mark Brown <broonie at kernel.org>