Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
In-Reply-To: <1649939418-19861-8-git-send-email-quic_c_skakit@quicinc.com>
References: <1649939418-19861-1-git-send-email-quic_c_skakit@quicinc.com> <1649939418-19861-8-git-send-email-quic_c_skakit@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Thu, 14 Apr 2022 17:25:49 -0700
Message-ID: <CAE-0n533obTi995x_rJG_ihUUquF3MQLJt6VMf7=oxyzMUL5DQ@mail.gmail.com>
Subject: Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Satya Priya <quic_c_skakit@quicinc.com>
Cc:     Lee Jones <lee.jones@linaro.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        quic_collinsd@quicinc.com, quic_subbaram@quicinc.com,
        quic_jprakash@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.