linux-kernel - Re: [PATCH v4 4/5] regulator: add regulator driver of hi655x pmic

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <568DDD0E.1060902@hisilicon.com>
Date:	Thu, 7 Jan 2016 11:35:42 +0800
From:	chenfeng <puck.chen@...ilicon.com>
To:	Mark Brown <broonie@...nel.org>
CC:	<lee.jones@...aro.org>, <lgirdwood@...il.com>,
	<linux-kernel@...r.kernel.org>, <yudongbin@...ilicon.com>,
	<saberlily.xia@...ilicon.com>, <suzhuangluan@...ilicon.com>,
	<kong.kongxinwei@...ilicon.com>, <xuyiping@...ilicon.com>,
	<z.liuxinliang@...ilicon.com>, <puck.chenfeng@...il.com>,
	<weidong2@...ilicon.com>, <w.f@...wei.com>,
	<qijiwen@...ilicon.com>, <peter.panshilin@...ilicon.com>,
	<dan.zhao@...ilicon.com>, <linuxarm@...wei.com>,
	<liguozhu@...ilicon.com>, <shimingxing@...ilicon.com>,
	<albert.lubing@...ilicon.com>, <oliver.fu@...ilicon.com>,
	<haojian.zhuang@...aro.org>
Subject: Re: [PATCH v4 4/5] regulator: add regulator driver of hi655x pmic



On 2016/1/5 23:44, Mark Brown wrote:
> On Mon, Jan 04, 2016 at 08:27:51PM +0800, Chen Feng wrote:
> 
>> +/*LDO 2 & LDO 14*/
> 
> Please use the normal kernel coding style for comments, I'm surprised
> checkpatch didn't warn you about this.
> 
ok,thanks!
>> +static const unsigned int ldo2_voltages[] = {
>> +	2500000, 2600000, 2700000, 2800000,
>> +	2900000, 3000000, 3100000, 3200000,
> 
> This looks like a linear range from 2.5V in steps of 100mV?  A linear
> range is better than a table since values can be mapped directly without
> having to scan a table.
> 
Agree with you.

>> +/*LDO7 & LDO10*/
>> +static const unsigned int ldo7_voltages[] = {
>> +	1800000, 1850000, 2850000, 2900000,
>> +	3000000, 3100000, 3200000, 3300000,
> 
> This is the sort of thing a voltage table is for where the voltages
> aren't evenly spaced and don't map onto a formula.
> 
>> +static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
>> +	{
>> +		.compatible = "hisilicon,hi655x-regulator",
>> +	},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
> 
> A couple of problems here.  One is that the compatible strings should be
> for specific devices, not use a wildcard.  The other is that since this
> is part of a PMIC and we already have a compatible string for the PMIC
> so this is really just a set of properties for that device rather than a
> totally separate device - we're not achieving any real reuse over
> multiple devices or anything.
ok.
> 
>> +	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
>> +		if (!of_node_cmp(np->name, regulators[i].rdesc.name))
>> +			break;
>> +	}
>> +
>> +	if (i == ARRAY_SIZE(regulators)) {
>> +		dev_err(dev, "error regulator %s int dts\n", np->name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	regulator = &regulators[i];
>> +	init_data = of_get_regulator_init_data(dev, np,
>> +					       &regulator->rdesc);
>> +	if (!init_data)
>> +		return -EINVAL;
> 
> Don't open code this, use the standard support with of_match and
> regulators_node.
> 
>> +	config.dev = dev;
>> +	config.init_data = init_data;
>> +	config.driver_data = regulator;
>> +	config.regmap = pmic->regmap;
>> +	config.of_node = pdev->dev.of_node;
> 
>> +	rdev = devm_regulator_register(dev, &regulator->rdesc,
>> +				       &config);
>> +	if (IS_ERR(rdev))
>> +		return PTR_ERR(rdev);
> 
> Though this looks like it's trying to have a device per regulator which
> is an unusual pattern and if we are doing that then relying on the node
> name to figure out which device this is a bit broken.
> 
ok, I will fix the patch and resend it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/