Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH V5 4/6] regulator: Add a regulator driver for the PM8008
 PMIC
To:     Mark Brown <broonie@kernel.org>
CC:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Lee Jones <lee.jones@linaro.org>,
        "Liam Girdwood" <lgirdwood@gmail.com>,
        Das Srinagesh <gurus@codeaurora.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <swboyd@chromium.org>,
        <quic_collinsd@quicinc.com>, <quic_subbaram@quicinc.com>,
        <quic_jprakash@quicinc.com>
References: <1644331940-18986-1-git-send-email-quic_c_skakit@quicinc.com>
 <1644331940-18986-5-git-send-email-quic_c_skakit@quicinc.com>
 <YgPNLq6tdj5/UpZE@sirena.org.uk>
From:   "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@quicinc.com>
Message-ID: <66830ee2-5846-8298-3371-359c7d1999db@quicinc.com>
Date:   Fri, 11 Feb 2022 15:30:48 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <YgPNLq6tdj5/UpZE@sirena.org.uk>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 2/9/2022 7:48 PM, Mark Brown wrote:
> On Tue, Feb 08, 2022 at 08:22:18PM +0530, Satya Priya wrote:
>
>> +static int pm8008_regulator_of_parse(struct device_node *node,
>> +			const struct regulator_desc *desc,
>> +			struct regulator_config *config)
>> +{
>> +	struct pm8008_regulator *pm8008_reg = config->driver_data;
>> +	int rc;
>> +	unsigned int reg;
>> +
>> +	/* get slew rate */
>> +	rc = regmap_bulk_read(pm8008_reg->regmap,
>> +			LDO_STEPPER_CTL_REG(pm8008_reg->base), &reg, 1);
>> +	if (rc < 0) {
>> +		dev_err(pm8008_reg->dev,
>> +			"%s: failed to read step rate configuration rc=%d\n",
>> +			pm8008_reg->rdesc.name, rc);
>> +		return rc;
>> +	}
>> +	reg &= STEP_RATE_MASK;
>> +	pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> reg;
>> +
>> +	return 0;
> This is not doing any parsing of any DT properties at all, it is just
> reading a default value back from the hardware.  This shouldn't be in
> the of_parse() callback, it should be done on probe() or something so
> that if someone adds ACPI support or whatever there's no surprise
> breakage, and so that we've got this configured even if there's no DT
> bindings for the specific regulator.


Okay, I'll move it to probe.


>
>> +}
>> +
>> +static int pm8008_regulator_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int id = pdev->id % PM8008_NUM_LDOS;
>> +	struct regulator_dev    *rdev;
>> +	struct pm8008_regulator *pm8008_reg;
>> +	struct regmap *regmap;
>> +	struct regulator_config reg_config = {};
>> +	int rc;
>> +
>> +	dev_dbg(dev, "DEBUG: Probing LDO%d\n", id + 1);
>> +
>> +	regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!regmap) {
>> +		dev_err(dev, "parent regmap is missing\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	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[id].base;
>> +
>> +	pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> +	pm8008_reg->rdesc.regulators_node = of_match_ptr("regulators");
>> +	pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> +	pm8008_reg->rdesc.name = reg_data[id].name;
>> +	pm8008_reg->rdesc.supply_name = reg_data[id].supply_name;
>> +	pm8008_reg->rdesc.of_match = reg_data[id].name;
>> +	pm8008_reg->rdesc.of_parse_cb = pm8008_regulator_of_parse;
>> +	pm8008_reg->rdesc.uV_step = VSET_STEP_UV;
>> +	pm8008_reg->rdesc.min_uV = reg_data[id].min_uv;
>> +	pm8008_reg->rdesc.n_voltages
>> +		= ((reg_data[id].max_uv - reg_data[id].min_uv)
>> +			/ pm8008_reg->rdesc.uV_step) + 1;
>> +	pm8008_reg->rdesc.linear_ranges = reg_data[id].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[id].min_dropout_uv;
>> +
>> +	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[id].name, rc);
>> +		return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver pm8008_regulator_driver = {
>> +	.driver	= {
>> +		.name		= "qcom,pm8008-regulators",
>> +	},
>> +	.probe	= pm8008_regulator_probe,
>> +};
>> +
>> +module_platform_driver(pm8008_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm PM8008 PMIC Regulator Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.7.4
>>