linux-kernel - Re: [PATCH 3/4] regulator: as3722: add regulator driver for AMS AS3722

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: <20130917114133.GV21013@sirena.org.uk>
Date:	Tue, 17 Sep 2013 12:41:33 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	lee.jones@...aro.org, sameo@...ux.intel.com,
	linus.walleij@...aro.org, akpm@...ux-foundation.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	rtc-linux@...glegroups.com, rob.herring@...xeda.com,
	mark.rutland@....com, pawel.moll@....com, swarren@...dotorg.org,
	rob@...dley.net, ijc+devicetree@...lion.org.uk,
	grant.likely@...aro.org, florian.lobmaier@....com
Subject: Re: [PATCH 3/4] regulator: as3722: add regulator driver for AMS
 AS3722

On Tue, Sep 17, 2013 at 12:15:37PM +0530, Laxman Dewangan wrote:

> +               ams,enable-oc-config: Enable overcurrent configuration of the
> +               ams,oc-trip-threshold-perphase: Overcurrent trip threshold
> +               ams,oc-alarm-threshold-perphase: Overcurrent alarm threshold

These look like you should be implementing the current setting
operations in the framework, probably mapping to the alarm limit if
nothing else.

> +static int as3722_ldo_get_voltage_sel(struct regulator_dev *rdev)
> +{
> +	int ret;
> +
> +	ret = regulator_get_voltage_sel_regmap(rdev);
> +	if (ret >= 0x40)
> +		ret -= 0x1B;
> +	return ret;
> +}

This looks very strange.  What's going on here?  A gap in the selectors?
If that's the case you probably want to be using linear ranges.

> +static struct regulator_ops as3722_ldo_extcntrl_ops = {
> +	.get_voltage_sel = as3722_ldo_get_voltage_sel,
> +	.set_voltage_sel = as3722_ldo_set_voltage_sel,
> +	.list_voltage = regulator_list_voltage_linear,
> +};

You ought to be providing map_voltage() too, same for some if not all of
the other regulators.

> +static unsigned int as3722_sd_get_mode(struct regulator_dev *dev)
> +{
> +	struct as3722_regulators *as3722_regs = rdev_get_drvdata(dev);
> +	struct as3722 *as3722 = as3722_regs->as3722;
> +	int id = rdev_get_id(dev);
> +	u32 val;
> +	int ret;
> +
> +	if (!as3722_reg_lookup[id].control_reg)
> +		return -ERANGE;

That seems a bit of a random error code.  -ENOTSUPP?

> +static int as3722_sd_list_voltage(struct regulator_dev *rdev, unsigned selector)
> +{
> +	if (selector >= AS3722_SD2_VSEL_MAX)
> +		return -EINVAL;
> +
> +	selector++;
> +	if (selector <= 0x40)
> +		return 600000 + selector * 12500;
> +	if (selector <= 0x70)
> +		return 1400000 + (selector - 0x40) * 25000;
> +	if (selector <= 0x7F)
> +		return 2600000 + (selector - 0x70) * 50000;
> +	return -EINVAL;
> +}

Use linear ranges.

> +static struct of_regulator_match as3722_regulator_matches[] = {
> +	{.name = "sd0", },

	{ .name = "sd0", },

> +		config.of_node = as3722_regulator_matches[id].of_node;
> +		rdev = regulator_register(&as3722_regs->desc[id], &config);
> +		if (IS_ERR(rdev)) {

devm_regualtor_register().

> +		if (reg_config->ext_control) {
> +			ret = regulator_enable_regmap(rdev);
> +			if (ret < 0) {
> +				dev_err(&pdev->dev,
> +					"Regulator %d enable failed: %d\n",
> +					id, ret);
> +				goto scrub;
> +			}

This looks wrong...  why is the regualtor being enabled by something
other than the core?

> +static int __init as3722_regulator_init(void)
> +{
> +	return platform_driver_register(&as3722_regulator_driver);
> +}
> +subsys_initcall(as3722_regulator_init);
> +
> +static void __exit as3722_regulator_exit(void)
> +{
> +	platform_driver_unregister(&as3722_regulator_driver);
> +}
> +module_exit(as3722_regulator_exit);

module_platform_driver().

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