linux-kernel - Re: [PATCH 3/4] regulator: Add a regulator driver for the PM8008 PMIC

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210917153837.GB4700@sirena.org.uk>
Date:   Fri, 17 Sep 2021 16:38:37 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Satya Priya <skakit@...eaurora.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>, mka@...omium.org,
        swboyd@...omium.org, Das Srinagesh <gurus@...eaurora.org>,
        David Collins <collinsd@...eaurora.org>, kgunda@...eaurora.org,
        Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] regulator: Add a regulator driver for the PM8008 PMIC

On Fri, Sep 17, 2021 at 04:15:37PM +0530, Satya Priya wrote:

> +static int pm8008_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc;
> +	u8 reg;
> +
> +	rc = pm8008_read(pm8008_reg->regmap,
> +			LDO_ENABLE_REG(pm8008_reg->base), &reg, 1);
> +	if (rc < 0) {
> +		pr_err("failed to read enable reg rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	return !!(reg & ENABLE_BIT);
> +}

This could just be regulator_is_enabled_regmap().  There's also a lot of
instances in the driver where it's using pr_err() not dev_err() (and
similarly for the debug prints).

> +
> +static int pm8008_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +	int rc, current_uv, delay_us, delay_ms, retry_count = 10;
> +	u8 reg;

This is the regmap helper.

> +	/*
> +	 * Wait for the VREG_READY status bit to be set using a timeout delay
> +	 * calculated from the current commanded voltage.
> +	 */
> +	delay_us = STARTUP_DELAY_USEC
> +			+ DIV_ROUND_UP(current_uv, pm8008_reg->step_rate);
> +	delay_ms = DIV_ROUND_UP(delay_us, 1000);

Set poll_enable_time and implement get_status() then this will be
handled by the core.

> +static int pm8008_regulator_disable(struct regulator_dev *rdev)
> +{

Use the regmap helper.

> +	rc = pm8008_write_voltage(pm8008_reg, min_uv, max_uv);
> +	if (rc < 0)
> +		return rc;

This is the only place where write_voltage() is called, may as well just
inline it.

> +	init_voltage = -EINVAL;
> +	of_property_read_u32(reg_node, "qcom,init-voltage", &init_voltage);

Why does this property exist and if it's needed why is it specific to
this device?  It looks like the device allows you to read the voltage on
startup from the regmap.

> +	init_data = of_get_regulator_init_data(dev, reg_node,
> +						&pm8008_reg->rdesc);
> +	if (init_data == NULL) {
> +		dev_err(dev, "%s: failed to get regulator data\n", name);
> +		return -ENODATA;
> +	}
> +	if (!init_data->constraints.name) {
> +		dev_err(dev, "%s: regulator name missing\n", name);
> +		return -EINVAL;
> +	}

Just let the core find the init data for you, there is no reason to
insist on a system provided name - that is an entirely optional property
for systems to use, there is no reason for a regulator driver to care.

> +	init_data->constraints.input_uV = init_data->constraints.max_uV;
> +	init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_STATUS
> +						| REGULATOR_CHANGE_VOLTAGE;

This is absolutely not something that a regulator driver should be
doing, the whole point with constraints is that they come from the
machine.

> +static int pm8008_parse_regulator(struct regmap *regmap, struct device *dev)
> +{
> +	int rc = 0;
> +	const char *name;
> +	struct device_node *child;
> +	struct pm8008_regulator *pm8008_reg;
> +
> +	/* parse each subnode and register regulator for regulator child */
> +	for_each_available_child_of_node(dev->of_node, child) {
> +		pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +		if (!pm8008_reg)

You shouldn't be doing this, just unconditionally register all the
regulators supported by the chip.  If they don't appear in the DT that's
totally fine - it gives read only access which can be useful for
diagnostics.

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