Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support

[Mark Brown <broonie@kernel.org>]

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

On Tue, Jun 07, 2022 at 01:52:40PM +0800, cy_huang wrote:

This looks mostly good, a few things though:

> +static void rt5120_fillin_regulator_desc(struct regulator_desc *desc, int rid)
> +{
> +	static const char * const name[] = { "buck1", "buck2", "buck3", "buck4",
> +					     "ldo", "exten" };
> +	static const char * const sname[] = { "vin1", "vin2", "vin3", "vin4",
> +					      "vinldo", NULL };

It would be easier and clearer to just make this a static table like
other drivers do, there's no need to generate anything dynamically as
far as I can see.

> +static int rt5120_of_parse_cb(struct rt5120_priv *priv, int rid,
> +			      struct of_regulator_match *match)
> +{
> +	struct regulator_desc *desc = priv->rdesc + rid;
> +	struct regulator_init_data *init_data = match->init_data;
> +
> +	if (!init_data || rid == RT5120_REGULATOR_BUCK1)
> +		return 0;
> +
> +	if (init_data->constraints.min_uV != init_data->constraints.max_uV) {
> +		dev_err(priv->dev, "Variable voltage for fixed regulator\n");
> +		return -EINVAL;
> +	}
> +
> +	desc->fixed_uV = init_data->constraints.min_uV;
> +	init_data->constraints.apply_uV = 0;

Drivers should never override constraints passed in by machine drivers,
if there's validation needed let the core do it.  The same probably
applies to providing a voltage range for a fixed regulator though that's
not modifying everything so not such a problem.

> +static int rt5120_parse_regulator_dt_data(struct rt5120_priv *priv)
> +{
> +	struct device *dev = priv->dev->parent;
> +	struct device_node *reg_node;
> +	int i, ret;
> +
> +	for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> +		rt5120_fillin_regulator_desc(priv->rdesc + i, i);
> +
> +		rt5120_regu_match[i].desc = priv->rdesc + i;
> +	}

Like I said above just make the list of regulators static data and loop
through registering them.

> +
> +	reg_node = of_get_child_by_name(dev->of_node, "regulators");
> +	if (!reg_node) {
> +		dev_err(priv->dev, "Couldn't find 'regulators' node\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_regulator_match(priv->dev, reg_node, rt5120_regu_match,
> +				 ARRAY_SIZE(rt5120_regu_match));
> +
> +	of_node_put(reg_node);
> +
> +	if (ret < 0) {
> +		dev_err(priv->dev,
> +			"Error parsing regulator init data (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < RT5120_MAX_REGULATOR; i++) {
> +		ret = rt5120_of_parse_cb(priv, i, rt5120_regu_match + i);
> +		if (ret) {
> +			dev_err(priv->dev, "Failed in [%d] of_passe_cb\n", i);
> +			return ret;
> +		}
> +	}

This is all open coding stuff that's in the core - just provde an
of_parse_cb() operation and let the core take care of calling it.

> +static int rt5120_device_property_init(struct rt5120_priv *priv)
> +{
> +	struct device *dev = priv->dev->parent;
> +	bool prot_enable;
> +	unsigned int prot_enable_val = 0;
> +
> +	/* Assign UV/OV HW protection behavior */
> +	prot_enable = device_property_read_bool(dev,
> +					"richtek,enable-undervolt-hiccup");
> +	if (prot_enable)
> +		prot_enable_val |= RT5120_UVHICCUP_MASK;

Use the DT APIs to parse DT - since ACPI has a very strong idea of how
power management works which is fundamentally incompatible with with the
DT model we should be writing code in a way that minimises the risk that
we'll end up trying to parse DT properties out of ACPI systems and
creating confusion as DT and ACPI software tries to run on the same
system.

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