linux-kernel - Re: [PATCH v2] regulator: pfuze100: add pfuze100 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: <20130712144037.GJ24508@sirena.org.uk>
Date:	Fri, 12 Jul 2013 15:40:37 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Robin Gong <b38343@...escale.com>
Cc:	linux-kernel@...r.kernel.org, grant.likely@...aro.org,
	rob.herring@...xeda.com, lgirdwood@...il.com, shawn.guo@...aro.org,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	rob@...dley.net
Subject: Re: [PATCH v2] regulator: pfuze100: add pfuze100 regulator driver

On Fri, Jul 12, 2013 at 12:54:15PM +0800, Robin Gong wrote:
> Add pfuze100 regulator driver.

This looks mostly good.  A few small issues below but nothing major.

> +enum pfuze_id {
> +	PFUZE_ID_PFUZE100,
> +	PFUZE_ID_INVALID,
> +};
> +struct pfuze_chip {

Missing blank line here - there are a few other small coding style
things, checkpatch should help.

> +static struct regulator_ops pfuze100_ldo_regulator_ops;
> +static struct regulator_ops pfuze100_fixed_regulator_ops;
> +static struct regulator_ops pfuze100_sw_regulator_ops;
> +static struct regulator_ops pfuze100_swb_regulator_ops;

Better to just reorder things so that no forward declaration is needed.

> +static const int pfuze100_swbst[] = {
> +	5000000, 5050000, 5100000, 5150000,
> +};

This looks like a linear map, the steps are all 50mV?

> +	num_regulators = pfuze_get_num_regulators_dt(&client->dev);
> +	if (num_regulators <= 0 && pdata)
> +		num_regulators = pdata->num_regulators;
> +	if (num_regulators <= 0) {
> +		dev_err(&client->dev, "no platform data,please add it!\n");
> +		ret = -EINVAL;
> +		return ret;

You should just register all the regulators rather than only registering
those that the user explicitly selects.  This allows users to inpect the
current configuration and simplifies the code - for example you don't
need to count the DT nodes and you can just have a simple array in the
platform data (see how wm831x does this for an example).

> +		/* SW2~SW4 high bit check and modify the voltage value table */
> +		if (i > PFUZE100_SW1C && i < PFUZE100_SWBST) {
> +			regmap_read(pfuze_chip->regmap, PFUZE100_SW2VOL +
> +					(i - PFUZE100_SW2) * 7, &val);
> +			if (val & 0x40) {
> +				pfuze100_regulators[id].desc.min_uV = 800000;
> +				pfuze100_regulators[id].desc.uV_step = 50000;
> +			}
> +		}

You should really be doing this on a copy of the regulators table rather
than on the table itself.

> +	for (i = 0; i < pfuze_chip->num_regulators; i++)
> +		regulator_unregister(pfuze_chip->regulators[i]);
> +	kfree(pfuze_chip);

Use devm_kzalloc().

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