RE: [PATCH v1 1/4] regulator: pca9450: add pca9450 pmic driver

[Robin Gong]

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.