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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yp+gS6r5Kpi33Ags@sirena.org.uk>
Date:   Tue, 7 Jun 2022 20:00:27 +0100
From:   Mark Brown <broonie@...nel.org>
To:     cy_huang <u0084500@...il.com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        lee.jones@...aro.org, dmitry.torokhov@...il.com,
        lgirdwood@...il.com, cy_huang@...htek.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org
Subject: Re: [PATCH 3/4] regulator: rt5120: Add PMIC regulator support

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.

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