On Sun, Sep 1, 2013 at 10:13 PM, Mark Brown <broo...@kernel.org> wrote: > 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?
Sure but then you need patch 1/4 in there as well. I don't know if that comment pertained to the situation prior to the v3.12 merge window though, can I take this in now maybe or is there still a stir in the regulator tree? >> +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? They are mutally exclusive. You cannot have both at the same time. The datasheet says: 0 Internal LDO VMMC is used 1 External VMMC is used If it is set to 1, the internal VMMC is disconnected. So it's like some internal mux. All designs I've seen use the internal regulator though. I move this to probe() and used regulator_enable_regmap() instead. > Should use regulator_disable_regmap(). > Should use regulator_is_enabled_regmap(). > Should use regulator_get/set_voltage_sel_regmap(). OK OK OK :-) All too new stuff to me, the helpers are really convenient! I only just rewrote the driver to use regmap and saved some lines, with this I save even more, cool. >> +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? This should've been just debug, and it's way simpler to do using just regmap. Deleted. >> + 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). Hm nice, if we can merge the MFD patch into the regulator tree I can rebase this on you branch for next and use that. >> +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, Yeah ... and module_init() level works just fine this is just some kind of artifact. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/