linux-kernel - Re: [PATCH V10 7/9] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAE-0n533obTi995x_rJG_ihUUquF3MQLJt6VMf7=oxyzMUL5DQ@mail.gmail.com>
Date:   Thu, 14 Apr 2022 17:25:49 -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 V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

Quoting Satya Priya (2022-04-14 05:30:16)
> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> new file mode 100644
> index 0000000..4375ac5
> --- /dev/null
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
> +
[..]
> +
> +static int pm8008_regulator_probe(struct platform_device *pdev)
> +{
> +       struct pm8008_data *chip = dev_get_drvdata(pdev->dev.parent);
> +       struct device *dev = &pdev->dev;
> +       struct regulator_dev    *rdev;
> +       struct pm8008_regulator *pm8008_reg;
> +       struct regmap *regmap;
> +       struct regulator_config reg_config = {};
> +       int rc, i;
> +       unsigned int reg;
> +
> +       regmap = pm8008_get_regmap(chip, PM8008_REGULATORS_SID);

Can we use dev_get_regmap(pdev->dev.parent, "regulators") here
instead? The benefit to that is we don't tightly couple this driver to
the pm8008_data structure or header file.

> +       if (!regmap) {
> +               dev_err(dev, "parent regmap is missing\n");
> +               return -EINVAL;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(reg_data); i++) {
> +               pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL);
> +               if (!pm8008_reg)
> +                       return -ENOMEM;
> +
> +               pm8008_reg->regmap = regmap;
> +               pm8008_reg->dev = dev;
> +               pm8008_reg->base = reg_data[i].base;
> +
> +               /* get slew rate */
> +               rc = regmap_bulk_read(pm8008_reg->regmap,
> +                               LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 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;
> +               pm8008_reg->rdesc.supply_name = reg_data[i].supply_name;
> +               pm8008_reg->rdesc.of_match = reg_data[i].name;
> +               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;
> +               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;
> +               pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
> +
> +               reg_config.dev = dev->parent;
> +               reg_config.driver_data = pm8008_reg;
> +
> +               rdev = devm_regulator_register(dev, &pm8008_reg->rdesc, &reg_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;
> +}
> +
> +static struct platform_driver pm8008_regulator_driver = {
> +       .driver = {
> +               .name           = "qcom-pm8008-regulator",

I'd prefer to use an of_device_id table here. That would let us populate
a "qcom,pm8008-regulators" node that had the ldo nodes as children and
avoid mfd cells.