Re: [PATCH 2/4] regulator: add STw481x VMMC driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]