Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Krzysztof Kozlowski 

On czw, 2014-11-27 at 18:43 +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Mark Brown 
On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
 On czw, 2014-11-27 at 18:43 +, 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.


signature.asc
Description: Digital signature

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Krzysztof Kozlowski 
On pią, 2014-11-28 at 11:38 +, Mark Brown wrote:
 On Fri, Nov 28, 2014 at 11:30:55AM +0100, Krzysztof Kozlowski wrote:
  On czw, 2014-11-27 at 18:43 +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-28 Thread Mark Brown 
On Fri, Nov 28, 2014 at 03:14:04PM +0100, Krzysztof Kozlowski wrote:
 On pią, 2014-11-28 at 11:38 +, 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.


signature.asc
Description: Digital signature

[PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-27 Thread Krzysztof Kozlowski 
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.kozlow...@samsung.com
---
 drivers/regulator/core.c | 96 ++--
 include/linux/regulator/driver.h |  5 +++
 2 files changed, 78 insertions(+), 23 deletions(-)

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, regulator_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, regulator_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

Re: [PATCH v4 4/7] regulator: Use ena_gpio supplied with generic regulator bindings

2014-11-27 Thread Mark Brown 
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?


signature.asc
Description: Digital signature