| Message ID | 1417087253-12306-5-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote: > Use ena_gpio from regulator constraints (filled by parsing generic > bindings) to initialize the GPIO enable control. Support also the old > way: ena_gpio supplied in regulator_config structure. > > This also adds a new set_ena_gpio() callback in regulator_ops structure > which driver may provide to actually enable the GPIO control in > hardware. This seems really confused like it's trying to work around some other problem - this all feels like it's at the wrong abstraction level. As far as I can tell this is trying to fix bugs in the previous patch and do some other refactorings (the "also add this other random op" bit especially) but I'm really not clear what the goal is. Please try to think if the code you're writing makes sense at the big picture level rather than just band aiding specific problems you see. It's also a good idea to keep random code motion separate from functional changes since it makes it much easier to follow what each is supposed to do. > @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev, > } > } > > + if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) { > + ret = ops->set_ena_gpio(rdev); > + if (ret < 0) { > + rdev_err(rdev, "failed to set enable GPIO control\n"); > + goto out; > + } > + } Why do we need some special magic operation for GPIO based enables that's separate to any other enable operation? This seems really confusing, if the constraint setting doesn't work somehow for GPIO based enables we should fix that. Though since this operation takes no parameters it's hard to see how it's supposed to apply constraints unless it reparses them which doesn't seem like a good idea... > static int regulator_ena_gpio_request(struct regulator_dev *rdev, > - const struct regulator_config *config) > - ret = gpio_request_one(config->ena_gpio, > - GPIOF_DIR_OUT | config->ena_gpio_flags, > + ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags, > rdev_get_name(rdev)); > +/* > + * Request GPIO for enable control from regulator_config > + * or init_data->constraints. > + */ > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev, > + const struct regulator_config *config, > + const struct regulator_init_data *init_data) Why is setting up the GPIO different to requesting it, especially given that we have an existing function called _request() which still exists?
On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote: > On Thu, Nov 27, 2014 at 12:20:50PM +0100, Krzysztof Kozlowski wrote: > > Use ena_gpio from regulator constraints (filled by parsing generic > > bindings) to initialize the GPIO enable control. Support also the old > > way: ena_gpio supplied in regulator_config structure. > > > > This also adds a new set_ena_gpio() callback in regulator_ops structure > > which driver may provide to actually enable the GPIO control in > > hardware. > > This seems really confused like it's trying to work around some other > problem - this all feels like it's at the wrong abstraction level. As > far as I can tell this is trying to fix bugs in the previous patch and > do some other refactorings (the "also add this other random op" bit > especially) but I'm really not clear what the goal is. > > Please try to think if the code you're writing makes sense at the big > picture level rather than just band aiding specific problems you see. > It's also a good idea to keep random code motion separate from > functional changes since it makes it much easier to follow what each is > supposed to do. > > > @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev, > > } > > } > > > > + if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) { > > + ret = ops->set_ena_gpio(rdev); > > + if (ret < 0) { > > + rdev_err(rdev, "failed to set enable GPIO control\n"); > > + goto out; > > + } > > + } > > Why do we need some special magic operation for GPIO based enables > that's separate to any other enable operation? This seems really > confusing, if the constraint setting doesn't work somehow for GPIO based > enables we should fix that. Though since this operation takes no > parameters it's hard to see how it's supposed to apply constraints > unless it reparses them which doesn't seem like a good idea... The regulator driver no longer parses GPIO control from DTS. So somehow it should be notified that regulator core parsed this and GPIO should be enabled. That is the purpose of ops->set_ena_gpio() call. > > > static int regulator_ena_gpio_request(struct regulator_dev *rdev, > > - const struct regulator_config *config) > > > - ret = gpio_request_one(config->ena_gpio, > > - GPIOF_DIR_OUT | config->ena_gpio_flags, > > + ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags, > > rdev_get_name(rdev)); > > > +/* > > + * Request GPIO for enable control from regulator_config > > + * or init_data->constraints. > > + */ > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev, > > + const struct regulator_config *config, > > + const struct regulator_init_data *init_data) > > Why is setting up the GPIO different to requesting it, especially given > that we have an existing function called _request() which still exists? Maybe the name was not a best choice. The setup calls request. My patchset here tried to retain the compatibility with "config.ena_gpio" way so the core would accept GPIOs passed in one of two ways: 1. old: config.ena_gpio, 2. new: parsed by core from DTS. The request function previously worked only on "config.ena_gpio" and I changed it here to accept any GPIO. The setup uses one of GPIO methods (old or new) and calls request with appropriate GPIO. Anyway this will change after your comments about not using constraints (patch 3/7). I'll keep your comments about big picture level in mind and start working on next version. Thanks for feedback! Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote: > > Why do we need some special magic operation for GPIO based enables > > that's separate to any other enable operation? This seems really > > confusing, if the constraint setting doesn't work somehow for GPIO based > > enables we should fix that. Though since this operation takes no > > parameters it's hard to see how it's supposed to apply constraints > > unless it reparses them which doesn't seem like a good idea... > The regulator driver no longer parses GPIO control from DTS. So somehow > it should be notified that regulator core parsed this and GPIO should be > enabled. > That is the purpose of ops->set_ena_gpio() call. This sort of thing is a sign that we're not saving much by moving the parsing to the core and perhaps there's more flexiblity here... It's also not something that should be in the constraints handling, it's not something that constrains the regulator. > > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev, > > > + const struct regulator_config *config, > > > + const struct regulator_init_data *init_data) > > Why is setting up the GPIO different to requesting it, especially given > > that we have an existing function called _request() which still exists? > Maybe the name was not a best choice. The setup calls request. > My patchset here tried to retain the compatibility with > "config.ena_gpio" way so the core would accept GPIOs passed in one of > two ways: > 1. old: config.ena_gpio, > 2. new: parsed by core from DTS. > The request function previously worked only on "config.ena_gpio" and I > changed it here to accept any GPIO. The setup uses one of GPIO methods > (old or new) and calls request with appropriate GPIO. We need to support both methods since not all the world is DT. What I can't tell is how this code achieves these objectives - it seems to be an awfully big patch if that's all it's supposed to be doing, I'd expect just a conditional where we try the two methods in turn. It may be that there's a good reason for all this but that needs to be made clear.
On pi?, 2014-11-28 at 11:38 +0000, Mark Brown wrote: > On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > > On czw, 2014-11-27 at 18:43 +0000, Mark Brown wrote: > > > > Why do we need some special magic operation for GPIO based enables > > > that's separate to any other enable operation? This seems really > > > confusing, if the constraint setting doesn't work somehow for GPIO based > > > enables we should fix that. Though since this operation takes no > > > parameters it's hard to see how it's supposed to apply constraints > > > unless it reparses them which doesn't seem like a good idea... > > > The regulator driver no longer parses GPIO control from DTS. So somehow > > it should be notified that regulator core parsed this and GPIO should be > > enabled. > > > That is the purpose of ops->set_ena_gpio() call. > > This sort of thing is a sign that we're not saving much by moving the > parsing to the core and perhaps there's more flexiblity here... The driver receive callbacks (or exposes other kind of interface) for other core-generalized code. Recent example is parsing regulator mode (added by Javier) and .of_map_mode() callback. I thought how to do this without this additional set_ena_gpio() call. One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and GPIO) but this would look somehow unnatural. > It's > also not something that should be in the constraints handling, it's not > something that constrains the regulator. Got it. > > > > +static int regulator_ena_gpio_setup(struct regulator_dev *rdev, > > > > + const struct regulator_config *config, > > > > + const struct regulator_init_data *init_data) > > > > Why is setting up the GPIO different to requesting it, especially given > > > that we have an existing function called _request() which still exists? > > > Maybe the name was not a best choice. The setup calls request. > > > My patchset here tried to retain the compatibility with > > "config.ena_gpio" way so the core would accept GPIOs passed in one of > > two ways: > > 1. old: config.ena_gpio, > > 2. new: parsed by core from DTS. > > > The request function previously worked only on "config.ena_gpio" and I > > changed it here to accept any GPIO. The setup uses one of GPIO methods > > (old or new) and calls request with appropriate GPIO. > > We need to support both methods since not all the world is DT. What I > can't tell is how this code achieves these objectives - it seems to be > an awfully big patch if that's all it's supposed to be doing, I'd expect > just a conditional where we try the two methods in turn. It may be that > there's a good reason for all this but that needs to be made clear. OK, I'll try to better describe the reason behind and to split the patches (if possible). Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote: > On pi?, 2014-11-28 at 11:38 +0000, Mark Brown wrote: > > This sort of thing is a sign that we're not saving much by moving the > > parsing to the core and perhaps there's more flexiblity here... > The driver receive callbacks (or exposes other kind of interface) for > other core-generalized code. Recent example is parsing regulator mode > (added by Javier) and .of_map_mode() callback. Right, but that's actually doing some device specific translation and successfully factoring out the bulk of the code - the fact that it's taking parameters and returning data is a good sign. This is a callback placed randomly away from any other related code (adding to the confusion - it's not integrated into the rest of the flow around this at all) without a clear purpose. > I thought how to do this without this additional set_ena_gpio() call. > One way would be to extend the regulator modes (FAST/IDLE/STANDBY/ and > GPIO) but this would look somehow unnatural. Yes, that's absolutely hideous.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index bbf93c9caca3..1760184cd8dc 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -26,6 +26,7 @@ #include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/regmap.h> #include <linux/regulator/of_regulator.h> #include <linux/regulator/consumer.h> @@ -1044,6 +1045,14 @@ static int set_machine_constraints(struct regulator_dev *rdev, } } + if (rdev->constraints->use_ena_gpio && ops->set_ena_gpio) { + ret = ops->set_ena_gpio(rdev); + if (ret < 0) { + rdev_err(rdev, "failed to set enable GPIO control\n"); + goto out; + } + } + print_constraints(rdev); return 0; out: @@ -1660,36 +1669,36 @@ EXPORT_SYMBOL_GPL(regulator_bulk_unregister_supply_alias); /* Manage enable GPIO list. Same GPIO pin can be shared among regulators */ static int regulator_ena_gpio_request(struct regulator_dev *rdev, - const struct regulator_config *config) + unsigned int gpio, + bool gpio_invert, + unsigned int gpio_flags) { struct regulator_enable_gpio *pin; struct gpio_desc *gpiod; int ret; - gpiod = gpio_to_desc(config->ena_gpio); + gpiod = gpio_to_desc(gpio); list_for_each_entry(pin, ®ulator_ena_gpio_list, list) { if (pin->gpiod == gpiod) { - rdev_dbg(rdev, "GPIO %d is already used\n", - config->ena_gpio); + rdev_dbg(rdev, "GPIO %d is already used\n", gpio); goto update_ena_gpio_to_rdev; } } - ret = gpio_request_one(config->ena_gpio, - GPIOF_DIR_OUT | config->ena_gpio_flags, + ret = gpio_request_one(gpio, GPIOF_DIR_OUT | gpio_flags, rdev_get_name(rdev)); if (ret) return ret; pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL); if (pin == NULL) { - gpio_free(config->ena_gpio); + gpio_free(gpio); return -ENOMEM; } pin->gpiod = gpiod; - pin->ena_gpio_invert = config->ena_gpio_invert; + pin->ena_gpio_invert = gpio_invert; list_add(&pin->list, ®ulator_ena_gpio_list); update_ena_gpio_to_rdev: @@ -1698,6 +1707,59 @@ update_ena_gpio_to_rdev: return 0; } +/* + * Request GPIO for enable control from regulator_config + * or init_data->constraints. + */ +static int regulator_ena_gpio_setup(struct regulator_dev *rdev, + const struct regulator_config *config, + const struct regulator_init_data *init_data) +{ + unsigned int gpio_flags; + bool gpio_invert; + int gpio, ret; + + if (config->ena_gpio || config->ena_gpio_initialized) { + gpio = config->ena_gpio; + gpio_invert = config->ena_gpio_invert; + gpio_flags = config->ena_gpio_flags; + } else if (init_data && init_data->constraints.use_ena_gpio) { + const struct regulation_constraints *c = &init_data->constraints; + + gpio = c->ena_gpio; + gpio_invert = false; + gpio_flags = GPIOF_OUT_INIT_HIGH; + + if (c->ena_gpio_flags & OF_GPIO_ACTIVE_LOW) { + gpio_invert = true; + gpio_flags = GPIOF_OUT_INIT_LOW; + } + + if (c->ena_gpio_open_drain) + gpio_flags |= GPIOF_OPEN_DRAIN; + } else { + return 0; + } + + if (!gpio_is_valid(gpio)) + return 0; + + ret = regulator_ena_gpio_request(rdev, gpio, gpio_invert, gpio_flags); + if (ret != 0) { + rdev_err(rdev, "Failed to request enable GPIO%d: %d\n", + gpio, ret); + return ret; + } + + if (gpio_flags & GPIOF_OUT_INIT_HIGH) + rdev->ena_gpio_state = 1; + + if (gpio_invert) + rdev->ena_gpio_state = !rdev->ena_gpio_state; + + return 0; +} + static void regulator_ena_gpio_free(struct regulator_dev *rdev) { struct regulator_enable_gpio *pin, *n; @@ -3650,21 +3712,9 @@ regulator_register(const struct regulator_desc *regulator_desc, dev_set_drvdata(&rdev->dev, rdev); - if ((config->ena_gpio || config->ena_gpio_initialized) && - gpio_is_valid(config->ena_gpio)) { - ret = regulator_ena_gpio_request(rdev, config); - if (ret != 0) { - rdev_err(rdev, "Failed to request enable GPIO%d: %d\n", - config->ena_gpio, ret); - goto wash; - } - - if (config->ena_gpio_flags & GPIOF_OUT_INIT_HIGH) - rdev->ena_gpio_state = 1; - - if (config->ena_gpio_invert) - rdev->ena_gpio_state = !rdev->ena_gpio_state; - } + ret = regulator_ena_gpio_setup(rdev, config, init_data); + if (ret != 0) + goto wash; /* set regulator constraints */ if (init_data) diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 28da08e4671f..fe967a0c6ce7 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -118,6 +118,9 @@ struct regulator_linear_range { * suspended. * @set_suspend_mode: Set the operating mode for the regulator when the * system is suspended. + * @set_ena_gpio: Turn on GPIO enable control for given regulator. Called + * by core during registration of regulator when regulator + * was configured for this mode by standard binding. * * This struct describes regulator operations which can be implemented by * regulator chip drivers. @@ -183,6 +186,8 @@ struct regulator_ops { /* set regulator suspend operating mode (defined in consumer.h) */ int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode); + + int (*set_ena_gpio)(struct regulator_dev *); }; /*
Use ena_gpio from regulator constraints (filled by parsing generic bindings) to initialize the GPIO enable control. Support also the old way: ena_gpio supplied in regulator_config structure. This also adds a new set_ena_gpio() callback in regulator_ops structure which driver may provide to actually enable the GPIO control in hardware. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/regulator/core.c | 96 ++++++++++++++++++++++++++++++---------- include/linux/regulator/driver.h | 5 +++ 2 files changed, 78 insertions(+), 23 deletions(-)