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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <VE1PR04MB66384243FA576A12E8A61E03896A0@VE1PR04MB6638.eurprd04.prod.outlook.com>
Date:   Fri, 3 Jul 2020 02:38:21 +0000
From:   Robin Gong <yibin.gong@....com>
To:     Schrempf Frieder <frieder.schrempf@...tron.de>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Anson Huang <anson.huang@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        John Lee <john.lee@....com>
CC:     "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH v1 1/4] regulator: pca9450: add pca9450 pmic driver

On 2020/07/02 20:05 Schrempf Frieder <frieder.schrempf@...tron.de> wrote:
> Hi Robin,
> 
> On 20.05.20 00:05, Robin Gong wrote:
> > Add NXP pca9450 pmic driver.
> >
> > Signed-off-by: Robin Gong <yibin.gong@....com>
> 
> 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.