linux-kernel - Re: [PATCH v2] Add ltc3562 voltage regulator 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141030165133.GB18557@sirena.org.uk>
Date:	Thu, 30 Oct 2014 16:51:33 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Mike Looijmans <mike.looijmans@...ic.nl>
Cc:	lgirdwood@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Add ltc3562 voltage regulator driver

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.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)