| 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
| ||
|
Message-ID: <CAE-0n53WLYR1pjnr6wASVmXXQ7xTq5n2Q7GdeKOCkWf4H4n=0A@mail.gmail.com>
Date: Fri, 20 May 2022 19:56:44 -0700
From: Stephen Boyd <swboyd@...omium.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Satya Priya <quic_c_skakit@...cinc.com>
Cc: Lee Jones <lee.jones@...aro.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
quic_collinsd@...cinc.com, quic_subbaram@...cinc.com,
quic_jprakash@...cinc.com
Subject: Re: [PATCH V13 7/9] regulator: Add a regulator driver for the PM8008 PMIC
Quoting Satya Priya (2022-05-20 03:49:35)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..6e815c6
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/qcom_pm8008.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
What in of_device.h is used?
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +
> +#define VSET_STEP_MV 8
> +#define VSET_STEP_UV (VSET_STEP_MV * 1000)
> +
> +#define LDO_ENABLE_REG(base) ((base) + 0x46)
> +#define ENABLE_BIT BIT(7)
> +
> +#define LDO_VSET_LB_REG(base) ((base) + 0x40)
> +
> +#define LDO_STEPPER_CTL_REG(base) ((base) + 0x3b)
> +#define DEFAULT_VOLTAGE_STEPPER_RATE 38400
> +#define STEP_RATE_MASK GENMASK(1, 0)
> +
> +struct pm8008_regulator_data {
> + const char *name;
> + const char *supply_name;
> + int min_uv;
> + int max_uv;
> + int min_dropout_uv;
> + const struct linear_range *voltage_range;
> +};
> +
> +struct pm8008_regulator {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regulator_desc rdesc;
> + u16 base;
> + int step_rate;
> + int voltage_selector;
> +};
> +
> +static const struct linear_range nldo_ranges[] = {
> + REGULATOR_LINEAR_RANGE(528000, 0, 122, 8000),
> +};
> +
> +static const struct linear_range pldo_ranges[] = {
> + REGULATOR_LINEAR_RANGE(1504000, 0, 237, 8000),
> +};
> +
> +static const struct pm8008_regulator_data reg_data[] = {
> + /* name parent min_uv max_uv headroom_uv voltage_range */
> + { "ldo1", "vdd_l1_l2", 528000, 1504000, 225000, nldo_ranges, },
> + { "ldo2", "vdd_l1_l2", 528000, 1504000, 225000, nldo_ranges, },
> + { "ldo3", "vdd_l3_l4", 1504000, 3400000, 300000, pldo_ranges, },
> + { "ldo4", "vdd_l3_l4", 1504000, 3400000, 300000, pldo_ranges, },
> + { "ldo5", "vdd_l5", 1504000, 3400000, 200000, pldo_ranges, },
> + { "ldo6", "vdd_l6", 1504000, 3400000, 200000, pldo_ranges, },
> + { "ldo7", "vdd_l7", 1504000, 3400000, 200000, pldo_ranges, },
If min/max_uv isn't changing per voltage_range maybe we can compare the
voltage_range pointer and set the min/max based on the pointer. That
would save some space in this table.
> +};
> +
> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> + return pm8008_reg->voltage_selector;
Can this read the hardware instead of caching the value from
pm8008_regulator_set_voltage()?
> +}
> +
> +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg,
> + int mV)
> +{
> + __le16 vset_raw;
> +
> + vset_raw = cpu_to_le16(mV);
> +
> + return regmap_bulk_write(pm8008_reg->regmap,
> + LDO_VSET_LB_REG(pm8008_reg->base),
> + (const void *)&vset_raw, sizeof(vset_raw));
> +}
> +
> +static int pm8008_regulator_set_voltage_time(struct regulator_dev *rdev,
> + int old_uV, int new_uv)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> +
> + return DIV_ROUND_UP(abs(new_uv - old_uV), pm8008_reg->step_rate);
> +}
> +
> +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> + int rc, mV;
> +
> + /* voltage control register is set with voltage in millivolts */
> + mV = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev, selector),
If regulator_list_voltage_linear_range() returns an error value then
we're going to div round up by 1000 and return it? Will it even be an
error value anymore?
Check for an error before div round up I suspect.
> + 1000);
> + if (mV < 0)
> + return mV;
> +
> + rc = pm8008_write_voltage(pm8008_reg, mV);
> + if (rc < 0)
> + return rc;
> +
> + pm8008_reg->voltage_selector = selector;
> + dev_dbg(&rdev->dev, "voltage set to %d\n", mV * 1000);
We have tracepoints for this, right? Remove the debug printk please and
use tracepoints instead.
> + return 0;
> +}
> +
> +static const struct regulator_ops pm8008_regulator_ops = {
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_voltage_sel = pm8008_regulator_set_voltage,
> + .get_voltage_sel = pm8008_regulator_get_voltage,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_voltage_time = pm8008_regulator_set_voltage_time,
> +};
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> + int rc, i;
> + u32 base;
> + unsigned int reg;
> + const char *name;
> + struct device *dev = &pdev->dev;
> + struct regulator_config reg_config = {};
> + struct regulator_dev *rdev;
> + const struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent);
> + struct pm8008_regulator *pm8008_reg;
> +
> + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> + if (!pm8008_reg)
> + return -ENOMEM;
> +
> + pm8008_reg->regmap = pm8008_get_regmap(chip);
> + if (!pm8008_reg->regmap) {
> + dev_err(dev, "parent regmap is missing\n");
> + return -EINVAL;
> + }
> +
> + pm8008_reg->dev = dev;
> +
> + rc = of_property_read_string(dev->of_node, "regulator-name", &name);
> + if (rc)
> + return rc;
> +
> + /* get the required regulator data */
> + for (i = 0; i < ARRAY_SIZE(reg_data); i++)
> + if (strstr(name, reg_data[i].name))
Why not find this via reg/address instead? It would save storing the
regulator name in the reg_data table.
> + break;
> +
> + if (i == ARRAY_SIZE(reg_data)) {
> + dev_err(dev, "Invalid regulator name %s\n", name);
> + return -EINVAL;
> + }
> +
> + rc = of_property_read_u32_index(dev->of_node, "reg", 1, &base);
> + if (rc < 0) {
> + dev_err(dev, "%s: failed to get regulator base rc=%d\n", name, rc);
> + return rc;
> + }
> + pm8008_reg->base = base;
> +
> + /* get slew rate */
> + rc = regmap_bulk_read(pm8008_reg->regmap,
> + LDO_STEPPER_CTL_REG(pm8008_reg->base), ®, 1);
> + if (rc < 0) {
> + dev_err(dev, "failed to read step rate configuration rc=%d\n", rc);
> + return rc;
> + }
> + reg &= STEP_RATE_MASK;
> + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
> +
> + pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
> + pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
> + pm8008_reg->rdesc.name = reg_data[i].name;
The name could be set via dev_name()?
> + pm8008_reg->rdesc.supply_name = reg_data[i].supply_name;
> + pm8008_reg->rdesc.of_match = reg_data[i].name;
Is this used?
> + pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
> + pm8008_reg->rdesc.min_uV = reg_data[i].min_uv;
> + pm8008_reg->rdesc.n_voltages
> + = ((reg_data[i].max_uv - reg_data[i].min_uv)
> + / pm8008_reg->rdesc.uV_step) + 1;
> + pm8008_reg->rdesc.linear_ranges = reg_data[i].voltage_range;
> + pm8008_reg->rdesc.n_linear_ranges = 1;
We should make sure nldo_ranges and pldo_ranges doesn't become larger
and we forget to update this. Can we use some static assert?
static_assert(ARRAY_SIZE(pldo_ranges) == 1 && ARRAY_SIZE(nldo_ranges) == 1);
> + pm8008_reg->rdesc.enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> + pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
> + pm8008_reg->rdesc.min_dropout_uV = reg_data[i].min_dropout_uv;
> + pm8008_reg->voltage_selector = -ENOTRECOVERABLE;
> +
> + reg_config.dev = dev->parent;
> + reg_config.driver_data = pm8008_reg;
> + reg_config.regmap = pm8008_reg->regmap;
> +
> + rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, ®_config);
> + if (IS_ERR(rdev)) {
> + rc = PTR_ERR(rdev);
> + dev_err(dev, "%s: failed to register regulator rc=%d\n",
> + reg_data[i].name, rc);
> + return rc;
> + }
> +
> + return 0;
> +}
> +
Powered by blists - more mailing lists