On 2020/07/02 20:05 Schrempf Frieder <frieder.schrempf@xxxxxxxxxx> wrote:
> Hi Robin,
>
> On 20.05.20 00:05, Robin Gong wrote:
> > Add NXP pca9450 pmic driver.
> >
> > Signed-off-by: Robin Gong <yibin.gong@xxxxxxx>
>
> I rebased and applied on v5.8-rc3 an tested this with our i.MX8MM board with
> PCA9450A. It seems to work fine. Below you can find some comments.
Glad to hear that, thanks :)
> > +static struct regulator_ops pca9450_ldo_regulator_ops = {
> > + .enable = regulator_enable_regmap,
> > + .disable = regulator_disable_regmap,
> > + .is_enabled = regulator_is_enabled_regmap,
> > + .list_voltage = regulator_list_voltage_linear_range,
> > + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> > + .get_voltage_sel = regulator_get_voltage_sel_regmap, };
> > +
> > +/*
> > + * BUCK1/2/3
> > + * 0.60 to 2.1875V (12.5mV step)
> > + */
> > +static const struct regulator_linear_range pca9450_dvs_buck_volts[] = {
> > + REGULATOR_LINEAR_RANGE(600000, 0x00, 0x7F, 12500), };
>
> With the latest kernel (v5.8-rc) this doesn't compile anymore because of
> 60ab7f4153b6 ("regulator: use linear_ranges helper").
Yes, I'll rebase it later.
> > + ret = of_property_read_u32(np, prop, &uv);
> > + if (ret) {
> > + if (ret != -EINVAL)
> > + return ret;
> > + return 0;
> > + }
>
> I think this nested condition is easier to read like this:
Okay, will fix it in v2.
> if (ret && ret == -EINVAL)
> return 0;
> else if (ret)
> return ret;
>
> > + {
> > + .desc = {
> > + .name = "buck4",
> > + .of_match = of_match_ptr("BUCK4"),
> > + .regulators_node = of_match_ptr("regulators"),
> > + .id = PCA9450_BUCK4,
> > + .ops = &pca9450_buck_regulator_ops,
> > + .type = REGULATOR_VOLTAGE,
> > + .n_voltages = PCA9450_BUCK4_VOLTAGE_NUM,
> > + .linear_ranges = pca9450_buck_volts,
> > + .n_linear_ranges = ARRAY_SIZE(pca9450_buck_volts),
> > + .vsel_reg = PCA9450_REG_BUCK4OUT,
> > + .vsel_mask = BUCK4OUT_MASK,
> > + .enable_reg = PCA9450_REG_BUCK4CTRL,
> > + .enable_mask = BUCK4_ENMODE_MASK,
> > + .owner = THIS_MODULE,
> > + },
> > + },
>
> The description for buck4 is added twice here.
Yes, will remove it.
> > +/*
> > + * Buck3 removed on PCA9450B and conneced with Buck1 internal for
> > +dual phase
>
> Missing 't' in connected
Will correct it.
> > + /* Unmask all interrupt except PWRON/WDOG/RSVD */
> > + ret = regmap_update_bits(pca9450->regmap, PCA9450_REG_INT1_MSK,
> > + IRQ_VR_FLT1 | IRQ_VR_FLT2 | IRQ_LOWVSYS |
> > + IRQ_THERM_105 | IRQ_THERM_125,
> > + IRQ_PWRON | IRQ_WDOGB | IRQ_RSVD);
> > + if (ret) {
> > + dev_err(&i2c->dev, "Unmask irq error\n");
> > + return ret;
> > + }
>
> What about adding a print when the probe has succeeded? Otherwise we don't
> see anything about the driver in the log, when it probed successfully. Maybe
> something like:
>
> dev_info(&i2c->dev, "probed\n");
>
> which will result in the following message:
>
> nxp-pca9450 0-0025: probed
>
Okay, will add it for notice.
