linux-kernel - Re: [PATCH 2/4] regulator: add STw481x VMMC 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]

Date:	Sun, 1 Sep 2013 21:13:21 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] regulator: add STw481x VMMC driver

On Sun, Sep 01, 2013 at 10:00:59PM +0200, Linus Walleij wrote:

> Hi Mark, I'm seeking an ACK for this driver eventually, to
> take it through the ARM SoC tree with the dependency MFD
> driver and the enablement patches.

I can put it on a branch so it can be pulled into arm-soc - it makes
life easier for framework refactorings to have the driver in the
regulator tree as well.  Would that be OK?

> +static int stw481x_vmmc_enable(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +	int ret;
> +
> +	/* First disable the external VMMC if it's active */
> +	ret = regmap_update_bits(stw481x->map, STW_CONF2,
> +				 STW_CONF2_VMMC_EXT, 0);
> +	if (ret)
> +		return ret;

This is a bit unusual - is there some definite relationship between the
two?

> +static int stw481x_vmmc_disable(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +
> +	return regmap_update_bits(stw481x->map, STW_CONF1,
> +				  STW_CONF1_PDN_VMMC, 0);
> +}

Should use regulator_disable_regmap().

> +static int stw481x_vmmc_is_enabled(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(stw481x->map, STW_CONF1, &val);
> +	if (ret)
> +		return ret;
> +	return !!(val & STW_CONF1_PDN_VMMC);
> +}

Should use regulator_is_enabled_regmap().

> +static int stw481x_vmmc_get_voltage_sel(struct regulator_dev *reg)
> +static int stw481x_vmmc_set_voltage_sel(struct regulator_dev *reg,
> +					unsigned selector)

Should use regulator_get/set_voltage_sel_regmap().

> +static ssize_t show_ctrl1(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct stw481x *stw481x = dev_get_platdata(dev);
> +
> +	ret = regmap_read(stw481x->map, STW_CONF1, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%#x\n", val);
> +};
> +
> +static DEVICE_ATTR(ctrl1, S_IRUGO, show_ctrl1, NULL);

I'd suggest setting max_register (and possibly readable_reg()) in the
regmap config rather than adding specific sysfs files.  Unless there's
some pressing reason for this beyond debug?

> +	stw481x->vmmc_regulator = regulator_register(&vmmc_regulator, &config);
> +	if (IS_ERR(stw481x->vmmc_regulator)) {
> +		dev_err(&pdev->dev,
> +			"error initializing STw481x VMMC regulator\n");
> +		return PTR_ERR(stw481x->vmmc_regulator);
> +	}

I sent patches for a devm_regulator_register() yesterday - it'll be
going into -next after the merge window, though the conversion can
always happen later (I didn't convert most of the drivers yet anyway).

> +static __init int stw481x_vmmc_regulator_init(void)
> +{
> +	return platform_driver_register(&stw481x_vmmc_regulator_driver);
> +}
> +
> +static __exit void stw481x_vmmc_regulator_exit(void)
> +{
> +	platform_driver_unregister(&stw481x_vmmc_regulator_driver);
> +}
> +
> +subsys_initcall(stw481x_vmmc_regulator_init);
> +module_exit(stw481x_vmmc_regulator_exit);

You should be able to use module_platform_driver() with modern systems,
deferred probing should cope with most uses of subsys_initcall().  Not
the end of the world though.

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