* [PATCH 1/2] regulators: max8973: fix multiple instance support @ 2013-06-21 6:30 Guennadi Liakhovetski 2013-06-21 6:30 ` [PATCH 2/2] regulators: max8973: initial DT support Guennadi Liakhovetski 2013-06-21 10:01 ` [PATCH 1/2] regulators: max8973: fix multiple instance support Mark Brown 0 siblings, 2 replies; 6+ messages in thread From: Guennadi Liakhovetski @ 2013-06-21 6:30 UTC (permalink / raw) To: linux-kernel; +Cc: Mark Brown, Liam Girdwood, Magnus Damm, linux-sh Currently the max8973 regulator driver uses a single static struct of regulator operations for all chip instances, but can overwrite some of its members depending on configuration. This will affect all other MAX8973 instances on the system. This patch fixes this bug by allocating a separate copy of the struct for each chip instance. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> --- drivers/regulator/max8973-regulator.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c index adb1414..b2dbdd7 100644 --- a/drivers/regulator/max8973-regulator.c +++ b/drivers/regulator/max8973-regulator.c @@ -100,6 +100,7 @@ struct max8973_chip { int curr_vout_reg; int curr_gpio_val; bool valid_dvs_gpio; + struct regulator_ops ops; }; /* @@ -240,7 +241,7 @@ static unsigned int max8973_dcdc_get_mode(struct regulator_dev *rdev) REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL; } -static struct regulator_ops max8973_dcdc_ops = { +static const struct regulator_ops max8973_dcdc_ops = { .get_voltage_sel = max8973_dcdc_get_voltage_sel, .set_voltage_sel = max8973_dcdc_set_voltage_sel, .list_voltage = regulator_list_voltage_linear, @@ -388,10 +389,11 @@ static int max8973_probe(struct i2c_client *client, } i2c_set_clientdata(client, max); + max->ops = max8973_dcdc_ops; max->dev = &client->dev; max->desc.name = id->name; max->desc.id = 0; - max->desc.ops = &max8973_dcdc_ops; + max->desc.ops = &max->ops; max->desc.type = REGULATOR_VOLTAGE; max->desc.owner = THIS_MODULE; max->desc.min_uV = MAX8973_MIN_VOLATGE; @@ -401,9 +403,9 @@ static int max8973_probe(struct i2c_client *client, if (!pdata->enable_ext_control) { max->desc.enable_reg = MAX8973_VOUT; max->desc.enable_mask = MAX8973_VOUT_ENABLE; - max8973_dcdc_ops.enable = regulator_enable_regmap; - max8973_dcdc_ops.disable = regulator_disable_regmap; - max8973_dcdc_ops.is_enabled = regulator_is_enabled_regmap; + max->ops.enable = regulator_enable_regmap; + max->ops.disable = regulator_disable_regmap; + max->ops.is_enabled = regulator_is_enabled_regmap; } max->enable_external_control = pdata->enable_ext_control; -- 1.7.2.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] regulators: max8973: initial DT support 2013-06-21 6:30 [PATCH 1/2] regulators: max8973: fix multiple instance support Guennadi Liakhovetski @ 2013-06-21 6:30 ` Guennadi Liakhovetski 2013-06-21 14:44 ` Mark Brown 2013-06-21 10:01 ` [PATCH 1/2] regulators: max8973: fix multiple instance support Mark Brown 1 sibling, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2013-06-21 6:30 UTC (permalink / raw) To: linux-kernel; +Cc: Mark Brown, Liam Girdwood, Magnus Damm, linux-sh This patch adds primitive DT support to the max8973 regulator driver. None of the configuration parameters, supported in the platform data are yet available in DT, therefore no configuration is performed if booting with no platform data. This means, that DT instantiation can only be used on boards, where no run-time configuration of the chip is required. In such cases the driver can be used to scale its output voltage. In the future support for configuration parameters should be added. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> --- .../bindings/regulator/max8973-regulator.txt | 25 +++++++ drivers/regulator/max8973-regulator.c | 73 ++++++++++++++++---- 2 files changed, 85 insertions(+), 13 deletions(-) create mode 100644 Documentation/devicetree/bindings/regulator/max8973-regulator.txt diff --git a/Documentation/devicetree/bindings/regulator/max8973-regulator.txt b/Documentation/devicetree/bindings/regulator/max8973-regulator.txt new file mode 100644 index 0000000..c1629f3 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/max8973-regulator.txt @@ -0,0 +1,25 @@ +* Maxim MAX8973 Voltage Regulator + +Required properties: + +- compatible: must be "maxium,max8973" +- reg: the i2c slave address of the regulator. It should be 0x1b. +- regulators: a subnode with a single regulator descriptor in it called "dcdc" + +The single dcdc regulator can use any standard regulator properties. + +Example: + + max8973@1b { + compatible = "maxium,max8973"; + reg = <0x1b>; + + regulators { + vdd_dvfs: dcdc { + regulator-min-microvolt = <935000>; + regulator-max-microvolt = <1200000>; + regulator-boot-on; + regulator-always-on; + }; + }; + }; diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c index b2dbdd7..ecfdea7 100644 --- a/drivers/regulator/max8973-regulator.c +++ b/drivers/regulator/max8973-regulator.c @@ -26,10 +26,12 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/err.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/regulator/driver.h> #include <linux/regulator/machine.h> #include <linux/regulator/max8973-regulator.h> +#include <linux/regulator/of_regulator.h> #include <linux/gpio.h> #include <linux/i2c.h> #include <linux/slab.h> @@ -353,6 +355,10 @@ static int max8973_init_dcdc(struct max8973_chip *max, return ret; } +static struct of_regulator_match max8973_regulator_match = { + .name = "dcdc", +}; + static const struct regmap_config max8973_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -360,6 +366,32 @@ static const struct regmap_config max8973_regmap_config = { .cache_type = REGCACHE_RBTREE, }; +static int max8973_regulator_parse_dt(struct device *dev) +{ + struct device_node *regulators = + of_find_node_by_name(dev->of_node, "regulators"); + int ret; + + if (!regulators) { + dev_err(dev, "regulator node not found\n"); + return -ENODEV; + } + + ret = of_regulator_match(dev, regulators, + &max8973_regulator_match, 1); + of_node_put(regulators); + if (ret < 0) { + dev_err(dev, "Error parsing regulator init data: %d\n", ret); + return ret; + } + if (!ret) { + dev_err(dev, "No regulator configuration found\n"); + return -ENODEV; + } + + return 0; +} + static int max8973_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -370,7 +402,12 @@ static int max8973_probe(struct i2c_client *client, int ret; pdata = client->dev.platform_data; - if (!pdata) { + + if (client->dev.of_node) { + ret = max8973_regulator_parse_dt(&client->dev); + if (ret < 0) + return ret; + } else if (!pdata) { dev_err(&client->dev, "No Platform data"); return -EIO; } @@ -400,7 +437,7 @@ static int max8973_probe(struct i2c_client *client, max->desc.uV_step = MAX8973_VOLATGE_STEP; max->desc.n_voltages = MAX8973_BUCK_N_VOLTAGE; - if (!pdata->enable_ext_control) { + if (!pdata || !pdata->enable_ext_control) { max->desc.enable_reg = MAX8973_VOUT; max->desc.enable_mask = MAX8973_VOUT_ENABLE; max->ops.enable = regulator_enable_regmap; @@ -408,12 +445,17 @@ static int max8973_probe(struct i2c_client *client, max->ops.is_enabled = regulator_is_enabled_regmap; } - max->enable_external_control = pdata->enable_ext_control; - max->dvs_gpio = pdata->dvs_gpio; - max->curr_gpio_val = pdata->dvs_def_state; - max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state; + if (pdata) { + max->dvs_gpio = pdata->dvs_gpio; + max->enable_external_control = pdata->enable_ext_control; + max->curr_gpio_val = pdata->dvs_def_state; + max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state; + } else { + max->dvs_gpio = -EINVAL; + max->curr_vout_reg = MAX8973_VOUT; + } + max->lru_index[0] = max->curr_vout_reg; - max->valid_dvs_gpio = false; if (gpio_is_valid(max->dvs_gpio)) { int gpio_flags; @@ -439,18 +481,23 @@ static int max8973_probe(struct i2c_client *client, max->lru_index[i] = i; max->lru_index[0] = max->curr_vout_reg; max->lru_index[max->curr_vout_reg] = 0; + } else { + max->valid_dvs_gpio = false; } - ret = max8973_init_dcdc(max, pdata); - if (ret < 0) { - dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret); - return ret; + if (pdata) { + ret = max8973_init_dcdc(max, pdata); + if (ret < 0) { + dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret); + return ret; + } } config.dev = &client->dev; - config.init_data = pdata->reg_init_data; + config.init_data = pdata ? pdata->reg_init_data : + max8973_regulator_match.init_data; config.driver_data = max; - config.of_node = client->dev.of_node; + config.of_node = max8973_regulator_match.of_node; config.regmap = max->regmap; /* Register the regulators */ -- 1.7.2.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] regulators: max8973: initial DT support 2013-06-21 6:30 ` [PATCH 2/2] regulators: max8973: initial DT support Guennadi Liakhovetski @ 2013-06-21 14:44 ` Mark Brown 2013-06-21 14:52 ` Guennadi Liakhovetski 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2013-06-21 14:44 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-kernel, Liam Girdwood, Magnus Damm, linux-sh [-- Attachment #1: Type: text/plain, Size: 1506 bytes --] On Fri, Jun 21, 2013 at 08:30:26AM +0200, Guennadi Liakhovetski wrote: > +Required properties: > + > +- compatible: must be "maxium,max8973" > +- reg: the i2c slave address of the regulator. It should be 0x1b. > +- regulators: a subnode with a single regulator descriptor in it called "dcdc" Why make this a subnode - if there's only one regulator on the device then it may as well just put all the regulator properties there? > + if (!regulators) { > + dev_err(dev, "regulator node not found\n"); > + return -ENODEV; > + } > + > + ret = of_regulator_match(dev, regulators, > + &max8973_regulator_match, 1); > + of_node_put(regulators); > + if (ret < 0) { > + dev_err(dev, "Error parsing regulator init data: %d\n", ret); > + return ret; > + } > + if (!ret) { > + dev_err(dev, "No regulator configuration found\n"); > + return -ENODEV; > + } > + > + return 0; This would simplify the code here, the driver can just call of_get_regulator_init_data() directly with the node. > - if (!pdata->enable_ext_control) { > + if (!pdata || !pdata->enable_ext_control) { > max->desc.enable_reg = MAX8973_VOUT; > max->desc.enable_mask = MAX8973_VOUT_ENABLE; > max->ops.enable = regulator_enable_regmap; A common approach here is just to embed the platform data in the driver data then copy actual platform data in there or parse the device tree bindings (when added) into the structure. This means that most of the driver can just assume there's platform data which makes life a bit simpler. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] regulators: max8973: initial DT support 2013-06-21 14:44 ` Mark Brown @ 2013-06-21 14:52 ` Guennadi Liakhovetski 2013-06-25 10:35 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Guennadi Liakhovetski @ 2013-06-21 14:52 UTC (permalink / raw) To: Mark Brown; +Cc: linux-kernel, Liam Girdwood, Magnus Damm, linux-sh Hi Mark Thanks for the review On Fri, 21 Jun 2013, Mark Brown wrote: > On Fri, Jun 21, 2013 at 08:30:26AM +0200, Guennadi Liakhovetski wrote: > > > +Required properties: > > + > > +- compatible: must be "maxium,max8973" > > +- reg: the i2c slave address of the regulator. It should be 0x1b. > > +- regulators: a subnode with a single regulator descriptor in it called "dcdc" > > Why make this a subnode - if there's only one regulator on the device > then it may as well just put all the regulator properties there? Right, I wasn't sure about this, I thought it was kind of a common practice even for just one regulator. Will embed, sure. > > > + if (!regulators) { > > + dev_err(dev, "regulator node not found\n"); > > + return -ENODEV; > > + } > > + > > + ret = of_regulator_match(dev, regulators, > > + &max8973_regulator_match, 1); > > + of_node_put(regulators); > > + if (ret < 0) { > > + dev_err(dev, "Error parsing regulator init data: %d\n", ret); > > + return ret; > > + } > > + if (!ret) { > > + dev_err(dev, "No regulator configuration found\n"); > > + return -ENODEV; > > + } > > + > > + return 0; > > This would simplify the code here, the driver can just call > of_get_regulator_init_data() directly with the node. Ok, I'll look at it. > > - if (!pdata->enable_ext_control) { > > + if (!pdata || !pdata->enable_ext_control) { > > max->desc.enable_reg = MAX8973_VOUT; > > max->desc.enable_mask = MAX8973_VOUT_ENABLE; > > max->ops.enable = regulator_enable_regmap; > > A common approach here is just to embed the platform data in the > driver data then copy actual platform data in there or parse the device > tree bindings (when added) into the structure. This means that most of > the driver can just assume there's platform data which makes life a bit > simpler. But we can do this later, when we add DT support for those parameters, right? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] regulators: max8973: initial DT support 2013-06-21 14:52 ` Guennadi Liakhovetski @ 2013-06-25 10:35 ` Mark Brown 0 siblings, 0 replies; 6+ messages in thread From: Mark Brown @ 2013-06-25 10:35 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-kernel, Liam Girdwood, Magnus Damm, linux-sh [-- Attachment #1: Type: text/plain, Size: 631 bytes --] On Fri, Jun 21, 2013 at 04:52:28PM +0200, Guennadi Liakhovetski wrote: > On Fri, 21 Jun 2013, Mark Brown wrote: > > A common approach here is just to embed the platform data in the > > driver data then copy actual platform data in there or parse the device > > tree bindings (when added) into the structure. This means that most of > > the driver can just assume there's platform data which makes life a bit > > simpler. > But we can do this later, when we add DT support for those parameters, > right? Sure, but it does make sense to structure the code that way from the start since it provides simplification. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] regulators: max8973: fix multiple instance support 2013-06-21 6:30 [PATCH 1/2] regulators: max8973: fix multiple instance support Guennadi Liakhovetski 2013-06-21 6:30 ` [PATCH 2/2] regulators: max8973: initial DT support Guennadi Liakhovetski @ 2013-06-21 10:01 ` Mark Brown 1 sibling, 0 replies; 6+ messages in thread From: Mark Brown @ 2013-06-21 10:01 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-kernel, Liam Girdwood, Magnus Damm, linux-sh [-- Attachment #1: Type: text/plain, Size: 434 bytes --] On Fri, Jun 21, 2013 at 08:30:22AM +0200, Guennadi Liakhovetski wrote: > Currently the max8973 regulator driver uses a single static struct of > regulator operations for all chip instances, but can overwrite some of its > members depending on configuration. This will affect all other MAX8973 > instances on the system. This patch fixes this bug by allocating a separate > copy of the struct for each chip instance. Applied, thanks. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-25 10:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-21 6:30 [PATCH 1/2] regulators: max8973: fix multiple instance support Guennadi Liakhovetski 2013-06-21 6:30 ` [PATCH 2/2] regulators: max8973: initial DT support Guennadi Liakhovetski 2013-06-21 14:44 ` Mark Brown 2013-06-21 14:52 ` Guennadi Liakhovetski 2013-06-25 10:35 ` Mark Brown 2013-06-21 10:01 ` [PATCH 1/2] regulators: max8973: fix multiple instance support Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®