Message-ID: <0fc33d57fe929cef3681cafd78e40a6a@codeaurora.org>
Date:   Wed, 27 Oct 2021 19:10:00 +0530
From:   skakit@...eaurora.org
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Das Srinagesh <gurus@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, mka@...omium.org,
        collinsd@...eaurora.org, subbaram@...eaurora.org,
        kgunda@...eaurora.org
Subject: Re: [PATCH V2 3/4] regulator: Add a regulator driver for the PM8008
 PMIC

On 2021-10-26 01:16, Stephen Boyd wrote:
> Quoting skakit@...eaurora.org (2021-10-22 05:28:34)
>> On 2021-10-06 00:05, Stephen Boyd wrote:
>> > Quoting Satya Priya (2021-09-30 21:00:58)
>> >> diff --git a/drivers/regulator/qcom-pm8008-regulator.c
>> >> b/drivers/regulator/qcom-pm8008-regulator.c
>> >> new file mode 100644
>> >> index 0000000..5dacaa4
>> >> --- /dev/null
>> >> +++ b/drivers/regulator/qcom-pm8008-regulator.c
>> >> @@ -0,0 +1,320 @@
>> >> +// SPDX-License-Identifier: GPL-2.0-only
>> >> +/* Copyright (c) 2021, The Linux Foundation. All rights reserved. */
>> >> +
>> >> +#include <linux/delay.h>
> [...]
>> >> +
>> >> +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
>> >> +{
>> >> +       struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
>> >> +       u8 vset_raw[2];
>> >> +       int rc;
>> >> +
>> >> +       rc = pm8008_read(pm8008_reg->regmap,
>> >> +                       LDO_VSET_LB_REG(pm8008_reg->base),
>> >> +                       vset_raw, 2);
>> >
>> > Can this be an __le16 mV?
>> >
>> 
>> Below is the diff after changing as per your suggestion, Please 
>> correct
>> me if wrong.
>> 
>> -       u8 vset_raw[2];
>> +       __le16 mV;
>>          int rc;
>> 
>> -       rc = pm8008_read(pm8008_reg->regmap,
>> -                       LDO_VSET_LB_REG(pm8008_reg->base),
>> -                       vset_raw, 2);
>> +       rc = regmap_bulk_read(pm8008_reg->regmap,
>> +                       LDO_VSET_LB_REG(pm8008_reg->base), &mV, 2);
>>          if (rc < 0) {
>>                  dev_err(pm8008_reg->dev, "failed to read regulator
>> voltage rc=%d\n", rc);
>>                  return rc;
>>          }
>> 
>> -       return (vset_raw[1] << 8 | vset_raw[0]) * 1000;
>> +       return le16_to_cpu(mV) * 1000;
> 
> Looks good. Does mV need to be casted when passed to 
> regmap_bulk_read()?
> 
>> 
>> Below is the diff:
>> 
>> -       int rc = 0, mv;
>> -       u8 vset_raw[2];
>> +       int rc, mv;
>> +       u16 vset_raw;
>>          [...]
>> -       vset_raw[0] = mv & 0xff;
>> -       vset_raw[1] = (mv & 0xff00) >> 8;
>> -       rc = pm8008_write(pm8008_reg->regmap,
>> LDO_VSET_LB_REG(pm8008_reg->base),
>> -                       vset_raw, 2);
>> +       vset_raw = cpu_to_le16(mv);
>> +
>> +       rc = regmap_bulk_write(pm8008_reg->regmap,
>> +                       LDO_VSET_LB_REG(pm8008_reg->base), &vset_raw,
>> +                       sizeof(vset_raw));
>> 
> 
> Ok, thanks
> 
>> >> +               dev_err(dev, "%s: failed to get regulator data\n",
>> >> name);
>> >> +               return -ENODATA;
>> >> +       }
>> >> +
>> >> +       init_data->constraints.input_uV =
>> >> init_data->constraints.max_uV;
>> >> +       reg_config.dev = dev;
>> >> +       reg_config.init_data = init_data;
>> >> +       reg_config.driver_data = pm8008_reg;
>> >> +       reg_config.of_node = reg_node;
>> >> +
>> >> +       pm8008_reg->rdesc.type = REGULATOR_VOLTAGE;
>> >> +       pm8008_reg->rdesc.ops = &pm8008_regulator_ops;
>> >> +       pm8008_reg->rdesc.name = init_data->constraints.name;
>> >> +       pm8008_reg->rdesc.supply_name = reg_data[i].supply_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.enable_reg = LDO_ENABLE_REG(base);
>> >> +       pm8008_reg->rdesc.enable_mask = ENABLE_BIT;
>> >> +       pm8008_reg->rdesc.min_dropout_uV = reg_data[i].min_dropout_uv;
>> >> +       of_property_read_u32(reg_node, "qcom,min-dropout-voltage",
>> >> +                            &pm8008_reg->rdesc.min_dropout_uV);
>> >
>> > Why do we allow DT to override this? Isn't it a property of the
>> > hardware
>> > that doesn't change? So the driver can hardcode the knowledge about the
>> > dropout.
>> >
>> 
>> The headroom values change with targets. We are adding some default
>> headroom values in the driver and later overwriting them with the 
>> actual
>> values specified in the DT.
> 
> What do you mean by "targets"? Is that the SoC the PMIC is paired with?

Yes I meant the SoC/board on which the pmic is present.

> I'd prefer it be a standard regulator property instead of qcom specific
> if it actually needs to be different based on different devices.
> 

Ok, I'll drop the qcom prefix.

>> 
>> >> +
>> >> +       pm8008_reg->rdev = devm_regulator_register(dev,
>> >> &pm8008_reg->rdesc,
>> >
>> > Is this assignment ever used? Seems like it would be better to merely
>> >
>> >       return PTR_ERR_OR_ZERO(devm_regulator_register(dev, ...));
>> >
>> 
>> Okay.
>> 
>> >> +                                               &reg_config);
>> >> +       if (IS_ERR(pm8008_reg->rdev)) {
>> >> +               rc = PTR_ERR(pm8008_reg->rdev);
>> >> +               dev_err(dev, "%s: failed to register regulator
>> >> rc=%d\n",
>> >> +                               pm8008_reg->rdesc.name, rc);
>> >> +               return rc;
>> >> +       }
>> >> +
>> >> +       dev_dbg(dev, "%s regulator registered\n", name);
>> >> +
>> >> +       return 0;
>> >> +}
>> >> +
>> >> +static int pm8008_parse_regulator(struct regmap *regmap, struct
>> >> device *dev)
>> >> +{
>> >> +       int rc = 0;
>> >
>> > Drop initialization.
>> >
>> 
>> Okay.
>> 
>> >> +       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);
>> >> +
>> >> +               pm8008_reg->regmap = regmap;
>> >> +               pm8008_reg->of_node = child;
>> >> +               pm8008_reg->dev = dev;
>> >> +
>> >> +               rc = of_property_read_string(child, "regulator-name",
>> >> &name);
>> >> +               if (rc)
>> >> +                       continue;
>> >> +
>> >> +               rc = pm8008_register_ldo(pm8008_reg, name);
>> >
>> > Can we use the of_parse_cb similar to qcom_spmi-regulator.c?
>> >
>> 
>> Are you suggesting to remove the pm8008_register_ldo API and add its
>> contents in probe itself and then use of_parse_cb callback like in
>> qcom_spmi-regulator.c?
> 
> Yes
> 

Okay.

>> 
>> Do we have any advantage using that here? Also I am not exactly sure
>> what all contents to put in that. Seems like we can put the step rate
>> and min-dropout-voltage configurations in there.
> 
> Right. The regulator code is setup to do "DT parsing stuff" for each
> regulator node already, so you don't need to duplicate that logic in
> this driver. That's the main goal, consolidate regulator matching and
> iteration into the core. Maybe Mark has more info.

Okay.

Powered by blists - more mailing lists