Messages in this thread | | | Date | Sun, 1 Sep 2013 21:13:21 +0100 | From | Mark Brown <> | 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. [unhandled content-type:application/pgp-signature] | |