lkml.org 
[lkml]   [2013]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-09-01 22:41    [W:0.088 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site