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 = ®ulators[i]; >> + init_data = of_get_regulator_init_data(dev, np, >> + ®ulator->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, ®ulator->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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/