Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301632

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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.

> +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.

> +/*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.

> +	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.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v4 0/5] Add Support for Hi6220 PMIC Hi6553 MFD Core Chen Feng <puck.chen@hisilicon.com> - 2016-01-04 13:30 +0100
  [PATCH v4 2/5] doc: bindings: Document for hi655x regulator driver Chen Feng <puck.chen@hisilicon.com> - 2016-01-04 13:40 +0100
    Re: [PATCH v4 2/5] doc: bindings: Document for hi655x regulator  driver Mark Brown <broonie@kernel.org> - 2016-01-05 16:50 +0100
      Re: [PATCH v4 2/5] doc: bindings: Document for hi655x regulator  driver chenfeng <puck.chen@hisilicon.com> - 2016-01-07 04:40 +0100
  [PATCH v4 5/5] hisilicon/dts: Add hi655x pmic dts node Chen Feng <puck.chen@hisilicon.com> - 2016-01-04 13:40 +0100
  [PATCH v4 3/5] mfd: hi655x: Add hi665x pmic driver Chen Feng <puck.chen@hisilicon.com> - 2016-01-04 13:40 +0100
    Re: [PATCH v4 3/5] mfd: hi655x: Add hi665x pmic driver Lee Jones <lee.jones@linaro.org> - 2016-01-11 07:00 +0100
  [PATCH v4 4/5] regulator: add regulator driver of hi655x pmic Chen Feng <puck.chen@hisilicon.com> - 2016-01-04 13:40 +0100
    Re: [PATCH v4 4/5] regulator: add regulator driver of hi655x pmic Mark Brown <broonie@kernel.org> - 2016-01-05 16:50 +0100
      Re: [PATCH v4 4/5] regulator: add regulator driver of hi655x pmic chenfeng <puck.chen@hisilicon.com> - 2016-01-07 04:40 +0100

csiph-web