On Wed, May 08, 2024 at 10:37:50PM +0000, Stephen Boyd wrote:
> Quoting Johan Hovold (2024-05-06 08:08:29)
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#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)
>
> Include bits.h?
Sure.
I wanted to avoid changing Qualcomm's v15 driver too much and
essentially submitted it unchanged except for the probe rework. I'll
take closer look at things like this for v2.
> > +struct pm8008_regulator {
> > + struct regmap *regmap;
> > + struct regulator_desc rdesc;
> > + u16 base;
> > + int step_rate;
>
> Is struct regulator_desc::vsel_step usable for this? If not, can it be
> unsigned?
Not sure, I'll take a look when respinning.
> > +};
> > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev);
> > + __le16 mV;
> > + int uV;
>
> Can this be unsigned? Doubt we have negative voltage and this would
> match rdesc.min_uV type.
Makes sense.
> > +
> > + regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2);
>
> Is struct regulator_desc::vsel_reg usable for this?
Will look into that.
> > +
> > + uV = le16_to_cpu(mV) * 1000;
> > + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step;
> > +}
> > +
> > +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));
>
> Is the cast to please sparse?
No idea, I think it's just a stylistic preference that can be dropped.
> > +}
> > +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;
> > +
> > + rc = regulator_list_voltage_linear_range(rdev, selector);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* voltage control register is set with voltage in millivolts */
> > + mV = DIV_ROUND_UP(rc, 1000);
> > +
> > + rc = pm8008_write_voltage(pm8008_reg, mV);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return 0;
>
> Can be shorter to save lines
>
> return pm8008_write_voltage(pm8008_reg, mV);
Possibly, but I tend to prefer explicit error paths (e.g. for symmetry).
> > +}
> > +static int pm8008_regulator_probe(struct platform_device *pdev)
> > +{
> > + struct regulator_config reg_config = {};
> > + struct pm8008_regulator *pm8008_reg;
> > + struct device *dev = &pdev->dev;
> > + struct regulator_desc *rdesc;
> > + struct regulator_dev *rdev;
> > + struct regmap *regmap;
> > + unsigned int val;
> > + int rc, i;
> > +
> > + regmap = dev_get_regmap(dev->parent, "secondary");
> > + if (!regmap)
> > + 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->base = reg_data[i].base;
> > +
> > + /* get slew rate */
> > + rc = regmap_bulk_read(pm8008_reg->regmap,
> > + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1);
> > + if (rc < 0) {
> > + dev_err(dev, "failed to read step rate: %d\n", rc);
>
> Is it step rate or slew rate? The comment doesn't agree with the error
> message.
Noticed that too, can update the comment.
> > + return rc;
> > + }
> > + val &= STEP_RATE_MASK;
> > + pm8008_reg->step_rate = DEFAULT_VOLTAGE_STEPPER_RATE >> val;
> > +
> > + rdesc = &pm8008_reg->rdesc;
> > + rdesc->type = REGULATOR_VOLTAGE;
> > + rdesc->ops = &pm8008_regulator_ops;
> > + rdesc->name = reg_data[i].name;
> > + rdesc->supply_name = reg_data[i].supply_name;
> > + rdesc->of_match = reg_data[i].name;
> > + rdesc->uV_step = VSET_STEP_UV;
> > + rdesc->linear_ranges = reg_data[i].voltage_range;
> > + rdesc->n_linear_ranges = 1;
> > + BUILD_BUG_ON((ARRAY_SIZE(pldo_ranges) != 1) ||
>
> This should be an && not || right?
No, I think this is correct as it stands if the intention is to prevent
anyone from extending either pldo_ranges or nldo_ranges.
> > + (ARRAY_SIZE(nldo_ranges) != 1));
> > +
> > + if (reg_data[i].voltage_range == nldo_ranges) {
> > + rdesc->min_uV = NLDO_MIN_UV;
> > + rdesc->n_voltages = ((NLDO_MAX_UV - NLDO_MIN_UV) / rdesc->uV_step) + 1;
> > + } else {
> > + rdesc->min_uV = PLDO_MIN_UV;
> > + rdesc->n_voltages = ((PLDO_MAX_UV - PLDO_MIN_UV) / rdesc->uV_step) + 1;
> > + }
> > +
> > + rdesc->enable_reg = LDO_ENABLE_REG(pm8008_reg->base);
> > + rdesc->enable_mask = ENABLE_BIT;
> > + rdesc->min_dropout_uV = reg_data[i].min_dropout_uv;
> > + rdesc->regulators_node = of_match_ptr("regulators");
> > +
> > + reg_config.dev = dev->parent;
> > + reg_config.driver_data = pm8008_reg;
> > + reg_config.regmap = pm8008_reg->regmap;
> > +
> > + rdev = devm_regulator_register(dev, rdesc, ®_config);
> > + if (IS_ERR(rdev)) {
> > + rc = PTR_ERR(rdev);
> > + dev_err(dev, "failed to register regulator %s: %d\n",
> > + reg_data[i].name, rc);
> > + return rc;
>
> Could be return dev_err_probe() to simplify.
Possibly, but I think I prefer not using it when there is nothing that
can trigger a probe deferral.
Johan
