Re: [PATCH v2] Add ltc3562 voltage regulator driver

[Mark Brown]

On Thu, Oct 30, 2014 at 12:26:55PM +0100, Mike Looijmans wrote:
> The ltc3562 is an I2C controlled regulator supporting 4 independent
> outputs.
> 
> v2: Prefix "lltc" to devicetree properties. Use the same property names
>     as the ltc3589 driver. Remove default-voltage property. Use
>     devm_register_regulator.

As covered in SubmittingPatches things like this should be after ---.

> +Required properties:
> +- compatible: "ltc3562"

This needs a vendor in the compatible string.

> +Required child node:
> +- regulators: Contains four regulator child nodes R400B, R600B, R400A, R600A,
> +  specifying the initialization data as documented in
> +  Documentation/devicetree/bindings/regulator/regulator.txt.

All regulator child nodes should be optional.

> +			R600A_reg: R600A {
> +				regulator-name = "R600A";

Remove these regulator-names, this is for the name of the supplies on
the board not the regulator itself.

> +static int ltc3562_write(struct i2c_client *i2c, u8 reg_a, u8 reg_b);
> +static int ltc3562_dummy_write(struct i2c_client *i2c);

This appears to be reimplementing regmap (including a cache).  Please
use that instead.  Pretty much the entire driver could then be replaced
with the regmap helpers, none of the operations look like they'd be
needed, and you'd get the regmap diagnostic infrastructure.

> +	np = of_node_get(i2c->dev.of_node);
> +	np_regulators = of_get_child_by_name(np, "regulators");

> +		np_child = of_get_child_by_name(np_regulators,
> +			ltc3562_regulators[i].name);
> +		if (np_child == NULL) {

Use the core support for looking up constraints please - set
regulators_node and so on.

> +static struct i2c_driver ltc3562_i2c_driver = {
> +	.driver = {
> +		.name = "LTC3562",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe    = ltc3562_i2c_probe,
> +	.id_table = ltc3562_i2c_id,
> +};

You need to supply an of_match_table too.

Attachment: signature.asc
Description: Digital signature